Skip to content

Commit 80ce21c

Browse files
finnegancarrollfinnegancarroll
authored
Remove X-Request-Id format restrictions and make size configurable (#21048)
Introduces dynamic http.request_id.max_length setting for configuring maximum length for X-Request-Id headers. Removes the alpha-numeric validation of X-Request-Id. Signed-off-by: Finn Carroll <carrofin@amazon.com>
1 parent 262c8fd commit 80ce21c

8 files changed

Lines changed: 133 additions & 41 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
7070
- Fix array_index_out_of_bounds_exception with wildcard and aggregations ([#20842](https://github.com/opensearch-project/OpenSearch/pull/20842))
7171
- Fix stale segment cleanup logic for remote store ([#20976](https://github.com/opensearch-project/OpenSearch/pull/20976))
7272
- Ensure that transient ThreadContext headers with propagators survive restore ([#169373](https://github.com/opensearch-project/OpenSearch/pull/20854))
73+
- Remove X-Request-Id format restrictions and make size configurable ([#21048](https://github.com/opensearch-project/OpenSearch/pull/21048))
7374
- Handle dependencies between analyzers ([#19248](https://github.com/opensearch-project/OpenSearch/pull/19248))
7475
- Restore default `shard_path_type` to FIXED for snapshot repositories ([#20643](https://github.com/opensearch-project/OpenSearch/issues/20643))
7576
- Fix `_field_caps` returning empty results and corrupted field names for `disable_objects: true` mappings ([#20800](https://github.com/opensearch-project/OpenSearch/pull/20800))

‎modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
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+
9+
package org.opensearch.rest;
10+
11+
import org.opensearch.client.Request;
12+
import org.opensearch.client.RequestOptions;
13+
import org.opensearch.client.Response;
14+
import org.opensearch.client.ResponseException;
15+
import org.opensearch.test.rest.OpenSearchRestTestCase;
16+
17+
import java.io.IOException;
18+
19+
import static org.hamcrest.Matchers.containsString;
20+
import static org.hamcrest.Matchers.equalTo;
21+
22+
public class Netty4RequestIdIT extends OpenSearchRestTestCase {
23+
24+
private Response requestWithId(String requestId) throws IOException {
25+
Request request = new Request("GET", "/_cluster/health");
26+
RequestOptions.Builder options = request.getOptions().toBuilder();
27+
options.addHeader("X-Request-Id", requestId);
28+
request.setOptions(options);
29+
return client().performRequest(request);
30+
}
31+
32+
public void testRequestIdExactlyAtMax() throws IOException {
33+
assertThat(requestWithId("a".repeat(128)).getStatusLine().getStatusCode(), equalTo(200));
34+
}
35+
36+
public void testRequestIdTooLong() {
37+
ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129)));
38+
assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400));
39+
assertThat(e.getMessage(), containsString("exceeds maximum allowed length"));
40+
}
41+
42+
public void testRequestIdAfterSettingUpdate() throws IOException {
43+
int newMax = 20;
44+
45+
// Expect request is valid under default
46+
assertThat(requestWithId("a".repeat(128)).getStatusLine().getStatusCode(), equalTo(200));
47+
48+
// Update setting
49+
Request updateSettings = new Request("PUT", "/_cluster/settings");
50+
updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}");
51+
client().performRequest(updateSettings);
52+
53+
// Was valid under default, now too long
54+
ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129)));
55+
assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400));
56+
assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]"));
57+
58+
// ID at new size passes
59+
assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200));
60+
}
61+
}

‎server/src/main/java/org/opensearch/action/ActionModule.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,7 @@
330330
import org.opensearch.extensions.action.ExtensionProxyTransportAction;
331331
import org.opensearch.extensions.rest.RestInitializeExtensionAction;
332332
import org.opensearch.extensions.rest.RestSendToExtensionAction;
333+
import org.opensearch.http.HttpTransportSettings;
333334
import org.opensearch.identity.IdentityService;
334335
import org.opensearch.index.seqno.RetentionLeaseActions;
335336
import org.opensearch.indices.SystemIndices;
@@ -613,6 +614,11 @@ public ActionModule(
613614
);
614615

615616
restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService);
617+
restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings));
618+
clusterSettings.addSettingsUpdateConsumer(
619+
HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH,
620+
restController::setRequestIdMaxLength
621+
);
616622
responseLimitSettings = new ResponseLimitSettings(clusterSettings, settings);
617623
}
618624

