[squid-dev] [PATCH] GnuTLS session redo

[Alex Rousskov]

On 08/05/2016 02:13 PM, Amos Jeffries wrote:
> On 6/08/2016 6:37 a.m., Alex Rousskov wrote:
>> On 08/03/2016 11:57 PM, Amos Jeffries wrote:
>>> +Security::SetSessionResumeData(const Security::SessionPointer &s, const Security::SessionStatePointer &data)
>>> +{
>>> +    if (data) {
>>> +#if USE_OPENSSL
>>> +        (void)SSL_set_session(s.get(), data.get());
>>> +#elif USE_GNUTLS
>>> +        (void)gnutls_session_set_data(s.get(), data->data, data->size);
>>> +#endif
>>> +    }
>>> +}


>> Why cast to (void) here? The current code does not do that AFAICT.
>> Casting the result to (void) usually means
>>
>> * I know this may fail, but I do not care.

>> It should be accompanied by a comment that explains why we do not care,
>> unless the code makes it obvious (the patched code does not).


> Failures there just mean a clean new session is used.

I do not think SSL_set_session() fails when "a clean new session is
used" but perhaps I am misinterpreting what you mean by that.

If you meant that an SSL_set_session() failure is not important because
a new session will be eventually created instead of the old one being
reused, then I agree regarding what will happen but disagree regarding
that failure unimportance. Session reuse is very important in some
environments, so the proposed "I do not care why it did not happen" is
the wrong approach. We do care, at least for triage/debugging purposes.

That kind of logic would be very similar to saying "I do not care that
nothing gets cached due to a failed disk -- the users will get their
responses from the origin server instead". While they will indeed, we
still do care as far as stats/logging/debugging are concerned.


> I dont see any reason to throw. Resume is just a speed optimization for TLS.

We should throw if the callers need to know about the failure. We should
not throw if the callers do not care about the failure. Resume being an
optional optimization is irrelevant in this particular decision --
either the callers have some cleanup/setup/debugging code to accompany
this call or they do not.


>>> +Security::SessionIsResumed(const Security::SessionPointer &s)
>>> +{
>>> +    return
>>> +#if USE_OPENSSL
>>> +        SSL_session_reused(s.get()) == 1;
>>> +#elif USE_GNUTLS
>>> +        gnutls_session_is_resumed(s.get()) != 0;
>>> +#else
>>> +        false;
>>> +#endif
>>> +}
>>> +
>>> +void
>>> +Security::GetSessionResumeData(const Security::SessionPointer &s, Security::SessionStatePointer &data)
>>> +{
>>> +    if (!SessionIsResumed(s)) {
>>> +#if USE_OPENSSL
>>> +        data.reset(SSL_get1_session(s.get()));
>>> +#elif USE_GNUTLS
>>> +        gnutls_datum_t *tmp = nullptr;
>>> +        (void)gnutls_session_get_data2(s.get(), tmp);
>>> +        data.reset(tmp);
>>> +#endif
>>> +    }
>>> +}
>>
>> Can we establish some rules for when to use "#else" in
>> OpenSSL/GnuTLS/Security contexts?

> Sure. So far I've been trying to only add #else when there was some
> meaningful code that needed to be done in those builds.

Not sure I understand the rule you are proposing (if you are proposing a
rule). Can you detail/rephrase it? Please keep the following two
questions in mind when doing so:

1. What are these rules trying to guarantee or accomplish?
   In other words, what will happen if we [do not] follow them?

2. If I see no "#else" (or no "#elif"), what does that mean?
   * We have not checked yet. It may not run or even compile.
   * We know it compiles (and runs?) without #else (or #elif).
   * Something should go there but we have not figured out what yet.
   * We know that nothing should go there; all callers will be fine.
   Etc.


> Usually it is for a default return statement or setting up error results
> indicating the related TLS/SSL feature not working. 

Should the "default return statement" always indicate an error of some
sort? If not, how to know when to implement the default success route
(your first reason) and when to set up an error (your second reason)?


> Code that is only
> run when the feature being used usually does not need #else.

Without a good set of rules, this gets messy very quickly IMO. For
example, your own GetSessionResumeData() does not reset the data
parameter in the [absent] #else case but does unconditionally reset it
(possibly to a nil value) in other cases. Is this a bug waiting to
happen, and intended side effect, or both? Without rules, it is
difficult to say for sure.


> I've built this with the full set of build layers. 

This is not simply a build matter IMO.


> /// Retrieve the data needed to resume this session on a later connection
> /// if it was not itself resumed from earlier details
> void MaybeGetSessionResumeData(...)

We should say what happens if "it was itself resumed". The current
"leave data pointer intact" logic looks very strange to me, but I cannot
tell whether this is a bug in the existing code (that you do not have to
fix, but can expose by adding XXX to your documentation) or intended
[but difficult to understand] behavior.


> Since this is all cosmetic, is/was any testing able to happen on your part?

The rules decisions we should make here are as far from cosmetic
polishing as they get IMO. The understanding of GetSessionResumeData()
side effects may not be cosmetic either.

As for testing, I cannot volunteer for that at this time. And given the
sad state of Squid automated QA, my testing with OpenSSL (or GnuTLS)
would be about as good as what you can do anyway.


Cheers,

Alex.