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

Enable FIPS140LicenseBootstrapCheck #32903

Merged
merged 2 commits into from
Aug 17, 2018

Conversation

jkakavas
Copy link
Member

This commit ensures that xpack.security.fips_mode.enabled: true cannot be set in a node that doesn't have the appropriate license. The bootstrap check was introduced in #32437 but was not enabled.

@jkakavas jkakavas added >bug v7.0.0 :Security/Security Security issues without another label v6.4.0 labels Aug 16, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM. I did not notice this previously, but all of these bootstrap checks should be using the settings from the context rather than passing constructor parameters in derived from settings. For example, this is a patch for this bootstrap check:

diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPS140LicenseBootstrapCheck.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPS140LicenseBootstrapCheck.java
index d1bce0dcdd2..0b98cd7479b 100644
--- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPS140LicenseBootstrapCheck.java
+++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPS140LicenseBootstrapCheck.java
@@ -10,6 +10,7 @@ import org.elasticsearch.bootstrap.BootstrapCheck;
 import org.elasticsearch.bootstrap.BootstrapContext;
 import org.elasticsearch.license.License;
 import org.elasticsearch.license.LicenseService;
+import org.elasticsearch.xpack.core.XPackSettings;
 
 import java.util.EnumSet;
 
@@ -21,15 +22,9 @@ final class FIPS140LicenseBootstrapCheck implements BootstrapCheck {
     static final EnumSet<License.OperationMode> ALLOWED_LICENSE_OPERATION_MODES =
         EnumSet.of(License.OperationMode.PLATINUM, License.OperationMode.TRIAL);
 
-    private final boolean isInFipsMode;
-
-    FIPS140LicenseBootstrapCheck(boolean isInFipsMode) {
-        this.isInFipsMode = isInFipsMode;
-    }
-
     @Override
-    public BootstrapCheckResult check(BootstrapContext context) {
-        if (isInFipsMode) {
+    public BootstrapCheckResult check(final BootstrapContext context) {
+        if (XPackSettings.FIPS_MODE_ENABLED.get(context.settings)) {
             License license = LicenseService.getLicense(context.metaData);
             if (license != null && ALLOWED_LICENSE_OPERATION_MODES.contains(license.operationMode()) == false) {
                 return BootstrapCheckResult.failure("FIPS mode is only allowed with a Platinum or Trial license");
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
index 388a1a25896..d6f7283dd1a 100644
--- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
+++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
@@ -303,7 +303,7 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw
                 new FIPS140SecureSettingsBootstrapCheck(settings, env),
                 new FIPS140JKSKeystoreBootstrapCheck(settings),
                 new FIPS140PasswordHashingAlgorithmBootstrapCheck(settings),
-                new FIPS140LicenseBootstrapCheck(XPackSettings.FIPS_MODE_ENABLED.get(settings))));
+                new FIPS140LicenseBootstrapCheck()));
             checks.addAll(InternalRealms.getBootstrapChecks(settings, env));
             this.bootstrapChecks = Collections.unmodifiableList(checks);
             Automatons.updateMaxDeterminizedStates(settings);

As a follow-up, can you address this for all the FIPS bootstrap checks?

@jkakavas
Copy link
Member Author

As a follow-up, can you address this for all the FIPS bootstrap checks?

Sure, on it.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM

@jkakavas
Copy link
Member Author

Jenkins test this please

@jkakavas
Copy link
Member Author

Jenkins test this please

@tvernum
Copy link
Contributor

tvernum commented Aug 17, 2018

all of these bootstrap checks should be using the settings from the context

I think we've fixed most of the cases where this matters (via #30953), but this doesn't work for secure settings. By the time the check runs, the es-keystore is closed, so you need to read the value from Settings during construction.

@jkakavas jkakavas merged commit 75014a2 into elastic:master Aug 17, 2018
jkakavas added a commit that referenced this pull request Aug 17, 2018
This commit ensures that xpack.security.fips_mode.enabled: true 
cannot be set in a node that doesn't have the appropriate license.
jkakavas added a commit that referenced this pull request Aug 17, 2018
This commit ensures that xpack.security.fips_mode.enabled: true 
cannot be set in a node that doesn't have the appropriate license.
jasontedor added a commit that referenced this pull request Aug 18, 2018
* elastic/master: (46 commits)
  NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (#32764)
  [DOCS] Splits the users API documentation into multiple pages (#32825)
  [DOCS] Splits the token APIs into separate pages (#32865)
  [DOCS] Creates redirects for role management APIs page
  Bypassing failing test PainlessDomainSplitIT#testHRDSplit (#32966)
  TEST: Mute testRetentionPolicyChangeDuringRecovery
  [DOCS] Fixes more broken links to role management APIs
  [Docs] Tweaks and fixes to rollup docs
  [DOCS] Fixes links to role management APIs
  [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature
  [DOCS] Splits the roles API documentation into multiple pages (#32794)
  [TEST]  Run pre 6.4 nodes in non-FIPS JVMs (#32901)
  Make Geo Context Mapping Parsing More Strict (#32821)
  [ML] fix updating opened jobs scheduled events (#31651) (#32881)
  Scripted metric aggregations: add deprecation warning and system property to control legacy params (#31597)
  Tests: Fix timezone conversion in DateTimeUnitTests
  Enable FIPS140LicenseBootstrapCheck (#32903)
  Fix InternalAutoDateHistogram reproducible failure (#32723)
  Remove assertion in testDocStats on deletedDocs counter (#32914)
  HLRC: Move ML request converters into their own class (#32906)
  ...
jasontedor added a commit that referenced this pull request Aug 18, 2018
* 6.x: (42 commits)
  [DOCS] Splits the users API documentation into multiple pages (#32825)
  [DOCS] Splits the token APIs into separate pages (#32865)
  [DOCS] Creates redirects for role management APIs page
  Bypassing failing test PainlessDomainSplitIT#testHRDSplit (#32966)
  TEST: Mute testRetentionPolicyChangeDuringRecovery
  [DOCS] Fixes more broken links to role management APIs
  [Docs] Tweaks and fixes to rollup docs
  [DOCS] Fixes links to role management APIs
  [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature
  [DOCS] Splits the roles API documentation into multiple pages (#32794)
  [TEST]  Run pre 6.4 nodes in non-FIPS JVMs (#32901)
  Remove assertion in testDocStats on deletedDocs counter (#32914)
  [ML] fix updating opened jobs scheduled events (#31651) (#32881)
  Enable FIPS140LicenseBootstrapCheck (#32903)
  HLRC: Move ML request converters into their own class (#32906)
  [DOCS] Update getting-started.asciidoc (#29518)
  Fix allowed value for HighlighterBuilder encoder in javadocs (#32780)
  [DOCS] Add "remove a tag" script logic as an example (#32556)
  RFC: Test that example plugins build stand-alone (#32235)
  Security: remove put privilege API (#32879)
  ...
@jkakavas jkakavas deleted the enable-licensefipsbootstrapcheck branch September 14, 2018 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants