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

Test adjustments for FIPS 140 #56526

Merged
merged 5 commits into from
May 15, 2020
Merged

Test adjustments for FIPS 140 #56526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
091d425
Test adjustments for FIPS 140
jkakavas May 11, 2020
c3c7af6
address feedback
jkakavas May 12, 2020
58539f9
remove pasted text
jkakavas May 12, 2020
ba92aeb
Merge branch '7.x' into fips-ci-7.x-take2
elasticmachine May 12, 2020
24a5306
Remove system property and do a runtime detection of the security pro…
jkakavas May 13, 2020
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
8 changes: 6 additions & 2 deletions libs/ssl-config/src/test/java/org/elasticsearch/common/ssl/PemUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.AlgorithmParameters;
import java.security.Key;
import java.security.KeyStore;
import java.security.PrivateKey;
import java.security.interfaces.ECPrivateKey;
import java.security.spec.ECGenParameterSpec;
import java.security.spec.ECParameterSpec;
import java.util.function.Supplier;

Expand Down Expand Up @@ -72,8 +74,10 @@ public void testReadEcKeyCurves() throws Exception {
PrivateKey privateKey = PemUtils.readPrivateKey(getDataPath("/certs/pem-utils/private_" + curve + ".pem"), ""::toCharArray);
assertThat(privateKey, instanceOf(ECPrivateKey.class));
ECParameterSpec parameterSpec = ((ECPrivateKey) privateKey).getParams();
// This is brittle but we can't access sun.security.util.NamedCurve
assertThat(parameterSpec.toString(), containsString(curve));
ECGenParameterSpec algorithmParameterSpec = new ECGenParameterSpec(curve);
AlgorithmParameters algoParameters = AlgorithmParameters.getInstance("EC");
algoParameters.init(algorithmParameterSpec);
assertThat(parameterSpec, equalTo(algoParameters.getParameterSpec(ECParameterSpec.class)));
}

public void testReadPKCS8EcKey() throws Exception {
Expand Down
44 changes: 36 additions & 8 deletions plugins/discovery-ec2/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,34 @@ task writeTestJavaPolicy {
throw new GradleException("failed to create temporary directory [${tmp}]")
}
final File javaPolicy = file("${tmp}/java.policy")
javaPolicy.write(
[
"grant {",
" permission java.util.PropertyPermission \"com.amazonaws.sdk.ec2MetadataServiceEndpointOverride\", \"write\";",
"};"
].join("\n"))
if (BuildParams.inFipsJvm) {
javaPolicy.write(
[
"grant {",
" permission java.security.SecurityPermission \"putProviderProperty.BCFIPS\";",
" permission java.security.SecurityPermission \"putProviderProperty.BCJSSE\";",
" permission java.lang.RuntimePermission \"getProtectionDomain\";",
" permission java.util.PropertyPermission \"java.runtime.name\", \"read\";",
" permission org.bouncycastle.crypto.CryptoServicesPermission \"tlsAlgorithmsEnabled\";",
" permission java.lang.RuntimePermission \"accessClassInPackage.sun.security.internal.spec\";",
" permission java.lang.RuntimePermission \"accessDeclaredMembers\";",
" permission java.util.PropertyPermission \"intellij.debug.agent\", \"read\";",
" permission java.util.PropertyPermission \"intellij.debug.agent\", \"write\";",
" permission org.bouncycastle.crypto.CryptoServicesPermission \"exportSecretKey\";",
" permission org.bouncycastle.crypto.CryptoServicesPermission \"exportPrivateKey\";",
" permission java.io.FilePermission \"\${javax.net.ssl.trustStore}\", \"read\";",
" permission java.util.PropertyPermission \"com.amazonaws.sdk.ec2MetadataServiceEndpointOverride\", \"write\";",
"};"
].join("\n")
)
} else {
javaPolicy.write(
[
"grant {",
" permission java.util.PropertyPermission \"com.amazonaws.sdk.ec2MetadataServiceEndpointOverride\", \"write\";",
"};"
].join("\n"))
}
}
}

