Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/162#issuecomment-69942861
Looks good to me. Originally concerned that the transparent_passthrough
state was being needlessly duplicated, but the HttpSM is not here yet at this
point. And you are following the pattern of the HttpSessionAccept. One minor
nit, is that I would "and" in port.m_inbound_transparent_p when setting the
transparent allowed flag in SSLNextProtocolAccept. It is just a bit of extra
error checking. If the connection is not at least inbound transparent,
transparent passthrough makes no sense. This is checked via the HttpSM method
is_transparent_passthrough_allowed on the Http path.
The check for the CLIENT_HELLO handshake operator in the SSL error case
seems fine. You may get unlucky and have non SSL traffic with a first byte of
0x16, and not blind tunnel it. But checking for more bytes gets complicated
(e.g. checking that the next byte is not any valid SSL version). In practice
the simple check is likely fine, and it is highly unlikely that you will suck
in legitimate SSL handshake exchanges into the blind tunnel.
I'm going to take it for a spin on my dev box, and then merge it up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
