diff --git a/changes-entries/65172.txt b/changes-entries/65172.txt new file mode 100644 index 00000000000..a45044783d9 --- /dev/null +++ b/changes-entries/65172.txt @@ -0,0 +1,7 @@ + *) mod_authn_dbd, mod_authn_socache: Preserve the extra AuthDBDUserPWQuery + columns exported as AUTHENTICATE_ 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] diff --git a/modules/aaa/mod_authn_dbd.c b/modules/aaa/mod_authn_dbd.c index 83828697ac0..e83c6b5f909 100644 --- a/modules/aaa/mod_authn_dbd.c +++ b/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; } diff --git a/modules/aaa/mod_authn_socache.c b/modules/aaa/mod_authn_socache.c index 757083e40fe..fd7be7205a6 100644 --- a/modules/aaa/mod_authn_socache.c +++ b/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 */ diff --git a/test/modules/aaa/conftest.py b/test/modules/aaa/conftest.py index be9b8ba449f..6ee7264effb 100644 --- a/test/modules/aaa/conftest.py +++ b/test/modules/aaa/conftest.py @@ -102,6 +102,25 @@ def env(pytestconfig) -> AAATestEnv: # AuthDigestProvider intentionally omitted: falls back to "file". f'AuthUserFile "{pwfile}"', ])) + # A Basic-auth location backed by the "socache dbd" provider pair, to + # exercise AuthDBDUserPWQuery columns beyond the password surviving a + # cached (as opposed to first, uncached) login (PR 65172). + conf.add([ + 'DBDriver sqlite3', + f'DBDParams "{env.dbd_file}"', + 'AuthnCacheSOCache shmcb', + 'AddHandler cgi-script .py', + f'', + ' Options +ExecCGI', + ' AuthType Basic', + ' AuthName "AAA DBD Socache Realm"', + ' AuthBasicProvider socache dbd', + ' AuthnCacheProvideFor dbd', + ' AuthDBDUserPWQuery "SELECT password, role FROM users WHERE user = %s"', + ' AuthnCacheTimeout 300', + ' Require valid-user', + '', + ]) # A request which fails header validation (e.g. no Host on HTTP/1.1) # is answered before the Digest module's post_read_request runs, so it # has no per-request Digest record; an ErrorDocument redirecting it into diff --git a/test/modules/aaa/env.py b/test/modules/aaa/env.py index 87fcb667762..a7e2a5bf189 100644 --- a/test/modules/aaa/env.py +++ b/test/modules/aaa/env.py @@ -1,7 +1,9 @@ +import base64 import hashlib import inspect import logging import os +import sqlite3 from typing import List, Optional from pyhttpd.env import HttpdTestEnv, HttpdTestSetup @@ -16,7 +18,10 @@ def __init__(self, env: 'HttpdTestEnv'): super().__init__(env=env) self.add_source_dir(os.path.dirname(inspect.getfile(AAATestSetup))) self.add_modules(["auth_digest", "authn_file", "authn_core", - "authz_core", "authz_user", "proxy", "proxy_http"]) + "authz_core", "authz_user", "proxy", "proxy_http", + "dbd", "authn_dbd", "authn_socache", "auth_basic", + "socache_shmcb"]) + self.add_cgi_module() class AAATestEnv(HttpdTestEnv): @@ -30,15 +35,43 @@ class AAATestEnv(HttpdTestEnv): # accounts under the same realm. DIGEST_PASSWORD_VHOST2 = "vhost2pass2617" + DBD_USER = "dbduser" + DBD_PASSWORD = "dbdpass2617" + DBD_ROLE = "editor" + def __init__(self, pytestconfig=None): super().__init__(pytestconfig=pytestconfig) - self.add_httpd_log_modules(["auth_digest", "authn_file", "authz_core"]) + self.add_httpd_log_modules(["auth_digest", "authn_file", "authz_core", + "authn_dbd", "dbd"]) self._digest_pwfile = os.path.join(self.server_dir, "digest.passwd") self._digest_pwfile2 = os.path.join(self.server_dir, "digest2.passwd") + self._dbd_file = os.path.join(self.server_dir, "authn_dbd.sqlite3") def setup_httpd(self, setup: HttpdTestSetup = None): super().setup_httpd(setup=AAATestSetup(env=self)) self._write_digest_pwfile() + self._write_dbd_file() + + def _write_dbd_file(self): + if os.path.exists(self._dbd_file): + os.remove(self._dbd_file) + con = sqlite3.connect(self._dbd_file) + try: + con.execute("CREATE TABLE users (user TEXT, password TEXT, role TEXT)") + # "{SHA}" + base64(sha1(password)): the format ap_password_validate() + # checks itself, without depending on which crypt(3) algorithms the + # host libc offers. + digest = hashlib.sha1(self.DBD_PASSWORD.encode()).digest() + pw_hash = "{SHA}" + base64.b64encode(digest).decode() + con.execute("INSERT INTO users (user, password, role) VALUES (?, ?, ?)", + (self.DBD_USER, pw_hash, self.DBD_ROLE)) + con.commit() + finally: + con.close() + + @property + def dbd_file(self) -> str: + return self._dbd_file def _write_digest_pwfile(self): def ha1(user, password): diff --git a/test/modules/aaa/htdocs/dbd/cgi/whoami.py b/test/modules/aaa/htdocs/dbd/cgi/whoami.py new file mode 100755 index 00000000000..e337ccd5554 --- /dev/null +++ b/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', '')}") diff --git a/test/modules/aaa/test_016_authn_dbd_cache.py b/test/modules/aaa/test_016_authn_dbd_cache.py new file mode 100644 index 00000000000..505bc5675bd --- /dev/null +++ b/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_ 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}"