[squid-dev] [PATCH] Initial libsecurity API

[Alex Rousskov]

On 01/14/2015 08:50 AM, Amos Jeffries wrote:
> This is the first step(s) towards a generic TLS/SSL security API for
> Squid.


> +    // BUG: ssl_client.sslContext will leak on reconfigure when Config gets memset()
...
> +    Config.ssl_client.sslContext = Security::ProxyOutgoingConfig.createContext();

Which memset(Config) call are you referring to here?

> void
> configFreeMemory(void)
> {   
>     free_all();
> #if USE_OPENSSL
>     SSL_CTX_free(Config.ssl_client.sslContext);
> #endif
> }

And is not Config.ssl_client.sslContext destroyed in the old
configFreeMemory() function quoted above?


> +    // it makes more sense to create a context per outbound connection instead of this

Please remove this comment. Since each context may consume gobbles of
RAM, I doubt what you are suggesting always makes more sense, but
discussing this is outside your project scope.


> +NAME: tls_outgoing_options

Please do not forget the recently added SSL_OP_NO_TICKET when merging.


> +} // namespace Security
> +
> +// parse the tls_outgoing_options directive
> +inline void
> +parse_securePeerOptions(Security::PeerOptions *opt)
> +{
> +    while(const char *token = ConfigParser::NextToken()) {
> +        opt->parse(token);
> +    }
> +}
> +
> +#define free_securePeerOptions(x) Security::ProxyOutgoingConfig.clear()
> +#define dump_securePeerOptions(e,n,x) // not supported yet


Please add an XXX to explain why is these are declared outside their
namespace. For example:

XXX: These declarations are outside their namespace because our
generated parsing code cannot handle namespaces.


> +// parse the tls_outgoing_options directive
> +inline void
> +parse_securePeerOptions(Security::PeerOptions *opt)
> +{
> +    while(const char *token = ConfigParser::NextToken()) {
> +        opt->parse(token);
> +    }
> +}

I see no reasons to inline this loop. The related code is slow for other
reasons and not in a critical path. Please do not inline unless really
necessary.


> +#define free_securePeerOptions(x) Security::ProxyOutgoingConfig.clear()
> +#define dump_securePeerOptions(e,n,x) // not supported yet

Why are these #defined? If they can be implemented as regular functions,
they should be IMO.


> +            // const loss is okay here, ssl_ex_index_server is only read and not assigned a destructor
> +            SSL_set_ex_data(ssl, ssl_ex_index_server, const_cast<SBuf*>(&peer->secure.sslDomain));

Perhaps I am missing something, but it looks like you are adding an SBuf
pointer where the rest of the code expects a char* pointer (both
SSL_set_ex_data and SSL_get_ex_data calls with the same
ssl_ex_index_server index). How did this change work or was this path
not tested?

If this is indeed a bug, please go through all ssl_ex_index_server users
to make sure they all agree on the data type. For example:

  bzr grep -n ssl_ex_index_server


> +#if WHEN_PEER_HOST_IS_SBUF
>          else
>              SSL_set_ex_data(ssl, ssl_ex_index_server, peer->host);
> +#endif

Lost of functionality here? Please restore if possible or
explain/document if not.


> +
> +class PeerOptions
> +{

Please add a one-line description to clarify what we mean by "Peer"
here: cache_peer, any server, any client or server, etc.


> +    /// generate a security context from the configured options
> +    Security::ContextPointer createContext();

Do we need to use the explicit Secutiry:: prefix inside namespace
Security {}?


> +    /// generate a security context from the configured options
> +    Security::ContextPointer createContext();

If the description is accurate, should not this method be moved outside
the PeerOptions class? And if the move is not possible for some reason,
should not this method be "const"?


> +    bool ssl;   ///< whether SSL is to be used on this connection

"this connection" is not clear in a PeerOptions context. Perhaps we can
say "with this peer"?

I thought you were opposed to using ssl for new stuff. Why not call this
"secured" or something like that? You have to change all calling code
anyway, right?


> +    bool ssl;   ///< whether SSL is to be used on this connection
> +
> +    SBuf certFile;       ///< path of file containing PEM format X509 certificate
> +    SBuf privateKeyFile; ///< path of file containing private key in PEM format
> +    SBuf sslOptions;     ///< library-specific options string
> +    SBuf caFile;         ///< path of file containing trusted Certificate Authority
> +    SBuf caDir;          ///< path of directory containign a set of trusted Certificate Authorities
> +    SBuf crlFile;        ///< path of file containing Certificate Revoke List
> +
> +    int sslVersion;
> +    SBuf sslCipher;
> +    SBuf sslFlags;
> +    SBuf sslDomain;

Please consider making "sslVersion" and "ssl" last data members (in that
order), to possibly save a tiny bit of padding and avoid giving a bad
order example for other classes (where it may actually matter).


If this change was not tested with and without SslBump, please test.


Thank you,

Alex.