feat(s2n-metric-subscriber): add supported parameters#5768
feat(s2n-metric-subscriber): add supported parameters#5768
Conversation
943575b to
c936971
Compare
* add copyright header
bindings/rust/standard/s2n-tls-metrics-subscriber/src/parsing/mod.rs
Outdated
Show resolved
Hide resolved
bindings/rust/standard/s2n-tls-metrics-subscriber/src/record.rs
Outdated
Show resolved
Hide resolved
bindings/rust/standard/s2n-tls-metrics-subscriber/src/record.rs
Outdated
Show resolved
Hide resolved
| if conn.client_hello_is_sslv2()? { | ||
| self.sslv2_client_hello.fetch_add(1, Ordering::Relaxed); | ||
| } else { | ||
| let supported_parameter = ClientHelloSupportedParameters::new(conn.client_hello()?); |
There was a problem hiding this comment.
Hmmm okay let me see if I understand the motivation for this PR is. Handshake records previously contained lists of negotiated crypto parameters. A user looks at the cumulative crypto parameters and understands what's getting negotiated(and therefore what is safe to drop support for.) Now we are adding all supported parameters to the handshake event, presumably because we want to know my peers support (i.e. what parameters are safe to move to). But do you know that? If one client doesn't support x25519, would that be lost in the noise of the other clients who do supported x25519? Or is the goal to do some reasoning, we did 100 handshakes and 99 handshakes had clients that supported x25519, therefore there must be one client that will be broken if we only offered x25519?
Just wondering since negotiated parameters is useful when cumulative, but supported parameters is more of a "per connection" type of datapoint in my mind.
There was a problem hiding this comment.
If one client doesn't support x25519, would that be lost in the noise of the other clients who do supported x25519?
No, because we also know the total handshake count.
Consider the use case where you want to shift an endpoint to a FIPS policy. Currently everyone negotiates x25519, because that's the key share that they are sending. Let's say that there are 100 handshakes on the endpoints
handshake_count: 100
negotiated.x25519: 94
negotiated.secp256r1: 6
supported.x25519: 95
supported.secp256r1: 99
In this case, we know that there is one connection which didn't support secp256r1, and would have been broken by the FIPS policy.
Goal
Add support for
supported_parametermetrics to the metrics subscriber.Why
This is necessary to determine if a security policy can be changed without breaking customers.
How
s2n-tls does not expose APIs to retrieve this information, so we manually parse them out of the relevant parts of the ClientHello.
It is admittedly not great to write a new client hello parser. However C is a violent creature that must be kept at arms length, so I think it's better to go this way than exposing more of the C parser.
Note that I do use the existing parser where possible to pull out the extensions.
Callouts
I am currently directly referencing s2n-codec through a git dependency. We'll need to update that for the next release.
Testing
Added unit tests. The snapshot test is also useful.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.