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

Deprecate RevocationStrategy and introduce RevocationCheckingStrategy #1282

Merged
merged 1 commit into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 28 additions & 4 deletions driver/src/main/java/org/neo4j/driver/Config.java
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ public enum Strategy {
private final Strategy strategy;
private final List<File> certFiles;
private boolean hostnameVerificationEnabled = true;
private RevocationStrategy revocationStrategy = RevocationStrategy.NO_CHECKS;
private RevocationCheckingStrategy revocationCheckingStrategy = RevocationCheckingStrategy.NO_CHECKS;

private TrustStrategy(Strategy strategy) {
this(strategy, Collections.emptyList());
Expand Down Expand Up @@ -883,7 +883,31 @@ public static TrustStrategy trustAllCertificates() {
* The revocation strategy used for verifying certificates.
* @return this {@link TrustStrategy}'s revocation strategy
*/
public RevocationCheckingStrategy revocationCheckingStrategy() {
return revocationCheckingStrategy;
}

/**
* The revocation strategy used for verifying certificates.
* @return this {@link TrustStrategy}'s revocation strategy
* @deprecated superseded by {@link TrustStrategy#revocationCheckingStrategy()}
*/
@Deprecated
public RevocationStrategy revocationStrategy() {
RevocationStrategy revocationStrategy;
switch (this.revocationCheckingStrategy) {
case VERIFY_IF_PRESENT:
revocationStrategy = RevocationStrategy.VERIFY_IF_PRESENT;
break;
case STRICT:
revocationStrategy = RevocationStrategy.STRICT;
break;
case NO_CHECKS:
revocationStrategy = RevocationStrategy.NO_CHECKS;
break;
default:
throw new IllegalStateException("Failed to map RevocationCheckingStrategy to RevocationStrategy.");
}
return revocationStrategy;
}

Expand All @@ -893,7 +917,7 @@ public RevocationStrategy revocationStrategy() {
* @return the current trust strategy
*/
public TrustStrategy withoutCertificateRevocationChecks() {
this.revocationStrategy = RevocationStrategy.NO_CHECKS;
this.revocationCheckingStrategy = RevocationCheckingStrategy.NO_CHECKS;
return this;
}

Expand All @@ -905,7 +929,7 @@ public TrustStrategy withoutCertificateRevocationChecks() {
* @return the current trust strategy
*/
public TrustStrategy withVerifyIfPresentRevocationChecks() {
this.revocationStrategy = RevocationStrategy.VERIFY_IF_PRESENT;
this.revocationCheckingStrategy = RevocationCheckingStrategy.VERIFY_IF_PRESENT;
return this;
}

Expand All @@ -919,7 +943,7 @@ public TrustStrategy withVerifyIfPresentRevocationChecks() {
* @return the current trust strategy
*/
public TrustStrategy withStrictRevocationChecks() {
this.revocationStrategy = RevocationStrategy.STRICT;
this.revocationCheckingStrategy = RevocationCheckingStrategy.STRICT;
return this;
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright (c) "Neo4j"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.neo4j.driver;

/**
* Defines strategy for revocation checks.
*/
public enum RevocationCheckingStrategy {
/** Don't do any OCSP revocation checks, regardless whether there are stapled revocation statuses or not. */
NO_CHECKS,
/** Verify OCSP revocation checks when the revocation status is stapled to the certificate, continue if not. */
VERIFY_IF_PRESENT,
/** Require stapled revocation status and verify OCSP revocation checks, fail if no revocation status is stapled to the certificate. */
STRICT;

public static boolean requiresRevocationChecking(RevocationCheckingStrategy revocationCheckingStrategy) {
return revocationCheckingStrategy.equals(STRICT) || revocationCheckingStrategy.equals(VERIFY_IF_PRESENT);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@
*/
package org.neo4j.driver.internal;

import org.neo4j.driver.RevocationCheckingStrategy;

/**
* Defines strategy for revocation checks.
*
* @deprecated superseded by {@link RevocationCheckingStrategy}
*/
@Deprecated
public enum RevocationStrategy {
/** Don't do any OCSP revocation checks, regardless whether there are stapled revocation statuses or not. */
NO_CHECKS,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.io.Serializable;
import java.security.GeneralSecurityException;
import org.neo4j.driver.Config;
import org.neo4j.driver.RevocationCheckingStrategy;
import org.neo4j.driver.exceptions.ClientException;
import org.neo4j.driver.internal.security.SecurityPlan;
import org.neo4j.driver.internal.security.SecurityPlanImpl;
Expand Down Expand Up @@ -56,6 +57,7 @@ private boolean isCustomized() {
return !(DEFAULT.encrypted() == this.encrypted() && DEFAULT.hasEqualTrustStrategy(this));
}

@SuppressWarnings("deprecation")
private boolean hasEqualTrustStrategy(SecuritySettings other) {
Config.TrustStrategy t1 = this.trustStrategy;
Config.TrustStrategy t2 = other.trustStrategy;
Expand All @@ -66,6 +68,7 @@ private boolean hasEqualTrustStrategy(SecuritySettings other) {
return t1.isHostnameVerificationEnabled() == t2.isHostnameVerificationEnabled()
&& t1.strategy() == t2.strategy()
&& t1.certFiles().equals(t2.certFiles())
&& t1.revocationCheckingStrategy() == t2.revocationCheckingStrategy()
&& t1.revocationStrategy() == t2.revocationStrategy();
}

Expand All @@ -92,9 +95,9 @@ private void assertSecuritySettingsNotUserConfigured(String uriScheme) {

private SecurityPlan createSecurityPlanFromScheme(String scheme) throws GeneralSecurityException, IOException {
if (isHighTrustScheme(scheme)) {
return SecurityPlanImpl.forSystemCASignedCertificates(true, RevocationStrategy.NO_CHECKS);
return SecurityPlanImpl.forSystemCASignedCertificates(true, RevocationCheckingStrategy.NO_CHECKS);
} else {
return SecurityPlanImpl.forAllCertificates(false, RevocationStrategy.NO_CHECKS);
return SecurityPlanImpl.forAllCertificates(false, RevocationCheckingStrategy.NO_CHECKS);
}
}

Expand All @@ -106,16 +109,16 @@ private static SecurityPlan createSecurityPlanImpl(boolean encrypted, Config.Tru
throws GeneralSecurityException, IOException {
if (encrypted) {
boolean hostnameVerificationEnabled = trustStrategy.isHostnameVerificationEnabled();
RevocationStrategy revocationStrategy = trustStrategy.revocationStrategy();
RevocationCheckingStrategy revocationCheckingStrategy = trustStrategy.revocationCheckingStrategy();
switch (trustStrategy.strategy()) {
case TRUST_CUSTOM_CA_SIGNED_CERTIFICATES:
return SecurityPlanImpl.forCustomCASignedCertificates(
trustStrategy.certFiles(), hostnameVerificationEnabled, revocationStrategy);
trustStrategy.certFiles(), hostnameVerificationEnabled, revocationCheckingStrategy);
case TRUST_SYSTEM_CA_SIGNED_CERTIFICATES:
return SecurityPlanImpl.forSystemCASignedCertificates(
hostnameVerificationEnabled, revocationStrategy);
hostnameVerificationEnabled, revocationCheckingStrategy);
case TRUST_ALL_CERTIFICATES:
return SecurityPlanImpl.forAllCertificates(hostnameVerificationEnabled, revocationStrategy);
return SecurityPlanImpl.forAllCertificates(hostnameVerificationEnabled, revocationCheckingStrategy);
default:
throw new ClientException("Unknown TLS authentication strategy: "
+ trustStrategy.strategy().name());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
package org.neo4j.driver.internal.security;

import javax.net.ssl.SSLContext;
import org.neo4j.driver.internal.RevocationStrategy;
import org.neo4j.driver.RevocationCheckingStrategy;

/**
* A SecurityPlan consists of encryption and trust details.
Expand All @@ -31,5 +31,5 @@ public interface SecurityPlan {

boolean requiresHostnameVerification();

RevocationStrategy revocationStrategy();
RevocationCheckingStrategy revocationCheckingStrategy();
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
*/
package org.neo4j.driver.internal.security;

import static org.neo4j.driver.internal.RevocationStrategy.VERIFY_IF_PRESENT;
import static org.neo4j.driver.internal.RevocationStrategy.requiresRevocationChecking;
import static org.neo4j.driver.RevocationCheckingStrategy.VERIFY_IF_PRESENT;
import static org.neo4j.driver.RevocationCheckingStrategy.requiresRevocationChecking;
import static org.neo4j.driver.internal.util.CertificateTool.loadX509Cert;

import java.io.File;
Expand All @@ -41,36 +41,39 @@
import javax.net.ssl.TrustManager;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509TrustManager;
import org.neo4j.driver.internal.RevocationStrategy;
import org.neo4j.driver.RevocationCheckingStrategy;

/**
* A SecurityPlan consists of encryption and trust details.
*/
public class SecurityPlanImpl implements SecurityPlan {
public static SecurityPlan forAllCertificates(
boolean requiresHostnameVerification, RevocationStrategy revocationStrategy)
boolean requiresHostnameVerification, RevocationCheckingStrategy revocationCheckingStrategy)
throws GeneralSecurityException {
SSLContext sslContext = SSLContext.getInstance("TLS");
sslContext.init(new KeyManager[0], new TrustManager[] {new TrustAllTrustManager()}, null);

return new SecurityPlanImpl(true, sslContext, requiresHostnameVerification, revocationStrategy);
return new SecurityPlanImpl(true, sslContext, requiresHostnameVerification, revocationCheckingStrategy);
}

public static SecurityPlan forCustomCASignedCertificates(
List<File> certFiles, boolean requiresHostnameVerification, RevocationStrategy revocationStrategy)
List<File> certFiles,
boolean requiresHostnameVerification,
RevocationCheckingStrategy revocationCheckingStrategy)
throws GeneralSecurityException, IOException {
SSLContext sslContext = configureSSLContext(certFiles, revocationStrategy);
return new SecurityPlanImpl(true, sslContext, requiresHostnameVerification, revocationStrategy);
SSLContext sslContext = configureSSLContext(certFiles, revocationCheckingStrategy);
return new SecurityPlanImpl(true, sslContext, requiresHostnameVerification, revocationCheckingStrategy);
}

public static SecurityPlan forSystemCASignedCertificates(
boolean requiresHostnameVerification, RevocationStrategy revocationStrategy)
boolean requiresHostnameVerification, RevocationCheckingStrategy revocationCheckingStrategy)
throws GeneralSecurityException, IOException {
SSLContext sslContext = configureSSLContext(Collections.emptyList(), revocationStrategy);
return new SecurityPlanImpl(true, sslContext, requiresHostnameVerification, revocationStrategy);
SSLContext sslContext = configureSSLContext(Collections.emptyList(), revocationCheckingStrategy);
return new SecurityPlanImpl(true, sslContext, requiresHostnameVerification, revocationCheckingStrategy);
}

private static SSLContext configureSSLContext(List<File> customCertFiles, RevocationStrategy revocationStrategy)
private static SSLContext configureSSLContext(
List<File> customCertFiles, RevocationCheckingStrategy revocationCheckingStrategy)
throws GeneralSecurityException, IOException {
KeyStore trustedKeyStore = KeyStore.getInstance(KeyStore.getDefaultType());
trustedKeyStore.load(null, null);
Expand All @@ -83,7 +86,7 @@ private static SSLContext configureSSLContext(List<File> customCertFiles, Revoca
}

PKIXBuilderParameters pkixBuilderParameters =
configurePKIXBuilderParameters(trustedKeyStore, revocationStrategy);
configurePKIXBuilderParameters(trustedKeyStore, revocationCheckingStrategy);

SSLContext sslContext = SSLContext.getInstance("TLS");
TrustManagerFactory trustManagerFactory =
Expand All @@ -101,11 +104,11 @@ private static SSLContext configureSSLContext(List<File> customCertFiles, Revoca
}

private static PKIXBuilderParameters configurePKIXBuilderParameters(
KeyStore trustedKeyStore, RevocationStrategy revocationStrategy)
KeyStore trustedKeyStore, RevocationCheckingStrategy revocationCheckingStrategy)
throws InvalidAlgorithmParameterException, KeyStoreException {
PKIXBuilderParameters pkixBuilderParameters = null;

if (requiresRevocationChecking(revocationStrategy)) {
if (requiresRevocationChecking(revocationCheckingStrategy)) {
// Configure certificate revocation checking (X509CertSelector() selects all certificates)
pkixBuilderParameters = new PKIXBuilderParameters(trustedKeyStore, new X509CertSelector());

Expand All @@ -115,7 +118,7 @@ private static PKIXBuilderParameters configurePKIXBuilderParameters(
// enables status_request extension in client hello
System.setProperty("jdk.tls.client.enableStatusRequestExtension", "true");

if (revocationStrategy.equals(VERIFY_IF_PRESENT)) {
if (revocationCheckingStrategy.equals(VERIFY_IF_PRESENT)) {
// enables soft-fail behaviour if no stapled response found.
Security.setProperty("ocsp.enable", "true");
}
Expand Down Expand Up @@ -146,23 +149,23 @@ private static void loadSystemCertificates(KeyStore trustedKeyStore) throws Gene
}

public static SecurityPlan insecure() {
return new SecurityPlanImpl(false, null, false, RevocationStrategy.NO_CHECKS);
return new SecurityPlanImpl(false, null, false, RevocationCheckingStrategy.NO_CHECKS);
}

private final boolean requiresEncryption;
private final SSLContext sslContext;
private final boolean requiresHostnameVerification;
private final RevocationStrategy revocationStrategy;
private final RevocationCheckingStrategy revocationCheckingStrategy;

private SecurityPlanImpl(
boolean requiresEncryption,
SSLContext sslContext,
boolean requiresHostnameVerification,
RevocationStrategy revocationStrategy) {
RevocationCheckingStrategy revocationCheckingStrategy) {
this.requiresEncryption = requiresEncryption;
this.sslContext = sslContext;
this.requiresHostnameVerification = requiresHostnameVerification;
this.revocationStrategy = revocationStrategy;
this.revocationCheckingStrategy = revocationCheckingStrategy;
}

@Override
Expand All @@ -181,8 +184,8 @@ public boolean requiresHostnameVerification() {
}

@Override
public RevocationStrategy revocationStrategy() {
return revocationStrategy;
public RevocationCheckingStrategy revocationCheckingStrategy() {
return revocationCheckingStrategy;
}

private static class TrustAllTrustManager implements X509TrustManager {
Expand Down
17 changes: 10 additions & 7 deletions driver/src/test/java/org/neo4j/driver/ConfigTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.neo4j.driver.internal.RevocationStrategy.NO_CHECKS;
import static org.neo4j.driver.internal.RevocationStrategy.STRICT;
import static org.neo4j.driver.internal.RevocationStrategy.VERIFY_IF_PRESENT;
import static org.neo4j.driver.internal.handlers.pulln.FetchSizeUtil.DEFAULT_FETCH_SIZE;

import java.io.File;
Expand All @@ -44,6 +41,7 @@
import org.junit.jupiter.params.provider.ValueSource;
import org.junit.platform.commons.support.HierarchyTraversalMode;
import org.junit.platform.commons.support.ReflectionSupport;
import org.neo4j.driver.internal.RevocationStrategy;
import org.neo4j.driver.internal.logging.ConsoleLogging;
import org.neo4j.driver.internal.logging.DevNullLogging;
import org.neo4j.driver.internal.logging.JULogging;
Expand Down Expand Up @@ -279,19 +277,24 @@ void shouldEnableAndDisableHostnameVerificationOnTrustStrategy() {
assertFalse(trustStrategy.isHostnameVerificationEnabled());
}

@SuppressWarnings("deprecation")
@Test
void shouldEnableAndDisableCertificateRevocationChecksOnTestStrategy() {
Config.TrustStrategy trustStrategy = Config.TrustStrategy.trustSystemCertificates();
assertEquals(NO_CHECKS, trustStrategy.revocationStrategy());
assertEquals(RevocationCheckingStrategy.NO_CHECKS, trustStrategy.revocationCheckingStrategy());
assertEquals(RevocationStrategy.NO_CHECKS, trustStrategy.revocationStrategy());

assertSame(trustStrategy, trustStrategy.withoutCertificateRevocationChecks());
assertEquals(NO_CHECKS, trustStrategy.revocationStrategy());
assertEquals(RevocationCheckingStrategy.NO_CHECKS, trustStrategy.revocationCheckingStrategy());
assertEquals(RevocationStrategy.NO_CHECKS, trustStrategy.revocationStrategy());

assertSame(trustStrategy, trustStrategy.withStrictRevocationChecks());
assertEquals(STRICT, trustStrategy.revocationStrategy());
assertEquals(RevocationCheckingStrategy.STRICT, trustStrategy.revocationCheckingStrategy());
assertEquals(RevocationStrategy.STRICT, trustStrategy.revocationStrategy());

assertSame(trustStrategy, trustStrategy.withVerifyIfPresentRevocationChecks());
assertEquals(VERIFY_IF_PRESENT, trustStrategy.revocationStrategy());
assertEquals(RevocationCheckingStrategy.VERIFY_IF_PRESENT, trustStrategy.revocationCheckingStrategy());
assertEquals(RevocationStrategy.VERIFY_IF_PRESENT, trustStrategy.revocationStrategy());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@
import org.junit.jupiter.api.extension.RegisterExtension;
import org.neo4j.driver.AuthToken;
import org.neo4j.driver.AuthTokens;
import org.neo4j.driver.RevocationCheckingStrategy;
import org.neo4j.driver.exceptions.AuthenticationException;
import org.neo4j.driver.exceptions.ServiceUnavailableException;
import org.neo4j.driver.internal.BoltServerAddress;
import org.neo4j.driver.internal.ConnectionSettings;
import org.neo4j.driver.internal.DefaultDomainNameResolver;
import org.neo4j.driver.internal.RevocationStrategy;
import org.neo4j.driver.internal.async.connection.BootstrapFactory;
import org.neo4j.driver.internal.async.connection.ChannelConnector;
import org.neo4j.driver.internal.async.connection.ChannelConnectorImpl;
Expand Down Expand Up @@ -222,6 +222,6 @@ private ChannelConnectorImpl newConnector(
}

private static SecurityPlan trustAllCertificates() throws GeneralSecurityException {
return SecurityPlanImpl.forAllCertificates(false, RevocationStrategy.NO_CHECKS);
return SecurityPlanImpl.forAllCertificates(false, RevocationCheckingStrategy.NO_CHECKS);
}
}
Loading