On 12/01/2017 06:48 μμ, Alex Rousskov wrote:
On 01/12/2017 08:35 AM, Christos Tsantilas wrote:
The patch fixes Squid to peeks (or stares) at the origin server as
configured, even if it does not recognize the client TLS
record/message.
s/to peeks (or stares)/to peek (or stare)/
I agree that this is the right thing to do, but I have some concerns
regarding _how_ we are doing that peeking or staring during step3 when
we essentially failed to get ClientHello during step2. Specifics are below.
The documentation of the patch is confusing, sorry.
If parsing ClientHello failed, and we are not able to recognize a
TLS/SSL hello message , we are not bumping.
The on_unsupported_protocol is used to decide how squid will handle this
request.
I need to recheck how squid-3.5 handle these cases.
if (!helloBuild && (bumpMode_ == Ssl::bumpPeek || bumpMode_ ==
Ssl::bumpStare)) {
+ // it is an SSL Version3 message and it is a Handshake/Hello message
+ Must (buf[1] >= 3 && buf[0] == 0x16);
If this is a Must() now, then we should not combine the two conditions
so that we know which of them has failed if one of them does. Also, we
should check the size of the buffer before accessing its contents!
Finally, it would be good to explain the odd (but correct) order of
checks. Here is a sketch:
ok
/* check that we have a v3+ record containing ClientHello */
Must(size >= 2); // enough for version and content_type checks below
Must(buf[1] >= 3); // record's version.major
// only now we know that we are dealing with supported record format
Must(buf[0] == 22); // TLSPlaintext.content_type == handshake in v3+
It is not 100% clear to me why we expect these checks to pass now. It
probably has something to do with the fact that we are inside BIO (and
so we expect to deal with SSL traffic) but what if this is an
unsupported SSL version, session resumption, or some other situation
where there is no ClientHello? Why do we demand a ClientHello presence
here? But only in peek or stare modes? I think we need to add a comment
to clarify all that.
Actually these checks are not really needed. To fail here it must be
something very wrong.
The buf buffer actually holds the TLS Hello message built by OpenSSL
subsystem. The Must here actually checks if this is really a TLS/Hello
message.
The OpenSSL generated message may replaced by the client hello message
for stare mode, or leave it as is for peek mode.
On a related note, did not we just try to parse the same information
already when looking at ClientHello? Why not use that parsed info
instead of re-parsing the same bytes? And if we did not try to parse
these bytes earlier (for one reason or another; e.g., server-first
bumping?), perhaps we should not try parsing it here either??
We are not actually parsing anything. It is just a check, but not on the
already parsed client hello.
+ auto ssl = fd_table[fd_].ssl.get();
+ if (ssl) {
If possible, merge into one statement to avoid leaking the ssl pointer
name outside the if-statement that ensures it is not nil.
ok
static bool
adjustSSL(SSL *ssl, Security::TlsDetails::Pointer const &details, SBuf
&helloMessage)
{
+ if (!details)
+ return false;
...
}
... but then ...
+ helloMsg.append(clientHelloMessage);
If details (i.e., clientTlsDetails) are missing, then
setClientFeatures() was not called. If setClientFeatures() was not
called, then clientHelloMessage would be missing as well, but we are
using it unconditionally above, even when adjustSSL() returns false.
This does not add up IMO. Perhaps we have to require that
clientTlsDetails are set before we use it?
The adjustSSL is always return false as is now unless the
SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK is set.
The "if(!details) return false;" should moved inside
SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK #ifdef.
The setClientFeatures is always called on peek and stare modes.
// If we do not build any hello message, copy the current
if (helloMsg.isEmpty())
helloMsg.append(buf, size);
And if we did build a helloMsg ourselves, then what happens to the buf
contents (that may not match our clientHelloMessage boundary)? For
example, how do we know that buf does not contain just 50% of the client
handshake and that the other 50% would not be later sent after our
We know this because we prevent actually writing data unless openSSL
subsystem generates full hello message. The openSSL buffers the output
and try to write it in one write. If this is change in the future, yes
we may have problem.
handshake, corrupting the stream? I think we know this because our
handshake parser waits until the end of the client handshake, but is it
possible that we are building helloMsg even though our parser has failed
(or was not engaged at all)?
We are talking about Ssl::ServerBio::write. The ServerBio is used in
Squid-to-Server communications (still we are confused from server-side
and client-side names!).
We are not running our parser on Hello messages written to the SSL
server. The client handshake parser is not involved here.
Did I understand your question?
Perhaps (bumpMode_ == Ssl::bumpPeek || bumpMode_ == Ssl::bumpStare)
implies that our parser has succeeded?? If yes, then we need a comment
here to clarify these hidden/assumed dependencies (at least).
Yes actually this is happens. We are here because our parser
succeeds.But I do not think a comment required here. It should not exist
any dependency here.
We need to now the bumping mode, because we have to send OpenSSL
generated client Hello message to SSL server for stare mode, or replace
with client hello message for peek mode. Only this.
We need the clientDetails, because we are trying to emulate the major
TLS settings of the squid client.
Thank you,
Alex.
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
