From 90aa086f6870ded621ff9d8a69fc79b56a6f8493 Mon Sep 17 00:00:00 2001 From: Jia Zhai Date: Thu, 17 Jun 2021 00:42:52 +0800 Subject: [PATCH 1/5] fix bc fips incompatible issue --- .../bookkeeper/tls/TLSContextFactory.java | 65 ++++++++++++++++++- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java index 29dbd143a53..94b8371b1d1 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java @@ -37,6 +37,8 @@ import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; import java.security.NoSuchProviderException; +import java.security.Provider; +import java.security.Security; import java.security.UnrecoverableKeyException; import java.security.cert.CertificateException; import java.security.spec.InvalidKeySpecException; @@ -58,9 +60,66 @@ */ public class TLSContextFactory implements SecurityHandlerFactory { - static { - // Fixes loading PKCS8Key file: https://stackoverflow.com/a/18912362 - java.security.Security.addProvider(new org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider()); + public static final Provider BC_PROVIDER = getProvider(); + public static final String BC_FIPS_PROVIDER_CLASS = "org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider"; + public static final String BC_NON_FIPS_PROVIDER_CLASS = "org.bouncycastle.jce.provider.BouncyCastleProvider"; + + // Security.getProvider("BC") / Security.getProvider("BCFIPS"). + // also used to get Factories. e.g. CertificateFactory.getInstance("X.509", "BCFIPS") + public static final String BC_FIPS = "BCFIPS"; + public static final String BC = "BC"; + + /** + * Get Bouncy Castle provider, and call Security.addProvider(provider) if success. + * 1. try get from classpath. + * 2. try get from Nar. + */ + public static Provider getProvider() { + boolean isProviderInstalled = + Security.getProvider(BC) != null || Security.getProvider(BC_FIPS) != null; + + if (isProviderInstalled) { + Provider provider = Security.getProvider(BC) != null + ? Security.getProvider(BC) + : Security.getProvider(BC_FIPS); + if (LOG.isDebugEnabled()) { + LOG.debug("Already instantiated Bouncy Castle provider {}", provider.getName()); + } + return provider; + } + + // Not installed, try load from class path + try { + return getBCProviderFromClassPath(); + } catch (Exception e) { + LOG.warn("Not able to get Bouncy Castle provider for both FIPS and Non-FIPS from class path:", e); + throw new RuntimeException(e); + } + } + + /** + * Get Bouncy Castle provider from classpath, and call Security.addProvider. + * Throw Exception if failed. + */ + public static Provider getBCProviderFromClassPath() throws Exception { + Class clazz; + try { + // prefer non FIPS, for backward compatibility concern. + clazz = Class.forName(BC_NON_FIPS_PROVIDER_CLASS); + } catch (ClassNotFoundException cnf) { + LOG.warn("Not able to get Bouncy Castle provider: {}, try to get FIPS provider {}", + BC_NON_FIPS_PROVIDER_CLASS, BC_FIPS_PROVIDER_CLASS); + // attempt to use the FIPS provider. + clazz = Class.forName(BC_FIPS_PROVIDER_CLASS); + } + + @SuppressWarnings("unchecked") + Provider provider = (Provider) clazz.getDeclaredConstructor().newInstance(); + Security.addProvider(provider); + if (LOG.isDebugEnabled()) { + LOG.debug("Found and Instantiated Bouncy Castle provider in classpath {}", provider.getName()); + } + return provider; } /** From 109518129165994612904448d782873f56b1331e Mon Sep 17 00:00:00 2001 From: Jia Zhai Date: Tue, 29 Jun 2021 11:01:25 +0800 Subject: [PATCH 2/5] fix test issue, fix comments --- .../bookkeeper/tls/TLSContextFactory.java | 52 ++++++++++--------- .../org/apache/bookkeeper/tls/TestTLS.java | 9 ++++ 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java index 94b8371b1d1..4ebe79d1413 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java @@ -48,6 +48,7 @@ import javax.net.ssl.KeyManagerFactory; import javax.net.ssl.TrustManagerFactory; +import lombok.extern.slf4j.Slf4j; import org.apache.bookkeeper.conf.AbstractConfiguration; import org.apache.bookkeeper.conf.ClientConfiguration; import org.apache.bookkeeper.conf.ServerConfiguration; @@ -58,6 +59,7 @@ /** * A factory to manage TLS contexts. */ +@Slf4j public class TLSContextFactory implements SecurityHandlerFactory { public static final Provider BC_PROVIDER = getProvider(); @@ -71,8 +73,6 @@ public class TLSContextFactory implements SecurityHandlerFactory { /** * Get Bouncy Castle provider, and call Security.addProvider(provider) if success. - * 1. try get from classpath. - * 2. try get from Nar. */ public static Provider getProvider() { boolean isProviderInstalled = @@ -82,8 +82,8 @@ public static Provider getProvider() { Provider provider = Security.getProvider(BC) != null ? Security.getProvider(BC) : Security.getProvider(BC_FIPS); - if (LOG.isDebugEnabled()) { - LOG.debug("Already instantiated Bouncy Castle provider {}", provider.getName()); + if (log.isDebugEnabled()) { + log.debug("Already instantiated Bouncy Castle provider {}", provider.getName()); } return provider; } @@ -92,7 +92,7 @@ public static Provider getProvider() { try { return getBCProviderFromClassPath(); } catch (Exception e) { - LOG.warn("Not able to get Bouncy Castle provider for both FIPS and Non-FIPS from class path:", e); + log.warn("Not able to get Bouncy Castle provider for both FIPS and Non-FIPS from class path:", e); throw new RuntimeException(e); } } @@ -104,20 +104,22 @@ public static Provider getProvider() { public static Provider getBCProviderFromClassPath() throws Exception { Class clazz; try { - // prefer non FIPS, for backward compatibility concern. - clazz = Class.forName(BC_NON_FIPS_PROVIDER_CLASS); - } catch (ClassNotFoundException cnf) { - LOG.warn("Not able to get Bouncy Castle provider: {}, try to get FIPS provider {}", - BC_NON_FIPS_PROVIDER_CLASS, BC_FIPS_PROVIDER_CLASS); - // attempt to use the FIPS provider. clazz = Class.forName(BC_FIPS_PROVIDER_CLASS); + } catch (ClassNotFoundException cnf) { + if (log.isDebugEnabled()) { + log.debug("Not able to get Bouncy Castle provider: {}, try to get FIPS provider {}", + BC_NON_FIPS_PROVIDER_CLASS, BC_FIPS_PROVIDER_CLASS); + } + // attempt to use the NON_FIPS provider. + clazz = Class.forName(BC_NON_FIPS_PROVIDER_CLASS); + } @SuppressWarnings("unchecked") Provider provider = (Provider) clazz.getDeclaredConstructor().newInstance(); Security.addProvider(provider); - if (LOG.isDebugEnabled()) { - LOG.debug("Found and Instantiated Bouncy Castle provider in classpath {}", provider.getName()); + if (log.isDebugEnabled()) { + log.debug("Found and Instantiated Bouncy Castle provider in classpath {}", provider.getName()); } return provider; } @@ -189,7 +191,7 @@ private KeyManagerFactory initKeyManagerFactory(String keyStoreType, String keyS KeyManagerFactory kmf = null; if (Strings.isNullOrEmpty(keyStoreLocation)) { - LOG.error("Key store location cannot be empty when Mutual Authentication is enabled!"); + log.error("Key store location cannot be empty when Mutual Authentication is enabled!"); throw new SecurityException("Key store location cannot be empty when Mutual Authentication is enabled!"); } @@ -212,7 +214,7 @@ private TrustManagerFactory initTrustManagerFactory(String trustStoreType, Strin TrustManagerFactory tmf; if (Strings.isNullOrEmpty(trustStoreLocation)) { - LOG.error("Trust Store location cannot be empty!"); + log.error("Trust Store location cannot be empty!"); throw new SecurityException("Trust Store location cannot be empty!"); } @@ -232,18 +234,18 @@ private TrustManagerFactory initTrustManagerFactory(String trustStoreType, Strin private SslProvider getTLSProvider(String sslProvider) { if (sslProvider.trim().equalsIgnoreCase("OpenSSL")) { if (OpenSsl.isAvailable()) { - LOG.info("Security provider - OpenSSL"); + log.info("Security provider - OpenSSL"); return SslProvider.OPENSSL; } Throwable causeUnavailable = OpenSsl.unavailabilityCause(); - LOG.warn("OpenSSL Unavailable: ", causeUnavailable); + log.warn("OpenSSL Unavailable: ", causeUnavailable); - LOG.info("Security provider - JDK"); + log.info("Security provider - JDK"); return SslProvider.JDK; } - LOG.info("Security provider - JDK"); + log.info("Security provider - JDK"); return SslProvider.JDK; } @@ -365,7 +367,7 @@ private synchronized SslContext getSSLContext() { || tlsKeyStorePasswordFilePath.checkAndRefresh() || tlsTrustStoreFilePath.checkAndRefresh() || tlsTrustStorePasswordFilePath.checkAndRefresh()) { try { - LOG.info("Updating tls certs certFile={}, keyStoreFile={}, trustStoreFile={}", + log.info("Updating tls certs certFile={}, keyStoreFile={}, trustStoreFile={}", tlsCertificateFilePath.getFileName(), tlsKeyStoreFilePath.getFileName(), tlsTrustStoreFilePath.getFileName()); if (isServerCtx) { @@ -374,7 +376,7 @@ private synchronized SslContext getSSLContext() { updateClientContext(); } } catch (Exception e) { - LOG.info("Failed to refresh tls certs", e); + log.info("Failed to refresh tls certs", e); } } } @@ -528,15 +530,15 @@ public SslHandler newTLSHandler() { if (protocols != null && protocols.length != 0) { sslHandler.engine().setEnabledProtocols(protocols); } - if (LOG.isDebugEnabled()) { - LOG.debug("Enabled cipher protocols: {} ", Arrays.toString(sslHandler.engine().getEnabledProtocols())); + if (log.isDebugEnabled()) { + log.debug("Enabled cipher protocols: {} ", Arrays.toString(sslHandler.engine().getEnabledProtocols())); } if (ciphers != null && ciphers.length != 0) { sslHandler.engine().setEnabledCipherSuites(ciphers); } - if (LOG.isDebugEnabled()) { - LOG.debug("Enabled cipher suites: {} ", Arrays.toString(sslHandler.engine().getEnabledCipherSuites())); + if (log.isDebugEnabled()) { + log.debug("Enabled cipher suites: {} ", Arrays.toString(sslHandler.engine().getEnabledCipherSuites())); } return sslHandler; diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java index f4b59d8568c..d2c420ce4ca 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java @@ -233,6 +233,15 @@ public void tearDown() throws Exception { super.tearDown(); } + /** + * Verify the BouncyCastleProvider Name is expected. + */ + @Test + public void testGetBouncyCastleProviderName() throws Exception { + String bcName = TLSContextFactory.getProvider().getName(); + Assert.assertEquals(bcName, TLSContextFactory.BC_FIPS); + } + /** * Verify that a server will not start if tls is enabled but no cert is specified. */ From dc34d8543a2c924d768d1975c8b1cfe64ec68a18 Mon Sep 17 00:00:00 2001 From: Jia Zhai Date: Tue, 29 Jun 2021 11:33:38 +0800 Subject: [PATCH 3/5] add related test --- tests/backward-compat/bc-non-fips/pom.xml | 79 +++++++++++++++++++ .../apache/bookkeeper/tls/TestBCNonFips.java | 39 +++++++++ tests/backward-compat/pom.xml | 1 + 3 files changed, 119 insertions(+) create mode 100644 tests/backward-compat/bc-non-fips/pom.xml create mode 100644 tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java diff --git a/tests/backward-compat/bc-non-fips/pom.xml b/tests/backward-compat/bc-non-fips/pom.xml new file mode 100644 index 00000000000..eba5b1d7219 --- /dev/null +++ b/tests/backward-compat/bc-non-fips/pom.xml @@ -0,0 +1,79 @@ + + + + 4.0.0 + + org.apache.bookkeeper.tests + backward-compat + 4.15.0-SNAPSHOT + .. + + + org.apache.bookkeeper.tests.backward-compat + bc-non-fips + jar + Apache BookKeeper :: Tests :: Backward Compatibility :: Test Bouncy Castle Provider load non FIPS version + + 1.68 + + + + + junit + junit + ${junit.version} + + + + org.apache.bookkeeper + bookkeeper-server + ${project.version} + + + org.bouncycastle + * + + + test + + + + org.bouncycastle + bcpkix-jdk15on + ${bc-non-fips.version} + + + + org.bouncycastle + bcprov-ext-jdk15on + ${bc-non-fips.version} + + + + + + + org.apache.maven.plugins + maven-deploy-plugin + + true + + + + + diff --git a/tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java b/tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java new file mode 100644 index 00000000000..8b9d4fbc666 --- /dev/null +++ b/tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java @@ -0,0 +1,39 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.bookkeeper.tls; + +import lombok.extern.slf4j.Slf4j; +import org.junit.Assert; +import org.junit.Test; + +/** + * Test Bouncy Castle Provider load non FIPS version. + */ +@Slf4j +public class TestBCNonFips { + + /** + * Verify the BouncyCastleProvider Name is expected. + */ + @Test + public void testGetBouncyCastleProviderName() throws Exception { + String bcName = TLSContextFactory.getProvider().getName(); + Assert.assertEquals(bcName, TLSContextFactory.BC); + log.info("Loaded BouncyCastle name: {}", bcName); + } +} diff --git a/tests/backward-compat/pom.xml b/tests/backward-compat/pom.xml index af9dbfa2814..396840bb8f4 100644 --- a/tests/backward-compat/pom.xml +++ b/tests/backward-compat/pom.xml @@ -36,5 +36,6 @@ old-cookie-new-cluster current-server-old-clients yahoo-custom-version + bc-non-fips From 5b1894eeb797048cb89922f9bc2fa8df68d4edce Mon Sep 17 00:00:00 2001 From: Jia Zhai Date: Tue, 29 Jun 2021 12:59:55 +0800 Subject: [PATCH 4/5] fix include error --- .../test/java/org/apache/bookkeeper/tls/TestBCNonFips.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java b/tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java index 8b9d4fbc666..eded70b0a59 100644 --- a/tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java +++ b/tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java @@ -17,23 +17,20 @@ */ package org.apache.bookkeeper.tls; -import lombok.extern.slf4j.Slf4j; import org.junit.Assert; import org.junit.Test; /** * Test Bouncy Castle Provider load non FIPS version. */ -@Slf4j public class TestBCNonFips { /** * Verify the BouncyCastleProvider Name is expected. */ @Test - public void testGetBouncyCastleProviderName() throws Exception { + public void testGetBouncyCastleProviderName() { String bcName = TLSContextFactory.getProvider().getName(); Assert.assertEquals(bcName, TLSContextFactory.BC); - log.info("Loaded BouncyCastle name: {}", bcName); } } From 5e87829305ee4c4a03ac874b3cb39a2904dff936 Mon Sep 17 00:00:00 2001 From: Jia Zhai Date: Tue, 29 Jun 2021 20:23:02 +0800 Subject: [PATCH 5/5] remove unused code --- .../main/java/org/apache/bookkeeper/tls/TLSContextFactory.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java index 4ebe79d1413..fdb2d019c34 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java @@ -53,8 +53,6 @@ import org.apache.bookkeeper.conf.ClientConfiguration; import org.apache.bookkeeper.conf.ServerConfiguration; import org.apache.commons.io.FileUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * A factory to manage TLS contexts. @@ -144,7 +142,6 @@ public String toString() { } } - private static final Logger LOG = LoggerFactory.getLogger(TLSContextFactory.class); private static final String TLSCONTEXT_HANDLER_NAME = "tls"; private String[] protocols; private String[] ciphers;