Skip to content

Commit 222523c

Browse files
cwperkscwperks
authored
Add shared security API request validation (#6595)
Signed-off-by: Craig Perkins <craig5008@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com>
1 parent dbcd9a4 commit 222523c

7 files changed

Lines changed: 339 additions & 29 deletions

File tree

‎src/integrationTest/java/org/opensearch/security/api/SecurityApiRequestValidationIntegrationTest.java‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
package org.opensearch.security.api;
9+
10+
import java.net.URLEncoder;
11+
import java.nio.charset.StandardCharsets;
12+
import java.util.List;
13+
import java.util.Map;
14+
15+
import org.junit.ClassRule;
16+
import org.junit.Test;
17+
18+
import org.opensearch.security.DefaultObjectMapper;
19+
import org.opensearch.test.framework.cluster.LocalCluster;
20+
import org.opensearch.test.framework.cluster.TestRestClient;
21+
22+
import static org.junit.Assert.assertEquals;
23+
import static org.junit.Assert.assertFalse;
24+
import static org.junit.Assert.assertTrue;
25+
26+
public class SecurityApiRequestValidationIntegrationTest {
27+
private static final String BASE = "_plugins/_security/api/";
28+
private static final String EXPRESSION = "${env.SECURITY_API_VALIDATION_TEST:-synthetic}";
29+
private static final String ROLE_BODY = "{\"cluster_permissions\":[\"cluster:monitor/main\"]}";
30+
31+
@ClassRule
32+
public static final LocalCluster CLUSTER = new LocalCluster.Builder().singleNode().build();
33+
34+
@Test
35+
public void rejectsBodyExpressionsAcrossEndpoints() throws Exception {
36+
try (TestRestClient admin = CLUSTER.getAdminCertRestClient()) {
37+
for (var entry : Map.of(
38+
"roles",
39+
Map.of("cluster_permissions", List.of(EXPRESSION)),
40+
"rolesmapping",
41+
Map.of("backend_roles", List.of(EXPRESSION)),
42+
"tenants",
43+
Map.of("description", EXPRESSION),
44+
"actiongroups",
45+
Map.of("allowed_actions", List.of(EXPRESSION)),
46+
"internalusers",
47+
Map.of("password", "Synthetic-test-password!42", "attributes", Map.of(EXPRESSION, "value")),
48+
"authfailurelisteners",
49+
Map.of("type", "ip", "ignore_hosts", List.of(EXPRESSION))
50+
).entrySet()) {
51+
String path = BASE + entry.getKey() + "/environment_validation_test";
52+
var original = admin.get(BASE + entry.getKey()).bodyAsJsonNode();
53+
assertRejected(admin.putJson(path, DefaultObjectMapper.writeValueAsString(entry.getValue(), false)));
54+
assertEquals(original, admin.get(BASE + entry.getKey()).bodyAsJsonNode());
55+
}
56+
}
57+
}
58+
59+
@Test
60+
public void rejectsEncodedRouteNames() {
61+
try (TestRestClient admin = CLUSTER.getAdminCertRestClient()) {
62+
String encodedName = URLEncoder.encode(EXPRESSION, StandardCharsets.UTF_8);
63+
for (var endpoint : Map.of(
64+
"roles",
65+
ROLE_BODY,
66+
"tenants",
67+
"{\"description\":\"ordinary\"}",
68+
"authfailurelisteners",
69+
"{\"type\":\"ip\"}"
70+
).entrySet()) {
71+
String path = BASE + endpoint.getKey() + "/" + encodedName;
72+
var original = admin.get(BASE + endpoint.getKey()).bodyAsJsonNode();
73+
assertRejected(admin.putJson(path, endpoint.getValue()));
74+
assertEquals(original, admin.get(BASE + endpoint.getKey()).bodyAsJsonNode());
75+
}
76+
}
77+
}
78+
79+
@Test
80+
public void rejectsEscapedExpressionsAndAsyncWritesWithoutChangingRole() {
81+
try (TestRestClient admin = CLUSTER.getAdminCertRestClient()) {
82+
String path = BASE + "roles/environment_validation_test";
83+
try {
84+
assertEquals(201, admin.putJson(path, ROLE_BODY).getStatusCode());
85+
var original = admin.get(path).bodyAsJsonNode();
86+
for (String suffix : List.of("", "?wait_for_completion=false")) {
87+
assertRejected(
88+
admin.putJson(path + suffix, "{\"cluster_permissions\":[\"\\u0024{env.SECURITY_API_VALIDATION_TEST:-synthetic}\"]}")
89+
);
90+
assertEquals(original, admin.get(path).bodyAsJsonNode());
91+
}
92+
} finally {
93+
admin.delete(path);
94+
}
95+
}
96+
}
97+
98+
@Test
99+
public void rejectsSingleAndMultiEntityPatchesAtomically() throws Exception {
100+
try (TestRestClient admin = CLUSTER.getAdminCertRestClient()) {
101+
String path = BASE + "roles/environment_validation_test";
102+
try {
103+
assertEquals(201, admin.putJson(path, ROLE_BODY).getStatusCode());
104+
var original = admin.get(path).bodyAsJsonNode();
105+
String singlePatch = DefaultObjectMapper.writeValueAsString(
106+
List.of(Map.of("op", "add", "path", "/cluster_permissions/-", "value", EXPRESSION)),
107+
false
108+
);
109+
assertRejected(admin.patch(path, singlePatch));
110+
String bulkPatch = DefaultObjectMapper.writeValueAsString(
111+
List.of(
112+
Map.of("op", "add", "path", "/ordinary_patch_role", "value", Map.of("cluster_permissions", List.of())),
113+
Map.of("op", "add", "path", "/" + EXPRESSION, "value", Map.of("cluster_permissions", List.of()))
114+
),
115+
false
116+
);
117+
assertRejected(admin.patch(BASE + "roles", bulkPatch));
118+
assertEquals(original, admin.get(path).bodyAsJsonNode());
119+
assertEquals(404, admin.get(BASE + "roles/ordinary_patch_role").getStatusCode());
120+
} finally {
121+
admin.delete(path);
122+
admin.delete(BASE + "roles/ordinary_patch_role");
123+
}
124+
}
125+
}
126+
127+
@Test
128+
public void permitsDlsUserAttributeTemplates() {
129+
try (TestRestClient admin = CLUSTER.getAdminCertRestClient()) {
130+
String path = BASE + "roles/environment_validation_template";
131+
try {
132+
assertEquals(201, admin.putJson(path, """
133+
{"index_permissions":[{"index_patterns":["test"],"allowed_actions":["read"],
134+
"dls":"{\\"term\\":{\\"owner\\":\\"${user.name}\\"}}"}]}
135+
""").getStatusCode());
136+
} finally {
137+
admin.delete(path);
138+
}
139+
}
140+
}
141+
142+
private void assertRejected(TestRestClient.HttpResponse response) {
143+
assertEquals(response.getBody(), 400, response.getStatusCode());
144+
assertTrue(response.getBody(), response.getBody().contains("must not contain environment variable expressions"));
145+
assertFalse(response.getBody().contains("SECURITY_API_VALIDATION_TEST"));
146+
}
147+
}

‎src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
import org.opensearch.security.dlic.rest.support.Utils;
5656
import org.opensearch.security.dlic.rest.validation.EndpointValidator;
5757
import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
58+
import org.opensearch.security.dlic.rest.validation.SecurityApiRequestValidator;
5859
import org.opensearch.security.dlic.rest.validation.ValidationResult;
5960
import org.opensearch.security.filter.SecurityRequestFactory;
6061
import org.opensearch.security.securityconf.DynamicConfigFactory;
@@ -713,6 +714,12 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
713714
securityApiDependencies.auditLog().logGrantedPrivileges(userName, SecurityRequestFactory.from(auditLogRequest));
714715
}
715716

