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

[Alex Rousskov]

On 04/26/2017 06:53 AM, Amos Jeffries wrote:
> Been digging around in library guts and yes you were right Alex - for
> OpenSSL and the very old ones based on SSL protocol. Just not for the
> other N libraries based on TLS design which I/we hope to support. There
> is structural difference between the two protocols layering and activities.

I am afraid you still do not fully understand the TLS connection concept
if you think our disagreement can be resolved based on how library X
calls something (or if you think that SSL connections exist but TLS
connections do not). The existence of a TLS connection in Squid context
is real, regardless of how libraries distinguish TLS sessions from TLS
connections. I used OpenSSL's SSL type only as one of many ways to
"point" to that concept.

(definitions for TLS connection and session concepts are further below)

We do not control 3rd party library designs. Our task is to map Squid
concepts to library concepts. Depending on the library, sometimes this
mapping looks "natural" and sometimes it does not. Library mappings is
an important but secondary problem compared to the primary problem of
identifying key concepts such as TLS connections and sessions.

I feel that the initial patch tries to avoid using "TLS connection" as a
valid concept. This approach is painful for me to watch because, on one
hand, the patch is fixing a few names, but, on the other hand, it
shovels your dissatisfaction into most fixes. If you are willing to fix
this even though I have not convinced you that TLS connections (as a
concept) exist, then pretend that they do. You cannot fix this properly
while pretending that they do not exist!

Finally, while GnuTLS calls its primary handler gnutls_session_t, it
clearly accesses connection (not session!) associated details through
that handler (e.g., GNUTLS_DATAGRAM and GNUTLS_NONBLOCK options have
nothing to do with TLS sessions but are set using gnutls_session_t). As
GnuTLS adds more TLS connection-specific features, you may feel more
comfortable with the idea that TLS connections exist even if the GnuTLS
primary handler is named after a different concept.


> I'm labeling the patch revert, but it is not a true revert as that would
> unwind much other progress and bug fixes. 

I agree that it is too late to revert the commits that called TLS
connection a "session". This patch starts fixing names and descriptions
of things related to TLS connections and sessions, and that is what we
should be doing at this point. Thank you.


> -Ssl::IcapPeerConnector::initialize(Security::SessionPointer &serverSession)
> +Ssl::IcapPeerConnector::initialize(Security::ConnectionPointer &serverSession)

and

> -Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession)
> +Security::BlindPeerConnector::initialize(Security::ConnectionPointer &serverSession)

s/serverSession/serverConnection/ or even simply
s/serverSession/server/

Please check for other similar occurrences -- no ConnectionPointer
object should be called "session" after these changes, including
GnuTLS-only code and debugs() text.


> +    Security::ConnectionPointer theAnSsl(fd_table[fd].ssl);

No "the" prefixes for local variables please. Those prefixes should be
used exclusively for (old) class data members. (Not to mention that
following "the" with "a[n]" does not make sense because the two
determiners are mutually exclusive.)

s/theAnSsl/ssl/ or, if you prefer,
s/theAnSsl/tls/ or
s/theAnSsl/tlsConnection/


> +// When Squid is built with GnuTLS this is a handle used to reference the TLS session.
> +// When Squid is built against OpenSSL this is a pointer to a TLS connection,

The above text tries to make the concept of a TLS connection depend on
the library. The concept exists (in Squid) regardless of the library
used. We need to define and use that concept uniformly across all
libraries instead of constantly asking ourselves "Wait, which library
has created this Connection pointer?".

I suggest this replacement text (shown without proper formatting):

// TLS connection is TLS info about communication over a
// single transport connection between two peers.
// TLS session is TLS info about security arrangements between two
// peers. Those two peers may stop and later resume communication.
// TLS sessions are typically created inside TLS connections.
// A TLS connection may reuse/resume a previously created TLS session.
// With session resumption, many concurrent TLS connections may be
// associated with a single TLS session.

