Feedback-ID: ie0a040ee:Fastmail
Message-ID: <b09bc106-32f2-4e71-8c35-4db8608776a1@eisentraut.org>
Date: Thu, 2 Jan 2025 11:11:07 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
To: Jacob Champion <jacob.champion@enterprisedb.com>,
 Daniel Gustafsson <daniel@yesql.se>
Cc: Antonin Houska <ah@cybertec.at>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com>
 <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>
 <6bde5f56-9e7a-4148-b81c-eb6532cb3651@eisentraut.org>
 <CAOYmi+=ee8H=GaGNH_gZvBE+YPtmK9eqUdXqemk=zKDnO8YXRw@mail.gmail.com>
 <CAOYmi+kLTJ1wZ6gxRbgtR52E=EyiCpmp6J3mmSvtc1a6i7sZ3Q@mail.gmail.com>
 <CAOYmi+=ceXepXOrQXsxca-u57GxM+x_q34fpyNJ8PLmbUWPwEQ@mail.gmail.com>
 <9B417838-B872-453D-BBFB-99FA957EB723@yesql.se>
 <CAOYmi+=vJQ8EUiVf-iHYBwz4-tLHLrvp8rYDoNicB2yJrBKraw@mail.gmail.com>
 <CAOYmi+=FzVg+C-pQHCwjW0qU-POHmzZaD2z3CdsACj==14H8kQ@mail.gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <CAOYmi+=FzVg+C-pQHCwjW0qU-POHmzZaD2z3CdsACj==14H8kQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/b09bc106-32f2-4e71-8c35-4db8608776a1%40eisentraut.org>
Precedence: bulk

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. ;-)