[squid-dev] [PATCH] Refactor TLS session extra data management

[Alex Rousskov]

On 04/22/2017 06:52 AM, Amos Jeffries wrote:
> On 21/04/17 15:58, Alex Rousskov wrote:
>> On 04/20/2017 04:50 PM, Amos Jeffries wrote:
>>> These two patches begin the refactoring work to replace OpenSSL
>>> SSL_set/get_ex_data() API with a generic libsecurity API. 
>> Thank you for working on this. I think the patch is on the right track
>> overall, but I found one bug, and I believe the proposed GetFrom() API
>> needs to be revised. I also object to adding any code that calls TLS
>> connections "sessions", especially when we already have code that
>> calls TLS sessions "sessions". If at all possible, please fix the
>> naming in the existing code (as already discussed elsewhere) before
>> adding any new stuff that makes the problem worse. 

> Lets not get bogged down in that again.

Let's not, but I do not understand what you are suggesting we do
instead. The problem exists, and your patch makes it worse. It is a
serious problem. I have made specific solution suggestions (and welcome
any better alternatives).

AFAICT, the previous patch with these problems was committed despite my
objections (v5 r15032). To avoid further misunderstanding, I have to
register my vote for this new set of patches as -1. Please do not commit
them until that vote changes.


>>> +/// extra data which we attach to a TLS session 
>> s/session/connection/ 

> No this data is explicitly not part of the "TLS connection". It persists
> long after the "TLS connection" is ended and may be shared across
> multiple if and when the "TLS session" does so.

We attach these extras to a TLS connection, not session (the OpenSSL SSL
object, not the OpenSSL SSL_SESSION object).

I agree that some future code might share some extras across multiple
TLS connections, but since most of the current extras deal specifically
with the first connection in a session, and since sessions have their
own metadata, it is not unlikely that the shared/session information
will be different. We will add session extras as needed.

Your patch is about moving individual connection extras into a single
ConnectionExtras object. Please do not enlarge the patch scope to
sharing extras among connections!


>>> +/// class ExtraData 

>> Please avoid names that end with State, Data, Info, Object, Status,
>> and similar noise/placeholder words without any specific meaning. In
>> this particular case, the class should be named ConnectionExtras and
>> described as "Squid-specific TLS connection details" or something like
>> that. The variables holding class objects should then become "extras"
>> instead of "data". 

> OpenSSL calls this "ex_data" and GnuTLS calls it a "datum pointer", 

Yes, for both libraries, our extras are just opaque "data", but it is
not opaque data to us! Besides, IMHO, the libraries should not have used
"data" for these APIs either, but that is their problem, not ours.


> in both cases it is documented as being attached to a session. 

OpenSSL attaches relevant extras to the SSL object. The SSL object
represents a connection, not session. The session is represented by
OpenSSL SSL_SESSION. We have discussed this before, including
http://lists.squid-cache.org/pipermail/squid-dev/2017-February/007959.html


> They are definitely NOT destructed when the connection is ended.

The extras are destructed when the SSL object is freed (i.e, when the
TLS connection has ended).


>>> + /// \retval nullptr only if session is nullptr + static
>>> Security::ExtraData *GetFrom(const SessionPointer &); 
>> The patch contains: * code that asserts if GetFrom() returns nil *
>> code that crashes if GetFrom() returns nil * code that pointlessly
>> checks whether GetFrom() returns nil I did not see any code that
>> actually expects a nil connection pointer and can handle it correctly.

> Specific alternatives would be useful.

Already provided: "change the method to accept a reference to the
connection (instead of a pointer) and return a reference to
ConnectionExtras (instead of a pointer)." In OpenSSL terminology, that
would be:

    ConnectionExtras &Of(SSL &connection);


>> The method itself does not store the TLS connection so it should not
>> need a pointer to one.

> Since when was it a requirement that all function parameters were
> stored? 

There is no such dubious requirement, of course (and using a clearly
unreasonable interpretation of a suggestion is a waste of time).

In general, when dealing with an object that cannot be simply copied,
there are three relevant cases:

1. optional info: the function can do its primary job w/o the object
2. stored info: the function needs to store the object somewhere
3. used info: the function uses the object without storing it

Cases #1 and #2 should use pointers. Case #3 -- a reference.
ConnectionExtras::Of() is case #3.

We violate that general principle in many cases, and sometimes for a
good reason, but there is no good reason to violate it in this case
AFAICT (and, as I have documented, there are good reasons not to violate
it).


>> To avoid multiplying these problems beyond the patch, please change
>> the method to accept a reference to the connection (instead of a
>> pointer) and return a reference to ConnectionExtras (instead of a
>> pointer).

> Er, did you read the pt1 code?

I did.


> The pointer received is passed on to the
> underlying library API. Using a reference to an object instance of
> unknown type is not doable. And we also agreed that mixing smart and raw
> pointers in our API was a bad idea.

The method code will still pass a raw SSL object pointer to the
underlying OpenSSL APIs, of course. There is no problem with doing that.
(I bet those APIs themselves would have used a connection reference
instead of a pointer if C had that mechanism).


> The connection is not accessible in most places this function is called.

In your current patch terminology (AFAICT), the connection is the object
returned by the "*session" expression, where "session" is the
Security::Extras::From() parameter. In OpenSSL terminology, the
connection is the SSL object used by that method.


> That code path is untested by me, yes. I cannot test the OpenSSL code
> changes, and thought I'd stated that enough by now. 

Please do identify any significant untested areas in every patch. It is
a fair request because it is much easier for you to disclose the problem
in every patch then for a reviewer to remember what you currently can or
cannot do, and then identify which areas have been tested and which have
not been, especially when you are changing general security code (that
just happens to lack GnuTLS support but is not otherwise specific to
OpenSSL). Knowing what has been tested also helps with future triage,
when it would be especially difficult to reconstruct what you could or
could not test at the time of the commit!

And, IMHO, you should not make TLS-related changes if you refuse to
properly test most of them.


>>> + Security::SessionPointer session(ssl, [](SSL *){/* do not destroy
>>> */}); 
>> This ugly hack will no longer be needed after the above changes AFAICT. 

> No such luck. At least it is part of the callback state preparation.

AFAICT, if you implement the changes I suggested, the hack should go
away because there will be no reason to create an SessionPointer object.
You will simply use "*ssl" expression to get access to the extras:

    /* const */ auto extras = ConnectionExtras::Of(*ssl);

>>> +class ExtraData +{ +public: + SBuf sni; 
>> This member may not actually contain an SNI extension value (neither
>> sent nor received). Let's not lie to ourselves and call it serverName.
>> Please document new data members. Consider: /// suspected or actual
>> connection destination (from various sources) SBuf serverName; 

> Sigh. Ironicly it started as that. Then realized the value being stored
> there was SNI or an SNI-to-be value.

The last part seems wrong to me: As I said, AFAICT, there are cases
where serverName was not SNI and does not become SNI.


Alex.