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

Mosquitto ACL Topic Wildcard at Beginning of Pattern Not Working #1539

Closed
LeonPoon opened this issue Dec 17, 2019 · 12 comments
Closed

Mosquitto ACL Topic Wildcard at Beginning of Pattern Not Working #1539

LeonPoon opened this issue Dec 17, 2019 · 12 comments

Comments

@LeonPoon
Copy link

@LeonPoon LeonPoon commented Dec 17, 2019

My acl_file says:

user openhab
topic readwrite +/cmnd/POWER2
topic readwrite device-a/cmnd/+

The log says:

1576575835: Received PUBLISH from openhab (d0, q1, r0, m20, 'device-a/cmnd/POWER2', ... (3 bytes))
1576575838: Denied PUBLISH from openhab (d0, q1, r0, m21, 'device-b/cmnd/POWER2', ... (3 bytes))

Why does device-a/ work but not device-b/? Shouldn't the + at the start of the first topic in acl match "device-b"?

Mosquitto debian 1.4.10-3+deb9u4.

(Also posted at https://stackoverflow.com/questions/59371683/)

@karlp

This comment has been minimized.

Copy link
Contributor

@karlp karlp commented Dec 17, 2019

Does it do what you want if you switch the order? I'd guess it's based on accepting/notaccepting first or last match. (that's also a very outdated mosquitto)

@LeonPoon

This comment has been minimized.

Copy link
Author

@LeonPoon LeonPoon commented Dec 17, 2019

Changing the order of the topic lines doesn't change the behaviour.

Also tried with just a single line of +/cmnd/POWER2 but this results in both publish being denied.

I am just using the version that comes with debian stretch. No time yet to upgrade to buster.

However I managed to dpkg install 1.5.7-1+deb10u1. Behaviour is the same.

Will try to get 1.6.7 if I can sort out debian dependencies...

@karlp

This comment has been minimized.

Copy link
Contributor

@karlp karlp commented Dec 17, 2019

http://repo.mosquitto.org/debian/ should help, you should get 1.6.8 there no problem

@LeonPoon

This comment has been minimized.

Copy link
Author

@LeonPoon LeonPoon commented Dec 17, 2019

Thanks @karlp. Got 1.6.8.

Got the source deb too. Did some printf debugging...

With printf("mosquitto_topic_matches_sub2 sub=%s[%d] topic=%s[%d]: result=%d, ret=%d(%s)\n", sub, sublen, topic, topiclen, *result, ret, err);, found:

mosquitto_topic_matches_sub2 sub=+/cmnd/+ [0] topic=device-a/cmnd/POWER[0]: result=0, ret=3(MOSQ_ERR_INVAL)
mosquitto_topic_matches_sub2 sub=+/tele/+[0] topic=device-a/cmnd/POWER[0]: result=0, ret=0(MOSQ_ERR_SUCCESS)
mosquitto_topic_matches_sub2 sub=+/stat/+[0] topic=device-a/cmnd/POWER[0]: result=0, ret=0(MOSQ_ERR_SUCCESS)
mosquitto_topic_matches_sub2 sub=+/cmnd/+ [0] topic=device-b/cmnd/POWER[0]: result=0, ret=3(MOSQ_ERR_INVAL)
mosquitto_topic_matches_sub2 sub=+/tele/+[0] topic=device-b/cmnd/POWER[0]: result=0, ret=0(MOSQ_ERR_SUCCESS)
mosquitto_topic_matches_sub2 sub=+/stat/+[0] topic=device-b/cmnd/POWER[0]: result=0, ret=0(MOSQ_ERR_SUCCESS)

Now it is working after sed -r -e 's/\s+$//g' -i acl.txt.

@karlp

This comment has been minimized.

Copy link
Contributor

@karlp karlp commented Dec 17, 2019

trailing whitespace? I would suggest that trailing whitespace would be considered a bug. Topics can contain whitespace, and you could have them at the end, but certainly not with a + or a #. # must be the last char, and + can only be followed by a /.

@LeonPoon

This comment has been minimized.

Copy link
Author

@LeonPoon LeonPoon commented Dec 17, 2019

you could have them at the end

Is that in the specs?

I guess this is why mosquitto doesn't automatically strip the trailing space when reading from the acl file, although I wonder why people will want to use something with trailing space as a topic.

@karlp

This comment has been minimized.

Copy link
Contributor

@karlp karlp commented Dec 17, 2019

you can have them anywhere you like. iirc, topics can't have a \0, but anything utf8 is fair game. spec's not very long, woudl be easy to go dig it up. 1.5.3 UTF-8 encoded strings says what's valid for topics. (3.3.2.1 Topic Name says that it's to be a utf8 string per 1.5.3)

@ralight

This comment has been minimized.

Copy link
Contributor

@ralight ralight commented Dec 18, 2019

A trailing space is perfectly valid, except for after a + or # as Karl says. It might be a daft thing to do, but there you go. I've just pushed a change to check for validity of the topics which would have helped in this specific case.

ralight added a commit that referenced this issue Dec 18, 2019
Closes #1539. Thanks to Leon Poon.
@CliveJL

This comment has been minimized.

Copy link

@CliveJL CliveJL commented Jan 17, 2020

I just got caught out when enabling ACLs for the first time by having a trailing space after the username in the ACL file. I couldn't understand why my user wasn't seeing any publishes after subscribing to a valid topic including a "#" wildcard at the end. My fault obviously, but it would be nice to have this validated on reload too maybe?

@karlp

This comment has been minimized.

Copy link
Contributor

@karlp karlp commented Jan 17, 2020

the fix here can only validate things are are illegal, what trailing space did you have? trailing space after a # or +? otherwise, it's "daft" but legal unfortunately. (there may be room for warning on it anyway though...)

Otherwise, I believe this ticket itself should be closed?

@CliveJL

This comment has been minimized.

Copy link

@CliveJL CliveJL commented Jan 17, 2020

It was just a trailing space after the username in my ACL file, i.e.
user myuser<trailing space>
topic read foo/#

So my own fault really (I'm still not sure how I managed to do that...), and I guess it may be a "valid" username, albeit one that didn't exist in my passwd file because of that extra space, hence the problem. A warning might be nice I guess, but it's not a blocker :)

ralight added a commit that referenced this issue Jan 30, 2020
Closes #1539. Thanks to CliveJL and LeonPoon.
ralight added a commit that referenced this issue Jan 30, 2020
Closes #1539. Thanks to CliveJL and LeonPoon.
@ralight

This comment has been minimized.

Copy link
Contributor

@ralight ralight commented Jan 30, 2020

Trailing space is now trimmed on both user and topic, so I'm going to close this. Although it's valid, I can't imagine it is a good idea.

@ralight ralight closed this Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.