Re: [PoC] Federated Authn/z with OAUTHBEARER

From: Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>
To: Peter Eisentraut <peter(at)eisentraut(dot)org>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Daniel Gustafsson <daniel(at)yesql(dot)se>
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
Date: 2024-08-05 17:53:24
Message-ID: CAOYmi+=pg=W5L1h=3MEP_EB24jaBu2FyATrLXqQHGe7cpuvwyg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Aug 2, 2024 at 11:48 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> Yes, I think with an adjusted comment and commit message, the actual
> change makes sense.

Done in v25.

...along with a bunch of other stuff:

1. All the debug-mode things that we want for testing but not in
production have now been hidden behind a PGOAUTHDEBUG environment
variable, instead of being enabled by default. At the moment, that
means 1) sensitive HTTP traffic gets printed on stderr, 2) plaintext
HTTP is allowed, and 3) servers may DoS the client by sending a
zero-second retry interval (which speeds up testing a lot). I've
resurrected some of Daniel's CURLOPT_DEBUGFUNCTION implementation for
this.

I think this feature needs more thought, but I'm not sure how much. In
particular I don't think a connection string option would be
appropriate (imagine the "fun" a proxy solution would have with a
spray-my-password-to-stderr switch). But maybe it makes sense to
further divide the dangerous behavior up, so that for example you can
debug the HTTP stream without also allowing plaintext connections, or
something. And maybe stricter maintainers would like to compile the
feature out entirely?

2. The verification_url variant from Azure and Google is now directly supported.

@Daniel: I figured out why I wasn't seeing the string-based-interval
issue in my testing. I've been using Azure's v2.0 OpenID endpoint,
which seems to be much more compliant than the original. Since this is
a new feature, would it be okay to just push new users to that
endpoint rather than supporting the previous weirdness in our code?
(Either way, I think we should support verification_url.)

Along those lines, with Azure I'm now seeing that device_code is not
advertised in grant_types_supported... is that new behavior? Or did
iddawc just not care?

3. I've restructured the libcurl calls to allow
curl_multi_socket_action() to synchronously succeed on its first call,
which we've been seeing a lot in the CI as mentioned upthread. This
led to a bunch of refactoring of the top-level state machine, which
had gotten too complex. I'm much happier with the code organization
now, but it's a big diff.

4. I've changed things around to get rid of two modern libcurl
deprecation warnings. I need to ask curl-library about my use of
curl_multi_socket_all(), which seems like it's exactly what our use
case needs.

Thanks,
--Jacob

Attachment Content-Type Size
since-v24.diff.txt text/plain 40.7 KB
v25-0005-backend-add-OAUTHBEARER-SASL-mechanism.patch application/octet-stream 68.3 KB
v25-0004-libpq-add-OAUTHBEARER-SASL-mechanism.patch application/octet-stream 118.6 KB
v25-0001-Revert-ECPG-s-use-of-pnstrdup.patch application/octet-stream 1.8 KB
v25-0002-Remove-fe_memutils-from-libpgcommon_shlib.patch application/octet-stream 2.3 KB
v25-0003-common-jsonapi-support-libpq-as-a-client.patch application/octet-stream 33.0 KB
v25-0006-Review-comments.patch application/octet-stream 7.3 KB
v25-0007-DO-NOT-MERGE-Add-pytest-suite-for-OAuth.patch application/octet-stream 180.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2024-08-05 18:50:51 Re: BlastRADIUS mitigation
Previous Message Robert Haas 2024-08-05 17:40:59 Re: Is *fast* 32-bit support still important?