Since parts of the changes in mod_ssl for SSLPolicy have now been affected by changes for TLSv1.3 and there has not been real interest in backporting SSLPolicy this year anyway, I withdraw the proposal.
The TLSv1.3 changes are not fit for backport since I was unable to verify that my fixes to client certificate handling were working (I have no working test setup for that). So, I cleaned up after Joe's review and that's it. If someone has the energy/interest in backporting any of this, feel free. -Stefan In more detail inline: > Am 23.05.2018 um 09:51 schrieb Joe Orton <[email protected]>: > > Easier to do here than dump in STATUS; looking at reviewing the 2.4.x > backport: > > https://svn.apache.org/repos/asf/httpd/httpd/patches/2.4.x/ssl-policy.patch > > 0. Overall looks good, I like the way this has been done! Thanks. > 1. Is there a reason why we need SSLPolicyRec with (name, sc) members > rather than having a hash directly of SSLSrvConfigRec *? The only time > ->name is used seems to be when creating the hash in add_policy(). > [Reading back through commits, I guess this was a legacy of having ->dc > in there too] > > 2. Storing the policies hash off pconf userdata seems surprising, is > there a reason that's done rather than putting it directly in > SSLModConfigRec? They are also looked up during configuration where SSLModConfigRec may is not available. See ssl_engine_config.c, line 608 > 3. get_policy_names() seems be only now called with create=1, and hence > the same is true for get_policies(), so there are some redundant paths + > redundant arguments here which could be removed? I'll eliminiate the parameter. > > 4. There is a bunch of legacy from the now-removed SSLPolicyDefine > AFAICT still included on trunk (not in the 2.4 patch); there is a > reference added in the docs in the 2.4 patch though. e.g. > > BOOL ssl_config_global_isfixed(SSLModConfigRec *mc) > { > - return mc->bFixed; > + return mc && mc->bFixed; > > is I think related and in trunk all the ssl_cmd_* handling of mc==NULL > case (again not in the backport). I'll revert that change. It caused crashes when indirectly called during configuration, as was the case in policy definition, I think. > 5. Very minor, please don't reformat the code now, but httpd code style > has case X: statements lined up with switch rather than indented. Uhm...ok? > Regards, Joe
