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

Enable testing in FIPS140 JVM #31666

Merged
merged 24 commits into from
Jul 17, 2018
Merged

Enable testing in FIPS140 JVM #31666

merged 24 commits into from
Jul 17, 2018

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Jun 28, 2018

JKS keystores cannot be used in a FIPS 140 JVM as attempting to use
one in order to init a KeyManagerFactory is not
allowed.( JKS keystore algorithms for private key encryption are not
FIPS 140 approved)
This commit replaces JKS keystores in our tests with the
corresponding PEM encoded key and certificates for most of key and
trust configurations. JKS keystores are still used in some places as
TrustManagerFactory can be init with JKS keystores.
Whenever it's not possible to refactor the test, i.e. when we are
testing that we can load a JKS keystore, etc. we attempt to
mute the test when we are running in FIPS 140 JVM. Testing for the
JVM is naive and is based on the name of the security provider as
we would control the testing infrastrtucture and so this would be
reliable enough.
Other cases of tests being muted are the ones that involve custom
TrustStoreManagers or KeyStoreManagers, null TLS Ciphers and the
SAMLAuthneticator class as we cannot sign XML documents in the
way we were doing. SAMLAuthenticator tests in a FIPS JVM can be
reenabled with precomputed and signed SAML messages at a later stage.

IT will be covered in a subsequent PR

JKS keystores cannot be used in a FIPS JVM as attempting to use one
in order to init a KeyManagerFactory or a TrustManagerFactory is not
allowed.( JKS keystore algorithms for private key encryption are not
FIPS 140 approved)
This commit replaces JKS keystores in our tests with the
corresponding PEM encoded key and certificates both for key and trust
configurations.
Whenever it's not possible to refactor the test, i.e. when we are
testing that we can load a JKS keystore, etc. we attempt to
mute the test when we are running in FIPS 140 JVM. Testing for the
JVM is naive and is based on the name of the security provider as
we would control the testing infrastrtucture and so this would be
reliable enough.
Other cases of tests being muted are the ones that involve custom
TrustStoreManagers or KeyStoreManagers, null TLS Ciphers and the
SAMLAuthneticator class as we cannot sign XML documents in the
way we were doing. SAMLAuthenticator tests in a FIPS JVM can be
reenabled with precomputed and signed SAML messages at a later stage.

IT will be covered in a subsequent PR
@jkakavas jkakavas added >test Issues or PRs that are addressing/adding tests review v7.0.0 :Security/TLS SSL/TLS, Certificates v6.4.0 labels Jun 28, 2018
@jkakavas jkakavas requested a review from jaymode June 28, 2018 16:33
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jkakavas
Copy link
Member Author

jkakavas commented Jun 29, 2018

