SSLPolicy code questions/backport review

[Joe Orton <jo...@redhat.com>]

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!

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?

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?

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

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.

Regards, Joe

Re: SSLPolicy code questions/backport review

[Stefan Eissing <st...@greenbytes.de>]

Thanks for the review! I will take this and clean up the code, mod_ssl certainly deserves it.

> Am 23.05.2018 um 09:51 schrieb Joe Orton <jo...@redhat.com>:
> 
> 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!
> 
> 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?
> 
> 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?
> 
> 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).
> 
> 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.
> 
> Regards, Joe

Re: SSLPolicy code questions/backport review

[Stefan Eissing <st...@greenbytes.de>]

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 <jo...@redhat.com>:
> 
> 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