SNOW-618478: support passing private key as base64 encoded string (private_key_base64) as alternative to (private_key_file) #1810

ets · 2024-07-02T16:10:04Z

Overview

What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

SNOW-618478: Unable to authenticate with a private key by using spring datasource properties #1053
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
- [] I am adding new logging messages
  - None were added but a few log messages were tweaked for clarity
- I am modifying authorization mechanisms
- I am adding new credentials
- I am modifying OCSP code
- I am adding a new dependency or upgrading an existing one
- I am adding new public/protected component not marked with @SnowflakeJdbcInternalApi (note that public/protected methods/fields in classes marked with this annotation are already internal)
Please describe how your code solves the related issue.

Please write a short description of how your code change solves the related issue.

There are currently only two options for passing a privateKey for authentication purposes. The key can be supplied as a fully instantiated PrivateKey object or a path to a private key file can be supplied and the PrivateKey object will be extracted. Neither option supports the usecase where the calling code needs to pass the privateKey as a String.

As the GitHub issue documents, there are several scenarios where passing the privateKey as a String is required (Hikari pool only accepts property values of type String, Spark.read if you don't want to push the file to every executor, etc..). This PR adds a new session property "private_key_base64" which complements the existing "private_key_file". The new logic ensures that only one of private_key_base64, private_key_file, or private_key are not null. Then it extracts, decrypts, and instantiates the needed private_key as appropriate.

The PR piggy-backs upon the existing tests for private_key_file and adds variants that instead leverage private_key_base64 to demonstrate expected behavior.

github-actions · 2024-07-02T16:10:19Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

ets · 2024-07-02T16:20:11Z

I have read the CLA Document and I hereby sign the CLA

ets · 2024-07-02T16:48:02Z

recheck

src/main/java/net/snowflake/client/core/SessionUtil.java

src/main/java/net/snowflake/client/core/SessionUtilKeyPair.java

src/main/java/net/snowflake/client/jdbc/SnowflakeBasicDataSource.java

sfc-gh-dprzybysz · 2024-07-05T09:58:25Z

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

+            + privateKeyBase64;
+
+    try (Connection connection = DriverManager.getConnection(uri, properties)) {}
+    // test with incorrect password for private key


let's make it a separate test - the same also in other sections below

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

sfc-gh-dprzybysz · 2024-07-05T09:59:36Z

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

+    }
+
+    // test with invalid public/private key combo (using 1st public key with 2nd private key)
+    try (Connection connection = getConnection();


can we create a method for adding public key and reuse it?

sfc-gh-dprzybysz · 2024-07-05T11:12:11Z

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

+    }
+  }
+
+  // This will only work with JDBC driver versions higher than 3.15.1


it will work in version higher than 3.17.0 probably

Not my insight... this comment exists above testPrivateKeyInConnectionStringWithBouncyCastle and if it does apply there then it also should apply here.

Still we wants to have LatestIT tests marked with comment with version

sfc-gh-dprzybysz · 2024-07-05T11:12:48Z

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

@@ -891,6 +950,146 @@ public void testPrivateKeyInConnectionStringWithBouncyCastle() throws SQLExcepti
    testPrivateKeyInConnectionString();
  }

+  @Test


please add a comment about version when the test will work > 3.17.0 probably

Not clear on the ask here. I assume the version limitation is based upon historical support for BouncyCastle - right?

System.setProperty(SecurityUtil.ENABLE_BOUNCYCASTLE_PROVIDER_JVM, "true");

So if that was introduced in 3.15.1 as the existing comment on testPrivateKeyInConnectionStringWithBouncyCastle suggests..then it should apply equally to the new testPrivateKeyBase64InConnectionStringWithBouncyCastle

Right?

On each our test added to classes ...LatestIT we add comment to mark from which version this test is correct. Currently the last released version is 3.17.0 so above newly added test that use new connection property you added there should be comment e.g. in other tests

ets · 2024-07-05T16:53:18Z

Hey @sfc-gh-dprzybysz thanks for reviewing this one. I knocked out most of your asks but see my feedback on the proposed changes to ConnectionLatestIt where I've introduced a couple test methods that are carbon copies of testPrivatekKeyFileXXX equivalents. I'm thinking we should keep those more or less in sync with respect to design - agreed?

josecsotomorales · 2024-07-08T13:15:18Z

Hey SF folks, any timeline of when this PR will be merged and released? This currently blocks several users of Spark and Hikari from using Key Pair Auth properly.

sfc-gh-dprzybysz · 2024-07-19T12:49:19Z

src/main/java/net/snowflake/client/core/SessionUtil.java

