Backchannel logout stopped working with file-based cache

[damisanet]

Hi,

I've just noticed that backchannel logout stopped working on my servers (not sure when). After my IdP sends backchannel logout requests to apps, entries like:

oidc_cache_file_set: could not delete cache file "/var/cache/httpd/mod_auth_openidc/mod-auth-openidc-d-XXX.AAA.tmp" (No such file or directory)
oidc_cache_file_set: could not delete cache file "/var/cache/httpd/mod_auth_openidc/mod-auth-openidc-s-YYY.BBB.tmp" (No such file or directory)

started to appear in Apache error log (I've redacted this a bit) - and that's right, there are no such files. But at the same time:

$ ls /var/cache/httpd/mod_auth_openidc/
mod-auth-openidc-d-XXX
mod-auth-openidc-s-YYY

files without .tmp suffix exist.

I believe this problem is caused by changes in #777:

static apr_byte_t oidc_cache_file_set(request_rec *r, const char *section, const char *key,
		const char *value, apr_time_t expiry) {
        // ...
	/* get the fully qualified path to the cache file based on the key name */
	const char *target = oidc_cache_file_path(r, section, key);
	const char *path = apr_psprintf(r->pool, "%s.%s.tmp", target, rnd);
        // ...
	/* just remove cache file if value is NULL */
	if (value == NULL) {
		if ((rc = apr_file_remove(path, r->pool)) != APR_SUCCESS) {
			oidc_error(r, "could not delete cache file \"%s\" (%s)", path, apr_strerror(rc, s_err, sizeof(s_err)));
		}
		return TRUE;
	}

	/* try to open the cache file for writing, creating it if it does not exist */
	if ((rc = apr_file_open(&fd, path, (APR_FOPEN_WRITE | APR_FOPEN_CREATE),
							APR_OS_DEFAULT, r->pool)) != APR_SUCCESS) {
		oidc_error(r, "cache file \"%s\" could not be opened (%s)", path, apr_strerror(rc, s_err, sizeof(s_err)));
		return FALSE;
	}
        // ...
	if (rename(path, target) != 0) {
		oidc_error(r, "cache file: %s could not be renamed to: %s", path, target);
		return FALSE;
	}
        // ...
}

With NULL values, module is trying to delete randomized path it just generated - which is supposed to be used for creating new temporary file for atomic replacement at the end of function.

This simple patch seems to resolve this (but I'm unsure if that's a proper solution):

diff -ur a/src/cache/file.c b/src/cache/file.c
--- a/src/cache/file.c	2022-09-23 09:40:37.000000000 +0000
+++ b/src/cache/file.c	2022-11-05 22:02:10.533027257 +0000
@@ -392,7 +392,7 @@
 
 	/* just remove cache file if value is NULL */
 	if (value == NULL) {
-		if ((rc = apr_file_remove(path, r->pool)) != APR_SUCCESS) {
+		if ((rc = apr_file_remove(target, r->pool)) != APR_SUCCESS) {
 			oidc_error(r, "could not delete cache file \"%s\" (%s)", path, apr_strerror(rc, s_err, sizeof(s_err)));
 		}
 		return TRUE;

[zandbelt]

I think you're right, thanks for your report, I'll confirm asap.