SNOW-1825607 Initial OCSP deprecation plan steps #973

sfc-gh-ext-simba-jy · 2024-11-25T19:23:37Z

Description

Please explain the changes you made here.

FAIL_OPEN log changed (message + log level)?
FAIL_OPEN passing request along if responder URL is missing?
[] FAIL_CLOSED - no NPE if responder URL is missing? Please check the message below about this.
Retries and timeouts confirmed (both to responder and cache server)?
disableOCSPChecks introduced?
- Deprecated insecureMode?
- disableOCSPChecks has precedence?

Checklist

Format code according to the existing code style (run npm run lint:check -- CHANGED_FILES and fix problems in changed code)
Create tests which fail without the change (if possible)
Make all tests (unit and integration) pass (npm run test:unit and npm run test:integration)
Extend the README / documentation and ensure is properly displayed (if necessary)
Provide JIRA issue id (if possible) or GitHub issue id in commit message

sfc-gh-ext-simba-jy · 2024-11-26T00:20:13Z

It seems like the node.js driver does not have the use case that the responder URL does not exist according to the code:

snowflake-connector-nodejs/lib/agent/check.js

Line 176 in bada542

    
           let parsedUrl = require('url').parse(process.env.SF_OCSP_RESPONSE_CACHE_SERVER_URL);

. As we have the default OCSP_RESPONSE_CACHE_SERVER_URL and there will be no error for the responder URL, this will not throw the error like JDBC.

codecov · 2024-11-26T00:44:59Z

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.91%. Comparing base (bada542) to head (6fc7ddc).

Files with missing lines	Patch %	Lines
lib/core.js	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #973      +/-   ##
==========================================
- Coverage   88.93%   88.91%   -0.02%     
==========================================
  Files          69       69              
  Lines        6748     6749       +1     
==========================================
  Hits         6001     6001              
- Misses        747      748       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sfc-gh-pfus · 2024-11-26T06:25:27Z

lib/agent/socket_util.js