+  }
+
+  /**
+   * Helper function to generate a JWT token


please add link in javadoc to function above

sfc-gh-dprzybysz · 2024-07-19T12:53:55Z

src/test/java/net/snowflake/client/core/SessionUtilLatestIT.java

-import java.util.Map;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.*;


it should still fail on checkstyle, please run mvn -P check-style validate

sfc-gh-dprzybysz · 2024-07-19T12:56:27Z

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

-import java.util.List;
-import java.util.Map;
-import java.util.Properties;
+import java.util.*;


here also the checkstyle check will fail

sfc-gh-dprzybysz · 2024-07-19T13:00:27Z

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

@@ -891,6 +950,146 @@ public void testPrivateKeyInConnectionStringWithBouncyCastle() throws SQLExcepti
    testPrivateKeyInConnectionString();
  }

+  @Test


On each our test added to classes ...LatestIT we add comment to mark from which version this test is correct. Currently the last released version is 3.17.0 so above newly added test that use new connection property you added there should be comment e.g. in other tests

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

sfc-gh-dprzybysz · 2024-07-19T13:03:05Z

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

+    properties.put("port", parameters.get("port"));
+    try (Connection connection = DriverManager.getConnection(uri, properties)) {}
+
+    // PKCS #1


@ets could you split the test into smaller ones?

sfc-gh-dprzybysz · 2024-07-19T13:04:53Z

src/test/java/net/snowflake/client/jdbc/ConnectionLatestIT.java

+    }
+  }
+
+  // This will only work with JDBC driver versions higher than 3.15.1


Still we wants to have LatestIT tests marked with comment with version

sfc-gh-dprzybysz · 2024-07-19T13:06:19Z

@ets Please also rebase your change when you will be addressing the comments

ets · 2024-08-09T19:34:07Z

@sfc-gh-dprzybysz I see that you got to the open items here before me. Are there any remaining that you'd like me to look into or is this PR ready for next steps now?

…ew session property key "private_key_base64" Rename private key password variable for clarity Add deprecation to private key file password

sfc-gh-mkubik · 2024-09-03T10:00:23Z

Merged with #1847

ets requested a review from a team as a code owner July 2, 2024 16:10

sfc-gh-dprzybysz reviewed Jul 5, 2024

View reviewed changes

sfc-gh-dprzybysz reviewed Jul 19, 2024

View reviewed changes

sfc-gh-dprzybysz mentioned this pull request Jul 30, 2024

SNOW-618478: Support private key base64 in connection parameters #1847

Merged

5 tasks

Support accepting a private key as a base64 encoded string with the n…

dd04f7d

…ew session property key "private_key_base64" Rename private key password variable for clarity Add deprecation to private key file password

ets force-pushed the master branch from 53014a4 to dd04f7d Compare August 10, 2024 20:38

sfc-gh-mkubik closed this Sep 3, 2024

github-actions bot locked and limited conversation to collaborators Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-618478: support passing private key as base64 encoded string (private_key_base64) as alternative to (private_key_file) #1810

SNOW-618478: support passing private key as base64 encoded string (private_key_base64) as alternative to (private_key_file) #1810

ets commented Jul 2, 2024 •

edited by sfc-gh-dprzybysz

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

ets commented Jul 2, 2024

ets commented Jul 2, 2024

sfc-gh-dprzybysz Jul 5, 2024

sfc-gh-dprzybysz Jul 5, 2024

sfc-gh-dprzybysz Jul 5, 2024

ets Jul 5, 2024

sfc-gh-dprzybysz Jul 19, 2024

sfc-gh-dprzybysz Jul 5, 2024

ets Jul 5, 2024

sfc-gh-dprzybysz Jul 19, 2024

ets commented Jul 5, 2024

josecsotomorales commented Jul 8, 2024

sfc-gh-dprzybysz Jul 19, 2024

sfc-gh-dprzybysz Jul 19, 2024

sfc-gh-dprzybysz Jul 19, 2024

sfc-gh-dprzybysz Jul 19, 2024

sfc-gh-dprzybysz Jul 19, 2024

sfc-gh-dprzybysz Jul 19, 2024

sfc-gh-dprzybysz commented Jul 19, 2024

ets commented Aug 9, 2024

sfc-gh-mkubik commented Sep 3, 2024

SNOW-618478: support passing private key as base64 encoded string (private_key_base64) as alternative to (private_key_file) #1810

SNOW-618478: support passing private key as base64 encoded string (private_key_base64) as alternative to (private_key_file) #1810

Conversation

ets commented Jul 2, 2024 • edited by sfc-gh-dprzybysz Loading

Overview

github-actions bot commented Jul 2, 2024 • edited Loading

ets commented Jul 2, 2024

ets commented Jul 2, 2024

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

ets commented Jul 5, 2024

josecsotomorales commented Jul 8, 2024

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

sfc-gh-dprzybysz commented Jul 19, 2024

ets commented Aug 9, 2024

sfc-gh-mkubik commented Sep 3, 2024

ets commented Jul 2, 2024 •

edited by sfc-gh-dprzybysz

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading