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

[Amos Jeffries]

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.

> Details below.
>> +/// 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.


>> +/// extra data which we attach to a TLS session +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", in 
both cases it is documented as being attached to a session. They are 
definitely NOT destructed when the connection is 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. I'm looking at a mix of code that 
can only occur when a SessionPointer is available [use without checks], 
code that can occur with or without one [check with an if], and code 
that I'm not sure of its status [check with an if where possible, or an 
assert as alternative to risking segfault].

> 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? it is being _used_.

Regardless of what that parameter is called it is the object that our 
class is being attached to. So yes it is absolutely needed.


> 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? 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 connection is not accessible in most places this function is called. 
What we have reliably is a pointer to the session state where our object 
is (to be) associated. Sometimes that session has a connection, but half 
of the calls are at times before the TLS session initiates a TLS connection.


> Please s/GetFrom/Of/ or, if you insist, s/GetFrom/From/ to shorten the 
> name and emphasize that this is not just an existing extras retrieval 
> method. From the caller point of view, all connections have a valid 
> extras object (and that is a good thing!).
>> - if (hostName) - SSL_set_ex_data(serverSession.get(), 
>> ssl_ex_index_server, (void*)hostName); + if (hostName.isEmpty()) { 
> You want the opposite condition. AFAICT, this bug implies that the 
> changes were essentially untested. Please explicitly disclose untested 
> patches. 

Thanks for the catch.

That code path is untested by me, yes. I cannot test the OpenSSL code 
changes, and thought I'd stated that enough by now. It is one of the 
reasons I'm going about these patches so slowly and piece-wise.


>> - SBuf *hostName = NULL; + SBuf hostName; 
> Or perhaps we can do the following instead? SBuf &hostName = extras->sni; 

Nice. Done.

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

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

>> - sniServer = !request->url.hostIsNumeric() ? request->url.host() : 
>> NULL; + sniServer = !request->url.hostIsNumeric() ? 
>> request->url.host() : nullptr; 
> An out-of-scope change.
>> // validationRequest disappears on return so no need to 
>> cbdataReference validationRequest.errors = errs; + try { 
> An out-of-scope change. Thank you, Alex.


Removed those.

Amos
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Security_ExtraData_pt1_mk2.patch
Type: text/x-patch
Size: 3689 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170423/144d9782/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Security_ExtraData_pt2_mk2.patch
Type: text/x-patch
Size: 9201 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170423/144d9782/attachment-0001.bin>