Trino client throws 401: 'Unauthorized' error while attempting to connect to a cluster behind a LB with 307/308 redirect policy set

[alprusty]

During 412 release as part of OkHttp version upgrade, explicit client retry logic was removed in the event of a 307/308 redirect. This will cause 401: 'Unauthorized' error when trino client is talking to a Trino Cluster that is deployed behind a load balancer(For example AWS ALB) with a redirect mode(e.g. 307/308) routing policy.

Reference: 4be1401

Okhttp by default does follow redirects. However it drops Authorization headers if the redirect is across hosts. This causes a 401: 'Unauthorized' error.

Reference: https://github.com/square/okhttp/blob/okhttp_4.10.x/okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt#L323-L328

[oneonestar]

We shouldn't make this change until we've a clear consensus on whether to include the Authorization header on redirects.
Most libraries decided to NOT send the header:

• Python requests
• C#
• Java okhttp
• Golang net/http

Trino Java client and Python client do not send Authorization on redirects.
I haven't check Go client yet. I guess it is the same because the underlying net/http doesn't.

We need to agree on what's the correct behavior and enforce the same behavior across different clients as much as possible.

• Do not send Authorization header
• Send Authorization header conditionally (eg. new domain is a subdomain match or exact match of the initial domain, etc)
• Provide an option for user to config
• Send Authorization header

This decision could also affect the future development of trino-gateway.
I'm also working on a related issue, see trinodb/trino-gateway#269

@dain @electrum Could you please take a look?

---

Current behavior:

okhttp 4.12.0
https://github.com/square/okhttp/blob/4984568367caaf359b82c452bd28b5e192824d1c/okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt#L323-L328
https://github.com/square/okhttp/blob/4984568367caaf359b82c452bd28b5e192824d1c/okhttp/src/main/kotlin/okhttp3/internal/Util.kt#L304-L307

host == other.host &&
    port == other.port &&
    scheme == other.scheme

https://go.dev/src/net/http/client.go

when forwarding sensitive headers like "Authorization"...
These headers will be ignored when following a redirect to a domain
that is not a subdomain match or exact match of the initial domain.
For example, a redirect from "foo.com" to either "foo.com" or "sub.foo.com"
will forward the sensitive headers, but a redirect to "bar.com" will not.

Python requests
https://github.com/psf/requests/blob/a58d7f2ffb4d00b46dca2d70a3932a0b37e22fac/src/requests/sessions.py#L290-L295
https://github.com/psf/requests/blob/a58d7f2ffb4d00b46dca2d70a3932a0b37e22fac/src/requests/sessions.py#L127-L157

If we get redirected to a new host, we should strip out any authentication headers.
Special case: allow http -> https redirect when using the standard ports.

Related comments:
#1047 (comment)

[alprusty]

Hello @oneonestar
Thank you for reviewing the issue and pointing to the reference implementations in java, python, go clients at great details and sharing some history around it.

We need to agree on what's the correct behavior and enforce the same behavior across different clients as much as possible.

• Do not send Authorization header
• Send Authorization header conditionally (eg. new domain is a subdomain match or exact match of the initial domain, etc)
• Provide an option for user to config
• Send Authorization header

I think providing an option for user to configure the behavior (Option 3 in the list) sounds like a great approach.

We can come up with a clientOptions flag e.g. --follow-redirects that can take true or false values, with false being default value. This optional clientOption will let clients choose the behavior in case they are using a redirect approach before the request lands on coordinator.

This approach will also give users a choice to retain the old behavior (prior to 412 4be1401) with a minimal change around the extra client option.

Shall I revise the PR: #21027 to add this extra clientOptions ?
cc @oneonestar @dain @electrum

[electrum]

@dain had a good idea that we could add a client option to disable redirects entirely. Authentication is a required part of the protocol, so if the client is going to follow redirects, then it must send authentication. Thoughts?

[oneonestar]

Yea. A flag to control whether to follow a redirect is also easy to understand.

However, I can’t think of when a user would turn on this option. “Turn on this option if you want better security” most likely won’t work.

How about making the no follow redirect as default and add an option to turn it on?

(This also prevents someone in the future assigning a CVE to Trino saying that the default behavior is not secure, blah blah)

[electrum]

This is one of those areas with a trade-off between usability and security. Within an organization, the choice to use redirection on the Trino server side is made centrally by the Trino administrators. If redirection is used, then every Trino client needs to be configured to use it. Every user will need to add an extra property or option.

It makes moving from a non-redirection setup to a redirection setup much harder, as every client will need to be updated. This often happens when Trino usage is large and growing. This means administrators will be reluctant to do this, which limits their options for scaling.

We can ask the question: when would following redirects, or sending authentication after a redirect, be a security issue?

I think this is only a problem in situations where a hosted environment allows untrusted users to connect to a user specified Trino server. In that case, the client application would not trust the server and thus should not follow redirects. In the normal case, the user can trust the Trino server. If the user is already sending their credentials to the Trino server, why would they not trust sending credentials to the target of a redirect?

[oneonestar]

This means administrators will be reluctant to do this, which limits their options for scaling.

Good point.

where a hosted environment allows untrusted users to connect to a user specified Trino server. In that case, the client application would not trust the server and thus should not follow redirects

If the server is untrusted/malicious, the credentials would already be leaked in the first connection. Follow the redirects or not is irrelevant. I doubt how useful will be to implement a no follow redirect option in this case.

[alprusty]

Updated the PR and added a new client options flag called --follow-redirects to give an option to the trino admins to selectively specify if a redirect will be allowed or rejected. Default value is false.

Please take a look. 🙏
cc @oneonestar @electrum

[electrum]

If the server is untrusted/malicious, the credentials would already be leaked in the first connection.

Yes, so I don’t see a problem with enabling this behavior by default.

It should only be disabled if you are connecting to arbitrary servers provided by users (such as a hosted BI tool or dashboard that connects to a user specified Trino server).

[oneonestar]

Reopen this.
Only Authorization: Bearer has been fixed. Authorization: Basic is still not working.

[oneonestar]

I think there is a regression on this issue caused by #22687

$ java -jar trino-cli-450-executable.jar  --server https://loadbalancer.trino --password --user star --execute "SELECT 1"
Authentication failed: Unauthorized

$ java -jar trino-cli-451-executable.jar  --server https://loadbalancer.trino --password --user star --execute "SELECT 1"
"1"

$ java -jar trino-cli-452-executable.jar  --server https://loadbalancer.trino --password --user star --execute "SELECT 1"
"1"

$ java -jar trino-cli-453-executable.jar  --server https://loadbalancer.trino --password --user star --execute "SELECT 1"
Authentication failed: Unauthorized