[squid-dev] [PATCH] GnuTLS session redo

[Amos Jeffries]

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).
> 

That was the intention. Failures there just mean a clean new session is
used.
It seems to only be useful info if we decided to add debugging to these
functions.

> 
> Not casting may mean many things, including:
> 
> * It returns void.
> * I did not even think about it.
> * I know it may fail, but I either do not know how to handle its failure
> or just do not have the time to handle it.
> 
> AFAICT, in this particular case, the current code probably matches one
> of the last two bullets. If I am right, then there are two correct ways
> to deal with it IMO:
> 
> 1. Return a boolean success/failure result.
>    The caller will still ignore the result.
> 
> 2. Remove "(void)". Ideally, add a "// TODO: throw on failures" comment.
> 
> My weak recommendation is for #2.
> 

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

> 
>> +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? The inconsistency illustrated above is
> a red flag. I do not think "use #else if it is needed to compile" is the
> rule used for the above code because Security::GetSessionResumeData()
> may produce an "unused parameter 'data'" warning, killing the build.

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. In the same way
as for eliding not-yet-implemented GnuTLS sections.

Usually it is for a default return statement or setting up error results
indicating the related TLS/SSL feature not working. Code that is only
run when the feature being used usually does not need #else.

I've built this with the full set of build layers. I'm not seeing that
warning/error show up in layer-01-minimal builds where it might. Though
I've only built the patch with GCC 5 and 6 so far, not clang.

> 
>> +/// Retrieve the data needed to resume this session on a later connection
>> +void GetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &);
> 
>> +void
>> +Security::GetSessionResumeData(const Security::SessionPointer &s, Security::SessionStatePointer &data)
>> +{
>> +    if (!SessionIsResumed(s)) {
> 
> The implementation does not seem to match the description. The
> implementation matches the current code, but the description says
> nothing about !SessionIsResumed() precondition. Please either change
> description and rename OR test the precondition in the callers,
> whichever you think is better long-term.
> 

OK. Altering the description and name to
"
/// Retrieve the data needed to resume this session on a later connection
/// if it was not itself resumed from earlier details
void MaybeGetSessionResumeData(...)
"

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

Amos