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

fix(mqtt): prioritize respecting ok-to-mqtt #5805

Closed

Conversation

ctso
Copy link

@ctso ctso commented Jan 10, 2025

If I'm reading this code correctly, if an MQTT server resolves to an RFC1918 address the packet will always be forwarded to the broker, even if the "OK to MQTT" flag is set to false.

This smells like a bit of a privacy concern. If users have explicitly opted out of having their packets published to an MQTT broker, it shouldn't matter if that server is resolving to an RFC1918 address or not. It's very trivial to run an MQTT broker that is publicly accessible, but configure your client to post to this via a simple TCP proxy that suddenly violates the "OK to MQTT" flag that other clients may have specified.

It's possible this logic exists for some other reason that I don't understand, so happy to close this if I'm wrong here.

@Talie5in
Copy link
Contributor

@ctso I feel like this comment on another issue is relevant by @thebentern

#5404 (comment)

And they are right, there are forks out there already that already ignore the OktoMqtt flag, or reversed the NeighbourInfo change so it still goes out the main public channels as another example.

Ultimately though, I think the only change here I think possibly one of the ANDs should probably be an OR, or checked separately to if its a RFC1918 address or not.

@thebentern
Copy link
Contributor

@Talie5in is correct. The local address broker being the exception to the mqtt uplink was the concession by popular demand. This may change in a later version ultimately, but for now, this is by design. And as was mentioned, the bit remains a polite request to not uplink which does not prevent modified firmwares from trivially uplinking to even public brokers

@thebentern thebentern closed this Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants