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

[Alex Rousskov]

On 07/09/2017 04:30 AM, Amos Jeffries wrote:
> On 09/07/17 15:28, Alex Rousskov wrote:
>> On 06/10/2017 06:27 AM, Amos Jeffries wrote:
>>>   - 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...

> class fde "ssl" member. This is a pointer to the TLS session state for
> the TCP connection whose FD the fd_table entry is storing.

fde::ssl is an std::shared_ptr<SSL>. It is trivial to fix the name of
this field type to use the new TLS Connection pointer type. That name
change itself will not require any other other code changes AFAICT. I do
not understand why you place that change outside this patch scope, but I
do not insist on making this change now. Said that, the sooner we fix
the remaining Security::SessionPointer occurrences the better.


> SessionPointer still exists for code performing TLS Session logic

Perhaps I misinterpreted what you meant by the above. To avoid
misunderstanding:

* Security::SessionPointer should be std::*_ptr<SSL_SESSION,...>,
* Security::ConnectionPointer should be std::shared_ptr<SSL>,
* Security::SessionStatePointer should be removed.

(I am omitting the known GnuTLS equivalents for clarity sake.)

In 99+% of cases, there is no ambiguity or complex decision making
involved in deciding which existing code should use which pointer type.
The existing code either works with SSL_SESSION or with SSL, telling us
which pointer type it wants to use. For example, fde::ssl wants the new
Security::ConnectionPointer (or whatever its name will be).

Once we resolve the namespace/naming problem, the remaining part of the
fix is simple, mechanical renaming. If you insist on doing renaming
separately from the primary naming patch, I cannot object to that, but
combining the two seems natural to me.


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

> They might be one day.

The above are facts that should guide our decisions today, not
predictions for future code changes.


> That is out of scope of this patch which
> concentrates solely on documenting the border between TLS connection and
> TLS session.

You patch introduces a new namespace that cannot exist in a sanely
organized code. Not today, not tomorrow. I am _not_ arguing for adding a
TLS Connection type. I only object to adding a TLS Connection namespace.
My objection is in scope.


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

>> I would replace TlsConnection::CreateServer, which reads like nonsense


> Take a look at the naming of this code in Squid-3. These are the names
> you have spent most of a year trying to get me to undo. Now you are
> saying they were nonsense.

I think you are misinterpreting what I am saying.
"TlsConnection::CreateServer" is nonsense. "Ssl::CreateServer" is far
from ideal but does make some sense.


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


> The CreateX() functions are the constructors of the _data structure_ for
> storing TLS context data. 

Yes, that and more.


> They have zero properties and similarity with
> accept(2) or connect(2) in TCP connections.

I disagree. These functions can be viewed as performing some of the very
first steps of standard TLS connecting or accepting sequences. They do
not accept or connect, but they are a part of accepting or connecting
procedure:

N+1. Setup BIO for connecting or accepting.
N+2. Configure TLS connection with the right BIO.
N+3. Call SSL_connect() or SSL_accept() on that TLS connection.

If we were dealing with TCP at the same low level, we would also have
constructors that prepare the TCP connection object for the next steps
in the standard TCP handshake sequence.

The PrepareToAccept() and PrepareToConnect() variants reflect the above
better than "pure" Accept() and Connect(). Just pick what you think
works best or suggest a better alternative.


> ** In no way does a TLS client code in PeerConnector accept(2) a
> connection. Therefore this logic is not doing accept(2).

You lost me here. PeerConnector is not a part of the accepting sequence.
It is a part of the connecting sequence, which works in the opposite
direction. So, yes, PeerConnector is not doing accept(2) or equivalent,
but I do not think anybody has argued that it does.


> b) Likewise, it is also not performing connect(2) equivalent semantics.

If "it" is still PeerConnector then it actually does call SSL_connect()
which has connect(2)-equivalent semantics.


> This is where your insistence that TLS connection and TCP connection are
> equivalent falls down completely.

I have never insisted on such nonsense. The two connections surely have
many similarities, but they are obviously not equivalent.


> Because a TLS connection _exists_ as a
> result of these functions, but cannot be used for I/O.

It _is_ used for I/O. The TLS handshake does not happen magically; it
requires sending and receiving bytes (i.e., I/O). Once a TLS connection
is constructed, it can be used for that handshake I/O.

If your argument is that the handshake must happen _before_ any useful
payload is transmitted, then the same argument applies to a TCP
connection. TCP has a handshake sequence as well, of course.

If you merely state that there are certain stages of a (TLS or TCP)
connection where payload cannot be sent or received immediately, then I
certainly agree but do not see the problem/relevance.


> In order to use as a transport channel a session must also be
> negotiated. That session negotiation is the equivalent of connect(2) -
> yet it does *not* create a "TLS connection" - it creates a "TLS session".

Session negotiation is rather similar to a TCP handshake. TLS session
negotiation happens inside an existing TLS connection. It is a required
step in any connecting or accepting sequence.



> In fact the more I think about it now, these functions are somewhat a
> TLS equivalent of socket(2).

Yes, they are. socket(2) is direction-agnostic so they are even more
like a socket(2)+bind(2) pair: They create a TLS connection and bind it
to the right BIO structure.


BTW, if you have spent all this time to simply illustrate that pure
"Accept" and "Connect" names are far from ideal, then I am sorry we have
wasted so much time. I know those names are far from ideal. I offered
several options for you to pick from (and am open to alternatives).
There is no need to re-highlight the problems with each name (and they
all have some problems).


> For lack of anything actually suitable I bowed to your request to retain
> the Squid-3 terminology here.

I do not recall requesting that. New better names are welcomed. And you
did not retain v3 naming because you introduced a new namespace.


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

> Er, they are in Security::

In your patch, they were not. They were inside Security::TlsConnection.
It is the TlsConnection part that I am strongly against. I added the
above paragraph just because I did not want you to misinterpret my
arguments as if I am against the Security part as well.


> Lets talks a short side-step here and consider the Comm::Connection
> object design.

That would be dangerous because we probably disagree what
Comm::Connection should be. For example, your code is abusing that class
to store connection-related info way before the connection exists and
even before we have plans to open the corresponding connection. I have
tried to object to that but failed to convince you that it is wrong.


> It is supposed to be a generic abstraction for *any*
> transport (including TLS encrypted ones) such that the code using it
> does not need to be aware of whether it was TCP, UDP, SCTP, TLS, DTLS or
> whatever - just pass the Conn object to read/write API with a buffer and
> get that buffer filled or emptied.

I am not sure Comm::Connection is supposed to be that. It certainly is
not today because some code using Comm::Connection knows what kind of
connection it is and cannot use any other kind. For example, giving a
PeerConnector object a TLS connection (instead of a raw TCP connection)
would not magically allow us to support TLS inside TLS.


> With "TLS connection" being defined as being a pair [transport
> connection, TLS session].

That is a usable abstraction in some contexts and an oversimplification
in others. For example, during a TLS handshake, there is no TLS session
(yet), but there is certainly TLS state that cannot be stored in the
transport connection object, whatever that object is.

Fortunately, we do not need to define the type of TLS Connection objects
because the libraries have done that for us. In OpenSSL, that object is
called "SSL".


> The TLS connection "object" is most correctly
> typed as either std::pair<Comm::Connection, Security::SessionPointer>,

That does not work for the TLS connection stages where there is no TLS
session (see above).


> OR adding a Comm::Connection with member pointing to the TLS session.

Same problem (and many more).


> Right now there is also a fuzzy line in the comm code between
> Comm::Connection and class fde, so IMO fde holding the SessionPointer is
> wrong, but that is where it currently has to be placed due to the CommIO
> handling logics. I'm intentionally leaving that tangled web out of scope
> for this patch.

Yes, untangling fde/Connection/TLS I/O is out of scope. Fixing the name
of the fde::ssl member type to become Security::Connection (or
equivalent) is perfectly in scope and requires no untangling at all!


> I did consider making these functions methods of a class TlsConnection,
> but that would easily lead to confusion of thinking that class *was*
> representing a TLS connection and get in the way of future cleanup of
> the above tangle.

A type named TlsConnection has to represent a TLS connection. Today,
that type is provided to us by the libraries. If we need a Squid-native
TlsConnection wrapper tomorrow, we will discuss it tomorrow. Today, such
discussions are probably out of scope.


> So for this patch I am simply adding a namespace placeholder for
> whatever the class will be and ensuring the static functions are in
> there.

I am strongly against adding namespaces as class placeholders. The two
concepts operate on very different layers/levels. One cannot be a
placeholder for another. This is like using Squid software as a
placeholder for a future car because both are going to be purple. It
just does not make any sense!

If you want a placeholder for the future TLS Connection class, you can
add a Security::TlsConnection class that has no non-static members but
has (naturally static) creation methods. Needless to say, to play that
game, you have to treat that placeholder class as a class (and those
creation methods as creation methods) when it comes to their naming and
interfaces. For example, you may have something like this:

class TlsConnection
{
public:
    typedef TlsConnectionPointer Pointer;

    static Pointer ToServer(...);
    static Pointer FromClient(...);

#if USE_OPENSSL
    static Pointer New(...);
#endif
};


Such static-only classes are naturally frowned upon, but if that is the
best we can agree on, I can live with that.


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

> The Client/Server words are following our current Squid-oriented
> (Squid-as-Client and Squid-as-Server) terminology.

Please avoid the "client connection" and "server connection"
terminology, especially in high-level little-context descriptions like
these, regardless of what the Client and Server words mean to you or
should mean to everybody. There is just too many "clients" and "servers"
that mean different things -- we should use qualifiers to be clear,
especially where there is little context to guide us (e.g., we are not
inside some Client class already). Some day, those qualifiers may become
unnecessary.


> ie. "Client connection structures" refers to TLS connection structures
> where Squid is the client.

I understand the terminology, but we should not use it for the reasons
stated above. We should use to-server, squid-server, or from-squid IMO.


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


> Done the \return change. The other doc changes are wrong because the
> CreateClient uses a to-server transport connection to generate a
> to-server / from-Squid TLS connection.

I am sure you can swap the "wrong" descriptions around to match the
"right" names. Hopefully, by the time your patch is applied, the names
will be less confusing.


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

> Maybe I was not clear enough.

I continue to believe that discussing alleged MUST and layering
violations by PeerConnector is outside this thread scope. I see serious
flaws in some of your logic, and I know there are problems with
PeerConnector code, but I refuse to discuss any of that here. If you
would like to continue this part of the discussion, please start a new
thread and, if possible, accompany your claims with specific code quotes
or references, to avoid misunderstanding while trying to interpret vague
terms such as "layering violations" and syntax awareness.



Thank you,

Alex.