Re: [PATCH] SSL server certificate fingerprint ACL type

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

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.
  - 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 src/acl/ServerCertificate.cc:
* please remove addition of $Id$ (same goes elsewhere)
* fde.h and client_side.h includes should be the other way around
(sorted alphabetical).
* please remove doubled empty lines
* please do not add whitespace between function name and parameter
lists. same goes on any new code.
   - s/::match\ (/::match(/

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

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

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 Fri Nov 23 2012 - 11:49:54 MST