diff --git a/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java b/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java index 9d4be55580..73d834dedc 100644 --- a/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java +++ b/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java @@ -140,7 +140,7 @@ private AuthTokenProcessorAction.Response handleImpl( String samlRequestId, String acsEndpoint, Saml2Settings saml2Settings - ) { + ) throws Exception { if (token_log.isDebugEnabled()) { try { token_log.debug( @@ -172,10 +172,10 @@ private AuthTokenProcessorAction.Response handleImpl( return responseBody; } catch (ValidationError e) { log.warn("Error while validating SAML response", e); - return null; + throw e; } catch (Exception e) { log.error("Error while converting SAML to JWT", e); - return null; + throw e; } } @@ -236,6 +236,12 @@ private Optional handleLowLevel(RestRequest restRequest) throw } catch (JsonProcessingException e) { log.warn("Error while parsing JSON for /_opendistro/_security/api/authtoken", e); return Optional.of(new SecurityResponse(HttpStatus.SC_BAD_REQUEST, "JSON could not be parsed")); + } catch (ValidationError e) { + log.warn("Error while validating SAML response", e); + return Optional.of(new SecurityResponse(HttpStatus.SC_BAD_REQUEST, "Error while validating SAML response")); + } catch (Exception e) { + log.warn("Error while converting SAML to JWT", e); + return Optional.of(new SecurityResponse(HttpStatus.SC_BAD_REQUEST, "Error while converting SAML to JWT")); } } diff --git a/src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java index ae3d1c9128..13b85ff316 100644 --- a/src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java @@ -185,7 +185,7 @@ public Optional reRequestAuthentication(final SecurityRequest final String suffix = matcher.matches() ? matcher.group(2) : null; if (API_AUTHTOKEN_SUFFIX.equals(suffix)) { - // Verficiation of SAML ASC endpoint only works with RestRequests + // Verification of SAML ASC endpoint only works with RestRequests if (!(request instanceof OpenSearchRequest)) { throw new SecurityRequestChannelUnsupported( API_AUTHTOKEN_SUFFIX + " not supported for request of type " + request.getClass().getName() @@ -208,8 +208,7 @@ public Optional reRequestAuthentication(final SecurityRequest if (e instanceof SecurityRequestChannelUnsupported) { throw (SecurityRequestChannelUnsupported) e; } - log.error("Error in reRequestAuthentication()", e); - return Optional.empty(); + return Optional.of(new SecurityResponse(HttpStatus.SC_BAD_REQUEST, e.getMessage())); } } diff --git a/src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java b/src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java index 21efcccf74..99365017a3 100644 --- a/src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java +++ b/src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java @@ -59,9 +59,11 @@ import org.opensearch.security.util.FakeRestRequest; import com.nimbusds.jwt.SignedJWT; +import com.onelogin.saml2.exception.ValidationError; import org.opensaml.saml.saml2.core.NameIDType; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator.IDP_METADATA_CONTENT; import static com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator.IDP_METADATA_URL; @@ -442,7 +444,7 @@ public void testMetadataBody() throws Exception { mockSamlIdpServer.loadSigningKeys("saml/kirk-keystore.jks", "kirk"); mockSamlIdpServer.setAuthenticateUser("horst"); mockSamlIdpServer.setEndpointQueryString(null); - mockSamlIdpServer.setAuthenticateUserRoles(Arrays.asList("Admin", "Developer")); + mockSamlIdpServer.setAuthenticateUserRoles(Arrays.asList("HR", "IT")); // Note: We need to replace endpoint with mockSamlIdpServer endpoint final String metadataBody = FileHelper.loadFile("saml/metadata.xml") @@ -457,37 +459,48 @@ public void testMetadataBody() throws Exception { .put("path.home", ".") .build(); - HTTPSamlAuthenticator samlAuthenticator = new HTTPSamlAuthenticator(settings, null); + SignedJWT jwt = getSignedJWT(settings); - AuthenticateHeaders authenticateHeaders = getAutenticateHeaders(samlAuthenticator); + Assert.assertEquals(List.of("HR", "IT"), jwt.getJWTClaimsSet().getClaim("roles")); + Assert.assertEquals("horst", jwt.getJWTClaimsSet().getClaim("sub")); + } - String encodedSamlResponse = mockSamlIdpServer.handleSsoGetRequestURI(authenticateHeaders.location); + @Test + public void testRolesWithRepeatAttributeNames() throws Exception { + mockSamlIdpServer.setSignResponses(true); + mockSamlIdpServer.loadSigningKeys("saml/kirk-keystore.jks", "kirk"); + mockSamlIdpServer.setAuthenticateUser("horst"); + mockSamlIdpServer.setEndpointQueryString(null); + mockSamlIdpServer.setAuthenticateUserRolesAsRepeatedAttributeName(Arrays.asList("Admin", "Developer")); - RestRequest tokenRestRequest = buildTokenExchangeRestRequest(encodedSamlResponse, authenticateHeaders); - String responseJson = getResponse(samlAuthenticator, tokenRestRequest); - HashMap response = DefaultObjectMapper.objectMapper.readValue( - responseJson, - new TypeReference>() { - } - ); - String authorization = (String) response.get("authorization"); + // Note: We need to replace endpoint with mockSamlIdpServer endpoint + final String metadataBody = FileHelper.loadFile("saml/metadata.xml") + .replaceAll("http://localhost:33667/", mockSamlIdpServer.getMetadataUri()); - Assert.assertNotNull("Expected authorization attribute in JSON: " + responseJson, authorization); + Settings settings = Settings.builder() + .put(IDP_METADATA_CONTENT, metadataBody) + .put("kibana_url", "http://wherever") + .put("idp.entity_id", mockSamlIdpServer.getIdpEntityId()) + .put("exchange_key", "abc") + .put("roles_key", "role") + .put("allow_repeat_attribute_names", true) + .put("path.home", ".") + .build(); - SignedJWT jwt = SignedJWT.parse(authorization.replaceAll("\\s*bearer\\s*", "")); + SignedJWT jwt = getSignedJWT(settings); Assert.assertEquals(List.of("Admin", "Developer"), jwt.getJWTClaimsSet().getClaim("roles")); Assert.assertEquals("horst", jwt.getJWTClaimsSet().getClaim("sub")); } @Test - public void testMetadataBodyAllowRepeatAttributeNames() throws Exception { + public void testRolesWithMixOfRepeatAttributeNamesAndListOfAttributesWithSingleName_rolesKeyRepeated() throws Exception { mockSamlIdpServer.setSignResponses(true); mockSamlIdpServer.loadSigningKeys("saml/kirk-keystore.jks", "kirk"); mockSamlIdpServer.setAuthenticateUser("horst"); mockSamlIdpServer.setEndpointQueryString(null); - mockSamlIdpServer.setAuthenticateUserRoles(Arrays.asList("Admin", "Developer")); - mockSamlIdpServer.setAllowRepeatAttributeNames(true); + mockSamlIdpServer.setAuthenticateUserRoles(Arrays.asList("HR", "IT")); + mockSamlIdpServer.setAuthenticateUserRolesAsRepeatedAttributeName(Arrays.asList("Admin", "Developer")); // Note: We need to replace endpoint with mockSamlIdpServer endpoint final String metadataBody = FileHelper.loadFile("saml/metadata.xml") @@ -498,11 +511,67 @@ public void testMetadataBodyAllowRepeatAttributeNames() throws Exception { .put("kibana_url", "http://wherever") .put("idp.entity_id", mockSamlIdpServer.getIdpEntityId()) .put("exchange_key", "abc") - .put("roles_key", "role") + .put("roles_key", "role") // "role" will extract roles using the repeated attribute names + .put("allow_repeat_attribute_names", true) + .put("path.home", ".") + .build(); + + SignedJWT jwt = getSignedJWT(settings); + + Assert.assertEquals(List.of("Admin", "Developer"), jwt.getJWTClaimsSet().getClaim("roles")); + Assert.assertEquals("horst", jwt.getJWTClaimsSet().getClaim("sub")); + } + + @Test + public void testRolesWithMixOfRepeatAttributeNamesAndListOfAttributesWithSingleName__rolesKeySingular() throws Exception { + mockSamlIdpServer.setSignResponses(true); + mockSamlIdpServer.loadSigningKeys("saml/kirk-keystore.jks", "kirk"); + mockSamlIdpServer.setAuthenticateUser("horst"); + mockSamlIdpServer.setEndpointQueryString(null); + mockSamlIdpServer.setAuthenticateUserRoles(Arrays.asList("HR", "IT")); + mockSamlIdpServer.setAuthenticateUserRolesAsRepeatedAttributeName(Arrays.asList("Admin", "Developer")); + + // Note: We need to replace endpoint with mockSamlIdpServer endpoint + final String metadataBody = FileHelper.loadFile("saml/metadata.xml") + .replaceAll("http://localhost:33667/", mockSamlIdpServer.getMetadataUri()); + + Settings settings = Settings.builder() + .put(IDP_METADATA_CONTENT, metadataBody) + .put("kibana_url", "http://wherever") + .put("idp.entity_id", mockSamlIdpServer.getIdpEntityId()) + .put("exchange_key", "abc") + .put("roles_key", "roles") // "role" will extract roles using the repeated attribute names .put("allow_repeat_attribute_names", true) .put("path.home", ".") .build(); + SignedJWT jwt = getSignedJWT(settings); + + Assert.assertEquals(List.of("HR", "IT"), jwt.getJWTClaimsSet().getClaim("roles")); + Assert.assertEquals("horst", jwt.getJWTClaimsSet().getClaim("sub")); + } + + @Test + public void testThrowsExceptionWhenRepeatAttributeNamesForbidden() throws Exception { + mockSamlIdpServer.setSignResponses(true); + mockSamlIdpServer.loadSigningKeys("saml/kirk-keystore.jks", "kirk"); + mockSamlIdpServer.setAuthenticateUser("horst"); + mockSamlIdpServer.setEndpointQueryString(null); + mockSamlIdpServer.setAuthenticateUserRolesAsRepeatedAttributeName(Arrays.asList("Admin", "Developer")); + + // Note: We need to replace endpoint with mockSamlIdpServer endpoint + final String metadataBody = FileHelper.loadFile("saml/metadata.xml") + .replaceAll("http://localhost:33667/", mockSamlIdpServer.getMetadataUri()); + + Settings settings = Settings.builder() + .put(IDP_METADATA_CONTENT, metadataBody) + .put("kibana_url", "http://wherever") + .put("idp.entity_id", mockSamlIdpServer.getIdpEntityId()) + .put("exchange_key", "abc") + .put("roles_key", "role") + .put("path.home", ".") + .build(); + HTTPSamlAuthenticator samlAuthenticator = new HTTPSamlAuthenticator(settings, null); AuthenticateHeaders authenticateHeaders = getAutenticateHeaders(samlAuthenticator); @@ -511,19 +580,7 @@ public void testMetadataBodyAllowRepeatAttributeNames() throws Exception { RestRequest tokenRestRequest = buildTokenExchangeRestRequest(encodedSamlResponse, authenticateHeaders); String responseJson = getResponse(samlAuthenticator, tokenRestRequest); - HashMap response = DefaultObjectMapper.objectMapper.readValue( - responseJson, - new TypeReference>() { - } - ); - String authorization = (String) response.get("authorization"); - - Assert.assertNotNull("Expected authorization attribute in JSON: " + responseJson, authorization); - - SignedJWT jwt = SignedJWT.parse(authorization.replaceAll("\\s*bearer\\s*", "")); - - Assert.assertEquals(List.of("Admin", "Developer"), jwt.getJWTClaimsSet().getClaim("roles")); - Assert.assertEquals("horst", jwt.getJWTClaimsSet().getClaim("sub")); + assertThat(responseJson, containsString("Error while validating SAML response")); } @Test(expected = RuntimeException.class) @@ -900,7 +957,8 @@ public void initialConnectionFailureTest() throws Exception { RestRequest restRequest = new FakeRestRequest(ImmutableMap.of(), new HashMap()); Optional maybeResponse = sendToAuthenticator(samlAuthenticator, restRequest); - assertThat(maybeResponse.isPresent(), Matchers.equalTo(false)); + assertThat(maybeResponse.isPresent(), Matchers.equalTo(true)); + assertThat(maybeResponse.get().getBody(), containsString("Could not find entity descriptor for http://test.entity")); mockSamlIdpServer.start(); @@ -931,6 +989,31 @@ public void initialConnectionFailureTest() throws Exception { } } + private SignedJWT getSignedJWT(Settings samlAuthenticatorSettings) throws Exception { + HTTPSamlAuthenticator samlAuthenticator = new HTTPSamlAuthenticator(samlAuthenticatorSettings, null); + + AuthenticateHeaders authenticateHeaders = getAutenticateHeaders(samlAuthenticator); + + String encodedSamlResponse = mockSamlIdpServer.handleSsoGetRequestURI(authenticateHeaders.location); + + RestRequest tokenRestRequest = buildTokenExchangeRestRequest(encodedSamlResponse, authenticateHeaders); + String responseJson = getResponse(samlAuthenticator, tokenRestRequest); + + if (responseJson.contains("Error while validating SAML response")) { + throw new ValidationError("Found an Attribute element with duplicated Name", 0); + } + HashMap response = DefaultObjectMapper.objectMapper.readValue( + responseJson, + new TypeReference>() { + } + ); + String authorization = (String) response.get("authorization"); + + Assert.assertNotNull("Expected authorization attribute in JSON: " + responseJson, authorization); + + return SignedJWT.parse(authorization.replaceAll("\\s*bearer\\s*", "")); + } + private AuthenticateHeaders getAutenticateHeaders(HTTPSamlAuthenticator samlAuthenticator) { RestRequest restRequest = new FakeRestRequest(ImmutableMap.of(), new HashMap()); SecurityResponse response = sendToAuthenticator(samlAuthenticator, restRequest).orElseThrow(); diff --git a/src/test/java/com/amazon/dlic/auth/http/saml/MockSamlIdpServer.java b/src/test/java/com/amazon/dlic/auth/http/saml/MockSamlIdpServer.java index c66c1d5e55..a15444e270 100644 --- a/src/test/java/com/amazon/dlic/auth/http/saml/MockSamlIdpServer.java +++ b/src/test/java/com/amazon/dlic/auth/http/saml/MockSamlIdpServer.java @@ -172,7 +172,7 @@ class MockSamlIdpServer implements Closeable { private Credential signingCredential; private String authenticateUser; private List authenticateUserRoles; - private boolean allowRepeatAttributes = false; + private List authenticateUserRolesAsRepeatedAttributeName; private int baseId = 1; private boolean signResponses = true; private X509Certificate spSignatureCertificate; @@ -466,31 +466,31 @@ private String createSamlAuthResponse(AuthnRequest authnRequest) { conditions.setNotOnOrAfter(Instant.now().plus(1, ChronoUnit.MINUTES)); if (authenticateUserRoles != null) { - if (!allowRepeatAttributes) { + AttributeStatement attributeStatement = createSamlElement(AttributeStatement.class); + assertion.getAttributeStatements().add(attributeStatement); + + Attribute attribute = createSamlElement(Attribute.class); + attributeStatement.getAttributes().add(attribute); + + attribute.setName("roles"); + attribute.setNameFormat("urn:oasis:names:tc:SAML:2.0:attrname-format:basic"); + + for (String role : authenticateUserRoles) { + attribute.getAttributeValues().add(createXSAny(AttributeValue.DEFAULT_ELEMENT_NAME, role)); + } + } + + if (authenticateUserRolesAsRepeatedAttributeName != null) { + for (String role : authenticateUserRolesAsRepeatedAttributeName) { AttributeStatement attributeStatement = createSamlElement(AttributeStatement.class); assertion.getAttributeStatements().add(attributeStatement); Attribute attribute = createSamlElement(Attribute.class); attributeStatement.getAttributes().add(attribute); - attribute.setName("roles"); + attribute.setName("role"); attribute.setNameFormat("urn:oasis:names:tc:SAML:2.0:attrname-format:basic"); - - for (String role : authenticateUserRoles) { - attribute.getAttributeValues().add(createXSAny(AttributeValue.DEFAULT_ELEMENT_NAME, role)); - } - } else { - for (String role : authenticateUserRoles) { - AttributeStatement attributeStatement = createSamlElement(AttributeStatement.class); - assertion.getAttributeStatements().add(attributeStatement); - - Attribute attribute = createSamlElement(Attribute.class); - attributeStatement.getAttributes().add(attribute); - - attribute.setName("role"); - attribute.setNameFormat("urn:oasis:names:tc:SAML:2.0:attrname-format:basic"); - attribute.getAttributeValues().add(createXSAny(AttributeValue.DEFAULT_ELEMENT_NAME, role)); - } + attribute.getAttributeValues().add(createXSAny(AttributeValue.DEFAULT_ELEMENT_NAME, role)); } } @@ -1127,8 +1127,12 @@ public void setAuthenticateUserRoles(List authenticateUserRoles) { this.authenticateUserRoles = authenticateUserRoles; } - public void setAllowRepeatAttributeNames(boolean allowRepeatAttributes) { - this.allowRepeatAttributes = allowRepeatAttributes; + public List getAuthenticateUserRolesAsRepeatedAttributeName() { + return authenticateUserRolesAsRepeatedAttributeName; + } + + public void setAuthenticateUserRolesAsRepeatedAttributeName(List authenticateUserRolesAsRepeatedAttributeName) { + this.authenticateUserRolesAsRepeatedAttributeName = authenticateUserRolesAsRepeatedAttributeName; } public boolean isSignResponses() {