public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: Thomas Munro <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Antonin Houska <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
Date: Thu, 6 Mar 2025 16:35:43 -0800
Message-ID: <CAOYmi+=4htNLXvuTS58GL96qwBptToDop09PWFy6uzDHNz4qTw@mail.gmail.com> (raw)
In-Reply-To: <CAOYmi+n4EDOOUL27_OqYT2-F2rS6S+3mK-ppWb2Ec92UEoUbYA@mail.gmail.com>
References: <[email protected]>
<CAOYmi+=zgJinCoqdQ70fvKmvyrLekd9-supkuj57Poa1FmX8bw@mail.gmail.com>
<[email protected]>
<CAOYmi+=1_AqPL0Y7qdqg1ER0-bBjCUKUZfrLcgob5bHjzSsvbA@mail.gmail.com>
<CAOYmi+nP8AM9xm+xUW5kDz7XDF7MKBjuDnQ4LjMEm07tpwDgrg@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAOYmi+nZUivtO3Ed4X3tBh3sLmMQTe+ir7utyc1Gv5L6ZDU5cA@mail.gmail.com>
<p4bd7mn6dxr2zdak74abocyltpfdxif4pxqzixqpxpetjwt34h@qc6jgfmoddvq>
<CAOYmi+nePFefTKmeRtajfVhunbXFq64etBr7VwbhxzDBQL-BBw@mail.gmail.com>
<uqzksikuuprgcu4mrpqdu65s7loh52pghdaq5rkzdruhgy4wse@4ojpvnsfumpc>
<CAOYmi+=Pyf9EuR7dRn46ymV-P9fhUft3qH8mqdS-m9ZksmooEg@mail.gmail.com>
<CAOYmi+mhqahb65y1zXtv60T9=mDYTaepV9b-wq-GHey00zuGOg@mail.gmail.com>
<CA+hUKGJ+WyJ26QGvO_nkgvbxgw+03U4EQ4Hxw+QBft6Np+XW7w@mail.gmail.com>
<CAOYmi+mf+hw7x+E3XkRypEGy=yR3n6zowjjXeXaSK20WxX3UGw@mail.gmail.com>
<CAOYmi+kr-Eh7utEDZBFq8Mc9byLLRF3ZNc0TaHt78SKwB4T5KA@mail.gmail.com>
<CA+hUKGL3yYmKmNHw1wnpuhCbqaiDw1DDUhghEDrh6y97Ygzw9Q@mail.gmail.com>
<CAOYmi+kOyST1DoF1WOMQvf75L56LiLHZR2+3qTJ+QCg2KC8raQ@mail.gmail.com>
<CA+hUKGL59uY=UpCHEmTL6dpzuWJBVf1Kc4bQADYDEWG9ZDCffA@mail.gmail.com>
<CAOYmi+k6HUMK4XQAfnxsmgs1oPOKnchyj2O2a+R7H9jOTU4LPQ@mail.gmail.com>
<CA+hUKG+mUfH81RuYm5fJrOxdm85hvxLEm=KxXm+n-12FJJH9jA@mail.gmail.com>
<CAOYmi+nk-KHGGBHz6BGik4+xHPmyPrwOj3ezSM3F4d=-kH_n2Q@mail.gmail.com>
<CAOYmi+n4EDOOUL27_OqYT2-F2rS6S+3mK-ppWb2Ec92UEoUbYA@mail.gmail.com>
On Thu, Mar 6, 2025 at 12:57 PM Jacob Champion
<[email protected]> wrote:
> Problem 1 is a simple patch. I am working on a fix for Problem 2, but
> I got stuck trying to get a "perfect" solution working yesterday...
> Since this is a partial(?) blocker for getting NetBSD going, I'm going
> to pivot to an ugly-but-simple approach today.
Attached:
- 0001 fixes IPv6 failures,
- 0002 fixes set_timer() on Mac, and
- 0003-0005 are the existing followup patches from upthread.
Thanks!
--Jacob
Attachments:
[application/octet-stream] 0001-oauth-Use-IPv4-only-issuer-in-oauth_validator-tests.patch (3.2K, ../CAOYmi+=4htNLXvuTS58GL96qwBptToDop09PWFy6uzDHNz4qTw@mail.gmail.com/2-0001-oauth-Use-IPv4-only-issuer-in-oauth_validator-tests.patch)
download | inline diff:
From 4e1024eb47a3d19145a8db42a48d55d608ad4054 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 4 Mar 2025 09:41:19 -0800
Subject: [PATCH 1/5] oauth: Use IPv4-only issuer in oauth_validator tests
The test authorization server implemented in oauth_server.py does not
listen on IPv6. Most of the time, libcurl happily falls back to IPv4
after failing its initial connection, but on NetBSD, something is
consistently showing up on the unreserved IPv6 port and causing a test
failure.
Rather than deal with dual-stack details across all test platforms,
change the issuer to enforce the use of IPv4 only. (This elicits more
punishing timeout behavior from libcurl, so it's a useful change from
the testing perspective as well.)
Reported-by: Thomas Munro <[email protected]>
Discussion: https://postgr.es/m/CAOYmi%2Bn4EDOOUL27_OqYT2-F2rS6S%2B3mK-ppWb2Ec92UEoUbYA%40mail.gmail.com
---
src/test/modules/oauth_validator/t/001_server.pl | 2 +-
src/test/modules/oauth_validator/t/OAuth/Server.pm | 4 ++--
src/test/modules/oauth_validator/t/oauth_server.py | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl
index 6fa59fbeb25..30295364ebd 100644
--- a/src/test/modules/oauth_validator/t/001_server.pl
+++ b/src/test/modules/oauth_validator/t/001_server.pl
@@ -68,7 +68,7 @@ END
}
my $port = $webserver->port();
-my $issuer = "http://localhost:$port";
+my $issuer = "http://127.0.0.1:$port";
unlink($node->data_dir . '/pg_hba.conf');
$node->append_conf(
diff --git a/src/test/modules/oauth_validator/t/OAuth/Server.pm b/src/test/modules/oauth_validator/t/OAuth/Server.pm
index 655b2870b0b..52ae7afa991 100644
--- a/src/test/modules/oauth_validator/t/OAuth/Server.pm
+++ b/src/test/modules/oauth_validator/t/OAuth/Server.pm
@@ -15,7 +15,7 @@ OAuth::Server - runs a mock OAuth authorization server for testing
$server->run;
my $port = $server->port;
- my $issuer = "http://localhost:$port";
+ my $issuer = "http://127.0.0.1:$port";
# test against $issuer...
@@ -28,7 +28,7 @@ daemon implemented in t/oauth_server.py. (Python has a fairly usable HTTP server
in its standard library, so the implementation was ported from Perl.)
This authorization server does not use TLS (it implements a nonstandard, unsafe
-issuer at "http://localhost:<port>"), so libpq in particular will need to set
+issuer at "http://127.0.0.1:<port>"), so libpq in particular will need to set
PGOAUTHDEBUG=UNSAFE to be able to talk to it.
=cut
diff --git a/src/test/modules/oauth_validator/t/oauth_server.py b/src/test/modules/oauth_validator/t/oauth_server.py
index 4faf3323d38..5bc30be87fd 100755
--- a/src/test/modules/oauth_validator/t/oauth_server.py
+++ b/src/test/modules/oauth_validator/t/oauth_server.py
@@ -251,7 +251,7 @@ class OAuthHandler(http.server.BaseHTTPRequestHandler):
def config(self) -> JsonObject:
port = self.server.socket.getsockname()[1]
- issuer = f"http://localhost:{port}"
+ issuer = f"http://127.0.0.1:{port}"
if self._alt_issuer:
issuer += "/alternate"
elif self._parameterized:
--
2.34.1
[application/octet-stream] 0002-oauth-Fix-postcondition-for-set_timer-on-BSD.patch (3.6K, ../CAOYmi+=4htNLXvuTS58GL96qwBptToDop09PWFy6uzDHNz4qTw@mail.gmail.com/3-0002-oauth-Fix-postcondition-for-set_timer-on-BSD.patch)
download | inline diff:
From 410dce1670a04de81d533dd1b5456e363b144ca8 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Thu, 6 Mar 2025 15:02:37 -0800
Subject: [PATCH 2/5] oauth: Fix postcondition for set_timer on BSD
On macOS, readding an EVFILT_TIMER to a kqueue does not appear to clear
out previously queued timer events, so checks for timer expiration do
not work correctly during token retrieval. Switching to IPv4-only
communication exposes the problem, because libcurl is no longer clearing
out other timeouts related to Happy Eyeballs dual-stack handling.
Fully remove and re-register the kqueue timer events during each call to
set_timer(), to clear out any stale expirations.
Discussion: https://postgr.es/m/CAOYmi%2Bn4EDOOUL27_OqYT2-F2rS6S%2B3mK-ppWb2Ec92UEoUbYA%40mail.gmail.com
---
src/interfaces/libpq/fe-auth-oauth-curl.c | 48 +++++++++++++++++------
1 file changed, 35 insertions(+), 13 deletions(-)
diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c
index 6e60a81574d..2d6d4b1a123 100644
--- a/src/interfaces/libpq/fe-auth-oauth-curl.c
+++ b/src/interfaces/libpq/fe-auth-oauth-curl.c
@@ -1326,6 +1326,10 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx,
* in the set at all times and just disarm it when it's not needed. For kqueue,
* the timer is removed completely when disabled to prevent stale timeouts from
* remaining in the queue.
+ *
+ * To meet Curl requirements for the CURLMOPT_TIMERFUNCTION, implementations of
+ * set_timer must handle repeated calls by fully discarding any previous running
+ * or expired timer.
*/
static bool
set_timer(struct async_ctx *actx, long timeout)
@@ -1373,26 +1377,44 @@ set_timer(struct async_ctx *actx, long timeout)
timeout = 1;
#endif
- /* Enable/disable the timer itself. */
- EV_SET(&ev, 1, EVFILT_TIMER, timeout < 0 ? EV_DELETE : (EV_ADD | EV_ONESHOT),
- 0, timeout, 0);
+ /*
+ * Always disable the timer, and remove it from the multiplexer, to clear
+ * out any already-queued events. (On some BSDs, adding an EVFILT_TIMER to
+ * a kqueue that already has one will clear stale events, but not on
+ * macOS.)
+ *
+ * If there was no previous timer set, the kevent calls will result in
+ * ENOENT, which is fine.
+ */
+ EV_SET(&ev, 1, EVFILT_TIMER, EV_DELETE, 0, 0, 0);
if (kevent(actx->timerfd, &ev, 1, NULL, 0, NULL) < 0 && errno != ENOENT)
{
- actx_error(actx, "setting kqueue timer to %ld: %m", timeout);
+ actx_error(actx, "deleting kqueue timer: %m", timeout);
return false;
}
- /*
- * Add/remove the timer to/from the mux. (In contrast with epoll, if we
- * allowed the timer to remain registered here after being disabled, the
- * mux queue would retain any previous stale timeout notifications and
- * remain readable.)
- */
- EV_SET(&ev, actx->timerfd, EVFILT_READ, timeout < 0 ? EV_DELETE : EV_ADD,
- 0, 0, 0);
+ EV_SET(&ev, actx->timerfd, EVFILT_READ, EV_DELETE, 0, 0, 0);
if (kevent(actx->mux, &ev, 1, NULL, 0, NULL) < 0 && errno != ENOENT)
{
- actx_error(actx, "could not update timer on kqueue: %m");
+ actx_error(actx, "removing kqueue timer from multiplexer: %m");
+ return false;
+ }
+
+ /* If we're not adding a timer, we're done. */
+ if (timeout < 0)
+ return true;
+
+ EV_SET(&ev, 1, EVFILT_TIMER, (EV_ADD | EV_ONESHOT), 0, timeout, 0);
+ if (kevent(actx->timerfd, &ev, 1, NULL, 0, NULL) < 0)
+ {
+ actx_error(actx, "setting kqueue timer to %ld: %m", timeout);
+ return false;
+ }
+
+ EV_SET(&ev, actx->timerfd, EVFILT_READ, EV_ADD, 0, 0, 0);
+ if (kevent(actx->mux, &ev, 1, NULL, 0, NULL) < 0)
+ {
+ actx_error(actx, "adding kqueue timer to multiplexer: %m");
return false;
}
--
2.34.1
[application/octet-stream] 0003-oauth-Disallow-synchronous-DNS-in-libcurl.patch (4.5K, ../CAOYmi+=4htNLXvuTS58GL96qwBptToDop09PWFy6uzDHNz4qTw@mail.gmail.com/4-0003-oauth-Disallow-synchronous-DNS-in-libcurl.patch)
download | inline diff:
From c2e098c592a8f2d2c3d8f12e82b2736a630ca282 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 24 Feb 2025 15:02:01 -0800
Subject: [PATCH 3/5] oauth: Disallow synchronous DNS in libcurl
There is concern that a blocking DNS lookup in libpq could stall a
backend process (say, via FDW). Since there's currently no strong
evidence that synchronous DNS is a popular option, disallow it entirely
rather than warning at configure time. We can revisit if anyone
complains.
Per query from Andres Freund.
Discussion: https://postgr.es/m/p4bd7mn6dxr2zdak74abocyltpfdxif4pxqzixqpxpetjwt34h%40qc6jgfmoddvq
---
config/programs.m4 | 10 +++++-----
configure | 14 +++++---------
meson.build | 18 ++++++------------
3 files changed, 16 insertions(+), 26 deletions(-)
diff --git a/config/programs.m4 b/config/programs.m4
index 061b13376ac..0a07feb37cc 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -316,7 +316,7 @@ AC_DEFUN([PGAC_CHECK_LIBCURL],
[Define to 1 if curl_global_init() is guaranteed to be thread-safe.])
fi
- # Warn if a thread-friendly DNS resolver isn't built.
+ # Fail if a thread-friendly DNS resolver isn't built.
AC_CACHE_CHECK([for curl support for asynchronous DNS], [pgac_cv__libcurl_async_dns],
[AC_RUN_IFELSE([AC_LANG_PROGRAM([
#include <curl/curl.h>
@@ -332,10 +332,10 @@ AC_DEFUN([PGAC_CHECK_LIBCURL],
[pgac_cv__libcurl_async_dns=yes],
[pgac_cv__libcurl_async_dns=no],
[pgac_cv__libcurl_async_dns=unknown])])
- if test x"$pgac_cv__libcurl_async_dns" != xyes ; then
- AC_MSG_WARN([
+ if test x"$pgac_cv__libcurl_async_dns" = xno ; then
+ AC_MSG_ERROR([
*** The installed version of libcurl does not support asynchronous DNS
-*** lookups. Connection timeouts will not be honored during DNS resolution,
-*** which may lead to hangs in client programs.])
+*** lookups. Rebuild libcurl with the AsynchDNS feature enabled in order
+*** to use it with libpq.])
fi
])# PGAC_CHECK_LIBCURL
diff --git a/configure b/configure
index 93fddd69981..559f535f5cd 100755
--- a/configure
+++ b/configure
@@ -12493,7 +12493,7 @@ $as_echo "#define HAVE_THREADSAFE_CURL_GLOBAL_INIT 1" >>confdefs.h
fi
- # Warn if a thread-friendly DNS resolver isn't built.
+ # Fail if a thread-friendly DNS resolver isn't built.
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for curl support for asynchronous DNS" >&5
$as_echo_n "checking for curl support for asynchronous DNS... " >&6; }
if ${pgac_cv__libcurl_async_dns+:} false; then :
@@ -12535,15 +12535,11 @@ fi
fi
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv__libcurl_async_dns" >&5
$as_echo "$pgac_cv__libcurl_async_dns" >&6; }
- if test x"$pgac_cv__libcurl_async_dns" != xyes ; then
- { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING:
-*** The installed version of libcurl does not support asynchronous DNS
-*** lookups. Connection timeouts will not be honored during DNS resolution,
-*** which may lead to hangs in client programs." >&5
-$as_echo "$as_me: WARNING:
+ if test x"$pgac_cv__libcurl_async_dns" = xno ; then
+ as_fn_error $? "
*** The installed version of libcurl does not support asynchronous DNS
-*** lookups. Connection timeouts will not be honored during DNS resolution,
-*** which may lead to hangs in client programs." >&2;}
+*** lookups. Rebuild libcurl with the AsynchDNS feature enabled in order
+*** to use it with libpq." "$LINENO" 5
fi
fi
diff --git a/meson.build b/meson.build
index 13c13748e5d..b6daa5b7040 100644
--- a/meson.build
+++ b/meson.build
@@ -909,9 +909,7 @@ if not libcurlopt.disabled()
cdata.set('HAVE_THREADSAFE_CURL_GLOBAL_INIT', 1)
endif
- # Warn if a thread-friendly DNS resolver isn't built.
- libcurl_async_dns = false
-
+ # Fail if a thread-friendly DNS resolver isn't built.
if not meson.is_cross_build()
r = cc.run('''
#include <curl/curl.h>
@@ -931,16 +929,12 @@ if not libcurlopt.disabled()
)
assert(r.compiled())
- if r.returncode() == 0
- libcurl_async_dns = true
- endif
- endif
-
- if not libcurl_async_dns
- warning('''
+ if r.returncode() != 0
+ error('''
*** The installed version of libcurl does not support asynchronous DNS
-*** lookups. Connection timeouts will not be honored during DNS resolution,
-*** which may lead to hangs in client programs.''')
+*** lookups. Rebuild libcurl with the AsynchDNS feature enabled in order
+*** to use it with libpq.''')
+ endif
endif
endif
--
2.34.1
[application/octet-stream] 0004-oauth-Improve-validator-docs-on-interruptibility.patch (1.9K, ../CAOYmi+=4htNLXvuTS58GL96qwBptToDop09PWFy6uzDHNz4qTw@mail.gmail.com/5-0004-oauth-Improve-validator-docs-on-interruptibility.patch)
download | inline diff:
From d9f12352eec4002d30aa397dd3921f4340401c08 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 25 Feb 2025 07:42:43 -0800
Subject: [PATCH 4/5] oauth: Improve validator docs on interruptibility
Andres pointed out that EINTR handling is inadequate for real-world use
cases. Direct module writers to our wait APIs instead.
Discussion: https://postgr.es/m/p4bd7mn6dxr2zdak74abocyltpfdxif4pxqzixqpxpetjwt34h%40qc6jgfmoddvq
---
doc/src/sgml/oauth-validators.sgml | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/oauth-validators.sgml b/doc/src/sgml/oauth-validators.sgml
index 356f11d3bd8..704089dd7b3 100644
--- a/doc/src/sgml/oauth-validators.sgml
+++ b/doc/src/sgml/oauth-validators.sgml
@@ -209,11 +209,13 @@
<para>
Modules must remain interruptible by signals so that the server can
correctly handle authentication timeouts and shutdown signals from
- <application>pg_ctl</application>. For example, a module receiving
- <symbol>EINTR</symbol>/<symbol>EAGAIN</symbol> from a blocking call
- should call <function>CHECK_FOR_INTERRUPTS()</function> before retrying.
- The same should be done during any long-running loops. Failure to follow
- this guidance may result in unresponsive backend sessions.
+ <application>pg_ctl</application>. For example, blocking calls on sockets
+ should generally be replaced with code that handles both socket events
+ and interrupts without races (see <function>WaitLatchOrSocket()</function>,
+ <function>WaitEventSetWait()</function>, et al), and long-running loops
+ should periodically call <function>CHECK_FOR_INTERRUPTS()</function>.
+ Failure to follow this guidance may result in unresponsive backend
+ sessions.
</para>
</listitem>
</varlistentry>
--
2.34.1
[application/octet-stream] 0005-oauth-Simplify-copy-of-PGoauthBearerRequest.patch (992B, ../CAOYmi+=4htNLXvuTS58GL96qwBptToDop09PWFy6uzDHNz4qTw@mail.gmail.com/6-0005-oauth-Simplify-copy-of-PGoauthBearerRequest.patch)
download | inline diff:
From b5beb488b7727a0ec88401833f7d08a37438bbf4 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 24 Feb 2025 15:43:09 -0800
Subject: [PATCH 5/5] oauth: Simplify copy of PGoauthBearerRequest
Follow-up to 03366b61d. Since there are no more const members in the
PGoauthBearerRequest struct, the previous memcpy() can be replaced with
simple assignment.
---
src/interfaces/libpq/fe-auth-oauth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c
index fb1e9a1a8aa..cf1a25e2ccc 100644
--- a/src/interfaces/libpq/fe-auth-oauth.c
+++ b/src/interfaces/libpq/fe-auth-oauth.c
@@ -781,7 +781,7 @@ setup_token_request(PGconn *conn, fe_oauth_state *state)
goto fail;
}
- memcpy(request_copy, &request, sizeof(request));
+ *request_copy = request;
conn->async_auth = run_user_oauth_flow;
conn->cleanup_async_auth = cleanup_user_oauth_flow;
--
2.34.1
view thread (244+ 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]
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
In-Reply-To: <CAOYmi+=4htNLXvuTS58GL96qwBptToDop09PWFy6uzDHNz4qTw@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