-
Notifications
You must be signed in to change notification settings - Fork 196
Handle secondary TLS port 8443 as https by default #580
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
base: master
Are you sure you want to change the base?
Handle secondary TLS port 8443 as https by default #580
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 cla@trino.io. 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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdd support for treating port 8443 as HTTPS by default by introducing a secondary TLS port constant, updating the connection parsing logic to recognize it, and expanding unit tests to cover this behavior. Entity relationship diagram for port constantserDiagram
CONSTANTS {
int DEFAULT_PORT
int DEFAULT_TLS_PORT
int SECONDARY_TLS_PORT
}
Class diagram for updated connection scheme handling in dbapiclassDiagram
class DBAPIConnection {
http_scheme: str
__init__(...)
}
class Constants {
DEFAULT_PORT: int = 8080
DEFAULT_TLS_PORT: int = 443
SECONDARY_TLS_PORT: int = 8443
HTTP: str
HTTPS: str
}
DBAPIConnection --|> Constants: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/unit/test_dbapi.py:328` </location>
<code_context>
("http://mytrinoserver.domain:9999", None, None, constants.HTTP),
# Infer from port
("mytrinoserver.domain", constants.DEFAULT_TLS_PORT, None, constants.HTTPS),
+ ("mytrinoserver.domain", constants.SECONDARY_TLS_PORT, None, constants.HTTPS),
("mytrinoserver.domain", constants.DEFAULT_PORT, None, constants.HTTP),
# http_scheme parameter has higher precedence than port parameter
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for connection strings with explicit 'http' and 'https' schemes using port 8443.
Adding these tests will verify that explicit scheme declarations take precedence over port-based inference for port 8443, mirroring the approach used for port 443.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
CLA submitted |
Description
Before 0.334.0, both
login@password:host:443andlogin@password:host:8443URLs were handled as HTTPS because the connection string contained a password, andhttpswas enforced by Trino in such situations.#530 removed this enforcement. And all connection strings without an explicitly specified additional schema parameter were treated as
http.#564 added a fix for the 443 port. And 443 port only.
Meanwhile, 8443 is a secondary HTTPS/TLS port, just as 8080 is a secondary port for port 80. Even trino TLS documentation uses 8443 by default: https://trino.io/docs/current/security/tls.html
This PR adds handling of 8443 port as HTTPS by default.
Non-technical explanation
Connection strings with 8443 port should be handled as HTTPS by default.
Release notes
( x ) Release notes are required, with the following suggested text: