Harden snapshot names on server side

Author: smiklosovic

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,8 +34,10 @@
 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
 {
@@ -88,7 +91,7 @@ public static SnapshotOptions userSnapshot(String tag, Map<String, String> 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 +192,47 @@ 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));
+            }
+
+            // "-" is important, we are already generating system snapshots with "-" in them so this has to stay
+            // \\w are just "words" consisting of [a-zA-Z0-9_]
+            if (!Pattern.compile("[\\w-]+").matcher(resolvedSnapshotname).matches())
+            {
+                throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotname);
+            }
+        }
+
+        private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl)
+        {
             if (ttl != null)
             {
                 int minAllowedTtlSecs = CassandraRelevantProperties.SNAPSHOT_MIN_ALLOWED_TTL_SECONDS.getInt();
@@ -202,12 +242,8 @@ 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);
         }
+
     }
 
     @Override

src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java

@@ -88,7 +88,7 @@ public Map<ColumnFamilyStore, TableSnapshot> getSnapshotsToCreate()
         else
             creationTime = options.creationTime;
 
-        snapshotName = options.getSnapshotName(creationTime);
+        snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, creationTime);
 
         Set<ColumnFamilyStore> entitiesForSnapshot = options.cfs == null ? parseEntitiesForSnapshot(options.entities) : Set.of(options.cfs);
 

test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java

@@ -26,23 +26,61 @@
 
 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()
+    {
+        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);
+
+        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");
+
+        assertThatThrownBy(() -> validate('a' + File.pathSeparator() + "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");
+    }
+
+    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<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, SnapshotType.USER);
+        List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, USER);
 
         for (SnapshotType type : sameNameTypes)
         {
             SnapshotOptions options = SnapshotOptions.systemSnapshot("a_name", type, "ks.tb")
                                                      .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 +95,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);
         }