Fixed OAuth2Authentication redirect_auth_url_handler callback not firing

[matialm]

Remove the Bearer from x_redirect_server, with the count remove only the first coincidence and it make fails the redirect auth url callback.

Description

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required. ( ) Release notes are required, please propose a release note for me. (X ) Release notes are required, with the following suggested text:

* Fix the non firing auth url callback issue when oauth2 is used ({issue}`444`)

[cla-bot[bot]]

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

[matialm]

Contributor License Agreement signed and sended

[hashhar]

cc: @lukasz-walkiewicz

[lukasz-walkiewicz]

Thanks @matialm for this PR but I don't understand what you're trying to fix here. Could you please try to explain it in the description and/or ideally add a test for the issue you're trying to fix?

[matialm]

Thanks @matialm for this PR but I don't understand what you're trying to fix here. Could you please try to explain it in the description and/or ideally add a test for the issue you're trying to fix?

Sorry for the late response, it fix the issue in this discussion: https://github.com/trinodb/trino-python-client/discussions/437

When I debug it I find it that the oauth2 redirect url callback wasn't been fired because it wasn't retriving the x_redirect_server parameter correctly. Long story short, when they were trying to remove the prefix "Bearer" from the headers because the "count=1" parameter it was removing that prefix only for the parameter x_token_server and not from x_redirect_server. Removing "count=1" when the regex is applied it remove it to every occurence of that prefix.

[mahic]

I can confirm an issue with callback not firing because of count=1 with Trino 427 with a basic OAuth setup verified working with DBeaver. It seems like the response from the server does not contain count=1 in the header for that particular version.

By removing count=1 on a debug install of 0.328 the callback fires. What does the counter do and was it added to Trino in a recent release?

[hashhar]

It's entirely unexpected. The auth challenge header format is standardised - see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate.

In general it's of the form <auth-scheme> auth-param1=token1, ..., auth-paramN=auth-paramN-token where <auth-scheme> = Bearer, x_redirect_server and x_token_server are auth-params. So it's impossible/invalid to have multiple Bearer occurrences before each auth-param. The count=1 means remove the leftmost occurrence if it exists, not more. Because it can then do things like: Bearer x_redirect_server=something-with-bearer-word x_token_server -> x_redirect_server=something-with-word (note that bearer got dropped).

FWIW this implementation can be changed to not even use regex - it's not needed. I'll send a PR for that.

Can you try to log the value of the headers? Also what IdP are you observing this with? I've tested with Okta and Azure AD for example and it works.

[hashhar]

Figured out the possible issue but will need you to share the values of the WWW-Authenticate headers that you see.

One thing which can happen is that the IdP supports multiple auth mechanisms and hence repsonds with multiple challenges - either within same WWW-Authenticate header or multiple occurrences of that header. Neither case is handled correctly today.

We need to change the parsing logic for www-authenticate header. This proposed fix is incorrect either way.

It'll end up something complicated looking like https://github.com/Azure/azure-sdk-for-python/blob/b09862b49daa95ba8b1c7360ca1d95ef9fd55011/sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/_shared/http_challenge.py#L16 because sadly I can't find any good libraries for handling www-authenticate headers.

[mahic]

@hashhar

Our Idp is ForgeRock This is the 401 Headers returned from the IdP:

HTTP/1.1 401 Unauthorized
date: Fri, 08 Mar 2024 15:56:27 GMT
content-type: text/plain;charset=utf-8
www-authenticate: Basic realm="Trino"
www-authenticate: Bearer realm="Trino", token_type="JWT"
www-authenticate: Bearer x_redirect_server="https://trino.ourdomain.com/oauth2/token/initiate/9e7e5766b1603b9151aa6712f995915658b990c18a6f9a7b350de889651f1ad6", x_token_server="https://trino.ourdomain.com/oauth2/token/c3ade6f9-2a19-1e26-a4a7-0102dbbe8022"
content-length: 12
x-envoy-upstream-service-time: 1
server: envoy

Seems you're right about the multiple auth-mechanisms, but this works fine with Trino JDBC (used in DBeaver) 🤷‍♂️

[matialm]

This days I'm out but next week I will share an example of the problem I'm refering to.

[hashhar]

Seems you're right about the multiple auth-mechanisms, but this works fine with Trino JDBC (used in DBeaver) 🤷‍♂️

Yes, the JDBC driver uses an HTTP library (OkHttp) which is able to return all list of challenges. Python requests collapses multi-valued HTTP headers into a single header with all values comma-separated. That's why changing the count for re.sub works. It'll also however break stuff if the sequence of characters bearer happens to appear anywhere in the URLs or tokens too.

Thanks for confirming the issue. That itself makes it much easier to test the fixes.