Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Topic-based authorisation for publishes #505

Closed
michaelklishin opened this issue Dec 17, 2015 · 21 comments
Closed

Topic-based authorisation for publishes #505

michaelklishin opened this issue Dec 17, 2015 · 21 comments

Comments

@michaelklishin
Copy link
Member

michaelklishin commented Dec 17, 2015

The basic idea is to introduce an authorisation mechanism that'd be centered around topics, as opposed to exchanges/bindings/queues, which don't always fit well protocols such as MQTT and STOMP (and even certain workloads that heavily use topic exchanges).

Some questions that were brought up when discussing this with the team:

  • How should the permissions be defined?
  • How should it be scoped?
  • What response code is appropriate?

Below is (some) of the reasoning and proposed solutions.

How should the permissions be defined?

Expanding existing command (set_permissions) is likely not going to work well. A new command — and much as I'd like to avoid user confusion between the two — seems to be a more sensible choice.

The command, rabbitmqctl set_topic_permissions, can look like this

rabbitmqctl set_topic_permissions -p [vhost] [user] [exchange]  [pattern]

(everything that's not matched is prohibited).

where [pattern] is a regular expression (same as with the configure, write, read permissions). Routing keys of messages published to the exchange [exchange] then would be checked for permissions after the exchange itself is.

The command will use a set of tables similar to those that back our core internal authz backend under the hood. A new HTTP API endpoint has to be developed.

How should it be scoped?

Currently the easiest and also most flexible solution seems to be "per exchange". This would work
well for STOMP and MQTT, too, since most of the time they use a single topic exchange (or at least
a small number of them).

What response code is appropriate?

Same as when exchange permissions are insufficient. AMQP 0-9-1 and STOMP make it easy to communicate an error to the clients. MQTT doesn't have to way to do that beyond closing TCP connection. That's a short sighted protocol design decision we cannot do much about (without forking all clients, which we have no intention of doing).

Implementation Considerations

Effect on Authz Plugins

It remains to be seen what kind of changes this may require in various authz plugins. For example,
if a plugin doesn't recognize #resource{kind = topic} resources, it will likely fail. So other
plugins may need extra consideration and/or updating, which justifies effort-medium.

The #resource record can be made to accommodate #resource{kind = topic} easily but we also need to consider how to pass the routing key along. For example, #resource can be extended
to include a new map field, options, that can carry arbitrary resource-specific information.

Throughput Effect on Channel Operations

Not reducing throughput for other exchange types and taking the smallest hit we can for topic exchanges is very important here.

We already perform 2 checks on exchanges in rabbit_channel:handle_method/1 head
that handles #'basic.publish' methods:

  • Resource write permissions
  • Whether the exchange is internal

This becomes the 3rd test that is easy to filter out for non-topic exchanges by matching
on #exchanges. We already do something very similar in rabbit_channel:check_internal_exchange/1:

check_internal_exchange(#exchange{name = Name, internal = true}) ->
    rabbit_misc:protocol_error(access_refused,
                               "cannot publish to internal ~s",
                               [rabbit_misc:rs(Name)]);
check_internal_exchange(_) ->
    ok.

and thus can do a similar thing here

check_topic_permisions(#exchange{name = Name, type = topic}) ->
    %% …
check_topic_permisions(_) ->
    ok.
@michaelklishin
Copy link
Member Author

@michaelplaing do you have an opinion on how this should work?

@michaelplaing
Copy link

I would recommend doing it the simplest way. I like the 'per exchange' approach.

@videlalvaro
Copy link
Contributor

Is this going to query the permissions DB for every message passing via the topic exchange? If this is the case, how do you prevent these queries if the user doesn't have a need for this feature?

@michaelklishin
Copy link
Member Author

@videlalvaro the user won't configure any topic permissions for such exchanges. All queries will be local reads, just like with topic trie segments.

@videlalvaro
Copy link
Contributor

You probably want to take a look at what this plugin is doing: https://github.com/airboxlab/rabbitmq-topic-authorization

@uvzubovs
Copy link

uvzubovs commented Jan 5, 2016

MQTT is often used with the clients deployed outside of the trust zone. Therefore, providing them with the precise error description is typically frowned upon for security reasons. Closing the connection on the authorization failure, but logging the true reason on the server correlated to the client id, seems reasonable.

@uvzubovs
Copy link

uvzubovs commented Jan 5, 2016

Since authorization decides access to a resource by an actor, where the actor is the logged in user and the resource is the topic, the pattern (in the proposed permission command) should be able to refer to the logged in user. You may see example of this in Mosquitto (look for acl_file in http://mosquitto.org/man/mosquitto-conf-5.html). Such approach addresses the performance concern present in https://github.com/airboxlab/rabbitmq-topic-authorization.

@michaelklishin
Copy link
Member Author

@uvzubovs right, just like rabbitmqctl set_permissions does.

@michaelplaing
Copy link

If going down this route, one might consider a white list / black list
approach with wildcards to cover more use cases.

On Wed, Jan 6, 2016 at 2:25 AM, Michael Klishin notifications@github.com
wrote:

@uvzubovs https://github.com/uvzubovs right, just like rabbitmqctl
set_permissions does.


Reply to this email directly or view it on GitHub
#505 (comment)
.

@michaelplaing
Copy link

Here's a simple demo of white / black in python.

It uses MQTT syntax but is easily translatable / generalizable to AMQP.

There are four lists of ACLs: W/B for the topic plus W/B for the retain
flag (illustrating ACL chaining).

https://gist.github.com/michaelplaing/2f6f209b5449212a4984

On Wed, Jan 6, 2016 at 6:25 AM, Laing, Michael michael.laing@nytimes.com
wrote:

If going down this route, one might consider a white list / black list
approach with wildcards to cover more use cases.

On Wed, Jan 6, 2016 at 2:25 AM, Michael Klishin notifications@github.com
wrote:

@uvzubovs https://github.com/uvzubovs right, just like rabbitmqctl
set_permissions does.


Reply to this email directly or view it on GitHub
#505 (comment)
.

@uvzubovs
Copy link

uvzubovs commented Jan 6, 2016

With respect to permission scoping, either virtual host or exchange would work since the MQTT plug-in uses single exchange in a virtual host. If you would like topic authorization to apply beyond MQTT, then exchange is certainly the way to go. However, since the MQTT client cannot specify the virtual host like the AMQP client can (prefixing username with virtual host on connect is not always possible as in some cases we use client SSL certificate instead of user credentials), perhaps, you would consider specifying the default virtual host on a per-MQTT listener rather than for the entire plug-in, i.e.

{rabbitmq_mqtt, [
      %% {vhost, <<"MQTT">>},
      {tcp_listeners, [{1883,"vhost1"}, {1884,"vhost2"}]}
]}

@michaelklishin
Copy link
Member Author

This issue is not about MQTT specifically but there is one MQTT-specific aspect that's hard to ignore: the wildcards. We have a way to translate them into AMQP 0-9-1 ones. The question is, how should this be exposed to end user, given that rabbitmqctl is currently not extendable from plugins.

@michaelplaing
Copy link

Hmm.

The simple way is to document the correspondence I guess and send newer
users to a UI.

On Wed, Jan 6, 2016 at 10:47 AM, Michael Klishin notifications@github.com
wrote:

This issue is not about MQTT specifically but there is one MQTT-specific
aspect that's hard to ignore: the wildcards. We have a way to translate
them into AMQP 0-9-1 ones. The question is, how should this be exposed to
end user, given that rabbitmqctl is currently not extendable from plugins.


Reply to this email directly or view it on GitHub
#505 (comment)
.

@michaelklishin
Copy link
Member Author

We'll try to get it into 3.6.7 but there's a chance that it will go into 3.6.8 or 3.7.0.

@michaelklishin
Copy link
Member Author

For topic pattern definitions, what if instead of AMQP 0-9-1 patterns we used regular expressions? We already use them for permission patterns anyway. They are not perfect but more protocol-neutral and I expect that in most cases basic topic prefixing would be sufficient.

On the upside for us, it would be easy to implement and the matching function is already quite heavily optimized in the standard library.

@michaelplaing
Copy link

michaelplaing commented Dec 16, 2016 via email

@michaelklishin michaelklishin modified the milestones: 3.7.0, 3.6.x Dec 20, 2016
@michaelklishin
Copy link
Member Author

Unfortunately this will require schema changes that cannot go into 3.6.x.

@michaelklishin
Copy link
Member Author

Moreover, this will likely require updates to existing authz plugins, which also rules out the possibility of being included into a 3.6.x release.

acogoluegnes added a commit to rabbitmq/rabbitmq-common that referenced this issue Dec 20, 2016
acogoluegnes added a commit to rabbitmq/rabbitmq-website that referenced this issue Dec 29, 2016
acogoluegnes added a commit to rabbitmq/rabbitmq-mqtt that referenced this issue Dec 29, 2016
check_resource_access used to be called with
the MQTT topic as resource name and kind = topic.
It makes more sense now to call check_topic_access
with the exchange as resource name, kind = topic,
and routing key in the context.

References rabbitmq/rabbitmq-server#505
acogoluegnes added a commit that referenced this issue Dec 29, 2016
acogoluegnes added a commit to rabbitmq/rabbitmq-common that referenced this issue Dec 30, 2016
acogoluegnes added a commit to rabbitmq/rabbitmq-auth-backend-http that referenced this issue Dec 30, 2016
acogoluegnes added a commit to rabbitmq/rabbitmq-auth-backend-ldap that referenced this issue Dec 30, 2016
acogoluegnes added a commit to rabbitmq/rabbitmq-auth-backend-amqp that referenced this issue Dec 30, 2016
@michaelklishin
Copy link
Member Author

We have our second prototype of this feature undergoing QA this week.

acogoluegnes added a commit to rabbitmq/rabbitmq-common that referenced this issue Jan 3, 2017
acogoluegnes added a commit to rabbitmq/rabbitmq-cli that referenced this issue Jan 3, 2017
acogoluegnes added a commit to rabbitmq/rabbitmq-auth-backend-http that referenced this issue Jan 3, 2017
acogoluegnes added a commit to rabbitmq/rabbitmq-management that referenced this issue Jan 3, 2017
@michaelklishin
Copy link
Member Author

MQTT plugin still needs a bit of work so we will close this issue but keep rabbitmq/rabbitmq-mqtt#95 and our (private) Pivotal Tracker story open.

acogoluegnes added a commit to rabbitmq/rabbitmq-mqtt that referenced this issue Jan 16, 2017
Plugin handles exit signal coming from the AMPQ core,
logs, and closes the client connnection (instead of letting
the whole process tree crash with scary log messages).

References rabbitmq/rabbitmq-server#505
acogoluegnes added a commit to rabbitmq/rabbitmq-mqtt that referenced this issue Jan 16, 2017
acogoluegnes added a commit to rabbitmq/rabbitmq-auth-backend-cache that referenced this issue Jan 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants