Add length validation for rename_replacement parameter in snapshot restore request

[gmjehovich]

Summary

Adds validation to the rename_replacement parameter in the snapshot restore API to prevent resource exhaustion.

Changes

• Added validation in RestoreSnapshotRequest.validate() to limit rename_replacement UTF-8 byte length to MAX_INDEX_NAME_BYTES (255 bytes)
• Added integration test testRenameReplacementTooLongRejected() to verify the validation works correctly
• Imported MAX_INDEX_NAME_BYTES from MetadataCreateIndexService to maintain consistency with index name constraints

Testing

• Integration test confirms validation rejects overly long values
• Test confirms valid values (≤255 bytes) are still accepted
• Verified existing restore functionality is not affected

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[elasticsearchmachine]

Hi @gmjehovich, I've created a changelog YAML for you.

[ywangd]

Looking good. I have a suggestion about whether we should further restrict building unnecessarily long strings for renamed index names.

[ywangd]

Due to usage of back-references and escapes, the result index name can be shorter than 255 when renameReplacement is longer than 255. So we could be preventing some legal index names in edge cases. I think it might be OK though since such long index names are very rare.

[gmjehovich]

Hmm, I hadn't considered this, it is a good point.

It seems to me like the easiest way to get around this would be to add a multiplier to make the validation more permissive for back-references/escapes, something like

            if (renameReplacement.getBytes(StandardCharsets.UTF_8).length > MAX_INDEX_NAME_BYTES * 2) {

The multiplier itself is kind of arbitrary, but it provides some breathing room for the cases you brought up. Let me know what you think.

[ywangd]

Yeah. We could do something along that line. I'd prefer to be a bit more strict, e.g. allowing an extra buffer of 10 chars.

[ywangd]

We can random the length from 256 to 10_000_000

[ywangd]

Can we test that a restore with rename pattern b fails expectedly, i.e. it does not OOM? It should create a replacement name of "a".repeat (255 * 255) which then fails index name validation.

I think ideally we'd change the replaceAll method call with a manual loop of find and append calls so that it bails out as soon as the string builder is too long, similar to how this safeReplace works. That way we avoid building a long string (up to 65,025) unnecessarily and then abandonned it shortly after. The long name also fills logs. The heap usage is certainly not as bad as, or even close to, the unbounded rename replacement. That said, a restore request could contain thousands of indices and there could be multiple requests running concurrently (up to 50).

[gmjehovich]

I've added a test that verifies the 255×255 scenario fails with proper index name validation rather than OOM.

Regarding the manual loop implementation similar to safeReplace, I attempted this approach but ran into issues with accurately estimating sizes for back-references ($1, $2, etc.). The conservative estimation (assuming each group could expand to the full match size) would reject valid cases where capture groups only match portions of the text. These false rejections would likely be edge cases (complex regex patterns with multiple capture groups), but they're still valid operations that would be blocked.

For now, I've kept the simpler validation that prevents the unbounded OOM cases. While the 65KB intermediate strings aren't ideal, they're bounded, and I don't believe they'll cause an OOM error (although I haven't actually tested it with 50 concurrent requests). As you noted, the heap usage is nowhere near the unbounded case this PR fixes.

How much of a concern is the current replaceAll implementation for you? The manual loop would be more efficient, but with the tradeoff that some valid rename patterns may be rejected. Happy to implement it if you think the efficiency gain is worth that tradeoff, or it could be explored more in a follow-up PR.

[ywangd]

I attempted this approach but ran into issues with accurately estimating sizes for back-references ($1, $2, etc.).
with the tradeoff that some valid rename patterns may be rejected.

I don't quite follow. I was thinking something like the following change to RestoreService.java

@@ -1074,7 +1075,22 @@ public final class RestoreService implements ClusterStateApplier {
         if (prefix != null) {
             index = index.substring(prefix.length());
         }
-        renamedIndex = index.replaceAll(request.renamePattern(), request.renameReplacement());
+        final var matcher = Pattern.compile(request.renamePattern()).matcher(index);
+        var found = matcher.find();
+        if (found) {
+            final var sb = new StringBuilder();
+            do {
+                matcher.appendReplacement(sb, request.renameReplacement());
+                found = matcher.find();
+            } while (found && sb.length() <= 255);
+            if (sb.length() > 255) {
+                throw new IllegalArgumentException();
+            }
+            matcher.appendTail(sb);
+            renamedIndex = sb.toString();
+        } else {
+            renamedIndex = index;
+        }
         if (prefix != null) {
             renamedIndex = prefix + renamedIndex;
         }

Does it not work and could reject valid cases?

it could be explored more in a follow-up PR

I am OK for it to be a follow-up if you prefer.

[gmjehovich]

Ahhh I see what you mean now, I think this avoids the issue I was running into... I was trying to copy the logic in safeReplace too closely, which does an estimation before actually making the string and led to that issue I described. I don't think we need to be that careful here, and this approach you provided should work without rejecting valid cases.

I will do some testing on it tomorrow.

[ywangd]

This looks great. I only had one minor comment. Thanks!

[ywangd]

It would be great to have an unit test for this method itself.

[ywangd]

LGTM

Thanks for the iterations!

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
✅	9.2
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	9.1	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 137859

[gmjehovich]

💚 All backports created successfully

Status	Branch	Result
✅	8.19
✅	9.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation