Rumqttc: Support for SEC1 encoded ECC keys.

[ijager]

Rumqttc: Support for SEC1 encoded ECC keys.

[ijager]

I still think it is a mistake to expose those details in your API since rumqtt doesn't need those details to work, but if you insist I will do it your way.

Here is my reasoning.
Even If you let the user specify the key type and we handle them correctly there will still be cases like this one:

Let's say we generate an RSA key with the following command

$ openssl version
OpenSSL 3.1.2 1 Aug 2023 (Library: OpenSSL 3.1.2 1 Aug 2023)
$ openssl genrsa -out client.key 2048

Although this is an RSA key, it is encoded in PKCS#8 format. Thus it will not work with Key::RSA as that variant assumes PKCS#1 encoding.. The user has to know to select either Key::ECC or Key::PKCS instead to make it work. Which is very hard to figure out if you're not that deep into TLS (it took me quite a while to understand why I got Err(Tls(NoValidCertInChain))).

So why not let Rustls deal with this instead of wrapping it in an extra layer that does not abstract anything.

For example the MongoDB rust driver also uses rustls and they accept any type of key even though they impose way more restrictions on how to certificates and keys are generated. The user does not need to specify which type they provide.

[swanandx]

good catch, I was dealing with lot of PRs at once so lost context of "c" & "d" haha.

Please feel free to open PR!

btw for rumqttc, can we use read_one fn as we did in mentioned PR? ( Instead of introducing new variants ) Or is it better the to introduce new variant? update: after seeing the code, introducing mew variant is the best option as you suggested!

Thank you so much 😁

[swanandx]

PR #752 is merged now, will close this issue, thank you!

[swanandx]

Hey, this issue will be resolved with PR #743 ! Please have a look once, thank you 😁

[ijager]

That PR only touches rumqttd, not rumqttc. But I guess both could benefit from full rustls support.

[ijager]

Ah I just implemented it using read_one. Let me know which way you prefer.

[swanandx]

Ah I just implemented it using read_one. Let me know which way you prefer.

As users explicitly specify which key type they are using while adding key, let's stick with existing code and add an variant 🚀

[ijager]

Okay, I went with a hybrid approach. I have added the variant as a way of documenting support for the different key types, but I also kept the read_one() approach of handling the key as it is much cleaner. See #752 .

[swanandx]

Okay, I went with a hybrid approach. I have added the variant as a way of documenting support for the different key types, but I also kept the read_one() approach of handling the key as it is much cleaner. See #752 .

Issue with the approach is that if I specify an RSA key using Key::EC(..), it will still work normally. This isn't what we intend right?

[ijager]

Yes that would still work, I agree it is a bit weird. Perhaps makes more sense to get rid of the Key enum as the current variants were flawed anyway (ECC didn't work for many ECC keys), but that would be a (small) breaking change.

If you want to make it error on 'wrong key format' you are burdening the user with stuff that actually works automatically but just chose to make more hard to use.

[swanandx]

If you want to make it error on 'wrong key format' you are burdening the user with stuff that actually works automatically but just chose to make more hard to use.

That was the case due to incorrect use of function, after we correct it, this should not be a problem right?

I believe letting users specify which Key format they actually want to use it good, and as it's how it is rn, it won't cause a big breaking change as you mentioned.

wdyt?

[swanandx]

As per your reasoning, it does make sense to remove the Key and handle reading internally.

Though I'm not much familiar with this TLS related things, so @henil & @de-sh could suggest it better!

Thank you for understanding 😊

[ijager]

I have removed the Key enum now.