Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support FIPS mode in kubernetes-client with BouncyCastleFipsProvider #2788

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

gulyaev13
Copy link
Contributor

@gulyaev13 gulyaev13 commented Feb 5, 2021

Support FIPS mode in kubernetes-client with BouncyCastleFipsProvider сonnected externally.
Solving the issue described in #2732

Description

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

@centos-ci
Copy link

Can one of the admins verify this patch?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 5, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

28.6% 28.6% Coverage
0.0% 0.0% Duplication

@@ -145,7 +145,7 @@ private static PrivateKey handleECKey(InputStream keyInputStream) throws IOExcep
@Override
public PrivateKey call() {
try {
if (Security.getProvider("BC") == null) {
if (Security.getProvider("BC") == null && Security.getProvider("BCFIPS") == null) {
Security.addProvider(new org.bouncycastle.jce.provider.BouncyCastleProvider());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you be adding the BouncyCastleFipsProvider here if it's not found, instead of the regular BC provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Bouncy Castle FIPS Java API User Guide security provider can be added in java.security file.

I chose to support only the external configuration due to the following reasons:

  • Additional flag for FIPS mode isn't required
  • No need to add dependency with BC FIPS and update versions
  • Adding FIPS provider at the state of initialization of Kubernetes client is too late. So much code can be executed in an application using kubernetes-client. Initialization on startup of JVM is preferable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, this statement prevents the the BouncyCastleProvider to be added if you configured your JVM to use the BCFIPS provider, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, these changes should be harmless in case no BC configuration is provided, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, still not convinced… Doesn't this mean that someone might be requesting the FIPS provider and actually getting the regular one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manusa , yes! Changes should be harmless in case no BC configuration is provided.
There are 4 cases:

  1. None of the BouncyCastle security providers configured in JVM -> BC will be added in runtime
  2. BC configured in JVM -> BC will not be added in runtime
  3. BCFIPS configured in JVM -> BC will not be added in runtime
  4. Another FIPS provider configured in JVM (is it exist?) -> BC will be added in runtime

@metacosm , looks like you are asking about case №4, aren't you?
I see 2 solutions:

  1. The additional flag can be added to prevent adding BC provider in case if someone would like to use an external provider.
  2. We can say that kubernetes-client works only with the family of BouncyCastle security providers (BC/BCFIPS).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't understand is why we add the BC provider when the user asks for BCFIPS and not the BouncyCastleFipsProvider? How will that address the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metacosm , because there is no option in kubernetes-client configuration for add BouncyCastleFipsProvider(BCFIPS) instead of BouncyCastleProvider(BC).

But if the user will configure JVM for use BouncyCastleFipsProvider(BCFIPS) kubernetes-client will not add BouncyCastleProvider(BC). See case №3 in my previous message.

@manusa manusa added this to the 5.1.0 milestone Feb 9, 2021
@gulyaev13
Copy link
Contributor Author

@oscerd , hello!
If my PR will be approved what should I do to backport changes to kubernetes-client 4.11.?
I've planned to add these changes in Jenkins plugin with Kubernetes API and looks like they are still using 4.11.
releases. Updating the Jenkins plugin to the 5.1.0 release seems a big project for this small patch.
https://github.com/jenkinsci/kubernetes-client-api-plugin/blob/master/pom.xml#L16

@oscerd
Copy link
Member

oscerd commented Feb 12, 2021

I don't think 4.11 is an active branch, only for critical CVE.

@gulyaev13
Copy link
Contributor Author

@oscerd , understood, thanks!
I will try to propose an update of kubernetes-client version in Jenkins plugins to 5 and more.

@manusa manusa modified the milestones: 5.1.0, 5.2.0 Feb 17, 2021
@manusa manusa modified the milestones: 5.2.0, 5.3.0 Mar 11, 2021
@gulyaev13
Copy link
Contributor Author

@manusa , hello!
Are there some blockers for merge?

@manusa
Copy link
Member

manusa commented Mar 13, 2021

Hi @gulyaev13,
Everything is fine with the PR. We're waiting on a security team to double check everything is OK (unfortunately it's taking longer than expected 😓, sorry).

@DimoDonchev
Copy link

@manusa , hello!
Is there any update on the double checking from the security team?

@manusa manusa modified the milestones: 5.4.0, 5.3.0 Apr 7, 2021
@manusa
Copy link
Member

manusa commented Apr 7, 2021

Hi @DimoDonchev, sorry for the delay. We'll try to include this in our upcoming 5.4.0 release.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

28.6% 28.6% Coverage
0.0% 0.0% Duplication

@manusa manusa modified the milestones: 5.3.0, 5.4.0 Apr 8, 2021
@manusa manusa merged commit 0c5a49b into fabric8io:master Apr 27, 2021
@manusa manusa added changelog missing A line to changelog.md regarding the change is not added and removed changelog missing A line to changelog.md regarding the change is not added labels Apr 27, 2021
@oleg-nenashev
Copy link

Hi @manusa . Is there a ETA for the 2.4.0 release? We use this library in Jenkins Kubernetes Plugin and in a few developer tools. It would be great to hqve a release. Please let me know if any help needed

@manusa
Copy link
Member

manusa commented May 12, 2021

Hi @manusa . Is there a ETA for the 2.4.0 release? We use this library in Jenkins Kubernetes Plugin and in a few developer tools. It would be great to hqve a release. Please let me know if any help needed

I guess you mean 5.4.0. Release is imminent, we are long overdue our initial ETA. Not sure if we'll be able to deliver by the end of the week, but our intention is to release ASAP.

@oleg-nenashev
Copy link

oleg-nenashev commented May 12, 2021 via email

@FuyaoLi2017
Copy link

@gulyaev13 @manusa Hello Team, I am using Apache Flink 1.13.0 and Flink references the 4.9.2 version of the Fabric8 kubernetes client. I am writing an kubernetes operator for Flink and 5.4.0 is not compatible with Flink since the APIs changes a lot between these two versions because I need to call some Flink internal methods which is actually using 4.9.2 Fabric library.

However, I still want to make Fabric8 version 4.9.2 FIPS compliant, is it possible to do to monkey patch for such changes?

@manusa
Copy link
Member

manusa commented Jun 10, 2021

I don't know how heavily Apache Flink relies on the client, but it would make sense to update their dependency.
4.9.2 was released almost a year ago, so it might be worthier to spend the time upgrading Flink to use the latest release of the Fabric8 client instead (as I said, depending on how complex this turn out to be).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants