Hello! On Fri, Nov 01, 2013 at 12:09:08PM +0000, Rob Stradling wrote:
> On 01/11/13 10:46, Maxim Dounin wrote: > <snip> > >>I'm afraid it's a much larger patch than I anticipated it would be > >>when I started working on it! > >> > >>Maxim, does this patch look commit-able? > > Maxim, thanks for your initial comments. > > >It looks like it needs to be broken down into a patch series to > >be at least reviewable. > > I thought you might say that. Is it acceptable for there to be > compilation errors if you only apply some of the patches in a patch > series? (I was assuming that would be unacceptable, hence the one > large patch). Each patch is expected to make sense by it's own, and shouldn't break anything previously working, including compilation (but may do e.g. otherwise unneeded and/or strange refactoring, or provide some incomplete functionality). > >I haven't looked into details yet, but I tend to dislike at least > >changing the ngx_ssl_certificate() function into a monster which > >configures everything. Preserving a separate call to configure > >stapling would be much better. > > I had hoped to keep those calls separate, but I couldn't see a clean > way to keep track of multiple server certs plus associated issuer > certs inbetween the calls to ngx_ssl_certificate() and > ngx_ssl_stapling(). > By combining the certificate configuration and stapling > configuration functions, I made this problem go away. > > To preserve ngx_ssl_certificate() and ngx_ssl_stapling() as separate > functions, I think I'd have to: > - change ngx_ssl_certificate_index to keep an array (either > ngx_array_t or STACK_OF) of server certs. > - have ngx_ssl_certificate() put all of the intermediate CA > certificates it encounters into a temporary cert store; have > ngx_ssl_stapling() look in this temporary cert store for issuer > certificates; then destroy the temporary cert store. > > Would that be preferable? Or do you have any better ideas? Given the number of things we have to store here and there, I tend to think we should eventually just add an index with some generic pointer to a struct with our data. To minimize changes in this particular case, using an array is probably good enough. > >Checks for extra ceritifcate chains with unsupported OpenSSL > >versions looks a bit too extensive. I would think of just > >dropping them completely. > > OK, (assuming you mean drop the checks, rather than drop support for > those OpenSSL versions!) Yes, I mean to drop checks. -- Maxim Dounin http://nginx.org/en/donation.html _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