@elasticmachine test this please ( The failure doesn't reproduce locally)

KeyStore keyStore = KeyStore.getInstance("JKS");
keyStore.load(in, "password".toCharArray());
KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
try (InputStream certFile = RestClientBuilderIntegTests.class.getResourceAsStream("/test.crt")) {
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment that this builds a keystore programmatically so that it is keystore format independent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it is not entirely keystore format independent. This was changed for the same reason the tests that load a JKS keystore from disk are changed. One can't KeyManagerFactory#init() with a JKS store in a FIPS JVM. Can it be that I missed the essence of your comment ?

Copy link
Member

Choose a reason for hiding this comment

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

The main reason I want a comment is so that someone doesn’t come along and go “this is silly let’s just use a keystore”

@@ -111,6 +111,8 @@ public void testInvalidJodaPattern() {
}

public void testJodaPatternLocale() {
//TODO investigate if this is a bug in Joda
Copy link
Member

Choose a reason for hiding this comment

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

Can you open a issue for this specifically? This is very odd for a FIPS JVM to have a date parse error

Copy link
Member Author

Choose a reason for hiding this comment

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

Was on my to-do list, will open ASAP.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #31724, will look into it in more detail

@@ -99,6 +99,9 @@ public static void setupKeyStore() throws IOException {

@Override
protected Settings nodeSettings(int nodeOrdinal) {
if (inFipsJvm()) {
Copy link
Member

Choose a reason for hiding this comment

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

can we have a assumeFalse in a before class here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue with this one is that it has many @BeforeClass already and there is no guarantee the assumeFalse will be ran before those. startHttp in particular will throw an error in a FIPS JVM, and so will the stopHttpd() that will run on cleanup because of @AfterClass. I discussed possibilities to mute this in a cleaner way with Tal and he suggested trying with ESIntegTestCase.ClusterScope(scope = Scope.TEST, numDataNodes = 0) so that no nodes are setup so I can try this if the current solution is too ugly.

Copy link
Member

Choose a reason for hiding this comment

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

Can you do the one before the class in a ClassRule? We use it in a few places for cases like this. One recent case that was a bit different is #30763 and I believe there is another case in SecurityIntegTestCase

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I wasn't aware of this

@@ -53,6 +53,8 @@ public void testReadPKCS8EcKey() throws Exception {
}

public void testReadEncryptedPKCS8Key() throws Exception {
assumeFalse("Can't run in a FIPS JVM, PBE KeySpec is not available",
inFipsJvm());
Copy link
Member

Choose a reason for hiding this comment

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

line wrap doesn't seem necessary

@@ -197,6 +204,8 @@ public void testThatCreateSSLEngineWithOnlyTruststoreWorks() throws Exception {


public void testCreateWithKeystoreIsValidForServer() throws Exception {
assumeFalse("Can't run in a FIPS JVM, JKS keystores can't be used",
Copy link
Member

Choose a reason for hiding this comment

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

remove line wrap

public static LDAPConnection openConnection(String url, String bindDN, String bindPassword, Path truststore) throws Exception {
boolean useGlobalSSL = ESTestCase.randomBoolean();
public static LDAPConnection openConnection(String url, String bindDN, String bindPassword, List<String> certificateAuthorities) throws
Exception {
Copy link
Member

Choose a reason for hiding this comment

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

for the line wrap can you split it right before List<String? I find this easier to read

@@ -1907,7 +1910,7 @@ private void signElement(Element parent, Tuple<X509Certificate, PrivateKey> keyP
final String refID = "#" + parent.getAttribute("ID");
final X509Certificate certificate = keyPair.v1();
final PrivateKey privateKey = keyPair.v2();
final XMLSignatureFactory fac = XMLSignatureFactory.getInstance("DOM");
final XMLSignatureFactory fac = XMLSignatureFactory.getInstance("DOM", "SUN");
Copy link
Member

Choose a reason for hiding this comment

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

can we leave the provider off?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was leftover from some testing

.put("xpack.security.audit.enabled", false)
.put(XPackSettings.WATCHER_ENABLED.getKey(), false)
.put(TestZenDiscovery.USE_MOCK_PINGS.getKey(), false);
.putList("transport.profiles.client.xpack.security.ssl.certificate_authorities", Arrays.asList
Copy link
Member

Choose a reason for hiding this comment

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

move Arrays.asList to the line below

@@ -96,6 +97,11 @@ private Path initTempDir() throws Exception {
return tempDir;
}

@BeforeClass
public static void chechFipsJvm() {
Copy link
Member

Choose a reason for hiding this comment

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

s/chech/check

builder.put("xpack.ssl.keystore.path", store)
.put("xpack.ssl.verification_mode", "certificate")
.put("xpack.ssl.truststore.path", store);
private void addSslSettingsForKeyPair(Settings.Builder builder, Path key, String keyPassphrase, Path cert, List<String>
Copy link
Member

Choose a reason for hiding this comment

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

split the line with both the type and variable name for readability

public static final ExternalResource MUTE_IN_FIPS_JVM = new ExternalResource() {
@Override
protected void before() {
assumeFalse("Can't run in a FIPS JVM because none if the supported Keystore types can be used", inFipsJvm());
Copy link
Member

Choose a reason for hiding this comment

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

s/if/of

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 a few minor comments, but otherwise LGTM

sslContext = SSLContext.getInstance("TLSv1.2");
sslContext.init(kmf.getKeyManagers(), tmf.getTrustManagers(), new SecureRandom());
sslContext.init(new KeyManager[]{km}, new TrustManager[]{tm}, new SecureRandom());
return sslContext;
Copy link
Member

Choose a reason for hiding this comment

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

indentation is off after other changes

SSLContext context = SSLContext.getInstance("TLSv1.2");
context.init(kmf.getKeyManagers(), tmf.getTrustManagers(), new SecureRandom());
context.init(new KeyManager[]{km}, new TrustManager[]{tm}, new SecureRandom());
Copy link
Member

Choose a reason for hiding this comment

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

can you add spaces? new KeyManager[] { km }, new TrustManager[] { tm }

This was previously mistakenly removed in one of the refactoring
phases.
Copy over a cert  with different size so that we are sure that the
SSLConfiguration will be resolved. The underlying potential bug
will be tackled in a separate issue.
@jkakavas
Copy link
Member Author

@elasticmachine test this please

@jkakavas jkakavas merged commit 9e529d9 into elastic:master Jul 17, 2018
dnhatn added a commit that referenced this pull request Jul 20, 2018
* master:
  Painless: Simplify Naming in Lookup Package (#32177)
  Handle missing values in painless (#32207)
  add support for write index resolution when creating/updating documents (#31520)
  ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864)
  Remove indication of future multi-homing support (#32187)
  Rest test - allow for snapshots to take 0 milliseconds
  Make x-pack-core generate a pom file
  Rest HL client: Add put watch action (#32026)
  Build: Remove pom generation for plugin zip files (#32180)
  Fix comments causing errors with Java 11
  Fix rollup on date fields that don't support epoch_millis (#31890)
  Detect and prevent configuration that triggers a Gradle bug (#31912)
  [test] port linux package packaging tests (#31943)
  Revert "Introduce a Hashing Processor (#31087)" (#32178)
  Remove empty @return from JavaDoc
  Adjust SSLDriver behavior for JDK11 changes (#32145)
  [test] use randomized runner in packaging tests (#32109)
  Add support for field aliases. (#32172)
  Painless: Fix caching bug and clean up addPainlessClass. (#32142)
  Call setReferences() on custom referring tokenfilters in _analyze (#32157)
  Fix BwC Tests looking for UUID Pre 6.4 (#32158)
  Improve docs for search preferences (#32159)
  use before instead of onOrBefore
  Add more contexts to painless execute api (#30511)
  Add EC2 credential test for repository-s3 (#31918)
  A replica can be promoted and started in one cluster state update (#32042)
  Fix Java 11 javadoc compile problem
  Fix CP for namingConventions when gradle home has spaces (#31914)
  Fix `range` queries on `_type` field for singe type indices (#31756)
  [DOCS] Update TLS on Docker for 6.3 (#32114)
  ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are
  Remove versionType from translog (#31945)
  Switch distribution to new style Requests (#30595)
  Build: Skip jar tests if jar disabled
  Painless: Add PainlessClassBuilder (#32141)
  Build: Make additional test deps of check (#32015)
  Disable C2 from using AVX-512 on JDK 10 (#32138)
  Build: Move shadow customizations into common code (#32014)
  Painless: Fix Bug with Duplicate PainlessClasses (#32110)
  Remove empty @param from Javadoc
  Re-disable packaging tests on suse boxes
  Docs: Fix missing example script quote (#32010)
  [ML] Wait for aliases in multi-node tests (#32086)
  [ML] Move analyzer dependencies out of categorization config (#32123)
  Ensure to release translog snapshot in primary-replica resync (#32045)
  Handle TokenizerFactory  TODOs (#32063)
  Relax TermVectors API to work with textual fields other than TextFieldType (#31915)
  Updates the build to gradle 4.9 (#32087)
  Mute :qa:mixed-cluster indices.stats/10_index/Index - all’
  Check that client methods match API defined in the REST spec (#31825)
  Enable testing in FIPS140 JVM (#31666)
  Fix put mappings java API documentation (#31955)
  Add exclusion option to `keep_types` token filter (#32012)
  [Test] Modify assert statement for ssl handshake (#32072)
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Jul 20, 2018
Ensure our tests can run in a FIPS JVM

JKS keystores cannot be used in a FIPS JVM as attempting to use one
in order to init a KeyManagerFactory or a TrustManagerFactory is not
allowed.( JKS keystore algorithms for private key encryption are not
FIPS 140 approved)
This commit replaces JKS keystores in our tests with the
corresponding PEM encoded key and certificates both for key and trust
configurations.
Whenever it's not possible to refactor the test, i.e. when we are
testing that we can load a JKS keystore, etc. we attempt to
mute the test when we are running in FIPS 140 JVM. Testing for the
JVM is naive and is based on the name of the security provider as
we would control the testing infrastrtucture and so this would be
reliable enough.
Other cases of tests being muted are the ones that involve custom
TrustStoreManagers or KeyStoreManagers, null TLS Ciphers and the
SAMLAuthneticator class as we cannot sign XML documents in the
way we were doing. SAMLAuthenticator tests in a FIPS JVM can be
reenabled with precomputed and signed SAML messages at a later stage.

IT will be covered in a subsequent PR
jkakavas added a commit that referenced this pull request Jul 20, 2018
* Enable testing in FIPS140 JVM (#31666)

Ensure our tests can run in a FIPS JVM

JKS keystores cannot be used in a FIPS JVM as attempting to use one
in order to init a KeyManagerFactory is not allowed.( JKS keystore 
algorithms for private key encryption are not FIPS 140 approved)
This commit replaces JKS keystores in our tests with the
corresponding PEM encoded key and certificates both for key and trust
configurations.
Whenever it's not possible to refactor the test, i.e. when we are
testing that we can load a JKS keystore, etc. we attempt to
mute the test when we are running in FIPS 140 JVM. Testing for the
JVM is naive and is based on the name of the security provider as
we would control the testing infrastrtucture and so this would be
reliable enough.
Other cases of tests being muted are the ones that involve custom
TrustStoreManagers or KeyStoreManagers, null TLS Ciphers and the
SAMLAuthneticator class as we cannot sign XML documents in the
way we were doing. SAMLAuthenticator tests in a FIPS JVM can be
reenabled with precomputed and signed SAML messages at a later stage.

IT will be covered in a subsequent PR
martijnvg added a commit that referenced this pull request Jul 21, 2018
* es/6.x: (24 commits)
  Fix broken backport
  Switch full-cluster-restart to new style Requests (#32140)
  Fix multi level nested sort (#32204)
  MINOR: Remove unused `IndexDynamicSettings` (#32237) (#32248)
  [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236)
  Switch rolling restart to new style Requests (#32147)
  Enhance Parent circuit breaker error message (#32056)
  [ML] Use default request durability for .ml-state index (#32233)
  Enable testing in FIPS140 JVM (#31666) (#32231)
  Remove indices stats timeout from monitoring docs
  TESTS: Check for Netty resource leaks (#31861) (#32225)
  Rename ranking evaluation response section (#32166)
  Dependencies: Upgrade to joda time 2.10 (#32160)
  Backport SSL context names (#30953) to 6.x (#32223)
  Require Gradle 4.9  as minimum version (#32200)
  Detect old trial licenses and mimic behaviour (#32209)
  Painless: Simplify Naming in Lookup Package (#32177)
  add support for write index resolution when creating/updating documents (#31520)
  A replica can be promoted and started in one cluster state update (#32042)
  Rest test - allow for snapshots to take 0 milliseconds
  ...
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Jul 24, 2018
Now that elastic#31666 and elastic#31989 are merged we can run our tests in
fips JVM. This commits enables us to run tests on a Java 8
JVM using BouncyCastleFIPS as a security Provider.
@jkakavas jkakavas mentioned this pull request Jul 24, 2018
jkakavas added a commit that referenced this pull request Jul 25, 2018
Now that #31666 and #31989 are merged we can run our tests in
fips JVM. This commits enables us to run tests on a Java 8
JVM using BouncyCastleFIPS as a security Provider.
jkakavas added a commit that referenced this pull request Jul 25, 2018
Now that #31666 and #31989 are merged we can run our tests in
fips JVM. This commits enables us to run tests on a Java 8
JVM using BouncyCastleFIPS as a security Provider.
@jkakavas jkakavas deleted the fips-testing branch September 14, 2018 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/TLS SSL/TLS, Certificates >test Issues or PRs that are addressing/adding tests v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants