[IdP plugin] Fix exception handling (elastic#106231)

* Add regression tests that test ACS and entity id mismatch, causing us to go into the initCause branch * Fix up exception creation: initCause it not allowed because ElasticsearchException initialises the cause to `null` already if it isn't passed as a contructor param. Signed-off-by: lloydmeta <[email protected]>
lkts · Mar 13, 2024 · d47a461 · d47a461
1 parent e435129
commit d47a461
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 9 deletions.
diff --git a/...ests/src/javaRestTest/java/org/elasticsearch/xpack/idp/WildcardServiceProviderRestIT.java b/...ests/src/javaRestTest/java/org/elasticsearch/xpack/idp/WildcardServiceProviderRestIT.java
@@ -8,8 +8,10 @@
 
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.Response;
+import org.elasticsearch.client.ResponseException;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.settings.SecureString;
+import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.xcontent.XContentBuilder;
 import org.elasticsearch.xcontent.XContentType;
 import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken;
@@ -87,6 +89,35 @@ public void testInitSingleSignOnToWildcardServiceProvider() throws Exception {
         deleteRole(roleName);
     }
 
+    public void testInitSingleSignOnToWildcardServiceProviderWithMismatchedACSandEntityIds() throws Exception {
+        final String owner = randomAlphaOfLength(8);
+        final String service = randomAlphaOfLength(8);
+        // From "wildcard_services.json"
+        final String entityId = "service:" + owner + ":" + service;
+        final String acs = "https://" + service + "extra_stuff_lol" + ".services.example.com/api/v1/saml";
+
+        final String username = randomAlphaOfLength(6);
+        final SecureString password = new SecureString((randomAlphaOfLength(6) + randomIntBetween(10, 99)).toCharArray());
+        final String roleName = username + "_role";
+        final User user = createUser(username, password, roleName);
+
+        final RoleDescriptor.ApplicationResourcePrivileges applicationPrivilege = RoleDescriptor.ApplicationResourcePrivileges.builder()
+            .application("elastic-cloud")
+            .privileges("sso:admin")
+            .resources("sso:" + entityId)
+            .build();
+        createRole(roleName, List.of(), List.of(), List.of(applicationPrivilege));
+
+        ResponseException exception = expectThrows(
+            ResponseException.class,
+            () -> initSso(entityId, acs, new UsernamePasswordToken(username, password))
+        );
+        assertThat(exception.getResponse().getStatusLine().getStatusCode(), equalTo(RestStatus.BAD_REQUEST.getStatus()));
+
+        deleteUser(username);
+        deleteRole(roleName);
+    }
+
     private void getMetadata(String entityId, String acs) throws IOException {
         final Map<String, Object> map = getAsMap("/_idp/saml/metadata/" + encode(entityId) + "?acs=" + encode(acs));
         assertThat(map, notNullValue());

diff --git a/...main/java/org/elasticsearch/xpack/idp/action/TransportSamlInitiateSingleSignOnAction.java b/...main/java/org/elasticsearch/xpack/idp/action/TransportSamlInitiateSingleSignOnAction.java
@@ -91,6 +91,7 @@ protected void doExecute(
                             StatusCode.RESPONDER,
                             RestStatus.BAD_REQUEST,
                             "Service Provider with Entity ID [{}] and ACS [{}] is not known to this Identity Provider",
+                            null,
                             request.getSpEntityId(),
                             request.getAssertionConsumerService()
                         )
@@ -108,7 +109,8 @@ protected void doExecute(
                             request.getAssertionConsumerService(),
                             StatusCode.REQUESTER,
                             RestStatus.FORBIDDEN,
-                            "Request is missing secondary authentication"
+                            "Request is missing secondary authentication",
+                            null
                         )
                     );
                     return;
@@ -124,6 +126,7 @@ protected void doExecute(
                                 StatusCode.REQUESTER,
                                 RestStatus.FORBIDDEN,
                                 "User [{}] is not permitted to access service [{}]",
+                                null,
                                 secondaryAuthentication.getUser().principal(),
                                 sp.getEntityId()
                             )
@@ -217,6 +220,7 @@ private SamlInitiateSingleSignOnException buildSamlInitiateSingleSignOnException
         final String statusCode,
         final RestStatus restStatus,
         final String messageFormatStr,
+        final Exception cause,
         final Object... args
     ) {
         final SamlInitiateSingleSignOnException ex;
@@ -231,10 +235,11 @@ private SamlInitiateSingleSignOnException buildSamlInitiateSingleSignOnException
             ex = new SamlInitiateSingleSignOnException(
                 exceptionMessage,
                 restStatus,
+                cause,
                 new SamlInitiateSingleSignOnResponse(spEntityId, acsUrl, samlFactory.getXmlContent(response), statusCode, exceptionMessage)
             );
         } else {
-            ex = new SamlInitiateSingleSignOnException(exceptionMessage, restStatus);
+            ex = new SamlInitiateSingleSignOnException(exceptionMessage, restStatus, cause);
         }
         return ex;
     }
@@ -247,15 +252,14 @@ private SamlInitiateSingleSignOnException buildResponderSamlInitiateSingleSignOn
     ) {
         final String exceptionMessage = cause.getMessage();
         final RestStatus restStatus = ExceptionsHelper.status(cause);
-        final SamlInitiateSingleSignOnException ex = buildSamlInitiateSingleSignOnException(
+        return buildSamlInitiateSingleSignOnException(
             authenticationState,
             spEntityId,
             acsUrl,
             StatusCode.RESPONDER,
             restStatus,
-            exceptionMessage
+            exceptionMessage,
+            cause
         );
-        ex.initCause(cause);
-        return ex;
     }
 }
diff --git a/...main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnException.java b/...main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnException.java
@@ -21,14 +21,15 @@ public class SamlInitiateSingleSignOnException extends ElasticsearchSecurityExce
     public SamlInitiateSingleSignOnException(
         String msg,
         RestStatus status,
+        Exception cause,
         SamlInitiateSingleSignOnResponse samlInitiateSingleSignOnResponse
     ) {
-        super(msg, status);
+        super(msg, status, cause);
         this.samlInitiateSingleSignOnResponse = samlInitiateSingleSignOnResponse;
     }
 
-    public SamlInitiateSingleSignOnException(String msg, RestStatus status) {
-        super(msg, status);
+    public SamlInitiateSingleSignOnException(String msg, RestStatus status, Exception cause) {
+        super(msg, status, cause);
     }
 
     @Override

diff --git a/...c/test/java/org/elasticsearch/xpack/idp/saml/sp/WildcardServiceProviderResolverTests.java b/...c/test/java/org/elasticsearch/xpack/idp/saml/sp/WildcardServiceProviderResolverTests.java
@@ -175,6 +175,11 @@ public void testResolveServices() throws IOException {
         assertThat(sp4.getAssertionConsumerService().toString(), equalTo("https://saml.example.net/12345/acs"));
         assertThat(sp4.getName(), equalTo("12345 at example.net"));
         assertThat(sp4.getPrivileges().getResource(), equalTo("service2:example:12345"));
+
+        expectThrows(
+            IllegalArgumentException.class,
+            () -> resolver.resolve("https://zbcdef.example.com/", "https://abcdef.service.example.com/saml2/acs")
+        );
     }
 
     public void testCaching() throws IOException {