public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: Zsolt Parragi <[email protected]>
Cc: Nikolay Shaplov <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: VASUKI M <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: [email protected]
Cc: Robert Haas <[email protected]>
Cc: [email protected]
Subject: Re: Custom oauth validator options
Date: Fri, 20 Mar 2026 10:46:46 -0700
Message-ID: <CAOYmi+nTXGcroZD_Mnkc8LYWYFbfDYNR4ML_yQ5sF9+DY2amcg@mail.gmail.com> (raw)
In-Reply-To: <CAN4CZFMGwGdMnxP07Rk2qrC9eGQt31Lrerrnk66vQuzRhDEwiw@mail.gmail.com>
References: <CAN4CZFPmF9fGOcFubwOxqXymhVo_RvbUx3bLoYQcfk=f0mwECw@mail.gmail.com>
<[email protected]>
<CAN4CZFPUfTj-BF-m5=F7_MnY_T3+Qh-DuG7N7ojdbJDkT8JHeA@mail.gmail.com>
<[email protected]>
<CAN4CZFMCh3vOWGPbU5pTB-bwnoAtgFuDJmGGv7z7xeez+WJiag@mail.gmail.com>
<CAN4CZFMGwGdMnxP07Rk2qrC9eGQt31Lrerrnk66vQuzRhDEwiw@mail.gmail.com>
Hi all,
First, an apology for the state of this thread -- I thought I had
already responded to the prior message last month. Turns out, I had
not... Instead, I'm dropping a surprise alternative patchset on top of
a thread that I let go cold, which is rude, and I'm sorry.
Here are my overall thoughts on the approaches shared so far. And
thank you, Zsolt, for doing so much legwork here; that effort was not
wasted at all.
1) A GUC-centric solution -- option (d) -- is the Ideal Solution here,
IMO. We shouldn't have to reinvent the wheel.
2) A GUC-centric solution is not going to land for 19, and I'd be
surprised if it landed for 20, given the coalition that I'll need to
build to say either "yes" or "no". There are too many questions about
session_preload_libraries and prefixing and etc., and I'm honestly not
a fan of the guc_prefix_enforcement approach.
3) I'm not convinced yet that GUCs and relation options are similar
enough to share a framework. (This is not a rejection of the reloption
refactoring work, just a statement that I don't want to rely on it to
solve this problem.)
4) I really don't like the hba_parse_option_hook. I prefer APIs over
hooks as a general architectural point, and more practically, I don't
want to hand control to extensions during HBA parsing. I don't think
they're going to coordinate with each other well, and I think they're
likely to couple against internals in ways we don't want to support
(which is my general problem with hooks).
= Option (b) =
I don't want this problem to go unfixed for 2+ years, so I think it
would be best to reinvent a very small wheel that doesn't take up a
lot of maintenance effort once it's in, and then simply replace it
with the Ideal Solution eventually. This is my take on option (b),
which is what Vasuki M advocated for upthread. It's just a
setter/getter API for string keys and values:
static const char* opts[] = { "my_parameter" };
void startup_cb(ValidatorModuleState *state)
{
RegisterOAuthHBAOptions(state, lengthof(opts), opts);
}
bool validate_cb(const ValidatorModuleState *state, ...)
{
const char *param;
if ((param = GetOAuthHBAOption(state, "my_parameter")) != NULL)
do_something_with(param);
...
}
And then in your HBA:
host all all ::1/128 oauth validator.my_parameter=foo ...
The core implementation is conceptually simple. Most of the lines in
the patch are guardrails, to reduce the probability of regret over
this temporary solution:
- Parameter names aren't freeform; they're restricted to
almost-alphanumeric identifiers. (We've wanted to steal symbols for
HBA features in the past.)
- Name syntax is checked on reload, but cross-referencing the
registered list must be deferred to connection time. That's unusual
for users, so the WARNING that gets printed in this case is extremely
verbose; that way hopefully no one will be confused about what is
happening.
- The compiler won't let you register names during validate_cb, and we
won't let you retrieve options during startup_cb, so we retain control
over the internal order of operations. (This is discussed more deeply
in the patch.)
I was worried that we'd need a third API call to report option parsing
failures from the validator. Instead of doing that, I've made it
easier to link an authentication failure to a validator internal error
that caused it, in v3-0001, which serendipitously fixes a separate
sharp edge [1]. I feel good about that patch even if -0002 doesn't
make it.
v3-0002 still lacks user documentation, which I need to write a lot of
-- but if everyone dislikes this approach then I'd rather not spend
the time there.
= WDYT? =
I realize this puts you all in a difficult position; the effect is
kind of "take it or leave it", which again was not my intent. I
considered letting this lapse for 19 instead. But since I believe the
ideal solution is one we can't have for a while, and there's good
research and discussion of alternatives in this thread, waiting may
not produce a more committable short-term solution in the end.
Let me know if any of you disagree, though -- especially if you think
the status quo is preferable! -- and I can shelve -0002 for now. (I'll
continue with -0001 at [1] either way.)
Thanks,
--Jacob
[1] https://postgr.es/m/202601241015.y5uvxd7oxnfs%40alvherre.pgsql
Attachments:
[application/octet-stream] v3-0001-oauth-Let-validators-provide-failure-DETAILs.patch (12.6K, 2-v3-0001-oauth-Let-validators-provide-failure-DETAILs.patch)
download | inline diff:
From f36e6becc34d05834c3d808575b04d39badf7569 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Thu, 19 Mar 2026 09:37:20 -0700
Subject: [PATCH v3 1/2] oauth: Let validators provide failure DETAILs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
At the moment, the only way for a validator module to report error
details on failure is to log them separately before returning from
validate_cb. Independently of that problem, the ereport() calls that we
make during validation failure partially duplicate some of the work of
auth_failed().
The end result is overly verbose and confusing for readers of the logs:
[768233] LOG: [my_validator] bad signature in bearer token
[768233] LOG: OAuth bearer authentication failed for user "jacob"
[768233] DETAIL: Validator failed to authorize the provided token.
[768233] FATAL: OAuth bearer authentication failed for user "jacob"
[768233] DETAIL: Connection matched file ".../pg_hba.conf" line ...
Solve both problems by making use of the existing logdetail pointer
that's provided by ClientAuthentication. Validator modules may set
ValidatorModuleResult->error_detail to override our default generic
message.
The end result looks something like
[242284] FATAL: OAuth bearer authentication failed for user "jacob"
[242284] DETAIL: [my_validator] bad signature in bearer token
Connection matched file ".../pg_hba.conf" line ...
Reported-by: Álvaro Herrera <[email protected]>
Reported-by: Zsolt Parragi <[email protected]>
Discussion: https://postgr.es/m/202601241015.y5uvxd7oxnfs%40alvherre.pgsql
Discussion: TODO
---
doc/src/sgml/oauth-validators.sgml | 21 +++++++++-
src/include/libpq/oauth.h | 14 +++++++
src/backend/libpq/auth-oauth.c | 24 +++++------
src/backend/libpq/auth.c | 2 +-
.../modules/oauth_validator/t/001_server.pl | 40 ++++++++++++++++++-
src/test/modules/oauth_validator/validator.c | 29 ++++++++++++++
6 files changed, 114 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/oauth-validators.sgml b/doc/src/sgml/oauth-validators.sgml
index 704089dd7b3..a7140eae84e 100644
--- a/doc/src/sgml/oauth-validators.sgml
+++ b/doc/src/sgml/oauth-validators.sgml
@@ -192,11 +192,18 @@
<term>Logging</term>
<listitem>
<para>
- Modules may use the same <link linkend="error-message-reporting">logging
+ To simply log the reason for a validation failure, validators may set
+ the freeform <structfield>error_detail</structfield> field during the
+ <xref linkend="oauth-validator-callback-validate"/>. This is printed only
+ to the server log, as part of the final authentication failure message,
+ and it is not shared with the client.
+ </para>
+ <para>
+ Modules may also use the same <link linkend="error-message-reporting">logging
facilities</link> as standard extensions; however, the rules for emitting
log entries to the client are subtly different during the authentication
phase of the connection. Generally speaking, modules should log
- verification problems at the <symbol>COMMERROR</symbol> level and return
+ problems at the <symbol>COMMERROR</symbol> level and return
normally, instead of using <symbol>ERROR</symbol>/<symbol>FATAL</symbol>
to unwind the stack, to avoid leaking information to unauthenticated
clients.
@@ -370,6 +377,7 @@ typedef struct ValidatorModuleResult
{
bool authorized;
char *authn_id;
+ char *error_detail;
} ValidatorModuleResult;
</programlisting>
@@ -387,6 +395,15 @@ typedef struct ValidatorModuleResult
Otherwise the validator should return <literal>true</literal> to indicate
that it has processed the token and made an authorization decision.
</para>
+ <para>
+ In either failure case (validation error or internal error) the module may
+ store a user-readable reason for the failure in <structfield>result->error_detail</structfield>.
+ This will be printed to the server logs (not sent to the client) as a
+ <literal>DETAIL</literal> entry for the authentication failure. The memory
+ pointed to by <structfield>error_detail</structfield> may be either palloc'd
+ or of static duration. <structfield>error_detail</structfield> is ignored
+ on success.
+ </para>
<para>
The behavior after <function>validate_cb</function> returns depends on the
specific HBA setup. Normally, the <structfield>result->authn_id</structfield> user
diff --git a/src/include/libpq/oauth.h b/src/include/libpq/oauth.h
index 4a822e9a1f2..60f493acddd 100644
--- a/src/include/libpq/oauth.h
+++ b/src/include/libpq/oauth.h
@@ -49,6 +49,20 @@ typedef struct ValidatorModuleResult
* delegation. See the validator module documentation for details.
*/
char *authn_id;
+
+ /*
+ * When validation fails, this may optionally be set to a string
+ * containing an explanation for the failure. It will be sent to the
+ * server log only; it is not provided to the client, and it's ignored if
+ * validation succeeds.
+ *
+ * This description will be attached to the final authentication failure
+ * message in the logs, as a DETAIL, which may be preferable to separate
+ * ereport() calls that have to be correlated by the reader.
+ *
+ * This string may be either of static duration or palloc'd.
+ */
+ char *error_detail;
} ValidatorModuleResult;
/*
diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c
index 11365048951..6d4be70aada 100644
--- a/src/backend/libpq/auth-oauth.c
+++ b/src/backend/libpq/auth-oauth.c
@@ -73,7 +73,7 @@ struct oauth_ctx
static char *sanitize_char(char c);
static char *parse_kvpairs_for_auth(char **input);
static void generate_error_response(struct oauth_ctx *ctx, char **output, int *outputlen);
-static bool validate(Port *port, const char *auth);
+static bool validate(Port *port, const char *auth, const char **logdetail);
/* Constants seen in an OAUTHBEARER client initial response. */
#define KVSEP 0x01 /* separator byte for key/value pairs */
@@ -279,7 +279,7 @@ oauth_exchange(void *opaq, const char *input, int inputlen,
errmsg("malformed OAUTHBEARER message"),
errdetail("Message contains additional data after the final terminator."));
- if (!validate(ctx->port, auth))
+ if (!validate(ctx->port, auth, logdetail))
{
generate_error_response(ctx, output, outputlen);
@@ -635,7 +635,7 @@ validate_token_format(const char *header)
* authorization. Returns true if validation succeeds.
*/
static bool
-validate(Port *port, const char *auth)
+validate(Port *port, const char *auth, const char **logdetail)
{
int map_status;
ValidatorModuleResult *ret;
@@ -662,7 +662,10 @@ validate(Port *port, const char *auth)
{
ereport(WARNING,
errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("internal error in OAuth validator module"));
+ errmsg("internal error in OAuth validator module"),
+ ret->error_detail ? errdetail_log("%s", ret->error_detail) : 0);
+
+ *logdetail = ret->error_detail;
return false;
}
@@ -675,10 +678,10 @@ validate(Port *port, const char *auth)
if (!ret->authorized)
{
- ereport(LOG,
- errmsg("OAuth bearer authentication failed for user \"%s\"",
- port->user_name),
- errdetail_log("Validator failed to authorize the provided token."));
+ if (ret->error_detail)
+ *logdetail = ret->error_detail;
+ else
+ *logdetail = _("Validator failed to authorize the provided token.");
status = false;
goto cleanup;
@@ -699,10 +702,7 @@ validate(Port *port, const char *auth)
/* Make sure the validator authenticated the user. */
if (ret->authn_id == NULL || ret->authn_id[0] == '\0')
{
- ereport(LOG,
- errmsg("OAuth bearer authentication failed for user \"%s\"",
- port->user_name),
- errdetail_log("Validator provided no identity."));
+ *logdetail = _("Validator provided no identity.");
status = false;
goto cleanup;
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index e04aa2e68ed..6801ea9c566 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -625,7 +625,7 @@ ClientAuthentication(Port *port)
status = STATUS_OK;
break;
case uaOAuth:
- status = CheckSASLAuth(&pg_be_oauth_mech, port, NULL, NULL);
+ status = CheckSASLAuth(&pg_be_oauth_mech, port, NULL, &logdetail);
break;
}
diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl
index cdad2ae8011..28d123b833e 100644
--- a/src/test/modules/oauth_validator/t/001_server.pl
+++ b/src/test/modules/oauth_validator/t/001_server.pl
@@ -521,8 +521,8 @@ $node->connect_fails(
expected_stderr => qr/OAuth bearer authentication failed/,
log_like => [
qr/connection authenticated: identity=""/,
- qr/DETAIL:\s+Validator provided no identity/,
qr/FATAL:\s+OAuth bearer authentication failed/,
+ qr/DETAIL:\s+Validator provided no identity/,
]);
# Even if a validator authenticates the user, if the token isn't considered
@@ -541,10 +541,48 @@ $node->connect_fails(
expected_stderr => qr/OAuth bearer authentication failed/,
log_like => [
qr/connection authenticated: identity="test\@example\.org"/,
+ qr/FATAL:\s+OAuth bearer authentication failed/,
qr/DETAIL:\s+Validator failed to authorize the provided token/,
+ ]);
+
+# Validators can provide their own explanations.
+$bgconn->query_safe(
+ "ALTER SYSTEM SET oauth_validator.error_detail TO 'something failed'");
+$node->reload;
+$log_start =
+ $node->wait_for_log(qr/reloading configuration files/, $log_start);
+
+$node->connect_fails(
+ "$common_connstr user=test",
+ "validator must authorize token explicitly (custom logdetail)",
+ expected_stderr => qr/OAuth bearer authentication failed/,
+ log_like => [
+ qr/connection authenticated: identity="test\@example\.org"/,
qr/FATAL:\s+OAuth bearer authentication failed/,
+ qr/DETAIL:\s+something failed/,
]);
+$bgconn->query_safe(
+ "ALTER SYSTEM SET oauth_validator.internal_error TO true");
+$node->reload;
+$log_start =
+ $node->wait_for_log(qr/reloading configuration files/, $log_start);
+
+$node->connect_fails(
+ "$common_connstr user=test",
+ "validator internal error (custom logdetail)",
+ expected_stderr => qr/OAuth bearer authentication failed/,
+ log_like => [
+ qr/WARNING:\s+internal error in OAuth validator module/,
+ qr/DETAIL:\s+something failed/,
+ ]);
+
+$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.error_detail");
+$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.internal_error");
+$node->reload;
+$log_start =
+ $node->wait_for_log(qr/reloading configuration files/, $log_start);
+
#
# Test user mapping.
#
diff --git a/src/test/modules/oauth_validator/validator.c b/src/test/modules/oauth_validator/validator.c
index 0b983a9dc8f..353e0e0d32a 100644
--- a/src/test/modules/oauth_validator/validator.c
+++ b/src/test/modules/oauth_validator/validator.c
@@ -40,6 +40,8 @@ static const OAuthValidatorCallbacks validator_callbacks = {
/* GUCs */
static char *authn_id = NULL;
static bool authorize_tokens = true;
+static char *error_detail = NULL;
+static bool internal_error = false;
/*---
* Extension entry point. Sets up GUCs for use by tests:
@@ -51,6 +53,13 @@ static bool authorize_tokens = true;
* - oauth_validator.authorize_tokens
* Sets whether to successfully validate incoming
* tokens. Defaults to true.
+ *
+ * - oauth_validator.error_detail
+ * Sets an error message to be included as a
+ * DETAIL on failure.
+ *
+ * - oauth_validator.internal_error
+ * Reports an internal error to the server.
*/
void
_PG_init(void)
@@ -71,6 +80,22 @@ _PG_init(void)
PGC_SIGHUP,
0,
NULL, NULL, NULL);
+ DefineCustomStringVariable("oauth_validator.error_detail",
+ "Error message to print during failures",
+ NULL,
+ &error_detail,
+ NULL,
+ PGC_SIGHUP,
+ 0,
+ NULL, NULL, NULL);
+ DefineCustomBoolVariable("oauth_validator.internal_error",
+ "Should the validator report an internal error?",
+ NULL,
+ &internal_error,
+ false,
+ PGC_SIGHUP,
+ 0,
+ NULL, NULL, NULL);
MarkGUCPrefixReserved("oauth_validator");
}
@@ -133,6 +158,10 @@ validate_token(const ValidatorModuleState *state,
MyProcPort->hba->oauth_issuer,
MyProcPort->hba->oauth_scope);
+ res->error_detail = error_detail; /* only relevant for failures */
+ if (internal_error)
+ return false;
+
res->authorized = authorize_tokens;
if (authn_id)
res->authn_id = pstrdup(authn_id);
--
2.34.1
[application/octet-stream] v3-0002-WIP-oauth-Allow-validators-to-register-custom-HBA.patch (20.0K, 3-v3-0002-WIP-oauth-Allow-validators-to-register-custom-HBA.patch)
download | inline diff:
From 9726ac39442dd3af479903bfab62894765a6ab2f Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 17 Mar 2026 12:01:28 -0700
Subject: [PATCH v3 2/2] WIP: oauth: Allow validators to register custom HBA
options
(lacks user documentation)
Two new API entry points for validator callbacks:
- RegisterOAuthHBAOptions
- GetOAuthHBAOption
Registering options "foo" and "bar" allows a user to set validator.foo
and validator.bar on an `oauth` HBA line.
The bulk of the patch is not the conceptually simple API implementation,
but guardrails on the simple API to make sure it doesn't bind our hands
in the future, either for callback architecture or HBA syntax.
Suggested-by: Zsolt Parragi <[email protected]>
Suggested-by: VASUKI M <[email protected]>
Investigated-by: Zsolt Parragi <[email protected]>
---
src/include/libpq/hba.h | 2 +
src/include/libpq/oauth.h | 15 +-
src/backend/libpq/auth-oauth.c | 225 ++++++++++++++++++
src/backend/libpq/hba.c | 25 ++
.../modules/oauth_validator/t/001_server.pl | 97 ++++++++
src/test/modules/oauth_validator/validator.c | 48 +++-
6 files changed, 407 insertions(+), 5 deletions(-)
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index c4570ce9b3f..e8898561c8c 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -140,6 +140,8 @@ typedef struct HbaLine
char *oauth_scope;
char *oauth_validator;
bool oauth_skip_usermap;
+ List *oauth_opt_keys;
+ List *oauth_opt_vals;
} HbaLine;
typedef struct IdentLine
diff --git a/src/include/libpq/oauth.h b/src/include/libpq/oauth.h
index 60f493acddd..86f463a284e 100644
--- a/src/include/libpq/oauth.h
+++ b/src/include/libpq/oauth.h
@@ -96,6 +96,17 @@ typedef struct OAuthValidatorCallbacks
ValidatorValidateCB validate_cb;
} OAuthValidatorCallbacks;
+/*
+ * A validator can register a list of custom option names during its startup_cb,
+ * then later retrieve the user settings for each during validation. This
+ * enables per-HBA-line configuration. For more information, refer to the OAuth
+ * validator modules documentation.
+ */
+extern void RegisterOAuthHBAOptions(ValidatorModuleState *state, int num,
+ const char *opts[]);
+extern const char *GetOAuthHBAOption(const ValidatorModuleState *state,
+ const char *optname);
+
/*
* Type of the shared library symbol _PG_oauth_validator_module_init which is
* required for all validator modules. This function will be invoked during
@@ -107,9 +118,7 @@ extern PGDLLEXPORT const OAuthValidatorCallbacks *_PG_oauth_validator_module_ini
/* Implementation */
extern PGDLLIMPORT const pg_be_sasl_mech pg_be_oauth_mech;
-/*
- * Ensure a validator named in the HBA is permitted by the configuration.
- */
extern bool check_oauth_validator(HbaLine *hbaline, int elevel, char **err_msg);
+extern bool valid_oauth_hba_option_name(const char *name);
#endif /* PG_OAUTH_H */
diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c
index 6d4be70aada..032b49e1743 100644
--- a/src/backend/libpq/auth-oauth.c
+++ b/src/backend/libpq/auth-oauth.c
@@ -25,6 +25,7 @@
#include "libpq/hba.h"
#include "libpq/oauth.h"
#include "libpq/sasl.h"
+#include "miscadmin.h"
#include "storage/fd.h"
#include "storage/ipc.h"
#include "utils/json.h"
@@ -40,10 +41,15 @@ static int oauth_exchange(void *opaq, const char *input, int inputlen,
static void load_validator_library(const char *libname);
static void shutdown_validator_library(void *arg);
+static bool check_validator_hba_options(Port *port, const char **logdetail);
static ValidatorModuleState *validator_module_state;
static const OAuthValidatorCallbacks *ValidatorCallbacks;
+static MemoryContext ValidatorMemoryContext;
+static List *ValidatorOptions;
+static bool ValidatorOptionsChecked;
+
/* Mechanism declaration */
const pg_be_sasl_mech pg_be_oauth_mech = {
.get_mechanisms = oauth_get_mechanisms,
@@ -108,6 +114,9 @@ oauth_init(Port *port, const char *selected_mech, const char *shadow_pass)
errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("client selected an invalid SASL authentication mechanism"));
+ /* Save our memory context for later use by client API calls. */
+ ValidatorMemoryContext = CurrentMemoryContext;
+
ctx = palloc0_object(struct oauth_ctx);
ctx->state = OAUTH_STATE_INIT;
@@ -279,6 +288,16 @@ oauth_exchange(void *opaq, const char *input, int inputlen,
errmsg("malformed OAUTHBEARER message"),
errdetail("Message contains additional data after the final terminator."));
+ /*
+ * Make sure all custom HBA options are understood by the validator before
+ * continuing, since we couldn't check them during server start/reload.
+ */
+ if (!check_validator_hba_options(ctx->port, logdetail))
+ {
+ ctx->state = OAUTH_STATE_FINISHED;
+ return PG_SASL_EXCHANGE_FAILURE;
+ }
+
if (!validate(ctx->port, auth, logdetail))
{
generate_error_response(ctx, output, outputlen);
@@ -807,6 +826,9 @@ shutdown_validator_library(void *arg)
{
if (ValidatorCallbacks->shutdown_cb != NULL)
ValidatorCallbacks->shutdown_cb(validator_module_state);
+
+ /* The backing memory for this is about to disappear. */
+ ValidatorOptions = NIL;
}
/*
@@ -892,3 +914,206 @@ done:
return (*err_msg == NULL);
}
+
+/*
+ * Client APIs for validator implementations
+ *
+ * Since we're currently not threaded, we only allow one validator in the
+ * process at a time. So we can make use of globals for now instead of looking
+ * up information using the state pointer. We probably shouldn't assume that the
+ * module hasn't temporarily changed memory contexts on us, though; functions
+ * here should defensively use an appropriate context when making global
+ * allocations.
+ */
+
+/*
+ * Adds to the list of allowed validator.* HBA options. Used during the
+ * startup_cb.
+ */
+void
+RegisterOAuthHBAOptions(ValidatorModuleState *state, int num,
+ const char *opts[])
+{
+ MemoryContext oldcontext;
+
+ if (!state)
+ {
+ Assert(false);
+ return;
+ }
+
+ oldcontext = MemoryContextSwitchTo(ValidatorMemoryContext);
+
+ for (int i = 0; i < num; i++)
+ {
+ if (!valid_oauth_hba_option_name(opts[i]))
+ {
+ /*
+ * The user can't set this option in the HBA, so GetOAuthHBAOption
+ * would always return NULL.
+ */
+ ereport(WARNING,
+ errmsg("HBA option name \"%s\" is invalid and will be ignored",
+ opts[i]),
+ /* translator: the second %s is a function name */
+ errcontext("validator module \"%s\", in call to %s",
+ MyProcPort->hba->oauth_validator,
+ "RegisterOAuthHBAOptions"));
+ continue;
+ }
+
+ ValidatorOptions = lappend(ValidatorOptions, pstrdup(opts[i]));
+ }
+
+ MemoryContextSwitchTo(oldcontext);
+
+ /*
+ * Wait to validate the HBA against the registered options until later
+ * (see check_validator_hba_options()).
+ *
+ * Delaying allows the validator to make multiple registration calls, to
+ * append to the list; it lets us make the check in a place where we can
+ * report the error without leaking details to the client; and it avoids
+ * exporting the order of operations between HBA matching and the
+ * startup_cb call as an API guarantee. (The last issue may become
+ * relevant with a threaded model.)
+ */
+}
+
+/*
+ * Restrict the names available to custom HBA options, so that we don't
+ * accidentally prevent future syntax extensions to HBA files.
+ */
+bool
+valid_oauth_hba_option_name(const char *name)
+{
+ /*
+ * This list is not incredibly principled, since the goal is just to bound
+ * compatibility guarantees for our HBA parser. Alphanumerics seem
+ * obviously fine, and it's difficult to argue against the punctuation
+ * that's already included in some HBA option names and identifiers.
+ */
+ static const char *name_allowed_set =
+ "abcdefghijklmnopqrstuvwxyz"
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+ "0123456789_-";
+
+ size_t span;
+
+ if (!name[0])
+ return false;
+
+ span = strspn(name, name_allowed_set);
+ return name[span] == '\0';
+}
+
+/*
+ * Verifies that all validator.* HBA options specified by the user were actually
+ * registered by the validator library in use.
+ */
+static bool
+check_validator_hba_options(Port *port, const char **logdetail)
+{
+ HbaLine *hba = port->hba;
+
+ foreach_ptr(char, key, hba->oauth_opt_keys)
+ {
+ bool found = false;
+
+ /* O(n^2) shouldn't be a problem here in practice. */
+ foreach_ptr(char, optname, ValidatorOptions)
+ {
+ if (strcmp(key, optname) == 0)
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ /*
+ * Bad option name. Mirror the error messages in hba.c here,
+ * keeping in mind that the original "validator." prefix was
+ * stripped from the key during parsing.
+ *
+ * Since this is affecting live connections, which is unusual for
+ * HBA, be noisy with a WARNING. (Warnings aren't sent to clients
+ * prior to successful authentication, so this won't disclose the
+ * server config.) It'll duplicate some of the information in the
+ * logdetail, but that should make it hard to miss the connection
+ * between the two.
+ */
+ char *name = psprintf("validator.%s", key);
+
+ *logdetail = psprintf(_("unrecognized authentication option name: \"%s\""),
+ name);
+ ereport(WARNING,
+ errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("unrecognized authentication option name: \"%s\"",
+ name),
+ /* translator: the first %s is the name of the module */
+ errdetail("The installed validator module (\"%s\") did not define an option named \"%s\".",
+ hba->oauth_validator, key),
+ errhint("All OAuth connections matching this line will fail. Correct the option and reload the server configuration."),
+ errcontext("line %d of configuration file \"%s\"",
+ hba->linenumber, hba->sourcefile));
+
+ return false;
+ }
+ }
+
+ ValidatorOptionsChecked = true; /* unfetter GetOAuthHBAOption() */
+ return true;
+}
+
+/*
+ * Retrieves the setting for a validator.* HBA option, or NULL if not found.
+ * This may only be used during the validate_cb and shutdown_cb.
+ */
+const char *
+GetOAuthHBAOption(const ValidatorModuleState *state, const char *optname)
+{
+ HbaLine *hba = MyProcPort->hba;
+ ListCell *lc_k;
+ ListCell *lc_v;
+ const char *ret = NULL;
+
+ if (!ValidatorOptionsChecked)
+ {
+ /*
+ * Prevent the startup_cb from retrieving HBA options that it has just
+ * registered. This probably seems strange -- why refuse to hand out
+ * information we already know? -- but this lets us reserve the
+ * ability to perform the startup_cb call earlier, before we know
+ * which HBA line is matched by a connection, without breaking this
+ * API.
+ */
+ return NULL;
+ }
+
+ if (!state || !hba)
+ {
+ Assert(false);
+ return NULL;
+ }
+
+ Assert(list_length(hba->oauth_opt_keys) == list_length(hba->oauth_opt_vals));
+
+ forboth(lc_k, hba->oauth_opt_keys, lc_v, hba->oauth_opt_vals)
+ {
+ const char *key = lfirst(lc_k);
+ const char *val = lfirst(lc_v);
+
+ if (strcmp(key, optname) == 0)
+ {
+ /*
+ * Don't return yet -- when regular HBA options are specified more
+ * than once, the last one wins. Do the same for these options.
+ */
+ ret = val;
+ }
+ }
+
+ return ret;
+}
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 87ee541e880..0569e8fced8 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -2497,6 +2497,31 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
REQUIRE_AUTH_OPTION(uaOAuth, "validator", "oauth");
hbaline->oauth_validator = pstrdup(val);
}
+ else if (strncmp(name, "validator.", strlen("validator.")) == 0)
+ {
+ const char *key = name + strlen("validator.");
+
+ /*
+ * Validator modules may register their own per-HBA-line options.
+ * Unfortunately, since we don't want to require these modules to be
+ * loaded into the postmaster, we don't know if the options are valid
+ * yet and must store them for later. Perform only a basic syntax
+ * check here.
+ */
+ if (!valid_oauth_hba_option_name(key))
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid OAuth validator option name: \"%s\"", name),
+ errcontext("line %d of configuration file \"%s\"",
+ line_num, file_name)));
+ return false;
+ }
+
+ REQUIRE_AUTH_OPTION(uaOAuth, name, "oauth");
+ hbaline->oauth_opt_keys = lappend(hbaline->oauth_opt_keys, pstrdup(key));
+ hbaline->oauth_opt_vals = lappend(hbaline->oauth_opt_vals, pstrdup(val));
+ }
else if (strcmp(name, "delegate_ident_mapping") == 0)
{
REQUIRE_AUTH_OPTION(uaOAuth, "delegate_ident_mapping", "oauth");
diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl
index 28d123b833e..0756e6038c4 100644
--- a/src/test/modules/oauth_validator/t/001_server.pl
+++ b/src/test/modules/oauth_validator/t/001_server.pl
@@ -579,10 +579,29 @@ $node->connect_fails(
$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.error_detail");
$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.internal_error");
+
+# We complain when bad option names are registered, but connections may proceed
+# (since users can't set those options in the HBA anyway).
+$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.authn_id");
+$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.authorize_tokens");
+$bgconn->query_safe("ALTER SYSTEM SET oauth_validator.invalid_hba TO true");
+
$node->reload;
$log_start =
$node->wait_for_log(qr/reloading configuration files/, $log_start);
+$node->connect_ok(
+ "$common_connstr user=test",
+ "bad registered HBA option",
+ expected_stderr =>
+ qr@Visit https://example\.com/ and enter the code: postgresuser@,
+ log_like => [
+ qr/WARNING:\s+HBA option name "bad option name" is invalid and will be ignored/,
+ qr/CONTEXT:\s+validator module "validator", in call to RegisterOAuthHBAOptions/,
+ ]);
+
+$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.invalid_hba");
+
#
# Test user mapping.
#
@@ -651,6 +670,84 @@ $node->reload;
$log_start =
$node->wait_for_log(qr/reloading configuration files/, $log_start);
+$bgconn->quit; # the tests below restart the server
+
+#
+# Test validator-specific HBA options.
+#
+
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf(
+ 'pg_hba.conf', qq{
+local all test oauth issuer="$issuer" scope="openid postgres" delegate_ident_mapping=1 \\
+ validator.authn_id="ignored" validator.authn_id="other-identity"
+local all testalt oauth issuer="$issuer" scope="openid postgres" validator.log="testalt message"
+});
+
+$node->reload;
+$log_start =
+ $node->wait_for_log(qr/reloading configuration files/, $log_start);
+
+$node->connect_ok(
+ "$common_connstr user=test",
+ "custom HBA setting (test)",
+ expected_stderr =>
+ qr@Visit https://example\.com/ and enter the code: postgresuser@,
+ log_like => [qr/connection authenticated: identity="other-identity"/]);
+$node->connect_ok(
+ "$common_connstr user=testalt",
+ "custom HBA setting (testalt)",
+ expected_stderr =>
+ qr@Visit https://example\.com/ and enter the code: postgresuser@,
+ log_like => [
+ qr/LOG:\s+testalt message/,
+ qr/connection authenticated: identity="testalt"/,
+ ]);
+
+# bad syntax
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf(
+ 'pg_hba.conf', qq{
+local all testalt oauth issuer="$issuer" scope="openid postgres" validator.=1
+});
+
+$log_start = -s $node->logfile;
+$node->restart(fail_ok => 1);
+$node->log_check("empty HBA option name",
+ $log_start,
+ log_like => [qr/invalid OAuth validator option name: "validator\."/]);
+
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf(
+ 'pg_hba.conf', qq{
+local all testalt oauth issuer="$issuer" scope="openid postgres" validator.@@=1
+});
+
+$log_start = -s $node->logfile;
+$node->restart(fail_ok => 1);
+$node->log_check("invalid HBA option name",
+ $log_start,
+ log_like => [qr/invalid OAuth validator option name: "validator\.@@"/]);
+
+# unknown settings (validation is deferred to connect time)
+unlink($node->data_dir . '/pg_hba.conf');
+$node->append_conf(
+ 'pg_hba.conf', qq{
+local all testalt oauth issuer="$issuer" scope="openid postgres" \\
+ validator.log=ignored validator.bad=1
+});
+$node->restart;
+
+$node->connect_fails(
+ "$common_connstr user=testalt",
+ "bad HBA setting",
+ expected_stderr => qr/OAuth bearer authentication failed/,
+ log_like => [
+ qr/WARNING:\s+unrecognized authentication option name: "validator\.bad"/,
+ qr/FATAL:\s+OAuth bearer authentication failed/,
+ qr/DETAIL:\s+unrecognized authentication option name: "validator\.bad"/,
+ ]);
+
#
# Test multiple validators.
#
diff --git a/src/test/modules/oauth_validator/validator.c b/src/test/modules/oauth_validator/validator.c
index 353e0e0d32a..85fb4c08bf2 100644
--- a/src/test/modules/oauth_validator/validator.c
+++ b/src/test/modules/oauth_validator/validator.c
@@ -42,13 +42,21 @@ static char *authn_id = NULL;
static bool authorize_tokens = true;
static char *error_detail = NULL;
static bool internal_error = false;
+static bool invalid_hba = false;
+
+/* HBA options */
+static const char *hba_opts[] = {
+ "authn_id", /* overrides the default authn_id */
+ "log", /* logs an arbitrary string */
+};
/*---
* Extension entry point. Sets up GUCs for use by tests:
*
* - oauth_validator.authn_id Sets the user identifier to return during token
* validation. Defaults to the username in the
- * startup packet.
+ * startup packet, or the validator.authn_id HBA
+ * option if it is set.
*
* - oauth_validator.authorize_tokens
* Sets whether to successfully validate incoming
@@ -96,6 +104,14 @@ _PG_init(void)
PGC_SIGHUP,
0,
NULL, NULL, NULL);
+ DefineCustomBoolVariable("oauth_validator.invalid_hba",
+ "Should the validator register an invalid option?",
+ NULL,
+ &invalid_hba,
+ false,
+ PGC_SIGHUP,
+ 0,
+ NULL, NULL, NULL);
MarkGUCPrefixReserved("oauth_validator");
}
@@ -124,6 +140,29 @@ validator_startup(ValidatorModuleState *state)
if (state->sversion != PG_VERSION_NUM)
elog(ERROR, "oauth_validator: sversion set to %d", state->sversion);
+ /*
+ * Test the behavior of custom HBA options. Registered options should not
+ * be retrievable during startup (we want to discourage modules from
+ * relying on the relative order of client connections and the
+ * startup_cb).
+ */
+ RegisterOAuthHBAOptions(state, lengthof(hba_opts), hba_opts);
+ for (int i = 0; i < lengthof(hba_opts); i++)
+ {
+ if (GetOAuthHBAOption(state, hba_opts[i]))
+ elog(ERROR,
+ "oauth_validator: GetOAuthValidatorOption(\"%s\") was non-NULL during startup_cb",
+ hba_opts[i]);
+ }
+
+ if (invalid_hba)
+ {
+ /* Register a bad option, which should print a WARNING to the logs. */
+ const char *invalid = "bad option name";
+
+ RegisterOAuthHBAOptions(state, 1, &invalid);
+ }
+
state->private_data = PRIVATE_COOKIE;
}
@@ -141,7 +180,7 @@ validator_shutdown(ValidatorModuleState *state)
/*
* Validator implementation. Logs the incoming data and authorizes the token by
- * default; the behavior can be modified via the module's GUC settings.
+ * default; the behavior can be modified via the module's GUC and HBA settings.
*/
static bool
validate_token(const ValidatorModuleState *state,
@@ -153,6 +192,9 @@ validate_token(const ValidatorModuleState *state,
elog(ERROR, "oauth_validator: private state cookie changed to %p in validate",
state->private_data);
+ if (GetOAuthHBAOption(state, "log"))
+ elog(LOG, "%s", GetOAuthHBAOption(state, "log"));
+
elog(LOG, "oauth_validator: token=\"%s\", role=\"%s\"", token, role);
elog(LOG, "oauth_validator: issuer=\"%s\", scope=\"%s\"",
MyProcPort->hba->oauth_issuer,
@@ -165,6 +207,8 @@ validate_token(const ValidatorModuleState *state,
res->authorized = authorize_tokens;
if (authn_id)
res->authn_id = pstrdup(authn_id);
+ else if (GetOAuthHBAOption(state, "authn_id"))
+ res->authn_id = pstrdup(GetOAuthHBAOption(state, "authn_id"));
else
res->authn_id = pstrdup(role);
--
2.34.1
view thread (25+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Custom oauth validator options
In-Reply-To: <CAOYmi+nTXGcroZD_Mnkc8LYWYFbfDYNR4ML_yQ5sF9+DY2amcg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox