Re: [users@httpd] mod_authz_dbd regression in apache 2.4.12?
Committed in http://svn.apache.org/r1679181 (and r1679182 for CHANGES entry).
Backport to 2.4.x proposed in r1679183 (including Jan's r1663647).
On Tue, May 12, 2015 at 1:46 PM, Yann Ylavic ylavic@gmail.com wrote:
(CC'ing Michel, sorry for the resend, my initial omission)
On Tue, May 12, 2015 at 10:41 AM, Yann Ylavic ylavic@gmail.com wrote:
This as been raised on users@.
-- Forwarded message --
From: Yann Ylavic ylavic@gmail.com
Date: Tue, May 12, 2015 at 10:09 AM
On Mon, May 11, 2015 at 10:54 PM, Michel Stam mic...@reverze.net wrote:
I was tinkering over the weekend with mod_authz_dbd and mysql, and i could
not get a RequireAny/RequireAll to match on multiple Require dbd-group
statements.
It would always match only the last result from the query, but once for
every row in the resultset.
Example:
LocationMatch /(?name[^/]+)/
RequireAny
Require user %{env:MATCH_NAME}
Require dbd-group %{env:MATCH_NAME}
Require dbd-group Administrators
/RequireAny
/LocationMatch
After some searching, it appeared to me to be a regression of this:
https://bz.apache.org/bugzilla/show_bug.cgi?id=46421
The fix mentioned there is about APR's dbd (mysql) code but has never
been pushed to a release (the bugzilla report is still open).
As already discussed in [1] (with a simililar fix for mod_authn_dbd in
[2]), I don't think it should be addressed in APR though (but in httpd
as you and the OP of bugzilla #46421 proposed).
There also seems to be other misuses of apr_dbd_get_entry() returned
values in httpd, I'll start a thread on the dev@ mailing-list and
propose a fix.
[1] http://www.mail-archive.com/dev@apr.apache.org/msg26024.html
[2] http://svn.apache.org/r1663647
-- End of forwarded message --
The issue is that apr_dbd_get_row()'s entries (usually pointed to by
apr_dbd_get_entry(), depending on dbd though) get destroyed whenever
apr_dbd_get_row() returns -1 (no more rows in iterative mode).
This seem to be the case for several dbd systems implemented in APR,
so I think we should take care of the entries' lifetime when used
after an apr_dbd_get_row() loop.
Thus, I think the attached patch should be applied, thoughts?
PS: there are also APR dbd systems where the entries are duplicated on
the apr_dbd_results' pool, so APR is not really consistent...
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 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, traw...@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=1679032r1=1679031r2=1679032view=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
On Wed, May 13, 2015 at 2:34 PM, Jeff Trawick traw...@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
On 05/13/2015 08:59 AM, Yann Ylavic wrote:
On Wed, May 13, 2015 at 2:34 PM, Jeff Trawick traw...@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
On Wed, May 13, 2015 at 4:28 PM, Jeff Trawick traw...@gmail.com wrote: On 05/13/2015 08:59 AM, Yann Ylavic wrote: On Wed, May 13, 2015 at 2:34 PM, Jeff Trawick traw...@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.
