Hello!
> Yep, we need to support back to Python 3.6.8 (which I've tested your
> patch with locally).
That's indeed good to know, there's a way to know which is the oldest
version of Python used? What do you think about adding this information
on top of the `oauth_server.py` file? Just as a reminder because I
suspected that the lower version will be 3.9 not 3.6
> Comments:
>
> > Signed-off-by: Jonathan Gonzalez V. <jonathan.abdiel@gmail.com>
>
> Note that we have no signoff convention here, at least that I'm aware
> of. It's not a problem to include it, but this will be replaced by an
> `Author:` line in the final commit. Submissions here fall under the
> Archive Policy:
>
> https://www.postgresql.org/about/policies/archives/
Yes, I'm fully aware of this, it's just my default git configuration to
add the sign-off, I'll check to remove that, thanks for the point!
>
> > +my $certdir = dirname(__FILE__) . "/../../../ssl/ssl";
> > +$ENV{PGOAUTHCAFILE} = "$certdir/root+server_ca.crt";
>
> I think we should inject this directory from the build system,
> instead
> of walking across the tree.
I wasn't aware of this, will keep in mind for hte future
>
> > - my $issuer = "http://127.0.0.1:$port";
> > + my $issuer = "https://localhost:$port";
>
> Unfortunately this will cause strange bugs [1]: we aren't listening
> on
> IPv6, but Curl will attempt to contact a DNS hostname on both IPv4
> and
> IPv6 simultaneously, leading to intermittent failures. So we'll need
> to get the certificate's SANs working...
I wasn't aware of this, good learning! I'll keep it in mind for the
future, and probably we can work on IPv6 support later.
> > server-ip-in-dnsname \
> > + server-ip-localhost \
> > server-single-alt-name \
>
> nitpick: Though the naming scheme is not really all that well
> defined,
> I think this probably belongs with the other -alt-name certs rather
> than the -ip- certs.
I had my questions if add it to the alt-name too, and now I'm thinking
in other test ideas due to this, like using multiple alternative names
for the host and issuer, but for sure, for the future not here!
> --
>
> To avoid making you play fetch-a-rock, I've attached these comments
> in
> code form as v1.1-0002 (but you're under no obligation to take them
> if
> you'd prefer to do it a different way :D). I also ran oauth_server.py
> through Black.
I did the same! just added the comments I mentioned, besides that, all
good on my side.
Thank you!
---
Jonathan Gonzalez V. <jonathan.abdiel@gmail.com>
EnterpriseDB
