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.
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
Refactored FIPS Checks into a Single Class (For Issue #34772) #36677
Refactored FIPS Checks into a Single Class (For Issue #34772) #36677
Changes from 6 commits
b824b97
0f7dc1a
5cc55d3
51a126d
7a4e6ff
aab85e4
aa2d09c
e124f8a
f8004cf
2a29fea
a071e40
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't think we actually need a
FIPSContext
class since we only encapsulate the settings here ( as opposed to i.e. aBootstrapContext
that encapsulates settings and metadata. )This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
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.
It's better to use variable names with context so for example
check1
could bekeystoreCheck
, etc.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.
we should probably consolidate the error messages from the results so that we don't only present the first (from a seemingly arbitrary check order) error that was encountered to the user
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.
How do you suggest we do it then?
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.
I was thinking something similar to how we use addValidationError
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.
Depending on other changes/suggestions, we should probably conditionally apply the FipsChecks if FIPS_MODE_ENABLED already in
Security.java
so that we don't have to check the settings value each time.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.
Do you mean add an if clause like
if (XPackSettings.FIPS_MODE_ENABLED.get(context.settings)) {}
surrounding it? I'm not sure what impact that would haveThere 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.
I meant that we can have the if clause once in
Security.java
and callcheck()
on all the checks (or the onecheck()
if we decide to implementFipsChecks
in a simpler manner ) IFfips_mode
is set