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

Extracion of crypto-pgp and making crypto work on FIPS #6241

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

JiriOndrusek
Copy link
Contributor

fixes #6088

Crypto component was split into crypto and crypto-pgp to support FIPS. See the commit. (this is the reason of why this PR is opened against camel-main)

  • crypto-pgp is not registering BC as a provider; component contains only PGPDataFormat
  • crypto contains all other (CryptoDataFormat, components)

How to run crypto on FIPS the BC dependency has to be excluded and BCFIPS added (or another BC implementation for FIPS) .

Limitation: Because of the BCFIPS, it is not possible to use crypto and crypto-pgp together on FIPS system (if BCFIPS is utilized)

This PR contains:

  • new extension based on crypto-pgp component (uses BC). The component and all tests are extracted from the crypto component. All tests work on FIPS system.
  • fixes in bouncycastle-support to work correctly on FIPS system
  • added fips profile in crypto integration test module, to allow execution on FIPS systems
  • some *.adoc addition for FIPS in crypto and crypto-pgp etensions
  • it is not possible to utilize certificate-generator-support because the tests require DES and the certificate-generator-support uses RSA.

@JiriOndrusek
Copy link
Contributor Author

@ppalaga FYI

@aldettinger
Copy link
Contributor

The failure seems merely linked to uncommitted changes.

@aldettinger
Copy link
Contributor

That's quite a pr. Many thanks for transparently explaining the intent in the first note and putting so much useful comment in the code. It really helps one without much fips knowledge to review :)

@JiriOndrusek
Copy link
Contributor Author

@ppalaga

It is still not clear to me what happens when bcprov is excluded from crypto-pgp and replaced with BCFIPS. Have you tried that by any chance?

There might be more problems related to this question. The major one is that bcpg depends on bcprov. Class BcKeyFingerprintCalculator references org.bouncycastle.crypto.Digest; The same class is not part of the bcfips. (I checked the jar downloaded by maven, and you can see it e.g in this fork)

Therefore it is not possible to replace bcprov with bcfips.

  • The only workaround I'm aware of is to have a different bouncycastle fips implementation, which uses different packages. In this case the bcprov and custom_bcfips can coexist in the project and should work together)
  • For illustration, the bcprov package org.bouncycastle contains 11 sub-packages; bcfip's org.bouncycastle contains 2 sub-packages. (So I expect more possible issues.)

Copy link
Contributor

@aldettinger aldettinger left a comment

Choose a reason for hiding this comment

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

@JiriOndrusek The ci failure seems linked to needed regeneration, beyond looks good

@JiriOndrusek
Copy link
Contributor Author

JiriOndrusek commented Jul 8, 2024

@JiriOndrusek The ci failure seems linked to needed regeneration, beyond looks good

Yes, I forgot to regenerate POMS when I was maintaining camel-main, hopefully now it is ok

@ppalaga
Copy link
Contributor

ppalaga commented Jul 9, 2024

@ppalaga

It is still not clear to me what happens when bcprov is excluded from crypto-pgp and replaced with BCFIPS. Have you tried that by any chance?

There might be more problems related to this question. The major one is that bcpg depends on bcprov. Class BcKeyFingerprintCalculator references org.bouncycastle.crypto.Digest; The same class is not part of the bcfips. (I checked the jar downloaded by maven, and you can see it e.g in this fork)

Therefore it is not possible to replace bcprov with bcfips.

Good to know, thanks for the information!

@JiriOndrusek
Copy link
Contributor Author

I switched this PR to draft and I'll rebase to main, as soon as Upgrade to Camel 4.7.0 is merged (#6264)

@JiriOndrusek
Copy link
Contributor Author

JiriOndrusek commented Jul 11, 2024

@ppalaga I applied the idea of crypto not needing BC.
• I excluded BC from camel-crypto (until getting change from Camel) via BOM
• User has to add dependency to BCFIPS and quarkus-security and register BCFIPS provider to make crypto work on FIPS -> I described that in README.adoc
• the tests for crypto extension got simplified

@github-actions github-actions bot force-pushed the camel-main branch 3 times, most recently from 84bb4f0 to 3df5f85 Compare August 2, 2024 01:48
@github-actions github-actions bot force-pushed the camel-main branch 2 times, most recently from 98d71ef to 9587b1a Compare August 5, 2024 01:55
@JiriOndrusek JiriOndrusek changed the base branch from camel-main to main August 5, 2024 12:53
@JiriOndrusek JiriOndrusek marked this pull request as ready for review August 5, 2024 12:54
@JiriOndrusek
Copy link
Contributor Author

The crypto/crypto-pgp changes are present in Camel 3.7.0.
I rebased this PR against main and I think that it could be approved/merged. All the questions and suggestions are answered or fixed.

@JiriOndrusek
Copy link
Contributor Author

@ppalaga WDYT? Can we merge this PR?

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @JiriOndrusek !

@ppalaga ppalaga merged commit ed8156c into apache:main Aug 12, 2024
24 checks passed
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.

Crypto does not work in FIPS
4 participants