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

Settings: Reimplement keystore format to use FIPS compliant algorithms #28255

Merged
merged 4 commits into from
Jan 26, 2018

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Jan 17, 2018

This commit switches the internal format of the elasticsearch keystore
to no longer use java's KeyStore class, but instead encrypt the binary
data of the secrets using AES-GCM. The cipher key is generated using
PBKDF2WithHmacSHA512. Tests are also added for backcompat reading the v1
and v2 formats.

This commit switches the internal format of the elasticsearch keystore
to no longer use java's KeyStore class, but instead encrypt the binary
data of the secrets using AES-GCM. The cipher key is generated using
PBKDF2WithHmacSHA512. Tests are also added for backcompat reading the v1
and v2 formats.
@rjernst rjernst requested a review from jaymode January 17, 2018 01:32
@DaveCTurner DaveCTurner added :Core/Infra/Settings Settings infrastructure and APIs v7.0.0 labels Jan 17, 2018
@DaveCTurner
Copy link
Contributor

I added :Settings and v7.0.0 labels - please add more if needed.

@jaymode jaymode added the review label Jan 17, 2018
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.

I left one comment, but other than that LGTM

/** The algorithm used to store file setting contents. */
private static final String NEW_KEYSTORE_FILE_KEY_ALGO = "PBE";
/** The number of bits for the cipher key. */
private static final int CIPHER_KEY_BITS = 256;
Copy link
Member

Choose a reason for hiding this comment

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

I'm all for 256 bit, but the Oracle JDK 8 ships with a limited JCE policy that restricts key length for AES to 128 bits. To prevent friction with using our secret settings, we need to use 128 bit keys until we have Java 9 as the minimum supported version, http://www.oracle.com/technetwork/java/javase/terms/readme/jdk9-readme-3852447.html#jce

@jaymode jaymode requested a review from jkakavas January 18, 2018 16:08
@jaymode jaymode added the v6.3.0 label Jan 18, 2018
Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

LGTM, a minor comment on testing and a mental not for documentation

output.writeString("file_setting");
output.writeString("FILE");

SecretKeyFactory secretFactory = SecretKeyFactory.getInstance("PBE");
Copy link
Member

Choose a reason for hiding this comment

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

We would probably want to run these in CI in a JVM with FIPS support in FIPS approved mode only, which means we won't be able to get a PBE instance of the factory. We could try and catch the NoSuchAlgorithmException and succeed the test as we can't offer backwards compatibility in the sense of "ability to read the old keystore format while running under FIPS approved only mode", anyway.

return bytes.toByteArray();
}

private void decryptLegacyEntries() throws GeneralSecurityException, IOException {
Copy link
Member

Choose a reason for hiding this comment

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

This won't work if the JVM is in FIPS approved mode only as PBE is not available and SecretKeyFactory.getInstance("PBE"); will throw an Exception.
I'm thinking of the case where someone wants to upgrade to 6.3.0 and at the same time enable FIPS approved only mode. We can highlight this and document manual steps that will ensure the keystore gets upgraded before FIPS approved mode is enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add an assumeFalse once we are ready to test that.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to adding an assumption once we get to the point of running tests on a FIPS enabled JVM

@rjernst rjernst merged commit b47b399 into elastic:master Jan 26, 2018
@rjernst rjernst deleted the keystore_v3 branch January 26, 2018 23:51
rjernst added a commit that referenced this pull request Jan 27, 2018
#28255)

This commit switches the internal format of the elasticsearch keystore
to no longer use java's KeyStore class, but instead encrypt the binary
data of the secrets using AES-GCM. The cipher key is generated using
PBKDF2WithHmacSHA512. Tests are also added for backcompat reading the v1
and v2 formats.
tvernum added a commit to tvernum/elasticsearch that referenced this pull request May 4, 2018
In elastic#28255 the implementation of the elasticsearch.keystore was changed
to no longer be built on top of a PKCS#12 keystore. A side effect of
that change was that calling getString or getFile on a closed
KeyStoreWrapper ceased to throw an exception, and would instead return
a value consisting of all 0 bytes.

This change restores the previous behaviour as closely as possible.
It is possible to retrieve the _keys_ from a closed keystore, but any
attempt to get or set the entries will throw an IllegalStateException.

The only divergence from the previous behaviour is that "isLoaded"
will now return false if the keystore is closed, in keeping with the
"loaded and retrievable" description in the parent javadoc.
tvernum added a commit that referenced this pull request May 14, 2018
In #28255 the implementation of the elasticsearch.keystore was changed
to no longer be built on top of a PKCS#12 keystore. A side effect of
that change was that calling getString or getFile on a closed
KeyStoreWrapper ceased to throw an exception, and would instead return
a value consisting of all 0 bytes.

This change restores the previous behaviour as closely as possible.
It is possible to retrieve the _keys_ from a closed keystore, but any
attempt to get or set the entries will throw an IllegalStateException.
tvernum added a commit that referenced this pull request May 15, 2018
In #28255 the implementation of the elasticsearch.keystore was changed
to no longer be built on top of a PKCS#12 keystore. A side effect of
that change was that calling getString or getFile on a closed
KeyStoreWrapper ceased to throw an exception, and would instead return
a value consisting of all 0 bytes.

This change restores the previous behaviour as closely as possible.
It is possible to retrieve the _keys_ from a closed keystore, but any
attempt to get or set the entries will throw an IllegalStateException.
tvernum added a commit that referenced this pull request May 15, 2018
In #28255 the implementation of the elasticsearch.keystore was changed
to no longer be built on top of a PKCS#12 keystore. A side effect of
that change was that calling getString or getFile on a closed
KeyStoreWrapper ceased to throw an exception, and would instead return
a value consisting of all 0 bytes.

This change restores the previous behaviour as closely as possible.
It is possible to retrieve the _keys_ from a closed keystore, but any
attempt to get or set the entries will throw an IllegalStateException.
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