SNOW-1821504: [JDBC] Initialal OCSP deprecation plan steps #2008

sfc-gh-ext-simba-vb · 2024-12-17T01:50:10Z

Overview

SNOW-1821504

Pre-review self checklist

PR branch is updated with all the changes from master branch
The code is correctly formatted (run mvn -P check-style validate)
New public API is not unnecessary exposed (run mvn verify and inspect target/japicmp/japicmp.html)
The pull request name is prefixed with SNOW-XXXX:
Code is in compliance with internal logging requirements

External contributors - please answer these questions before submitting a pull request. Thanks!

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

Issue: #NNNN
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
- 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.

…into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

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

…on-plan-steps

sfc-gh-pfus · 2024-12-18T07:30:43Z

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

    Boolean insecureMode = (Boolean) connectionPropertiesMap.get(SFSessionProperty.INSECURE_MODE);
-    if (insecureMode != null && insecureMode) {
+    if ((disableOCSPMode != null && disableOCSPMode) || (insecureMode != null && insecureMode)) {


I don't think it works correctly in a case when someone deliberately specified disableOCSPMode = false, insecureMode = true, because the first && will evaluate to false and we evaluate the second && to true - and we shouldn't.

this case should not happen - it's miss configuration
I think we can throw the exception when both disableOCSPMode and insecureMode are not null and are not equal

guys didn't we call this disableOCSPChecks in other drivers, and in the requirements?

The property name is disableOCSPChecks in Connection Properties. disableOCSPMode was just a variable name in the method. I will change it to disableOCSPChecks all over the place.

got it, no need to change if it's just an internal variable name. i wanted to ensure disableOCSPChecks naming is consistent across all drivers

sfc-gh-pfus · 2024-12-18T07:31:54Z

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

-        + "as it could not obtain a valid OCSP Response to use from the CA OCSP "
-        + "responder. Details: \n"
+    return "OCSP responder didn't respond correctly. Assuming certificate is "
+        + "not revoked. Details: \n"


Do we need this newline? It's not good to add newlines in logs as it breaks log gathering by some tools.

sfc-gh-pfus · 2024-12-18T07:33:07Z

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

@@ -1014,6 +1014,27 @@ public void testFailOverOrgAccount() throws SQLException {
    }
  }

+  /** Test production connectivity with disableOCSPChecksMode enabled. */
+  @Test
+  public void testDisableOCSPChecksMode() throws SQLException {


Can we have more tests on various combinations of disableOCSPChecks and insecureMode? Or maybe this is not a good place, it should be checked in a place where connection string is parsed?

sfc-gh-pfus · 2024-12-18T07:33:55Z

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

+    properties.put("account", "fakeaccount");
+    try {
+      DriverManager.getConnection(deploymentUrl, properties);
+      fail();


We have junit5, we can check something like this:

SQLException e = assertThrows(() -> DriverManager.getConnection(deploymentUrl, properties)) assertThat(e.getErrorCode()....

sfc-gh-dprzybysz · 2024-12-18T09:21:05Z

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

    Boolean insecureMode = (Boolean) connectionPropertiesMap.get(SFSessionProperty.INSECURE_MODE);
-    if (insecureMode != null && insecureMode) {
+    if ((disableOCSPMode != null && disableOCSPMode) || (insecureMode != null && insecureMode)) {


this case should not happen - it's miss configuration
I think we can throw the exception when both disableOCSPMode and insecureMode are not null and are not equal

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

…into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

…' of https://github.com/snowflakedb/snowflake-jdbc into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

sfc-gh-ext-simba-vb · 2024-12-19T03:31:12Z

Hi @sfc-gh-pfus, code review comments implemented.

sfc-gh-dszmolka · 2024-12-19T05:17:58Z

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

-      // A custom TrustManager is required only if insecureMode is disabled,
-      // which is by default in the production. insecureMode can be enabled
+    if (key != null && key.getOcspMode() != OCSPMode.DISABLE_OCSP_CHECKS) {
+      // A custom TrustManager is required only if disableOCSPMode is disabled,


disableOCSPChecks isn't it ? here and all the other places

sfc-gh-dszmolka · 2024-12-19T05:18:46Z

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

    Boolean insecureMode = (Boolean) connectionPropertiesMap.get(SFSessionProperty.INSECURE_MODE);
-    if (insecureMode != null && insecureMode) {
+
+    if ((disableOCSPMode != null && insecureMode != null) && (disableOCSPMode != insecureMode)) {


disableOCSPChecks is the flag name, in other drivers too

sfc-gh-pfus · 2024-12-19T06:11:35Z

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

+        && (disableOCSPChecks != insecureMode)) {
+      logger.error(
+          "The values for 'disableOCSPChecks' and 'insecureMode' must be identical. "
+              + "Please ensure both properties are set to the same value.");


I'd add "or unset insecureMode".

sfc-gh-pfus · 2024-12-19T06:13:06Z

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

@@ -440,6 +445,49 @@ public void testWrongHost() throws InterruptedException {
    fail("All retries failed");
  }

+  /** Test connectivity with disableOCSPChecksMode and insecure mode enabled. */
+  @Test
+  public void testDisableOCSPChecksModeAndInsecureMode() throws SQLException {


Also worth having tests for disableOCSPChecks only and insecureMode only.

These tests are already added in ConnectionIT class. I shifted them to same class.

…on-plan-steps

…' of https://github.com/snowflakedb/snowflake-jdbc into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

sfc-gh-pfus · 2024-12-20T06:07:57Z

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

+            });
+
+    assertThat(
+        thrown.getErrorCode(), anyOf(is(INVALID_CONNECTION_INFO_CODE), is(BAD_REQUEST_GS_CODE)));


What is the purpose of this assertion? It only tests options mismatch, not the effect. We could make a typo in config and it wouldn't end up in the same exception, right?

The test name for option mismatch is testDisableOCSPChecksModeAndInsecureModeMismatched.
This above test testDisableOCSPChecksModeAndInsecureMode is a good case where both options are enabled. It will skip the OCSP check and try to connect to db, but it ends up with an exception as the user and account are fake.
For testDisableOCSPChecksModeAndInsecureModeMismatched test, it will get the new error code 200064.

…on-plan-steps

sfc-gh-ext-simba-vb added 4 commits December 9, 2024 18:15

FAIL_OPEN log changed (message + log level)

c4fdb4c

Merge branch 'master' of https://github.com/snowflakedb/snowflake-jdbc …

e5d2224

…into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

Merge branch 'master' of https://github.com/snowflakedb/snowflake-jdbc …

c9607ee

…into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

disableOCSPChecks property added.

4d66099

sfc-gh-ext-simba-jf reviewed Dec 17, 2024

View reviewed changes

src/test/java/net/snowflake/client/jdbc/ConnectionIT.java Outdated Show resolved Hide resolved

sfc-gh-ext-simba-vb and others added 3 commits December 17, 2024 14:28

Testcase changes.

1967502

Correcting the checklist.

f26374e

Merge branch 'master' into SNOW-1821504-jdbc-initialal-ocsp-deprecati…

47ab1dc

…on-plan-steps

sfc-gh-ext-simba-vb marked this pull request as ready for review December 17, 2024 22:45

sfc-gh-ext-simba-vb requested a review from a team as a code owner December 17, 2024 22:45

sfc-gh-ext-simba-vb requested review from sfc-gh-dprzybysz and sfc-gh-pbulawa December 17, 2024 22:46

sfc-gh-pfus reviewed Dec 18, 2024

View reviewed changes

sfc-gh-dprzybysz reviewed Dec 18, 2024

View reviewed changes

sfc-gh-ext-simba-vb added 4 commits December 18, 2024 19:23

Merge branch 'master' of https://github.com/snowflakedb/snowflake-jdbc …

b244875

…into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

Review comments incorporated.

a5237da

Merge branch 'SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps…

e0055ca

…' of https://github.com/snowflakedb/snowflake-jdbc into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

Check Style correction.

20a9483

sfc-gh-dszmolka reviewed Dec 19, 2024

View reviewed changes

Review comments implemented.

b76683b

sfc-gh-pfus reviewed Dec 19, 2024

View reviewed changes

sfc-gh-ext-simba-vb and others added 3 commits December 19, 2024 12:06

Merge branch 'master' into SNOW-1821504-jdbc-initialal-ocsp-deprecati…

c8e578e

…on-plan-steps

Fixed tests failure on old driver.

28bbf9d

Merge branch 'SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps…

1dff792

…' of https://github.com/snowflakedb/snowflake-jdbc into SNOW-1821504-jdbc-initialal-ocsp-deprecation-plan-steps

sfc-gh-pfus reviewed Dec 20, 2024

View reviewed changes

Merge branch 'master' into SNOW-1821504-jdbc-initialal-ocsp-deprecati…

829b5c6

…on-plan-steps

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1821504: [JDBC] Initialal OCSP deprecation plan steps #2008

SNOW-1821504: [JDBC] Initialal OCSP deprecation plan steps #2008

sfc-gh-ext-simba-vb commented Dec 17, 2024

sfc-gh-pfus Dec 18, 2024

sfc-gh-dprzybysz Dec 18, 2024

sfc-gh-dszmolka Dec 19, 2024

sfc-gh-ext-simba-vb Dec 19, 2024

sfc-gh-dszmolka Dec 19, 2024

sfc-gh-pfus Dec 18, 2024

sfc-gh-pfus Dec 18, 2024

sfc-gh-pfus Dec 18, 2024

sfc-gh-dprzybysz Dec 18, 2024

sfc-gh-ext-simba-vb commented Dec 19, 2024

sfc-gh-dszmolka Dec 19, 2024 •

edited

Loading

sfc-gh-dszmolka Dec 19, 2024

sfc-gh-pfus Dec 19, 2024

sfc-gh-pfus Dec 19, 2024

sfc-gh-ext-simba-vb Dec 19, 2024 •

edited

Loading

sfc-gh-pfus Dec 20, 2024

sfc-gh-ext-simba-vb Dec 20, 2024

SNOW-1821504: [JDBC] Initialal OCSP deprecation plan steps #2008

Are you sure you want to change the base?

SNOW-1821504: [JDBC] Initialal OCSP deprecation plan steps #2008

Conversation

sfc-gh-ext-simba-vb commented Dec 17, 2024

Overview

Pre-review self checklist

External contributors - please answer these questions before submitting a pull request. Thanks!

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

Choose a reason for hiding this comment

sfc-gh-ext-simba-vb commented Dec 19, 2024

sfc-gh-dszmolka Dec 19, 2024 • edited Loading

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-ext-simba-vb Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-dszmolka Dec 19, 2024 •

edited

Loading

sfc-gh-ext-simba-vb Dec 19, 2024 •

edited

Loading