Skip to content

Commit

Permalink
Ensure CI is run in FIPS 140 approved only mode (#66805)
Browse files Browse the repository at this point in the history
We were depending on the BouncyCastle FIPS own mechanics to set
itself in approved only mode since we run with the Security
Manager enabled. The check during startup seems to happen before we
set our restrictive SecurityManager though in
org.elasticsearch.bootstrap.Elasticsearch , and this means that
BCFIPS would not be in approved only mode, unless explicitly
configured so.

This commit sets the appropriate JVM property to explicitly set
BCFIPS in approved only mode in CI and adds tests to ensure that we
will be running with BCFIPS in approved only mode when we expect to.
It also sets xpack.security.fips_mode.enabled to true for all test clusters
used in fips mode and sets the distribution to the default one. It adds a
password to the elasticsearch keystore for all test clusters that run in fips
mode.
Moreover, it changes a few unit tests where we would use bcrypt even in
FIPS 140 mode. These would still pass since we are bundling our own
bcrypt implementation, but are now changed to use FIPS 140 approved
algorithms instead for better coverage.

It also addresses a number of tests that would fail in approved only mode
Mainly:

    Tests that use PBKDF2 with a password less than 112 bits (14char). We
    elected to change the passwords used everywhere to be at least 14
    characters long instead of mandating
    the use of pbkdf2_stretch because both pbkdf2 and
    pbkdf2_stretch are supported and allowed in fips mode and it makes sense
    to test with both. We could possibly figure out the password algorithm used
    for each test and adjust password length accordingly only for pbkdf2 but
    there is little value in that. It's good practice to use strong passwords so if
    our docs and tests use longer passwords, then it's for the best. The approach
    is brittle as there is no guarantee that the next test that will be added won't
    use a short password, so we add some testing documentation too.
    This leaves us with a possible coverage gap since we do support passwords
    as short as 6 characters but we only test with > 14 chars but the
    validation itself was not tested even before. Tests can be added in a followup,
    outside of fips related context.

    Tests that use a PKCS12 keystore and were not already muted.

    Tests that depend on running test clusters with a basic license or
    using the OSS distribution as FIPS 140 support is not available in
    neither of these.

Finally, it adds some information around FIPS 140 testing in our testing
documentation reference so that developers can hopefully keep in
mind fips 140 related intricacies when writing/changing docs.
  • Loading branch information
jkakavas committed Dec 24, 2020
1 parent 0770d33 commit 4557e57
Show file tree
Hide file tree
Showing 176 changed files with 1,107 additions and 617 deletions.
75 changes: 75 additions & 0 deletions TESTING.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,81 @@ repository without fetching latest. For these use cases, you can set the system
property `tests.bwc.git_fetch_latest` to `false` and the BWC builds will skip
fetching the latest from the remote.

== Testing in FIPS 140-2 mode

We have a CI matrix job that periodically runs all our tests with the JVM configured
to be FIPS 140-2 compliant with the use of the BouncyCastle FIPS approved Security Provider.
FIPS 140-2 imposes certain requirements that affect how our tests should be set up or what
can be tested. This section summarizes what one needs to take into consideration so that
tests won't fail when run in fips mode.

=== Muting tests in FIPS 140-2 mode

If the following limitations cannot be observed, or there is a need to actually test some use
case that is not available/allowed in fips mode, the test can be muted. For unit tests or Java
rest tests one can use

------------------------------------------------
assumeFalse("Justification why this cannot be run in FIPS mode", inFipsJvm());
------------------------------------------------

For specific YAML rest tests one can use

------------------------------------------------
- skip:
features: fips_140
reason: "Justification why this cannot be run in FIPS mode"
------------------------------------------------

For disabling entire types of tests for subprojects, one can use for example:

------------------------------------------------
if (BuildParams.inFipsJvm){
// This test cluster is using a BASIC license and FIPS 140 mode is not supported in BASIC
tasks.named("javaRestTest").configure{enabled = false }
}
------------------------------------------------

in `build.gradle`.

=== Limitations

The following should be taken into consideration when writing new tests or adjusting existing ones:

==== TLS

`JKS` and `PKCS#12` keystores cannot be used in FIPS mode. If the test depends on being able to use
a keystore, it can be muted when needed ( see `ESTestCase#inFipsJvm` ). Alternatively, one can use
PEM encoded files for keys and certificates for the tests or for setting up TLS in a test cluster.
Also, when in FIPS 140 mode, hostname verification for TLS cannot be turned off so if you are using
`*.verification_mode: none` , you'd need to mute the test in fips mode.

When using TLS, ensure that private keys used are longer than 2048 bits, or mute the test in fips mode.

==== Password hashing algorithm

Test clusters are configured with `xpack.security.fips_mode.enabled` set to true. This means that
FIPS 140-2 related bootstrap checks are enabled and the test cluster will fail to form if the
password hashing algorithm is set to something else than a PBKDF2 based one. You can delegate the choice
of algorithm to i.e. `SecurityIntegTestCase#getFastStoredHashAlgoForTests` if you don't mind the
actual algorithm used, or depend on default values for the test cluster nodes.

==== Password length

While using `pbkdf2` as the password hashing algorithm, FIPS 140-2 imposes a requirement that
passwords are longer than 14 characters. You can either ensure that all test user passwords in
your test are longer than 14 characters and use i.e. `SecurityIntegTestCase#getFastStoredHashAlgoForTests`
to randomly select a hashing algorithm, or use `pbkdf2_stretch` that doesn't have the same
limitation.

==== Keystore Password

In FIPS 140-2 mode, the elasticsearch keystore needs to be password protected with a password
of appropriate length. This is handled automatically in `fips.gradle` and the keystore is unlocked
on startup by the test clusters tooling in order to have secure settings available. However, you
might need to take into consideration that the keystore is password-protected with `keystore-password`
if you need to interact with it in a test.

== How to write good tests?

=== Base classes for test cases
Expand Down
12 changes: 11 additions & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,17 @@ tasks.register("verifyVersions") {
*/

boolean bwc_tests_enabled = true
final String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */
String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */
/*
* FIPS 140-2 behavior was fixed in 7.11.0. Before that there is no way to run elasticsearch in a
* JVM that is properly configured to be in fips mode with BCFIPS. For now we need to disable
* all bwc testing in fips mode.
*/

if ( BuildParams.inFipsJvm ) {
bwc_tests_enabled = false
bwc_tests_disabled_issue = "https://github.com/elastic/elasticsearch/issues/66772"
}
if (bwc_tests_enabled == false) {
if (bwc_tests_disabled_issue.isEmpty()) {
throw new GradleException("bwc_tests_disabled_issue must be set when bwc_tests_enabled == false")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ public synchronized void start() {
if (keystoreSettings.isEmpty() == false || keystoreFiles.isEmpty() == false) {
logToProcessStdout("Adding " + keystoreSettings.size() + " keystore settings and " + keystoreFiles.size() + " keystore files");

keystoreSettings.forEach((key, value) -> runKeystoreCommandWithPassword(keystorePassword, value.toString(), "add", "-x", key));
keystoreSettings.forEach((key, value) -> runKeystoreCommandWithPassword(keystorePassword, value.toString(), "add", key));

for (Map.Entry<String, File> entry : keystoreFiles.entrySet()) {
File file = entry.getValue();
Expand Down
6 changes: 3 additions & 3 deletions client/rest-high-level/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,14 @@ File pkiTrustCert = file("./src/test/resources/org/elasticsearch/client/security
tasks.named("integTest").configure {
systemProperty 'tests.rest.async', 'false'
systemProperty 'tests.rest.cluster.username', System.getProperty('tests.rest.cluster.username', 'test_user')
systemProperty 'tests.rest.cluster.password', System.getProperty('tests.rest.cluster.password', 'test-password')
systemProperty 'tests.rest.cluster.password', System.getProperty('tests.rest.cluster.password', 'test-user-password')
}

// Requires https://github.com/elastic/elasticsearch/pull/64403 to have this moved to task avoidance api.
TaskProvider<RestIntegTestTask> asyncIntegTest = tasks.register("asyncIntegTest", RestIntegTestTask) {
systemProperty 'tests.rest.async', 'true'
systemProperty 'tests.rest.cluster.username', System.getProperty('tests.rest.cluster.username', 'test_user')
systemProperty 'tests.rest.cluster.password', System.getProperty('tests.rest.cluster.password', 'test-password')
systemProperty 'tests.rest.cluster.password', System.getProperty('tests.rest.cluster.password', 'test-user-password')
}

tasks.named("check").configure {
Expand Down Expand Up @@ -110,7 +110,7 @@ testClusters.all {
keystore 'xpack.security.transport.ssl.truststore.secure_password', 'testnode'
extraConfigFile 'roles.yml', file('roles.yml')
user username: System.getProperty('tests.rest.cluster.username', 'test_user'),
password: System.getProperty('tests.rest.cluster.password', 'test-password'),
password: System.getProperty('tests.rest.cluster.password', 'test-user-password'),
role: System.getProperty('tests.rest.cluster.role', 'admin')
user username: 'admin_user', password: 'admin-password'

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ private PutUserRequest randomPutUserRequest(boolean enabled) {
}

private static PutUserRequest randomPutUserRequest(User user, boolean enabled) {
final char[] password = randomAlphaOfLengthBetween(6, 10).toCharArray();
final char[] password = randomAlphaOfLengthBetween(14, 19).toCharArray();
return new PutUserRequest(user, password, enabled, RefreshPolicy.IMMEDIATE);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,7 @@ public void testReindex() throws Exception {
Integer remotePort = host.getPort();
String remoteHost = host.getHostName();
String user = "test_user";
String password = "test-password";
String password = "test-user-password";

// tag::reindex-request-remote
request.setRemoteInfo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,9 @@ protected Settings restAdminSettings() {
public void testGetUsers() throws Exception {
final RestHighLevelClient client = highLevelClient();
String[] usernames = new String[] {"user1", "user2", "user3"};
addUser(client, usernames[0], randomAlphaOfLengthBetween(6, 10));
addUser(client, usernames[1], randomAlphaOfLengthBetween(6, 10));
addUser(client, usernames[2], randomAlphaOfLengthBetween(6, 10));
addUser(client, usernames[0], randomAlphaOfLengthBetween(14, 18));
addUser(client, usernames[1], randomAlphaOfLengthBetween(14, 18));
addUser(client, usernames[2], randomAlphaOfLengthBetween(14, 18));
{
//tag::get-users-request
GetUsersRequest request = new GetUsersRequest(usernames[0]);
Expand Down Expand Up @@ -251,7 +251,7 @@ public void testPutUser() throws Exception {

{
//tag::put-user-password-request
char[] password = new char[]{'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
char[] password = new char[]{'t', 'e', 's', 't', '-', 'u','s','e','r','-','p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
User user = new User("example", Collections.singletonList("superuser"));
PutUserRequest request = PutUserRequest.withPassword(user, password, true, RefreshPolicy.NONE);
//end::put-user-password-request
Expand All @@ -270,7 +270,7 @@ public void testPutUser() throws Exception {
byte[] salt = new byte[32];
// no need for secure random in a test; it could block and would not be reproducible anyway
random().nextBytes(salt);
char[] password = new char[]{'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
char[] password = new char[]{'t', 'e', 's', 't', '-', 'u','s','e','r','-','p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
User user = new User("example2", Collections.singletonList("superuser"));

//tag::put-user-hash-request
Expand Down Expand Up @@ -326,7 +326,7 @@ public void onFailure(Exception e) {

public void testDeleteUser() throws Exception {
RestHighLevelClient client = highLevelClient();
addUser(client, "testUser", "testPassword");
addUser(client, "testUser", "testUserPassword");

{
// tag::delete-user-request
Expand Down Expand Up @@ -566,7 +566,7 @@ public void onFailure(Exception e) {

public void testEnableUser() throws Exception {
RestHighLevelClient client = highLevelClient();
char[] password = new char[]{'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
char[] password = new char[]{'t', 'e', 's', 't', '-', 'u','s','e','r','-','p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
User enable_user = new User("enable_user", Collections.singletonList("superuser"));
PutUserRequest putUserRequest = PutUserRequest.withPassword(enable_user, password, true, RefreshPolicy.IMMEDIATE);
PutUserResponse putUserResponse = client.security().putUser(putUserRequest, RequestOptions.DEFAULT);
Expand Down Expand Up @@ -611,7 +611,7 @@ public void onFailure(Exception e) {

public void testDisableUser() throws Exception {
RestHighLevelClient client = highLevelClient();
char[] password = new char[]{'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
char[] password = new char[]{'t', 'e', 's', 't', '-', 'u','s','e','r','-','p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
User disable_user = new User("disable_user", Collections.singletonList("superuser"));
PutUserRequest putUserRequest = PutUserRequest.withPassword(disable_user, password, true, RefreshPolicy.IMMEDIATE);
PutUserResponse putUserResponse = client.security().putUser(putUserRequest, RequestOptions.DEFAULT);
Expand Down Expand Up @@ -1183,8 +1183,9 @@ public void onFailure(Exception e) {

public void testChangePassword() throws Exception {
RestHighLevelClient client = highLevelClient();
char[] password = new char[]{'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
char[] newPassword = new char[]{'n', 'e', 'w', 'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
char[] password = new char[]{'t', 'e', 's', 't', '-', 'u','s','e','r','-','p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
char[] newPassword =
new char[]{'n', 'e', 'w', '-', 't', 'e', 's', 't', '-', 'u','s','e','r','-','p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
User user = new User("change_password_user", Collections.singletonList("superuser"), Collections.emptyMap(), null, null);
PutUserRequest putUserRequest = PutUserRequest.withPassword(user, password, true, RefreshPolicy.NONE);
PutUserResponse putUserResponse = client.security().putUser(putUserRequest, RequestOptions.DEFAULT);
Expand Down Expand Up @@ -1403,14 +1404,14 @@ public void testCreateToken() throws Exception {
{
// Setup user
User token_user = new User("token_user", Collections.singletonList("kibana_user"));
PutUserRequest putUserRequest = PutUserRequest.withPassword(token_user, "password".toCharArray(), true,
PutUserRequest putUserRequest = PutUserRequest.withPassword(token_user, "test-user-password".toCharArray(), true,
RefreshPolicy.IMMEDIATE);
PutUserResponse putUserResponse = client.security().putUser(putUserRequest, RequestOptions.DEFAULT);
assertTrue(putUserResponse.isCreated());
}
{
// tag::create-token-password-request
final char[] password = new char[]{'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
final char[] password = new char[]{'t', 'e', 's', 't', '-', 'u','s','e','r','-','p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
CreateTokenRequest createTokenRequest = CreateTokenRequest.passwordGrant("token_user", password);
// end::create-token-password-request

Expand Down Expand Up @@ -1480,7 +1481,7 @@ public void testInvalidateToken() throws Exception {
String refreshToken;
{
// Setup users
final char[] password = "password".toCharArray();
final char[] password = "test-user-password".toCharArray();
User user = new User("user", Collections.singletonList("kibana_user"));
PutUserRequest putUserRequest = PutUserRequest.withPassword(user, password, true, RefreshPolicy.IMMEDIATE);
PutUserResponse putUserResponse = client.security().putUser(putUserRequest, RequestOptions.DEFAULT);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ public HttpAsyncClientBuilder customizeHttpClient(
final CredentialsProvider credentialsProvider =
new BasicCredentialsProvider();
credentialsProvider.setCredentials(AuthScope.ANY,
new UsernamePasswordCredentials("user", "password"));
new UsernamePasswordCredentials("user", "test-user-password"));

RestClientBuilder builder = RestClient.builder(
new HttpHost("localhost", 9200))
Expand All @@ -378,7 +378,7 @@ public HttpAsyncClientBuilder customizeHttpClient(
final CredentialsProvider credentialsProvider =
new BasicCredentialsProvider();
credentialsProvider.setCredentials(AuthScope.ANY,
new UsernamePasswordCredentials("user", "password"));
new UsernamePasswordCredentials("user", "test-user-password"));

RestClientBuilder builder = RestClient.builder(
new HttpHost("localhost", 9200))
Expand Down
17 changes: 12 additions & 5 deletions distribution/docker/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,17 @@ def createAndSetWritable(Object... locations) {
}
}

tasks.register("copyKeystore", Sync) {
tasks.register("copyNodeKeyMaterial", Sync) {
from project(':x-pack:plugin:core')
.file('src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.jks')
.files(
'src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.pem',
'src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt'
)
into "${buildDir}/certs"
doLast {
file("${buildDir}/certs").setReadable(true, false)
file("${buildDir}/certs/testnode.jks").setReadable(true, false)
file("${buildDir}/certs/testnode.pem").setReadable(true, false)
file("${buildDir}/certs/testnode.crt").setReadable(true, false)
}
}

Expand All @@ -241,7 +245,7 @@ elasticsearch_distributions {

tasks.named("preProcessFixture").configure {
dependsOn elasticsearch_distributions.docker_default, elasticsearch_distributions.docker_oss
dependsOn "copyKeystore"
dependsOn "copyNodeKeyMaterial"
doLast {
// tests expect to have an empty repo
project.delete(
Expand All @@ -261,7 +265,10 @@ tasks.named("preProcessFixture").configure {

tasks.named("processTestResources").configure {
from project(':x-pack:plugin:core')
.file('src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.jks')
.files(
'src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.pem',
'src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt'
)
}

tasks.register("integTest", Test) {
Expand Down

0 comments on commit 4557e57

Please sign in to comment.