Refactor FIPS BootstrapChecks to simple checks #47499

jkakavas · 2019-10-03T10:46:34Z

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
Replaces: #36677

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

elasticmachine · 2019-10-03T10:46:36Z

Pinging @elastic/es-security (:Security/Security)

jasontedor

LGTM.

tvernum · 2019-10-07T02:57:25Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -326,8 +326,11 @@ public Security(Settings settings, final Path configPath) {

    }

-    private static void runStartupChecks(Settings settings) {


Why is this no longer static? validateForFips is static, so I can't see why this method needs to change.

Leftover change from uncompleted attempt I'd guess, wasn't left on purpose

tvernum · 2019-10-07T03:02:24Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+        }
+        Settings keystorePathSettings = settings.filter(k -> k.endsWith("keystore.path"))
+            .filter(k -> settings.hasValue(k.replace(".path", ".type")) == false);
+        // Default Keystore type is JKS in JDK8 and PKCS12 in > JDK9 if not explicitly set


This is not the case. We explicitly set the keystore type to JKS, and don't depend on the JVM default.

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLConfigurationSettings.java

Lines 245 to 252 in 9945d5c

private static String inferKeyStoreType(String path) {

String name = path == null ? "" : path.toLowerCase(Locale.ROOT);

if (name.endsWith(".p12") || name.endsWith(".pfx") || name.endsWith(".pkcs12")) {

return PKCS12_KEYSTORE_TYPE;

} else {

return DEFAULT_KEYSTORE_TYPE;

}

}

Perhaps we should call inferKeyStoreType here instead.

tvernum · 2019-10-07T03:04:39Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -303,19 +307,15 @@ public Security(Settings settings, final Path configPath) {
        this.env = new Environment(settings, configPath);
        this.enabled = XPackSettings.SECURITY_ENABLED.get(settings);
        if (enabled) {
-            runStartupChecks(settings);
+            runStartupChecks(settings, getLicenseState());


Is license state meaningful at this point during node start up?

I see your point. Should we just remove checks for license on node startup and only leave ValidateLicenseForFIPS on node joins ?

tvernum · 2019-10-07T03:05:10Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+        }
+
+        if (licenseState != null && FIPS_ALLOWED_LICENSE_OPERATION_MODES.contains(licenseState.getOperationMode()) == false) {
+            validationErrors.add("FIPS mode is only allowed with a Platinum or Trial license");


The error message should be built from FIPS_ALLOWED_LICENSE_OPERATION_MODES

tvernum · 2019-10-07T03:05:55Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -273,6 +275,8 @@
        DiscoveryPlugin, MapperPlugin, ExtensiblePlugin {

    private static final Logger logger = LogManager.getLogger(Security.class);
+    static final EnumSet<License.OperationMode> FIPS_ALLOWED_LICENSE_OPERATION_MODES =
+        EnumSet.of(License.OperationMode.PLATINUM, License.OperationMode.TRIAL);


Can we move this to XPackLicenseState? We ought to keep licensing decisions centralised.

tvernum · 2019-10-07T03:08:54Z

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java

@@ -253,7 +255,7 @@ public void testJoinValidatorForFIPSLicense() throws Exception {
        new Security.ValidateLicenseForFIPS(false).accept(node, state);

        final boolean isLicenseValidForFips =
-            FIPS140LicenseBootstrapCheck.ALLOWED_LICENSE_OPERATION_MODES.contains(license.operationMode());
+            Security.FIPS_ALLOWED_LICENSE_OPERATION_MODES.contains(license.operationMode());


This (existing) test is testing 2 different scenarios based on randomness in the license type.
Can we move it to 2 explicit test cases?

jkakavas · 2019-10-11T14:42:56Z

@tvernum I believe I have addressed your comments, if you want to take another look at this

jkakavas · 2019-10-12T11:07:15Z