717+
// Shared by all endpoint handlers, including overrides and asynchronous config writes.
718+
final var requestValidation = SecurityApiRequestValidator.validate(request);
719+
if (!requestValidation.isValid()) {
720+
return channel -> Responses.response(channel, requestValidation.status(), requestValidation.errorMessage());
721+
}
722+
716723
final var originalUserAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadPool.getThreadContext());
717724
final Object originalOrigin = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN);
718725

‎src/main/java/org/opensearch/security/dlic/rest/api/RateLimitersApiAction.java‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import java.util.List;
1616
import java.util.Map;
1717
import java.util.Set;
18-
import java.util.regex.Pattern;
1918

2019
import com.google.common.collect.ImmutableList;
2120
import com.google.common.collect.ImmutableMap;
@@ -75,7 +74,6 @@ public class RateLimitersApiAction extends AbstractApiAction {
7574

7675
private static final FieldValidator POSITIVE_INTEGER_VALIDATOR = integerRangeValidator(1, Integer.MAX_VALUE);
7776
private static final FieldValidator NON_NEGATIVE_INTEGER_VALIDATOR = integerRangeValidator(0, Integer.MAX_VALUE);
78-
private static final Pattern REJECTED_PATTERNS = Pattern.compile("\\$\\{env");
7977

8078
private static final List<Route> ROUTES = addRoutesPrefix(
8179
ImmutableList.of(
@@ -152,7 +150,7 @@ public Map<String, FieldConfiguration> allowedKeys() {
152150
return allowedKeys.put(TYPE_JSON_PROPERTY, FieldConfiguration.of(DataType.STRING))
153151
.put(
154152
IGNORE_HOSTS_JSON_PROPERTY,
155-
FieldConfiguration.of(DataType.ARRAY, RateLimitersApiAction::validateIgnoreHosts)
153+
FieldConfiguration.of(DataType.ARRAY, RequestContentValidator.ARRAY_OF_STRINGS_VALIDATOR)
156154
)
157155
.put(AUTHENTICATION_BACKEND_JSON_PROPERTY, FieldConfiguration.of(DataType.STRING))
158156
.put(ALLOWED_TRIES_JSON_PROPERTY, FieldConfiguration.of(DataType.INTEGER, POSITIVE_INTEGER_VALIDATOR))
@@ -283,17 +281,6 @@ private ValidationResult<SecurityJsonNode> validateAuthFailureListener(SecurityJ
283281
return ValidationResult.success(authFailureListener);
284282
}
285283

286-
private static void validateIgnoreHosts(String fieldName, Object value) {
287-
RequestContentValidator.ARRAY_OF_STRINGS_VALIDATOR.validate(fieldName, value);
288-
if (value instanceof JsonNode arrayNode) {
289-
for (JsonNode element : arrayNode) {
290-
if (element.isTextual() && REJECTED_PATTERNS.matcher(element.asText()).find()) {
291-
throw new IllegalArgumentException(fieldName + " must not contain environment variable expressions");
292-
}
293-
}
294-
}
295-
}
296-
297284
private static FieldValidator integerRangeValidator(int minimum, int maximum) {
298285
return (fieldName, value) -> {
299286
if (value instanceof JsonNode node) {

‎src/main/java/org/opensearch/security/dlic/rest/validation/SecurityApiRequestValidator.java‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
package org.opensearch.security.dlic.rest.validation;
9+
10+
import java.util.regex.Pattern;
11+
12+
import org.opensearch.core.rest.RestStatus;
13+
import org.opensearch.rest.RestRequest;
14+
import org.opensearch.security.DefaultObjectMapper;
15+
16+
import tools.jackson.core.JacksonException;
17+
import tools.jackson.core.JsonToken;
18+
19+
import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage;
20+
21+
/**
22+
* Shared input validation before dispatching security API requests to endpoint-specific handlers.
23+
* Currently rejects environment expressions that could be expanded when persisted configuration is loaded.
24+
*/
25+
public final class SecurityApiRequestValidator {
26+
// Matches the shared prefix of ${env.}, ${envbc.} and ${envbase64.} substitutions, including malformed forms.
27+
private static final Pattern ENV_EXPRESSION_PATTERN = Pattern.compile("\\$\\{env");
28+
29+
private SecurityApiRequestValidator() {}
30+
31+
public static ValidationResult<RestRequest> validate(RestRequest request) {
32+
if (request.method() != RestRequest.Method.PUT
33+
&& request.method() != RestRequest.Method.PATCH
34+
&& request.method() != RestRequest.Method.POST) {
35+
// Reads and deletes must remain available to inspect or remove existing configuration.
36+
return ValidationResult.success(request);
37+
}
38+
for (var parameter : request.params().entrySet()) {
39+
if (containsExpression(parameter.getKey()) || containsExpression(parameter.getValue())) {
40+
return rejected();
41+
}
42+
}
43+
if (request.hasContent()) {
44+
try (var parser = DefaultObjectMapper.objectMapper().createParser(request.content().utf8ToString())) {
45+
// Jackson handles nesting and escapes for both property names and string values.
46+
JsonToken token;
47+
while ((token = parser.nextToken()) != null) {
48+
if ((token == JsonToken.PROPERTY_NAME || token == JsonToken.VALUE_STRING) && containsExpression(parser.getString())) {
49+
return rejected();
50+
}
51+
}
52+
} catch (JacksonException e) {
53+
return ValidationResult.error(
54+
RestStatus.BAD_REQUEST,
55+
(builder, params) -> builder.startObject()
56+
.field("status", "error")
57+
.field("reason", RequestContentValidator.ValidationError.BODY_NOT_PARSEABLE.message())
58+
.endObject()
59+
);
60+
}
61+
}
62+
return ValidationResult.success(request);
63+
}
64+
65+
private static boolean containsExpression(String value) {
66+
return value != null && ENV_EXPRESSION_PATTERN.matcher(value).find();
67+
}
68+
69+
private static ValidationResult<RestRequest> rejected() {
70+
// Do not echo potentially sensitive request content or attempt substitution.
71+
return ValidationResult.error(
72+
RestStatus.BAD_REQUEST,
73+
badRequestMessage("Security API request bodies and parameters must not contain environment variable expressions")
74+
);
75+
}
76+
}

‎src/test/java/org/opensearch/security/dlic/rest/api/RateLimitersApiActionTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ public void testInvalidPutScenarios() throws Exception {
165165
);
166166
assertThat(
167167
updateAuthFailuresResponseWithEnvExpression.getBody(),
168-
containsString("ignore_hosts must not contain environment variable expressions")
168+
containsString("Security API request bodies and parameters must not contain environment variable expressions")
169169
);
170170

171171
RestHelper.HttpResponse updateAuthFailuresResponseWithMalformedHost = rh.executePutRequest(

‎src/test/java/org/opensearch/security/dlic/rest/api/RateLimitersApiActionValidationTest.java‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -68,20 +68,6 @@ public void validateIgnoreHostsElements() throws IOException {
6868
assertInvalidField(nonStringContent, "ignore_hosts", "should only contain string values");
6969
}
7070

71-
@Test
72-
public void rejectEnvironmentExpressionsInIgnoreHosts() throws IOException {
73-
for (String expression : List.of(
74-
"${env.IGNORED_HOST}",
75-
"${envbc.IGNORED_HOST}",
76-
"${envbase64.IGNORED_HOST}",
77-
"${envbase64:IGNORED_HOST}"
78-
)) {
79-
final ObjectNode content = objectMapper.createObjectNode().put("type", "ip");
80-
content.putArray("ignore_hosts").add(expression);
81-
assertInvalidField(content, "ignore_hosts", "must not contain environment variable expressions");
82-
}
83-
}
84-
8571
@Test
8672
public void rejectInvalidNumericRanges() throws IOException {
8773
for (String field : List.of("time_window_seconds", "block_expiry_seconds", "max_blocked_clients", "max_tracked_clients")) {

0 commit comments

Comments
 (0)