Skip to content
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 Support for non default keystore and truststore type in presto CLI and JDBC #22556

Merged
merged 1 commit into from May 3, 2024

Conversation

evanvdia
Copy link
Contributor

@evanvdia evanvdia commented Apr 18, 2024

Description

Added Support for non default keystore and truststore type in presto CLI and JDBC.

Motivation and Context

Issue : #22555

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
    * Add support for non default keystore and truststore type.

@steveburnett
Copy link
Contributor

Suggest changing title, Description, release note entry, and commit message from "keystroke" to "keystore", based on the edited files and on the Desription in Issue 22555.

@evanvdia evanvdia changed the title Add Support for non default keystroke and truststore type in presto CLI and JDBC Add Support for non default keystore and truststore type in presto CLI and JDBC Apr 19, 2024
@evanvdia
Copy link
Contributor Author

@steveburnett changes done

@@ -178,7 +180,7 @@ public static void setupSsl(
// load TrustStore if configured, otherwise use KeyStore
KeyStore trustStore = keyStore;
if (trustStorePath.isPresent()) {
trustStore = loadTrustStore(new File(trustStorePath.get()), trustStorePassword);
trustStore = loadTrustStore(new File(trustStorePath.get()), trustStorePassword, trustStoreType.get());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a validation to ensure that these parameters are present before getting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have set default values in ClientOptions.java

image image

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood, but that's in a different module. Best practice is to either fail, or only pass in String (and check not null).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@evanvdia evanvdia force-pushed the jdbc_fips_compliant_mode branch 2 times, most recently from c8610cf to 3c67586 Compare April 23, 2024 12:20
@tdcmeehan
Copy link
Contributor

Looks good, but the commit message is too long (see contributing guide). Perhaps just mention Add Support for non default keystroke and truststore type

@evanvdia
Copy link
Contributor Author

Changed commit message.

@evanvdia
Copy link
Contributor Author

evanvdia commented May 3, 2024

@tdcmeehan Review comments are incorporated. Could you please review.

@tdcmeehan tdcmeehan merged commit cc0726e into prestodb:master May 3, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants