[improve][broker, client] Fixing BC FIPS based payload encryption#2
Open
z-kovacs wants to merge 19 commits into
Open
[improve][broker, client] Fixing BC FIPS based payload encryption#2z-kovacs wants to merge 19 commits into
z-kovacs wants to merge 19 commits into
Conversation
- adding bc-common module to contain an interface (BcVersionSpecificUtility) which will provide a facade for FIPS and non-FIPS implementations - bouncy-castle-common (bc-common) module will be a dependency of bouncy-castle-bc and bouncy-castle-bcfips - MessageCryptoBc: moving private key and public key loading to bc version-specific (FIPS or non-FIPS) utility classes - MessageCryptoBc: moving data key encryption and decryption to bc version-specific (FIPS or non-FIPS) utility classes
- bouncy-castle-common following the dependency pattern used across pulsar (broker and client-original is setting up dependency, everywhere else is provided)
…d encrypt functionality respectively (using wrap would be the right way probably, but in non-FIPS mode ECIES cypher cannot wrap - keeping as is for backwards compatibility)
15 tasks
added 12 commits
June 8, 2023 16:04
- using apache commons codec for hexadecimal to/from conversion - fixing typo - remove unnecessary dependency - remove unnecessary LICENSE file - fixing bcfips specific implementation package to match with non-fips + renaming BC FIPS specific encryption test to represent the test better and 'bcfips' should not be only in module name
…ariable, plus adding final
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes apache#20373
Motivation
If we configure pulsar following the doc to use BouncyCastle FIPS library instead of the default non-FIPS version, we receive a
ClassNotFoundException: org.bouncycastle.jce.provider.BouncyCastleProvider- as the MessageCryptoBc implementation is not prepared to use the FIPS library API (way more restrictive).BC FIPS has some restrictions:
Modifications
I have lifted out the BC FIPS and non-FIPS related code from
MessageCryptoBcinto the already existingbouncy-castle-bcandbouncy-castle-bcfipsmodule, and created abouncy-castle-commonmodule with an interface definition and a dynamic loader of the implementation based on the security provider in classpath.MessageCryptoBcnow accessing the BC version specific crypto functionality via the dynamically loaded delegate.in non-FIPS mode we use encrypt mode of cyphers to encrypt data key (probably should be wrap, but ECIES cypher does not support wrap).
In FIPS mode you have to use wrap. So as of now it is not possible to unify code to wrap the data key.
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Note:
we have to warn users if they want to change the client to use BC FIPS client lib, the messages produced with non FIPS setup and where the master key was ECDSA key - won't be readable! (FIPS has way less enabled cyphers.)
we have to add to website to BC instructions, that you can mix fips ad non-fips producers and consumers as long as you use RSA master keys, since ECDSA keys cannot be used to wrap keys in BC FIPS.
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: