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.
