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. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev