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

On 11/08/2011 01:27 AM, Alex Rousskov wrote:
> 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.

Amos, we have not always system errno in the case of
ERR_SECURE_CONNECT_FAIL. We may have in the case of
SQUID_ERR_SSL_HANDSHAKE ssl error which added today to trunk.

What are you suggesting? If I am understanding well, you are suggesting
to try log the system errno when exist?

The "%err_detail" formating code prints the errno (SYSERR=errno) only
when there is not any other useful info (eg a
"ERR_DETAIL_ICAP_XACT_START" detail or a "EXCEPTION=0x53" info)

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

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?

Correct.
In this patch, we are fixing the "%err_detail" formating code to display
more useful error details in the case of ERR_SECURE_CONNECT_FAIL. In
this case we may have a system errno but we may have not (in most cases).

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

We need a new log access formating code, lets say "%errno", or add
parameters to the "%err_detail" formating code to manage its behavior.

>
>
> Thank you,
>
> Alex.
>
Received on Tue Nov 08 2011 - 16:25:32 MST