Re: [PATCH] Tying validation errors to certificates

On 5/06/2013 8:53 p.m., Tsantilas Christos wrote:
> On 06/04/2013 05:36 AM, Amos Jeffries wrote:
>> On 4/06/2013 12:40 a.m., Tsantilas Christos wrote:
>>> On 06/02/2013 02:35 PM, Amos Jeffries wrote:
>>>> On 29/05/2013 8:59 p.m., Tsantilas Christos wrote:
>>>>> When Squid sends errors to the certificate validation daemon, the
>>>>> daemon
>>>>> cannot tell which certificate caused which error. This is especially
>>>>> bad
>>>>> because the validator has to return that same information in the
>>>>> response (the response format requires the validator to match the error
>>>>> to the certificate).
>>>>> This patch adjust the validation request format to provide that
>>>>> information using a set of the following key=value pairs:
>>>>>
>>>>> error_name_N=the name of the certificate error number N
>>>>> error_cert_N=the ID of the certificate which caused error_name_N
>>>>>
>>>>> where N is non-negative integer. N values start from zero and increase
>>>>> sequentially.
>>>>>
>>>>> This is a Measurement Factory project
>>>> I think this problem is a side-effect of not following my suggestion
>>>> earlier to split the certificates across concurrency channels. Yes?
>>> I think no....
>>> The server may sent more than one certificates, eg the site certificate
>>> plus an intermediate certificate.
>>> The error maybe exist in the first certificate or in the second
>>> certificate. Currently we just sent to helper the error name.
>>> This patch ties the error to the certificate.
>>>
>>> For example if the site certificate is expired and we can not find the
>>> issuer of intermediate certificate the message will send to the
>>> validator helper will looks like the following:
>>> cert_0=.....-Site Certificate-.....
>>> cert_1=.....-Intermediate Certificate-....
>>> error_name_0=X509_V_ERR_CERT_HAS_EXPIRED
>>> error_cert_0=cert_0
>>> error_name_1=X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY
>>> error_cert_1=cert_1
>>>
>>>> If that were done each channel would be dealing with only one
>>>> certificate and its errors. No need to explicitly tie them together like
>>>> this.
>>> If the server uses a certificate which signed using an intermediate
>>> certificate, then the validator helper will need also the intermediate
>>> certificate to verify server certificate.
>>> In this case we are verifying a chain of certificates.
>>> So they must included in the same request.
>> Okay then. +0.5 from me (since my perl skills are fairly low still).
> If there is not any objection I will commit this patch to trunk.

Okay with me.

>
>> There is one other change which seems unrelated to the cert errors:
>>
>> - $request =~ s/^host=.*\n//;
>> + $request =~ s/^host=.*$//m;
>>
>> That could also do with a mention about why its there in the commit
>> message (or removal).
> This change just allow the "host=" parameter to exist anywhere in the
> request body data. Before this change it should be placed at the begin
> of body data...
> I can remove it , or post it as separated patch, if required...

Ah that.
Separate patch might be good to document it, but no worries if you want
to combine them.

Amos
Received on Wed Jun 05 2013 - 12:15:20 MDT