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

On 12.11.24 22:47, Jacob Champion wrote:
> On Fri, Nov 8, 2024 at 1:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> 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.
> 
> It's patterned directly off of -Dssl/--with-ssl (which I liberally
> borrowed from) because the builtin client implementation used to have
> multiple options for the library in use. I can change it if needed,
> but I thought it'd be helpful for future devs if I didn't undo the
> generalization.

Personally, I'm not even a fan of the -Dssl/--with-ssl system.  I'm more 
attached to --with-openssl.  But if you want to stick with that, a more 
suitable naming would be something like, say, --with-httplib=curl, which 
means, use curl for all your http needs.  Because if we later add other 
functionality that can use some http, I don't think we want to enable or 
disable them all individually, or even mix different http libraries for 
different features.  In practice, curl is a widely available and 
respected library, so I'd expect packagers to be just turn it all on 
without much further consideration.


>> I'm confused by the use of PG_MAX_AUTH_TOKEN_LENGTH in the
>> pg_be_oauth_mech definition.  What does that mean?
> 
> Just that Bearer tokens can be pretty long, so we don't want to limit
> them to 1k like SCRAM does. 64k is probably overkill, but I've seen
> anecdotal reports of tens of KBs and it seemed reasonable to match
> what we're doing for GSS tokens.

Ah, ok, I totally misread that code.  Could you maybe write this definition

+/* Mechanism declaration */
+const pg_be_sasl_mech pg_be_oauth_mech = {
+   oauth_get_mechanisms,
+   oauth_init,
+   oauth_exchange,
+
+   PG_MAX_AUTH_TOKEN_LENGTH,
+};

with designated initializers:

const pg_be_sasl_mech pg_be_oauth_mech = {
     .get_mechanisms = oauth_get_mechanisms,
     .init = oauth_init,
     .exchange = oauth_exchange,
     .max_message_length = PG_MAX_AUTH_TOKEN_LENGTH,
};


>> The CURL_IGNORE_DEPRECATION thing needs clarification.  Is that in
>> progress?
> 
> Thanks for the nudge, I've started a thread:
> 
>      https://curl.se/mail/lib-2024-11/0028.html

It looks like this has been clarified, so let's put that URL into a code 
comment.


>> This is only used once, in append_urlencoded(), and there are other
>> ways to communicate errors, for example returning a bool.
> 
> I'd rather not introduce two parallel error indicators for the caller
> to have to check for that particular part. But I can change over to
> using the (identical!) termPQExpBuffer. I felt like the other API
> signaled the intent a little better, though.

I think it's better to not drill a new hole into an established API for 
such a limited use.  So termPQExpBuffer() seems better for now.  If it 
later turns out, many callers are using termPQExpBuffer() for fake error 
handling purposes, then that can be considered independently.


>> 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)
> 
> No, builtin client support does not exist on Windows. If/when it's
> added, the 001_server tests will need to be ported.

Could you put some kind of explicit conditional or a comment in there. 
Right now, it's not possible to tell that Windows is not supported.