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

[JEP-237] Do not support shortening of HMAC code on FIPS mode #8612

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

SujathaH
Copy link
Contributor

@SujathaH SujathaH commented Oct 16, 2023

HMACConfidentialKey supports an option to return trimmed HMAC code. Usage of trimmed code can cause issues when used from security context. This change is to throw an exception when callers try to get trimmed HMAC code on FIPS mode and also
this change assumes that Legacy Token is disabled on FIPS mode. There is a plan to remove support for Legacy Token and enhancement JENKINS-71573 exists for same. Hence, Legacy Token flag needs to be disabled on FIPS mode for this change to work successfully.

Verified across all the plugins and found there are no plugins which are using the truncated HMAC code.
The only impact is on Legacy Token in Jenkins Core

See JEP-237.

Testing done

Verified exception is thrown when HMAC Key is being created with length less than original length of HAMC code on FIPS mode
Verified trimmed code is returned when HMAC Key is being created with length less than 32 on non FIPS mode

Proposed changelog entries

Prevent trimming HMAC codes (using HAMCConfidentialKey) when running in FIPS mode only.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@jtnord

Before the changes are marked as ready-for-merge:

Maintainer checklist

@olamy olamy added the fips label Oct 16, 2023
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

the unit tests look like they will pass when they should fail.

Also please leave links to any plugins or code that will be impactec by this change in the summary.

@SujathaH SujathaH requested a review from jtnord October 17, 2023 09:40
@jtnord jtnord requested a review from a team October 17, 2023 13:41
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 17, 2023
@jtnord jtnord added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Oct 17, 2023
@Test
public void testTruncatedMacOnNonFips() {
HMACConfidentialKey key1 = new HMACConfidentialKey("test", 16);
String str = key1.mac("Hello World");

Choose a reason for hiding this comment

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

how about make a var as "Hello World"?

@timja timja merged commit 577f427 into jenkinsci:master Oct 19, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fips ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants