Rumqttc: Support for SEC1 encoded ECC keys.

[ijager]

Expected Behavior

I would like to use an "ECC" key with an ECDSA certificate. I have generated the key with the following openssl command:

openssl ecparam -name prime256v1 -genkey -noout -out client.key

Current Behavior

rumqtt cannot parse this key and this results in a Tls(NoValidCertInChain) error.

Failure Information (for bugs)

Things go wrong in rumqttc/src/tls.rs

let read_keys = match &client.1 {
    Key::RSA(k) => rustls_pemfile::rsa_private_keys(&mut BufReader::new(
        Cursor::new(k.clone()),
    )),
    Key::ECC(k) => rustls_pemfile::pkcs8_private_keys(&mut BufReader::new(
        Cursor::new(k.clone()),
    )),
};

The ECC() variant uses pkcs8_private_keys() to parse the key. However my key does not use PKCS#8 encoding, instead it uses SEC1 encoding, which was specifically designed for Elliptic Curve Cryptography. (PKCS#8 can also encode ECC keys, but it involves converting the keys).

Rustls does support SEC1 encoding via ec_private_keys(). Rumqtt could add support by adding a Key variant.

Perhaps something like this:

 let read_keys = match &client.1 {
    Key::RSA(k) => rustls_pemfile::rsa_private_keys(&mut BufReader::new(
        Cursor::new(k.clone()),
    )),
    Key::PKCS(k) => rustls_pemfile::pkcs8_private_keys(&mut BufReader::new(
        Cursor::new(k.clone()),
    )),
    Key::EC(k) => rustls_pemfile:: ec_private_keys(&mut BufReader::new(
        Cursor::new(k.clone()),
    )),
};

I changed ECC to PKCS And added EC. This would be a breaking change, so not sure if that is desired. But we could also keep the Key::ECC(k) => rustls_pemfile::pkcs8_private_keys( variant for backwards compatibility and make it deprecated.

It would be in line with the three variants that Rustls has for keys:

/// A DER-encoded plaintext RSA private key; as specified in PKCS#1/RFC3447
RSAKey(Vec<u8>),

/// A DER-encoded plaintext private key; as specified in PKCS#8/RFC5958
PKCS8Key(Vec<u8>),

/// A Sec1-encoded plaintext private key; as specified in RFC5915
ECKey(Vec<u8>),

I'll be happy to make a PR, but I'd like to check with this crate's preferences first via this issue.

[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.

[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 😁

[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?

[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]

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.

[swanandx]

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