[squid-dev] [PATCH] initial GnuTLS support for encrypted server connections

[Alex Rousskov]

On 01/19/2017 12:16 AM, Amos Jeffries wrote:
> On 15/01/2017 8:09 p.m., Alex Rousskov wrote:
>> I am trying to understand how a standard std::shared_ptr can co-exist
>> with OpenSSL locking.

> whenever the shared_ptr has a non-nil value the library lock count is >= 1.

Understood, thank you. This looks safe indeed. I do not like that we
make it possible, even trivial, to create a bogus OpenSSL SessionPointer
object that does not call SSL_free() but it may be difficult to fully
address that.


> +    Security::SessionPointer session(SSL_new(ctx.get()), [](SSL *p) {
> +            debugs(83, 5, "SSL_free session=" << (void*)p);
> +            SSL_free(p);
> +        });
> +    debugs(83, 5, "SSL_new session=" << (void*)session.get());

and

> +    Security::SessionPointer ssl(SSL_new(ctx.get()), [](SSL *p) {
> +            debugs(83, 5, "SSL_free session=" << (void*)p);
> +            SSL_free(p);
> +        });
> +    debugs(83, 5, "SSL_new session=" << (void*)ssl.get());


To avoid code duplication and to reduce the chances of bogus
SessionPointer creation, please add an Security::MakeConnection()
function that does the above and returns Security::SessionPointer.
Please double check that all SSL_new() calls in the code happen inside
the new wrapper after your changes.



>> Does the patched code continue to work well with OpenSSL?

You have not answered this question. Please do not commit these changes
until the OpenSSL build is tested.


>>> +#if !USE_OPENSSL && USE_GNUTLS
>>> +typedef std::unique_ptr<struct gnutls_priority_st, HardFun<void, gnutls_priority_t, &gnutls_priority_deinit>> ParsedOptionsPointer;
>>> +#else
>>> +typedef std::unique_ptr<long> ParsedOptionsPointer;
>>> +#endif
>>
>> It looks strange that GnuTLS is OK with std::unique_ptr<long> as a
>> ParsedOptionsPointer when built together with OpenSSL but not when built
>> alone. I doubt that is what you meant, but that is how this #if
>> statement reads.
> 
> That is exactly what I meant there and elsewhere with the same.

Your explanation shows that this is not quite what you meant here. You
meant that GnuTLS is not used [as far as ParsedOptions are concerned]
when OpenSSL is available. This code does not reflect that intent well.
Please rewrite along these lines:

#if USE_OPENSSL
    typedef long ParsedOptions;
#elsif USE_GNUTLS
    typedef std::shared_ptr<...> ParsedOptions;
#else
    // we never parse/use TLS options in this case
    class ParsedOptions {};
#endif

Replacing the empty class with "long" is OK if really necessary, but
please do use three separate clauses for the three separate cases.

Any #if !USE_OPENSSL && USE_GNUTLS statements without the #else part can
stay the same because they are clear enough. However, please consider
clarifying them and avoiding future code repetition by adding a
USING_GNUTLS_ALONE macro.


[ BTW, please do not misinterpret my comments as being content with
supporting builds using both SSL libraries at the same time. Allowing
that complexity was wrong IMO, but it is probably too late to revert
that, and this thread is probably not the right place to discuss that
decision anyway. ]


> Double-checking that I've found a few that were not. They are now also
> fixed in this updated patch.

I found one more case where USE_GNUTLS overwrites USE_OPENSSL: The
BIO_TO_* declarations in src/security/forward.h.

I only checked the patch, not the rest of the code though.


>>> +        fatalf("Unknown TLS option '%s'", err);

>> s/option/option near/ -- the err does not isolate the unknown option name.

> The err is pointed to the start of the option(s) which is unknown.

I know. From the error message reader point of view, it may not print
the unknown TLS option though. It may print more than that. For example,
the following would be misleading for an admin that knows that both
SECURE256 and EXPORT are valid/known TLS options:

    FATAL: Unknown TLS option 'SECURE256,EXPORT'

The "near" or "starting at" would improve this:

    FATAL: Unknown TLS option near 'SECURE256,EXPORT'

In fact, I would probably relax it even further by saying

    FATAL: Unable to parse GnuTLS options near 'SECURE256,EXPORT'

Not a big deal, of course.


> The equivalent OpenSSL logic does the same output, but for each
> individual broken option as an ERROR message. And does not exit with
> fatal. Which is unfortunately not [yet] possible for GnuTLS.

AFAICT, it is possible but you decided not to implement it that way. I
think your decision was the right one: Relying on the built-in GnuTLS
options parser is probably better than adding our own parsing bugs.

BTW, squid.conf.documented is missing a reference to GnuTLS options
AFAICT. I doubt we should document each supported option, but we should
tell the reader that GnuTLS options are not OpenSSL options, give an
example, and point to the right external GnuTLS/OpenSSL documentation
for details.


>>> +Security::PeerOptions::operator =(const Security::PeerOptions &p)
>>> +{
>>> +    sslOptions = p.sslOptions;
>>> +    if (!sslOptions.isEmpty())
>>> +        parseOptions(parsedOptions); // re-parse after sslOptions copied.
>>
>> Missing parseOptions reset when p.sslOptions are empty but
>> this->sslOptions where not.
> 
> Done by removing the if and using shared_ptr. Same in copy-constructor.

How about defaulting both the custom assignment operator and the custom
copy-constructor instead? If their current implementation is correct,
then I do not see why those two methods need a hand-written
implementation, and I see that their current implementation is badly
violating flags_[::YesNoNone] boundaries.


>>> +/// close an active TLS session.
>>> +/// set fdOnError to the connection FD when the session is being closed
>>> +/// due to an encryption error, otherwise omit.
>>> +void SessionClose(const Security::SessionPointer &, int fdOnError = -1);
>>
>> Please split SessionClose() into two different functions, one for each
>> use case (initiating proper SSL connection closure vs. immediate SSL
>> connection state destruction). Mixing the two actions results in
>> confusing API instructions and dangerous usage/implementation IMO.


> Well, there is no such thing as a "SSL connection" - it is security
> added onto some *other* Transport Protocol's layer.

There is. The "security added onto some other Transport Protocol's
layer" is called SSL connection. It is not a TCP connection, of course.
It is an SSL connection. See RFC 5246 for numerous examples of this
usage. Calling that connection a "session" in Squid sources is abomination.


> I agree 'close' is a bad word there, but doing what you suggest assumes
> there is a connection to close and/or data is being destroyed.
> Neither of those is actually happening.

On the contrary, that is exactly what is happening. The SSL connection
is being closed and the associated state will be destroyed (which may
have nothing to do with the underlying TCP connection). Using the wrong
terminology makes things a lot more difficult than they really are.


>> I suggest StartSessionClosing() and DestroySession() names (while going
>> along with the current terrible choice of calling both SSL connections
>> and SSL sessions "sessions" which leads to code that close an SSL
>> "session" while reusing the associated SSL session!).

> There is no explicit destroy for SSL sessions. 

I think you are talking about SSL sessions while the corresponding code
and I are talking about SSL connections [that Squid calls SSL "sessions"].


If the fixes mentioned in this email are implemented without surprises,
then I do not think this patch warrants another review cycle.


Thank you,

Alex.