aws / s2n-tls

An implementation of the TLS/SSL protocols
https://aws.github.io/s2n-tls/usage-guide/
Apache License 2.0
4.53k stars 708 forks source link

initial config influences negotiation when using client hello config resolution #4585

Open jmayclin opened 5 months ago

jmayclin commented 5 months ago

Problem:

Customers commonly want to configure a config based on the presented SNI. For example, setting different security policies or different certificates based on SNI. Our client-hello-config-resolution example shows how to set this up.

The example includes the following comment https://github.com/aws/s2n-tls/blob/4dae2b27e0218e1f4a4d48c5a64ce57a6c1e9dac/bindings/rust-examples/client-hello-config-resolution/src/bin/server.rs#L96-L101

While this is true in the general case, there are some suprising side effects that we either need to change or document.

We call the client-hello-cb at the end of s2n_client_hello_recv, but s2n_parse_client_hello is called with the initial config. s2n_parse_client_hello relies on the initial config in two places.

SSLv2 Client Hellos

As part of parse_client_hello, we handle SSLv2 client hellos. We reject some of the SSLv2 formatted client hellos based off of the supported version of the security policy on the initial client hello.

https://github.com/aws/s2n-tls/blob/4dae2b27e0218e1f4a4d48c5a64ce57a6c1e9dac/tls/s2n_client_hello.c#L814-L831

Default Curve

If the client doesn't explicitly specify the supported elliptic curves, s2n-tls will choose the default based on the curves in the initial config.

https://github.com/aws/s2n-tls/blob/4dae2b27e0218e1f4a4d48c5a64ce57a6c1e9dac/tls/s2n_client_hello.c#L474-L500

Solution:

Option 1

Make no mutating actions until after the client hello callback has been called. This is a conceptually cleaner approach that is significantly easier to reason about.

Option 2

Document the tricky things.

maddeleine commented 3 months ago

As of #4676 this issue is partial complete, however, I didn't touch the SSLv2 client hello logic, as that does reference the config before the client hello callback. So closing this issue is dependent on getting an integration test in place that ensures we don't screw up that logic while refactoring it.