‎server/src/main/java/org/opensearch/common/settings/ClusterSettings.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,7 @@ public void apply(Settings value, Settings current, Settings previous) {
428428
HttpTransportSettings.SETTING_HTTP_TRACE_LOG_INCLUDE,
429429
HttpTransportSettings.SETTING_HTTP_TRACE_LOG_EXCLUDE,
430430
HttpTransportSettings.SETTING_HTTP_HTTP3_ENABLED,
431+
HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH,
431432
HierarchyCircuitBreakerService.USE_REAL_MEMORY_USAGE_SETTING,
432433
HierarchyCircuitBreakerService.TOTAL_CIRCUIT_BREAKER_LIMIT_SETTING,
433434
HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING,

‎server/src/main/java/org/opensearch/common/util/RequestUtils.java‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,22 +27,17 @@ public static String generateID() {
2727
}
2828

2929
/**
30-
* Validate whether X-Request-id is valid or not.
30+
* Validate whether X-Request-Id is valid or not.
31+
* The request ID must be non-empty and not exceed the configured maximum length.
3132
*/
32-
public static void validateRequestId(String requestId) {
33+
public static void validateRequestId(String requestId, int maxLength) {
3334
if (requestId == null || requestId.isBlank()) {
3435
throw new IllegalArgumentException("X-Request-Id should not be null or empty");
3536
}
36-
37-
if (requestId.length() != 32) {
38-
throw new IllegalArgumentException("Invalid X-Request-Id passed. Should be 32 hexadecimal characters: " + requestId);
39-
}
40-
41-
for (int i = 0; i < requestId.length(); i++) {
42-
char c = requestId.charAt(i);
43-
if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'))) {
44-
throw new IllegalArgumentException("Invalid X-Request-Id passed: " + requestId);
45-
}
37+
if (requestId.length() > maxLength) {
38+
throw new IllegalArgumentException(
39+
"X-Request-Id length [" + requestId.length() + "] exceeds maximum allowed length [" + maxLength + "]"
40+
);
4641
}
4742
}
4843

‎server/src/main/java/org/opensearch/http/HttpTransportSettings.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,15 @@ public final class HttpTransportSettings {
259259
Setting.Property.NodeScope
260260
);
261261

262+
public static final Setting<Integer> SETTING_HTTP_REQUEST_ID_MAX_LENGTH = intSetting(
263+
"http.request_id.max_length",
264+
128,
265+
16,
266+
1024,
267+
Setting.Property.Dynamic,
268+
Setting.Property.NodeScope
269+
);
270+
262271
// Enable HTTP/3 protocol if supported by the operating system and architecture
263272
// The HTTP/3 transport is still experimental and should be used with caution.
264273
public static final Setting<Boolean> SETTING_HTTP_HTTP3_ENABLED = Setting.boolSetting(

‎server/src/main/java/org/opensearch/rest/RestController.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.opensearch.common.io.stream.BytesStreamOutput;
4141
import org.opensearch.common.logging.DeprecationLogger;
4242
import org.opensearch.common.path.PathTrie;
43+
import org.opensearch.common.settings.Settings;
4344
import org.opensearch.common.util.RequestUtils;
4445
import org.opensearch.common.util.concurrent.ThreadContext;
4546
import org.opensearch.common.util.io.Streams;
@@ -55,6 +56,7 @@
5556
import org.opensearch.core.xcontent.XContentBuilder;
5657
import org.opensearch.http.HttpChunk;
5758
import org.opensearch.http.HttpServerTransport;
59+
import org.opensearch.http.HttpTransportSettings;
5860
import org.opensearch.tasks.Task;
5961
import org.opensearch.transport.client.node.NodeClient;
6062
import org.opensearch.usage.UsageService;
@@ -98,6 +100,8 @@ public class RestController implements HttpServerTransport.Dispatcher {
98100
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestController.class);
99101
private static final String OPENSEARCH_PRODUCT_ORIGIN_HTTP_HEADER = "X-opensearch-product-origin";
100102

103+
private volatile int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY);
104+
101105
private static final BytesReference FAVICON_RESPONSE;
102106

103107
static {
@@ -145,6 +149,10 @@ public RestController(
145149
);
146150
}
147151

152+
public void setRequestIdMaxLength(int maxLength) {
153+
this.requestIdMaxLength = maxLength;
154+
}
155+
148156
/**
149157
* Returns an iterator over registered REST method handlers.
150158
* @return {@link Iterator} of {@link MethodHandlers}
@@ -435,7 +443,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
435443
threadContext.putHeader(name, String.join(",", distinctHeaderValues));
436444
// Validate request-id header if present
437445
if (Task.X_REQUEST_ID.equals(restHeader.getName())) {
438-
RequestUtils.validateRequestId(distinctHeaderValues.getFirst());
446+
RequestUtils.validateRequestId(distinctHeaderValues.getFirst(), requestIdMaxLength);
439447
}
440448
}
441449
}

‎server/src/test/java/org/opensearch/common/util/RequestUtilsTests.java‎

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,73 +8,84 @@
88

99
package org.opensearch.common.util;
1010

11+
import org.opensearch.common.settings.Settings;
1112
import org.opensearch.core.common.Strings;
13+
import org.opensearch.http.HttpTransportSettings;
1214
import org.opensearch.test.OpenSearchTestCase;
1315

1416
public class RequestUtilsTests extends OpenSearchTestCase {
1517

18+
private static final int DEFAULT_MAX_LENGTH = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY);
19+
1620
public void testGenerateID() {
1721
assertTrue(Strings.hasText(RequestUtils.generateID()));
1822
}
1923

2024
public void testValidateRequestIdValid() {
21-
RequestUtils.validateRequestId("a1b2c3d4e5f67890abcdef1234567890");
22-
RequestUtils.validateRequestId("ABCDEF1234567890abcdef1234567890");
23-
RequestUtils.validateRequestId("00000000000000000000000000000000");
24-
RequestUtils.validateRequestId("ffffffffffffffffffffffffffffffff");
25-
RequestUtils.validateRequestId("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF");
25+
RequestUtils.validateRequestId("a1b2c3d4e5f67890abcdef1234567890", DEFAULT_MAX_LENGTH);
26+
RequestUtils.validateRequestId("ABCDEF1234567890abcdef1234567890", DEFAULT_MAX_LENGTH);
27+
RequestUtils.validateRequestId("00000000000000000000000000000000", DEFAULT_MAX_LENGTH);
28+
RequestUtils.validateRequestId("ffffffffffffffffffffffffffffffff", DEFAULT_MAX_LENGTH);
29+
RequestUtils.validateRequestId("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF", DEFAULT_MAX_LENGTH);
2630
}
2731

2832
public void testValidateRequestIdNull() {
29-
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> RequestUtils.validateRequestId(null));
33+
IllegalArgumentException exception = expectThrows(
34+
IllegalArgumentException.class,
35+
() -> RequestUtils.validateRequestId(null, DEFAULT_MAX_LENGTH)
36+
);
3037
assertEquals("X-Request-Id should not be null or empty", exception.getMessage());
3138
}
3239

3340
public void testValidateRequestIdEmpty() {
34-
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> RequestUtils.validateRequestId(""));
41+
IllegalArgumentException exception = expectThrows(
42+
IllegalArgumentException.class,
43+
() -> RequestUtils.validateRequestId("", DEFAULT_MAX_LENGTH)
44+
);
3545
assertEquals("X-Request-Id should not be null or empty", exception.getMessage());
3646
}
3747

3848
public void testValidateRequestIdBlank() {
39-
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> RequestUtils.validateRequestId(" "));
40-
assertEquals("X-Request-Id should not be null or empty", exception.getMessage());
41-
}
42-
43-
public void testValidateRequestIdTooShort() {
4449
IllegalArgumentException exception = expectThrows(
4550
IllegalArgumentException.class,
46-
() -> RequestUtils.validateRequestId("a1b2c3d4e5f67890")
51+
() -> RequestUtils.validateRequestId(" ", DEFAULT_MAX_LENGTH)
4752
);
48-
assertEquals("Invalid X-Request-Id passed. Should be 32 hexadecimal characters: a1b2c3d4e5f67890", exception.getMessage());
53+
assertEquals("X-Request-Id should not be null or empty", exception.getMessage());
4954
}
5055

5156
public void testValidateRequestIdTooLong() {
57+
String tooLong = "a".repeat(DEFAULT_MAX_LENGTH + 1);
5258
IllegalArgumentException exception = expectThrows(
5359
IllegalArgumentException.class,
54-
() -> RequestUtils.validateRequestId("a1b2c3d4e5f67890abcdef1234567890extra")
60+
() -> RequestUtils.validateRequestId(tooLong, DEFAULT_MAX_LENGTH)
5561
);
5662
assertEquals(
57-
"Invalid X-Request-Id passed. Should be 32 hexadecimal characters: a1b2c3d4e5f67890abcdef1234567890extra",
63+
"X-Request-Id length [" + (DEFAULT_MAX_LENGTH + 1) + "] exceeds maximum allowed length [" + DEFAULT_MAX_LENGTH + "]",
5864
exception.getMessage()
5965
);
6066
}
6167

62-
public void testValidateRequestIdInvalidCharacters() {
63-
IllegalArgumentException exception = expectThrows(
64-
IllegalArgumentException.class,
65-
() -> RequestUtils.validateRequestId("g1b2c3d4e5f67890abcdef1234567890")
66-
);
67-
assertEquals("Invalid X-Request-Id passed: g1b2c3d4e5f67890abcdef1234567890", exception.getMessage());
68+
public void testValidateRequestIdNonHexCharactersAllowed() {
69+
// Previously rejected, now allowed
70+
RequestUtils.validateRequestId("g1b2c3d4e5f67890abcdef1234567890", DEFAULT_MAX_LENGTH);
6871
}
6972

70-
public void testValidateRequestIdWithSpecialCharacters() {
73+
public void testValidateRequestIdWithSpecialCharactersAllowed() {
74+
// UUID with dashes - previously rejected, now allowed
75+
RequestUtils.validateRequestId("a1b2c3d4-e5f6-7890-abcd-ef1234567890", DEFAULT_MAX_LENGTH);
76+
}
77+
78+
public void testValidateRequestIdExactlyAtMaxLength() {
79+
RequestUtils.validateRequestId("a".repeat(DEFAULT_MAX_LENGTH), DEFAULT_MAX_LENGTH);
80+
}
81+
82+
public void testValidateRequestIdCustomMaxLength() {
83+
RequestUtils.validateRequestId("a".repeat(256), 256);
84+
7185
IllegalArgumentException exception = expectThrows(
7286
IllegalArgumentException.class,
73-
() -> RequestUtils.validateRequestId("a1b2c3d4-e5f6-7890-abcd-ef1234567890")
74-
);
75-
assertEquals(
76-
"Invalid X-Request-Id passed. Should be 32 hexadecimal characters: a1b2c3d4-e5f6-7890-abcd-ef1234567890",
77-
exception.getMessage()
87+
() -> RequestUtils.validateRequestId("a".repeat(33), 32)
7888
);
89+
assertEquals("X-Request-Id length [33] exceeds maximum allowed length [32]", exception.getMessage());
7990
}
8091
}

0 commit comments

Comments
 (0)