honeymoose/OpenSearch
1
0
Fork 0
mirror of https://github.com/honeymoose/OpenSearch.git synced 2025-02-17 10:25:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

Expand logging when SAML Audience condition fails (#45027)

Browse Source 
A mismatched configuration between the IdP and SP will often result in
SAML authentication attempts failing because the audience condition is
not met (because the IdP and SP disagree about the correct form of the
SP's Entity ID).

Previously the error message in this case did not provide sufficient
information to resolve the issue because the IdP's expected audience
would be truncated if it exceeeded 32 characters. Since the error did
not provide both IDs in full, it was not possible to determine the
correct fix (in detail) based on the error alone.

This change expands the message that is included in the thrown
exception, and also adds additional logging of every failed audience
condition, with diagnostics of the match failure.

Backport of: #44334
This commit is contained in:
Tim Vernum 2019-07-31 19:40:17 +10:00 committed by GitHub
parent 5e3010a606
commit 3c17d4379d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 159 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticator.java
View File

@ -5,15 +5,6 @@
*/
package org.elasticsearch.xpack.security.authc.saml;
import java.time.Clock;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.common.Strings;
@ -41,6 +32,14 @@ import org.opensaml.xmlsec.encryption.support.DecryptionException;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
import java.time.Clock;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;
import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException;
import static org.opensaml.saml.saml2.core.SubjectConfirmation.METHOD_BEARER;
@ -354,15 +353,37 @@ class SamlAuthenticator extends SamlRequestHandler {
}
private void checkAudienceRestrictions(List<AudienceRestriction> restrictions) {
final String spEntityId = this.getSpConfiguration().getEntityId();
final Predicate<AudienceRestriction> predicate = ar ->
ar.getAudiences().stream().map(Audience::getAudienceURI).anyMatch(spEntityId::equals);
if (restrictions.stream().allMatch(predicate) == false) {
if (restrictions.stream().allMatch(this::checkAudienceRestriction) == false) {
throw samlException("Conditions [{}] do not match required audience [{}]",
restrictions.stream().map(r -> text(r, 32)).collect(Collectors.joining(" | ")), getSpConfiguration().getEntityId());
restrictions.stream().map(r -> text(r, 56, 8)).collect(Collectors.joining(" | ")), getSpConfiguration().getEntityId());
}
}
private boolean checkAudienceRestriction(AudienceRestriction restriction) {
final String spEntityId = this.getSpConfiguration().getEntityId();
if (restriction.getAudiences().stream().map(Audience::getAudienceURI).anyMatch(spEntityId::equals) == false) {
restriction.getAudiences().stream().map(Audience::getAudienceURI).forEach(uri -> {
int diffChar;
for (diffChar = 0; diffChar < uri.length() && diffChar < spEntityId.length(); diffChar++) {
if (uri.charAt(diffChar) != spEntityId.charAt(diffChar)) {
break;
}
}
// If the difference is less than half the length of the string, show it in detail
if (diffChar >= spEntityId.length() / 2) {
logger.info("Audience restriction [{}] does not match required audience [{}] " +
"(difference starts at character [#{}] [{}] vs [{}])",
uri, spEntityId, diffChar, uri.substring(diffChar), spEntityId.substring(diffChar));
} else {
logger.info("Audience restriction [{}] does not match required audience [{}]", uri, spEntityId);
}
});
return false;
}
return true;
}
private void checkLifetimeRestrictions(Conditions conditions) {
// In order to compensate for clock skew we construct 2 alternate realities
// - a "future now" that is now + the maximum skew we will tolerate. Essentially "if our clock is 2min slow, what time is it now?"

22
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandler.java
View File

@ -5,8 +5,8 @@
*/
package org.elasticsearch.xpack.security.authc.saml;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.common.CheckedFunction;
@ -44,6 +44,7 @@ import org.w3c.dom.Document;
import org.w3c.dom.Element;
import org.xml.sax.SAXException;
import javax.xml.parsers.DocumentBuilder;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.security.PrivilegedActionException;
@ -59,8 +60,6 @@ import java.util.List;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import javax.xml.parsers.DocumentBuilder;
import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException;
import static org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport.getUnmarshallerFactory;
@ -265,13 +264,26 @@ public class SamlRequestHandler {
}
protected String text(XMLObject xml, int length) {
return text(xml, length, 0);
}
protected static String text(XMLObject xml, int prefixLength, int suffixLength) {
final Element dom = xml.getDOM();
if (dom == null) {
return null;
}
final String text = dom.getTextContent().trim();
if (text.length() >= length) {
return Strings.cleanTruncate(text, length) + "...";
final int totalLength = prefixLength + suffixLength;
if (text.length() > totalLength) {
final String prefix = Strings.cleanTruncate(text, prefixLength) + "...";
if (suffixLength == 0) {
return prefix;
}
int suffixIndex = text.length() - suffixLength;
if (Character.isHighSurrogate(text.charAt(suffixIndex))) {
suffixIndex++;
}
return prefix + text.substring(suffixIndex);
} else {
return text;
}

56
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java
View File

@ -5,7 +5,9 @@
*/
package org.elasticsearch.xpack.security.authc.saml;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.xml.security.Init;
import org.apache.xml.security.encryption.EncryptedData;
import org.apache.xml.security.encryption.EncryptedKey;
@ -17,7 +19,9 @@ import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.common.CheckedConsumer;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.test.MockLogAppender;
import org.elasticsearch.xpack.core.watcher.watch.ClockMock;
import org.hamcrest.Matchers;
import org.junit.AfterClass;
@ -76,6 +80,7 @@ import java.util.Map;
import java.util.Set;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
@ -1355,7 +1360,7 @@ public class SamlAuthenticatorTests extends SamlTestCase {
public void testContentIsRejectedIfRestrictedToADifferentAudience() throws Exception {
final String audience = "https://some.other.sp/SAML2";
final String xml = getResponseWithAudienceRestriction(audience);
final String xml = getResponseWithAudienceRestrictions(audience);
final SamlToken token = token(signDoc(xml));
final ElasticsearchSecurityException exception = expectSamlException(() -> authenticator.authenticate(token));
assertThat(exception.getMessage(), containsString("required audience"));
@ -1366,13 +1371,47 @@ public class SamlAuthenticatorTests extends SamlTestCase {
}
public void testContentIsAcceptedIfRestrictedToOurAudience() throws Exception {
final String xml = getResponseWithAudienceRestriction(SP_ENTITY_ID);
final String xml = getResponseWithAudienceRestrictions(SP_ENTITY_ID);
final SamlToken token = token(signDoc(xml));
final SamlAttributes attributes = authenticator.authenticate(token);
assertThat(attributes, notNullValue());
assertThat(attributes.attributes(), not(empty()));
}
public void testLoggingWhenAudienceCheckFails() throws Exception {
final String similarAudience = SP_ENTITY_ID.replaceFirst("/$", ":80/");
final String wrongAudience = "http://" + randomAlphaOfLengthBetween(4, 12) + "." + randomAlphaOfLengthBetween(6, 8) + "/";
final String xml = getResponseWithAudienceRestrictions(similarAudience, wrongAudience);
final SamlToken token = token(signDoc(xml));
final Logger samlLogger = LogManager.getLogger(authenticator.getClass());
final MockLogAppender mockAppender = new MockLogAppender();
mockAppender.start();
try {
Loggers.addAppender(samlLogger, mockAppender);
mockAppender.addExpectation(new MockLogAppender.SeenEventExpectation(
"similar audience",
authenticator.getClass().getName(),
Level.INFO,
"Audience restriction [" + similarAudience + "] does not match required audience [" + SP_ENTITY_ID +
"] (difference starts at character [#" + (SP_ENTITY_ID.length() - 1) + "] [:80/] vs [/])"
));
mockAppender.addExpectation(new MockLogAppender.SeenEventExpectation(
"not similar audience",
authenticator.getClass().getName(),
Level.INFO,
"Audience restriction [" + wrongAudience + "] does not match required audience [" + SP_ENTITY_ID + "]"
));
final ElasticsearchSecurityException exception = expectSamlException(() -> authenticator.authenticate(token));
assertThat(exception.getMessage(), containsString("required audience"));
mockAppender.assertAllExpectationsMatched();
} finally {
Loggers.removeAppender(samlLogger, mockAppender);
mockAppender.stop();
}
}
public void testContentIsRejectedIfNotMarkedAsSuccess() throws Exception {
final String xml = getStatusFailedResponse();
final SamlToken token = token(signDoc(xml));
@ -2101,14 +2140,13 @@ public class SamlAuthenticatorTests extends SamlTestCase {
"</proto:Response>";
}
private String getResponseWithAudienceRestriction(String requiredAudience) {
private String getResponseWithAudienceRestrictions(String... requiredAudiences) {
String inner = Stream.of(requiredAudiences)
.map(s -> "<assert:Audience>" + s + "</assert:Audience>")
.collect(Collectors.joining());
return getSimpleResponse(clock.instant()).replaceFirst("<assert:AuthnStatement",
"<assert:Conditions><assert:AudienceRestriction>" +
"<assert:Audience>" +
requiredAudience +
"</assert:Audience>" +
"</assert:AudienceRestriction></assert:Conditions>" +
"$0");
"<assert:Conditions><assert:AudienceRestriction>" + inner + "</assert:AudienceRestriction></assert:Conditions>" +
"$0");
}
private String randomId() {

60
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandlerTests.java Normal file
View File

@ -0,0 +1,60 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
package org.elasticsearch.xpack.security.authc.saml;
import org.elasticsearch.test.ESTestCase;
import org.opensaml.core.xml.XMLObject;
import org.w3c.dom.Element;
import static org.hamcrest.Matchers.equalTo;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
public class SamlRequestHandlerTests extends ESTestCase {
public void testXmlObjectToTextWhenExceedsLength() {
final int prefixLength = randomIntBetween(10, 30);
final int suffixLength = randomIntBetween(5, 20);
final String prefix = randomAlphaOfLength(prefixLength);
final String suffix = randomAlphaOfLength(suffixLength);
final String text = prefix + randomAlphaOfLengthBetween(1, 50) + suffix;
final XMLObject xml = mock(XMLObject.class);
final Element element = mock(Element.class);
when(xml.getDOM()).thenReturn(element);
when(element.getTextContent()).thenReturn(text);
assertThat(SamlRequestHandler.text(xml, prefixLength, suffixLength), equalTo(prefix + "..." + suffix));
}
public void testXmlObjectToTextPrefixOnly() {
final int length = randomIntBetween(10, 30);
final String prefix = randomAlphaOfLength(length);
final String text = prefix + randomAlphaOfLengthBetween(1, 50);
final XMLObject xml = mock(XMLObject.class);
final Element element = mock(Element.class);
when(xml.getDOM()).thenReturn(element);
when(element.getTextContent()).thenReturn(text);
assertThat(SamlRequestHandler.text(xml, length, 0), equalTo(prefix + "..."));
}
public void testXmlObjectToTextWhenShortedThanRequiredLength() {
final int prefixLength = randomIntBetween(10, 30);
final int suffixLength = randomIntBetween(10, 25);
final String text = randomAlphaOfLengthBetween(prefixLength + 1, prefixLength + suffixLength);
final XMLObject xml = mock(XMLObject.class);
final Element element = mock(Element.class);
when(xml.getDOM()).thenReturn(element);
when(element.getTextContent()).thenReturn(text);
assertThat(SamlRequestHandler.text(xml, prefixLength, suffixLength), equalTo(text));
}
}