[squid-dev] [PATCH] switch session/connection for OpenSSL

[Alex Rousskov]

On 06/10/2017 06:27 AM, Amos Jeffries wrote:

> Yes this is far from complete, and intentionally much smaller that
> the previous patch.

I believe this patch is a huge step in the right direction. It has one
serious flaw (namespace misuse), but I hope that can be addressed
without too much controversy. The details are further below.


> On 27/04/17 05:24, Alex Rousskov wrote:
>> Needless to say, I would be happy if we can come up with better
>> definitions or even better concepts. The above is a starting point.

> I do not think it is up to us to define these things. So I have taken a
> much longer reading of all the RFCs since SSLv3.0 through to current
> TLS/1.3 and isolated what are the authoritative definitions AFAICT.

For the record, I am happy to adopt any "authoritative" definitions that
suite Squid specifics well, but, IMO, it is up to _us_ to decide
suitability and, if necessary, to define and/or clarify things within
Squid context. Quoting RFCs is often useful, but it is not a panacea for
any problem, including the terminology problem.

For example, a TLS Connection exists, as a concept, whether any RFC
defines or not. An RFC may not need that definition, may assume that
every reader already understands that concept, or may just be sloppy. We
should reuse what we can and fill the gaps with our own definitions as
needed.


> * moves the pieces that are doing what is defined as solely TLS
> Connection things to security/TlsConnection.* files.

Please do not misinterpret the namespace comments below as a
disagreement with the above change. Moving TLS Connection code to
security/TlsConnection.* files is fine with me.


> * adds a Security::TlsConnection::Pointer type for use by code dealing
> with TLS Connection logic.

This is also great.


>  - SessionPointer still exists for code performing TLS Session logic.
> see PeerConnector description for the distinction.

Sure, this is fine. TLS Session is a perfectly valid concept as well.


>  - I have not gone through and renamed uses of SessionPointer beyond
> those directly involved with the above code shuffle.

Can you give an example or two of old uses that should be fixed? Do you
plan on doing this renaming as a part of this series of patches? It
would be nice to get rid of the inconsistencies in one sweep or, if
necessary, several close-in-time changes...


> +namespace TlsConnection {

We should not use namespaces for what is a class. Very roughly speaking:
If you can create multiple instances of X, then X is (or can be) a
class. It is not a namespace. In this context:

* I can have five "TLS connections". A "TLS connection" is (or can be) a
class.

* All TLS-related functions, classes, constants, and other interfaces
can be grouped under a single "Tls" namespace. I cannot have five TLSs.
"Tls" is (or can be) a namespace.

N.B. I do not suggest adding a TLS connection class now (and we already
have library-specific types that currently fulfill that role,
essentially). Using Security::ConnectionPointer is enough.


> +    if (Security::TlsConnection::CreateServer(ctx, conn, "client https start")) {

I would replace TlsConnection::CreateServer, which reads like nonsense
IMHO, with something like:

* Accept
* AcceptConnection
* PrepareToAccept
* CreateConnection(ToSquid)

And the corresponding names for the Squid-to-peer TLS connections:

* Connect
* InitiateConnection
* PrepareToConnect
* CreateConnection(FromSquid)

"Accept" and "connect" are common networking terms that can be applied
to TLS connections well. The last two versions in each group are more
precise because these functions only _prepare_ the TLS connection for
future negotiations; they do not actually start those negotiations.

All these functions should be inside the existing Security namespace. If
you think we will have non-SSL/TLS things inside Security, then we can
also add a Tls namespace, but I do not see the need for that right now.


> +/// Create TLS state objects and associate with a TLS connection to form a TLS connection.
> +/// Client connection structures and TLS/SSL I/O (Comm and BIO) are initialized.


> +/// Create TLS state objects and associate with a TLS connection to form a TLS connection.
> +/// Server connection structures and TLS/SSL I/O (Comm and BIO) are initialized.


I am guessing something went wrong in the first sentences during
find-and-replace ("TLS connection to form a TLS connection"). See below
for a specific fix suggestion.

Also, please avoid the "client connection" and "server connection"
terminology, especially in high-level little-context descriptions like
these, because such terms are very confusing in Squid context. As you
know, we already have "client side" that has "server agents", and
"server side" that has "client agents". To reduce confusion, use
"from-client connection" and "to-server connection" phrasing instead
(or, if you prefer, the verbose "client-to-Squid connection" and
"Squid-to-server connection" variants.

Please consider using these replacements:

/// Creates a TLS Connection and
/// associates it with the given from-client transport connection.
/// \returns false on errors (and logs details using DBG_IMPORTANT)

/// Creates a TLS Connection and
/// associates it with the given to-server transport connection.
/// \returns false on errors (and logs details using DBG_IMPORTANT)


The other comments are about relatively minor problems:


> + * Initializes TLS data structures and associates with a TCP Connection
> + * to form a Client or Server TLS Connection.

> + * Initiates encryption on a TCP Connection to peers or servers.

> + * The caller must monitor the connection for TCP closure because this job will


s/TCP Connection/transport connection/g

s/the connection for TCP/the transport connection for/

because when we start supporting SslBump for HTTPS proxies and/or HTTP/2
for HTTPS proxies, that transport will not be TCP. There is nothing
specific to TCP in that code IIRC.

Please apply similar adjustments to other "TCP" phrase added by the patch.


> + * Initializes TLS data structures and associates with a TCP Connection
> + * to form a Client or Server TLS Connection.

s/associates with/associates them with/

or

s/Initializes ... and associates/Initializes and associates .../

because "associates with" is missing an object in this context
("Associates what?") -- the function itself does not associate with the
TCP connection.


> + * Initializes TLS data structures and associates with a TCP Connection
> + * to form a Client or Server TLS Connection.

I would replace "to form a Client or Server TLS Connection" with "to a
TLS Connection with a Client or Server".


> ... The primary operational
> + * difference between the server and client is that the server is
> + * generally authenticated, while the client is only optionally
> + * authenticated."

I recommend removing that text -- that "primary operational difference"
has little to do with the CreateTlsConnection code.

Overall, quoting fairly generic "client" and "server" definitions feels
like noise to me, but I am not going to fight you about that.


> -    // Temporary ssl for getting X509 certificate from SSL_CTX.
> -    Security::SessionPointer ssl(Security::NewSessionObject(ctx));
> +    // Temporary Pointer<SSL> for getting X509 certificate from SSL_CTX.
> +    Security::TlsConnection::Pointer ssl(Security::TlsConnection::NewSslObject(ctx));

s/Pointer<SSL>/SSL object/

... because the comment is about the temporary OpenSSL SSL object, not
the pointer to it (the local pointer variable is also temporary, but
that is not the point here -- it is the temporary nature of the
underlying SSL object that is rather unusual and worth commenting on).


> PS: Applying the definitions to PeerConnector, it has become clear that
> it (and children) not following a MUST requirement about the underlying
> TCP transport connection being terminated in the case where Handshake
> negotiation failed due to a Record protocol violation. They are leaving
> this closure to the caller which is a layering violation

There is no layering violation in letting the transport-aware caller to
do transport-specific things. I seriously doubt separating handshake
failure detection code from the code reacting to that failure violates
any RFC MUSTs! There are certainly interface problems between
PeerConnector and its users, but they are mostly software problems
unrelated to RFCs/terminology and are outside this thread scope AFAICT.


Thank you,

Alex.