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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
