search

VolantMQ / volantmq

High-Performance MQTT Server
Apache License 2.0
981 stars 169 forks source link

ACL Ambigious #167

Closed arihantdaga closed 4 years ago

arihantdaga commented 4 years ago

Please fill out the sections below to help us address your issue.

Version of VolantMQ (or SHA)

99c21c3f43ecea668f77030f71b7f0d95c05e7d4

Version of Go (go version)?

go version go1.14.1 linux/amd64

What issue did you see?

Please forgive me if I am wrong. I found working of ACL is little ambiguous. First, In multiple Auth plugin environment, For checking the permission of subscribing to a topic, VolantMQ iterate through all plugins one by one and not the plugin which was used for authentication of that client. I think it would make more sense to use the same plugin for acl which authenticated user. And it'll be little bit more efficient also. For example - If we have two two auth plugins, simpleAuth and mongo auth. Simple auth is used for authentication of certain users, while mongo is used for rest. But simpleAuth allows subscription to any topic. While mongo plugin wants to restrict a particular user to only certain topics. VolantMQ will go through all plugins sequentially and if simpleAuth is in a prior order, since simpleAuth allows alll topics, It'll allow any client subscribing to any topic.

Second, I noticed this thing for checking permission for publish i found this was called https://github.com/VolantMQ/volantmq/blob/99c21c3f43ecea668f77030f71b7f0d95c05e7d4/connection/connection.go#L864. But here even if plugin returns StatusDeny, the message is actually published.

Third We are sending username as a blank string in ACL method in the above-mentioned line. (Why that ?). Is it right to check ACL just using client id ? because for authentication we are using username, and a client can claim any clientId while connecting to the broker. I was thinking that we should send the username also here.

P.S - I am very new to this, But i want to contribute. I'll be happy to do PRs. Please Let me know if i am thinking in the right direction?

troian commented 4 years ago

  1. Auth subsystem designed to be flexible. The main idea is each listener may have own auth list and it's order. Generally, if two different auth backends can authenticate the same user with different credentials it's a security pothole. Such a case shall not be considered. If you have a user with rights to subscribe/publish to any topic (for example using simpleAuth) this user shall have a good password and other auth backends with this listener must not have the user with the same name. Regarding storing a reference to the auth backend: I'm not sure it is a good idea though it provides the right use case as you mentioned. I'll give this problem a little think.

  2. if e := s.permissions.ACL(s.id, "", pkt.Topic(), vlauth.AccessWrite); e != vlauth.StatusAllow that's a bug. The user should be in there.

arihantdaga commented 4 years ago

Hi @troian I tried to fix it here https://github.com/VolantMQ/volantmq/pull/169. Let me know your opinion. Also because of this - https://github.com/VolantMQ/volantmq/blob/99c21c3f43ecea668f77030f71b7f0d95c05e7d4/cmd/volantmq/auth.go#L38-L40 Simple auth is always allowing by default. I think we'll have to do something if we want to block a user from pub/sub to any topic. Also this caught my attention - https://github.com/VolantMQ/volantmq/blob/f15e5d90611fc133d5c158f8ba75c7dc7e1bfa9b/connection/connection.go#L925-L930

This explains why my message passed through even if i set default of simpleAuth to StatusDeny. The message I was publishing was with QOS0 Shall i go ahead trying to fix this ?

troian commented 4 years ago

Yes, that's why it called simpleAuth. It is intended to do user/password authentication not pub/sub. So there is nothing to do with it. pub/sub filtering is up to more complex auth backends

Issue you mention is fixed in pr #168