[squid-dev] [PATCH] Initial libsecurity API

Alex Rousskov rousskov at measurement-factory.com
Mon Jan 26 19:26:56 UTC 2015


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.


More information about the squid-dev mailing list