Re: [PATCH] SSL server certificate fingerprint ACL type

On 11/23/2012 01:49 PM, Amos Jeffries wrote:
> On 15/11/2012 1:12 a.m., Tsantilas Christos wrote:
>> SSL server certificate fingerprint ACL type
>>
>> This patch add the "server_ssl_cert_fingerprint" acl type to match
>> against server SSL certificate fingerprint.
>> The new acl type has the form:
>> acl aclname server_ssl_cert_fingerprint [-sha1] fingerprint1 ...
>>
>> The fingerprint must given in the form:
>> XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX:XX
>> where X are any valid hexadecimal number
>>
>> Example usage:
>> acl BrokeServer dst 192.168.1.23
>> acl GoodCert server_ssl_cert_fingerprint
>> AB:2A:82:AF:46:AE:1F:31:21:74:65:BF:56:47:25:D1:87:51:41:AE
>> sslproxy_cert_error allow BrokeServer GoodCert
>> sslproxy_cert_error deny all
>>
>> Someone can retrieve the fingerprint of a certificate using the openssl
>> command:
>> # openssl x509 -fingerprint -in test.pem -noout
>> # openssl s_client -host www.paypal.com -port 443 2> /dev/null |
>> openssl x509 -fingerprint -noout
>>
>>
>> This is a Measurement Factory project
>
> Finally got to it. Sorry this has taken so long...
>
> * ACL name seems to be a bit on the long side. How about dropping the
> "ssl_" sub-section out of it and changing "_fingerprint" to "_sha1"?
> ==> server_cert_sha1

This is not correct.
The -sha1 is an argument to specify the type of fingerprint. In the
future we may add the "-md5" argument to specify an server_cert_md5 acl.

I renamed the acl name to
server_cert_fingerprint [-sha1]

>
> in src/acl/CertificateData.cc:
> * debugs() at CRITICAL level need ERROR/FATAL/WARNING and to explain
> where they are reporting from in a simple way.

ok

> - In ACLCertificateData::parse() the message "required attribute
> argument missing" does not say anything useful for fixing the problem
> when it hits the logs. The other messages after are a bit more
> expressive, since they at least mention 'Acl' or what the attribute
> should be, but they all need a bit friendliness polish.
> + something along the lines of "FATAL: Acl '"<< name? <<"' ..."

In logs someone will see something like:
 2012/11/24 17:38:10| FATAL: required attribute argument missing
 FATAL: Bungled squid-cert-validation.conf line 151: acl USERCERT user_cert

Will understand whe wrong syntax.
We do not have the ability inside ACLCertificateData to print acl name
or other informations.

>
>
> in src/acl/ServerCertificate.cc:
> * please remove addition of $Id$ (same goes elsewhere)

ok

> * fde.h and client_side.h includes should be the other way around
> (sorted alphabetical).
ok

> * please remove doubled empty lines
ok

> * please do not add whitespace between function name and parameter
> lists. same goes on any new code.
> - s/::match\ (/::match(/

ok

>
>
> in src/acl/ServerCertificate.h
> * $Id$ again.
> * please do not add useless empty lines at the beginning of classes.
> "+{\n\npublic:\n" -> "+{\npublic:\n"

ok

>
> in src/acl/StringData.h:
> * documentation on insert()... whats a custom value when its string data?

ok.

>
>
> That is all that stands out at me. Not familiar enough with the cert
> operations yet to do a more through scan through, sorry.
>
>
> NP: this patch meshes with cert validator and will also need re-testing
> for build issues after that other patches wrapper macros are fixed up.
>
> Amos
>