Re: [PoC] Federated Authn/z with OAUTHBEARER

Hi Daniel,

On Mon, Apr 1, 2024 at 3:07 PM Jacob Champion
<jacob(dot)champion(at)enterprisedb(dot)com> wrote:
> Of the Cirrus machines, it looks like only FreeBSD has a new enough
> libcurl for that. Ubuntu won't until 24.04, Debian Bookworm doesn't
> have it unless you're running backports, RHEL 9 is still on 7.x... I
> think requiring libcurl 8 is effectively saying no one will be able to
> use this for a long time. Is there an alternative?

Since the exit() checks appear to be happy now that fe_memutils is
out, I've lowered the requirement to the version of libcurl that seems
to be shipped in RHEL 8 (7.61.0). This also happens to be when TLS 1.3
ciphersuite control was added to Curl, which seems like something we
may want in the very near future, so I'm taking that as a good sign
for what is otherwise a very arbitrary cutoff point. Counterproposals
welcome :D

> Good catch. application/json no longer defines charsets officially
> [1], so I think we should be able to just ignore them. The new
> strncasecmp needs to handle a spurious prefix, too; I have that on my
> TODO list.

I've expanded this handling in v24, attached.

> This new way doesn't do the same thing. Here's a sample error:
>
> connection to server at "127.0.0.1", port 56619 failed: failed to
> fetch OpenID discovery document: Weird server reply ( Trying
> 127.0.0.1:36647...
> Connected to localhost (127.0.0.1) port 36647 (#0)
> Mark bundle as not supporting multiuse
> HTTP 1.0, assume close after body
> Invalid Content-Length: value
> Closing connection 0
> )
>
> IMO that's too much noise. Prior to the change, the same error would have been
>
> connection to server at "127.0.0.1", port 56619 failed: failed to
> fetch OpenID discovery document: Weird server reply (Invalid
> Content-Length: value)

I have reverted this change for now, but I'm still hoping there's an
alternative that can help us clean up?

> `running` can be set to zero on success, too. I'm having trouble
> forcing that code path in a test so far, but we're going to have to do
> something special in that case.

For whatever reason, the magic timing for this is popping up more and
more often on Cirrus, leading to really annoying test failures. So I
may have to abandon the search for a perfect test case and just fix
it.

> > I did drop
> > the Python pytest patch since I feel that it's unlikely to go in from this
> > thread (adding pytest seems worthy of its own thread and discussion), and the
> > weight of it makes this seem scarier than it is.
>
> Until its coverage gets ported over, can we keep it as a `DO NOT
> MERGE` patch? Otherwise there's not much to run in Cirrus.

I have added this back (marked loudly as don't-merge) so that we keep
the test coverage for now. The Perl suite (plus Python server) has
been tricked out a lot more in v24, so it shouldn't be too bad to get
things ported.

> > Next I intend to work on writing documentation for this.
>
> Awesome, thank you! I will start adding coverage to the new code paths.

Peter E asked for some documentation stubs to ease review, which I've
added. Hopefully that doesn't step on your toes any.

A large portion of your "Review comments" patch has been pulled
backwards into the previous commits; the remaining pieces are things
I'm still peering at and/or writing tests for. I also owe this thread
an updated roadmap and summary, to make it a little less daunting for
new reviewers. Soon (tm).

Thanks!
--Jacob