Use aws_lc_rs instead of ring to check for supported key type

[Alvenix]

Can we change the check for public key from ring to awc_rs_ls? Both depend on features enabled on rustls. Currently awc_rs_ls is the default (on 0.23) and support more keys.

Edit: I probably should try to update rustls to 0.23 first.

[SergioBenitez]

As it stands, aws-lc-rs is much more onerous to build than ring (NASM + cmake). I think until it's as easy to build, we should stick with ring.

[Alvenix]

As it stands, aws-lc-rs is much more onerous to build than ring (NASM + cmake). I think until it's as easy to build, we should stick with ring.

Yes I agree.

If only there was a way to to support both and let the user choose. In stable Rust, this is only possible by adding additional features to rocket which is probably unnecessary complexity.

Thankfully it is easy to patch dependencies in Rust for those who needs to.

[SergioBenitez]

If only there was a way to to support both and let the user choose. In stable Rust, this is only possible by adding additional features to rocket which is probably unnecessary complexity.

I'm not opposed to adding some features here. IE adding tls-ring and tls-aws-lc, and having tls enable tls-ring by default. The key would be in making these additive: what happens when both are enabled?

[Alvenix]

If only there was a way to to support both and let the user choose. In stable Rust, this is only possible by adding additional features to rocket which is probably unnecessary complexity.

I'm not opposed to adding some features here. IE adding tls-ring and tls-aws-lc, and having tls enable tls-ring by default. The key would be in making these additive: what happens when both are enabled?

What about having tls-aws-lc override tls-ring?

[Alvenix]

I added a single feature: tls-aws-lc, ring is enabled by default as it have easier build requirements. Enabling this feature would override ring. Is this approach acceptable? I will try to add documentation and a test (or an example) for the feature.

[Alvenix]

I changed the approach to making ring default by rocket, and using CryptoProvider::get_default where possible. This way user of Rocket can specify at runtime (using CryptoProvider::install at the start of the main for example) if they want to change ring.

It would have been better if I could remove all ring usage in rocket then the end user would choose by enabling the appropriate feature.

The only thing blocking full ring independence is creating a ticketer. I am not really sure how to do that. So currently I am using ring ticketer.

There is also the list of supported cipher algorithms but I feel that is easily solvable by adding them to Rocket.

However, I feel the current approach is very good.

[SergioBenitez]

Right, that's definitely the mechanism we should use. But, other parts of Rocket currently use ring "things" where they should be using aws-lc things if aws-lc is installed as the default crypto provider. A quick grep yields at least two places:

core/lib/src/listener/tls.rs
62:        tls_config.ticketer = rustls::crypto::ring::Ticketer::new()?;

core/lib/src/tls/config.rs
479:        use rustls::crypto::ring::cipher_suite;

The former is aws_lc_rs::Ticketer. The latter should use aws_lc_rs::ALL_CIPHER_SUITES and aws_lc_rs::DEFAULT_CIPHER_SUITES.

[Alvenix]

Right, that's definitely the mechanism we should use. But, other parts of Rocket currently use ring "things" where they should be using aws-lc things if aws-lc is installed as the default crypto provider. A quick grep yields at least two places:

core/lib/src/listener/tls.rs
62:        tls_config.ticketer = rustls::crypto::ring::Ticketer::new()?;

core/lib/src/tls/config.rs
479:        use rustls::crypto::ring::cipher_suite;

The former is aws_lc_rs::Ticketer. The latter should use aws_lc_rs::ALL_CIPHER_SUITES and aws_lc_rs::DEFAULT_CIPHER_SUITES.

I left them to use ring intentionally as Rocket enable only ring by default (and disables aws_lc_rs) as it has simpler compilation requirement as you said.

It is up to the user to enable aws_ls_rs feature.

If the user want Rocket to use aws_lc_rs, he can add the following to the Cargo.toml and main.rs

rustls = { version = "*", features = ["aws_lc_rs"] }

fn main() {
   rustls::crypto::aws_lc_rs::default_provider().install_default().unwrap();
}

