Move `Security.addProvider(new BouncyCastleProvider())` into a `Callable` block to avoid NoClassDefFoundError #5312

jmarti44 · 2023-07-05T21:28:15Z

Description

Moved Security.addProvider call inside a Callable block so that whole block does not throw NoClassDefFoundError for BouncyCastleProvider (In user's scenario bc-fips is used instead of bcprov-jdk18on that does not contain org.bouncycastle.jce.provider.BouncyCastleProvider
Added a new module kubernetes-client-deps-compatibility-tests to Kubernetes Client project for having project specific tests where we can test behavior of client in presence of different dependencies on the classpath
Added a test for the abovementioned scenario in a separate module named kubernetes-client-deps-compatibility-tests/kubernetes-client-init-bc-fips with org.bouncycastle:bc-fips and org.bouncycastle:bcpkix-fips dependencies.

where CertUtils would automatically try to create an instance of the bouncy castle provider within the callable when there is a not bouncy castle fips provider provided. Wrapping it in a callable in this fashion would automatically throw a NoClassDefFound error despite already providing a security bouncy castle fips provider. Moving the conditional where the application searches for the fips provider just outside and before the callable will successfully create an instance of PEMKeyPair and will return successfully when a BouncyCastleFipsProvider is created.

I have added two zipped maven projects to help illustrate the issue and solution. They are both simple projects that print a successful message when both a Kubernetes Client and OpenShift Client is successfully built when a FIPS provider is provided. The project fabric8_issue utilizes a version of the kubernetes client api with this proposed fix. It is able to print a success message as shown in the screenshot below

The second project (fabric8_issue2) is the same project with the current version of the Kubernetes Client API (pulled from master). The NoClassDefFound Error is thrown here.

These projects were built using JDK17.

simple_projects.zip

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

rohanKanojia · 2023-07-06T04:24:40Z

@jmarti44 : Could you please link the issue which you're trying to fix in this PR?

Would be nice to add some tests to validate the fix as well.

rohanKanojia · 2023-07-06T04:59:28Z

@jmarti44 : Sorry, your PR description was hidden by comment. I've updated it now

jmarti44 · 2023-07-06T16:47:52Z

@rohanKanojia Would unit testing be neccessary for the method I changed since it's private method? What kind of testing could I utilize? My attempt was to illustrate screenshots from my sample projects and zipping the projects themselves as well

sonarcloud · 2023-07-06T17:18:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

sunix · 2023-07-19T08:44:35Z

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/internal/CertUtils.java

@@ -149,13 +149,19 @@ private static PrivateKey loadKey(InputStream keyInputStream, String clientKeyAl
  private static PrivateKey handleECKey(InputStream keyInputStream) {
    // Let's wrap the code to a callable inner class to avoid NoClassDef when loading this class.
    try {
+      if (Security.getProvider("BC") == null && Security.getProvider("BCFIPS") == null) {
+        new Callable<String>() {


I am not sure to understand why you are creating a anonymous Callable object just to execute the call method without reusing it afterwards?
Could probably be simplified with just

Security.addProvider(new org.bouncycastle.jce.provider.BouncyCastleProvider());

It's in case the dependency is not present at runtime. There's a utility for this sort of thing called OptionalDependencyWrapper, but it's in common, not api. We could consider moving that up to the api module for this.

rohanKanojia · 2024-02-12T16:52:55Z

@jmarti44 : Hello, Sorry for late reply. I downloaded your reproducer project and extracted it to my machine.

~~However I couldn't reproduce the issue either on fabric8-issue or fabric8-issue-2 reproducer projects~~. On my machine both apps print CLIENT BUILD SUCCESS:

fabric8-issue (using 6.11-SNAPSHOT)

fabric8-issue : $ mvn exec:java -Dexec.mainClass=com.mycompany.app.App
[INFO] Scanning for projects...
[INFO] 
[INFO] ----------------------< com.mycompany.app:my-app >----------------------
[INFO] Building my-app 1.0-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- exec:3.1.1:java (default-cli) @ my-app ---
SLF4J(W): No SLF4J providers were found.
SLF4J(W): Defaulting to no-operation (NOP) logger implementation
SLF4J(W): See https://www.slf4j.org/codes.html#noProviders for further details.
CLIENT BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  2.180 s
[INFO] Finished at: 2024-02-12T22:21:02+05:30
[INFO] ------------------------------------------------------------------------

fabric8-issue-2 (using Fabric8 Kubernetes Client 6.3.1)

fabric8-issue-2 : $ mvn exec:java -Dexec.mainClass=com.mycompany.app.App
[INFO] Scanning for projects...
[INFO] 
[INFO] ----------------------< com.mycompany.app:my-app >----------------------
[INFO] Building my-app 1.0-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- exec:3.1.1:java (default-cli) @ my-app ---
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
CLIENT BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  2.043 s
[INFO] Finished at: 2024-02-12T22:22:28+05:30
[INFO] ------------------------------------------------------------------------

rohanKanojia · 2024-02-13T07:13:29Z

@jmarti44 : ~~I also tried reproducing on MacOS environment using GitHub action but couldn't reproduce it ( I uploaded your reproducer repository to this github project).~~

Could you please check what I might be missing?

edit: This can only be reproduced in a FIPS enabled environment.

rohanKanojia · 2024-02-13T18:18:38Z

@jmarti44 : I tried your project by deploying it on a Red Hat OpenShift FIPS complaint cluster.

I deployed your application to OpenShift cluster using OpenShift Maven Plugin.
However, I'm facing NoClassDefFoundError in both fabric8-issue 'fabric8-issue 2' sample projects

Along with a FIPS complaint cluster, you also need a cluster that uses Elliptic Curve keys

rohanKanojia · 2024-02-15T16:39:14Z

The problem reported in #5296 is caused due to a mix of how CertUtils.handleECKey is written and user's project configuration.

The user is relying in these bouncycastle dependencies:

These dependencies contain all classes used in CertUtils.handleECKey except org.bouncycastle.jce.provider.BouncyCastleProvider (which comes from org.bouncycastle:bcprov-jdk18on)

Upon debugging this, I noticed that NoClassDefFoundError is thrown even when the if statement in this block is evaluated as false

User sets Security.addProvider(new BouncyCastleFipsProvider()) before initializing KubernetesClient, so Security.getProvider("BCFIPS") == null evaluates to false

kubernetes-client/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/internal/CertUtils.java

Lines 186 to 188 in e18b463

    
           if (Security.getProvider("BC") == null && Security.getProvider("BCFIPS") == null) { 
        
             Security.addProvider(new org.bouncycastle.jce.provider.BouncyCastleProvider()); 
        
           }

When Security.addProvider(new org.bouncycastle.jce.provider.BouncyCastleProvider()) is moved inside a Callable, no NoClassDefFoundError is thrown.

          if (Security.getProvider("BC") == null && Security.getProvider("BCFIPS") == null) {
            new Callable<String>() {
              @Override
              public String call() {
                Security.addProvider(new org.bouncycastle.jce.provider.BouncyCastleProvider());
                return null;
              }
            }.call();
          }

If I move this Callable block outside if block, I start getting NoClassDefFound for org/bouncycastle/jce/provider/BouncyCastleProvider

I checked by adding debug statements, control does not go inside the if block; it goes to next line where keyInputStream is read into pomObject and returned as PemKeyPair:

kubernetes-client/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/internal/CertUtils.java

Line 189 in e18b463

    
           Object pemObject = new PEMParser(new InputStreamReader(keyInputStream)).readObject();

manusa · 2024-02-16T05:57:11Z

Yes, as we discussed internally, this fix is basically swallowing the NoClassDefFound exception and moving on to the next statements.

We should try to make the if-statement comply with the valid user setup instead.

+ Add a comment for the reasoning behing moving Security.addProvider into a Callable block + Add a test inside a separate module using `bc-fips` and `bcpkis-fips` that reproduces the issue Signed-off-by: Rohan Kumar <[email protected]>

sonarcloud · 2024-02-16T17:29:32Z

Quality Gate passed

Issues
1 New issue

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

jmarti44 requested review from manusa, oscerd, rohanKanojia, shawkins and sunix as code owners July 5, 2023 21:28

sunix reviewed Jul 19, 2023

View reviewed changes

rohanKanojia force-pushed the iss_5296 branch from b86a67c to 6b12573 Compare February 13, 2024 06:58

proposed fix for JCPEM FIPS provider bug

4f19dee

rohanKanojia force-pushed the iss_5296 branch 2 times, most recently from e87e03a to 738651c Compare February 16, 2024 16:10

rohanKanojia changed the title ~~proposed fix for JCPEM FIPS provider bug~~ Move Security.addProvider(new BouncyCastleProvider()) into a Callable block to avoid NoClassDefFoundError Feb 16, 2024

rohanKanojia force-pushed the iss_5296 branch from 738651c to 766c08d Compare February 16, 2024 17:02

manusa approved these changes Feb 19, 2024

View reviewed changes

manusa added this to the 6.11.0 milestone Feb 19, 2024

manusa merged commit 2f11261 into fabric8io:main Feb 19, 2024
17 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `Security.addProvider(new BouncyCastleProvider())` into a `Callable` block to avoid NoClassDefFoundError #5312

Move `Security.addProvider(new BouncyCastleProvider())` into a `Callable` block to avoid NoClassDefFoundError #5312

jmarti44 commented Jul 5, 2023 •

edited by rohanKanojia

Loading

rohanKanojia commented Jul 6, 2023

rohanKanojia commented Jul 6, 2023

jmarti44 commented Jul 6, 2023

sonarcloud bot commented Jul 6, 2023

sunix Jul 19, 2023

shawkins Jul 19, 2023

rohanKanojia commented Feb 12, 2024 •

edited

Loading

rohanKanojia commented Feb 13, 2024 •

edited

Loading

rohanKanojia commented Feb 13, 2024 •

edited

Loading

rohanKanojia commented Feb 15, 2024

manusa commented Feb 16, 2024

sonarcloud bot commented Feb 16, 2024

Move Security.addProvider(new BouncyCastleProvider()) into a Callable block to avoid NoClassDefFoundError #5312

Move Security.addProvider(new BouncyCastleProvider()) into a Callable block to avoid NoClassDefFoundError #5312

Conversation

jmarti44 commented Jul 5, 2023 • edited by rohanKanojia Loading

Description

Type of change

Checklist

rohanKanojia commented Jul 6, 2023

rohanKanojia commented Jul 6, 2023

jmarti44 commented Jul 6, 2023

sonarcloud bot commented Jul 6, 2023

sunix Jul 19, 2023

Choose a reason for hiding this comment

shawkins Jul 19, 2023

Choose a reason for hiding this comment

rohanKanojia commented Feb 12, 2024 • edited Loading

rohanKanojia commented Feb 13, 2024 • edited Loading

rohanKanojia commented Feb 13, 2024 • edited Loading

rohanKanojia commented Feb 15, 2024

manusa commented Feb 16, 2024

sonarcloud bot commented Feb 16, 2024

Quality Gate passed

Move `Security.addProvider(new BouncyCastleProvider())` into a `Callable` block to avoid NoClassDefFoundError #5312

Move `Security.addProvider(new BouncyCastleProvider())` into a `Callable` block to avoid NoClassDefFoundError #5312

jmarti44 commented Jul 5, 2023 •

edited by rohanKanojia

Loading

rohanKanojia commented Feb 12, 2024 •

edited

Loading

rohanKanojia commented Feb 13, 2024 •

edited

Loading

rohanKanojia commented Feb 13, 2024 •

edited

Loading