Skip to content

Add hostnameInCertificate JDBC property #17939

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.

Sign up for GitHub

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

Jump to bottom
Merged
kokosing merged 2 commits into from
Jul 5, 2023
Merged

Add hostnameInCertificate JDBC property #17939

kokosing merged 2 commits into from
Jul 5, 2023

Conversation

@thepetlyon
Copy link
Contributor

@thepetlyon thepetlyon commented Jun 16, 2023 •
edited
Loading

Description

adds hostnameInCertificate connection property for JDBC driver.

Additional context and related issues

Our system requires calling one of multiple separate servers with a different hostname than the final Trino host. For maximum security we still want the benefits of full SSL verification with hostname validation and do not want to use CA validation only.

This property is available in other JDBC drivers such as SqlServer and SAP HANA - documentation linked below.

Release notes

JDBC

  • Allow verification of alternative hostname for Full SSL Verification of JDBC Driver by using hostnameInCertificate property.

@cla-bot cla-bot bot added the cla-signed label Jun 16, 2023
@github-actions github-actions bot added the jdbc Relates to Trino JDBC driver label Jun 16, 2023
@thepetlyon thepetlyon marked this pull request as ready for review June 16, 2023 21:46
@thepetlyon thepetlyon requested review from aalbu and lpoulain June 16, 2023 21:47
@hovaesco hovaesco requested a review from electrum June 22, 2023 08:07
@hovaesco
Copy link
Member

hovaesco commented Jun 22, 2023

Can you add some tests?

@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch 2 times, most recently from 9249c14 to fb9d503 Compare June 22, 2023 18:26
kokosing
kokosing reviewed Jun 22, 2023
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please provide tests?

@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch from fb9d503 to e7d4686 Compare June 22, 2023 20:15
@thepetlyon
Copy link
Contributor Author

thepetlyon commented Jun 22, 2023

Test case added. Addressing other comments!

@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch from e7d4686 to 7f0c6c5 Compare June 22, 2023 20:18
@thepetlyon
Copy link
Contributor Author

thepetlyon commented Jun 22, 2023 •
edited
Loading

Comments addressed!

@thepetlyon thepetlyon changed the title Add support for hostnameInCertificate connection property for alternative hostname verification Add support for hostnameInCertificate property Jun 22, 2023
@thepetlyon thepetlyon requested a review from kokosing June 22, 2023 20:19
@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch from 7f0c6c5 to 9031cf4 Compare June 23, 2023 14:21
@thepetlyon thepetlyon changed the title Add support for hostnameInCertificate property Add hostnameInCertificate JDBC property Jun 23, 2023
kokosing
kokosing reviewed Jun 23, 2023
View reviewed changes
@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch from 9031cf4 to a6bf4fa Compare June 26, 2023 22:34
@thepetlyon thepetlyon requested a review from kokosing June 26, 2023 22:37
@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch from a6bf4fa to 72b3b4d Compare June 27, 2023 02:37
@thepetlyon
Copy link
Contributor Author

thepetlyon commented Jun 27, 2023

Failed check seems to have succeeded.

kokosing
kokosing approved these changes Jun 28, 2023
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

% comments

@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch 3 times, most recently from e5eb9f0 to a9b5730 Compare June 28, 2023 16:26
@thepetlyon
Copy link
Contributor Author

thepetlyon commented Jun 28, 2023 •
edited
Loading

All comments addressed and the one cancelled check shows as succeeding in the logs. Let me know if you have any questions or if we could get an authorized maintainer to merge!

@kokosing
Copy link
Member

kokosing commented Jun 29, 2023

@electrum Do you want to take a look?

kokosing
kokosing approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please suggest the release notes?

client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriverAuth.java Outdated
Copy link
Member

@kokosing kokosing Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you please merge try statements? Same above.

Copy link
Contributor Author

@thepetlyon thepetlyon Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch from a9b5730 to 0de5aa3 Compare June 30, 2023 20:44
@thepetlyon
Copy link
Contributor Author

thepetlyon commented Jun 30, 2023

Can you please suggest the release notes?

Something along the lines of "Allow verification of alternative hostname for Full SSL Verification of JDBC Driver" would be great!

kokosing
kokosing reviewed Jul 1, 2023
View reviewed changes
client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriverAuth.java Outdated
Copy link
Member

@kokosing kokosing Jul 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connection and Statement needs to be closed. By merging try statements I meant:

try (Connection connection = DriverManager.getConnection(url, properties);
      Statement statement = connection.createConnection()) {

I have noticed that this how other tests are implemented in this class. So can you please that other try blocks are also improved in preparatory commit before the main change of your PR?

Copy link
Contributor Author

@thepetlyon thepetlyon Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do!

thepetlyon added 2 commits July 3, 2023 10:53
@thepetlyon
Merge try statements in TestTrinoDriverAuth
32c351e
@thepetlyon
Add hostnameInCertificate JDBC property
3d623b5 
adds hostnameInCertificate connection property for JDBC driver.
@thepetlyon thepetlyon force-pushed the hostnameInCertificate branch from 0de5aa3 to 3d623b5 Compare July 3, 2023 15:54
@kokosing kokosing merged commit bf2ddfe into trinodb:master Jul 5, 2023
@kokosing
Copy link
Member

kokosing commented Jul 5, 2023

Thanks!

@thepetlyon would you like to provide documentation changes for this?

@github-actions github-actions bot added this to the 421 milestone Jul 5, 2023
@thepetlyon
Copy link
Contributor Author

thepetlyon commented Jul 5, 2023

Sure! All that we need to add will be in the Parameter Reference in the JDBC Driver Documentation:

Name Description
hostnameInCertificate (FULL SSLVerification only) Expected hostname in the certificate presented by the Trino server.

@colebow colebow mentioned this pull request Jul 5, 2023
Merged
@mosabua
Copy link
Member

mosabua commented Jul 5, 2023

Agreed @thepetlyon .. please send a PR for the documentation.

@thepetlyon thepetlyon mentioned this pull request Jul 10, 2023
Merged
@thepetlyon
Copy link
Contributor Author

thepetlyon commented Jul 10, 2023

#18218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kokosing kokosing kokosing approved these changes

@aalbu aalbu Awaiting requested review from aalbu

@lpoulain lpoulain Awaiting requested review from lpoulain

@electrum electrum Awaiting requested review from electrum

Assignees

No one assigned

Labels

cla-signed jdbc Relates to Trino JDBC driver

Milestone

421

Development

Successfully merging this pull request may close these issues.

4 participants

@thepetlyon @hovaesco @kokosing @mosabua