Re: [PATCH] SSL server certificate fingerprint ACL type

On 25/11/2012 4:47 a.m., 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]

Hmm. Okay. I was thinking we should have a different ACL for each
fingerprint type, that would help prevent wrong fingerprints being
entered. I'm not strongly for this though, so the above change is accptible.

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

Administrators are commonly using automated tools now to grep for
message lines. And additionally I am hoping to remove the "bungled"
completely from Squid.

I'm okay with defering this fix for later since it affects all ACLs.

+0. There are no more issues from me, but as said I'm not quite up to
scratch yet on the implementation details in these areas of Squid, so I
can't speak towards its correctness.

Amos
Received on Fri Nov 30 2012 - 11:49:22 MST