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

Added support for MQTT Client ID #25

Merged
merged 1 commit into from May 31, 2019
Merged

Conversation

dimaj
Copy link
Contributor

@dimaj dimaj commented May 23, 2019

Some configurations of MQTT Brokers require Client ID to have a certain
prefix in order to connect to them. This addresses that scenario.

By default client id is generated in a form of tdm-XXXXXX, where XXXXXX is a 6 character alpha-numeric string that is randomly generated.

Some configurations of MQTT Brokers require Client ID to have a certain
prefix in order to connect to them. This addresses that scenario
@jziolkowski
Copy link
Owner

Thanks for the contribution. I will close this PR not because it's wrong or anything (it's actualy very well done) however I will integrate the concept into version 0.2 I'm making now. I'm reworking huge parts of code for this.

@dimaj
Copy link
Contributor Author

dimaj commented May 31, 2019

That's fine.

Why not have both of them around? This way, users that needs this functionality can have it today while you are still working on 0.2.

@andrethomas
Copy link

@dimaj I think the reason would be that it cannot be merged in its current form with changes @jziolkowski already made on his local code so it will need to be deferred until after the 0.2 updates are merged to the repository. Really nicely contribution though :)

@dimaj
Copy link
Contributor Author

dimaj commented May 31, 2019

@andrethomas thanks :)

I guess my confusion is coming from how 0.2 is being worked on. If you are working on a new version of the software, you would branch off by either making a new branch for new version until it's ready to be merged into master or by branching latest release into its own branch. In which case, patches and fixes to the latest release could happen independently of latest version. Of course, if the issue being fixed relates to bleeding edge, it would need to be merged in there too.

In any case, I think this tool is very cool and if people need this functionality while 0.2 is still under development, it is available in my fork under client_id branch and I am not planning on removing it anytime soon.

@andrethomas
Copy link

@dimaj Agreed but in @jziolkowski 's defence, I did not think he was anticipating taking long to make the transition to 0.2 and like most of us get distracted by other activities... and he is the only person working on 0.2 so there would not have been any logical purpose to doing what most would seem to be logical in hindsight. Most projects start out this way so thanks for making the client_id branch available and retaining it for the foreseeable future.

@jziolkowski
Copy link
Owner

You are both correct :) @dimaj you are absolutely right about how it should be done git-wise. I'm indeed not using it to its full potential. @andrethomas is right that RL stuff took over my attention for long enough that people are making PRs already :) In the end, I will branch my current local codebase and merge your PR. I will deal with merging/rebasing when the time comes. Thank you both.

@jziolkowski jziolkowski reopened this May 31, 2019
@jziolkowski
Copy link
Owner

However, isn't client I'd assigned randomly by mqtt broker when it's not set by the client?

@dimaj
Copy link
Contributor Author

dimaj commented May 31, 2019

hahaha

Just to be clear, I was not trying to attack @jziolkowski in any way shape or form. I was truly trying to understand the flow.

re: client_id being auto-generated/assigned - I believe that is the case. However, in my personal setup, since my mqtt broker is exposed to the world, I am requiring client ids to be of a certain format in order for connection to be established. So, I have 2 levels of authentication: user/pass + client_id format.

https://mosquitto.org/man/mosquitto-conf-5.html

clientid_prefixes prefix
If defined, only clients that have a clientid with a prefix that matches clientid_prefixes will be allowed to connect to the broker. For example, setting "secure-" here would mean a client "secure-client" could connect but another with clientid "mqtt" couldn't. By default, all client ids are valid.

@andrethomas
Copy link

andrethomas commented May 31, 2019

To my knowledge, the mqtt broker will not necessarily issue a client-id but will allow connections to be made without a client-id being specified by the client but it adds security benefits on the broker side insofar that you can limit connections using a prefix of sort.

Edit: What he said ^^

@dimaj
Copy link
Contributor Author

dimaj commented May 31, 2019

@andrethomas, based on this document, that would seem to be correct that it is not required / issued by the broker, but it is used to identify the client for state preservation:

https://www.hivemq.com/blog/mqtt-essentials-part-3-client-broker-connection-establishment/

The client identifier (ClientId) identifies each MQTT client that connects to an MQTT broker. The broker uses the ClientID to identify the client and the current state of the client.Therefore, this ID should be unique per client and broker. In MQTT 3.1.1 (the current standard), you can send an empty ClientId, if you don’t need a state to be held by the broker. The empty ClientID results in a connection without any state. In this case, the clean session flag must be set to true or the broker will reject the connection.

@andrethomas
Copy link

Sure, also correct but the security aspect is more interesting to me... having a random client id with a specific prefix won't do any good for state preservation on TDM since the client id would be different anyway on each new connection and even if it wasn't I don't think TDM needs any state preservation from the broker side anyway.

@dimaj
Copy link
Contributor Author

dimaj commented May 31, 2019

so, if i understand the last snippet I sent form hivemq, client id is completely optional. if it is not provided, it is not issued.
when you specify a client id, it is not augmented by the broker. what broker gets, broker uses. So, in my setup, I have my prefix set on the broker as "abc123" and then I set my client ids on my devices as: "abc123-deviceX". That is what broker will see and use.
If I set my client id as "tdm-abc123lasdkf", broker will deny connection as client id does not start with "abc123"

@jziolkowski jziolkowski merged commit 2aae89d into jziolkowski:master May 31, 2019
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.

None yet

3 participants