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