diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index ce35c9ca1bd2..fc82ed8bf826 100644 --- a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java +++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.Map; import java.util.function.Predicate; +import java.util.regex.Pattern; import com.google.common.util.concurrent.RateLimiter; @@ -33,13 +34,21 @@ import org.apache.cassandra.config.DurationSpec; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.io.sstable.format.SSTableReader; +import org.apache.cassandra.schema.SchemaConstants; import static java.lang.String.format; +import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH; public class SnapshotOptions { public static final String SKIP_FLUSH = "skipFlush"; public static final String TTL = "ttl"; + + // AWS S3 "Safe characters" for object keys: + // 0-9 a-z A-Z ! - _ . * ' ( ) + // See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines + // Hyphen is placed last in the character class, so it stays literal and never becomes a range operator. + private static final Pattern SAFE_SNAPSHOT_NAME = Pattern.compile("[a-zA-Z0-9!_.*'()-]+"); public final SnapshotType type; public final String tag; public final DurationSpec.IntSecondsBound ttl; @@ -88,7 +97,7 @@ public static SnapshotOptions userSnapshot(String tag, Map optio return builder.build(); } - public String getSnapshotName(Instant creationTime) + public static String getSnapshotName(SnapshotType type, String tag, Instant creationTime) { // Diagnostic snapshots have very specific naming convention hence we are keeping it. // Repair snapshots rely on snapshots having name of their repair session ids @@ -189,10 +198,58 @@ public Builder rateLimiter(RateLimiter rateLimiter) } public SnapshotOptions build() + { + validateTag(tag); + validateTTL(ephemeral, ttl); + + if (rateLimiter == null) + rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter(); + + return new SnapshotOptions(this); + } + + private void validateTag(String tag) { if (tag == null || tag.isEmpty()) throw new RuntimeException("You must supply a snapshot name."); + // Pre-generate snapshot name for the sake of the validation. + // getSnapshotName logic does not return raw "tag" as snapshot name every time, + // it e.g. prepends timestamp and type for system snapshots, and we need to validate it as a whole. + // If, for example, tag would be less than max allowed FILENAME_LENGTH, + // we might in fact produce a snapshot name longer than FILENAME_LENGTH if we prepended a timestamp to it. + String resolvedSnapshotname = SnapshotOptions.getSnapshotName(type, tag, Instant.now()); + + // the length of valid snapshot name has to be less than or equal to FILENAME_LEGTH - that is 255 - + // we are following the max length as it is in SchemaConstants for table name. + if (resolvedSnapshotname.length() > SchemaConstants.FILENAME_LENGTH) + { + throw new IllegalArgumentException(format("Snapshot name must not be more than %d characters long for " + + "resolved snapshot name (got %d characters for \"%s\")", + FILENAME_LENGTH, resolvedSnapshotname.length(), resolvedSnapshotname)); + } + + // Allowed characters follow the AWS S3 "Safe characters" set documented at + // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines : + // 0-9 a-z A-Z ! - _ . * ' ( ) + // The path separator '/' is intentionally excluded, + // which is what blocks traversal attempts such as "../../mysnapshot" + if (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotname).matches()) + { + throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotname); + } + + // Because '.' is part of the safe set, the names "." and ".." would pass the charset check + // but resolve to the snapshots/ directory itself or its parent (the live table directory) + // when used as a path component. Reject them explicitly. + if (resolvedSnapshotname.equals(".") || resolvedSnapshotname.equals("..")) + { + throw new IllegalArgumentException("Snapshot name '" + resolvedSnapshotname + "' is reserved"); + } + } + + private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl) + { if (ttl != null) { int minAllowedTtlSecs = CassandraRelevantProperties.SNAPSHOT_MIN_ALLOWED_TTL_SECONDS.getInt(); @@ -202,11 +259,6 @@ public SnapshotOptions build() if (ephemeral && ttl != null) throw new IllegalStateException(format("can not take ephemeral snapshot (%s) while ttl is specified too", tag)); - - if (rateLimiter == null) - rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter(); - - return new SnapshotOptions(this); } } diff --git a/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java b/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java index 236a3ab783aa..2c69309d0b79 100644 --- a/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java +++ b/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java @@ -88,7 +88,7 @@ public Map getSnapshotsToCreate() else creationTime = options.creationTime; - snapshotName = options.getSnapshotName(creationTime); + snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, creationTime); Set entitiesForSnapshot = options.cfs == null ? parseEntitiesForSnapshot(options.entities) : Set.of(options.cfs); diff --git a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java index 30b94d32b1c5..4ad201c14f15 100644 --- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java +++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java @@ -26,15 +26,81 @@ import org.junit.Test; +import org.apache.cassandra.io.util.File; + import static java.lang.String.format; +import static org.apache.cassandra.service.snapshot.SnapshotType.USER; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.Assert.assertEquals; public class SnapshotOptionsTest { + @Test + public void testSnapshotNameValidation() + { + // Previously-allowed alphanumerics, '-' and '_' must still be accepted. + validate("atag", USER); + validate("a-tag", USER); + validate("a_tag", USER); + validate("a_tag" + Instant.now().toEpochMilli(), USER); + validate("a_tag_1and_something2-more", USER); + validate("a".repeat(255), USER); + + // AWS S3 "Safe characters" newly accepted by the relaxed allowlist: + // ! . * ' ( ) + // See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines + validate("snap.2026-05-20", USER); + validate("important!", USER); + validate("backup*", USER); + validate("o'snap", USER); + validate("snap(1)", USER); + validate("!._-*'()", USER); + // Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory. + validate("a..tag", USER); + + assertThatThrownBy(() -> validate("a".repeat(256), USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name must not be more than 255 characters long for " + + "resolved snapshot name (got 256 characters for \"" + "a".repeat(256) + "\")"); + + // this would append timestamp + type which would violate < 255 when tag is 250 chars long only + assertThatThrownBy(() -> validate("a".repeat(250), SnapshotType.UPGRADE)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Snapshot name must not be more than 255 characters long"); + + // '/' is not in the S3-safe set; this is what kills traversal attempts like "../../mysnapshot". + assertThatThrownBy(() -> validate('a' + File.pathSeparator() + "tag", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a/tag"); + + // Other characters outside the S3-safe set must still be rejected. + assertThatThrownBy(() -> validate("a tag", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a tag"); + assertThatThrownBy(() -> validate("a:tag", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a:tag"); + + // "." and ".." pass the charset check but resolve to the snapshots/ dir itself + // and its parent (the live table dir) respectively, so they must be rejected as reserved. + assertThatThrownBy(() -> validate(".", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '.' is reserved"); + + assertThatThrownBy(() -> validate("..", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '..' is reserved"); + } + + private void validate(String tag, SnapshotType type) + { + new SnapshotOptions.Builder(tag, type, s -> true, "ks.tb").rateLimiter(RateLimiter.create(1)).build(); + } + @Test public void testSnapshotName() { - List sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, SnapshotType.USER); + List sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, USER); for (SnapshotType type : sameNameTypes) { @@ -42,7 +108,7 @@ public void testSnapshotName() .rateLimiter(RateLimiter.create(5)) .build(); - String snapshotName = options.getSnapshotName(Instant.now()); + String snapshotName = SnapshotOptions.getSnapshotName(type, options.tag, Instant.now()); assertEquals("a_name", snapshotName); } @@ -57,7 +123,7 @@ public void testSnapshotName() Instant now = Instant.now(); - String snapshotName = options.getSnapshotName(now); + String snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, now); assertEquals(format("%d-%s-%s", now.toEpochMilli(), type.label, "a_name"), snapshotName); }