Handle secondary TLS port 8443 as https by default#580
Handle secondary TLS port 8443 as https by default#580Spikhalskiy wants to merge 1 commit intotrinodb:masterfrom
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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 |
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
The Trino docs do use 8443 as the default TLS port so I understand the motivation. Hardcoding specific ports is fragile though and there is no limit to ports. The better approach would be to detect the presence of
credentials/auth and infer HTTPS from that (as the client did before #530). That would cover 8443 and any other port without needing special cases. Note that Trino can accept credentials
over HTTP too, so even credential detection isn't perfect, but it's a much better heuristic than port numbers.
As-is, this change risks silently breaking users running HTTP on 8443 who don't pass http_scheme explicitly.
|
Also current behaviour is intentionally aligned to JDBC driver so that all clients behave similarly. |
2 participants
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: