Re: [PATCH] SSL server certificate validator implementation

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

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.

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

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

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

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

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.

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.

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

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

* 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().

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

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.

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

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

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.

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.

* 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

* 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)

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

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.
   + minimal and maximus build test levels in test-suite/buildtests/*
need ./configure options added to control this USE_ macro
   + when these are all done, please re-run the build tests to check
nothing is broken by the wrapper changes.

Amos
Received on Thu Nov 22 2012 - 12:11:13 MST