GH-39014: [Java] Add default truststore along with KeychainStore when on Mac system #39235

ravjotbrar · 2023-12-14T19:48:55Z

Rationale for this change

As described in #39014, when using the system TrustStore on Mac, the certificates returned do not include Root CAs trusted by the system. This change adds the default KeyStore instance along with the KeyChainStore to include trusted Root CAs. The reason we add the default KeyStore instance is because there is no easy way to get the certificates from the System Roots keychain.

What changes are included in this PR?

I've updated ClientAuthenticationUtils to get the default KeyStore instance when the operating system is macOS and have updated the tests to include this change.

Are these changes tested?

See changes made in ClientAuthenticationUtilsTest.java.

Are there any user-facing changes?

No

Closes: [Java][FlightSQL] useSystemTrustStore on MacOS returns user's keychain instead of System Roots keychain #39014

github-actions · 2023-12-14T19:49:22Z

⚠️ GitHub issue #39014 has been automatically assigned in GitHub to PR creator.

davisusanibar · 2023-12-14T23:13:32Z

...-core/src/main/java/org/apache/arrow/driver/jdbc/client/utils/ClientAuthenticationUtils.java

@@ -156,16 +166,9 @@ public static InputStream getCertificateInputStreamFromSystem(String password) t
      keyStoreList.add(getKeyStoreInstance("Windows-MY"));
    } else if (isMac()) {
      keyStoreList.add(getKeyStoreInstance("KeychainStore"));


Is it not necessary to delete this method now?

I considered it, but I don't see the harm in keeping it, especially if users are now importing certificates into their user keychain as a workaround.

Okay, thanks for letting me know.

Not a big deal to keep it for now imho.

davisusanibar · 2023-12-14T23:37:42Z

...-core/src/main/java/org/apache/arrow/driver/jdbc/client/utils/ClientAuthenticationUtils.java

+      throws KeyStoreException, CertificateException, NoSuchAlgorithmException, IOException {
+    try (InputStream fileInputStream = getKeystoreInputStream()) {
+      KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+      keyStore.load(fileInputStream, password == null ? null : password.toCharArray());


Does it need to load all cacerts files? Is there a way to restrict files needed to be loaded based on security concerns?

Could you elaborate on the security concerns from loading all the cacerts files?

Based on the current user workaround, there is no more question for my side #39235 (comment)

It looks good to me. Maybe it would be better to name this method geetDefaultTrustStore() but that's not a big deal.

jbonofre · 2023-12-15T04:34:48Z

Thanks for the PR. I'm doing a pass as well 😄

jbonofre · 2023-12-21T06:34:06Z

...-core/src/main/java/org/apache/arrow/driver/jdbc/client/utils/ClientAuthenticationUtils.java

+      throws KeyStoreException, CertificateException, NoSuchAlgorithmException, IOException {
+    try (InputStream fileInputStream = getKeystoreInputStream()) {
+      KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+      keyStore.load(fileInputStream, password == null ? null : password.toCharArray());


It looks good to me. Maybe it would be better to name this method geetDefaultTrustStore() but that's not a big deal.

jbonofre · 2023-12-21T06:34:38Z

...-core/src/main/java/org/apache/arrow/driver/jdbc/client/utils/ClientAuthenticationUtils.java

@@ -156,16 +166,9 @@ public static InputStream getCertificateInputStreamFromSystem(String password) t
      keyStoreList.add(getKeyStoreInstance("Windows-MY"));
    } else if (isMac()) {
      keyStoreList.add(getKeyStoreInstance("KeychainStore"));


Not a big deal to keep it for now imho.

lidavidm · 2023-12-21T13:49:13Z

...-core/src/main/java/org/apache/arrow/driver/jdbc/client/utils/ClientAuthenticationUtils.java

@@ -156,16 +166,9 @@ public static InputStream getCertificateInputStreamFromSystem(String password) t
      keyStoreList.add(getKeyStoreInstance("Windows-MY"));
    } else if (isMac()) {
      keyStoreList.add(getKeyStoreInstance("KeychainStore"));
+      keyStoreList.add(getDefaultKeyStoreInstance(password));


Does the order matter? Should the system key store come first?

It appears order does not matter, given that pemWriter writes data for all keystores added to the keystore list. I also did a manual test that required the default keystore and it passed successfully.

conbench-apache-arrow · 2023-12-22T21:44:44Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit cd5a1bd.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…e when on Mac system (apache#39235) ### Rationale for this change As described in apache#39014, when using the system TrustStore on Mac, the certificates returned do not include Root CAs trusted by the system. This change adds the default KeyStore instance along with the KeyChainStore to include trusted Root CAs. The reason we add the default KeyStore instance is because there is no easy way to get the certificates from the System Roots keychain. ### What changes are included in this PR? I've updated ClientAuthenticationUtils to get the default KeyStore instance when the operating system is macOS and have updated the tests to include this change. ### Are these changes tested? See changes made in ClientAuthenticationUtilsTest.java. ### Are there any user-facing changes? No * Closes: apache#39014 Authored-by: Ravjot Brar <ravjot.brar@dremio.com> Signed-off-by: David Li <li.davidm96@gmail.com>

ravjotbrar added 3 commits December 14, 2023 11:21

Add default keystore when on Mac

a4a9671

Fix spacing

8ed561a

Fix test

71dbfd9

ravjotbrar requested a review from lidavidm as a code owner December 14, 2023 19:48

github-actions bot added Component: Java awaiting review Awaiting review labels Dec 14, 2023

davisusanibar reviewed Dec 14, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 14, 2023

davisusanibar reviewed Dec 14, 2023

View reviewed changes

davisusanibar approved these changes Dec 21, 2023

View reviewed changes

jbonofre approved these changes Dec 21, 2023

View reviewed changes

lidavidm approved these changes Dec 21, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge awaiting committer review Awaiting committer review awaiting review Awaiting review and removed awaiting committer review Awaiting committer review awaiting merge Awaiting merge awaiting review Awaiting review labels Dec 21, 2023

lidavidm merged commit cd5a1bd into apache:main Dec 22, 2023
25 of 26 checks passed

lidavidm removed the awaiting committer review Awaiting committer review label Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39014: [Java] Add default truststore along with KeychainStore when on Mac system #39235

GH-39014: [Java] Add default truststore along with KeychainStore when on Mac system #39235

ravjotbrar commented Dec 14, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Dec 14, 2023

davisusanibar Dec 14, 2023

ravjotbrar Dec 18, 2023

davisusanibar Dec 21, 2023

jbonofre Dec 21, 2023

davisusanibar Dec 14, 2023

ravjotbrar Dec 18, 2023

davisusanibar Dec 21, 2023

jbonofre Dec 21, 2023

jbonofre commented Dec 15, 2023

jbonofre Dec 21, 2023

jbonofre Dec 21, 2023

lidavidm Dec 21, 2023

ravjotbrar Dec 21, 2023

conbench-apache-arrow bot commented Dec 22, 2023

GH-39014: [Java] Add default truststore along with KeychainStore when on Mac system #39235

GH-39014: [Java] Add default truststore along with KeychainStore when on Mac system #39235

Conversation

ravjotbrar commented Dec 14, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbonofre commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Dec 22, 2023

ravjotbrar commented Dec 14, 2023 •

edited by github-actions bot

Loading