Re: [PATCH] SSL server certificate fingerprint ACL type

On 11/30/2012 04:49 AM, Amos Jeffries wrote:
> On 25/11/2012 4:47 a.m., Tsantilas Christos wrote:
>>>> acl aclname server_ssl_cert_fingerprint [-sha1] fingerprint1 ...

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

I do not think we should have different ACLs for different fingerprint
types. The fingerprinting algorithm is just a parameter -- all other
aspects of the ACL remain the same. This is similar to having one ACL
for all HTTP response headers.

As for preventing typos, I do not see why accidentally typing "-sha1"
instead of "-md5" is somehow more likely than typing "_sha1" instead of
"_md5".

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

Yes, we should use the existing interface until a new interface is
available. It is better to be consistent, especially if we care about
automated tools (that will grep for FATAL and see both messages BTW).

Thank you,

Alex.
Received on Fri Nov 30 2012 - 17:09:06 MST