-
Notifications
You must be signed in to change notification settings - Fork 27
Rebase to master hash, #334
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
Conversation
Fix hpke_test errors
0e98007 to
87539b3
Compare
patches/000-fips.patch
Outdated
| ) | ||
|
|
||
| func Example() { | ||
| + if boring.Enabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change required or can we update the test using it instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function Example() was added recently and it appears that it can be called directly, and looks like skipping it, might need us to convert it to a test Is that what you are proposing here? @derekparker
updated hash to 509ddf38689c10643d89c464e8386f53364635e8 Addressed review comments by removing changes to pq.go and retaining most changes to test files
derekparker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
However, I'd like a second look from @dbenoit17 and @ueno if possible.
|
Hi @dbenoit17 @ueno can you please review the patch for rebasing to the latest hash, there were a few new calls introduced in this cycle among other things such as Example() which was causing an infinite loop as it was presumably working on test inputs not meant to be used in Fips only mode. |
|
If i dont exit Example() early checking on FIPS mode the program hangs when i run |
ueno
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is getting into a loop here, given X25519 key generation always returns error in FIPS mode. I agree that it should be safe to skip the test (assuming no existing user of HPKE). It might make sense to check that and return an error in kem.NewPrivateKey, though I suspect other parts of crypto/hpke need more adjustment for OpenSSL backed FIPS mode.
And an implementation to allow bypass of X25519 under MLKEM is underway so till then skip the TestVectors() Convert Example() to a test so that it can be skipped as well
dbenoit17
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
Notes from discussion with David and Daiki: The underlying technical reason for the skipping and issues is FIPS 140 compliance: The native FIPS backend already supports the MLKEM768X25519 hybrid KEM, but there's a bug that rejects it in fips140=only mode. An in-progress PR (commit 86bbea0cfa72...) is adding an override mechanism to fix this issue in the future, but it's not yet implemented. The current OpenSSL-based FIPS backend (which is being tested here) does not yet support MLKEM768 at all, leading to these testing problems. Also neither fips140=on nor fips140=only is currently recommended for use as neither is yet NIST certified. |
while applying earlier patch, the code wrt hpke had changed quite a bit in the new master branch so i removed the earlier patch first and tried to run and got the errors to confirm its the same errors corresponding to the now absent patch and not anything new, this time i got an additional time out error in the same test
Later i fed it to claude and asked it to compare with earlier patches to come up with a fix around skipping X25519 in Fips mode and avoiding the time out issue
and this is what it came up with:
The issue had two components:
that attempted to create a curve private key. In FIPS mode, X25519 is not allowed, so
kem.curve.NewPrivateKey(seedT) always returns an error, causing the loop to run forever and hang the
test.
IDs 0x0020 and 0x647a) didn't handle FIPS mode restrictions, causing failures or hangs.
Changes Made
Following the established pattern from the 000-fips.patch file for handling X25519 in FIPS mode:
- Added imports for boring (crypto/internal/backend) and strings
- Modified Example() function to skip execution and print expected output when in FIPS mode (since
it uses MLKEM768X25519 which contains X25519)
- Added check in testVectors() to skip tests for X25519-based KEMs (0x0020 and 0x647a) when the
expected error occurs in FIPS mode
- Changed infinite loop to limited loop with maxAttempts = 256 (sufficient for rejection sampling)
- Added fallback logic to return the actual error if all attempts fail (e.g., when FIPS mode
consistently rejects the curve)
Test Results
✅ All tests pass in FIPS mode: GOLANG_FIPS=1 OPENSSL_FORCE_FIPS_MODE=1 go test -v
✅ All tests pass in normal mode: go test -v
✅ No timeouts or hangs
✅ X25519-based tests are properly skipped in FIPS mode
✅ FIPS-compliant tests (P-256, P-384, P-521, ML-KEM) all pass
───────