[squid-dev] Broken trunk after r14735, r14726

[Alex Rousskov]

On 07/20/2016 10:04 AM, Christos Tsantilas wrote:

> Why do we need common types for both SDKs?

You have answered your own question below:


> The only type needed by squid for openSSL is the "SSL *" which is stored
> inside fde class. And the gnutls_session_int for gnutls. These are
> should be enough (maybe with some other like the certificate errors.).
> 
> Squid should use SSL or gnutls_session_int and pass it to openSSL or
> gnuTLS  related classes


The combination of "what to pass" and "where to pass it" is the common
API! There is no sane way to avoid that API if we want to support
multiple SSL libraries.

With that API, instead of writing general code like this:

    #if OpenSSL
        auto x = SSL_foo1(fd_table[fd].openSslConnection);
        SSL_foo2(x, 12345);
        z = AsyncJob::Start(new OpenSslFooDoer(x));
        SSL_free(x);
    #elsif GnuTLS
        auto y = GnuTLS_bar1(fd_table[fd].gnuTlsConnection);
        GnuTLS_bar2(y, 54321);
        z = AsyncJob::Start(new GnuTlsBarDoer(y));
        // y is refcounted
    #else
        throw UnrechableCode(Here);
    #endif

we write either

    z = Security::Baz(fd_table[fd].ssl);

or

    z = AsyncJob::Start(new Security::BazDoer(y));


Designing a high-quality API to achieve the above is very difficult, of
course. We have done some baby steps in a few places, but a lot more
work remains, and some of those changes must be wrong because each
developer doing those steps does not know enough about the problem as a
whole to make the right decisions (and for other reasons).


Note that Security::* is _not_ meant to be a general-purpose SSL SDK! It
is not a wrapper around OpenSSL and GnuTLS. Its primary API is the
boundary between the non-SSL Squid code and the SSL Squid code. Its
secondary purpose is to encapsulate common OpenSSL and GnuTLS code.


> If you decide to implement  NEW GnuTls PeerConnector class, without
> sslbump for the beggining, it is easy. The PeerConnector class is about
> 500 lines of code and the BlindPeerConnector other 100 lines.
> 
> And after this is ready the common code like the certificates validation
> (150 lines of code) can be moved into a parent class.

s/after/during/ -- you do not want to sanction blatant code duplication
without a good reason. And whether that common code belongs to a parent
or some helper class(es) is another open question that the developer
would have to answer.


> But trying to fit GnuTLS and openSSL into the same PeerConnector class
> will fail and will make the code complex.

I do not know whether that is true, but I do know that drawing those
boundaries correctly is a very difficult task, regardless of whether the
correct design is "GnuTlsConnector with PeerConnector parent" or
"PeerConnector with Security::Foo() calls inside".


> The SSLBump also is still under heavy development.
> Again here the PeekingPeerConnector is about 400 lines of code. Is this
> code we are worrying that it will be duplicated?

Frankly, I am worried about any code duplication, regardless of whether
it is 400 lines of super complex SslBump code, 4000 lines of relatively
straightforward SSL code, or something else.


>> 4) designing our code to use an abstraction API that renames all the
>> library structures and functions to some thing we understand easier **.

> I believe that that the PeerConnector is a part of this API.

It could be indeed! In this email, I am not taking a position on whether
it is or it is not.


We agree that making correct decisions in this area is difficult and any
decision requires a lot of knowledge and legwork. IMHO, the Project does
not have the necessary skills and cycles for that right now, but that
lack of resources necessary for a decent outcome has not stopped anybody
before.

Alex.