Re: [PoC] Federated Authn/z with OAUTHBEARER - Mailing list pgsql-hackers

On 06.11.24 00:33, Jacob Champion wrote:
> Done in v36, attached.

Assorted review comments from me:

Everything in the commit message between

     = Debug Mode =

and

     Several TODOs:

should be moved to the documentation.  In some cases, it already is,
but it doesn't always have the same level of detail.

(You could point from the commit message to .sgml files if you want to
highlight usage instructions, but I don't think this is generally
necessary.)

* config/programs.m4

Can we do the libcurl detection using pkg-config only?  Seems simpler,
and maintains consistency to meson.

* doc/src/sgml/client-auth.sgml

In the list of terms (this could be a <variablelist>), state how these
terms map to a PostgreSQL installation.  You already explain what the
client and the resource server are, but not who the resource owner is
and what the authorization server is.  It would also be good to be
explicit and upfront that the authorization server is a third-party
component that needs to be obtained separately.

trust_validator_authz: Personally, I'm not a fan of the "authz" and
"authn" abbreviations.  I know this is security jargon.  But are
regular users going to understand this?  Can we just spell it out?

* doc/src/sgml/config.sgml

Also here maybe state that these OAuth libraries have to be obtained
separately.

* doc/src/sgml/installation.sgml

I find the way the installation options are structured a bit odd.  I
would have expected --with-libcurl and -Dlibcurl (or --with-curl and
-Dcurl).  These build options usually just say, use this library.  We
don't spell out what, for example, libldap is used for, we just use it
and enable all the features that require it.

* doc/src/sgml/libpq.sgml

Maybe oauth_issuer should be oauth_issuer_url?  Otherwise one might
expect to just write "google" here or something.  Or there might be
other ways to contact an issuer in the future?  Just a thought.

* doc/src/sgml/oauth-validators.sgml

This chapter says "libpq" several times, but I think this is a server
side plugin, so libpq does not participate.  Check please.

* src/backend/libpq/auth-oauth.c

I'm confused by the use of PG_MAX_AUTH_TOKEN_LENGTH in the
pg_be_oauth_mech definition.  What does that mean?

+#define KVSEP 0x01
+#define AUTH_KEY "auth"
+#define BEARER_SCHEME "Bearer "

Add comments to these.

Also, add comments to all functions defined here that don't have one
yet.

* src/backend/utils/misc/guc_tables.c

Why is oauth_validator_library GUC_NOT_IN_SAMPLE?

Also, shouldn't this be an hba option instead?  What if you want to
use different validators for different connections?

* src/interfaces/libpq/fe-auth-oauth-curl.c

The CURL_IGNORE_DEPRECATION thing needs clarification.  Is that in
progress?

+#define MAX_OAUTH_RESPONSE_SIZE (1024 * 1024)

Add a comment about why this value.

+   union
+   {
+       char      **scalar;     /* for all scalar types */
+       struct curl_slist **array;  /* for type == JSON_TOKEN_ARRAY_START */
+   };

This is an anonymous union, which requires C11.  Strangely, I cannot
get clang to warn about this with -Wc11-extensions.  Probably better
to fix anyway.  (The trailing supported MSVC versions don't support
C11 yet.)

* src/interfaces/libpq/fe-auth.h

+extern const pg_fe_sasl_mech pg_oauth_mech;

Should this rather be in fe-auth-oauth.h?

* src/interfaces/libpq/libpq-fe.h

The naming scheme of types and functions in this file is clearly
obscure and has grown randomly over time.  But at least my intuition
is that the preferred way is

types start with PG
function start with PQ

and the next letter is usually lower case. (PQconnectdb, PQhost,
PGconn, PQresult)

Maybe check your additions against that.

* src/interfaces/libpq/pqexpbuffer.c
* src/interfaces/libpq/pqexpbuffer.h

Let's try to do this without opening up additional APIs here.

This is only used once, in append_urlencoded(), and there are other
ways to communicate errors, for example returning a bool.

* src/test/modules/oauth_validator/

Everything in this directory needs more comments, at least on a file
level.

Add a README in this directory.  Also update the README in the upper
directory.

* src/test/modules/oauth_validator/t/001_server.pl

On Cirrus CI Windows task, this test reports SKIP.  Can't tell why,
because the log is not kept.  I suppose you expect this to work on
Windows (but see my comment below), so it would be good to get this
test running.

* src/test/modules/oauth_validator/t/002_client.pl

+my $issuer = "https://127.0.0.1:54321";

Use PostgreSQL::Test::Cluster::get_free_port() instead of hardcoding
port numbers.

Or is this a real port?  I don't see it used anywhere else.

+   diag "running '" . join("' '", @cmd) . "'";

This should be "note" instead.  Otherwise it garbles the output.

* src/test/perl/PostgreSQL/Test/OAuthServer.pm

Add some comments to this file, what it's for.

Is this meant to work on Windows?  Just thinking, things like

kill(15, $self->{'pid'});

pgperlcritic complains:

src/test/perl/PostgreSQL/Test/OAuthServer.pm: Return value of flagged 
function ignored - read at line 39, column 2.

* src/tools/pgindent/typedefs.list

We don't need to typedef every locally used enum or similar into a
full typedef.  I suggest the following might be unnecessary:

AsyncAuthFunc
OAuthStep
fe_oauth_state_enum