Re: [PATCH] SSL server certificate fingerprint ACL type

If there is not objections I will apply the latest "SSL server
certificate fingerprint ACL type" patch
(certificate-fingerprint-ACL-v3.patch) to trunk

On 11/24/2012 05:47 PM, Tsantilas Christos wrote:
> 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
>>
>
Received on Tue Dec 04 2012 - 14:55:48 MST