diff --git a/CHANGELOG.md b/CHANGELOG.md index f9e9f243d5..ab4c1135a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add support for X509 v3 extensions (SAN) for authentication ([#5701](https://github.com/opensearch-project/security/pull/5701)) - [Resource Sharing] Requires default_owner for resource/migrate API ([#5789](https://github.com/opensearch-project/security/pull/5789)) - Add --timeout (-to) as an option to securityadmin.sh ([#5787](https://github.com/opensearch-project/security/pull/5787)) +- Hardens input validation for resource sharing APIs ([#5831](https://github.com/opensearch-project/security/pull/5831) ### Bug Fixes - Create a WildcardMatcher.NONE when creating a WildcardMatcher with an empty string ([#5694](https://github.com/opensearch-project/security/pull/5694)) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/AccessibleResourcesApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/AccessibleResourcesApiTests.java index fe4d2fd402..fcb4868248 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/AccessibleResourcesApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/AccessibleResourcesApiTests.java @@ -74,13 +74,14 @@ public void clearIndices() { } @Test - @SuppressWarnings("unchecked") public void testListAccessibleResources_gibberishParams() { try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { TestRestClient.HttpResponse response = client.get(SECURITY_LIST_ENDPOINT + "?resource_type=" + "some-type"); - response.assertStatusCode(HttpStatus.SC_OK); - List types = (List) response.bodyAsMap().get("resources"); - assertThat(types.size(), equalTo(0)); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat( + response.getBody(), + containsString("Invalid resource type: some-type. Must be one of: [sample-resource, sample-resource-group]") + ); } } diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java index 58ac9e2b2c..7bf0c0011a 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java @@ -401,7 +401,185 @@ public void testMigrateAPIWithSuperAdmin_invalidDefaultAccessLevel() { migrateResponse, RestMatchers.isBadRequest( "/message", - "Invalid access level blah for resource sharing for resource type [" + RESOURCE_TYPE + "]" + "Invalid access level blah for resource type [" + + RESOURCE_TYPE + + "]. Allowed: sample_read_write, sample_read_only, sample_full_access" + ) + ); + } + } + + @Test + public void testMigrateAPI_inputValidation_invalidValues() { + // Ensure there is at least one resource so migration can proceed to validation + createSampleResource(); + + try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { + + // ------------------------------ + // 1) Invalid username_path (whitespace) + // ------------------------------ + String invalidUserPathPayload = """ + { + "source_index": "%s", + "username_path": " /user", + "backend_roles_path": "/user/backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "sample_read_only" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + TestRestClient.HttpResponse response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidUserPathPayload); + + assertThat(response, RestMatchers.isBadRequest("/message", "username_path must not contain whitespace")); + + // ------------------------------ + // 2) Invalid backend_roles_path (whitespace) + // ------------------------------ + String invalidBackendPathPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by. backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "sample_read_only" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidBackendPathPayload); + + assertThat(response, RestMatchers.isBadRequest("/message", "backend_roles_path must not contain whitespace")); + + // ------------------------------ + // 3) Invalid default_owner (bad characters) + // ------------------------------ + String invalidDefaultOwnerPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "owner name", + "default_access_level": { + "%s": "sample_read_only" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidDefaultOwnerPayload); + + assertThat( + response, + RestMatchers.isBadRequest("/message", "default_owner contains invalid characters; allowed: A-Z a-z 0-9 _ - :") + ); + + // ------------------------------ + // 4) default_access_level is NOT an object + // ------------------------------ + String defaultAccessNotObjectPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": "sample_read_only" + } + """.formatted(RESOURCE_INDEX_NAME); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, defaultAccessNotObjectPayload); + + assertThat(response, RestMatchers.isBadRequest("/reason", "Wrong datatype")); + + // ------------------------------ + // 5) default_access_level is an empty object {} + // ------------------------------ + String defaultAccessEmptyObjectPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": { } + } + """.formatted(RESOURCE_INDEX_NAME); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, defaultAccessEmptyObjectPayload); + + assertThat(response, RestMatchers.isBadRequest("/message", "default_access_level cannot be empty")); + + // ------------------------------ + // 6) default_access_level has empty value for a type + // ------------------------------ + String defaultAccessEmptyValuePayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, defaultAccessEmptyValuePayload); + + assertThat( + response, + RestMatchers.isBadRequest("/message", "default_access_level for type [" + RESOURCE_TYPE + "] must be a non-empty string") + ); + + // ------------------------------ + // 7) Invalid source_index (not in protected types) + // ------------------------------ + String invalidSourceIndexPayload = """ + { + "source_index": "some-other-index", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "sample_read_only" + } + } + """.formatted(RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidSourceIndexPayload); + + assertThat( + response, + RestMatchers.isBadRequest( + "/message", + "Invalid resource index [some-other-index]. Allowed indices: [" + RESOURCE_INDEX_NAME + "]" + ) + ); + + // ------------------------------ + // 8) Invalid access level value for valid type + // (this exercises validateAccessLevel + last try/catch) + // ------------------------------ + String invalidAccessLevelPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "blah" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidAccessLevelPayload); + + assertThat( + response, + RestMatchers.isBadRequest( + "/message", + "Invalid access level blah for resource type [sample-resource]. Allowed: sample_read_write, sample_read_only, sample_full_access" ) ); } diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java index f4260ffade..96a9002173 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java @@ -95,7 +95,7 @@ public void testGibberishPayload() { SECURITY_SHARE_ENDPOINT + "?resource_id=" + "some-id" + "&resource_type=" + RESOURCE_TYPE ); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); // since resource-index exists but resource-id doesn't, but user - // shouldn't know that + // shouldn't know that response = client.get(SECURITY_SHARE_ENDPOINT + "?resource_id=" + adminResId + "&resource_type=" + "some-type"); response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); // since type doesn't exist, so does the corresponding index @@ -108,7 +108,7 @@ public void testGibberishPayload() { putSharingInfoPayload("some-id", RESOURCE_TYPE, SAMPLE_READ_ONLY, Recipient.USERS, NO_ACCESS_USER.getName()) ); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); // since resource-index exists but resource-id doesn't, but user - // shouldn't know that + // shouldn't know that response = client.putJson( SECURITY_SHARE_ENDPOINT, @@ -342,6 +342,102 @@ public void testPostSharingInfo() { response.assertStatusCode(HttpStatus.SC_FORBIDDEN); } } - } + @Test + public void testShareApi_putAndPatch_invalidPayloadValidation() { + // Use admin so we actually hit payload parsing & validation, not auth failures + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + + // 1) PUT with invalid resource_id (invalid chars) + String putInvalidResourceIdPayload = """ + { + "resource_id": "invalid id", // space not allowed + "resource_type": "%s", + "share_with": { + "%s": { + "users": ["%s"] + } + } + } + """.formatted(RESOURCE_TYPE, SAMPLE_READ_ONLY, NO_ACCESS_USER.getName()); + + TestRestClient.HttpResponse response = client.putJson(SECURITY_SHARE_ENDPOINT, putInvalidResourceIdPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("resource_id")); + assertThat(response.getBody(), containsString("contains invalid characters")); + + // 2) PUT with invalid principal value (users entry contains space) + String putInvalidUserPayload = """ + { + "resource_id": "%s", + "resource_type": "%s", + "share_with": { + "%s": { + "users": ["invalid user"] + } + } + } + """.formatted(adminResId, RESOURCE_TYPE, SAMPLE_READ_ONLY); + + response = client.putJson(SECURITY_SHARE_ENDPOINT, putInvalidUserPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("users")); + assertThat(response.getBody(), containsString("contains invalid characters")); + + // 3) PUT with invalid access level key + String putInvalidAccessLevelPayload = """ + { + "resource_id": "%s", + "resource_type": "%s", + "share_with": { + "blah": { + "users": ["%s"] + } + } + } + """.formatted(adminResId, RESOURCE_TYPE, NO_ACCESS_USER.getName()); + + response = client.putJson(SECURITY_SHARE_ENDPOINT, putInvalidAccessLevelPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("access_level must be one of:")); + assertThat(response.getBody(), not(containsString("blah"))); + + // 4) PATCH with invalid principal value (same validation path as PUT, via Recipients) + String patchInvalidUserPayload = """ + { + "resource_id": "%s", + "resource_type": "%s", + "add": { + "%s": { + "users": ["invalid user"] + } + } + } + """.formatted(adminResId, RESOURCE_TYPE, SAMPLE_READ_ONLY); + + response = client.patch(SECURITY_SHARE_ENDPOINT, patchInvalidUserPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("users")); + assertThat(response.getBody(), containsString("contains invalid characters")); + + // 5) PATCH with invalid access level key + String patchInvalidAccessLevelPayload = """ + { + "resource_id": "%s", + "resource_type": "%s", + "add": { + "blah": { + "users": ["%s"] + } + } + } + """.formatted(adminResId, RESOURCE_TYPE, NO_ACCESS_USER.getName()); + + response = client.patch(SECURITY_SHARE_ENDPOINT, patchInvalidAccessLevelPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("access_level must be one of:")); + assertThat(response.getBody(), not(containsString("blah"))); + } + } + } } diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 57af519311..9a492342dd 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -708,7 +708,14 @@ public List getRestHandlers( new ShareRestAction(resourcePluginInfo, resourceSharingEnabledSetting, resourceSharingProtectedResourceTypesSetting) ); handlers.add(new ResourceTypesRestAction(resourcePluginInfo, resourceSharingEnabledSetting)); - handlers.add(new AccessibleResourcesRestAction(resourceAccessHandler, resourcePluginInfo, resourceSharingEnabledSetting)); + handlers.add( + new AccessibleResourcesRestAction( + resourceAccessHandler, + resourcePluginInfo, + resourceSharingEnabledSetting, + resourceSharingProtectedResourceTypesSetting + ) + ); } log.debug("Added {} rest handler(s)", handlers.size()); diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java index f3056250df..8e1120334e 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java @@ -32,6 +32,7 @@ import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.rest.RestRequest; import org.opensearch.security.DefaultObjectMapper; +import org.opensearch.security.resources.sharing.Recipient; import com.flipkart.zjsonpatch.JsonDiff; @@ -85,6 +86,77 @@ public static enum DataType { BOOLEAN; } + /** + * Validator interface for field-level validation + */ + @FunctionalInterface + public interface FieldValidator { + /** + * Validate a field value + * @param fieldName the name of the field being validated + * @param value the value to validate (can be String, JsonNode, etc.) + * @throws IllegalArgumentException if validation fails + */ + void validate(String fieldName, Object value); + } + + /** + * Configuration for a field including its type and validation rules + */ + public static class FieldConfiguration { + private final DataType dataType; + private final Integer maxLength; + private final FieldValidator validator; + + private FieldConfiguration(DataType dataType, Integer maxLength, FieldValidator validator) { + this.dataType = dataType; + this.maxLength = maxLength; + this.validator = validator; + } + + public static FieldConfiguration of(DataType dataType) { + return new FieldConfiguration(dataType, null, null); + } + + public static FieldConfiguration of(DataType dataType, int maxLength) { + return new FieldConfiguration(dataType, maxLength, null); + } + + public static FieldConfiguration of(DataType dataType, FieldValidator validator) { + return new FieldConfiguration(dataType, null, validator); + } + + public static FieldConfiguration of(DataType dataType, int maxLength, FieldValidator validator) { + return new FieldConfiguration(dataType, maxLength, validator); + } + + public DataType getDataType() { + return dataType; + } + + public Integer getMaxLength() { + return maxLength; + } + + public FieldValidator getValidator() { + return validator; + } + + public void validate(String fieldName, Object value) { + // Validate max length for strings + if (maxLength != null && value instanceof String strValue) { + if (strValue.length() > maxLength) { + throw new IllegalArgumentException(fieldName + " length [" + strValue.length() + "] exceeds max [" + maxLength + "]"); + } + } + + // Run custom validator if present + if (validator != null) { + validator.validate(fieldName, value); + } + } + } + public interface ValidationContext { default boolean hasParams() { @@ -105,6 +177,15 @@ default Set mandatoryOrKeys() { Map allowedKeys(); + /** + * Optional: Returns field configurations with validation rules. + * If not overridden, returns null and the validator will use allowedKeys() instead. + * This is an opt-in enhancement for more flexible validation. + */ + default Map allowedKeysWithConfig() { + return null; + } + } protected ValidationError validationError; @@ -177,7 +258,14 @@ protected ValidationResult validateJsonKeys(final JsonNode jsonContent mandatory.removeAll(requestedKeys); missingMandatoryKeys.addAll(mandatory); - final Set allowed = new HashSet<>(validationContext.allowedKeys().keySet()); + // Use allowedKeysWithConfig if provided, otherwise fall back to allowedKeys + final Map fieldConfigs = validationContext.allowedKeysWithConfig(); + final Set allowed; + if (fieldConfigs != null) { + allowed = new HashSet<>(fieldConfigs.keySet()); + } else { + allowed = new HashSet<>(validationContext.allowedKeys().keySet()); + } requestedKeys.removeAll(allowed); invalidKeys.addAll(requestedKeys); @@ -189,14 +277,32 @@ protected ValidationResult validateJsonKeys(final JsonNode jsonContent } private ValidationResult validateDataType(final JsonNode jsonContent) { + // Check if enhanced validation is available + final Map fieldConfigs = validationContext.allowedKeysWithConfig(); + final boolean useEnhancedValidation = (fieldConfigs != null); + try (final JsonParser parser = DefaultObjectMapper.objectMapper.treeAsTokens(jsonContent)) { JsonToken token; while ((token = parser.nextToken()) != null) { if (token.equals(JsonToken.FIELD_NAME)) { - String currentName = parser.getCurrentName(); - final DataType dataType = validationContext.allowedKeys().get(currentName); + String currentName = parser.currentName(); + + // Get data type from either FieldConfiguration or simple DataType map + final DataType dataType; + final FieldConfiguration fieldConfig; + + if (useEnhancedValidation) { + fieldConfig = fieldConfigs.get(currentName); + dataType = (fieldConfig != null) ? fieldConfig.getDataType() : null; + } else { + fieldConfig = null; + dataType = validationContext.allowedKeys().get(currentName); + } + if (dataType != null) { JsonToken valueToken = parser.nextToken(); + + // Validate data type switch (dataType) { case INTEGER: if (valueToken != JsonToken.VALUE_NUMBER_INT) { @@ -206,16 +312,45 @@ private ValidationResult validateDataType(final JsonNode jsonContent) case STRING: if (valueToken != JsonToken.VALUE_STRING) { wrongDataTypes.put(currentName, "String expected"); + } else if (fieldConfig != null) { + // Enhanced validation: validate string-specific constraints + String stringValue = parser.getText(); + try { + fieldConfig.validate(currentName, stringValue); + } catch (IllegalArgumentException e) { + wrongDataTypes.put(currentName, e.getMessage()); + } } break; case ARRAY: if (valueToken != JsonToken.START_ARRAY && valueToken != JsonToken.END_ARRAY) { wrongDataTypes.put(currentName, "Array expected"); + } else if (fieldConfig != null && fieldConfig.getValidator() != null) { + // Enhanced validation: validate array content + JsonNode arrayNode = jsonContent.get(currentName); + try { + fieldConfig.validate(currentName, arrayNode); + } catch (IllegalArgumentException e) { + wrongDataTypes.put(currentName, e.getMessage()); + } } break; case OBJECT: if (!valueToken.equals(JsonToken.START_OBJECT) && !valueToken.equals(JsonToken.END_OBJECT)) { wrongDataTypes.put(currentName, "Object expected"); + } else if (fieldConfig != null && fieldConfig.getValidator() != null) { + // Enhanced validation: validate object content + JsonNode objectNode = jsonContent.get(currentName); + try { + fieldConfig.validate(currentName, objectNode); + } catch (IllegalArgumentException e) { + wrongDataTypes.put(currentName, e.getMessage()); + } + } + break; + case BOOLEAN: + if (valueToken != JsonToken.VALUE_TRUE && valueToken != JsonToken.VALUE_FALSE) { + wrongDataTypes.put(currentName, "Boolean expected"); } break; } @@ -393,4 +528,240 @@ public String message() { } } + + /* ======================================================================== + * Input Validation Utilities (for resource-sharing REST APIs) + * ======================================================================== */ + + public static final int MAX_RESOURCE_ID_LENGTH = 256; + public static final int MAX_RESOURCE_TYPE_LENGTH = 256; + public static final int MAX_ACCESS_LEVEL_LENGTH = 256; + public static final int MAX_PRINCIPAL_LENGTH = 256; + public static final int MAX_PATH_LENGTH = 256; + public static final int MAX_INDEX_NAME_LENGTH = 256; + public static final int MAX_ARRAY_SIZE = 100_000; + + // Alphanumeric + _ - : OR : * - "*" is only allowed as standalone + private static final java.util.regex.Pattern SAFE_VALUE = java.util.regex.Pattern.compile("^(\\*|[A-Za-z0-9_:-]+)$"); + + /* ---------------------- generic helpers ---------------------- */ + + public static void requireNonEmpty(String fieldName, String value) { + if (Strings.isNullOrEmpty(value)) { + throw new IllegalArgumentException(fieldName + " must not be null or empty"); + } + } + + public static void validateMaxLength(String fieldName, String value, int maxLength) { + if (value.length() > maxLength) { + throw new IllegalArgumentException(fieldName + " length [" + value.length() + "] exceeds max [" + maxLength + "]"); + } + } + + public static void validateSafeValue(String fieldName, String value, int maxLength) { + requireNonEmpty(fieldName, value); + validateMaxLength(fieldName, value, maxLength); + if (!SAFE_VALUE.matcher(value).matches()) { + throw new IllegalArgumentException( + fieldName + + " contains invalid characters; allowed: " + + (fieldName.equals(Recipient.USERS.getName()) ? "* OR " : "") + + "A-Z a-z 0-9 _ - :" + ); + } + } + + public static void validateArrayEntryCount(String fieldName, int count) { + if (count > MAX_ARRAY_SIZE) { + throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); + } + } + + public static void validateResourceId(String resourceId) { + validateSafeValue("resource_id", resourceId, MAX_RESOURCE_ID_LENGTH); + } + + public static void validateResourceType(String resourceType, java.util.List allowedTypes) { + validateSafeValue("resource_type", resourceType, MAX_RESOURCE_TYPE_LENGTH); + + if (allowedTypes == null || allowedTypes.isEmpty()) { + throw new IllegalStateException("No protected resource types configured"); + } + + if (!allowedTypes.contains(resourceType)) { + throw new IllegalArgumentException("Unsupported resource_type [" + resourceType + "], allowed types: " + allowedTypes); + } + } + + public static void validatePrincipalValue(String fieldName, String value) { + // users / roles / backend_roles entries + validateSafeValue(fieldName, value, MAX_PRINCIPAL_LENGTH); + } + + public static void validateAccessLevel(String accessLevel, Set validAccessLevels) { + requireNonEmpty("access_level", accessLevel); + + validateMaxLength("access_level", accessLevel, MAX_ACCESS_LEVEL_LENGTH); + + if (!SAFE_VALUE.matcher(accessLevel).matches()) { + throw new IllegalArgumentException("Invalid access_level [" + accessLevel + "]. Allowed characters: A-Z a-z 0-9 _ - :"); + } + + // Check against configured access-level set + if (validAccessLevels == null || validAccessLevels.isEmpty()) { + throw new IllegalStateException("No access levels configured."); + } + + if (!validAccessLevels.contains(accessLevel)) { + throw new IllegalArgumentException( + "Invalid access_level [" + accessLevel + "]. Allowed values: " + String.join(", ", validAccessLevels) + ); + } + } + + /* -------- JSON helpers for migrate API -------- */ + + public static String getRequiredText(JsonNode body, String fieldName, int maxLength) { + JsonNode node = body.get(fieldName); + if (node == null || node.isNull() || !node.isTextual()) { + throw new IllegalArgumentException("Field [" + fieldName + "] is required and must be a non-empty string"); + } + String value = node.asText(); + requireNonEmpty(fieldName, value); + validateMaxLength(fieldName, value, maxLength); + return value; + } + + public static String getOptionalText(JsonNode body, String fieldName, int maxLength) { + JsonNode node = body.get(fieldName); + if (node == null || node.isNull()) { + return null; + } + if (!node.isTextual()) { + throw new IllegalArgumentException("Field [" + fieldName + "] must be a string when provided"); + } + String value = node.asText(); + if (value.isEmpty()) { + return null; + } + validateMaxLength(fieldName, value, maxLength); + return value; + } + + /* --------- migrate-specific primitives --------- */ + + public static void validateJsonPath(String fieldName, String path) { + requireNonEmpty(fieldName, path); + validateMaxLength(fieldName, path, MAX_PATH_LENGTH); + // simple rule: no whitespace anywhere + if (!path.equals(path.trim()) || path.chars().anyMatch(Character::isWhitespace)) { + throw new IllegalArgumentException(fieldName + " must not contain whitespace"); + } + } + + public static void validateSourceIndex(String sourceIndex, Set allowedIndices) { + requireNonEmpty("source_index", sourceIndex); + validateMaxLength("source_index", sourceIndex, MAX_INDEX_NAME_LENGTH); + if (allowedIndices == null || allowedIndices.isEmpty()) { + throw new IllegalStateException("No protected resource indices configured"); + } + if (!allowedIndices.contains(sourceIndex)) { + throw new IllegalArgumentException("Invalid resource index [" + sourceIndex + "]. Allowed indices: " + allowedIndices); + } + } + + public static void validateDefaultOwner(String defaultOwner) { + if (defaultOwner == null) { + return; // optional + } + validatePrincipalValue("default_owner", defaultOwner); + } + + public static void validateDefaultAccessLevelNode(JsonNode node) { + if (node == null || node.isNull()) { + return; // field is optional + } + + if (!node.isObject()) { + throw new IllegalArgumentException("default_access_level must be an object"); + } + + if (!node.fieldNames().hasNext()) { + throw new IllegalArgumentException("default_access_level cannot be empty"); + } + + // Validate values are non-empty strings + node.fields().forEachRemaining(entry -> { + JsonNode val = entry.getValue(); + if (!val.isTextual() || val.asText().isEmpty()) { + throw new IllegalArgumentException("default_access_level for type [" + entry.getKey() + "] must be a non-empty string"); + } + }); + } + + /* ======================================================================== + * Pre-built Field Validators for Common Use Cases + * ======================================================================== */ + + /** + * Validator for principal values (users, roles, backend_roles) + */ + public static final FieldValidator PRINCIPAL_VALIDATOR = (fieldName, value) -> { + if (value instanceof String strValue) { + requireNonEmpty(fieldName, strValue); + validateMaxLength(fieldName, strValue, MAX_PRINCIPAL_LENGTH); + if (!SAFE_VALUE.matcher(strValue).matches()) { + throw new IllegalArgumentException( + fieldName + + " contains invalid characters; allowed: " + + (fieldName.equals(Recipient.USERS.getName()) ? "* OR " : "") + + "A-Z a-z 0-9 _ - :" + ); + } + } + }; + + /** + * Validator for JSON paths (no whitespace allowed) + */ + public static final FieldValidator JSON_PATH_VALIDATOR = (fieldName, value) -> { + if (value instanceof String strValue) { + requireNonEmpty(fieldName, strValue); + validateMaxLength(fieldName, strValue, MAX_PATH_LENGTH); + if (!strValue.equals(strValue.trim()) || strValue.chars().anyMatch(Character::isWhitespace)) { + throw new IllegalArgumentException(fieldName + " must not contain whitespace"); + } + } + }; + + /** + * Validator for array entry counts (works with JsonNode arrays) + */ + public static final FieldValidator ARRAY_SIZE_VALIDATOR = (fieldName, value) -> { + if (value instanceof JsonNode node) { + if (node.isArray() && node.size() > MAX_ARRAY_SIZE) { + throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); + } + } else if (value instanceof Integer) { + int count = (Integer) value; + if (count > MAX_ARRAY_SIZE) { + throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); + } + } + }; + + /** + * Creates a validator that checks if a string value is in an allowed set + */ + public static FieldValidator allowedValuesValidator(Set allowedValues, String errorMessage) { + return (fieldName, value) -> { + if (value instanceof String strValue) { + if (!allowedValues.contains(strValue)) { + throw new IllegalArgumentException( + errorMessage != null ? errorMessage : fieldName + " must be one of: " + String.join(", ", allowedValues) + ); + } + } + }; + } } diff --git a/src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java b/src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java index 4ace9be889..5c3deeae6a 100644 --- a/src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java +++ b/src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java @@ -295,4 +295,22 @@ public Set getResourceIndicesForProtectedTypes() { } } + public List currentProtectedTypes() { + lock.readLock().lock(); + try { + return protectedTypesSetting.getDynamicSettingValue(); + } finally { + lock.readLock().unlock(); + } + } + + public Set availableAccessLevels() { + lock.readLock().lock(); + try { + return typeToAccessLevels.values().stream().flatMap(Set::stream).collect(Collectors.toCollection(LinkedHashSet::new)); + } finally { + lock.readLock().unlock(); + } + } + } diff --git a/src/main/java/org/opensearch/security/resources/api/list/AccessibleResourcesRestAction.java b/src/main/java/org/opensearch/security/resources/api/list/AccessibleResourcesRestAction.java index 0337b095a5..d19619555c 100644 --- a/src/main/java/org/opensearch/security/resources/api/list/AccessibleResourcesRestAction.java +++ b/src/main/java/org/opensearch/security/resources/api/list/AccessibleResourcesRestAction.java @@ -10,7 +10,6 @@ import java.io.IOException; import java.util.List; -import java.util.Objects; import java.util.Set; import com.google.common.collect.ImmutableList; @@ -46,16 +45,19 @@ public class AccessibleResourcesRestAction extends BaseRestHandler { private final ResourceAccessHandler resourceAccessHandler; private final ResourcePluginInfo resourcePluginInfo; private final OpensearchDynamicSetting resourceSharingEnabledSetting; + private final OpensearchDynamicSetting> resourceSharingProtectedTypesSetting; public AccessibleResourcesRestAction( final ResourceAccessHandler resourceAccessHandler, ResourcePluginInfo resourcePluginInfo, - OpensearchDynamicSetting resourceSharingEnabledSetting + OpensearchDynamicSetting resourceSharingEnabledSetting, + OpensearchDynamicSetting> resourceSharingProtectedTypesSetting ) { super(); this.resourceAccessHandler = resourceAccessHandler; this.resourcePluginInfo = resourcePluginInfo; this.resourceSharingEnabledSetting = resourceSharingEnabledSetting; + this.resourceSharingProtectedTypesSetting = resourceSharingProtectedTypesSetting; } @Override @@ -73,19 +75,30 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli if (!resourceSharingEnabledSetting.getDynamicSettingValue()) { return channel -> { channel.sendResponse(new BytesRestResponse(RestStatus.NOT_IMPLEMENTED, "Feature disabled.")); }; } - final String resourceType = Objects.requireNonNull(request.param("resource_type"), "resource_type is required"); + final String resourceType = request.param("resource_type"); + + if (resourceType == null) { + return channel -> { channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, "resource_type is required")); }; + } final String resourceIndex = resourcePluginInfo.indexByType(resourceType); if (resourceIndex == null) { - return channel -> { handleResponse(channel, Set.of()); }; + return channel -> { + String message = String.format( + "Invalid resource type: %s. Must be one of: %s", + resourceType, + resourceSharingProtectedTypesSetting.getDynamicSettingValue() + ); + channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, message)); + }; } return channel -> resourceAccessHandler.getResourceSharingInfoForCurrentUser(resourceType, ActionListener.wrap(rows -> { handleResponse(channel, rows); }, e -> handleError(channel, e))); } - private void handleResponse(RestChannel channel, Set records) throws IOException { + private void handleResponse(RestChannel channel, Set records) { try (XContentBuilder b = channel.newBuilder()) { b.startObject(); b.startArray("resources"); diff --git a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java index 5884de5c8a..3386c06b9f 100644 --- a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java +++ b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java @@ -10,6 +10,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -153,44 +154,68 @@ private void handleMigrate(RestChannel channel, RestRequest request, Client clie private ValidationResult loadCurrentSharingInfo(RestRequest request, Client client) throws IOException { JsonNode body = Utils.toJsonNode(request.content().utf8ToString()); + // Extract fields - basic validation done by RequestContentValidator framework String sourceIndex = body.get("source_index").asText(); String userNamePath = body.get("username_path").asText(); String backendRolesPath = body.get("backend_roles_path").asText(); - String defaultOwner = body.get("default_owner").asText(); - JsonNode node = body.get("default_access_level"); - Map typeToDefaultAccessLevel = Utils.toMapOfStrings(node); - if (!resourcePluginInfo.getResourceIndicesForProtectedTypes().contains(sourceIndex)) { - String badRequestMessage = "Invalid resource index " + sourceIndex + "."; - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(badRequestMessage)); + + // Optional field + JsonNode defaultOwnerNode = body.get("default_owner"); + String defaultOwner = (defaultOwnerNode != null && !defaultOwnerNode.isNull()) ? defaultOwnerNode.asText() : null; + + // Raw JSON for default_access_level + JsonNode defaultAccessNode = body.get("default_access_level"); + + // Business logic validation (after framework validation) + try { + RequestContentValidator.validateJsonPath("username_path", userNamePath); + RequestContentValidator.validateJsonPath("backend_roles_path", backendRolesPath); + RequestContentValidator.validateDefaultOwner(defaultOwner); + RequestContentValidator.validateSourceIndex(sourceIndex, resourcePluginInfo.getResourceIndicesForProtectedTypes()); + RequestContentValidator.validateDefaultAccessLevelNode(defaultAccessNode); + } catch (IllegalArgumentException | IllegalStateException e) { + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(e.getMessage())); } + // Convert after structural validation + Map typeToDefaultAccessLevel = defaultAccessNode == null || defaultAccessNode.isNull() + ? Collections.emptyMap() + : Utils.toMapOfStrings(defaultAccessNode); + String typePath = null; - for (String type : typeToDefaultAccessLevel.keySet()) { + + // Validate each type + its accessLevel + for (Map.Entry entry : typeToDefaultAccessLevel.entrySet()) { + String type = entry.getKey(); + String defaultAccessLevelForType = entry.getValue(); + + // Validate resource type exists ResourceProvider provider = resourcePluginInfo.getResourceProvider(type); - String defaultAccessLevelForType = typeToDefaultAccessLevel.get(type); - LOGGER.info("Default access level for resource type [{}] is [{}]", type, typeToDefaultAccessLevel.get(type)); - // check that access level exists for given resource-index if (provider == null) { - String badRequestMessage = "Invalid resource type " + type + "."; - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(badRequestMessage)); + // We do not expect this to be null, as validation check will already have been performed in request content validator + String message = String.format("resource_type be one of: %s", resourcePluginInfo.currentProtectedTypes()); + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(message)); } - typePath = provider.typeField(); // All types in the same index must have same typeField - var accessLevels = resourcePluginInfo.flattenedForType(type).actionGroups(); - if (!accessLevels.contains(defaultAccessLevelForType)) { - LOGGER.error( - "Invalid access level {} for resource sharing for resource type [{}]. Available access-levels are [{}]", - defaultAccessLevelForType, - type, - accessLevels + + typePath = provider.typeField(); + + // Allowed access-levels for this type + Set accessLevels = resourcePluginInfo.flattenedForType(type).actionGroups(); + + try { + RequestContentValidator.validateAccessLevel(defaultAccessLevelForType, accessLevels); + } catch (Exception e) { + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage( + "Invalid access level " + + defaultAccessLevelForType + + " for resource type [" + + type + + "]. Allowed: " + + String.join(", ", accessLevels) + ) ); - String badRequestMessage = "Invalid access level " - + defaultAccessLevelForType - + " for resource sharing for resource type [" - + type - + "]. Available access-levels are [" - + accessLevels - + "]"; - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(badRequestMessage)); } } @@ -412,12 +437,76 @@ public Set mandatoryKeys() { @Override public Map allowedKeys() { + // Provide basic type information for backward compatibility return ImmutableMap.builder() - .put("source_index", RequestContentValidator.DataType.STRING) // name of the resource plugin index - .put("username_path", RequestContentValidator.DataType.STRING) // path to resource creator's name - .put("backend_roles_path", RequestContentValidator.DataType.STRING) // path to backend_roles - .put("default_owner", RequestContentValidator.DataType.STRING) // default owner name for resources without owner - .put("default_access_level", RequestContentValidator.DataType.OBJECT) // default access level by type + .put("source_index", RequestContentValidator.DataType.STRING) + .put("username_path", RequestContentValidator.DataType.STRING) + .put("backend_roles_path", RequestContentValidator.DataType.STRING) + .put("default_owner", RequestContentValidator.DataType.STRING) + .put("default_access_level", RequestContentValidator.DataType.OBJECT) + .build(); + } + + @Override + public Map allowedKeysWithConfig() { + // Validate source_index is in allowed set + RequestContentValidator.FieldValidator sourceIndexValidator = (fieldName, value) -> { + if (value instanceof String strValue) { + RequestContentValidator.requireNonEmpty(fieldName, strValue); + Set allowedIndices = resourcePluginInfo.getResourceIndicesForProtectedTypes(); + if (allowedIndices == null || allowedIndices.isEmpty()) { + throw new IllegalStateException("No protected resources configured"); + } + if (!allowedIndices.contains(strValue)) { + throw new IllegalArgumentException("source_index must be one of: " + allowedIndices); + } + } + }; + + return ImmutableMap.builder() + .put( + "source_index", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.STRING, + RequestContentValidator.MAX_INDEX_NAME_LENGTH, + sourceIndexValidator + ) + ) + .put( + "username_path", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.STRING, + RequestContentValidator.MAX_PATH_LENGTH, + RequestContentValidator.JSON_PATH_VALIDATOR + ) + ) + .put( + "backend_roles_path", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.STRING, + RequestContentValidator.MAX_PATH_LENGTH, + RequestContentValidator.JSON_PATH_VALIDATOR + ) + ) + .put( + "default_owner", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.STRING, + RequestContentValidator.MAX_PRINCIPAL_LENGTH, + RequestContentValidator.PRINCIPAL_VALIDATOR + ) + ) + .put( + "default_access_level", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.OBJECT, + (fieldName, value) -> { + if (value instanceof JsonNode) { + RequestContentValidator.validateDefaultAccessLevelNode((JsonNode) value); + } + } + ) + ) .build(); } }); diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java index a6cc04cb52..cc6a5fe98c 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java @@ -9,6 +9,8 @@ package org.opensearch.security.resources.api.share; import java.io.IOException; +import java.util.List; +import java.util.Set; import com.fasterxml.jackson.annotation.JsonProperty; @@ -20,6 +22,7 @@ import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.xcontent.XContentParser; import org.opensearch.rest.RestRequest; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator; import org.opensearch.security.resources.ResourcePluginInfo; import org.opensearch.security.resources.sharing.ShareWith; @@ -188,6 +191,15 @@ public ShareRequest build() { } public void parseContent(XContentParser xContentParser, ResourcePluginInfo resourcePluginInfo) throws IOException { + final List allowedTypes = resourcePluginInfo.currentProtectedTypes(); + final Set validAccessLevels = resourcePluginInfo.availableAccessLevels(); + + // Create access level validator using the pre-built helper + final RequestContentValidator.FieldValidator accessLevelValidator = RequestContentValidator.allowedValuesValidator( + validAccessLevels, + null + ); + try (XContentParser parser = xContentParser) { XContentParser.Token token; // START_OBJECT while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { @@ -196,20 +208,35 @@ public void parseContent(XContentParser xContentParser, ResourcePluginInfo resou parser.nextToken(); switch (name) { case "resource_id": - this.resourceId(parser.text()); + String resourceId = parser.text(); + RequestContentValidator.validateResourceId(resourceId); + this.resourceId(resourceId); break; case "resource_type": - this.resourceType(parser.text()); - this.resourceIndex(resourcePluginInfo.indexByType(parser.text())); + String resourceType = parser.text(); + + // Validate type syntax + membership in dynamic protected-types list + RequestContentValidator.validateResourceType(resourceType, allowedTypes); + + // Resolve to index, using pluginInfo + String indexName = resourcePluginInfo.indexByType(resourceType); + if (indexName == null) { + throw new IllegalArgumentException( + "Invalid resource_type [" + resourceType + "]. Allowed types: " + String.join(", ", allowedTypes) + ); + } + + this.resourceType(resourceType); + this.resourceIndex(resourcePluginInfo.indexByType(resourceType)); break; case "share_with": - this.shareWith(ShareWith.fromXContent(parser)); + this.shareWith(ShareWith.fromXContent(parser, accessLevelValidator)); break; case "add": - this.add(ShareWith.fromXContent(parser)); + this.add(ShareWith.fromXContent(parser, accessLevelValidator)); break; case "revoke": - this.revoke(ShareWith.fromXContent(parser)); + this.revoke(ShareWith.fromXContent(parser, accessLevelValidator)); break; default: parser.skipChildren(); diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java b/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java index 7213d80c62..b627e2a539 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java @@ -16,6 +16,7 @@ import org.apache.logging.log4j.Logger; import org.opensearch.core.rest.RestStatus; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestRequest; @@ -85,7 +86,12 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli builder.resourceType(resourceType); if (request.hasContent()) { - builder.parseContent(request.contentParser(), resourcePluginInfo); + try (XContentParser parser = request.contentParser()) { + builder.parseContent(parser, resourcePluginInfo); + } catch (IllegalArgumentException | IllegalStateException e) { + return channel -> { channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, e.getMessage())); }; + } + } if (builder.resourceId == null || builder.resourceType == null) { diff --git a/src/main/java/org/opensearch/security/resources/sharing/Recipients.java b/src/main/java/org/opensearch/security/resources/sharing/Recipients.java index 759e8d65ac..3520b06cda 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/Recipients.java +++ b/src/main/java/org/opensearch/security/resources/sharing/Recipients.java @@ -22,6 +22,7 @@ import org.opensearch.core.xcontent.ToXContentFragment; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator; /** * This class represents the entities with which a resource is shared for a particular action-group. @@ -96,19 +97,55 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder.endObject(); } - public static Recipients fromXContent(XContentParser parser) throws IOException { + /** + * Parse Recipients from XContent with validators + * @param parser the XContent parser + * @param arraySizeValidator optional validator for array size (can be null) + * @param elementValidator optional validator for each array element value (can be null) + */ + public static Recipients fromXContent( + XContentParser parser, + RequestContentValidator.FieldValidator arraySizeValidator, + RequestContentValidator.FieldValidator elementValidator + ) throws IOException { Map> recipients = new HashMap<>(); XContentParser.Token token; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { String fieldName = parser.currentName(); - Recipient recipient = Recipient.valueOf(fieldName.toUpperCase(Locale.ROOT)); + + final Recipient recipient; + try { + recipient = Recipient.valueOf(fieldName.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Unknown recipient type [" + fieldName + "]", e); + } parser.nextToken(); + if (parser.currentToken() != XContentParser.Token.START_ARRAY) { + throw new IllegalArgumentException("Expected array for [" + fieldName + "], but found [" + parser.currentToken() + "]"); + } + Set values = new HashSet<>(); + int count = 0; + while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - values.add(parser.text()); + count++; + + // Validate array size if validator provided + if (arraySizeValidator != null) { + arraySizeValidator.validate(fieldName, count); + } + + String value = parser.text(); + + // Validate element value if validator provided + if (elementValidator != null) { + elementValidator.validate(fieldName, value); + } + + values.add(value); } recipients.put(recipient, values); } diff --git a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java index 84a3ccbfc8..f2e9c3eac8 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java @@ -22,6 +22,7 @@ import org.opensearch.core.xcontent.ToXContentFragment; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator; /** * This class contains information about whom a resource is shared with and what is the action-group associated with it. @@ -96,7 +97,20 @@ public XContentBuilder toXContent(XContentBuilder b, Params params) throws IOExc return b.endObject(); } + /** + * Parse ShareWith from XContent without validation + */ public static ShareWith fromXContent(XContentParser parser) throws IOException { + return fromXContent(parser, null); + } + + /** + * Parse ShareWith from XContent with custom access level validator + * @param parser the XContent parser + * @param accessLevelValidator optional validator for access level field names (can be null) + */ + public static ShareWith fromXContent(XContentParser parser, RequestContentValidator.FieldValidator accessLevelValidator) + throws IOException { Map sharingInfo = new HashMap<>(); if (parser.currentToken() != XContentParser.Token.START_OBJECT) { @@ -109,9 +123,18 @@ public static ShareWith fromXContent(XContentParser parser) throws IOException { if (token == XContentParser.Token.FIELD_NAME) { String accessLevel = parser.currentName(); + // Validate access level if validator is provided + if (accessLevelValidator != null) { + accessLevelValidator.validate("access_level", accessLevel); + } + parser.nextToken(); - Recipients recipients = Recipients.fromXContent(parser); + Recipients recipients = Recipients.fromXContent( + parser, + RequestContentValidator.ARRAY_SIZE_VALIDATOR, + RequestContentValidator.PRINCIPAL_VALIDATOR + ); sharingInfo.put(accessLevel, recipients); } } diff --git a/src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java b/src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java index 72ad0ae76a..b6204a03ce 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java @@ -12,7 +12,10 @@ package org.opensearch.security.dlic.rest.validation; import java.io.IOException; +import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; @@ -300,7 +303,7 @@ public Map allowedKeys() { ObjectNode payload = DefaultObjectMapper.objectMapper.createObjectNode().putObject("a"); payload.putArray("a").add("arrray"); - payload.put("b", true).put("d", "some_string").put("e", "true").put("f", 1); + payload.put("b", true).put("d", "some_string").put("e", false).put("f", 1); payload.putObject("c"); when(httpRequest.content()).thenReturn(new BytesArray(payload.toString())); @@ -327,4 +330,385 @@ private void assertErrorMessage(final JsonNode jsonNode, final RequestContentVal assertThat(jsonNode.get("reason").asText(), is(expectedValidationError.message())); } + /* ======================================================================== + * Tests for Static Utility Methods (moved from InputValidationTests) + * ======================================================================== */ + + private static String repeat(char c, int count) { + char[] arr = new char[count]; + Arrays.fill(arr, c); + return new String(arr); + } + + private static void expectThrows(Class expected, Runnable runnable) { + try { + runnable.run(); + org.junit.Assert.fail("Expected exception of type " + expected.getName()); + } catch (Throwable t) { + assertTrue( + "Unexpected exception type. Expected " + expected.getName() + " but got " + t.getClass().getName(), + expected.isInstance(t) + ); + } + } + + /* ---------------------- requireNonEmpty ---------------------- */ + + @Test + public void testRequireNonEmptyAcceptsNonEmpty() { + RequestContentValidator.requireNonEmpty("field", "value"); + } + + @Test + public void testRequireNonEmptyRejectsNull() { + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.requireNonEmpty("field", null)); + } + + @Test + public void testRequireNonEmptyRejectsEmpty() { + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.requireNonEmpty("field", "")); + } + + /* ---------------------- validateMaxLength ---------------------- */ + + @Test + public void testValidateMaxLengthAtBoundary() { + String value = repeat('a', 5); + RequestContentValidator.validateMaxLength("field", value, 5); + } + + @Test + public void testValidateMaxLengthRejectsOverLimit() { + String value = repeat('a', 6); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateMaxLength("field", value, 5)); + } + + /* ---------------------- validateSafeValue ---------------------- */ + + @Test + public void testValidateSafeValueAcceptsAllowedCharacters() { + String value = "Abc123_-:xyz"; + RequestContentValidator.validateSafeValue("field", value, 50); + } + + @Test + public void testValidateSafeValueRejectsInvalidCharacters() { + String value = "bad value$"; // space + $ + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateSafeValue("field", value, 50)); + } + + @Test + public void testValidateSafeValueRejectsTooLong() { + String value = repeat('a', 11); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateSafeValue("field", value, 10)); + } + + /* ---------------------- validateArrayEntryCount ---------------------- */ + + @Test + public void testValidateArrayEntryCountAtMaxBoundary() { + RequestContentValidator.validateArrayEntryCount("field", RequestContentValidator.MAX_ARRAY_SIZE); + } + + @Test + public void testValidateArrayEntryCountRejectsAboveMax() { + int overMax = RequestContentValidator.MAX_ARRAY_SIZE + 1; + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateArrayEntryCount("field", overMax)); + } + + /* ---------------------- validateResourceId ---------------------- */ + + @Test + public void testValidateResourceIdAcceptsValidId() { + String id = "resource_123-ABC:xyz"; + RequestContentValidator.validateResourceId(id); + } + + @Test + public void testValidateResourceIdRejectsInvalidCharacters() { + String id = "invalid id!"; // space + ! + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateResourceId(id)); + } + + @Test + public void testValidateResourceIdRejectsTooLong() { + String id = repeat('a', RequestContentValidator.MAX_RESOURCE_ID_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateResourceId(id)); + } + + /* ---------------------- validateResourceType ---------------------- */ + + @Test + public void testValidateResourceTypeAcceptsValidTypeInAllowedList() { + List allowedTypes = Arrays.asList("anomaly-detector", "forecaster", "ml-model"); + RequestContentValidator.validateResourceType("anomaly-detector", allowedTypes); + } + + @Test + public void testValidateResourceTypeRejectsInvalidCharacters() { + List allowedTypes = List.of("anomaly-detector"); + String resourceType = "anomaly detector"; // contains space + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateResourceType(resourceType, allowedTypes)); + } + + @Test + public void testValidateResourceTypeRejectsWhenNoAllowedTypesConfiguredNull() { + expectThrows(IllegalStateException.class, () -> RequestContentValidator.validateResourceType("anomaly-detector", null)); + } + + @Test + public void testValidateResourceTypeRejectsWhenNoAllowedTypesConfiguredEmpty() { + expectThrows( + IllegalStateException.class, + () -> RequestContentValidator.validateResourceType("anomaly-detector", Collections.emptyList()) + ); + } + + @Test + public void testValidateResourceTypeRejectsWhenTypeNotInAllowedList() { + List allowedTypes = Arrays.asList("anomaly-detector", "forecaster"); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateResourceType("ml-model", allowedTypes)); + } + + /* ---------------------- validatePrincipalValue ---------------------- */ + + @Test + public void testValidatePrincipalValueAcceptsValidPrincipal() { + RequestContentValidator.validatePrincipalValue("users", "user_123-role:1"); + } + + @Test + public void testValidatePrincipalValueRejectsInvalidCharacters() { + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.validatePrincipalValue("users", "user name") // space + ); + } + + @Test + public void testValidatePrincipalValueRejectsTooLong() { + String principal = repeat('u', RequestContentValidator.MAX_PRINCIPAL_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validatePrincipalValue("users", principal)); + } + + /* ---------------------- validateAccessLevel ---------------------- */ + + @Test + public void testValidateAccessLevelAcceptsValidValueInSet() { + Set allowed = new HashSet<>(Arrays.asList("rd_read_only", "rd_write", "forecast:read", "forecast:write")); + + RequestContentValidator.validateAccessLevel("rd_read_only", allowed); + RequestContentValidator.validateAccessLevel("forecast:write", allowed); + } + + @Test + public void testValidateAccessLevelRejectsNullAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel(null, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsEmptyAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel("", allowed)); + } + + @Test + public void testValidateAccessLevelRejectsTooLongAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + String longAccess = repeat('a', RequestContentValidator.MAX_ACCESS_LEVEL_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel(longAccess, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsInvalidCharacters() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + String invalid = "rd read"; // space + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel(invalid, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsWhenNoAccessLevelsConfiguredNull() { + expectThrows(IllegalStateException.class, () -> RequestContentValidator.validateAccessLevel("rd_read_only", null)); + } + + @Test + public void testValidateAccessLevelRejectsWhenNoAccessLevelsConfiguredEmpty() { + expectThrows( + IllegalStateException.class, + () -> RequestContentValidator.validateAccessLevel("rd_read_only", Collections.emptySet()) + ); + } + + @Test + public void testValidateAccessLevelRejectsWhenNotInAllowedSet() { + Set allowed = new HashSet<>(Arrays.asList("rd_read_only", "rd_write")); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel("forecast:read", allowed)); + } + + /* ---------------------- getRequiredText ---------------------- */ + + @Test + public void testGetRequiredTextReturnsValueWhenPresent() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"source_index\":\"index-1\"}"); + + String result = RequestContentValidator.getRequiredText(body, "source_index", RequestContentValidator.MAX_INDEX_NAME_LENGTH); + + assertThat(result, is("index-1")); + } + + @Test + public void testGetRequiredTextThrowsWhenMissing() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"other\":\"value\"}"); + + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.getRequiredText(body, "source_index", RequestContentValidator.MAX_INDEX_NAME_LENGTH) + ); + } + + @Test + public void testGetRequiredTextThrowsWhenNonTextual() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"source_index\":123}"); + + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.getRequiredText(body, "source_index", RequestContentValidator.MAX_INDEX_NAME_LENGTH) + ); + } + + /* ---------------------- getOptionalText ---------------------- */ + + @Test + public void testGetOptionalTextReturnsNullWhenMissing() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"other\":\"value\"}"); + + String result = RequestContentValidator.getOptionalText(body, "default_owner", RequestContentValidator.MAX_PRINCIPAL_LENGTH); + + assertNull(result); + } + + @Test + public void testGetOptionalTextReturnsValueWhenPresent() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"default_owner\":\"owner_1\"}"); + + String result = RequestContentValidator.getOptionalText(body, "default_owner", RequestContentValidator.MAX_PRINCIPAL_LENGTH); + + assertThat(result, is("owner_1")); + } + + @Test + public void testGetOptionalTextThrowsWhenNonTextual() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"default_owner\":123}"); + + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.getOptionalText(body, "default_owner", RequestContentValidator.MAX_PRINCIPAL_LENGTH) + ); + } + + /* ---------------------- validateJsonPath ---------------------- */ + + @Test + public void testValidateJsonPathAcceptsValidPath() { + RequestContentValidator.validateJsonPath("username_path", "user.details.name"); + } + + @Test + public void testValidateJsonPathRejectsEmpty() { + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateJsonPath("username_path", "")); + } + + @Test + public void testValidateJsonPathRejectsWhitespace() { + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateJsonPath("username_path", " user . name ")); + } + + /* ---------------------- validateSourceIndex ---------------------- */ + + @Test + public void testValidateSourceIndexAcceptsWhenInAllowedSet() { + Set allowed = new HashSet<>(); + allowed.add("index-1"); + allowed.add("index-2"); + + RequestContentValidator.validateSourceIndex("index-1", allowed); + } + + @Test + public void testValidateSourceIndexRejectsWhenNotInAllowedSet() { + Set allowed = new HashSet<>(); + allowed.add("index-1"); + + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateSourceIndex("index-2", allowed)); + } + + @Test + public void testValidateSourceIndexRejectsWhenNoIndicesConfiguredNull() { + expectThrows(IllegalStateException.class, () -> RequestContentValidator.validateSourceIndex("index-1", null)); + } + + @Test + public void testValidateSourceIndexRejectsWhenNoIndicesConfiguredEmpty() { + expectThrows(IllegalStateException.class, () -> RequestContentValidator.validateSourceIndex("index-1", Collections.emptySet())); + } + + /* ---------------------- validateDefaultOwner ---------------------- */ + + @Test + public void testValidateDefaultOwnerAllowsNull() { + // optional field + RequestContentValidator.validateDefaultOwner(null); + } + + @Test + public void testValidateDefaultOwnerAcceptsValidPrincipal() { + RequestContentValidator.validateDefaultOwner("owner_123-role:1"); + } + + @Test + public void testValidateDefaultOwnerRejectsInvalidCharacters() { + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.validateDefaultOwner("owner name") // space + ); + } + + /* ---------------------- validateDefaultAccessLevelNode ---------------------- */ + + @Test + public void testValidateDefaultAccessLevelNodeAllowsNull() { + // field absent / null is allowed (optional) + RequestContentValidator.validateDefaultAccessLevelNode(null); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsNonObject() throws Exception { + JsonNode node = DefaultObjectMapper.readTree("\"string-not-object\""); + + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsEmptyObject() throws Exception { + JsonNode node = DefaultObjectMapper.readTree("{}"); + + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsEmptyValue() throws Exception { + JsonNode node = DefaultObjectMapper.readTree("{\"anomaly-detector\":\"\"}"); + + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeAcceptsNonEmptyValues() throws Exception { + JsonNode node = DefaultObjectMapper.readTree("{ \"anomaly-detector\": \"rd_read_only\", \"forecaster\": \"rd_write\" }"); + + // should not throw + RequestContentValidator.validateDefaultAccessLevelNode(node); + } + } diff --git a/src/test/java/org/opensearch/security/multitenancy/test/TenancyMultitenancyEnabledTests.java b/src/test/java/org/opensearch/security/multitenancy/test/TenancyMultitenancyEnabledTests.java index 32b9bb2156..2075636eec 100644 --- a/src/test/java/org/opensearch/security/multitenancy/test/TenancyMultitenancyEnabledTests.java +++ b/src/test/java/org/opensearch/security/multitenancy/test/TenancyMultitenancyEnabledTests.java @@ -92,7 +92,7 @@ public void testMultitenancyDisabled_endToEndTest() throws Exception { final HttpResponse updateMutlitenancyToDisabled = nonSslRestHelper().executePutRequest( "/_plugins/_security/api/tenancy/config", - "{\"multitenancy_enabled\": \"false\"}", + "{\"multitenancy_enabled\": false}", AS_REST_API_USER ); assertThat(updateMutlitenancyToDisabled.getStatusCode(), equalTo(HttpStatus.SC_OK)); diff --git a/src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java b/src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java index 4f2d2c3505..e3a59697e3 100644 --- a/src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java +++ b/src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java @@ -93,7 +93,7 @@ public void testPrivateTenantDisabled_Update_EndToEnd() throws Exception { final HttpResponse disablePrivateTenantResponse = nonSslRestHelper().executePutRequest( "/_plugins/_security/api/tenancy/config", - "{\"private_tenant_enabled\": \"false\"}", + "{\"private_tenant_enabled\": false}", AS_REST_API_USER ); assertThat(disablePrivateTenantResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); diff --git a/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTest.java b/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java similarity index 99% rename from src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTest.java rename to src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java index c1b44c31f9..8fdf5dd584 100644 --- a/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTest.java +++ b/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java @@ -42,7 +42,7 @@ @RunWith(MockitoJUnitRunner.class) @SuppressWarnings("unchecked") // action listener mock -public class ResourceAccessHandlerTest { +public class ResourceAccessHandlerTests { @Mock private ThreadPool threadPool; diff --git a/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java b/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java index f300a5f38a..ab3650ab1a 100644 --- a/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java +++ b/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java @@ -58,7 +58,10 @@ public void testFromXContentWhenCurrentTokenIsNotStartObject() throws IOExceptio parser.nextToken(); - ShareWith shareWith = ShareWith.fromXContent(parser); + ShareWith shareWith = ShareWith.fromXContent( + parser, + org.opensearch.security.dlic.rest.validation.RequestContentValidator.allowedValuesValidator(Set.of("read_only"), null) + ); assertThat(shareWith, notNullValue()); Recipients readOnly = shareWith.atAccessLevel("read_only"); @@ -77,7 +80,7 @@ public void testFromXContentWithEmptyInput() throws IOException { String emptyJson = "{}"; XContentParser parser = XContentType.JSON.xContent().createParser(NamedXContentRegistry.EMPTY, null, emptyJson); - ShareWith result = ShareWith.fromXContent(parser); + ShareWith result = ShareWith.fromXContent(parser, null); assertThat(result, notNullValue()); assertThat(result.isPrivate(), is(true)); @@ -106,7 +109,13 @@ public void testFromXContentWithStartObject() throws IOException { parser.nextToken(); - ShareWith shareWith = ShareWith.fromXContent(parser); + ShareWith shareWith = ShareWith.fromXContent( + parser, + org.opensearch.security.dlic.rest.validation.RequestContentValidator.allowedValuesValidator( + Set.of("read-only", "default"), + null + ) + ); assertThat(shareWith, notNullValue()); @@ -132,7 +141,7 @@ public void testFromXContentWithUnexpectedEndOfInput() throws IOException { when(mockParser.currentToken()).thenReturn(XContentParser.Token.START_OBJECT); when(mockParser.nextToken()).thenReturn(XContentParser.Token.END_OBJECT, (XContentParser.Token) null); - ShareWith result = ShareWith.fromXContent(mockParser); + ShareWith result = ShareWith.fromXContent(mockParser, null); assertThat(result, notNullValue()); assertThat(result.isPrivate(), is(true)); @@ -202,7 +211,7 @@ public void test_fromXContent_emptyObject() throws IOException { parser = XContentType.JSON.xContent().createParser(null, null, builder.toString()); } - ShareWith shareWith = ShareWith.fromXContent(parser); + ShareWith shareWith = ShareWith.fromXContent(parser, null); assertThat(shareWith.isPrivate(), is(true)); assertThat(shareWith.isPublic(), is(false));