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.

Sign up for GitHub

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

Jump to bottom

Refactor FIPS BootstrapChecks to simple checks #47499

Merged
merged 4 commits into from
Oct 22, 2019
Merged

Refactor FIPS BootstrapChecks to simple checks #47499

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5bf8e7
Refactor FIPS BootstrapChecks to simple checks
jkakavas Oct 3, 2019
1843c70
Address feedback
jkakavas Oct 7, 2019
2816d5f
Merge remote-tracking branch 'origin/master' into refactor-fips-checks
jkakavas Oct 7, 2019
57d88a3
Merge branch 'master' into refactor-fips-checks
elasticmachine Oct 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 0 additions & 49 deletions ...rity/src/main/java/org/elasticsearch/xpack/security/FIPS140JKSKeystoreBootstrapCheck.java
View file
Open in desktop

This file was deleted.

35 changes: 0 additions & 35 deletions ...security/src/main/java/org/elasticsearch/xpack/security/FIPS140LicenseBootstrapCheck.java
View file
Open in desktop

This file was deleted.

34 changes: 0 additions & 34 deletions .../java/org/elasticsearch/xpack/security/FIPS140PasswordHashingAlgorithmBootstrapCheck.java
View file
Open in desktop

This file was deleted.

53 changes: 0 additions & 53 deletions ...y/src/main/java/org/elasticsearch/xpack/security/FIPS140SecureSettingsBootstrapCheck.java
View file
Open in desktop

This file was deleted.

54 changes: 46 additions & 8 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.elasticsearch.action.support.ActionFilter;
import org.elasticsearch.action.support.DestructiveOperations;
import org.elasticsearch.bootstrap.BootstrapCheck;
import org.elasticsearch.bootstrap.JavaVersion;
import org.elasticsearch.client.Client;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.IndexMetaData;
Expand Down Expand Up @@ -247,6 +248,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand All @@ -273,6 +275,8 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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


private final Settings settings;
private final Environment env;
Expand Down Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// we load them all here otherwise we can't access secure settings since they are closed once the checks are
// fetched
final List<BootstrapCheck> checks = new ArrayList<>();
checks.addAll(Arrays.asList(
new ApiKeySSLBootstrapCheck(),
new TokenSSLBootstrapCheck(),
new PkiRealmBootstrapCheck(getSslService()),
new TLSLicenseBootstrapCheck(),
new FIPS140SecureSettingsBootstrapCheck(settings, env),
new FIPS140JKSKeystoreBootstrapCheck(),
new FIPS140PasswordHashingAlgorithmBootstrapCheck(),
new FIPS140LicenseBootstrapCheck()));
new TLSLicenseBootstrapCheck()));
checks.addAll(InternalRealms.getBootstrapChecks(settings, env));
this.bootstrapChecks = Collections.unmodifiableList(checks);
Automatons.updateConfiguration(settings);
Expand All @@ -326,8 +326,11 @@ public Security(Settings settings, final Path configPath) {

}

private static void runStartupChecks(Settings settings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

private void runStartupChecks(Settings settings, XPackLicenseState licenseState) {
validateRealmSettings(settings);
if (XPackSettings.FIPS_MODE_ENABLED.get(settings)) {
validateForFips(settings, licenseState);
}
}

// overridable by tests
Expand Down Expand Up @@ -830,6 +833,41 @@ static void validateRealmSettings(Settings settings) {
}
}

static void validateForFips(Settings settings, XPackLicenseState licenseState) {
final List<String> validationErrors = new ArrayList<>();
Settings keystoreTypeSettings = settings.filter(k -> k.endsWith("keystore.type"))
.filter(k -> settings.get(k).equalsIgnoreCase("jks"));
if (keystoreTypeSettings.isEmpty() == false) {
validationErrors.add("JKS Keystores cannot be used in a FIPS 140 compliant JVM. Please " +
"revisit [" + keystoreTypeSettings.toDelimitedString(',') + "] settings");
}
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (keystorePathSettings.isEmpty() == false && JavaVersion.current().compareTo(JavaVersion.parse("9")) < 0) {
validationErrors.add("JKS Keystores cannot be used in a FIPS 140 compliant JVM. Please " +
"revisit [" + keystorePathSettings.toDelimitedString(',') + "] settings");
}
final String selectedAlgorithm = XPackSettings.PASSWORD_HASHING_ALGORITHM.get(settings);
if (selectedAlgorithm.toLowerCase(Locale.ROOT).startsWith("pbkdf2") == false) {
validationErrors.add("Only PBKDF2 is allowed for password hashing in a FIPS 140 JVM. Please set the " +
"appropriate value for [ " + XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey() + " ] setting.");
}

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");
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message should be built from FIPS_ALLOWED_LICENSE_OPERATION_MODES

}
if (validationErrors.isEmpty() == false) {
final StringBuilder sb = new StringBuilder();
sb.append("Validation for FIPS 140 mode failed: \n");
int index = 0;
for (String error : validationErrors) {
sb.append(++index).append(": ").append(error).append(";\n");
}
throw new IllegalArgumentException(sb.toString());
}
}

@Override
public List<TransportInterceptor> getTransportInterceptors(NamedWriteableRegistry namedWriteableRegistry, ThreadContext threadContext) {
if (enabled == false) { // don't register anything if we are not enabled
Expand Down Expand Up @@ -998,7 +1036,7 @@ public void accept(DiscoveryNode node, ClusterState state) {
if (inFipsMode) {
License license = LicenseService.getLicense(state.metaData());
if (license != null &&
FIPS140LicenseBootstrapCheck.ALLOWED_LICENSE_OPERATION_MODES.contains(license.operationMode()) == false) {
FIPS_ALLOWED_LICENSE_OPERATION_MODES.contains(license.operationMode()) == false) {
throw new IllegalStateException("FIPS mode cannot be used with a [" + license.operationMode() +
"] license. It is only allowed with a Platinum or Trial license.");

Expand Down
49 changes: 0 additions & 49 deletions ...src/test/java/org/elasticsearch/xpack/security/FIPS140JKSKeystoreBootstrapCheckTests.java
View file
Open in desktop

This file was deleted.

45 changes: 0 additions & 45 deletions ...ity/src/test/java/org/elasticsearch/xpack/security/FIPS140LicenseBootstrapCheckTests.java
View file
Open in desktop

This file was deleted.

Loading