[Backport 3.5] [Bugfix] Remove X-Request-Id format restrictions and make size configurable

CHANGELOG.md

@@ -65,6 +65,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Harden detection of HTTP/3 support by ensuring Quic native libraries are available for the target platform ([#20680](https://github.com/opensearch-project/OpenSearch/pull/20680))
 - Fix the regression of terms agg optimization at high cardinality ([#20623](https://github.com/opensearch-project/OpenSearch/pull/20623))
 - Fix TLS cert hot-reload for Arrow Flight transport ([#20732](https://github.com/opensearch-project/OpenSearch/pull/20732))
+- Remove X-Request-Id format restrictions and make size configurable ([#21048](https://github.com/opensearch-project/OpenSearch/pull/21048))
 
 ### Dependencies
 - Bump `com.google.auth:google-auth-library-oauth2-http` from 1.38.0 to 1.41.0 ([#20183](https://github.com/opensearch-project/OpenSearch/pull/20183))

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

@@ -0,0 +1,61 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.rest;
+
+import org.opensearch.client.Request;
+import org.opensearch.client.RequestOptions;
+import org.opensearch.client.Response;
+import org.opensearch.client.ResponseException;
+import org.opensearch.test.rest.OpenSearchRestTestCase;
+
+import java.io.IOException;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+
+public class Netty4RequestIdIT extends OpenSearchRestTestCase {
+
+    private Response requestWithId(String requestId) throws IOException {
+        Request request = new Request("GET", "/_cluster/health");
+        RequestOptions.Builder options = request.getOptions().toBuilder();
+        options.addHeader("X-Request-Id", requestId);
+        request.setOptions(options);
+        return client().performRequest(request);
+    }
+
+    public void testRequestIdExactlyAtMax() throws IOException {
+        assertThat(requestWithId("a".repeat(128)).getStatusLine().getStatusCode(), equalTo(200));
+    }
+
+    public void testRequestIdTooLong() {
+        ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129)));
+        assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400));
+        assertThat(e.getMessage(), containsString("exceeds maximum allowed length"));
+    }
+
+    public void testRequestIdAfterSettingUpdate() throws IOException {
+        int newMax = 20;
+
+        // Expect request is valid under default
+        assertThat(requestWithId("a".repeat(128)).getStatusLine().getStatusCode(), equalTo(200));
+
+        // Update setting
+        Request updateSettings = new Request("PUT", "/_cluster/settings");
+        updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}");
+        client().performRequest(updateSettings);
+
+        // Was valid under default, now too long
+        ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129)));
+        assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400));
+        assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]"));
+
+        // ID at new size passes
+        assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200));
+    }
+}

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

@@ -330,6 +330,7 @@
 import org.opensearch.extensions.action.ExtensionProxyTransportAction;
 import org.opensearch.extensions.rest.RestInitializeExtensionAction;
 import org.opensearch.extensions.rest.RestSendToExtensionAction;
+import org.opensearch.http.HttpTransportSettings;
 import org.opensearch.identity.IdentityService;
 import org.opensearch.index.seqno.RetentionLeaseActions;
 import org.opensearch.indices.SystemIndices;
@@ -613,6 +614,11 @@ public ActionModule(
         );
 
         restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService);
+        restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings));
+        clusterSettings.addSettingsUpdateConsumer(
+            HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH,
+            restController::setRequestIdMaxLength
+        );
         responseLimitSettings = new ResponseLimitSettings(clusterSettings, settings);
     }
 

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

