public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zsolt Parragi <[email protected]>
To: Jacob Champion <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: Jonathan Gonzalez V. <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Make PGOAUTHCAFILE in libpq-oauth work out of debug mode
Date: Fri, 12 Dec 2025 11:05:15 +0000
Message-ID: <CAN4CZFNvZ9+pQ=OA4m=HcDgip84GHnekh4gUhYWfK3Q4+rBMxA@mail.gmail.com> (raw)
In-Reply-To: <CAOYmi+=HcXJub1rDsQ7vpKMHuBB6NTA2Z5T=zAkaFdRThf+9zg@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CAOYmi+=fbZNJSkHVci=GpR8XPYObK=H+2ERRha0LDTS+ifsWnw@mail.gmail.com>
<CAN4CZFPhm2NCRWzZpX=kRLqyxu4Ps-d0xE5W75a-iDoKrLbXBw@mail.gmail.com>
<CAOYmi+=HcXJub1rDsQ7vpKMHuBB6NTA2Z5T=zAkaFdRThf+9zg@mail.gmail.com>
Hello
I implemented a simple patch based on the above suggestion
(PGOAUTHDEBUG=UNSAFE:http...). I did not update the documentation yet,
let's see what everyone thinks about it before that, and I also have
some concerns/questions.
I added the new functions into a common source file which gets
included in both the oauth module and libpq. I'm not entirely happy
about this, but I didn't see a better way without duplicating the
code.
My concern, which is also there with the current version: is an
environment variable the best way to control these settings in a
library included into many applications? Wouldn't it be better to make
these settings in libpq (or the oauth module), and only add the
environment variables to psql?
This can be used to inject a CA into an application without the user
noticing it, or without the application developer being aware of the
possibility. With the current single-value variable, it is already
possible, and in an application without a visible standard output, it
is already hidden. But by splitting the setting into multiple flags,
this can go unnoticed even in a console application.
Another question is what to do with the CA file - currently it remains
a separate (environment) variable, but maybe it could be included in
the option string instead:
PGOAUTHDEBUG=UNSAFE:custom-ca=/path/to/the/file
What do you think about it?
Attachments:
[application/octet-stream] 0001-Split-PGOAUTHDEBUG-UNSAFE-into-multiple-options.patch (13.5K, 2-0001-Split-PGOAUTHDEBUG-UNSAFE-into-multiple-options.patch)
download | inline diff:
From 1632eb23a16954a53b325e3336cdddba19db5439 Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <[email protected]>
Date: Thu, 11 Dec 2025 23:56:08 +0000
Subject: [PATCH] Split PGOAUTHDEBUG=UNSAFE into multiple options
---
src/interfaces/libpq-oauth/Makefile | 12 +-
src/interfaces/libpq-oauth/meson.build | 6 +-
src/interfaces/libpq-oauth/oauth-curl.c | 18 +--
src/interfaces/libpq-oauth/oauth-utils.c | 11 --
src/interfaces/libpq-oauth/oauth-utils.h | 2 +-
src/interfaces/libpq/Makefile | 3 +-
src/interfaces/libpq/fe-auth-oauth-debug.c | 142 +++++++++++++++++++++
src/interfaces/libpq/fe-auth-oauth.c | 16 +--
src/interfaces/libpq/fe-auth-oauth.h | 19 ++-
src/interfaces/libpq/meson.build | 1 +
10 files changed, 192 insertions(+), 38 deletions(-)
create mode 100644 src/interfaces/libpq/fe-auth-oauth-debug.c
diff --git a/src/interfaces/libpq-oauth/Makefile b/src/interfaces/libpq-oauth/Makefile
index 51145f085a8..1ae7b00e816 100644
--- a/src/interfaces/libpq-oauth/Makefile
+++ b/src/interfaces/libpq-oauth/Makefile
@@ -30,15 +30,25 @@ override CFLAGS += $(PTHREAD_CFLAGS)
OBJS = \
$(WIN32RES)
-OBJS_STATIC = oauth-curl.o
+OBJS_STATIC = \
+ oauth-curl.o \
+ fe-auth-oauth-debug.o
# The shared library needs additional glue symbols.
OBJS_SHLIB = \
oauth-curl_shlib.o \
oauth-utils.o \
+ fe-auth-oauth-debug_shlib.o
oauth-utils.o: override CPPFLAGS += -DUSE_DYNAMIC_OAUTH
oauth-curl_shlib.o: override CPPFLAGS_SHLIB += -DUSE_DYNAMIC_OAUTH
+fe-auth-oauth-debug_shlib.o: override CPPFLAGS_SHLIB += -DUSE_DYNAMIC_OAUTH
+
+fe-auth-oauth-debug.o: $(libpq_srcdir)/fe-auth-oauth-debug.c
+ $(CC) $(CFLAGS) $(CPPFLAGS) -c $< -o $@
+
+fe-auth-oauth-debug_shlib.o: $(libpq_srcdir)/fe-auth-oauth-debug.c
+ $(CC) $(CFLAGS) $(CFLAGS_SL) $(CPPFLAGS) $(CPPFLAGS_SHLIB) -c $< -o $@
# Add shlib-/stlib-specific objects.
$(shlib): override OBJS += $(OBJS_SHLIB)
diff --git a/src/interfaces/libpq-oauth/meson.build b/src/interfaces/libpq-oauth/meson.build
index 505e1671b86..5c916b25e81 100644
--- a/src/interfaces/libpq-oauth/meson.build
+++ b/src/interfaces/libpq-oauth/meson.build
@@ -6,6 +6,7 @@ endif
libpq_oauth_sources = files(
'oauth-curl.c',
+ '../libpq/fe-auth-oauth-debug.c',
)
# The shared library needs additional glue symbols.
@@ -50,7 +51,10 @@ libpq_oauth_so = shared_module(libpq_oauth_name,
libpq_oauth_test_deps = []
-oauth_test_sources = files('test-oauth-curl.c') + libpq_oauth_so_sources
+oauth_test_sources = files(
+ 'test-oauth-curl.c',
+ '../libpq/fe-auth-oauth-debug.c',
+) + libpq_oauth_so_sources
if host_system == 'windows'
oauth_test_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
diff --git a/src/interfaces/libpq-oauth/oauth-curl.c b/src/interfaces/libpq-oauth/oauth-curl.c
index bd0a656a166..185dbb64bab 100644
--- a/src/interfaces/libpq-oauth/oauth-curl.c
+++ b/src/interfaces/libpq-oauth/oauth-curl.c
@@ -277,7 +277,7 @@ struct async_ctx
int running; /* is asynchronous work in progress? */
bool user_prompted; /* have we already sent the authz prompt? */
bool used_basic_auth; /* did we send a client secret? */
- bool debugging; /* can we give unsafe developer assistance? */
+ oauth_debug_flags debug_flags; /* can we give developer assistance */
int dbg_num_calls; /* (debug mode) how many times were we called? */
};
@@ -978,7 +978,7 @@ parse_interval(struct async_ctx *actx, const char *interval_str)
parsed = ceil(parsed);
if (parsed < 1)
- return actx->debugging ? 0 : 1;
+ return actx->debug_flags.fast_retry ? 0 : 1;
else if (parsed >= INT_MAX)
return INT_MAX;
@@ -1753,7 +1753,7 @@ setup_curl_handles(struct async_ctx *actx)
*/
CHECK_SETOPT(actx, CURLOPT_NOSIGNAL, 1L, return false);
- if (actx->debugging)
+ if (actx->debug_flags.trace)
{
/*
* Set a callback for retrieving error information from libcurl, the
@@ -1785,7 +1785,7 @@ setup_curl_handles(struct async_ctx *actx)
const long unsafe = CURLPROTO_HTTPS | CURLPROTO_HTTP;
#endif
- if (actx->debugging)
+ if (actx->debug_flags.http)
protos = unsafe;
CHECK_SETOPT(actx, popt, protos, return false);
@@ -1799,7 +1799,7 @@ setup_curl_handles(struct async_ctx *actx)
* the flow to work at all, so any changes to the roots are likely to be
* done system-wide.
*/
- if (actx->debugging)
+ if (actx->debug_flags.custom_ca)
{
const char *env;
@@ -2265,7 +2265,7 @@ check_for_device_flow(struct async_ctx *actx)
* decent time to bail out if we're not using HTTPS for the endpoints
* we'll use for the flow.
*/
- if (!actx->debugging)
+ if (!actx->debug_flags.http)
{
if (pg_strncasecmp(provider->device_authorization_endpoint,
HTTPS_SCHEME, strlen(HTTPS_SCHEME)) != 0)
@@ -2787,8 +2787,8 @@ pg_fe_run_oauth_flow_impl(PGconn *conn)
actx->mux = PGINVALID_SOCKET;
actx->timerfd = -1;
- /* Should we enable unsafe features? */
- actx->debugging = oauth_unsafe_debugging_enabled();
+ /* Parse debug flags from environment */
+ actx->debug_flags = oauth_get_debug_flags();
state->async_ctx = actx;
@@ -3068,7 +3068,7 @@ pg_fe_run_oauth_flow(PGconn *conn)
actx = state->async_ctx;
Assert(actx || result == PGRES_POLLING_FAILED);
- if (actx && actx->debugging)
+ if (actx && actx->debug_flags.poll_counts)
{
actx->dbg_num_calls++;
if (result == PGRES_POLLING_OK || result == PGRES_POLLING_FAILED)
diff --git a/src/interfaces/libpq-oauth/oauth-utils.c b/src/interfaces/libpq-oauth/oauth-utils.c
index 45fdc7579f2..719208fead3 100644
--- a/src/interfaces/libpq-oauth/oauth-utils.c
+++ b/src/interfaces/libpq-oauth/oauth-utils.c
@@ -142,17 +142,6 @@ libpq_gettext(const char *msgid)
#endif /* ENABLE_NLS */
-/*
- * Returns true if the PGOAUTHDEBUG=UNSAFE flag is set in the environment.
- */
-bool
-oauth_unsafe_debugging_enabled(void)
-{
- const char *env = getenv("PGOAUTHDEBUG");
-
- return (env && strcmp(env, "UNSAFE") == 0);
-}
-
/*
* Duplicate SOCK_ERRNO* definitions from libpq-int.h, for use by
* pq_block/reset_sigpipe().
diff --git a/src/interfaces/libpq-oauth/oauth-utils.h b/src/interfaces/libpq-oauth/oauth-utils.h
index f4ffefef208..1762db7d3be 100644
--- a/src/interfaces/libpq-oauth/oauth-utils.h
+++ b/src/interfaces/libpq-oauth/oauth-utils.h
@@ -76,7 +76,7 @@ typedef enum
} PGTernaryBool;
extern void libpq_append_conn_error(PGconn *conn, const char *fmt,...) pg_attribute_printf(2, 3);
-extern bool oauth_unsafe_debugging_enabled(void);
+extern oauth_debug_flags oauth_get_debug_flags(void);
extern int pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending);
extern void pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending, bool got_epipe);
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 9fe321147fc..157f80cfb5b 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -44,7 +44,8 @@ OBJS = \
legacy-pqsignal.o \
libpq-events.o \
pqexpbuffer.o \
- fe-auth.o
+ fe-auth.o \
+ fe-auth-oauth-debug.o
# File shared across all SSL implementations supported.
ifneq ($(with_ssl),no)
diff --git a/src/interfaces/libpq/fe-auth-oauth-debug.c b/src/interfaces/libpq/fe-auth-oauth-debug.c
new file mode 100644
index 00000000000..56bc05e6820
--- /dev/null
+++ b/src/interfaces/libpq/fe-auth-oauth-debug.c
@@ -0,0 +1,142 @@
+/*-------------------------------------------------------------------------
+ *
+ * fe-auth-oauth-debug.c
+ * Parsing logic for PGOAUTHDEBUG environment variable
+ *
+ * This file contains pure string parsing logic with no dependencies on
+ * libpq or libpq-oauth implementation details. It's compiled into both
+ * libraries to avoid code duplication.
+ *
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ * src/interfaces/libpq/fe-auth-oauth-debug.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres_fe.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "fe-auth-oauth.h"
+
+/*
+ * Parse a single debug option from PGOAUTHDEBUG.
+ * Updates the flag if the option is recognized and allowed.
+ * Prints warnings for unrecognized options or unsafe options without permission.
+ */
+static void
+parse_debug_option(const char *option, oauth_debug_flags *flags, bool allow_unsafe)
+{
+ bool *flag_ptr = NULL;
+ bool is_unsafe = false;
+
+ if (strcmp(option, "http") == 0)
+ {
+ flag_ptr = &flags->http;
+ is_unsafe = true;
+ }
+ else if (strcmp(option, "trace") == 0)
+ {
+ flag_ptr = &flags->trace;
+ is_unsafe = true;
+ }
+ else if (strcmp(option, "custom-ca") == 0)
+ {
+ flag_ptr = &flags->custom_ca;
+ is_unsafe = true;
+ }
+ else if (strcmp(option, "fast-retry") == 0)
+ {
+ flag_ptr = &flags->fast_retry;
+ }
+ else if (strcmp(option, "poll-counts") == 0)
+ {
+ flag_ptr = &flags->poll_counts;
+ }
+ else if (strcmp(option, "print-plugin-errors") == 0)
+ {
+ flag_ptr = &flags->print_plugin_errors;
+ }
+
+ if (!flag_ptr)
+ {
+ fprintf(stderr,
+ "WARNING: PGOAUTHDEBUG: unrecognized debug option \"%s\" (ignored)\n",
+ option);
+ return;
+ }
+
+ if (is_unsafe && !allow_unsafe)
+ {
+ fprintf(stderr,
+ "WARNING: PGOAUTHDEBUG: unsafe option \"%s\" requires UNSAFE: prefix (ignored)\n"
+ "Use: PGOAUTHDEBUG=UNSAFE:%s\n",
+ option, option);
+ return;
+ }
+
+ *flag_ptr = true;
+}
+
+/*
+ * Parses the PGOAUTHDEBUG environment variable and returns debug flags.
+ *
+ * Supported formats:
+ * PGOAUTHDEBUG=UNSAFE - legacy format, enables all features
+ * PGOAUTHDEBUG=option1,option2 - enable safe features only
+ * PGOAUTHDEBUG=UNSAFE:opt1,opt2 - enable unsafe and/or safe features
+ *
+ * Prints a warning and skips the invalid option if:
+ * - An unrecognized option is specified
+ * - An unsafe option is specified without the UNSAFE: prefix
+ */
+oauth_debug_flags
+oauth_get_debug_flags(void)
+{
+ oauth_debug_flags flags = {0};
+ const char *env = getenv("PGOAUTHDEBUG");
+ char *options_str;
+ char *option;
+ char *saveptr = NULL;
+ bool unsafe_prefix = false;
+
+ if (!env || env[0] == '\0')
+ return flags;
+
+ if (strcmp(env, "UNSAFE") == 0)
+ {
+ flags.http = true;
+ flags.trace = true;
+ flags.custom_ca = true;
+ flags.fast_retry = true;
+ flags.poll_counts = true;
+ flags.print_plugin_errors = true;
+ return flags;
+ }
+
+ if (strncmp(env, "UNSAFE:", 7) == 0)
+ {
+ unsafe_prefix = true;
+ env += 7;
+ }
+
+ options_str = strdup(env);
+ if (!options_str)
+ return flags;
+
+ option = strtok_r(options_str, ",", &saveptr);
+ while (option != NULL)
+ {
+ parse_debug_option(option, &flags, unsafe_prefix);
+ option = strtok_r(NULL, ",", &saveptr);
+ }
+
+ free(options_str);
+
+ return flags;
+}
diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c
index d146c5f567c..96878c59369 100644
--- a/src/interfaces/libpq/fe-auth-oauth.c
+++ b/src/interfaces/libpq/fe-auth-oauth.c
@@ -376,7 +376,7 @@ issuer_from_well_known_uri(PGconn *conn, const char *wkuri)
authority_start = wkuri + strlen(HTTPS_SCHEME);
if (!authority_start
- && oauth_unsafe_debugging_enabled()
+ && oauth_get_debug_flags().http
&& pg_strncasecmp(wkuri, HTTP_SCHEME, strlen(HTTP_SCHEME)) == 0)
{
/* Allow http:// for testing only. */
@@ -870,7 +870,7 @@ use_builtin_flow(PGconn *conn, fe_oauth_state *state)
*
* Note that POSIX dlerror() isn't guaranteed to be threadsafe.
*/
- if (oauth_unsafe_debugging_enabled())
+ if (oauth_get_debug_flags().print_plugin_errors)
fprintf(stderr, "failed dlopen for libpq-oauth: %s\n", dlerror());
return false;
@@ -884,7 +884,7 @@ use_builtin_flow(PGconn *conn, fe_oauth_state *state)
* This is more of an error condition than the one above, but due to
* the dlerror() threadsafety issue, lock it behind PGOAUTHDEBUG too.
*/
- if (oauth_unsafe_debugging_enabled())
+ if (oauth_get_debug_flags().print_plugin_errors)
fprintf(stderr, "failed dlsym for libpq-oauth: %s\n", dlerror());
dlclose(state->builtin_flow);
@@ -1385,13 +1385,3 @@ pqClearOAuthToken(PGconn *conn)
conn->oauth_token = NULL;
}
-/*
- * Returns true if the PGOAUTHDEBUG=UNSAFE flag is set in the environment.
- */
-bool
-oauth_unsafe_debugging_enabled(void)
-{
- const char *env = getenv("PGOAUTHDEBUG");
-
- return (env && strcmp(env, "UNSAFE") == 0);
-}
diff --git a/src/interfaces/libpq/fe-auth-oauth.h b/src/interfaces/libpq/fe-auth-oauth.h
index 0d59e91605b..1f0cbea7eeb 100644
--- a/src/interfaces/libpq/fe-auth-oauth.h
+++ b/src/interfaces/libpq/fe-auth-oauth.h
@@ -42,8 +42,25 @@ typedef struct
void *builtin_flow;
} fe_oauth_state;
+/*
+ * Debug flags for PGOAUTHDEBUG environment variable.
+ * Each flag controls a specific debug feature.
+ */
+typedef struct oauth_debug_flags
+{
+ /* potentially UNSAFE features */
+ bool http;
+ bool trace;
+ bool custom_ca;
+
+ /* SAFE features */
+ bool fast_retry;
+ bool poll_counts;
+ bool print_plugin_errors;
+} oauth_debug_flags;
+
extern void pqClearOAuthToken(PGconn *conn);
-extern bool oauth_unsafe_debugging_enabled(void);
+extern oauth_debug_flags oauth_get_debug_flags(void);
extern bool use_builtin_flow(PGconn *conn, fe_oauth_state *state);
/* Mechanisms in fe-auth-oauth.c */
diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build
index b259c998fa2..19508b39a67 100644
--- a/src/interfaces/libpq/meson.build
+++ b/src/interfaces/libpq/meson.build
@@ -2,6 +2,7 @@
libpq_sources = files(
'fe-auth-oauth.c',
+ 'fe-auth-oauth-debug.c',
'fe-auth-scram.c',
'fe-auth.c',
'fe-cancel.c',
--
2.43.0
view thread (24+ 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]
Subject: Re: Make PGOAUTHCAFILE in libpq-oauth work out of debug mode
In-Reply-To: <CAN4CZFNvZ9+pQ=OA4m=HcDgip84GHnekh4gUhYWfK3Q4+rBMxA@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