FIPS Compliance

[artoonie]

Our RSA validation algorithm needs to be FIPS Compliant. Bouncycastle is compliant (certificate #4616) but only up to Java 17.

Luckily 1.3.2 uses Java 17, but 1.4.x will not, so we'll need to figure something out then.

In the meantime, there are two methods that are not FIPS compliant:

1. The psuedo-random number generator
2. The XML canonicalization library

(2) is not directly related to cryptography, and is likely a non-issue. We will need to determine if (1) is an issue. In the meantime, this is ready for review @tarheel @HEdingfield.

[tarheel]

I have basically zero context on this FIPS compliance work. Will let @HEdingfield look also.

Regarding the random number generation code that we already had: I believe it's only relevant to the tie-breaking logic. There's a longstanding unresolved discussion about making it more secure (see #170, for example). I can't remember what's left to do there...

[artoonie]

The random number generator is needed by generatePublicKey, and is referenced here: https://github.com/BrightSpots/rcv/pull/765/files#diff-3f597618fe0497e6e5c6b9d51ed6ab1071e5a7d3e6ca438f022417ff5fe3382dR88

I don't know why it's being used, and have asked here: bcgit/bc-java#1520

However, it is used within the cryptography process.

The context is basically what you've gathered: we've been informed that we need to be "FIPS Compliant," and using the bouncycastle library is a software-only solution that has been federally certified. The default libraries are not certified, and despite some arguments that they are more secure, they do not have the FIPS certification.

[yezr]

Following some discussions with folks that have certification related FIPs experience, for signature verification FIPS 140-2 compliance we are going to move forward with the BouncyCastle implementation and call out the missing FIPS certified random number implementation with the following in the TDP

RCTab uses #4616 BC-FJA (Bouncy Castle FIPS Java API), a Level 1 FIPS 140-2 Validated Cryptographic Module to verify the cryptographic signature of Hart Verity Signed CVRs.

Note: We have BouncyCastle implemented as a provider, but we still use a non-FIPS-certified Sun implementation of a random number generator. The random number generator is only used to validate that the chosen public key is valid -- it's used as part of a primality-checking algorithm. Since we know that the public key we're given is already valid (as it is created by Hart Verity using their FIPS 140-2 certified implementation), this check is superfluous.

[HEdingfield]

I see this is being targetted to rsa-validation-onto-1.3.2 -- do we not want to include this in 1.4.0 as well?

[HEdingfield]

I see this is being targetted to rsa-validation-onto-1.3.2 -- do we not want to include this in 1.4.0 as well?

Just saw the note in the description. That's unfortunate... are there no other options besides Bouncycastle at this point?

If not, can we at least create a new issue (or note in an existing one) that this divergence between 1.3.2 and 1.4.0 needs to be reconciled?

[artoonie]

@HEdingfield Yes, we do need to reconcile this for 1.4.0, either by researching whether bouncycastle will soon have a new FIPS certification for the latest version of Java, or by determining we need to downgrade Java, or we need to replace bouncycastle with something else.

@yezr can you make an issue for 1.4.0 for this problem?

[yezr]

created #767 to research a way to reconcile this PR in 1.4.0

[HEdingfield]

I'm super annoyed that this diff doesn't seem to be working properly... this file already exists in hotfix/1.3.2 because it was created by #764, which has now been merged in. I've tried changing the base back and forth in GitHub. This isn't really reviewable as-is. Any idea how to fix this problem?

[artoonie]

This is the issue caused by squash commits + chained PRs that we noted a while back.

My workflow is to cherry-pick only the relevant commits and force-push to the branch, e.g.:

Base < Feature1 [commit1, commit2] < Feature2 [commit3, commit4]
When Feature1 merges into Base:

git checkout Feature2
git reset --hard Base
git cherrypick commit3..commit4
git push --force

[HEdingfield]

Is it worth adding a section to the Wiki about this, or will you just keep it on your end as domain knowledge, since it seems to be specific to your workflow (unless I'm misunderstanding)?

[artoonie]

Can do: https://github.com/BrightSpots/rcv/wiki/Notes-on-Development

Maybe worth adding the IntelliJ setup you use to this wiki too (I think it's on the README now?)

[HEdingfield]

Follow-up on this: could this have been avoided if, in your local branch and after the first squash was merged into the main branch, you pulled back from the main branch (with the new squash merge) into your local branch and reconciled changes? I believe this is what I've historically done in these situations, but there might be some subtle difference I'm missing.

[artoonie]

Yes, that's what I used to do, but reconciling differences is manual and error-prone, especially if there are dozens. Bugs have slipped in before caused by this (e.g. #670), so I don't recommend it.

This method is less error-prone.

[HEdingfield]

Ok cool. It's generally pretty painless with IntelliJ -- I'll mention that method in the Wiki as well.

[HEdingfield]

Filed #771 for updating Wiki with IntelliJ info.

[artoonie]

Merging now to start building the release candidate but still hoping for approval after-the-fact @HEdingfield

[HEdingfield]

So I guess this is something that gets set up (i.e. all of SecurityConfig is run) when the app initializes, but it only matters / gets checked during Hart tabulation?

[artoonie]

Yep, though I'd maybe use the term verified over matters, in that it may affect other places in the code (and some may take "matters" to mean "affects")