/// points to a library-specific object for TLS connection manipulation
#if USE_OPENSSL
typedef std::shared_ptr<SSL> ConnectionPointer;
...

The top comment blob applies to both security/Session.h and
security/Connection.h. I suspect that the best location for that comment
is actually security/Context.h (and we should add text that defines what
Security::Context is, of course).

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.


> +//  ... which gets indirectly de-referenced to manage the TLS session state.

Please remove this sentence. OpenSSL uses the Connection pointer for
more things than access to the underlying TLS session.


> + * Initializes the data structures for a TLS session with a remote server/peer
> + * and associates them with the given transport connection.

Given the definitions provided above, this description becomes (shown
here without proper formatting):

/// Creates a TLS connection for communicating with a remote server/peer
over the given opened transport connection. Stores the result in
fd_table[].ssl. Does no I/O (e.g., does not initiate a TLS handshake).

(I also added/polished details after the first sentence.)


> + * Initializes the data structures for a TLS session with a remote client
> + * and associates them with the given transport connection.

Similarly (except servers do not initiate handshakes):

/// Creates a TLS connection for communicating with a remote client over
the given accepted transport connection. Stores the result in
fd_table[].ssl. Does no I/O (e.g., does not read the TLS client handshake).

Since CreateClient does not create a client and since we often use
"client connection" to mean the opposite of what CreateClient creates, I
suggest:

  s/CreateClient/PrepareToConnect/
  s/CreateServer/PrepareToAccept/


> +/// 'Close' a given TLS connection (if any) by sending the shutdown/bye notice.

Please remove the quotes around Close. "Close" is the correct term! What
this description is missing is whether this TLS closure is synchronous.

* If it is asynchronous, then the correct description would be something
like this:

/// Initiates TLS connection closure by sending a close_notify alert.
/// Squid may still receive TLS records from the peer after this call.

* If it is synchronous, then the correct description would be something
like this:

/// Synchronously closes the TLS connection. No communication over this
TLS connection is possible after this call.


> +inline Security::ContextPointer
> +GetFrom(Security::ConnectionPointer &p)

This API is wrong -- one can "get" many different things from a
connection and C++ cannot overload based on function return types. This
function should be named ContextOf() instead.

I also suspect (but have not checked) that it should return the context
instead of an extremely dangerous locking-unaware pointer that pretends
to be locking-aware.

I do not know whether fixing either of those flaws is in scope. If you
have to change callers for other reasons, then renaming (at least) is.


> +Security::ConnectionPointer NewSslObject(const Security::ContextPointer &);

s/NewSslObject/NewConnection/


>      /// Calls parent initialize(), configures the created TLS session object

s/session object/connection/

> +        errAction = "failed to allocate handle";
...
> +        errAction = "failed to initialize session";

Keep these identical or at least symmetric. Both refer to exactly the
same error from the caller point of view. For example:

* failed to create a TLS connection handle
* failed to create a TLS connection handle

The fewer differences we have between GnuTLS and OpenSSL code, the
better, but, if insist on propagating the exact library used here, then
something like this may work:

* OpenSSL failed to create a TLS connection handle
* GnuTLS failed to create a TLS connection handle

The fact that GnuTLS calls the handle type "gnutls_session_t" is
irrelevant from the library-agnostic Squid code point of view, and that
errAction variable is for that library-agnostic code. The
Security::ErrorString() will expose the necessary library-specific
details...


> +                peer->secure.updateSessionOptions(handle);
> +            else
> +                Security::ProxyOutgoingConfig.updateSessionOptions(handle);

s/updateSessionOptions/setOptions/

This renaming avoids doubts whether anything in the parsedOptions data
member may affect the TLS connection rather than just the TLS session.

The updateSessionOptions() implementation should refer to the handle as
connection, of course.


Please grep recent changes for other cases where the code may be using
the term "session" instead of "connection" in TLS context.


Thank you,

Alex.