Re: [PATCH] squidclient TLS support

On 17/04/2014 6:56 a.m., Alex Rousskov wrote:
> 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.

I don't know of any official term for verifying if a certificate has
been signed by a CA. Perhapse "Trusted" by itself. Making the
mechanisms: "Trusted", Anonymous, TOFU and DANE.

The terms "authenticated" or "non-anonymous" are those used in the RFC
but describe the process end-states more that the certificate
verification method itself.

>
>> + // x509 certificate credentials
>> + gnutls_certificate_credentials_t certCredentials;
>
> client or server certificate credentials? I assume it is the latter but:
>

Everything in this is client oriented since "squidclient" is a client. I
did not think it necessary to spell that out in every variable name.
Added the word "client" to the description since it is used in the
anonymous credentials description.

>
>> + /* 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?
>

Server certificate is never stored nor configured by squidclient.
If anyone wants to implement an explicit client certificate PEM file
loading that is how it's done.

>
>> + // x509 certificate authorities file
>> + const char * caFile = "/etc/ssl/certs/ca-certificates.crt";
>
> If it is easy to make that value configurable, please do.
>

Done.

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

Fixed all those.

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

To *any* server. I have been testing against Google, Facebook, and some
servers with known HTTPS issues.
It is not even limited specifically to HTTPS servers, although at
present the tool does try to always deliver an HTTP/1.x message with the
usual results.

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

Yes...

 The prime factor being that one endpoint uses the *TLS* mechanism to
negotiate use of SSL/3.0 format messaging.

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

"secure" would be lying since "--secure --anonymous" then means
*insecure* as do various cipher settings, none of which are failures cases.

"Transport Security" is the most accurate descriptive term. Indicating
the presence of security systems without implying they work or provide
any actual protection. I believe that is one reason why the protocol is
not called SSL in technical documents about it.

It is named as per the title of the RFC:
 The Transport Layer Security (TLS) Protocol Version 1.2

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

Okay, fixed.

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

Done.

>
>> + 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: /

Yes it shows the sub-string where the error occurs.
Also it is not specific to ciphers, other pieces like the acceptible
version numbers and sundry extension flags can be set there too.

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

Done.

>
>> 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 for the time.

Amos
Received on Thu Apr 17 2014 - 20:20:01 MDT