On Aug 19, 2014, at 12:46 PM, Susan Hinrichs <shinr...@network-geographics.com> wrote:
> James, > > Thanks for the feedback. I think Alan already addressed most of the issues. > Here are my comments on the remaining items. > > >> >> Why do TSSslCertFindByName() and TSSslCertFindByAddress() take a TSSslVConn >> argument? > > I'm using the TSSslVConn to cache a pointer to the global cert table (loaded > from ssl_multicert.config). Since in theory the ssl_multicert.config could > be reloaded at any point, we acquire() a copy of the pointer before going > into the PRE_HANDSHAKE and SNI hooks, and release() that pointer at the end. > Originally I thought of having a TSSslCertTableGet() and then pass that into > the TSSslCertFind*() functions, but decided to squish out the explicit > LookupTable call. > > Actually, as I'm writing this up, I realize that since I removed the explicit > reference to the Lookup table from the API, I don't need to cache a copy of > the lookup table in the framework. The TSSslCertFind*() functions can just > acquire and release as they go. I'll go ahead and do that API simplification. > That will remove the TSSslVConn argument. Great! >> Previously, then we looked at SSL integration APIs, we eschewed wrapped >> types like TSSslContext in favour of directly returning OpenSSL pointers (as >> void *). My argument at the time was that there's a lot of API surface >> needed to deal with SSL and not much value in just wrapping OpenSSL. > > Can definitely go either way here. Another benefit of the opaque types is > that the compiler can do some minimal type checking for you. Not a huge > issue for me either way. Fair enough. We should go back and make typedefs for the other API that exposes SSL * then. >> >> >> There's no TSSslVConnOp value for the default action? ie. to just accept the >> SSL session? > Definitely would be good to have the default to undo. Will get that added >> >> IIRC, OpenSSL doesn't guarantee anything about the SNI name except that is >> is a bag of bytes. Is it OK for TSSslVConnServernameGet() to present that as >> a C string? > The servername is not null terminated in the packet, but OpenSSL does null > terminate it before handing back the value via SSL_get_servername(). I went > back through the openssl to verify the null termination. Interestingly some > data structures in there are storing the servername as buffer plus length, > but the one returned is NULL terminated and data only. Internally they are > doing many strlen and strcmp operations on it. >> >> Finally, I'm not thrilled with the idea of using ssl_multicert.config to >> specify the tunneling action. What would you do if you had a wildcard cert >> and only wanted to tunnel a specific name? > In the wildcard case, e.g, a cert for *.bob.com, you could create a self > signed cert to represent fred.bob.com and then use that cert to enter a > config line to blind tunnel traffic to fred.bob.com. In the tunnel case the > cert is only used select traffic, so it need not be signed by a 'real' CA. > > Of course, I'm open to other options. As Alan noted, one could write a > plugin for any complex scenarios, but adding a config-only option for simple > cases seems like a good idea. Yeh, my concern here is that ssl_multicert is purely a data source right now. This change adds processing rules to it, which is a bit of a change of character. In fact, as your example above points out, it ends up doing 2 rather different things. What I'm planning to do later in the year is to rewrite ssl_multicert.config so that it works more like overridable records configuration. There are a number of SSL certificate settings that are only configurable globally, and duplicating everything in a different syntax in ssl_multicert is becoming unwieldy. I think the action is probably ok, but I hope we don't go too far down this path :) J
