Re: [PATCH] Logging of honored certificate validation error names

On 11/07/2011 03:58 PM, Amos Jeffries wrote:
> On Mon, 07 Nov 2011 13:48:46 -0700, Alex Rousskov wrote:
>> On 11/05/2011 04:15 PM, Amos Jeffries wrote:
>>> On 2/11/2011 11:13 p.m., Tsantilas Christos wrote:
>>>> Currently the %err_detail access_log formating code does not display
>>>> something useful for the system admin in the case of the certificate
>>>> validation errors.
>>>>
>>>> This patch in the case of an ERR_SECURE_CONNECT_FAIL error displays
>>>> the certificate validation error name.
>>>>
>>>
>>> +1. Looks okay.
>>>
>>> I'm a little dubious about passing request->detailError() the SSL error
>>> code instead of the errno. But have no strong objections.
>>
>> Error detailing code was specifically designed to record
>> context-specific details beyond errno (which was already available in
>> most cases) and the request->detailError() method itself is usually used
>> to store non-errno details:
>>
>>> ./src/Server.cc: request->detailError(ERR_ICAP_FAILURE,
>>> ERR_DETAIL_RESPMOD_BLOCK_LATE);
>>> ./src/client_side_request.cc:
>>> request->detailError(ERR_ACCESS_DENIED, ERR_DETAIL_REQMOD_BLOCK);
>>> ./src/client_side_request.cc:
>>> request->detailError(ERR_ICAP_FAILURE, ERR_DETAIL_CLT_REQMOD_RESP_BODY);
>>> ./src/client_side_request.cc:
>>> request->detailError(ERR_ICAP_FAILURE, errDetail);
>>
>>
>> We even document HttpRequest::errDetail to be errType-specific:
>>
>>> err_type errType;
>>> int errDetail; ///< errType-specific detail about the transaction
>>> error
>>
>>
>> Why are you dubious about passing request->detailError() the SSL error
>> code?
>>
>
> That in this case we seem to have both an errno and an extended error
> with values. Its not clear to me whether this change is keeping the
> system errno around for the report tokens which display pure errno (or
> '-') rather than our extended err_type.
>
> Not a major issue since I think our err_type is a clearer message anyway.

Christos,

    I think your patch does not change pure errno rendering for error
pages where we have separate macros for displaying errno and error
detail, right?

For access logs, we do not have separate macros so we have to log what
is most relevant. In case of a certificate validation errors, the SSL
validation error is more relevant than system errno, which is often not
set for those errors, correct?

When/if access log gains all the error page macros, the admin would be
able to log the system errno separately from the SSL error name.

Thank you,

Alex.
Received on Mon Nov 07 2011 - 23:27:41 MST