In this commit: even if the user enable aws_lc_rs, he would be only be using ring for the ticketer. I think that is acceptable for my use case as it doesn't effect the more supported signature keys. (I assume that using ring ticketer with aws_lc_rs tls will work, but I will need to verify this)

I am actually not sure about the cipher suites, I thought because they had the same names they were internally the same. But I probably have to fix that. I think it would be better to get them dynamically from the CryptoProvider struct as it has array of supported ciphers.

[Alvenix]

I changed the ciphersuite mapping function to be dynamic instead of using ring or aws_lc_rs feature based on the default crypto provider. Note that the user can provide his own custom crypto provider which may not support the chosen cipher suite, in that case I used expect.

The ticketer remain but I feel that is less important (if the code work correctly, but I will test it). If it is important I will check if there is a way.

[SergioBenitez]

In this commit: even if the user enable aws_lc_rs, he would be only be using ring for the ticketer. I think that is acceptable for my use case as it doesn't effect the more supported signature keys. (I assume that using ring ticketer with aws_lc_rs tls will work, but I will need to verify this)

I think this would be rather unexpected. Is there a way for a user to "install" a ticketer? Or to get the default ticket from a CryptoProvider? We should do that, if so.

I changed the ciphersuite mapping function to be dynamic instead of using ring or aws_lc_rs feature based on the default crypto provider. Note that the user can provide his own custom crypto provider which may not support the chosen cipher suite, in that case I used expect.

We can't have that expect(). Rocket should never panic, ever, unless it's completely "done" (like how Error panics on Drop).

I think what I'm realizing is that if the user installs a CryptoProvider, we should use the ciphersuites in that cryptoprovider as opposed to the ones configured in the TlsConfig. Otherwise we're simply overriding the user's provider's wishes. Does that make sense to you? If so, then it sounds like we should move the From logic to the util::default_crypto_provider() function and use it only when we return our default crypto provider.

[Alvenix]

I think this would be rather unexpected. Is there a way for a user to "install" a ticketer? Or to get the default ticket from a CryptoProvider? We should do that, if so.

I don't think there is currently any one to do so but I opened a feature request in rustls repo for this.

[Alvenix]

I think what I'm realizing is that if the user installs a CryptoProvider, we should use the ciphersuites in that cryptoprovider as opposed to the ones configured in the TlsConfig. Otherwise we're simply overriding the user's provider's wishes. Does that make sense to you? If so, then it sounds like we should move the From logic to the util::default_crypto_provider() function and use it only when we return our default crypto provider.

Yes this feel much better. I have pushed this change.

Ticketer remain but in my opinion I think it is okay to use ring Ticketer untill rustls support getting it dynamically.

[Alvenix]

quic used in Rocket currently use older rustls so I had to use ring parsing for it. Currently windows failed because I added usage of awc-lc-rs in the tls example for testing. Should I make the aws-lc-rs part run only in linux?

[Alvenix]

When someone use aws-lc-rs CryptoProvider this could load keys that are not supported by older versions of rustls (e.g: P521), so there may be failure later when running the quic server.

I am noting this, because I used boolean before to error early. If a later error is acceptable that is fine but I don't want you to accidentally miss this during review.

[SergioBenitez]

Thanks for the note! That's useful to know. This quic part is temporary. Upstream has already updated rustls, so this whole bit will be gone shortly.

[Alvenix]

This affect the whole process, so it could affect other tests. I am still researching how to separate this test into a standalone process.

[Alvenix]

I used integration test to separate the tests. If additional integration tests is needed maybe we can add different test folder using [[test]] in Cargo.toml.

[SergioBenitez]

Right, but at that point it doesn't really matter if it affects the entire process - only other tests in the same crate would be affected. We simply need to test it with the default and then aws-lc, which this test accomplishes. I preferred the approach here, which is nice and short.

[Alvenix]

Yes I agree, should I remove the last commit?

"We simply need to test it with the default and then aws-lc, which this test accomplishes." yes but what if this was the first test that was started. Then all the other tests would be tested with aws-lc instead of the default