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

> On 28 Feb 2024, at 15:05, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
>
> [Trying again, with all patches unzipped and the CC list temporarily
> removed to avoid flooding people's inboxes. Original message follows.]
>
> On Fri, Feb 23, 2024 at 5:01 PM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>> The
>> patchset is now carrying a lot of squash-cruft, and I plan to flatten
>> it in the next version.
>
> This is done in v17, which is also now based on the two patches pulled
> out by Daniel in [1]. Besides the squashes, which make up most of the
> range-diff, I've fixed a call to strncasecmp() which is not available
> on Windows.
>
> Daniel and I discussed trying a Python version of the test server,
> since the standard library there should give us more goodies to work
> with. A proof of concept is in 0009. I think the big question I have
> for it is, how would we communicate what we want the server to do for
> the test? (We could perhaps switch on magic values of the client ID?)
> In the end I'd like to be testing close to 100% of the failure modes,
> and that's likely to mean a lot of back-and-forth if the server
> implementation isn't in the Perl process.

Thanks for the new version, I'm digesting the test patches but for now I have a
few smaller comments:


+#define ALLOC(size) malloc(size)
I wonder if we should use pg_malloc_extended(size, MCXT_ALLOC_NO_OOM) instead
to self document the code.  We clearly don't want feature-parity with server-
side palloc here.  I know we use malloc in similar ALLOC macros so it's not
unique in that regard, but maybe?


+#ifdef FRONTEND
+               destroyPQExpBuffer(lex->errormsg);
+#else
+               pfree(lex->errormsg->data);
+               pfree(lex->errormsg);
+#endif
Wouldn't it be nicer if we abstracted this into a destroyStrVal function to a)
avoid the ifdefs and b) make it more like the rest of the new API?  While it's
only used in two places (close to each other) it's a shame to let the
underlying API bleed through the abstraction.


+   CURLM      *curlm;      /* top-level multi handle for cURL operations */
Nitpick, but curl is not capitalized cURL anymore (for some value of "anymore"
since it changed in 2016 [0]).  I do wonder if we should consistently write
"libcurl" as well since we don't use curl but libcurl.


+   PQExpBufferData     work_data;  /* scratch buffer for general use (remember
+                                      to clear out prior contents first!) */
This seems like asking for subtle bugs due to uncleared buffers bleeding into
another operation (especially since we are writing this data across the wire).
How about having an array the size of OAuthStep of unallocated buffers where
each step use it's own?  Storing the content of each step could also be useful
for debugging.  Looking at the statemachine here it's not an obvious change but
also not impossible.


+ * TODO: This disables DNS resolution timeouts unless libcurl has been
+ * compiled against alternative resolution support. We should check that.
curl_version_info() can be used to check for c-ares support.


+ * so you don't have to write out the error handling every time. They assume
+ * that they're embedded in a function returning bool, however.
It feels a bit iffy to encode the returntype in the macro, we can use the same
trick that DISABLE_SIGPIPE employs where a failaction is passed in.


+  if (!strcmp(name, field->name))
Project style is to test for (strcmp(x,y) == 0) rather than (!strcmp()) to
improve readability.


+  libpq_append_conn_error(conn, "out of memory");
While not introduced in this patch, it's not an ideal pattern to report "out of
memory" errors via a function which may allocate memory.


+  appendPQExpBufferStr(&conn->errorMessage,
+           libpq_gettext("server's error message contained an embedded NULL"));
We should maybe add ", discarding" or something similar after this string to
indicate that there was an actual error which has been thrown away, the error
wasn't that the server passed an embedded NULL.


+#ifdef USE_OAUTH
+       else if (strcmp(mechanism_buf.data, OAUTHBEARER_NAME) == 0 &&
+               !selected_mechanism)
I wonder if we instead should move the guards inside the statement and error
out with "not built with OAuth support" or something similar like how we do
with TLS and other optional components?


+   errdetail("Comma expected, but found character %s.",
+             sanitize_char(*p))));
The %s formatter should be wrapped like '%s' to indicate that the message part
is the character in question (and we can then reuse the translation since the
error message already exist for SCRAM).


+       temp = curl_slist_append(temp, "authorization_code");
+       if (!temp)
+           oom = true;
+
+       temp = curl_slist_append(temp, "implicit");
While not a bug per se, it reads a bit odd to call another operation that can
allocate memory when the oom flag has been set.  I think we can move some
things around a little to make it clearer.

The attached diff contains some (most?) of the above as a patch on top of your
v17, but as a .txt to keep the CFBot from munging on it.

--
Daniel Gustafsson