Re: [PATCH] SSL server certificate validator implementation

A new patch.
This patch includes most (/all I hope) of Amos requested changes.

On 11/22/2012 02:10 PM, Amos Jeffries wrote:
> On 22/11/2012 9:14 a.m., Tsantilas Christos wrote:
>> If there is not any objection I will commit this patch plus the "cert
>> validation cache" patches to trunk
>
> Sorry. I have not gotten around to auditing these yet. Thanks for the
> reminder I just did a quick check...
>
>
> ** please update the wiki Feature/AddonHelpers page with the protocol
> this patch is actually adding. I notice the host= key name has changed
> to domain=.

fixed

>
>
> helpers/ssl/cert_valid.pl:
> * Please do "use strict;" in the perl helper. Its lack will prevent
> some distros being willing to bundle it and if there are problems we
> should sort them out before publishing. Or at minimum if they are major
> problems we can't resolve within a reasonable timeframe please document
> what they are and why "strict" is disabled.

OK strict is used now

>
> * please present something about what the BH was caused by in a
> message="" kv-pair sent back to Squid.

ok

>
> * please do not use custom log files in helpers. stderr is available for
> cache.log and is guaranteed to be writable (/tmp does not always exist).

ok the stderr used

>
> * please DO define the -h (usage help) and -d (debug info to stderr)
> command line options at minimum.

OK some basic help added.

> - for debug output I would prefer it if the lines started with;
> timestamp then SP then program name or $0 value then PID number then "|"
> character another SP then your message. That way they will fit into
> cache.log and each helpers stream will be identifiable.

ok for this to. It is not the best but can be changed by developer of a
production service.

>
> * please enable concurrency by default. Since this is a new interface we
> have no legacy excuses to hold us back on good performance.

Concurrency is enabled but only one thread running.

> src/cf.data.pre:
> * missing documentation about concurrency= support on this interface.
> * if the bundled helper is upgraded to concurrency support the config
> default can be changed to enable some number of channels.

By default it is zero.

I put it to concurrency=1?

>
>
> src/forward.cc:
> * debugs in catch statement seems to be incorrect. It talks about
> "ssl_crtd" and seems to be saying it is about to validate the message...
> which was what just threw not about to happen.

fixed

>
> * Please check for BH status being returned by the helper differently
> from the "NULL reply" error case. The handler can receive a reply with
> BH having content. Ssl::CertValidationHelper::sslSubmit() being one
> place which uses a message for Squid-side errors in the helper transaction.

Now BH checked.

>
> * in FwdState::sslCrtvdCheckForErrors() please explain the significance
> of "ignore????". Is it an unexpected result state? an error? or
> something the admin can configure?
> - nobody can answer or investigate your question-"????" if we dont even
> know what the result means.

Is not possible to have i->error_no == SSL_ERROR_NONE here. It can be
replaced with an assertion or something similar.
I add an assertion for now.

>
> * please use X509_Pointer wherever possible instead of X509*. peerCert
> at minimum looks like a good place since it allows you to remove an
> explicit free().

OK.
>
>
> src/forward.h:
> * I think we can get away with just a class HelperReply pre-define
> instead of #include.

OK.

>
>
> src/main.cc:
> * in what circumstances can "if
> (Ssl::CertValidationHelper::GetInstance())" be false? is this a case of
> omitting USE_SSL_CERT_VALIDATOR ?
> - same for the Shutdown() equivalents. ssl_crtd helepr does not seem
> to need checking the instances existence first.

Just none validator helper is configured.
I add a "// && USE_SSL_CERT_VALIDATOR" near the "#if USE_SSL"

>
>
> src/ssl/cert_validate_message.cc:
> * s/Vailidator/Validator/
>
fixed

> * Ssl::CertValidationResponse is manually mintaining a 'cert' member
> alloc/free state. Please use X509_Pointer instead.

It is used inside CertValidationResponse::RecvdError class. This class
used as member of a vector. We need in this class copy constructors and
"=" operator.
The X509_Pointer supports the resetAndLock member, which can used here...
I fixed this, I hope I did not break anything....

>
>
> src/ssl/cert_validate_message.h
> * Please do not add $Id$ CVS tags to new files. They are ignored by bzr
> and immediately become incorrect.
>

OK fixed.

> src/ssl/helper.cc:
> * in Ssl::CertValidationHelper::Init() no need to use
> safe_free(tmp_begin) on local variables as they die with scope exit. Use
> xfree(tmp_begin) instead.

OK.

>
> * please do not use HERE at level-1 debug. Use a NOTICE/WARNING/ERROR
> label and use a fully descriptive message. "Queue Overload" is not
> enough. For example what queue? and what should the admin fix to resolve
> the error? etc
>
ssl_crtvd queue overload

> * in Ssl::CertValidationHelper::sslSubmit() fatal message is a bit
> strange. "SSL servers not responding for 3 minutes" or ssl_crtvd helpers
> not responding? or just the queue being overloaded for a long time?
> (helpers responding, but not fast enough to clear the queue in under 3
> minutes from peak traffic starting)

I changed the message to "ssl_crtvd queue being overloaded for long time"

>
>
> src/ssl/support.cc
> * apparently useless include of protos.h.
removed

>
>
> Everywhere:
>
> * please remove all instances of " HERE <<" in new code. No longer needed.
>
> * please use DBG_IMPORTANT for all level-1 messages.
>
> * please fix all instances of "#if 1 // USE_SSL_CERT_VALIDATOR" and "#if
> USE_SSL //&& USE_SSL_CERT_VALIDATOR" . If the USE_ macro is retained
> also ...
> + the #includes only done for this helper can be wrapped as well.
> + "class CertValidationHelper" definition missing wrapper?? please
> also check for other places needing the wrapper.

I try my best here.
There was 1-2 point where if ssl_crtd is not enabled then the cert
validator will not compiled correctly.

> + minimal and maximus build test levels in test-suite/buildtests/*
> need ./configure options added to control this USE_ macro

We are not adding new configure option. The cert validaror does not add
any extra overhead if not used.

The "#if 1 // USE_VALIDATOR" are here in the case we decide in the
future to add it.

> + when these are all done, please re-run the build tests to check
> nothing is broken by the wrapper changes.
>
>
> Amos
>