Expand All @@ -78,9 +100,15 @@ test {
// this is needed for insecure plugins, remove if possible!
systemProperty 'tests.artifact', project.name

// this is needed to manipulate com.amazonaws.sdk.ec2MetadataServiceEndpointOverride system property
// Setting a custom policy to manipulate com.amazonaws.sdk.ec2MetadataServiceEndpointOverride system property
// it is better rather disable security manager at all with `systemProperty 'tests.security.manager', 'false'`
systemProperty 'java.security.policy', "file://${buildDir}/tmp/java.policy"
if (BuildParams.inFipsJvm){
// Using the key==value format to override default JVM security settings and policy
// see also: https://docs.oracle.com/javase/8/docs/technotes/guides/security/PolicyFiles.html
systemProperty 'java.security.policy', "=file://${buildDir}/tmp/java.policy"
Copy link
Member

Choose a reason for hiding this comment

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

What does the prefixed = do here? I wasn't able to find documentation on that special syntax. We should add a comment explaining for future readers.

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 explain this here

elasticsearch/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

Line 165 in 1fe5264

// Using the key==value format to override default JVM security settings and policy
, if you think we should copy the explanation in every use, happy to add it here too

} else {
systemProperty 'java.security.policy', "file://${buildDir}/tmp/java.policy"
}
}

