public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Eisentraut <[email protected]>
To: Jacob Champion <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Antonin Houska <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
Date: Thu, 2 Jan 2025 11:11:07 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAOYmi+=FzVg+C-pQHCwjW0qU-POHmzZaD2z3CdsACj==14H8kQ@mail.gmail.com>
References: <[email protected]>
<4503.1727703521@antos>
<CAOYmi+=8rkKh_8o9iyGQk_J4MQRCfpq3Qj3-dDyrnJPQ96bHYQ@mail.gmail.com>
<63648.1728384409@antos>
<2453.1729230681@antos>
<CAOYmi+mJkc4mLD0+LtbZ1VxeFUSOQvXQ_HUx6eBBmHTXLuoK3g@mail.gmail.com>
<CAOYmi+=xf76rSLy+eMVecgmyJwEnRp+iFXrbdo4z+p5McrxSBA@mail.gmail.com>
<CAOYmi+mbiD+efRiS+hH1mdTB4J6pjbr053+jo+BsXKrQjopCSg@mail.gmail.com>
<CAOYmi+kHXMG2Z2KR7y36Cbsop_7t-YugrpfCnOXuXz8BpfaXaw@mail.gmail.com>
<[email protected]>
<CAOYmi+=ee8H=GaGNH_gZvBE+YPtmK9eqUdXqemk=zKDnO8YXRw@mail.gmail.com>
<CAOYmi+kLTJ1wZ6gxRbgtR52E=EyiCpmp6J3mmSvtc1a6i7sZ3Q@mail.gmail.com>
<CAOYmi+=ceXepXOrQXsxca-u57GxM+x_q34fpyNJ8PLmbUWPwEQ@mail.gmail.com>
<[email protected]>
<CAOYmi+=vJQ8EUiVf-iHYBwz4-tLHLrvp8rYDoNicB2yJrBKraw@mail.gmail.com>
<CAOYmi+=FzVg+C-pQHCwjW0qU-POHmzZaD2z3CdsACj==14H8kQ@mail.gmail.com>
On 20.12.24 02:00, Jacob Champion wrote:
> v40 also contains:
> - explicit testing for connect_timeout compatibility
> - support for require_auth=oauth, including compatibility with
> require_auth=!scram-sha-256
> - the ability for a validator to set authn_id even if the token is not
> authorized, for auditability in the logs
> - the use of pq_block_sigpipe() for additional safety in the face of
> CURLOPT_NOSIGNAL
>
> I have split out the require_auth changes temporarily (0002) for ease
> of review, and I plan to ping the last thread where SASL support in
> require_auth was discussed [1].
Some review of v40:
General:
There is a mix of using "URL" and "URI" throughout the patch. I tried
to look up in the source material (RFCs) what the correct use would
be, but even they are mixing it in nonobvious ways. Maybe this is
just hopelessly confused, or maybe there is a system that I don't
recognize.
* .cirrus.tasks.yml
Since libcurl is an "auto" meson option, it doesn't need to be enabled
explicitly. At least that's how most of the other feature options are
handled. So probably better to stick to that pattern.
* config/programs.m4
Useless whitespace change.
* configure.ac
+AC_MSG_CHECKING([whether to build with libcurl support for OAuth client
flows])
etc.
Let's just write something like 'whether to build with libcurl
support' here. So we don't have to keep updating it if the scope of
the option changes.
* meson_options.txt
+option('libcurl', type : 'feature', value: 'auto',
+ description: 'libcurl support for OAuth client flows')
Similarly, let's just write something like 'libcurl support' here.
* src/backend/libpq/auth-oauth.c
+typedef enum
+{
+ OAUTH_STATE_INIT = 0,
+ OAUTH_STATE_ERROR,
+ OAUTH_STATE_FINISHED,
+} oauth_state;
+
+/* Mechanism callback state. */
+struct oauth_ctx
+{
+ oauth_state state;
This is the only use of that type definition. I think you can skip
the typedef and use the enum tag directly.
* src/interfaces/libpq/libpq-fe.h
+#ifdef WIN32
+#include <winsock2.h> /* for SOCKET */
+#endif
Including a system header like that seems a bit unpleasant. AFAICT,
you only need it for this:
+ PostgresPollingStatusType (*async) (PGconn *conn,
+ struct _PGoauthBearerRequest
*request,
+ SOCKTYPE * altsock);
But that function could already get the altsock handle via
conn->altsock. So maybe that is a way to avoid the whole socket type
dance in this header.
* src/test/authentication/t/001_password.pl
I suppose this file could be a separate commit? It just separates the
SASL/SCRAM terminology for existing functionality.
* src/test/modules/oauth_validator/fail_validator.c
+{
+ elog(FATAL, "fail_validator: sentinel error");
+ pg_unreachable();
+}
This pg_unreachable() is probably not necessary after elog(FATAL).
* .../modules/oauth_validator/oauth_hook_client.c
+#include <stdio.h>
+#include <stdlib.h>
These are generally not necessary, as they come in via c.h.
+#ifdef WIN32
+#include <winsock2.h>
+#else
+#include <sys/socket.h>
+#endif
I don't think this special Windows handling is necessary, since there
is src/include/port/win32/sys/socket.h.
+static void
+usage(char *argv[])
+{
+ fprintf(stderr, "usage: %s [flags] CONNINFO\n\n", argv[0]);
Help output should go to stdout.
With the above changes, I think this patch set is structurally okay now.
Now it just needs to do the right things. ;-)
view thread (67+ 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: [PoC] Federated Authn/z with OAUTHBEARER
In-Reply-To: <[email protected]>
* 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