On 09/07/17 15:28, Alex Rousskov wrote:
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...
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.
+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. That is out of scope of this patch which
concentrates solely on documenting the border between TLS connection and
TLS session.
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.
The classes in Squid which most accurately match the "TLS connection"
definition are actually class fde and class Comm::Connection. Since fde
stores the FD value, I/O handlers and pointer to an active SSL session
(indirectly via the SSL* pointer), and Comm::Connection stores the FD
and related state/session data.
+ 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.
"
SSL *
Ssl::CreateClient(SSL_CTX *sslContext, const int fd, const char *squidCtx)
{
return SslCreate(sslContext, fd, Ssl::Bio::BIO_TO_SERVER, squidCtx);
}
SSL *
Ssl::CreateServer(SSL_CTX *sslContext, const int fd, const char *squidCtx)
{
return SslCreate(sslContext, fd, Ssl::Bio::BIO_TO_CLIENT, squidCtx);
}
"
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. They have zero properties and similarity with
accept(2) or connect(2) in TCP connections.
*IF* these functions actually did connection semantics then all of the
logic in the class PeerConnector hierarchy is completely wrong.
(you may recall that one of the major mistakes I made very early on
which led down this whole muckup was thinking that these did the
'connect' and PeerConnector did the encryption).
This distinction is supported directly by the fact that this logic in
these functions is identical for clients and servers.
** In no way does a TLS client code in PeerConnector accept(2) a
connection. Therefore this logic is not doing accept(2).
b) Likewise, it is also not performing connect(2) equivalent semantics.
This is where your insistence that TLS connection and TCP connection are
equivalent falls down completely. Because a TLS connection _exists_ as a
result of these functions, but cannot be used for I/O.
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".
In fact the more I think about it now, these functions are somewhat a
TLS equivalent of socket(2). Since a socket/FD has to be allocated
before a connection can begin to be actively started, and lots of
behaviour flags etc get applied to the socket/FD after creation and
before first use to manage the connections eventual behaviour.
For lack of anything actually suitable I bowed to your request to retain
the Squid-3 terminology here. That being the words Create and
Client/Server rather than connect/accept/listen/prepare etc.
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::
Lets talks a short side-step here and consider the Comm::Connection
object design. 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.
With "TLS connection" being defined as being a pair [transport
connection, TLS session]. The TLS connection "object" is most correctly
typed as either std::pair<Comm::Connection, Security::SessionPointer>,
OR adding a Comm::Connection with member pointing to the TLS session.
(ignoring for now that SessionPointer is currently borked).
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.
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.
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. In the long term end product they may stay as statics like this
in a class, or be renamed to non-static methods.
"Tls" is being used here to distinguish from Ssl:: things, but as you
point out we have not gone far enough to warrant a Tls:: namespace yet.
I do fully expect that there will be other security types before too
much longer. Specifically some Security::SslFoo things. Then there is
SCTP etc.
So making the name fully descriptive and avoiding even more renames
seems to be a good choice. There should be no harm in documenting that
these were created to be TLS specific things.
+/// 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.
Oops. Yes. That first should be "TCP". Though using "transport" now as
per below change.
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.
ie. "Client connection structures" refers to TLS connection structures
where Squid is the client.
The TLS terminology, Squid terminology, library APIs seem to align. Only
the BIO enum naming uses server/client the other way around, and those
explicitly state "_TO_" for clarity. So I don't seen much in the way of
confusion here.
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.
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.
Done.
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/
Done.
+ * 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".
Nouns. The TLS data structures are not being associated with a TLS
connection, they *are* the "Client TLS" or "Server TLS" part that makes
the transport turn into a "Client or Server TLS connection"
collective/aggregate thing.
... 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.
That is the distinguishing factor for client vs server at this level of
TLS. It relates directly to what is correct handling of the BIO
TO_SERVER/TO_CLIENT parameter sent to this function.
Sadly that is as detailed as they are. Quoting in full in part documents
where the line is between the fuzzy formal definitions and anything we
tack on to, as you said earlier, 'fill the gaps'.
- // 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).
Okay, done.
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.
* The layering violation is in requiring the caller / semantics layer to
be aware of TLS syntax and how issues there need to be handled.
* The RFC violation is that the caller does *not* perform the close due
to the protocol syntax errors. It seems to instead die on semantic
interpretations of the bad syntax, which might be leaving nasty security
holes on some garbage.
The TLS layer already is aware that there is a transport (relatively
opaque in type though). So it is already in a position to close() that
transport as required in the spec and let the caller do whatever its
handler was for that.
The caller already has a close handler in place to do its own "stuff"
when TLS layer aborts with the close().
Thank you
Amos
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
