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

[Alex Rousskov]

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.

Details below.


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

s/session/connection/


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


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

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

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

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.


> -        SBuf *hostName = NULL;
> +        SBuf hostName;

Or perhaps we can do the following instead?

  SBuf &hostName = extras->sni;


> +    Security::SessionPointer session(ssl, [](SSL *){/* do not destroy */});

This ugly hack will no longer be needed after the above changes AFAICT.


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


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