Skip to content

Commit

Permalink
[Enhancement] Input Validation for generate obo token endpoint (#3553)
Browse files Browse the repository at this point in the history 
Add Input Validation for generate obo token endpoint

Signed-off-by: Ryan Liang <jiallian@amazon.com>
  • Loading branch information
@RyanL1997
RyanL1997 authored Oct 18, 2023
1 parent f2124cf commit 0a8c498
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 9 deletions.
34 changes: 32 additions & 2 deletions src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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\": \""
Expand Down Expand Up @@ -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<String, Object> oboEndPointResponse = (Map<String, Object>) 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<String, Object> oboEndPointResponse = (Map<String, Object>) 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<String, Object> oboEndPointResponse = (Map<String, Object>) response.getBodyAs(Map.class);
assertThat(
oboEndPointResponse,
Expand Down
51 changes: 44 additions & 7 deletions src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@
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;
import java.util.Set;
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;
Expand Down Expand Up @@ -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<String> RECOGNIZED_PARAMS = new HashSet<>(
Arrays.asList("durationSeconds", "description", "roleSecurityMode", "service")
);

@Subscribe
public void onConfigModelChanged(ConfigModel configModel) {
this.configModel = configModel;
Expand Down Expand Up @@ -128,13 +138,13 @@ public void accept(RestChannel channel) throws Exception {
final String clusterIdentifier = clusterService.getClusterName().value();

final Map<String, Object> 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)
Expand All @@ -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);
}
Expand All @@ -176,4 +191,26 @@ private Set<String> mapRoles(final User user) {
return this.configModel.mapSecurityRoles(user, null);
}

private void validateRequestParameters(Map<String, Object> 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.");
}
}

0 comments on commit 0a8c498

Please sign in to comment.