mod_authn_dbd, mod_authn_socache: Preserve extra query columns on a c…

changes-entries/65172.txt

@@ -0,0 +1,7 @@
+  *) mod_authn_dbd, mod_authn_socache: Preserve the extra AuthDBDUserPWQuery
+     columns exported as AUTHENTICATE_<NAME> environment variables across
+     a cached (AuthBasicProvider/AuthDigestProvider "socache dbd") login.
+     Previously, only the first, uncached login for a user carried those
+     variables; every following login served from the cache lost them
+     silently until the cache entry expired. PR 65172.
+     [Arturo Bernal]

modules/aaa/mod_authn_dbd.c

@@ -92,11 +92,28 @@ static const command_rec authn_dbd_cmds[] =
     {NULL}
 };
 
+/* Append "name=value\n" to *cache_extra, for a column already exported to
+ * r->subprocess_env, so a cache hit can restore it (see
+ * restore_cached_env() in mod_authn_socache.c). A value holding a '\n'
+ * is skipped: it would be misread back as the start of a new entry. Such
+ * a column is still set in this request's own environment above, only
+ * not preserved across a cache hit.
+ */
+static void authn_dbd_cache_append(apr_pool_t *p, const char **cache_extra,
+                                   const char *name, const char *value)
+{
+    if (value && !ap_strchr_c(value, '\n')) {
+        *cache_extra = apr_pstrcat(p, *cache_extra ? *cache_extra : "",
+                                   name, "=", value, "\n", NULL);
+    }
+}
+
 static authn_status authn_dbd_password(request_rec *r, const char *user,
                                        const char *password)
 {
     apr_status_t rv;
     const char *dbd_password = NULL;
+    const char *cache_extra = NULL;
     apr_dbd_prepared_t *statement;
     apr_dbd_results_t *res = NULL;
     apr_dbd_row_t *row = NULL;
@@ -165,6 +182,8 @@ static authn_status authn_dbd_password(request_rec *r, const char *user,
                 }
                 apr_table_set(r->subprocess_env, str,
                               apr_dbd_get_entry(dbd->driver, row, i));
+                authn_dbd_cache_append(r->pool, &cache_extra, str,
+                                      apr_dbd_get_entry(dbd->driver, row, i));
                 i++;
             }
 
@@ -177,7 +196,9 @@ static authn_status authn_dbd_password(request_rec *r, const char *user,
     if (!dbd_password) {
         return AUTH_USER_NOT_FOUND;
     }
-    AUTHN_CACHE_STORE(r, user, NULL, dbd_password);
+    AUTHN_CACHE_STORE(r, user, NULL, cache_extra
+                      ? apr_pstrcat(r->pool, dbd_password, "\n", cache_extra, NULL)
+                      : dbd_password);
 
     rv = ap_password_validate(r, user, password, dbd_password);
 
@@ -193,6 +214,7 @@ static authn_status authn_dbd_realm(request_rec *r, const char *user,
 {
     apr_status_t rv;
     const char *dbd_hash = NULL;
+    const char *cache_extra = NULL;
     apr_dbd_prepared_t *statement;
     apr_dbd_results_t *res = NULL;
     apr_dbd_row_t *row = NULL;
@@ -260,6 +282,8 @@ static authn_status authn_dbd_realm(request_rec *r, const char *user,
                 }
                 apr_table_set(r->subprocess_env, str,
                               apr_dbd_get_entry(dbd->driver, row, i));
+                authn_dbd_cache_append(r->pool, &cache_extra, str,
+                                      apr_dbd_get_entry(dbd->driver, row, i));
                 i++;
             }
 
@@ -272,7 +296,9 @@ static authn_status authn_dbd_realm(request_rec *r, const char *user,
     if (!dbd_hash) {
         return AUTH_USER_NOT_FOUND;
     }
-    AUTHN_CACHE_STORE(r, user, realm, dbd_hash);
+    AUTHN_CACHE_STORE(r, user, realm, cache_extra
+                      ? apr_pstrcat(r->pool, dbd_hash, "\n", cache_extra, NULL)
+                      : dbd_hash);
     *rethash = apr_pstrdup(r->pool, dbd_hash);
     return AUTH_USER_FOUND;
 }

modules/aaa/mod_authn_socache.c

@@ -348,6 +348,50 @@ static void ap_authn_cache_store(request_rec *r, const char *module,
     }
 }
 
+/* A cached value may carry additional per-request environment entries
+ * after the password hash, one NAME=VALUE pair per line, appended by a
+ * provider whose real (uncached) lookup also populates
+ * r->subprocess_env (e.g. mod_authn_dbd's extra result columns, see
+ * authn_dbd_cache_append() there). Restore any such entries, and shrink
+ * *vallen to cover only the hash itself, as if they had never been
+ * appended, so a cache hit behaves like a real lookup to the caller.
+ */
+static void restore_cached_env(request_rec *r, char *val, unsigned int *vallen)
+{
+    unsigned int hashlen = 0;
+    char *extra;
+    unsigned int extralen;
+
+    while (hashlen < *vallen && val[hashlen] != '\n') {
+        hashlen++;
+    }
+    if (hashlen == *vallen) {
+        return;
+    }
+
+    extra = val + hashlen + 1;
+    extralen = *vallen - hashlen - 1;
+    *vallen = hashlen;
+
+    while (extralen) {
+        char *nl = memchr(extra, '\n', extralen);
+        apr_size_t linelen = nl ? (apr_size_t)(nl - extra) : extralen;
+        char *eq = memchr(extra, '=', linelen);
+
+        if (eq) {
+            apr_table_setn(r->subprocess_env,
+                          apr_pstrmemdup(r->pool, extra, eq - extra),
+                          apr_pstrmemdup(r->pool, eq + 1,
+                                        linelen - (eq - extra) - 1));
+        }
+        if (!nl) {
+            break;
+        }
+        extra = nl + 1;
+        extralen -= linelen + 1;
+    }
+}
+
 #define MAX_VAL_LEN 256
 static authn_status check_password(request_rec *r, const char *user,
                                    const char *password)
@@ -386,6 +430,7 @@ static authn_status check_password(request_rec *r, const char *user,
         /* OK, we got a value */
         ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01685)
                       "Authn cache: found credentials for %s", user);
