listener: add support for multiple protocol sniffers. #3260
Conversation
Previously, TLS Inspector set detected transport protocol to either "tls"
or "raw_buffer", however, such approach didn't allow multiple sniffers to
be installed at the same time, since they would overwrite results from
each other.
After this change, protocol sniffers will set detected transport protocol
only when they find the one they know about it, and listener will set the
default one ("raw_buffer") in case no known transport protocol was found.
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
|
cc @ggreenway @lizan |
| @@ -148,6 +150,11 @@ void ConnectionHandlerImpl::ActiveSocket::continueFilterChain(bool success) { | |||
| // prevent further redirection. | |||
| new_listener->onAccept(std::move(socket_), false); | |||
| } else { | |||
| // Set default transport protocol if none of the listener filters did it. | |||
| if (socket_->detectedTransportProtocol().empty()) { | |||
| socket_->setDetectedTransportProtocol( | |||
There was a problem hiding this comment.
Should we at least add an ASSERT inside the implementation of setDetectedTransportProtocol that it's not set multiple times?
There was a problem hiding this comment.
Depending on what filters are installed and what they're trying to sniff, I could see it being possible for two heuristics to both return true. I'd say allow it, and maybe have a stat for it.
There was a problem hiding this comment.
I've been going back and forth on this, really.
I expect that sooner or later we'll have multiple transport sockets implementing the same transport protocol (e.g. we could have builtin BoringSSL implementation and another implementation with HSM integration, both using "tls" transport protocol). In theory, single TLS Inspector is all that's needed, but I expect we're going to see "sniffer + transport socket" combos, once the ecosystem of custom extensions starts growing up.
There was a problem hiding this comment.
That makes sense, I just wonder if we should add the ASSERT for now for sanity. We can always remove it later. I don't have a strong opinion either way.
There was a problem hiding this comment.
Let's skip it -ASSERT() on 2nd setDetectedTransportProtocol() might result in hard to debug crashes, is going to be removed at some point anyway, and it might prevent people from deploying custom and/or private sniffers.
Previously, TLS Inspector set detected transport protocol to either "tls"
or "raw_buffer", however, such approach didn't allow multiple sniffers to
be installed at the same time, since they would overwrite results from
each other.
After this change, protocol sniffers will set detected transport protocol
only when they find the one they know about it, and listener will set the
default one ("raw_buffer") in case no known transport protocol was found.
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Rama <rama.rao@salesforce.com>
Previously, TLS Inspector set detected transport protocol to either "tls"
or "raw_buffer", however, such approach didn't allow multiple sniffers to
be installed at the same time, since they would overwrite results from
each other.
After this change, protocol sniffers will set detected transport protocol
only when they find the one they know about it, and listener will set the
default one ("raw_buffer") in case no known transport protocol was found.