Improve password handling #159
Conversation
The MQTT 3.1.1 specification includes the following statements: * "The password field contains 0 to 65535 bytes of binary data prefixed with a two byte length field..." * "If the User Name Flag is set to 0, the Password Flag MUST be set to 0" This change addresses both of these statements by: * Changing the password representation from a null terminated string to a byte array with length. * Enforcing that a username must be set if a password is set before attempting to connect to an MQTT broker. Signed-off-by: David Frey <dfrey@sierrawireless.com>
|
FYI: I get a 500 error when I click the login link on https://projects.eclipse.org, so I was unable to sign the CLA. |
|
I have now signed the eclipse CLA. What do I need to do to get the checks run again? |
|
Don't worry - the person who would merge the code could do a check outside of the github pull request. |
|
While this looks good in itself, I think I need to support the old way of adding passwords at the same time, for backwards compatibility. |
Squashed commit of the following: commit 907b9ec Author: Ian Craggs <icraggs@uk.ibm.com> Date: Wed Apr 5 23:33:44 2017 +0100 Use binary passwords in tests commit 3930b18 Author: Ian Craggs <icraggs@uk.ibm.com> Date: Wed Apr 5 17:15:29 2017 +0100 Binary MQTT password support commit 4abe6a9 Author: Ian Craggs <icraggs@uk.ibm.com> Date: Wed Apr 5 16:13:18 2017 +0100 Fix test config commit e742ed1 Author: Ian Craggs <icraggs@uk.ibm.com> Date: Wed Apr 5 15:46:53 2017 +0100 Async binary will message commit 9313e66 Author: Ian Craggs <icraggs@uk.ibm.com> Date: Wed Apr 5 11:50:10 2017 +0100 Binary will message support, take 2 commit 5cf5485 Author: Ian Craggs <icraggs@uk.ibm.com> Date: Tue Apr 4 22:54:15 2017 +0100 Binary will message
|
Thanks for the contribution. I've added an update for this to the develop branch, including tests. My update is a bit different - changing the external data structures in a backward compatible way. |
|
Sorry I didn't have a chance to update the PR, but thanks for implementing this change. There is one thing that my pull request added that yours does not do which is requiring that a password may only be present if a username is also present. See 2774efe#diff-866e259bd7df83e722c88fdb7bb9f9a8R2176 Perhaps that should have been submitted as a separate pull request. |
|
The MQTT protocol allows username and password to be included independently - a password can be sent without a username. I figured that although it seems like a feature that wouldn't be used, or useful, that the client library shouldn't disallow it. |
|
My change was based on this line from the spec:
I interpret this as a requirement to check that the username is present if the password is present. Is this incorrect? |
|
Well that statement is pretty clear, for sure. You got me there. Interestingly, that constraint seems to have been removed from the latest MQTT v5 draft, and I thought that it wasn't in 3.1 (or 3.1.1 for that matter). My feeling is still to leave it as it is, then the application is at the mercy of the server to decide. Alternatively, if we do check, we probably ought to return an error (no password without username) so that it's clear what we are doing. |
# By Ian Craggs (50) and others # Via Ian Craggs (10) and others * testcase-for-issue-190: (119 commits) Include MQTTAsync_create() call in the test iteration loop for test2d Set use_identity_as_username false for mosquitto listener 18884 Enable the server certificate in test5-2d Set the serverURI in the corresponding array of the connect options for test5-2d Ignore the build.paho directory, which is created by travis build script Rerun test5-2d 100 times Add new test2d for issue eclipse#190 Binary passwords and will messages: PR eclipse#159, issues eclipse#167 and eclipse#34 Squashed commit of the following: Fix for issue eclipse#164 Documentation updates Some small changes to PR eclipse#200 Some small cleanup Tidy up test3 and CMake config Reduce size on messages in test8-4 Fix TLS certs dir on Windows Add applink.c to windows TLS test program Fix startprocess path Fix startprocess syntax Fix proxy start Different approach to starting proxy ... Signed-off-by: Juergen Kosel <juergen.kosel@softing.com>
# By Ian Craggs (50) and others # Via Ian Craggs (10) and others * testcase-for-issue-190-without-serverURI: (118 commits) In test5 - 2d the client must not have the valid client certificate! Stop any mosquitto instance which may be still running from previous runs Correct / update the unit test labeling Reduce time for test2d Include MQTTAsync_create() call in the test iteration loop for test2d Set use_identity_as_username false for mosquitto listener 18884 Enable the server certificate in test5-2d Set the serverURI in the corresponding array of the connect options for test5-2d Ignore the build.paho directory, which is created by travis build script Rerun test5-2d 100 times Add new test2d for issue eclipse#190 Binary passwords and will messages: PR eclipse#159, issues eclipse#167 and eclipse#34 Squashed commit of the following: Fix for issue eclipse#164 Documentation updates Some small changes to PR eclipse#200 Some small cleanup Tidy up test3 and CMake config Reduce size on messages in test8-4 Fix TLS certs dir on Windows Add applink.c to windows TLS test program ... Signed-off-by: Juergen Kosel <juergen.kosel@softing.com>
Support binary passwords by treating passwords as uint8_t arrays rather than null terminated strings. This pull request does not include updates to the test or sample code. I have tried the synchronous client and it seems to work. I want to gauge whether there is interest in this change before I go through the work of updating the tests and samples. This change also enforces that a password may not be present without a username.