Re: [PATCH] SSL server certificate validator implementation

On 11/24/2012 04:23 PM, Tsantilas Christos wrote:
> A new patch.
> This patch includes most (/all I hope) of Amos requested changes.

If there is no objection I will commit the latest patch
(cert_validator-v3.patch) to trunk.

>
>
> 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
>>
>
Received on Mon Dec 03 2012 - 16:58:58 MST