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

[Alex Rousskov]

On 01/14/2017 10:16 AM, Amos Jeffries wrote:

> The Security::SessionPointer is converted to std::shared_ptr. This is
> required because GnuTLS does not expose the locking like OpenSSL. Since
> we store the SessionPointer to fd_table[].ssl we can always access it
> from there one way or another and there is actually no need for OpenSSL
> locking sessions now.

> -typedef Security::LockingPointer<SSL, Security::SSL_free_cpp, HardFun<int, SSL *, SSL_up_ref> > SessionPointer;
> +typedef std::shared_ptr<SSL> SessionPointer;

I am trying to understand how a standard std::shared_ptr can co-exist
with OpenSSL locking. Even if Squid does not lock the SSL object,
OpenSSL would still lock and unlock it, destroying the SSL object when
its OpenSSL lock counter reaches zero, regardless of our
std::shared_ptr<SSL> state. What am I missing?

Also, if OpenSSL requires that the SSL object is freed using an
SSL_free() function, how can std::shared_ptr<SSL> be compatible with
OpenSSL without that knowledge?

Does the patched code continue to work well with OpenSSL?


>      // Our job is done. The callabck recepient will probably close the failed
>      // peer connection and try another peer or go direct (if possible). We
>      // can close the connection ourselves (our error notification would reach
>      // the recepient before the fd-closure notification), but we would rather
>      // minimize the number of fd-closure notifications and let the recepient
>      // manage the TCP state of the connection.
> +
> +#if USE_GNUTLS
> +    // but we do need to release the bad TLS related details in fd_table
> +    // ... or GnuTLS will SEGFAULT.
> +    const int fd = serverConnection()->fd;
> +    Security::SessionClose(fd_table[fd].ssl, fd);
> +#endif


The "or GnuTLS will SEGFAULT" part is a red flag for me. Squid may close
the connection without or before reaching this particular method (for a
variety of reasons). A connection closure should always be safe. Please
explain why the usual connection closing steps cannot "release the bad
TLS related details in fd_table" [but the above code can].

And why should GnuTLS be treated as a special case here compared to OpenSSL?


> +#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.


> +    // 'options=' value being set to session is a GnuTLS specific thing.
> +#if !USE_OPENSSL && USE_GNUTLS

A similar concern here: If options value being set to session is a
GnuTLS-specific thing, why is the code not executed for GnuTLS when it
is built together with OpenSSL? In other words, why mention OpenSSL in a
GnuTLS-specific code?

I suspect that we can never build with both OpenSSL and GnuTLS enabled
at the same time. If that assumption is correct, then please remove the
!USE_OPENSSL part from these (and any similar) conditions. They only
create unnecessary doubts. And add an OpenSSL-specific conditions
elsewhere as/if needed, of course.


> +#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

Why does OpenSSL need a pointer here? Can we do something like the
following instead?

#if USE_GNUTLS
    typedef std::unique_ptr<...> ParsedOptions;
#elsif USE_OPENSSL
    typedef long ParsedOptions;
#endif


> -                parsedOptions |= SSL_OP_NO_TLSv1;
> +                *parsedOptions |= SSL_OP_NO_TLSv1;

and

> +        SSL_CTX_set_options(t.get(), (setOptions ? *parsedOptions : 0));

and

> +    SSL_CTX_set_options(ctx.get(), *port.secure.parsedOptions);

...


What if parsedOptions is nil here? parseOptions() are not called if
sslOptions are empty and if there are no options=... in configuration so
parsedOptions might be nil in some cases. I do not know whether we are
lucky that they are never nil in these particular cases.

Moreover, should not the options always be initialized, even when
sslOptions are empty, so that we do not need to worry about their
pointer being nil? For OpenSSL where there is no
gnutls_set_default_priority(), but even that code can use empty
sslOptions string to detect that defaults are in use. After all, OpenSSL
was fine with non-pointer options before these changes...


> +    const char *priorities = (sslOptions.isEmpty() ? nullptr : sslOptions.c_str());
> +    int x = gnutls_priority_init(&op, priorities, &err);

Are you sure gnutls_priority_init() can handle nil priorities c-string?
Just checking -- the manual page does not say...

BTW, you can make "x" constant here. Giving it a more telling name like
"result" here and in similar code would be nice too.


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

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


> +        debugs(83, 1, "Failed to set TLS options. error: " << Security::ErrorString(x));

Use DBG_IMPORTANT instead of 1?

It would be nice to print sslOptions that we failed to set (in case of
non-nil parsedOptions) or mention that they were "default" (otherwise).



>  {
> +    if (!sslOptions.isEmpty())
> +        parseOptions(parsedOptions); // re-parse after sslOptions copied.
>      memcpy(&flags, &p.flags, sizeof(flags));
>  }

Have you considered using a shared_ptr for GnuTLS so that we do not have
to reparse when copying? Doing so might also eliminate the need to
change the parseOptions() API and write confusing phrases like

   parseOptions(parsedOptions)

Since OpenSSL and GnuTLS use these options very differently, I doubt
both have to be pointers, but you know better.


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


>  #if USE_OPENSSL
> +        // NP: GnuTLS uses 'priorities' which are set per-session instead.
> +        SSL_CTX_set_options(t.get(), (setOptions ? *parsedOptions : 0));
> +
>          // XXX: temporary performance regression. c_str() data copies and prevents this being a const method
> -        Ssl::InitClientContext(t, *this, (setOptions ? parsedOptions : 0), parsedFlags);
> +        Ssl::InitClientContext(t, *this, parsedFlags);
>  #endif


It feels like the SSL_CTX_set_options() call belongs to the
InitClientContext() function. I can easily imagine somebody moving the
InitClientContext() call while forgetting to move the
SSL_CTX_set_options() call with it. InitClientContext() is supposed to
initialize the context and setting options is one of the initialization
steps...

BTW, is that XXX comment about c_str() stale? I do not see a c_str in
this code...


> +        BIO_TO_CLIENT = GNUTLS_SERVER,
> +        BIO_TO_SERVER = GNUTLS_CLIENT

BIO_WITH_CLIENT and BIO_WITH_SERVER may be better names for
_bidirectional_ I/O. Might as well fix this if you have to touch all the
code using these names anyway.


> +Security::PeerOptions::parseOptions(Security::ParsedOptionsPointer &theOut)

Please do not use the prefix "the" for parameter names. The prefix is
used for [older] class data member names so naming method parameters
with that prefix confuses the reader of the method who keeps looking for
a data member named theOut.

Moreover, if the parseOptions() method is always called with the
parsedOptions argument that is a data member of the same PeerOptions
class, then why do you need to pass parsedOptions as a parameter at all?
The method can just reset parsedOptions directly.


> +/// 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.

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


Thank you,

Alex.