Re: [PoC] Federated Authn/z with OAUTHBEARER

On 12.11.24 22:47, Jacob Champion wrote:
> On Fri, Nov 8, 2024 at 1:21 AM Peter Eisentraut <peter(at)eisentraut(dot)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.