-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add redirect guards #26597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add redirect guards #26597
Conversation
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds redirect guards to the Trino client to prevent security vulnerabilities where malicious redirects could expose sensitive information like AWS IAM credentials from link-local addresses.
- Implements a SafeRedirector interceptor that validates redirect destinations before allowing them
- Adds a new connection property
useSafeRedirectto enable/disable this security feature - Blocks redirects to private/local/loopback addresses that could expose sensitive services like AWS IMDS
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| SafeRedirector.java | Core implementation that validates redirect destinations and blocks unsafe addresses |
| TestSafeRedirector.java | Comprehensive test coverage for the redirect validation logic |
| ConnectionProperties.java | Adds the new USE_SAFE_REDIRECT connection property definition |
| PropertyName.java | Adds the property name enum value for the new configuration |
| TrinoUri.java | Exposes the safe redirect setting through the URI configuration |
| HttpClientFactory.java | Integrates the SafeRedirector interceptor when the feature is enabled |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
client/trino-client/src/main/java/io/trino/client/SafeRedirector.java
Outdated
Show resolved
Hide resolved
client/trino-client/src/main/java/io/trino/client/SafeRedirector.java
Outdated
Show resolved
Hide resolved
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
43beeb4 to
3c96459
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reconsidered this PR and I'm good with shipping it.
3c96459 to
88e1e05
Compare
|
@wendigo Could you update "Release notes" section? |
This is my first PR for Trino. We are using this patch in production to prevent the attack described below. Offering this upstream if you guys want it.
The driver will currently follow any redirects from the server. This can cause problems. For example when running on AWS there is a service running on a link-local address which provides IAM credentials (IMDS). If the server redirects to this address, the driver will return an error message with the IAM credentials.
(x) Release notes are required. Please propose a release note for me.
Client libraries
Add a feature to disallow following redirects to the local addresses.
Note: I emailed the signed CLA form last week.