Allow ECC Key Configuration with rustls

[AlexandreCassagne]

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

• Formatted with cargo fmt
• Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

Possible remaining actions

• I have not included any tests yet. If you think this PR may be accepted I will look at that Monday
• I think that we should add a mention in the documentation for this feature
• I have made the choice of adding the ecc_ prefix. Perhaps it makes more sense to detect the header or algorithm values found in the certificate (----- BEGIN EC PRIVATE KEY -----). Also, I didn't want to add a breaking change, but this assumes the RSA certs are the defaults.
• Consider implementing the above breaking change (maybe in future PR), with the following approach:
• rsa_ prefix allows current certs (such as the ones generated by provision, which I think are PKCS#1), i.e BEGIN RSA PRIVATE KEY
• ec_ or ecc_ prefix with the BEGIN EC PRIVATE KEY
• no prefix for universal private key files, of the form BEGIN PRIVATE KEY

Anyway, someone more senior in this project can help make those decisions. I don't fully understand if there was a reason for hardcoding RSA in the code, but my use case requires ECC (embedded environments)

[swanandx]

Hey, thanks for the PR 🚀

I believe introduction of new Enum variant and other changes can be avoided. If you see the docs of rustls_pemfile, we can use something similar to extract only the keys, and as it checks headers ( rsa, ecc etc. ) internally, it should work fine right?

Basically, instead of calling rsa_private_keys(), extract the keys using read_one() as shown in example ( we can use read_all() and then discard the other variants of Item which aren't key, but ignoring them as we go seems much elegant ).

Hope this helps! let me know what do you think 😁

[AlexandreCassagne]

Thanks for your reply! Yes, that makes a lot of sense. I will play with read_one/read_all and update my PR.

I will return the first EC or RSA private key that appears by invoking read_one() repeatedly on the PEM file.

[swanandx]

Hey @AlexandreCassagne 👋 any updates on the PR? Is it ready for review?

[AlexandreCassagne]

Hey @swanandx,

I do think it is finished! However, do you think we should add additional tests?

[AlexandreCassagne]

thanks for review - will apply your recommendations now

[swanandx]

Hey, pushed some commits to fix clippy warnings & some fmt, plz have a look once!

Do you think we should add additional tests?

ideally we should! but not sure how easy / hard it would get. If you think it's quite easy, feel free to add them, otherwise for now, let us manually test if everything is working as expected to unblock merging of this PR. Can you please just test it once with different keys?

Thank you!

[AlexandreCassagne]

Sounds good. I have tested once with ECCs and will check RSAs

[AlexandreCassagne]

ideally we should! but not sure how easy / hard it would get.

@swanandx You were right; it was a nightmare but thankfully it worked. I only implemented RSA tests, I will add an EC variant of the test before we can merge.

Note that rumqttc suffers from the same logic problems that we noticed in rumqttd for tls.

I am not very proud of the last commit that patches it (it is ugly).

I think it requires a stronger refactor of that method -- I'd suggest reusing the first_private_key_in_pemfile we included earlier in this PR. That will probably require some rewrites in the function, possibly also suggesting that we should also remove the RSA/EC rumqttc Key enum (at rumqttc/src/lib.rs:209, delegating that responsibility to rustls' read_one method similarly to the rumqttd approach

However, all this is out of scope here --- would you mind reviewing the minimal changes I made to the client, along with the tests? And perhaps we move the above suggestions to a different PR to unblock?

[AlexandreCassagne]

Also worth reviewing this commit.

[swanandx]

Hey @AlexandreCassagne , thanks for the efforts and awesome work with getting it to actually work lol 💯

I do appreciate your work, but tbh this is a bit overkill, we should not modify rumqttc in this PR ( as this is strictly related to broker ). And for testing part, using rumqttc for testing rumqttd is cool, but it would be better to test broker externally ( maybe with tools like mqttwrk for conformace, which does the same, though it doesn't have tls yet ) for now ( getting out of scope of this PR as well haha )

suggesting that we should also remove the RSA/EC rumqttc Key enum

This is tackled in #752

will it be possible to revert these changes? ( this is the last working commit iirc )

It would be better to have a separate PR just dedicated to add tests ( like basic integrity test and all ) to rumqttd in future, let's avoid that in this PR.

Thank you once again 😄

[AlexandreCassagne]

Sounds good, I'll revert and move the additional commits into another branch of mine, as for our use case, we do need to be able to run tests before we push into production.

Nonetheless, I'd suggest keeping opening another PR with some of the code I wrote for these tests eventually. I learnt a lot about how rumqttc/d implementation of TLS worked by writing this, and also I ended up understanding why the Provision certificates worked, but not the ones I made with openssl. A documentation PR may come later :)

[AlexandreCassagne]

Also - one thing I noticed is that the interface of Rumqttd's configuration doesn't really allow programmatic instantiation. As a result, I was effectively forced into saving files in the file system and using toml

Are you open to a PR for allowing other means of instantiating the TLS config programmatically with rustls data structures directly?

[AlexandreCassagne]

@swanandx Alright, reverted. Backup of the 7 deleted commits is in AlexandreCassagne/tests-tls-rumqttc-can-connect-rumqttd

[swanandx]

I learnt a lot about how rumqttc/d implementation of TLS worked by writing this, and also I ended up understanding why the Provision certificates worked, but not the ones I made with openssl. A documentation PR may come later :)

That's great 💯

Are you open to a PR for allowing other means of instantiating the TLS config programmatically with rustls data structures directly?

I didn't get what do you mean by rustls data structures 😅 We can init the config like Config::default() and then edit it further right? kinda like you did in PR.

Thank you so much for the contribution 🚀