You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2015/05/12 21:32:10 UTC
Re: svn commit: r1679032 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_ssl.xml modules/ssl/mod_ssl.c modules/ssl/ssl_engine_config.c
modules/ssl/ssl_private.h modules/ssl/ssl_util_stapling.c
On Tue, May 12, 2015 at 8:59 PM, <tr...@apache.org> wrote:
> Author: trawick
> Date: Tue May 12 18:59:29 2015
> New Revision: 1679032
>
> URL: http://svn.apache.org/r1679032
> Log:
> mod_ssl OCSP Stapling: Don't block initial handshakes while refreshing
> the OCSP response for a different certificate. mod_ssl has an additional
> global mutex, "ssl-stapling-refresh".
>
[]
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c?rev=1679032&r1=1679031&r2=1679032&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c Tue May 12 18:59:29 2015
[]
> +
> +static int get_and_check_cached_response(server_rec *s, modssl_ctx_t *mctx,
> + OCSP_RESPONSE **rsp, BOOL *ok,
> + certinfo *cinf, apr_pool_t *p)
> +{
> + int rv;
> +
> + /* Check to see if we already have a response for this certificate */
> + rv = stapling_get_cached_response(s, rsp, ok, cinf, p);
> + if (rv == FALSE) {
> + return SSL_TLSEXT_ERR_ALERT_FATAL;
> + }
> +
> + if (*rsp) {
> + /* see if response is acceptable */
> + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01953)
> + "stapling_cb: retrieved cached response");
> + rv = stapling_check_response(s, mctx, cinf, *rsp, NULL);
> + if (rv == SSL_TLSEXT_ERR_ALERT_FATAL) {
> + OCSP_RESPONSE_free(*rsp);
> + return SSL_TLSEXT_ERR_ALERT_FATAL;
> + }
> + else if (rv == SSL_TLSEXT_ERR_NOACK) {
> + /* Error in response. If this error was not present when it was
> + * stored (i.e. response no longer valid) then it can be
> + * renewed straight away.
> + *
> + * If the error *was* present at the time it was stored then we
> + * don't renew the response straight away; we just wait for the
> + * cached response to expire.
> + */
> + if (ok) {
if (*ok) ?
Or maybe 'ok' shouldn't be a pointer (not updated here)?
> + OCSP_RESPONSE_free(*rsp);
> + *rsp = NULL;
> + }
> + else if (!mctx->stapling_return_errors) {
> + OCSP_RESPONSE_free(*rsp);
> + return SSL_TLSEXT_ERR_NOACK;
> + }
> + }
> + }
> + return 0;
> +}
> +
Regards,
Yann.
Re: svn commit: r1679032 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_ssl.xml modules/ssl/mod_ssl.c modules/ssl/ssl_engine_config.c
modules/ssl/ssl_private.h modules/ssl/ssl_util_stapling.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, May 13, 2015 at 4:28 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On 05/13/2015 08:59 AM, Yann Ylavic wrote:
>>
>> On Wed, May 13, 2015 at 2:34 PM, Jeff Trawick <tr...@gmail.com> wrote:
>>>
>>> Thanks again!
>>
>> You're welcome ;)
>>
>> WDYT of the following?
>> (cosmetic only, but helps read/reuse-ability a bit)
>>
>> Index: modules/ssl/ssl_util_stapling.c
>> ===================================================================
>> --- modules/ssl/ssl_util_stapling.c (revision 1679195)
>> +++ modules/ssl/ssl_util_stapling.c (working copy)
>> @@ -250,13 +250,11 @@ static BOOL stapling_cache_response(server_rec *s,
>>
>> i2d_OCSP_RESPONSE(rsp, &p);
>>
>> - if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
>> - stapling_cache_mutex_on(s);
>> + stapling_cache_mutex_on(s);
>> rv = mc->stapling_cache->store(mc->stapling_cache_context, s,
>> cinf->idx, sizeof(cinf->idx),
>> expiry, resp_der, stored_len, pool);
>> - if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
>> - stapling_cache_mutex_off(s);
>> + stapling_cache_mutex_off(s);
>
>
> At the moment I very slightly prefer seeing the reminder that there isn't
> always a mutex, but I won't care before long. I prefer that this matches
> the implementation of the session cache mutex on where the socache flag is
> checked, but if it makes you happy and you change the session cache
> equivalent to match then go for it :)
Nope, I have no strong opinion either, let's keep it as is, that makes sense.
Re: svn commit: r1679032 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_ssl.xml
modules/ssl/mod_ssl.c modules/ssl/ssl_engine_config.c modules/ssl/ssl_private.h
modules/ssl/ssl_util_stapling.c
Posted by Jeff Trawick <tr...@gmail.com>.
On 05/13/2015 08:59 AM, Yann Ylavic wrote:
> On Wed, May 13, 2015 at 2:34 PM, Jeff Trawick <tr...@gmail.com> wrote:
>> Thanks again!
> You're welcome ;)
>
> WDYT of the following?
> (cosmetic only, but helps read/reuse-ability a bit)
>
> Index: modules/ssl/ssl_util_stapling.c
> ===================================================================
> --- modules/ssl/ssl_util_stapling.c (revision 1679195)
> +++ modules/ssl/ssl_util_stapling.c (working copy)
> @@ -250,13 +250,11 @@ static BOOL stapling_cache_response(server_rec *s,
>
> i2d_OCSP_RESPONSE(rsp, &p);
>
> - if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
> - stapling_cache_mutex_on(s);
> + stapling_cache_mutex_on(s);
> rv = mc->stapling_cache->store(mc->stapling_cache_context, s,
> cinf->idx, sizeof(cinf->idx),
> expiry, resp_der, stored_len, pool);
> - if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
> - stapling_cache_mutex_off(s);
> + stapling_cache_mutex_off(s);
At the moment I very slightly prefer seeing the reminder that there
isn't always a mutex, but I won't care before long. I prefer that this
matches the implementation of the session cache mutex on where the
socache flag is checked, but if it makes you happy and you change the
session cache equivalent to match then go for it :)
> if (rv != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01929)
> "stapling_cache_response: OCSP response session
> store error!");
> @@ -277,13 +275,11 @@ static BOOL stapling_get_cached_response(server_re
> const unsigned char *p;
> unsigned int resp_derlen = MAX_STAPLING_DER;
>
> - if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
> - stapling_cache_mutex_on(s);
> + stapling_cache_mutex_on(s);
> rv = mc->stapling_cache->retrieve(mc->stapling_cache_context, s,
> cinf->idx, sizeof(cinf->idx),
> resp_der, &resp_derlen, pool);
> - if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
> - stapling_cache_mutex_off(s);
> + stapling_cache_mutex_off(s);
> if (rv != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01930)
> "stapling_get_cached_response: cache miss");
> @@ -623,8 +619,11 @@ static int stapling_cache_mutex_on(server_rec *s)
> {
> SSLModConfigRec *mc = myModConfig(s);
>
> - return stapling_mutex_on(s, mc->stapling_cache_mutex,
> - SSL_STAPLING_CACHE_MUTEX_TYPE);
> + if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE) {
> + return stapling_mutex_on(s, mc->stapling_cache_mutex,
> + SSL_STAPLING_CACHE_MUTEX_TYPE);
> + }
> + return TRUE;
> }
>
> static int stapling_cache_mutex_off(server_rec *s)
> @@ -631,8 +630,11 @@ static int stapling_cache_mutex_off(server_rec *s)
> {
> SSLModConfigRec *mc = myModConfig(s);
>
> - return stapling_mutex_off(s, mc->stapling_cache_mutex,
> - SSL_STAPLING_CACHE_MUTEX_TYPE);
> + if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE) {
> + return stapling_mutex_off(s, mc->stapling_cache_mutex,
> + SSL_STAPLING_CACHE_MUTEX_TYPE);
> + }
> + return TRUE;
> }
>
> static int stapling_refresh_mutex_on(server_rec *s)
> --
Re: svn commit: r1679032 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/mod_ssl.xml modules/ssl/mod_ssl.c modules/ssl/ssl_engine_config.c
modules/ssl/ssl_private.h modules/ssl/ssl_util_stapling.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, May 13, 2015 at 2:34 PM, Jeff Trawick <tr...@gmail.com> wrote:
>
> Thanks again!
You're welcome ;)
WDYT of the following?
(cosmetic only, but helps read/reuse-ability a bit)
Index: modules/ssl/ssl_util_stapling.c
===================================================================
--- modules/ssl/ssl_util_stapling.c (revision 1679195)
+++ modules/ssl/ssl_util_stapling.c (working copy)
@@ -250,13 +250,11 @@ static BOOL stapling_cache_response(server_rec *s,
i2d_OCSP_RESPONSE(rsp, &p);
- if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
- stapling_cache_mutex_on(s);
+ stapling_cache_mutex_on(s);
rv = mc->stapling_cache->store(mc->stapling_cache_context, s,
cinf->idx, sizeof(cinf->idx),
expiry, resp_der, stored_len, pool);
- if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
- stapling_cache_mutex_off(s);
+ stapling_cache_mutex_off(s);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01929)
"stapling_cache_response: OCSP response session
store error!");
@@ -277,13 +275,11 @@ static BOOL stapling_get_cached_response(server_re
const unsigned char *p;
unsigned int resp_derlen = MAX_STAPLING_DER;
- if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
- stapling_cache_mutex_on(s);
+ stapling_cache_mutex_on(s);
rv = mc->stapling_cache->retrieve(mc->stapling_cache_context, s,
cinf->idx, sizeof(cinf->idx),
resp_der, &resp_derlen, pool);
- if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE)
- stapling_cache_mutex_off(s);
+ stapling_cache_mutex_off(s);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01930)
"stapling_get_cached_response: cache miss");
@@ -623,8 +619,11 @@ static int stapling_cache_mutex_on(server_rec *s)
{
SSLModConfigRec *mc = myModConfig(s);
- return stapling_mutex_on(s, mc->stapling_cache_mutex,
- SSL_STAPLING_CACHE_MUTEX_TYPE);
+ if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE) {
+ return stapling_mutex_on(s, mc->stapling_cache_mutex,
+ SSL_STAPLING_CACHE_MUTEX_TYPE);
+ }
+ return TRUE;
}
static int stapling_cache_mutex_off(server_rec *s)
@@ -631,8 +630,11 @@ static int stapling_cache_mutex_off(server_rec *s)
{
SSLModConfigRec *mc = myModConfig(s);
- return stapling_mutex_off(s, mc->stapling_cache_mutex,
- SSL_STAPLING_CACHE_MUTEX_TYPE);
+ if (mc->stapling_cache->flags & AP_SOCACHE_FLAG_NOTMPSAFE) {
+ return stapling_mutex_off(s, mc->stapling_cache_mutex,
+ SSL_STAPLING_CACHE_MUTEX_TYPE);
+ }
+ return TRUE;
}
static int stapling_refresh_mutex_on(server_rec *s)
--
Re: svn commit: r1679032 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_ssl.xml
modules/ssl/mod_ssl.c modules/ssl/ssl_engine_config.c modules/ssl/ssl_private.h
modules/ssl/ssl_util_stapling.c
Posted by Jeff Trawick <tr...@gmail.com>.
On 05/12/2015 04:50 PM, Jeff Trawick wrote:
> On 05/12/2015 03:32 PM, Yann Ylavic wrote:
>> On Tue, May 12, 2015 at 8:59 PM, <tr...@apache.org> wrote:
>>> Author: trawick
>>> Date: Tue May 12 18:59:29 2015
>>> New Revision: 1679032
>>>
>>> URL: http://svn.apache.org/r1679032
>>> Log:
>>> mod_ssl OCSP Stapling: Don't block initial handshakes while refreshing
>>> the OCSP response for a different certificate. mod_ssl has an
>>> additional
>>> global mutex, "ssl-stapling-refresh".
>>>
>> []
>>> Modified: httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c
>>> URL:
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c?rev=1679032&r1=1679031&r2=1679032&view=diff
>>> ==============================================================================
>>>
>>> --- httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c (original)
>>> +++ httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c Tue May 12
>>> 18:59:29 2015
>> []
>>> +
>>> +static int get_and_check_cached_response(server_rec *s,
>>> modssl_ctx_t *mctx,
>>> + OCSP_RESPONSE **rsp, BOOL
>>> *ok,
>>> + certinfo *cinf, apr_pool_t
>>> *p)
>>> +{
>>> + int rv;
>>> +
>>> + /* Check to see if we already have a response for this
>>> certificate */
>>> + rv = stapling_get_cached_response(s, rsp, ok, cinf, p);
>>> + if (rv == FALSE) {
>>> + return SSL_TLSEXT_ERR_ALERT_FATAL;
>>> + }
>>> +
>>> + if (*rsp) {
>>> + /* see if response is acceptable */
>>> + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01953)
>>> + "stapling_cb: retrieved cached response");
>>> + rv = stapling_check_response(s, mctx, cinf, *rsp, NULL);
>>> + if (rv == SSL_TLSEXT_ERR_ALERT_FATAL) {
>>> + OCSP_RESPONSE_free(*rsp);
>>> + return SSL_TLSEXT_ERR_ALERT_FATAL;
>>> + }
>>> + else if (rv == SSL_TLSEXT_ERR_NOACK) {
>>> + /* Error in response. If this error was not present
>>> when it was
>>> + * stored (i.e. response no longer valid) then it can be
>>> + * renewed straight away.
>>> + *
>>> + * If the error *was* present at the time it was stored
>>> then we
>>> + * don't renew the response straight away; we just wait
>>> for the
>>> + * cached response to expire.
>>> + */
>>> + if (ok) {
>> if (*ok) ?
>> Or maybe 'ok' shouldn't be a pointer (not updated here)?
>
> Thanks a bunch! I'll sort it out tomorrow
r1679192
> and make sure I'm testing more paths.
TBD :)
Thanks again!
>>
>>> + OCSP_RESPONSE_free(*rsp);
>>> + *rsp = NULL;
>>> + }
>>> + else if (!mctx->stapling_return_errors) {
>>> + OCSP_RESPONSE_free(*rsp);
>>> + return SSL_TLSEXT_ERR_NOACK;
>>> + }
>>> + }
>>> + }
>>> + return 0;
>>> +}
>>> +
>> Regards,
>> Yann.
>
Re: svn commit: r1679032 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_ssl.xml
modules/ssl/mod_ssl.c modules/ssl/ssl_engine_config.c modules/ssl/ssl_private.h
modules/ssl/ssl_util_stapling.c
Posted by Jeff Trawick <tr...@gmail.com>.
On 05/12/2015 03:32 PM, Yann Ylavic wrote:
> On Tue, May 12, 2015 at 8:59 PM, <tr...@apache.org> wrote:
>> Author: trawick
>> Date: Tue May 12 18:59:29 2015
>> New Revision: 1679032
>>
>> URL: http://svn.apache.org/r1679032
>> Log:
>> mod_ssl OCSP Stapling: Don't block initial handshakes while refreshing
>> the OCSP response for a different certificate. mod_ssl has an additional
>> global mutex, "ssl-stapling-refresh".
>>
> []
>> Modified: httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c?rev=1679032&r1=1679031&r2=1679032&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c (original)
>> +++ httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c Tue May 12 18:59:29 2015
> []
>> +
>> +static int get_and_check_cached_response(server_rec *s, modssl_ctx_t *mctx,
>> + OCSP_RESPONSE **rsp, BOOL *ok,
>> + certinfo *cinf, apr_pool_t *p)
>> +{
>> + int rv;
>> +
>> + /* Check to see if we already have a response for this certificate */
>> + rv = stapling_get_cached_response(s, rsp, ok, cinf, p);
>> + if (rv == FALSE) {
>> + return SSL_TLSEXT_ERR_ALERT_FATAL;
>> + }
>> +
>> + if (*rsp) {
>> + /* see if response is acceptable */
>> + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01953)
>> + "stapling_cb: retrieved cached response");
>> + rv = stapling_check_response(s, mctx, cinf, *rsp, NULL);
>> + if (rv == SSL_TLSEXT_ERR_ALERT_FATAL) {
>> + OCSP_RESPONSE_free(*rsp);
>> + return SSL_TLSEXT_ERR_ALERT_FATAL;
>> + }
>> + else if (rv == SSL_TLSEXT_ERR_NOACK) {
>> + /* Error in response. If this error was not present when it was
>> + * stored (i.e. response no longer valid) then it can be
>> + * renewed straight away.
>> + *
>> + * If the error *was* present at the time it was stored then we
>> + * don't renew the response straight away; we just wait for the
>> + * cached response to expire.
>> + */
>> + if (ok) {
> if (*ok) ?
> Or maybe 'ok' shouldn't be a pointer (not updated here)?
Thanks a bunch! I'll sort it out tomorrow and make sure I'm testing
more paths.
>
>> + OCSP_RESPONSE_free(*rsp);
>> + *rsp = NULL;
>> + }
>> + else if (!mctx->stapling_return_errors) {
>> + OCSP_RESPONSE_free(*rsp);
>> + return SSL_TLSEXT_ERR_NOACK;
>> + }
>> + }
>> + }
>> + return 0;
>> +}
>> +
> Regards,
> Yann.