From 95826f07c194580a73776392fd3792635b4fffc0 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 31 May 2018 09:58:48 +0300 Subject: [PATCH 1/2] Ensure that a purposefully wrong key is used Uses a specific keypair for tests that require a purposefully wrong keypair instead of selecting one randomly from the same pull from which the correct one is selected. Entropy is low because of the small space and the same key can be randomly selected as both the correct one and the wrong one, causing the tests to fail. Resolves #30970 --- .../xpack/security/authc/saml/SamlAuthenticatorTests.java | 5 ++--- .../xpack/security/authc/saml/SamlTestCase.java | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java index ef60764d26de5..54e2fee9aa30f 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java @@ -374,7 +374,7 @@ public void testFailWhenAssertionsCannotBeDecrypted() throws Exception { final String xml = getSimpleResponse(now); // Encrypting with different cert instead of sp cert will mean that the SP cannot decrypt - final String encrypted = encryptAssertions(xml, readKeyPair("RSA_1024")); + final String encrypted = encryptAssertions(xml, readKeyPair("RSA_4096_updated")); assertThat(encrypted, not(equalTo(xml))); final String signed = signDoc(encrypted); @@ -896,7 +896,6 @@ public void testIdpInitiatedLoginIsAllowed() throws Exception { assertThat(attributes.attributes(), iterableWithSize(1)); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/30970") public void testIncorrectSigningKeyIsRejected() throws Exception { final CryptoTransform signer = randomBoolean() ? this::signDoc : this::signAssertions; Instant now = clock.instant(); @@ -938,7 +937,7 @@ public void testIncorrectSigningKeyIsRejected() throws Exception { assertThat(authenticator.authenticate(token(signer.transform(xml, idpSigningCertificatePair))), notNullValue()); // check is rejected when signed by a different key-pair - final Tuple wrongKey = readRandomKeyPair(randomSigningAlgorithm()); + final Tuple wrongKey = readKeyPair("RSA_4096_updated"); final ElasticsearchSecurityException exception = expectThrows(ElasticsearchSecurityException.class, () -> authenticator.authenticate(token(signer.transform(xml, wrongKey)))); assertThat(exception.getMessage(), containsString("SAML Signature")); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlTestCase.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlTestCase.java index bbd98445295d5..51a6d8732a5b3 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlTestCase.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlTestCase.java @@ -71,7 +71,7 @@ protected static Tuple readRandomKeyPair() throws E } /** - * Generates key pair for given algorithm and then associates with a certificate. + * Reads a key pair and associated certificate for given algorithm and key length * For testing, for "EC" algorithm 256 key size is used, others use 2048 as default. * @param algorithm * @return X509Certificate a signed certificate, it's PrivateKey {@link Tuple} From 8db000cae6af2824dee025389f49a0859ff550d1 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 31 May 2018 14:00:16 +0300 Subject: [PATCH 2/2] Ensure that the purposefully wrong key is not used in other tests saml_RSA_4096_updated.key is used in testSigningKeyIsReloadedForEachRequest and was not cleaned up afterwards so if testIncorrectKeyIsRejected ran after that, the assertion would be signed with that key --- .../xpack/security/authc/saml/SamlAuthenticatorTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java index 54e2fee9aa30f..8bb9890151ff0 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java @@ -953,10 +953,12 @@ public void testSigningKeyIsReloadedForEachRequest() throws Exception { assertThat(authenticator.authenticate(token(signer.transform(xml, idpSigningCertificatePair))), notNullValue()); final Tuple oldKeyPair = idpSigningCertificatePair; - //Ensure we won't read any of the ones we could have picked randomly before + // Ensure we won't read any of the ones we could have picked randomly before idpSigningCertificatePair = readKeyPair("RSA_4096_updated"); assertThat(idpSigningCertificatePair.v2(), not(equalTo(oldKeyPair.v2()))); assertThat(authenticator.authenticate(token(signer.transform(xml, idpSigningCertificatePair))), notNullValue()); + // Restore the keypair to one from the keypair pool of all algorithms and keys + idpSigningCertificatePair = readRandomKeyPair(randomSigningAlgorithm()); } public void testParsingRejectsTamperedContent() throws Exception {