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

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/main/java/net/snowflake/client/core/HttpUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -345,9 +345,9 @@ public static CloseableHttpClient buildHttpClient(
}

TrustManager[] trustManagers = null;
if (key != null && key.getOcspMode() != OCSPMode.INSECURE) {
// 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,
Copy link
Contributor

@sfc-gh-dszmolka sfc-gh-dszmolka Dec 19, 2024

Choose a reason for hiding this comment

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

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

// which is by default in the production. disableOCSPMode can be enabled
// 1) OCSP service is down for reasons, 2) PowerMock test that doesn't
// care OCSP checks.
// OCSP FailOpen is ON by default
Expand Down Expand Up @@ -742,7 +742,7 @@ public static String executeRequest(
HttpClientSettingsKey ocspAndProxyKey,
ExecTimeTelemetryData execTimeData)
throws SnowflakeSQLException, IOException {
boolean ocspEnabled = !(ocspAndProxyKey.getOcspMode().equals(OCSPMode.INSECURE));
boolean ocspEnabled = !(ocspAndProxyKey.getOcspMode().equals(OCSPMode.DISABLE_OCSP_CHECKS));
logger.debug("Executing request with OCSP enabled: {}", ocspEnabled);
execTimeData.setOCSPStatus(ocspEnabled);
return executeRequestInternal(
Expand Down
11 changes: 9 additions & 2 deletions src/main/java/net/snowflake/client/core/OCSPMode.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,15 @@ public enum OCSPMode {
*/
FAIL_OPEN(1),

/** Insure mode. No OCSP check is made. */
INSECURE(2);
/**
* @deprecated Use {@link #DISABLE_OCSP_CHECKS} for clarity. This configuration option is used to
* disable OCSP verification. Insure mode. No OCSP check is made.
*/
@Deprecated
INSECURE(2),

/** Disable OCSP checks. It's used to disable OCSP verification. */
DISABLE_OCSP_CHECKS(3);

private final int value;

Expand Down
6 changes: 4 additions & 2 deletions src/main/java/net/snowflake/client/core/SFBaseSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -714,10 +714,12 @@ public void unsetInvalidProxyHostAndPort() {
public OCSPMode getOCSPMode() {
OCSPMode ret;

Boolean disableOCSPMode =
(Boolean) connectionPropertiesMap.get(SFSessionProperty.DISABLE_OCSP_CHECKS);
Boolean insecureMode = (Boolean) connectionPropertiesMap.get(SFSessionProperty.INSECURE_MODE);
if (insecureMode != null && insecureMode) {
if ((disableOCSPMode != null && disableOCSPMode) || (insecureMode != null && insecureMode)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// skip OCSP checks
ret = OCSPMode.INSECURE;
ret = OCSPMode.DISABLE_OCSP_CHECKS;
} else if (!connectionPropertiesMap.containsKey(SFSessionProperty.OCSP_FAIL_OPEN)
|| (boolean) connectionPropertiesMap.get(SFSessionProperty.OCSP_FAIL_OPEN)) {
// fail open (by default, not set)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ public enum SFSessionProperty {
APP_ID("appId", false, String.class),
APP_VERSION("appVersion", false, String.class),
OCSP_FAIL_OPEN("ocspFailOpen", false, Boolean.class),
/**
* @deprecated Use {@link #DISABLE_OCSP_CHECKS} for clarity. This configuration option is used to
* disable OCSP verification.
*/
@Deprecated
INSECURE_MODE("insecureMode", false, Boolean.class),
DISABLE_OCSP_CHECKS("disableOCSPChecks", false, Boolean.class),
QUERY_TIMEOUT("queryTimeout", false, Integer.class),
STRINGS_QUOTED("stringsQuotedForColumnDef", false, Boolean.class),
APPLICATION("application", false, String.class),
Expand Down
10 changes: 4 additions & 6 deletions src/main/java/net/snowflake/client/core/SFTrustManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -841,10 +841,8 @@ private void executeRevocationStatusChecks(
}

private String generateFailOpenLog(String logData) {
return "WARNING!!! Using fail-open to connect. Driver is connecting to an "
+ "HTTPS endpoint without OCSP based Certificate Revocation checking "
+ "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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

+ logData;
}

Expand Down Expand Up @@ -981,7 +979,7 @@ private void executeOneRevocationStatusCheck(
ocspLog = telemetryData.generateTelemetry(SF_OCSP_EVENT_TYPE_VALIDATION_ERROR, error);
if (isOCSPFailOpen()) {
// Log includes fail-open warning.
logger.error(generateFailOpenLog(ocspLog), false);
logger.debug(generateFailOpenLog(ocspLog), false);
} else {
// still not success, raise an error.
logger.debug(ocspLog, false);
Expand Down Expand Up @@ -1163,7 +1161,7 @@ private OCSPResp fetchOcspResponse(
new DecorrelatedJitterBackoff(sleepTime, MAX_SLEEPING_TIME_IN_MILLISECONDS);
boolean success = false;

final int maxRetryCounter = isOCSPFailOpen() ? 1 : 3;
final int maxRetryCounter = isOCSPFailOpen() ? 1 : 2;
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
Exception savedEx = null;
CloseableHttpClient httpClient =
ocspCacheServerClient.computeIfAbsent(
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/net/snowflake/client/jdbc/ConnectionIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -1014,6 +1014,27 @@ public void testFailOverOrgAccount() throws SQLException {
}
}

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

Choose a reason for hiding this comment

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

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?


String deploymentUrl =
"jdbc:snowflake://sfcsupport.snowflakecomputing.com?disableOCSPChecks=true";
Properties properties = new Properties();
properties = new Properties();

properties.put("user", "fakeuser");
properties.put("password", "fakepwd");
properties.put("account", "fakeaccount");
try {
DriverManager.getConnection(deploymentUrl, properties);
fail();
Copy link
Contributor

Choose a reason for hiding this comment

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

We have junit5, we can check something like this:

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

} catch (SQLException e) {
assertThat(
e.getErrorCode(), anyOf(is(INVALID_CONNECTION_INFO_CODE), is(BAD_REQUEST_GS_CODE)));
}
}

private class ConcurrentConnections implements Runnable {

ConcurrentConnections() {}
Expand Down
Loading