From 4f9480fd2dd9ad70c674c5259d8f545b01391cbe Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 16 Oct 2023 12:27:38 -0700 Subject: [PATCH 1/3] Add a valid check for durationSeconds for generate obo endpoint Signed-off-by: Ryan Liang --- .../http/OnBehalfOfJwtAuthenticationTest.java | 19 +++++++++- .../onbehalf/CreateOnBehalfOfTokenAction.java | 38 +++++++++++++++---- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java index 266463d934..90e89e5986 100644 --- a/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java @@ -42,6 +42,7 @@ import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasKey; +import static org.junit.Assert.assertTrue; import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX; import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; @@ -71,6 +72,10 @@ public class OnBehalfOfJwtAuthenticationTest { public static final String OBO_TOKEN_REASON = "{\"reason\":\"Test generation\"}"; public static final String OBO_ENDPOINT_PREFIX = "_plugins/_security/api/generateonbehalfoftoken"; public static final String OBO_DESCRIPTION = "{\"description\":\"Testing\", \"service\":\"self-issued\"}"; + + public static final String INVALID_OBO_DESCRIPTION = + "{\"description\":\"Testing\", \"service\":\"self-issued\", \"durationSeconds\":\"invalid-seconds\"}"; + public static final String HOST_MAPPING_IP = "127.0.0.1"; public static final String OBO_USER_NAME_WITH_HOST_MAPPING = "obo_user_with_ip_role_mapping"; public static final String CURRENT_AND_NEW_PASSWORDS = "{ \"current_password\": \"" @@ -178,11 +183,23 @@ public void shouldNotIncludeRolesFromHostMappingInOBOToken() { Assert.assertFalse(roles.contains("host_mapping_role")); } + @Test + public void shouldNotAuthenticateWithInvalidDurationSeconds() { + try (TestRestClient client = cluster.getRestClient(ADMIN_USER_NAME, DEFAULT_PASSWORD)) { + client.assertCorrectCredentials(ADMIN_USER_NAME); + TestRestClient.HttpResponse response = client.postJson(OBO_ENDPOINT_PREFIX, INVALID_OBO_DESCRIPTION); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + Map oboEndPointResponse = (Map) response.getBodyAs(Map.class); + ; + assertTrue(oboEndPointResponse.containsValue("durationSeconds must be an integer.")); + } + } + private String generateOboToken(String username, String password) { try (TestRestClient client = cluster.getRestClient(username, password)) { client.assertCorrectCredentials(username); TestRestClient.HttpResponse response = client.postJson(OBO_ENDPOINT_PREFIX, OBO_TOKEN_REASON); - response.assertStatusCode(200); + response.assertStatusCode(HttpStatus.SC_OK); Map oboEndPointResponse = (Map) response.getBodyAs(Map.class); assertThat( oboEndPointResponse, diff --git a/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java b/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java index 2459d469df..86508be2c3 100644 --- a/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java +++ b/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java @@ -19,6 +19,8 @@ import java.util.stream.Collectors; import com.google.common.collect.ImmutableList; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.greenrobot.eventbus.Subscribe; import org.opensearch.client.node.NodeClient; @@ -61,6 +63,8 @@ public class CreateOnBehalfOfTokenAction extends BaseRestHandler { public static final String DEFAULT_SERVICE = "self-issued"; + protected final Logger log = LogManager.getLogger(this.getClass()); + @Subscribe public void onConfigModelChanged(ConfigModel configModel) { this.configModel = configModel; @@ -128,13 +132,11 @@ public void accept(RestChannel channel) throws Exception { final String clusterIdentifier = clusterService.getClusterName().value(); final Map requestBody = request.contentOrSourceParamParser().map(); - final String description = (String) requestBody.getOrDefault("description", null); - final Integer tokenDuration = Optional.ofNullable(requestBody.get("durationSeconds")) - .map(value -> (String) value) - .map(Integer::parseInt) - .map(value -> Math.min(value, OBO_MAX_EXPIRY_SECONDS)) // Max duration seconds are 600 - .orElse(OBO_DEFAULT_EXPIRY_SECONDS); // Fallback to default + Integer tokenDuration = parseAndValidateDurationSeconds(requestBody.get("durationSeconds")); + tokenDuration = Math.min(tokenDuration, OBO_MAX_EXPIRY_SECONDS); + + final String description = (String) requestBody.getOrDefault("description", null); final Boolean roleSecurityMode = Optional.ofNullable(requestBody.get("roleSecurityMode")) .map(value -> (Boolean) value) @@ -161,8 +163,13 @@ public void accept(RestChannel channel) throws Exception { builder.endObject(); response = new BytesRestResponse(RestStatus.OK, builder); + } catch (IllegalArgumentException iae) { + builder.startObject().field("error", iae.getMessage()).endObject(); + response = new BytesRestResponse(RestStatus.BAD_REQUEST, builder); } catch (final Exception exception) { - builder.startObject().field("error", exception.toString()).endObject(); + log.error("Unexpected error occurred: ", exception); + + builder.startObject().field("error", "An unexpected error occurred. Please check the input and try again.").endObject(); response = new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, builder); } @@ -176,4 +183,21 @@ private Set mapRoles(final User user) { return this.configModel.mapSecurityRoles(user, null); } + private Integer parseAndValidateDurationSeconds(Object durationObj) throws IllegalArgumentException { + if (durationObj == null) { + return OBO_DEFAULT_EXPIRY_SECONDS; + } + + if (durationObj instanceof Integer) { + return (Integer) durationObj; + } else if (durationObj instanceof String) { + try { + return Integer.parseInt((String) durationObj); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("durationSeconds must be an integer."); + } + } else { + throw new IllegalArgumentException("durationSeconds must be an integer."); + } + } } From 5628dd386e5b42654020bd9e48ef7adc141b2bda Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Mon, 16 Oct 2023 13:23:14 -0700 Subject: [PATCH 2/3] Add check for api parameters Signed-off-by: Ryan Liang --- .../http/OnBehalfOfJwtAuthenticationTest.java | 21 +++++++++++++++---- .../onbehalf/CreateOnBehalfOfTokenAction.java | 16 ++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java index 90e89e5986..90f8ef3407 100644 --- a/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java @@ -69,13 +69,16 @@ public class OnBehalfOfJwtAuthenticationTest { public static final String OBO_USER_NAME_NO_PERM = "obo_user_no_perm"; public static final String DEFAULT_PASSWORD = "secret"; public static final String NEW_PASSWORD = "testPassword123!!"; - public static final String OBO_TOKEN_REASON = "{\"reason\":\"Test generation\"}"; + public static final String OBO_TOKEN_REASON = "{\"description\":\"Test generation\"}"; public static final String OBO_ENDPOINT_PREFIX = "_plugins/_security/api/generateonbehalfoftoken"; public static final String OBO_DESCRIPTION = "{\"description\":\"Testing\", \"service\":\"self-issued\"}"; - public static final String INVALID_OBO_DESCRIPTION = + public static final String OBO_DESCRIPTION_WITH_INVALID_DURIATIONSECONDS = "{\"description\":\"Testing\", \"service\":\"self-issued\", \"durationSeconds\":\"invalid-seconds\"}"; + public static final String OBO_DESCRIPTION_WITH_INVALID_PARAMETERS = + "{\"description\":\"Testing\", \"service\":\"self-issued\", \"invalidParameter\":\"invalid-parameter\"}"; + public static final String HOST_MAPPING_IP = "127.0.0.1"; public static final String OBO_USER_NAME_WITH_HOST_MAPPING = "obo_user_with_ip_role_mapping"; public static final String CURRENT_AND_NEW_PASSWORDS = "{ \"current_password\": \"" @@ -187,14 +190,24 @@ public void shouldNotIncludeRolesFromHostMappingInOBOToken() { public void shouldNotAuthenticateWithInvalidDurationSeconds() { try (TestRestClient client = cluster.getRestClient(ADMIN_USER_NAME, DEFAULT_PASSWORD)) { client.assertCorrectCredentials(ADMIN_USER_NAME); - TestRestClient.HttpResponse response = client.postJson(OBO_ENDPOINT_PREFIX, INVALID_OBO_DESCRIPTION); + TestRestClient.HttpResponse response = client.postJson(OBO_ENDPOINT_PREFIX, OBO_DESCRIPTION_WITH_INVALID_DURIATIONSECONDS); response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); Map oboEndPointResponse = (Map) response.getBodyAs(Map.class); - ; assertTrue(oboEndPointResponse.containsValue("durationSeconds must be an integer.")); } } + @Test + public void shouldNotAuthenticateWithInvalidAPIParameter() { + try (TestRestClient client = cluster.getRestClient(ADMIN_USER_NAME, DEFAULT_PASSWORD)) { + client.assertCorrectCredentials(ADMIN_USER_NAME); + TestRestClient.HttpResponse response = client.postJson(OBO_ENDPOINT_PREFIX, OBO_DESCRIPTION_WITH_INVALID_PARAMETERS); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + Map oboEndPointResponse = (Map) response.getBodyAs(Map.class); + assertTrue(oboEndPointResponse.containsValue("Unrecognized parameter: invalidParameter")); + } + } + private String generateOboToken(String username, String password) { try (TestRestClient client = cluster.getRestClient(username, password)) { client.assertCorrectCredentials(username); diff --git a/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java b/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java index 86508be2c3..880791a21a 100644 --- a/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java +++ b/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java @@ -12,6 +12,8 @@ package org.opensearch.security.action.onbehalf; import java.io.IOException; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -65,6 +67,10 @@ public class CreateOnBehalfOfTokenAction extends BaseRestHandler { protected final Logger log = LogManager.getLogger(this.getClass()); + private static final Set RECOGNIZED_PARAMS = new HashSet<>( + Arrays.asList("durationSeconds", "description", "roleSecurityMode", "service") + ); + @Subscribe public void onConfigModelChanged(ConfigModel configModel) { this.configModel = configModel; @@ -133,6 +139,8 @@ public void accept(RestChannel channel) throws Exception { final Map requestBody = request.contentOrSourceParamParser().map(); + validateRequestParameters(requestBody); + Integer tokenDuration = parseAndValidateDurationSeconds(requestBody.get("durationSeconds")); tokenDuration = Math.min(tokenDuration, OBO_MAX_EXPIRY_SECONDS); @@ -183,6 +191,14 @@ private Set mapRoles(final User user) { return this.configModel.mapSecurityRoles(user, null); } + private void validateRequestParameters(Map requestBody) throws IllegalArgumentException { + for (String key : requestBody.keySet()) { + if (!RECOGNIZED_PARAMS.contains(key)) { + throw new IllegalArgumentException("Unrecognized parameter: " + key); + } + } + } + private Integer parseAndValidateDurationSeconds(Object durationObj) throws IllegalArgumentException { if (durationObj == null) { return OBO_DEFAULT_EXPIRY_SECONDS; From a302f66aba2f257f11250fa04eb1c05244511a4d Mon Sep 17 00:00:00 2001 From: Ryan Liang Date: Tue, 17 Oct 2023 10:54:42 -0700 Subject: [PATCH 3/3] Fix some comments Signed-off-by: Ryan Liang --- .../security/http/OnBehalfOfJwtAuthenticationTest.java | 4 ++-- .../action/onbehalf/CreateOnBehalfOfTokenAction.java | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java index 90f8ef3407..123fb5c770 100644 --- a/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java @@ -73,7 +73,7 @@ public class OnBehalfOfJwtAuthenticationTest { public static final String OBO_ENDPOINT_PREFIX = "_plugins/_security/api/generateonbehalfoftoken"; public static final String OBO_DESCRIPTION = "{\"description\":\"Testing\", \"service\":\"self-issued\"}"; - public static final String OBO_DESCRIPTION_WITH_INVALID_DURIATIONSECONDS = + public static final String OBO_DESCRIPTION_WITH_INVALID_DURATIONSECONDS = "{\"description\":\"Testing\", \"service\":\"self-issued\", \"durationSeconds\":\"invalid-seconds\"}"; public static final String OBO_DESCRIPTION_WITH_INVALID_PARAMETERS = @@ -190,7 +190,7 @@ public void shouldNotIncludeRolesFromHostMappingInOBOToken() { public void shouldNotAuthenticateWithInvalidDurationSeconds() { try (TestRestClient client = cluster.getRestClient(ADMIN_USER_NAME, DEFAULT_PASSWORD)) { client.assertCorrectCredentials(ADMIN_USER_NAME); - TestRestClient.HttpResponse response = client.postJson(OBO_ENDPOINT_PREFIX, OBO_DESCRIPTION_WITH_INVALID_DURIATIONSECONDS); + TestRestClient.HttpResponse response = client.postJson(OBO_ENDPOINT_PREFIX, OBO_DESCRIPTION_WITH_INVALID_DURATIONSECONDS); response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); Map oboEndPointResponse = (Map) response.getBodyAs(Map.class); assertTrue(oboEndPointResponse.containsValue("durationSeconds must be an integer.")); diff --git a/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java b/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java index 880791a21a..fe58e2adb1 100644 --- a/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java +++ b/src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java @@ -209,11 +209,8 @@ private Integer parseAndValidateDurationSeconds(Object durationObj) throws Illeg } else if (durationObj instanceof String) { try { return Integer.parseInt((String) durationObj); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("durationSeconds must be an integer."); - } - } else { - throw new IllegalArgumentException("durationSeconds must be an integer."); + } catch (NumberFormatException ignored) {} } + throw new IllegalArgumentException("durationSeconds must be an integer."); } }