@@ -120,7 +115,7 @@ exports.secureSocket = function (socket, host, agent, mock) {
 function isOcspValidationDisabled(host) {
  // ocsp is disabled if insecure-connect is enabled, or if we've disabled ocsp
  // for non-snowflake endpoints and the host is a non-snowflake endpoint
-  return GlobalConfig.isInsecureConnect() ||
+  return GlobalConfig.isOCSPChecksDisabled() ||


Does this cover also a case when someone sets insecureMode?

sfc-gh-pfus · 2024-11-26T06:25:45Z

lib/agent/socket_util.js

@@ -158,7 +153,7 @@ function canEarlyExitForOCSP(errors) {
      const err = errors[errorIndex];
      if (err && !isValidOCSPError(err)) {
        // any of the errors is NOT good/revoked/unknown
-        Logger.getInstance().warn(ocspFailOpenWarning + err);
+        Logger.getInstance().debug('OCSP responder didn\'t respond correctly. Assuming certificate is not revoked');


Can we leave details as it was before?

sfc-gh-pfus · 2024-11-26T06:26:51Z

lib/core.js

-        const insecureConnect = options.insecureConnect;
-        if (Util.exists(insecureConnect)) {
+        if (Util.exists(options.insecureConnect)) {
+          Logger.getInstance().warn('Warning! The option insecureConnect will be deprecated. Please use the disableOCSPChecks.');


Actually it is deprecated, not will be :)

sfc-gh-pfus · 2024-11-26T06:27:36Z

lib/core.js

+          Logger.getInstance().warn('Warning! The option insecureConnect will be deprecated. Please use the disableOCSPChecks.');
+        }
+
+        const disableOCSPChecks = options.insecureConnect || options.disableOCSPChecks;


Are this values nullable? If yes and both are not null, disableOCSPChecks should have precedence, not the alternative.

sfc-gh-pfus · 2024-11-26T06:29:03Z

test/unit/ocsp/test_unit_ocsp_mode.js

@@ -9,13 +9,13 @@ const assert = require('assert');
 describe('OCSP mode', function () {
  it('getOcspMode', function (done) {
    // insecure mode
-    GlobalConfig.setInsecureConnect(true);
+    GlobalConfig.setDisableOCSPChecks(true);


Can we have also a test confirming that insecureMode still works?

sfc-gh-pfus · 2024-11-26T06:29:25Z

test/unit/snowflake_config_test.js

@@ -15,7 +15,7 @@ describe('Snowflake Configure Tests', function () {
  before(function () {
    originalConfig = {
      logLevel: Logger.getInstance().getLevelTag(),
-      insecureConnect: GlobalConfig.isInsecureConnect(),
+      disableOCSPChecks: GlobalConfig.isOCSPChecksDisabled(),


Same here + test for setting both values at once.

sfc-gh-pfus · 2024-11-26T06:57:02Z

It seems like the node.js driver does not have the use case that the responder URL does not exist according to the code:

snowflake-connector-nodejs/lib/agent/check.js

Line 176 in bada542

let parsedUrl = require('url').parse(process.env.SF_OCSP_RESPONSE_CACHE_SERVER_URL);

. As we have the default OCSP_RESPONSE_CACHE_SERVER_URL and there will be no error for the responder URL, this will not throw the error like JDBC.

What you mentioned is not a responder URL - it is a cache server URL. Responder URL is provided in each certificate.

sfc-gh-dszmolka · 2024-11-26T09:21:00Z

It seems like the node.js driver does not have the use case that the responder URL does not exist according to the code:

snowflake-connector-nodejs/lib/agent/check.js

Line 176 in bada542

let parsedUrl = require('url').parse(process.env.SF_OCSP_RESPONSE_CACHE_SERVER_URL);

. As we have the default OCSP_RESPONSE_CACHE_SERVER_URL and there will be no error for the responder URL, this will not throw the error like JDBC.

OCSP Responder url != OCSP Cache URL. The (default) OCSP Cache URL is always the same, by default, ocsp.snowflakecomputing.com (gracefully ignoring privatelink situations here)

However OCSP Responder URL is not from Snowflake. It is always hardcoded into the particular certificate, which is under verification. See this random cert as an example: https://crt.sh/?sha256=9c3f2fd11c57d7c649ad5a0932c0f0d29756f6a0a1c74c43e1e89a62d64cd320

            Authority Information Access: 
                CA Issuers - URI:http://i.pki.goog/r4.crt

It does not have OCSP endpoint. The driver today will complain if it encounters such certs which doesn't have the OCSP endpoint, because the driver expects the cert to have it. See this issue what exactly happens: #932

As a comparison, here's a cert which does have OCSP endpoint: https://crt.sh/?id=12092745633
Observe:

            Authority Information Access: 
>>>>            OCSP - URI:http://ocsp.r2m03.amazontrust.com
                CA Issuers - URI:http://crt.r2m03.amazontrust.com/r2m03.cer

(this section is missing from the first cert)

The goal here, is for the driver if encountering a cert without the said OCSP endpoint, then it should just log the debug-level message and gracefully continue as if the verification was successful.
Implementing this change will also fix 932.

edit: I see Piotr also pointed the same out while I was typing this message :)

testing

0e08ba9

sfc-gh-ext-simba-jy requested a review from a team as a code owner November 25, 2024 19:23

sfc-gh-ext-simba-jy changed the title ~~testing~~ SNOW-1825607 Nov 25, 2024

sfc-gh-ext-simba-jy marked this pull request as draft November 25, 2024 19:24

fix error

6fc7ddc

sfc-gh-ext-simba-jy marked this pull request as ready for review November 26, 2024 00:20

sfc-gh-ext-simba-jy changed the title ~~SNOW-1825607~~ SNOW-1825607 Initialal OCSP deprecation plan steps Nov 26, 2024

sfc-gh-ext-simba-jy requested review from sfc-gh-dprzybysz and sfc-gh-pfus November 26, 2024 01:58

sfc-gh-pfus reviewed Nov 26, 2024

View reviewed changes

sfc-gh-dszmolka changed the title ~~SNOW-1825607 Initialal OCSP deprecation plan steps~~ SNOW-1825607 Initial OCSP deprecation plan steps Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1825607 Initial OCSP deprecation plan steps #973

SNOW-1825607 Initial OCSP deprecation plan steps #973

sfc-gh-ext-simba-jy commented Nov 25, 2024 •

edited

Loading

sfc-gh-ext-simba-jy commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024

sfc-gh-pfus Nov 26, 2024

sfc-gh-pfus Nov 26, 2024

sfc-gh-pfus Nov 26, 2024

sfc-gh-pfus Nov 26, 2024

sfc-gh-pfus Nov 26, 2024

sfc-gh-pfus Nov 26, 2024

sfc-gh-pfus commented Nov 26, 2024

sfc-gh-dszmolka commented Nov 26, 2024 •

edited

Loading

SNOW-1825607 Initial OCSP deprecation plan steps #973

Are you sure you want to change the base?

SNOW-1825607 Initial OCSP deprecation plan steps #973

Conversation

sfc-gh-ext-simba-jy commented Nov 25, 2024 • edited Loading

Description

Checklist

sfc-gh-ext-simba-jy commented Nov 26, 2024 • edited Loading

codecov bot commented Nov 26, 2024

Codecov Report

sfc-gh-pfus Nov 26, 2024

Choose a reason for hiding this comment

sfc-gh-pfus Nov 26, 2024

Choose a reason for hiding this comment

sfc-gh-pfus Nov 26, 2024

Choose a reason for hiding this comment

sfc-gh-pfus Nov 26, 2024

Choose a reason for hiding this comment

sfc-gh-pfus Nov 26, 2024

Choose a reason for hiding this comment

sfc-gh-pfus Nov 26, 2024

Choose a reason for hiding this comment

sfc-gh-pfus commented Nov 26, 2024

sfc-gh-dszmolka commented Nov 26, 2024 • edited Loading

sfc-gh-ext-simba-jy commented Nov 25, 2024 •

edited

Loading

sfc-gh-ext-simba-jy commented Nov 26, 2024 •

edited

Loading

sfc-gh-dszmolka commented Nov 26, 2024 •

edited

Loading