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.

Sign up for GitHub

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

Jump to bottom

ACL Ambigious #167

Closed
arihantdaga opened this issue Apr 7, 2020 · 3 comments
Closed

ACL Ambigious #167

arihantdaga opened this issue Apr 7, 2020 · 3 comments
Assignees
@troian
Labels
question

Comments

@arihantdaga
Copy link

arihantdaga commented Apr 7, 2020 •
edited
Loading

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

Version of VolantMQ (or SHA)

99c21c3

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

volantmq/connection/connection.go

Line 864 in 99c21c3

if e := s.permissions.ACL(s.id, "", pkt.Topic(), vlauth.AccessWrite); e != vlauth.StatusAllow {
. 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
Copy link
Member

troian commented Apr 7, 2020

  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 added a commit to arihantdaga/volantmq that referenced this issue Apr 8, 2020
@arihantdaga
[Fix] Sending blank username to ACL in onPublish
0f2968b 
- Attenmpt to fix VolantMQ#167 partially.
@arihantdaga arihantdaga mentioned this issue Apr 8, 2020
Closed
@arihantdaga
Copy link
Author

Hi @troian I tried to fix it here #169. Let me know your opinion.
Also because of this -

volantmq/cmd/volantmq/auth.go

Lines 38 to 40 in 99c21c3

func (a *simpleAuth) ACL(clientID, user, topic string, access vlauth.AccessType) error {
return vlauth.StatusAllow
}

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 -

volantmq/connection/connection.go

Lines 925 to 930 in f15e5d9

case mqttp.QoS0: // QoS 0
// [MQTT-4.3.1]
// [MQTT-4.3.2-4]
// TODO(troian): ignore if publish permissions not validated
if err = s.publishToTopic(pkt); err != nil {
s.log.Error("Couldn't publish message",

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
Copy link
Member

troian commented Apr 8, 2020

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

@troian troian closed this as completed Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@troian troian

Labels
question
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fix] Sending blank username to ACL in onPublish arihantdaga/volantmq
2 participants
@troian @arihantdaga