tls: add DetectSni middleware

[zaharidichev]

This change adds a new DetectSni middleware to be used in the outbound stack
in order to extract the SNI extension from the ClientHello of a TLS session and apply
routing decisions based on it.

In contrast to DetectTls this new middleware is concerned with just extracting the SNI
as opposed to terminating the TLS session.

Signed-off-by: Zahari Dichev [email protected]

[olix0r]

I'm a little skeptical on modifying the existing behavior--trying to decouple detection and termination, because the termination ends up being highly contingent on the other layer setting it up properly--we end up exposing a bunch of the implementation details outside the module, which is complex.

It seems like we probably need to get something exposed that only reads the SNI. The TLS server module can probably reuse some internals, but it would probably be easier to focus on adding something that does exactly what we need it do and only on that so we don't incur additional type complexity.

[olix0r]

Using a tuple-target is a bit of an anti-pattern: it's fragile and doesn't really permit any intermediate layers to work. It may be appropriate here if we can bundle it up so that it's opaque to the caller, but it's preferable to use a Param or ExtractParam to get a ServerName from the target.

[olix0r]

This no longer detects really, right? Should this be renamed to reflect what it does?

It's also pretty awkward to me that we pass an optional SNI and the local identity here and then at process-time we check whether we should actually do TLS termination.

[zaharidichev]

I'm a little skeptical on modifying the existing behavior--trying to decouple detection and termination

@olix0r Makes sense. I have reverted these commits and pushed a simpler version that just adds a DetectSni middleware that makes use of the bits and pieces in the server module. Also, I have incorporated your feedback about tupled targets and tried to rely on InsertParam.

Not sure whether we want to just move all sni and client hellos internals into their own module that is used by both server and detect_sni or keep it as it is to minimize the change.

One option is to have the following mods:

• extract_sni - contains the detect_sni function + client hello details + tests
• server - contains TLS termination logic, etc
• detect_sni - contains the middleware to detect the sni.

WDYT?

[olix0r]

I'm curious about the timeout handling here and whether this should be instrumented here. On a known TLS connection, what do we do in the face of a timeout? Error? The timeout case exists on the existing detection logic so that we can forward connections as opaque after a timeout elapses, but there's no such fallback logic here, is there? I'd probably have to go back to the higher level draft PR to get a sense of how this fits together, but my gut instinct is that we do not need a timeout here.

[zaharidichev]

Good point. So yes, essentially we would error with a SniDetectionTimeoutError if the timeout elapses. Does that make sense here?

[olix0r]

Please update the commit message subject and description before merging: this no longer 'splits' TLS detection logic, we're adding a new DetectSni middleware to the TLS crate... The commit message body should include brief description of what this middleware does.