[squid-dev] [PATCH] PeerConnector shuffling to libsecurity

[Alex Rousskov]

On 04/14/2016 06:23 AM, Amos Jeffries wrote:
> This patch shuffles the Ssl::PeerConnector to Security::TlsPeerEncryptor
> and Ssl::BlindPeerConnector to Security::BlindTlsPeerEncryptor.

I have already given up on fighting you about pointless and inconsistent
SSL/TLS/Security renames, but please at least do not nest layers until
we actually need that nesting: Use either Security::PeerEncryptor or
TlsPeerEncryptor, not Security::TlsPeerEncryptor. I recommend
Security::PeerEncryptor because the API users do not care much about the
protocol being used.


> +#elif USE_GNUTLS
> +    // XXX: no GnuTLS support for cert validation helper yet
> +#endif
...
> +#elif USE_GNUTLS
> +        // TODO
> +#endif
...

I object to ongoing pollution of innocent code with USE_GNUTLS that
cannot be used: Having to deal with USE_OPENSSL is bad enough. Please do
not quadruple the problem by adding [unused] #USE_GNUTLS everywhere!

> +#if USE_OPENSSL
>      if (serverConnection()->getPeer() && !SSL_session_reused(ssl)) {
...
> +#elif USE_GNUTLS
> +    if (serverConnection()->getPeer() && !gnutls_session_is_resumed(ssl)) {
> +#endif

The above is just one of many examples of how GnuTLS support should
_not_ be introduced. The above code should look like this:

  if (serverConnection()->getPeer() && ssl->resumedSession())
    ...

or, at the very least, like this:

  if (serverConnection()->getPeer() && !session_is_resumed(ssl))
    ...

No duplication of logic, no constant thinking of what API each library
uses, no #ifdefs, and no invasive predictions about what GnuTLS code
will need when GnuTLS support is actually added. If you are not ready to
add GnuTLS support the right way, please do not add GnuTLS support at all!

The "wrong way to move towards GnuTLS support" problem discussed above
is the biggest problem with the proposed patch that I can see. Please do
not commit until that problem is resolved.


> This shuffling is a required step to simplify converting the basic TLS
> I/O logic to libsecurity and for GnuTLS to actually begin adding useful
> implementation bits.

This required step was done in the wrong direction IMO: The proposed
patch adds a dozen new "#if USE_"s. If we are moving "basic TLS I/O
logic to libsecurity", then we should be reducing the number of ifdefs,
not increasing them. Is this is a necessary pain to see some beautiful
results in the future? Should not we wait with the pain until that
future becomes a reality so that we can judge whether it is worth the pain?



> -#if USE_OPENSSL
> -    /// Gives PeerConnector access to Answer in the TunnelStateData callback dialer.
> -    class MyAnswerDialer: public CallDialer, public Ssl::PeerConnector::CbDialer
> +#if USE_OPENSSL || USE_GNUTLS
> +    /// Gives Securit::TlsPeerEncryptor access to Answer in the TunnelStateData callback dialer.
> +    class MyAnswerDialer: public CallDialer, public Security::TlsPeerEncryptor::CbDialer

I understand why MyAnswerDialer was wrapped in USE_OPENSSL before -- it
was using an Ssl::PeerConnector, and stuff in src/ssl/ is not generic.
However, now, it is using Security::TlsPeerEncryptor and src/security
API is supposed to be OpenSSL-neutral. Can we remove USE_OPENSSL from
this (and possibly some other) tunnel.cc parts now instead of adding
USE_GNUTLS?


> -class IcapPeerConnector: public PeerConnector {
> +class IcapPeerConnector: public Security::TlsPeerEncryptor {

Why was not this class renamed as well?


> -Ssl::CreateClient(Security::ContextPtr sslContext, const int fd, const char *squidCtx)
> -{
> -    return SslCreate(sslContext, fd, Ssl::Bio::BIO_TO_SERVER, squidCtx);
> -}

That function seemed like a perfect pair for the CreateServer() which
you did not remove. Why introduce such inconsistency and bother callers
with such ugly details as BIO_TO_SERVER constants?


> -               theName.termedBuf(),static_cast<int64_t>(theSize), xstrerr(savedError));
> +               theName.termedBuf(), static_cast<int64_t>(theSize), xstrerr(savedError));

Out-of-scope formatting change.


> -    if (!Comm::IsConnOpen(serverConnection()) || fd_table[serverConnection()->fd].closing())
> +    if (!Comm::IsConnOpen(serverConnection()))

Out-of-scope optimization(?) change.

> -    if (request != NULL)
> +    if (request)

Out-of-scope code style change.


There may be more out-of-scope changes; I stopped looking.


Alex.