CASSANDRA-21389 trunk Harden snapshot names on server side

[smiklosovic]

Thanks for sending a pull request! Here are some tips if you're new here:

• Ensure you have added or run the appropriate tests for your PR.
• Be sure to keep the PR description updated to reflect all changes.
• Write your PR title to summarize what this PR proposes.
• If possible, provide a concise example to reproduce the issue for a faster review.
• Read our contributor guidelines
• If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

[Jollyplum]

Had some minor nits but apart from that LGTM

[Jollyplum]

Part of me is wondering if we allow folks to configure the check off, maybe it may also make sense to allow them to configure what to restrict on?
Then if they encounter some character that they do actually validly need or can't change for some reason, operators can still benefit from some restrictions.
Debatable though maybe adds more configuration/reasoning complexity where we just want simplicity.

[smiklosovic]

we are complicating it too much imho with allowing them to do that, I think that is just premature as of now, they have a way to just completely remove the validation, that is good enough imho

[Jollyplum]

I think we may need to reject both the native path separator and '/' since on windows '\' is the path separator but '/' is still treated as a path separator, so in theory the path traversal attack remains on windows via that means.

[Jollyplum]

Actually potentially this is fine it seems we've remove windows support as of:
https://issues.apache.org/jira/browse/CASSANDRA-16171
https://issues.apache.org/jira/browse/CASSANDRA-16956

[smiklosovic]

we do not support Windows

[frankgh]

From the Alex's shallow/deep review, it seems we need to include + in the charset for system tagged snapshots

The UPGRADE snapshot tag is built from version strings:

  // SystemKeyspace.snapshotOnVersionChange()
  format("%s-%s", previous, next)   // e.g. "6.0.1-7.0.0"

  Cassandra version strings follow a SemVer-ish grammar that allows build metadata, and the separator for that metadata is a literal +:

  // CassandraVersion.VERSION_REGEXP
  "...(-(?<prerelease>[-.\\w]+))?([.+](?<build>[.\\w]+))?"
  //                                ^^^^  '.' or '+' introduces build metadata

  // CassandraVersion.toString()
  if (build != null)
      sb.append('+').append(StringUtils.join(build, "."));   // emits e.g. "7.0.0+githash"

  So a version like 7.0.0+abc123 is a perfectly legal Cassandra version. The charset regex is:

  [a-zA-Z0-9_.-]      // letters, digits, _ . -

  + is not in that set. So if the persisted previous-version or the running version carries build metadata, the resolved UPGRADE tag contains +, fails the charset check, throws, and — because this runs during boot and the throw is converted to
  exitOrFail — the node won't start.

  (In a stock ASF build the version is just 7.0 / 7.0-SNAPSHOT, no +, so it doesn't bite. It bites on vendor/custom builds, -Dcassandra.releaseVersion=... with metadata, or a previously-installed binary that had a + version.)

[frankgh]

one option is to only validate the snapshot names when the input is SnapshotType.USER

[frankgh]

I played a little more with this PR, and I think we should add checks in listsnapshots and clearsnapshot

./bin/nodetool listsnapshots -k keyspace1 -t standard1 -n "aa/../bb"    
./bin/nodetool clearsnapshot keyspace1 -t "aa/../bb"

While this has no effect at the moment, and I couldn't clear other snapshots, I think it's a good idea to fail with the validation.

Also, I think we should explicitly tell users which characters are allowed when creating the snapshot. Here's the sample output from the create snapshot command

./bin/nodetool snapshot keyspace1 -cf standard1 -t "a@a"
Requested creating snapshot(s) for [keyspace1] with snapshot name [a@a] and options {skipFlush=false}
error: Snapshot name contains illegal characters: a@a
-- StackTrace --
java.lang.IllegalArgumentException: Snapshot name contains illegal characters: a@a

We should instead say
error: Snapshot name contains illegal characters: a@a. Allowed characters match the pattern [a-zA-Z0-9_.-]+ with a maximum of length of 255 characters

or something along those lines

[smiklosovic]

@frankgh no objections with more refined message with allowed chars and length but I am not sure about list/clear snapshot validation. It is overkill. In 6.0+, because it is on manager, we are literally checking snapshot name against user's argument. If it does not match then nothing is done. So it is irrelevant that a user wants to clear "../../abc", it will just filter snapshots with such a name - and since it is not possible to create a snapshot with a name like that this kind of a predicate always yields false, so nothing happens.

I have pushed a commit called "Francisco review" with fixes. Please check again.

[frankgh]

+1 looks good to me