+        restore_cached_env(r, (char *)val, &vallen);
         val[vallen] = 0;
     }
     else {
@@ -431,6 +476,7 @@ static authn_status get_realm_hash(request_rec *r, const char *user,
         /* OK, we got a value */
         ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01688)
                       "Authn cache: found credentials for %s", user);
+        restore_cached_env(r, (char *)val, &vallen);
     }
     else {
         /* error: give up and pass the buck */

test/modules/aaa/htdocs/dbd/cgi/whoami.py

@@ -0,0 +1,6 @@
+#!/usr/bin/env python3
+import os
+
+print("Content-Type: text/plain\n")
+print(f"user={os.environ.get('REMOTE_USER', '')}")
+print(f"role={os.environ.get('AUTHENTICATE_ROLE', '')}")

test/modules/aaa/test_016_authn_dbd_cache.py

@@ -0,0 +1,44 @@
+"""mod_authn_dbd's extra query columns must survive a socache cache hit.
+
+AuthDBDUserPWQuery may select columns beyond the password (documented
+feature: modules/aaa/mod_authn_dbd.c exports each extra column as
+AUTHENTICATE_<NAME> in r->subprocess_env). With AuthBasicProvider
+"socache dbd", the first (uncached) login goes through
+authn_dbd_password() and gets those variables; mod_authn_socache then
+caches only the password hash. Every following login for the same user,
+while the cache entry is still fresh, is answered by mod_authn_socache's
+own check_password() directly from the cached hash -- authn_dbd_password()
+is never called again, so AUTHENTICATE_* is silently absent (PR 65172).
+"""
+
+from .env import AAATestEnv
+
+
+class TestAuthnDbdSocacheCache:
+
+    def url(self, env):
+        return env.mkurl("http", "aaa", "/dbd/cgi/whoami.py")
+
+    def whoami(self, env):
+        r = env.curl_get(self.url(env), options=[
+            "-u", f"{AAATestEnv.DBD_USER}:{AAATestEnv.DBD_PASSWORD}",
+        ])
+        assert r.exit_code == 0, f"{r}"
+        assert r.response["status"] == 200, f"{r}"
+        return r.response["body"].decode()
+
+    def test_authn_dbd_016_role_survives_cached_login(self, env):
+        # First login: a cache miss, goes through the real DBD query.
+        body1 = self.whoami(env)
+        assert f"user={AAATestEnv.DBD_USER}" in body1
+        assert f"role={AAATestEnv.DBD_ROLE}" in body1, \
+            f"AUTHENTICATE_ROLE missing on the first (uncached) login: {body1!r}"
+
+        # Second login with the same credentials, within AuthnCacheTimeout:
+        # a cache hit. Unpatched, AUTHENTICATE_ROLE silently disappears here
+        # because authn_dbd_password() (the only code that sets it) is
+        # never invoked for a cache hit.
+        body2 = self.whoami(env)
+        assert f"user={AAATestEnv.DBD_USER}" in body2
+        assert f"role={AAATestEnv.DBD_ROLE}" in body2, \
+            f"AUTHENTICATE_ROLE lost on a cached login: {body2!r}"