@@ -428,6 +428,7 @@ public void apply(Settings value, Settings current, Settings previous) {
                 HttpTransportSettings.SETTING_HTTP_TRACE_LOG_INCLUDE,
                 HttpTransportSettings.SETTING_HTTP_TRACE_LOG_EXCLUDE,
                 HttpTransportSettings.SETTING_HTTP_HTTP3_ENABLED,
+                HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH,
                 HierarchyCircuitBreakerService.USE_REAL_MEMORY_USAGE_SETTING,
                 HierarchyCircuitBreakerService.TOTAL_CIRCUIT_BREAKER_LIMIT_SETTING,
                 HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING,

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

@@ -27,22 +27,17 @@ public static String generateID() {
     }
 
     /**
-     * Validate whether X-Request-id is valid or not.
+     * Validate whether X-Request-Id is valid or not.
+     * The request ID must be non-empty and not exceed the configured maximum length.
      */
-    public static void validateRequestId(String requestId) {
+    public static void validateRequestId(String requestId, int maxLength) {
         if (requestId == null || requestId.isBlank()) {
             throw new IllegalArgumentException("X-Request-Id should not be null or empty");
         }
-
-        if (requestId.length() != 32) {
-            throw new IllegalArgumentException("Invalid X-Request-Id passed. Should be 32 hexadecimal characters: " + requestId);
-        }
-
-        for (int i = 0; i < requestId.length(); i++) {
-            char c = requestId.charAt(i);
-            if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'))) {
-                throw new IllegalArgumentException("Invalid X-Request-Id passed: " + requestId);
-            }
+        if (requestId.length() > maxLength) {
+            throw new IllegalArgumentException(
+                "X-Request-Id length [" + requestId.length() + "] exceeds maximum allowed length [" + maxLength + "]"
+            );
         }
     }
 

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

@@ -259,6 +259,15 @@ public final class HttpTransportSettings {
         Setting.Property.NodeScope
     );
 
+    public static final Setting<Integer> SETTING_HTTP_REQUEST_ID_MAX_LENGTH = intSetting(
+        "http.request_id.max_length",
+        128,
+        16,
+        1024,
+        Setting.Property.Dynamic,
+        Setting.Property.NodeScope
+    );
+
     // Enable HTTP/3 protocol if supported by the operating system and architecture
     // The HTTP/3 transport is still experimental and should be used with caution.
     public static final Setting<Boolean> SETTING_HTTP_HTTP3_ENABLED = Setting.boolSetting(

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

@@ -40,6 +40,7 @@
 import org.opensearch.common.io.stream.BytesStreamOutput;
 import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.path.PathTrie;
+import org.opensearch.common.settings.Settings;
 import org.opensearch.common.util.RequestUtils;
 import org.opensearch.common.util.concurrent.ThreadContext;
 import org.opensearch.common.util.io.Streams;
@@ -55,6 +56,7 @@
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.http.HttpChunk;
 import org.opensearch.http.HttpServerTransport;
+import org.opensearch.http.HttpTransportSettings;
 import org.opensearch.tasks.Task;
 import org.opensearch.transport.client.node.NodeClient;
 import org.opensearch.usage.UsageService;
@@ -98,6 +100,8 @@ public class RestController implements HttpServerTransport.Dispatcher {
     private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestController.class);
     private static final String OPENSEARCH_PRODUCT_ORIGIN_HTTP_HEADER = "X-opensearch-product-origin";
 
+    private volatile int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY);
+
     private static final BytesReference FAVICON_RESPONSE;
 
     static {
@@ -145,6 +149,10 @@ public RestController(
         );
     }
 
+    public void setRequestIdMaxLength(int maxLength) {
+        this.requestIdMaxLength = maxLength;
+    }
+
     /**
      * Returns an iterator over registered REST method handlers.
      * @return {@link Iterator} of {@link MethodHandlers}
@@ -435,7 +443,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
                     threadContext.putHeader(name, String.join(",", distinctHeaderValues));
                     // Validate request-id header if present
                     if (Task.X_REQUEST_ID.equals(restHeader.getName())) {
-                        RequestUtils.validateRequestId(distinctHeaderValues.getFirst());
+                        RequestUtils.validateRequestId(distinctHeaderValues.getFirst(), requestIdMaxLength);
                     }
                 }
             }

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

@@ -8,73 +8,84 @@
 
 package org.opensearch.common.util;
 
+import org.opensearch.common.settings.Settings;
 import org.opensearch.core.common.Strings;
+import org.opensearch.http.HttpTransportSettings;
 import org.opensearch.test.OpenSearchTestCase;
 
 public class RequestUtilsTests extends OpenSearchTestCase {
 
+    private static final int DEFAULT_MAX_LENGTH = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY);
+
     public void testGenerateID() {
         assertTrue(Strings.hasText(RequestUtils.generateID()));
     }
 
     public void testValidateRequestIdValid() {
-        RequestUtils.validateRequestId("a1b2c3d4e5f67890abcdef1234567890");
-        RequestUtils.validateRequestId("ABCDEF1234567890abcdef1234567890");
-        RequestUtils.validateRequestId("00000000000000000000000000000000");
-        RequestUtils.validateRequestId("ffffffffffffffffffffffffffffffff");
-        RequestUtils.validateRequestId("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF");
+        RequestUtils.validateRequestId("a1b2c3d4e5f67890abcdef1234567890", DEFAULT_MAX_LENGTH);
+        RequestUtils.validateRequestId("ABCDEF1234567890abcdef1234567890", DEFAULT_MAX_LENGTH);
+        RequestUtils.validateRequestId("00000000000000000000000000000000", DEFAULT_MAX_LENGTH);
+        RequestUtils.validateRequestId("ffffffffffffffffffffffffffffffff", DEFAULT_MAX_LENGTH);
+        RequestUtils.validateRequestId("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF", DEFAULT_MAX_LENGTH);
     }
 
     public void testValidateRequestIdNull() {
-        IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> RequestUtils.validateRequestId(null));
+        IllegalArgumentException exception = expectThrows(
+            IllegalArgumentException.class,
+            () -> RequestUtils.validateRequestId(null, DEFAULT_MAX_LENGTH)
+        );
         assertEquals("X-Request-Id should not be null or empty", exception.getMessage());
     }
 
     public void testValidateRequestIdEmpty() {
-        IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> RequestUtils.validateRequestId(""));
+        IllegalArgumentException exception = expectThrows(
+            IllegalArgumentException.class,
+            () -> RequestUtils.validateRequestId("", DEFAULT_MAX_LENGTH)
+        );
         assertEquals("X-Request-Id should not be null or empty", exception.getMessage());
     }
 
     public void testValidateRequestIdBlank() {
-        IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> RequestUtils.validateRequestId("   "));
-        assertEquals("X-Request-Id should not be null or empty", exception.getMessage());
-    }
-
-    public void testValidateRequestIdTooShort() {
         IllegalArgumentException exception = expectThrows(
             IllegalArgumentException.class,
-            () -> RequestUtils.validateRequestId("a1b2c3d4e5f67890")
+            () -> RequestUtils.validateRequestId("   ", DEFAULT_MAX_LENGTH)
         );
-        assertEquals("Invalid X-Request-Id passed. Should be 32 hexadecimal characters: a1b2c3d4e5f67890", exception.getMessage());
+        assertEquals("X-Request-Id should not be null or empty", exception.getMessage());
     }
 
     public void testValidateRequestIdTooLong() {
+        String tooLong = "a".repeat(DEFAULT_MAX_LENGTH + 1);
         IllegalArgumentException exception = expectThrows(
             IllegalArgumentException.class,
-            () -> RequestUtils.validateRequestId("a1b2c3d4e5f67890abcdef1234567890extra")
+            () -> RequestUtils.validateRequestId(tooLong, DEFAULT_MAX_LENGTH)
         );
         assertEquals(
-            "Invalid X-Request-Id passed. Should be 32 hexadecimal characters: a1b2c3d4e5f67890abcdef1234567890extra",
+            "X-Request-Id length [" + (DEFAULT_MAX_LENGTH + 1) + "] exceeds maximum allowed length [" + DEFAULT_MAX_LENGTH + "]",
             exception.getMessage()
         );
     }
 
-    public void testValidateRequestIdInvalidCharacters() {
-        IllegalArgumentException exception = expectThrows(
-            IllegalArgumentException.class,
-            () -> RequestUtils.validateRequestId("g1b2c3d4e5f67890abcdef1234567890")
-        );
-        assertEquals("Invalid X-Request-Id passed: g1b2c3d4e5f67890abcdef1234567890", exception.getMessage());
+    public void testValidateRequestIdNonHexCharactersAllowed() {
+        // Previously rejected, now allowed
+        RequestUtils.validateRequestId("g1b2c3d4e5f67890abcdef1234567890", DEFAULT_MAX_LENGTH);
     }
 
-    public void testValidateRequestIdWithSpecialCharacters() {
+    public void testValidateRequestIdWithSpecialCharactersAllowed() {
+        // UUID with dashes - previously rejected, now allowed
+        RequestUtils.validateRequestId("a1b2c3d4-e5f6-7890-abcd-ef1234567890", DEFAULT_MAX_LENGTH);
+    }
+
+    public void testValidateRequestIdExactlyAtMaxLength() {
+        RequestUtils.validateRequestId("a".repeat(DEFAULT_MAX_LENGTH), DEFAULT_MAX_LENGTH);
+    }
+
+    public void testValidateRequestIdCustomMaxLength() {
+        RequestUtils.validateRequestId("a".repeat(256), 256);
+
         IllegalArgumentException exception = expectThrows(
             IllegalArgumentException.class,
-            () -> RequestUtils.validateRequestId("a1b2c3d4-e5f6-7890-abcd-ef1234567890")
-        );
-        assertEquals(
-            "Invalid X-Request-Id passed. Should be 32 hexadecimal characters: a1b2c3d4-e5f6-7890-abcd-ef1234567890",
-            exception.getMessage()
+            () -> RequestUtils.validateRequestId("a".repeat(33), 32)
         );
+        assertEquals("X-Request-Id length [33] exceeds maximum allowed length [32]", exception.getMessage());
     }
 }