Re: [PATCH] squidclient TLS support

On 04/16/2014 07:44 AM, Amos Jeffries wrote:

> Both CA verified and anonymous TLS are supported with CA verification
> being the default.

I cannot find "CA verified" term in RFC 5246. Is there a better term to
use? Is this about verifying the server certificate or the client
certificate? I assume it is the latter, but please be more explicit in
the commit message either way.

> + // x509 certificate credentials
> + gnutls_certificate_credentials_t certCredentials;

client or server certificate credentials? I assume it is the latter but:

> + /* If client holds a certificate it can be set using the following:
> + *
> + gnutls_certificate_set_x509_key_file(Transport::Config.certCredentials, "cert.pem", "key.pem", GNUTLS_X509_FMT_PEM);
> + */

the above implies that the same member can actually store the client
certificate? Does GnuTLS store both client and server certificate in the
same place?

> + // x509 certificate authorities file
> + const char * caFile = "/etc/ssl/certs/ca-certificates.crt";

If it is easy to make that value configurable, please do.

> +/// parameters controlling 'ping' mode message looping.
> +class TheConfig

A stale comment?

> + // options for controlling squidclient ping mode
> + static struct option pingOptions[] = {

A stale comment and stale variable name?

Please search the patch to double check that there are no inappropriate
references to "ping".

> + << " --tls [TLS options] Use TLS on the connection" << std::endl
> + << std::endl
> + << " TLS options:" << std::endl
> + << " --anonymous Use Anonymous TLS." << std::endl

AFAICT, the new code allows squidclient to establish a secure connection
to Squid. The connection can be encrypted using SSL or TLS, depending on
various factors, right? If yes, please make the new option name
protocol-neutral (e.g., --secure) so that the tool user does not have to
lookup new (to them) protocol abbreviations or change their scripts as
protocol names change.

If OpenSSL support is added, most admins should still be able to use
--secure and get the "most secure" mode possible unless they provide
more details (such as future options to select the exact secure protocol
name, version, or even library).

> + default:
> + Transport::InitTls();
> + // rewind and let the caller handle unknown options
> + --optind;
> + opterr = saved_opterr;
> + return true;
> + }
> + }
> +
> + Transport::InitTls();
> + opterr = saved_opterr;
> + return false;

AFAICT, the above InitTls()-calling code is executed, in part, for
command line options that do not imply TLS use (e.g., --host or
--nosuchoption).

Please do not call InitTls() unless TLS is explicitly requested (--tls).
If the code can be sure that TLS is implicitly required, it can enable
TLS in those cases as well, but I do not see what information the code
can use to detect such an implication (--port 443 is probably not
sufficient).

> + std::cerr << "ERROR: Syntax error at: " << err << std::endl;

> + std::cerr << "ERROR: *** Anonymous TLS credentials setup failed (" << x << ") " << std::endl;

> + std::cerr << "ERROR: *** " << type << " TLS Handshake failed (" << ret << ") "

> + std::cerr << "ERROR: TLS support not available." << std::endl;

Do "***" markings have some hidden meaning in some contexts? If not,
please remove them to be more consistent when reporting errors.

> + std::cerr << "ERROR: Syntax error at: " << err << std::endl;

Does this really report the location of the error? If it reports the
error itself, consider

  s/Syntax error at: /Cannot set ciphers: /

> + const char *err = NULL;
> + int x;
> + if ((x = gnutls_priority_set_direct(Transport::Config.session, ciphers, &err)) != GNUTLS_E_SUCCESS) {
> + if (x == GNUTLS_E_INVALID_REQUEST)
> + std::cerr << "ERROR: Syntax error at: " << err << std::endl;
> + gnutls_perror(x);
> + return false;
> + }

The above code is repeated at least twice. Please encapsulate if possible.

> GnuTLS library detection is done in a compatible way such that Squid as
> a whole can be built against both, either, or neither of OpenSSL and
> GnuTLS

GnuTLS-specific operations could be encapsulated better to ease possible
future addition of OpenSSL support, but I do not think that such
isolation should be a precondition for this patch acceptance.

I have not fully reviewed the patch (and am not qualified to do so), but
have no objections to its polished version going in.

Thank you,

Alex.
Received on Wed Apr 16 2014 - 18:56:53 MDT