This is an automated email from the ASF dual-hosted git repository. zhaijia pushed a commit to branch branch-4.14 in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/branch-4.14 by this push: new e54be34 Fix Bouncy Castle fips incompatible issue (#2740) e54be34 is described below commit e54be3451726c6b7e2fe6a9d412f1a2ab78d8203 Author: Jia Zhai <zhai...@apache.org> AuthorDate: Mon Jul 5 15:22:23 2021 +0800 Fix Bouncy Castle fips incompatible issue (#2740) ### Motivation More details are provided in [Pulsar # 10937](https://github.com/apache/pulsar/issues/10937). In #2631, the default BouncyCastle was changed from non-fips into fips version. But the default version of BouncyCastle in Pulsar is the [non-fips](https://github.com/apache/pulsar/blob/v2.8.0/pulsar-client/pom.xml#L56) one(aimed to make it compatible with the old version of Pulsar). Bouncy Castle provides both FIPS and non-FIPS versions, but in a JVM, it can not include both of the 2 versions(non-Fips and Fips), and we have to exclude the current version before including the other. This makes the backward compatible a little hard, and that's why Pulsar has to involve an individual module for [Bouncy Castle](https://pulsar.apache.org/docs/en/security-bouncy-castle). And if we want to start BookKeeper with TLS enabled through Pulsar's binary, it will meet the following error: ``` Exception in thread "main" java.lang.NoClassDefFoundError: org/bouncycastle/jcajce/provider/BouncyCastleFipsProvider at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:315) at org.apache.bookkeeper.common.util.ReflectionUtils.forName(ReflectionUtils.java:49) at org.apache.bookkeeper.tls.SecurityProviderFactoryFactory.getSecurityProviderFactory(SecurityProviderFactoryFactory.java:39) at org.apache.bookkeeper.proto.BookieServer.<init>(BookieServer.java:129) at org.apache.bookkeeper.server.service.BookieService.<init>(BookieService.java:52) at org.apache.bookkeeper.server.Main.buildBookieServer(Main.java:304) at org.apache.bookkeeper.server.Main.doMain(Main.java:226) at org.apache.bookkeeper.server.Main.main(Main.java:208) Caused by: java.lang.ClassNotFoundException: org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) ... 9 more ``` This fix is to use the reflection to get the loaded bc version to avoid the hard-coded bc version. ### Changes Use the reflection to get the loaded bc version to avoid the hard-coded bc version Add backward compatible test for bc-non-fips version (cherry picked from commit d03b046c2526a3b89305da6460b601656ecd0700) --- .../apache/bookkeeper/tls/TLSContextFactory.java | 94 +++++++++++++++++----- .../java/org/apache/bookkeeper/tls/TestTLS.java | 9 +++ tests/backward-compat/bc-non-fips/pom.xml | 79 ++++++++++++++++++ .../org/apache/bookkeeper/tls/TestBCNonFips.java | 36 +++++++++ tests/backward-compat/pom.xml | 1 + 5 files changed, 201 insertions(+), 18 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 29dbd14..fdb2d01 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.KeyStore; 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; @@ -46,21 +48,78 @@ import java.util.concurrent.TimeUnit; 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; import org.apache.commons.io.FileUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * A factory to manage TLS contexts. */ +@Slf4j 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. + */ + 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 { + 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()); + } + return provider; } /** @@ -83,7 +142,6 @@ public class TLSContextFactory implements SecurityHandlerFactory { } } - private static final Logger LOG = LoggerFactory.getLogger(TLSContextFactory.class); private static final String TLSCONTEXT_HANDLER_NAME = "tls"; private String[] protocols; private String[] ciphers; @@ -130,7 +188,7 @@ public class TLSContextFactory implements SecurityHandlerFactory { 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!"); } @@ -153,7 +211,7 @@ public class TLSContextFactory implements SecurityHandlerFactory { 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!"); } @@ -173,18 +231,18 @@ public class TLSContextFactory implements SecurityHandlerFactory { 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; } @@ -306,7 +364,7 @@ public class TLSContextFactory implements SecurityHandlerFactory { || 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) { @@ -315,7 +373,7 @@ public class TLSContextFactory implements SecurityHandlerFactory { updateClientContext(); } } catch (Exception e) { - LOG.info("Failed to refresh tls certs", e); + log.info("Failed to refresh tls certs", e); } } } @@ -469,15 +527,15 @@ public class TLSContextFactory implements SecurityHandlerFactory { 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 d235073..bc3fbcc 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 @@ -235,6 +235,15 @@ public class TestTLS extends BookKeeperClusterTestCase { } /** + * 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. */ @Test 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 0000000..eba5b1d --- /dev/null +++ b/tests/backward-compat/bc-non-fips/pom.xml @@ -0,0 +1,79 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation=" http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.bookkeeper.tests</groupId> + <artifactId>backward-compat</artifactId> + <version>4.15.0-SNAPSHOT</version> + <relativePath>..</relativePath> + </parent> + + <groupId>org.apache.bookkeeper.tests.backward-compat</groupId> + <artifactId>bc-non-fips</artifactId> + <packaging>jar</packaging> + <name>Apache BookKeeper :: Tests :: Backward Compatibility :: Test Bouncy Castle Provider load non FIPS version</name> + <properties> + <bc-non-fips.version>1.68</bc-non-fips.version> + </properties> + + <dependencies> + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + <version>${junit.version}</version> + </dependency> + + <dependency> + <groupId>org.apache.bookkeeper</groupId> + <artifactId>bookkeeper-server</artifactId> + <version>${project.version}</version> + <exclusions> + <exclusion> + <groupId>org.bouncycastle</groupId> + <artifactId>*</artifactId> + </exclusion> + </exclusions> + <scope>test</scope> + </dependency> + + <dependency> + <groupId>org.bouncycastle</groupId> + <artifactId>bcpkix-jdk15on</artifactId> + <version>${bc-non-fips.version}</version> + </dependency> + + <dependency> + <groupId>org.bouncycastle</groupId> + <artifactId>bcprov-ext-jdk15on</artifactId> + <version>${bc-non-fips.version}</version> + </dependency> + + </dependencies> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-deploy-plugin</artifactId> + <configuration> + <skip>true</skip> + </configuration> + </plugin> + </plugins> + </build> +</project> 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 0000000..eded70b --- /dev/null +++ b/tests/backward-compat/bc-non-fips/src/test/java/org/apache/bookkeeper/tls/TestBCNonFips.java @@ -0,0 +1,36 @@ +/** + * 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 org.junit.Assert; +import org.junit.Test; + +/** + * Test Bouncy Castle Provider load non FIPS version. + */ +public class TestBCNonFips { + + /** + * Verify the BouncyCastleProvider Name is expected. + */ + @Test + public void testGetBouncyCastleProviderName() { + String bcName = TLSContextFactory.getProvider().getName(); + Assert.assertEquals(bcName, TLSContextFactory.BC); + } +} diff --git a/tests/backward-compat/pom.xml b/tests/backward-compat/pom.xml index c22fb9f..0027d1d 100644 --- a/tests/backward-compat/pom.xml +++ b/tests/backward-compat/pom.xml @@ -36,5 +36,6 @@ <module>old-cookie-new-cluster</module> <module>current-server-old-clients</module> <module>yahoo-custom-version</module> + <module>bc-non-fips</module> </modules> </project>