You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/06/26 23:25:35 UTC
[GitHub] [bookkeeper] Ghatage commented on a change in pull request #2740: [WIP] Fix Bouncy Castle fips incompatible issue
Ghatage commented on a change in pull request #2740:
URL: https://github.com/apache/bookkeeper/pull/2740#discussion_r659237000
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java
##########
@@ -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)
Review comment:
This will never be true because since we don't add the BC provider in code, and expect it to be loaded via classpath, it should be present in the classpath.
Currently, we add the dependencies of the project via a set_module to the classpath at runtime.
[Right now only fips based bouncy castle is present in the dependencies](https://github.com/apache/bookkeeper/blob/90aa086f6870ded621ff9d8a69fc79b56a6f8493/pom.xml#L304).
Furthermore I see [this flag to be set by default in the bookeeper start script](https://github.com/apache/bookkeeper/blame/90aa086f6870ded621ff9d8a69fc79b56a6f8493/bin/bookkeeper#L149) which is going to run BK in fips mode.
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java
##########
@@ -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.
Review comment:
What is the backward compatibility concern here @jiazhai?
AFAIK FIPS approved functions are the same as non-FIPS, but the underlying ciphers / algos being used are just the ones which are approved by the government. You can have 1 service running FIPS (say BK) and another running in non-FIPS (pulsar) and it would not cause any problem.
Also, please refer to the earlier comment, it seems we are using FIPS based BC by default so this code might not work as expected.
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java
##########
@@ -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.
Review comment:
@jiazhai I see the implementation for loading from classpath only here, not from Nar.
I guess we'll add it in this PR eventually as it is a WIP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org