From 0a8c4988bd2c019af0a734bb17b74b3db9d6ae21 Mon Sep 17 00:00:00 2001 From: Ryan Liang <109499885+RyanL1997@users.noreply.github.com> Date: Wed, 18 Oct 2023 10:31:53 -0700 Subject: [PATCH] [Enhancement] Input Validation for generate obo token endpoint (#3553) Add Input Validation for generate obo token endpoint Signed-off-by: Ryan Liang --- .../http/OnBehalfOfJwtAuthenticationTest.java | 34 ++++++++++++- .../onbehalf/CreateOnBehalfOfTokenAction.java | 51 ++++++++++++++++--- 2 files changed, 76 insertions(+), 9 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..123fb5c770 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; @@ -68,9 +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 OBO_DESCRIPTION_WITH_INVALID_DURATIONSECONDS = + "{\"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\": \"" @@ -178,11 +186,33 @@ 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, 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.")); + } + } + + @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); 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..fe58e2adb1 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; @@ -19,6 +21,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 +65,12 @@ public class CreateOnBehalfOfTokenAction extends BaseRestHandler { public static final String DEFAULT_SERVICE = "self-issued"; + 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; @@ -128,13 +138,13 @@ 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 + validateRequestParameters(requestBody); + + 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 +171,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 +191,26 @@ 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; + } + + if (durationObj instanceof Integer) { + return (Integer) durationObj; + } else if (durationObj instanceof String) { + try { + return Integer.parseInt((String) durationObj); + } catch (NumberFormatException ignored) {} + } + throw new IllegalArgumentException("durationSeconds must be an integer."); + } }