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. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