check {
Expand Down
4 changes: 2 additions & 2 deletions plugins/ingest-attachment/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,6 @@ if (BuildParams.inFipsJvm) {
// rather than provide a long list of exclusions, disable the check on FIPS.
jarHell.enabled = false
test.enabled = false
integTest.enabled = false;
testingConventions.enabled = false;
integTest.enabled = false
testingConventions.enabled = false
}
14 changes: 13 additions & 1 deletion x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.apache.logging.log4j.Logger;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.bootstrap.JavaVersion;
import org.elasticsearch.common.CheckedSupplier;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.logging.DeprecationLogger;
Expand Down Expand Up @@ -46,6 +47,7 @@
import java.security.GeneralSecurityException;
import java.security.KeyManagementException;
import java.security.NoSuchAlgorithmException;
import java.security.Security;
import java.security.cert.Certificate;
import java.security.cert.X509Certificate;
import java.util.ArrayList;
Expand Down Expand Up @@ -829,11 +831,21 @@ private static String sslContextAlgorithm(List<String> supportedProtocols) {
}

private boolean shouldEnableDiagnoseTrust() {
if (XPackSettings.FIPS_MODE_ENABLED.get(settings) && DIAGNOSE_TRUST_EXCEPTIONS_SETTING.exists(settings) == false ) {
// We disable the DiagnosticTrustManager in Java 8 when SunJSSE is set in FIPS 140 mode, as it doesn't allow X509TrustManager to be
// wrapped
if (inSunJsseInFipsMode()) {
logger.info("diagnostic messages for SSL/TLS trust cannot be enabled for SunJSSE in FIPS mode.");
return false;
} else if (XPackSettings.FIPS_MODE_ENABLED.get(settings) && DIAGNOSE_TRUST_EXCEPTIONS_SETTING.exists(settings) == false) {
logger.info("diagnostic messages for SSL/TLS trust failures are not enabled in FIPS 140 mode by default.");
return false;
} else {
return DIAGNOSE_TRUST_EXCEPTIONS_SETTING.get(settings);
}
}

static boolean inSunJsseInFipsMode() {
return JavaVersion.current().getVersion().get(0) == 8 && Arrays.stream(Security.getProviders())
.anyMatch(provider -> provider.getName().equals("SunJSSE") && provider.getInfo().contains("FIPS mode"));
}
}
8 changes: 4 additions & 4 deletions ...test/java/org/elasticsearch/xpack/core/security/transport/ProfileConfigurationsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ public void testGetInsecureTransportProfileConfigurations() {
}

private Settings.Builder getBaseSettings() {
final Path keystore = randomBoolean()
? getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.jks")
: getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.p12");

final Path keystore = inFipsJvm()
? getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.p12")
: getDataPath(randomFrom("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.jks",
"/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.p12"));
MockSecureSettings secureSettings = new MockSecureSettings();
secureSettings.setString("xpack.security.transport.ssl.keystore.secure_password", "testnode");

Expand Down
8 changes: 6 additions & 2 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/PemUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.AlgorithmParameters;
import java.security.Key;
import java.security.KeyStore;
import java.security.PrivateKey;
import java.security.interfaces.ECPrivateKey;
import java.security.spec.ECGenParameterSpec;
import java.security.spec.ECParameterSpec;

import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -70,8 +72,10 @@ public void testReadEcKeyCurves() throws Exception {
("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/private_" + curve + ".pem"), ""::toCharArray);
assertThat(privateKey, instanceOf(ECPrivateKey.class));
ECParameterSpec parameterSpec = ((ECPrivateKey) privateKey).getParams();
// This is brittle but we can't access sun.security.util.NamedCurve
assertThat(parameterSpec.toString(), containsString(curve));
ECGenParameterSpec algorithmParameterSpec = new ECGenParameterSpec(curve);
AlgorithmParameters algoParameters = AlgorithmParameters.getInstance("EC");
algoParameters.init(algorithmParameterSpec);
assertThat(parameterSpec, equalTo(algoParameters.getParameterSpec(ECParameterSpec.class)));
}

public void testReadEncryptedPKCS8Key() throws Exception {
Expand Down
24 changes: 16 additions & 8 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/SSLServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import java.util.concurrent.atomic.AtomicInteger;

import static org.elasticsearch.test.TestMatchers.throwableWithMessage;
import static org.elasticsearch.xpack.core.ssl.SSLService.inSunJsseInFipsMode;
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -90,14 +91,19 @@ public class SSLServiceTests extends ESTestCase {

@Before
public void setup() throws Exception {
// Randomise the keystore type (jks/PKCS#12)
if (randomBoolean()) {
testnodeStore = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.jks");
// The default is to use JKS. Randomly test with explicit and with the default value.
testnodeStoreType = "jks";
} else {
// Randomise the keystore type (jks/PKCS#12) when possible
if (inFipsJvm()) {
testnodeStore = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.p12");
testnodeStoreType = randomBoolean() ? "PKCS12" : null;
Copy link
Member

Choose a reason for hiding this comment

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

Is the null to test the store type can be auto-detected?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it shouldn't make a difference as we should be able to auto detect. Arguably we don't need to test this here, but I believe it was originally there too and I didn't introduce this in this change

} else {
if (randomBoolean()) {
testnodeStore = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.jks");
// The default is to use JKS. Randomly test with explicit and with the default value.
testnodeStoreType = "jks";
} else {
testnodeStore = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.p12");
testnodeStoreType = randomBoolean() ? "PKCS12" : null;
}
}
logger.info("Using [{}] key/truststore [{}]", testnodeStoreType, testnodeStore);
testnodeCert = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt");
Expand Down Expand Up @@ -820,6 +826,7 @@ public void testThatSSLIOSessionStrategyTrustsJDKTrustedCAs() throws Exception {
}

public void testWrapTrustManagerWhenDiagnosticsEnabled() {
assumeFalse("We explicitly disable diagnostic trust manager in SunJSSE in FIPS mode ", inSunJsseInFipsMode());
final Settings.Builder builder = Settings.builder();
if (randomBoolean()) { // randomly select between default, and explicit enabled
builder.put("xpack.security.ssl.diagnose.trust", true);
Expand All @@ -841,7 +848,7 @@ public void testDontWrapTrustManagerWhenDiagnosticsDisabled() {
assertThat(sslService.wrapWithDiagnostics(baseTrustManager, sslConfiguration), sameInstance(baseTrustManager));
}

public void testDontWrapTrustManagerByDefaultWhenInFips(){
public void testDontWrapTrustManagerByDefaultWhenInFips() {
final Settings.Builder builder = Settings.builder();
builder.put("xpack.security.fips_mode.enabled", true);
final SSLService sslService = new SSLService(builder.build(), env);
Expand All @@ -850,7 +857,8 @@ public void testDontWrapTrustManagerByDefaultWhenInFips(){
assertThat(sslService.wrapWithDiagnostics(baseTrustManager, sslConfiguration), sameInstance(baseTrustManager));
}

public void testWrapTrustManagerWhenInFipsAndExplicitlyConfigured(){
public void testWrapTrustManagerWhenInFipsAndExplicitlyConfigured() {
assumeFalse("We explicitly disable diagnostic trust manager in SunJSSE in FIPS mode ", inSunJsseInFipsMode());
final Settings.Builder builder = Settings.builder();
builder.put("xpack.security.fips_mode.enabled", true);
builder.put("xpack.security.ssl.diagnose.trust", true);
Expand Down
6 changes: 6 additions & 0 deletions .../security/src/test/java/org/elasticsearch/xpack/ssl/SSLReloadDuringStartupIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
import org.elasticsearch.test.InternalTestCluster.RestartCallback;
import org.elasticsearch.test.SecurityIntegTestCase;
import org.junit.BeforeClass;

import java.io.IOException;
import java.io.UncheckedIOException;
Expand All @@ -28,6 +29,11 @@
@ClusterScope(transportClientRatio = 0)
public class SSLReloadDuringStartupIntegTests extends SecurityIntegTestCase {

@BeforeClass
public static void skipInFips() {
assumeFalse("Can't use JKS keystores in FIPS JVM", inFipsJvm());
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to make this work if p12 file is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? But we don't support PKCS12 nor JKS stores in FIPS mode so there is not much value in testing reloading such a keystore.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Forgot about this again.

}

@Override
public Settings nodeSettings(int nodeOrdinal) {
Settings settings = super.nodeSettings(nodeOrdinal);
Expand Down