On Nov 30, 2012, at 2:20 AM, Igor Galić <i.ga...@brainsware.org> wrote:
> > > ----- Original Message ----- >> Hi all, >> >> I've just finished a big SSL certificate refactoring, whose goal was >> to decouple SSLCertLookup enough for me to write a standalone test >> harness for it. >> >> https://github.com/jpeach/trafficserver/tree/jpeach/ssl-linkage >> >> Since there's a fair amount of code shuffling, it would be great if >> anyone could review it. > > It is a big code drop indeed, and I would've rather seen its slow > evolution. Being this big, the created (optimized?) diff is anything > but optimized for easy reading. Yes, I agree that smaller changes would be better. In my defence, the set of working commits were so much worse. > That said, comments (from me, anyway) > will be going to the list, not to github, so we can keep focused. Yep, the list is better than github. > Be prepared for stupid questions. > > from the commit message: > > to release them when we reload the contexts. This also sames us > > > +struct SSLCertLookup : public ConfigInfo > > Why is the destructor Virtual? Well it's redundant because ConfigInfo is virtual (and has to be). But it's done like this in some other places and I felt that it was a nice heads-up to future devs. > +namespace ssl { namespace detail { > > That's a very nice use of namespaces to pull out scoped_config! > > > in > struct SSLConfigParams : public ConfigInfo > > You're making all members public: Does that already break ABI? Nope. we don't have an internal ABI. > And if it does, why not go all the way and reorder them to align nicely? > > > SSLUtils.h > > +#if !defined(OPENSSL_THREADS) > +#error Traffic Server requires a OpenSSL library that support threads > > grammar aside, shouldn't this be a compile time issue? Do you mean configure time? Yes, that would be better ... > Please add a @brief to the .h and .cc files. > > Just a side note: > > + static const char hextab[16] = { > + '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', > 'e', 'f' > > This is the fourth time we define a hextab in the code ;) > > > > - return SSL_TLSEXT_ERR_NOACK; > + explicit > + SSLAddressLookupKey(const IpEndpoint& ip) > + { > > only now do I realize that a number of our constructors may do well > with being prefixed by explicit... Yes, explicit should probably be our default in most cases. > Design question: > > class SSLContextStorage becomes struct SSLContextStorage > the internal struct SSLEntry remains private - is that the > reason it doesn't get the same treatment as scoped_config? I prefer 'struct' to 'class' mainly because I don't like typing the extra keywords. scoped_config doesn't really apply to SSLContextStorage because it's for when you need to get something from the config processor and put it back. SSLContextStorage is just an internal code organization class to prevent SSLCertLookup from getting too complicated. > maybe this: > > reverse_dns_name(const char * hostname, char > (&reversed)[TS_MAX_HOST_NAME_LEN+1]) > > would make more sense in hostdb? > > > > + box.check(wildcard.match("foo.com") == false, "foo.com is not a wildcard"); > + box.check(wildcard.match("*.foo.com") == true, "*.foo.com not a wildcard"); > + box.check(wildcard.match("bar*.foo.com") == false, "bar*.foo.com not a > wildcard"); > + box.check(wildcard.match("*") == false, "* is not a wildcard"); > + box.check(wildcard.match("") == false, "'' is not a wildcard"); > > I'm confused, why are those not wildcards? We don't support generalized wildcards. We only support wildcards of the form "*.foo". This restricted form makes them amenable to longest match lookup using the Trie. If we wanted to support the arbitrary wildcards then the lookup would be much more involved. I believe that "*.foo" is the 99% case for using wildcard certificates. > > > - CACertFilename = CACertPath = > - clientCertPath = clientKeyPath = > - clientCACertFilename = clientCACertPath = > + serverCACertFilename = > + serverCACertPath = > + clientCertPath = > + clientKeyPath = > + clientCACertFilename = > + clientCACertPath = > > +1 for unifying those names! > > > - if (client_ctx) > + if (client_ctx) { > SSL_CTX_free(client_ctx); > - client_ctx = NULL; > + client_ctx = NULL; > } > > hah: Nice - if it's *already* NULL, why assign NULL to it? It's not already NULL :) > +SSLNetProcessor::start(int number_of_ssl_threads) > + // This initialization order matters ... > + SSLInitializeLibrary(); > + SSLConfig::startup(); > > if (HttpProxyPort::hasSSL()) { > - // Only init server stuff if SSL is enabled in the config file > - SSLCertLookup::startup(); > + SSLCertificateConfig::startup(); > > I'm confused, why do we startup() the SSLConfig even if we have no SSL? > Who calls SSLNetProcessor::start()? We might need SSLConfig for the HTTP client. SSLNetProcessor::start gets called somewhere during initialization. > Another side note: Has anyone considered borrowing httpd's idea of > uniquely numbering each error log message? That's a great idea ... how do they manage automation of message number assignments? > Inspired by things like: > + SSLError("SSL_write"); > which would look extremely unhelpful to me, if I were trying to debug > an issue ;) > > > (after almost an one and a half hour I am skipping most of > iocore/net/SSLUtils.cc > it's all just added to a single file and it's hard to tell what was there > before, and what refactored: This would've been better with many small > commits.) > > +SSLDebugBufferPrint(const char * tag, const char * buffer, unsigned buflen, > const char * message) > + for (unsigned ii = 0; ii < buflen; ii++) { > + putc(buffer[ii], stdout); > + } > + putc('\n', stdout); > why do we use putc here? I really don't know ... that code was there before me. > I'm noticing that you're abstracting away a lot of OpenSSL's > "A"PI, I'll have more to say on that another time. That wasn't really the intention. The goal was to remove the generic "configure stuff in openssl" out of SSLNetProcessor. This is all standalone code and coupling it into a class doesn't serve any useful purpose. > Sadly, we have custom configuration parsing here, just as we do > every where else :c > > > + // On Darwin, fail the tests if we have any memory leaks. > +#if defined(darwin) > + if (system("xcrun leaks test_certlookup") != 0) { > + RegressionTest::final_status = REGRESSION_TEST_FAILED; > + } > +#endif > > Wouldn't it be a good idea to do this in general? Yes it would be good to do it in general, but this was a tiny hack to let me test. > i.e.: Use valgrind (or which ever tool) on the regression test suit > if it's available and/or enabled, i.e.: If we --enable-debug && --enable-tests Yes, for valgrind you would have to enable this at build time and integrate it into the test harness system. I don't know how long it's been since anyone has valgrind'd ATS so there's a probably a bit of work involved in making is clean. Thanks for taking the time to review ... J
