You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2018/10/16 17:21:54 UTC
Re: svn commit: r1844001 - in /httpd/httpd/trunk/modules/ssl:
mod_ssl.c ssl_engine_kernel.c ssl_private.h
On 10/16/2018 02:53 PM, jfclere@apache.org wrote:
> Author: jfclere
> Date: Tue Oct 16 12:53:18 2018
> New Revision: 1844001
>
> URL: http://svn.apache.org/viewvc?rev=1844001&view=rev
> Log:
> And a way to custom modules to guess and extract ssl variable.
> See https://github.com/jfclere/JBCSP-17 for example...
>
> Modified:
> httpd/httpd/trunk/modules/ssl/mod_ssl.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
> httpd/httpd/trunk/modules/ssl/ssl_private.h
>
> Modified: httpd/httpd/trunk/modules/ssl/mod_ssl.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/mod_ssl.c?rev=1844001&r1=1844000&r2=1844001&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/mod_ssl.c (original)
> +++ httpd/httpd/trunk/modules/ssl/mod_ssl.c Tue Oct 16 12:53:18 2018
> @@ -776,6 +776,8 @@ static void ssl_register_hooks(apr_pool_
> AUTHZ_PROVIDER_VERSION,
> &ssl_authz_provider_verify_client,
> AP_AUTH_INTERNAL_PER_CONF);
> + ap_register_provider(p, "mod_ssl" , "ssl_variables", "0",
> + ssl_hook_GetVars());
1. Instead of using literals you should use defines for them in mod_ssl.h. I understand that you want to use this
in other modules. So it needs to become part of the mod_ssl API.
2. Also the provider seems to provide only one method it should be stored in a clearly defined struct like e.g.
authz_provider in mod_auth.h
3. Why using the provider interface at all? You have only one method and it doesn't look like that other providers
should be implemented. Looks like a perfect case for an optional function to me which we have various in mod_ssl,
e.g. ssl_proxy_enable
Regards
Rüdiger
Re: svn commit: r1844001 - in /httpd/httpd/trunk/modules/ssl:
mod_ssl.c ssl_engine_kernel.c ssl_private.h
Posted by jean-frederic clere <jf...@gmail.com>.
On 16/10/2018 19:21, Ruediger Pluem wrote:
>
>
> On 10/16/2018 02:53 PM, jfclere@apache.org wrote:
>> Author: jfclere
>> Date: Tue Oct 16 12:53:18 2018
>> New Revision: 1844001
>>
>> URL: http://svn.apache.org/viewvc?rev=1844001&view=rev
>> Log:
>> And a way to custom modules to guess and extract ssl variable.
>> See https://github.com/jfclere/JBCSP-17 for example...
>>
>> Modified:
>> httpd/httpd/trunk/modules/ssl/mod_ssl.c
>> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
>> httpd/httpd/trunk/modules/ssl/ssl_private.h
>>
>> Modified: httpd/httpd/trunk/modules/ssl/mod_ssl.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/mod_ssl.c?rev=1844001&r1=1844000&r2=1844001&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/ssl/mod_ssl.c (original)
>> +++ httpd/httpd/trunk/modules/ssl/mod_ssl.c Tue Oct 16 12:53:18 2018
>> @@ -776,6 +776,8 @@ static void ssl_register_hooks(apr_pool_
>> AUTHZ_PROVIDER_VERSION,
>> &ssl_authz_provider_verify_client,
>> AP_AUTH_INTERNAL_PER_CONF);
>> + ap_register_provider(p, "mod_ssl" , "ssl_variables", "0",
>> + ssl_hook_GetVars());
>
> 1. Instead of using literals you should use defines for them in mod_ssl.h. I understand that you want to use this
> in other modules. So it needs to become part of the mod_ssl API.
> 2. Also the provider seems to provide only one method it should be stored in a clearly defined struct like e.g.
> authz_provider in mod_auth.h
> 3. Why using the provider interface at all? You have only one method and it doesn't look like that other providers
> should be implemented. Looks like a perfect case for an optional function to me which we have various in mod_ssl,
> e.g. ssl_proxy_enable
That is a lot simpler in fact :D
--
Cheers
Jean-Frederic
Re: svn commit: r1844001 - in /httpd/httpd/trunk/modules/ssl:
mod_ssl.c ssl_engine_kernel.c ssl_private.h
Posted by jean-frederic clere <jf...@gmail.com>.
On 17/10/2018 15:44, Joe Orton wrote:
> On Wed, Oct 17, 2018 at 02:29:42PM +0200, jean-frederic clere wrote:
>> One of the customer complains is that having the variables exposed like
>> in StdEnvars has a huge performances cost (everything is exposed for
>> each request) , he wants to check one variable and then decide in his
>> code what are the other he needs to access and yes he wants to know what
>> we can expose...
>
> That is all possible with ssl_var_lookup() today. What is it you're
> trying to do that isn't possible (or efficient) today?
The customer is OK to use ssl_var_lookup(), I am good with that part ;-)
>
> The API which mod_ssl exposes is the list of SSL_ variable, a list of
> (name,value) pairs. That some of those keys are listed in the
> ssl_hook_Fixup_vars array and some are not is an implementation detail
> which makes no sense to expose in the API.
>
> If you wanted a new API which exposes the available SSL variable *names*
> without computing the values, that might makes sense but it is expensive
> in the general case because you'd still need to parse the DNs.
Good point, but it is still less expensive than using the StdEnvars
logic. I have rollback my commit and I am reworking it the next days.
Many thanks for the comments
>
>
>
--
Cheers
Jean-Frederic
Re: svn commit: r1844001 - in /httpd/httpd/trunk/modules/ssl:
mod_ssl.c ssl_engine_kernel.c ssl_private.h
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Oct 17, 2018 at 02:29:42PM +0200, jean-frederic clere wrote:
> One of the customer complains is that having the variables exposed like
> in StdEnvars has a huge performances cost (everything is exposed for
> each request) , he wants to check one variable and then decide in his
> code what are the other he needs to access and yes he wants to know what
> we can expose...
That is all possible with ssl_var_lookup() today. What is it you're
trying to do that isn't possible (or efficient) today?
The API which mod_ssl exposes is the list of SSL_ variable, a list of
(name,value) pairs. That some of those keys are listed in the
ssl_hook_Fixup_vars array and some are not is an implementation detail
which makes no sense to expose in the API.
If you wanted a new API which exposes the available SSL variable *names*
without computing the values, that might makes sense but it is expensive
in the general case because you'd still need to parse the DNs.
Re: svn commit: r1844001 - in /httpd/httpd/trunk/modules/ssl:
mod_ssl.c ssl_engine_kernel.c ssl_private.h
Posted by jean-frederic clere <jf...@gmail.com>.
On 17/10/2018 12:44, Joe Orton wrote:
> On Tue, Oct 16, 2018 at 07:21:54PM +0200, Ruediger Pluem wrote:
>> On 10/16/2018 02:53 PM, jfclere@apache.org wrote:
>>> Author: jfclere
>>> Date: Tue Oct 16 12:53:18 2018
>>> New Revision: 1844001
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1844001&view=rev
>>> Log:
>>> And a way to custom modules to guess and extract ssl variable.
>>> See https://github.com/jfclere/JBCSP-17 for example...
> ...
>> 1. Instead of using literals you should use defines for them in mod_ssl.h. I understand that you want to use this
>> in other modules. So it needs to become part of the mod_ssl API.
>> 2. Also the provider seems to provide only one method it should be stored in a clearly defined struct like e.g.
>> authz_provider in mod_auth.h
>> 3. Why using the provider interface at all? You have only one method and it doesn't look like that other providers
>> should be implemented. Looks like a perfect case for an optional function to me which we have various in mod_ssl,
>> e.g. ssl_proxy_enable
>
> Doesn't make sense to expose that structure at all IMO, it's an
> implementation detail. Might make sense to have a new optional function
> which is passed an apr_table_t * and populate it exactly as +StdEnvars
> does, i.e. factor that code out:
>
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c?revision=1844001&view=markup#l1549
>
> actually make it equivalent to ssl_hook_Fixup() as a whole maybe.
One of the customer complains is that having the variables exposed like
in StdEnvars has a huge performances cost (everything is exposed for
each request) , he wants to check one variable and then decide in his
code what are the other he needs to access and yes he wants to know what
we can expose...
I see that just exposing, like StdEnvars the ones he may need already
helps, but that isn't what he wants.
>
>
--
Cheers
Jean-Frederic
Re: svn commit: r1844001 - in /httpd/httpd/trunk/modules/ssl:
mod_ssl.c ssl_engine_kernel.c ssl_private.h
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Oct 16, 2018 at 07:21:54PM +0200, Ruediger Pluem wrote:
> On 10/16/2018 02:53 PM, jfclere@apache.org wrote:
> > Author: jfclere
> > Date: Tue Oct 16 12:53:18 2018
> > New Revision: 1844001
> >
> > URL: http://svn.apache.org/viewvc?rev=1844001&view=rev
> > Log:
> > And a way to custom modules to guess and extract ssl variable.
> > See https://github.com/jfclere/JBCSP-17 for example...
...
> 1. Instead of using literals you should use defines for them in mod_ssl.h. I understand that you want to use this
> in other modules. So it needs to become part of the mod_ssl API.
> 2. Also the provider seems to provide only one method it should be stored in a clearly defined struct like e.g.
> authz_provider in mod_auth.h
> 3. Why using the provider interface at all? You have only one method and it doesn't look like that other providers
> should be implemented. Looks like a perfect case for an optional function to me which we have various in mod_ssl,
> e.g. ssl_proxy_enable
Doesn't make sense to expose that structure at all IMO, it's an
implementation detail. Might make sense to have a new optional function
which is passed an apr_table_t * and populate it exactly as +StdEnvars
does, i.e. factor that code out:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c?revision=1844001&view=markup#l1549
actually make it equivalent to ssl_hook_Fixup() as a whole maybe.