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

FIPS checks should not be bootstrap checks #34772

Closed
jaymode opened this issue Oct 23, 2018 · 10 comments · Fixed by #47499
Closed

FIPS checks should not be bootstrap checks #34772

jaymode opened this issue Oct 23, 2018 · 10 comments · Fixed by #47499
Labels
>refactoring :Security/Security Security issues without another label

Comments

@jaymode
Copy link
Member

jaymode commented Oct 23, 2018

We currently have FIPS bootstrap checks, but really these do not fit as a bootstrap check since they are always enforced. We should refactor these checks into a FIPSChecks class that can be instantiated on startup. cc @jasontedor

@jaymode jaymode added help wanted adoptme >refactoring :Security/Security Security issues without another label labels Oct 23, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jasontedor
Copy link
Member

Yes!

@jkakavas jkakavas self-assigned this Oct 23, 2018
@jkakavas
Copy link
Member

I let that slip after 6.4 ,I'll be taking it up shortly

@jkakavas jkakavas removed the help wanted adoptme label Nov 14, 2018
@LuisAlvelaMendes
Copy link

I'm not so sure I understand this issue .. I presume you're talking about classes like " FIPS140JKSKeystoreBootstrapCheck" and so forth, the ones called "FIPS140(..)". The idea would be to refactor them as subclasses of a FIPSChecks, changing their name but leaving all of their functionality the same?

Here's a somewhat illustrative diagram:
(I removed the FIPS140 repetition for subclasses and also the designation "Bootstrap".
org elasticsearch xpack security_

@jkakavas
Copy link
Member

Hi @LuisAlvelaMendes, thank you for your interest in this issue. The idea is that we want to remove all the FIPS 140 related checks (the ones you have correctly identified above) from being Bootstrap Checks as the FIPS checks are always enforced. FIPSChecks will not implement BootstrapCheck

In the benefit of not duplicating effort, note that I removed the help_wanted label a few days ago because I have this on my plate for one of the next minor releases. That said, I haven't actually started working on it yet so if this is something that interests you, I'd gladly work with you to get this merged if you still want to work on it

@LuisAlvelaMendes
Copy link

@jkakavas I'd like to work with you on this issue, yes,

@LuisAlvelaMendes
Copy link

@jkakavas I understand now that FIPSChecks will require it's own FIPSInterface - however, my problem is understanding "BootstrapContext" better.

I suppose these FIPS 140 related checks will have to receive a FIPSContext to make decisions on instead of a BootstrapContext - I simply don't understand what the difference between a FIPS context and a Bootstrap one is, won't they both need the settings and metadata of the node?

@jkakavas
Copy link
Member

jkakavas commented Dec 4, 2018

Hi @LuisAlvelaMendes , apologies for the late reply. This fell through the cracks of my to-do list.

There is a conceptual difference. Bootstrap checks are meant to be enforced only on production mode ( not on development mode ) and thus adding the FIPS checks as Bootstrap checks in the beginning was an abuse of the interface ( as FIPS checks should always be enforced ).
You can make decisions for the FIPS checks by looking at the settings only.

A fitting place to call the checks is in createComponents of the Security plugin

@LuisAlvelaMendes
Copy link

LuisAlvelaMendes commented Dec 5, 2018

@jkakavas What about the license check? Where it reads:

License license = LicenseService.getLicense(context.metaData);

if (license != null && ALLOWED_LICENSE_OPERATION_MODES.contains(license.operationMode()) == false) {
return FIPSCheckResult.failure("FIPS mode is only allowed with a Platinum or Trial license");
}

Doesn't that require the context's metadata?

@jkakavas
Copy link
Member

@LuisAlvelaMendes

We can use getLicenseState() from Security.java to get an XPackLicenseState and implement an isFipsAllowed() in there

@jkakavas jkakavas removed their assignment Sep 10, 2019
jkakavas added a commit to jkakavas/elasticsearch that referenced this issue Oct 3, 2019
FIPS 140 bootstrap checks should not be bootstrap checks as they
are always enforced. This commit moves the validation logic within
the security plugin.
The FIPS140SecureSettingsBootstrapCheck was not applicable as the
keystore was being loaded on init, before the Bootstrap checks
were checked, so an elasticsearch keystore of version < 3 would
cause the node to fail in a FIPS 140 JVM before the bootstrap check
kicked in, and as such hasn't been migrated.

Resolves: elastic#34772
jkakavas added a commit that referenced this issue Oct 22, 2019
FIPS 140 bootstrap checks should not be bootstrap checks as they
are always enforced. This commit moves the validation logic within
the security plugin.
The FIPS140SecureSettingsBootstrapCheck was not applicable as the
keystore was being loaded on init, before the Bootstrap checks
were checked, so an elasticsearch keystore of version < 3 would
cause the node to fail in a FIPS 140 JVM before the bootstrap check
kicked in, and as such hasn't been migrated.

Resolves: #34772
jkakavas added a commit to jkakavas/elasticsearch that referenced this issue Oct 22, 2019
FIPS 140 bootstrap checks should not be bootstrap checks as they
are always enforced. This commit moves the validation logic within
the security plugin.
The FIPS140SecureSettingsBootstrapCheck was not applicable as the
keystore was being loaded on init, before the Bootstrap checks
were checked, so an elasticsearch keystore of version < 3 would
cause the node to fail in a FIPS 140 JVM before the bootstrap check
kicked in, and as such hasn't been migrated.

Resolves: elastic#34772
jkakavas added a commit that referenced this issue Oct 22, 2019
FIPS 140 bootstrap checks should not be bootstrap checks as they
are always enforced. This commit moves the validation logic within
the security plugin.
The FIPS140SecureSettingsBootstrapCheck was not applicable as the
keystore was being loaded on init, before the Bootstrap checks
were checked, so an elasticsearch keystore of version < 3 would
cause the node to fail in a FIPS 140 JVM before the bootstrap check
kicked in, and as such hasn't been migrated.

Resolves: #34772
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>refactoring :Security/Security Security issues without another label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants