Refactor: move isFileForbiddenInConfigSets #4193
Refactor: move isFileForbiddenInConfigSets #4193dsmiley wants to merge 5 commits intoapache:mainfrom
Conversation
…anceUtils to ConfigSetService Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
…c block Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Needs review from a security standpoint, at least. Look at the original JIRA.
| private static final Set<String> USE_FORBIDDEN_FILE_TYPES; | ||
|
|
||
| static { | ||
| String userForbiddenFileTypes = EnvUtils.getProperty(FORBIDDEN_FILE_TYPES_PROP); | ||
| USE_FORBIDDEN_FILE_TYPES = | ||
| StrUtils.isNullOrEmpty(userForbiddenFileTypes) | ||
| ? DEFAULT_FORBIDDEN_FILE_TYPES | ||
| : Set.of(userForbiddenFileTypes.split(",")); | ||
| } |
There was a problem hiding this comment.
this is now much simpler -- no lazy initialization with double-checked locking. Wasn't necessary!
| public void uploadFileToConfig( | ||
| String configName, String fileName, byte[] data, boolean overwriteOnExists) | ||
| throws IOException { | ||
| if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(fileName)) { |
There was a problem hiding this comment.
this line (and really the very definition of that method in a ZK specific class) irritated the hell out of me, prompting me to do this PR
| if (children.size() == 0) { | ||
| // If we didn't copy data down, then we also didn't create the file. But we still need a | ||
| // marker on the local disk so create an empty file. | ||
| if (isFileForbiddenInConfigSets(zkPath)) { |
There was a problem hiding this comment.
I don't see why this was here. This method doesn't even say "configSet" in it, but lets pretend it did. So nonetheless, what risk does this stop? I think it's sufficient to prevent ConfigSetService from adding "forbidden" files to its storage (usually ZK). If such a file got there already, then it's either by design (sys-admin knows what they are doing) or if bad actors can get stuff into ZK then you've already lost.
| + " to ZooKeeper at " | ||
| + zkHost); | ||
| FileTypeMagicUtil.assertConfigSetFolderLegal(confPath); | ||
| Files.walkFileTree( |
There was a problem hiding this comment.
I like adding the validation earlier in the process!
Is there an opportunity to DRY up this block of code in this plus ZkConfigSetService?
There was a problem hiding this comment.
Come to think of it, I disagree with limiting what the CLI tool can upload on principle. This is a false protection -- if you can use the CLI tool (to touch zookeeper, then you already have permission to do so by your ability to do that very thing.
New behavior: uploading configs with forbidden files will now stop the configset from being created! Yay
There was a problem hiding this comment.
The test was oddly asserting an additional configSet being there. It turns out, this was the "_testconf" that was poisoned with the forbidden file. The previous behavior would leave a partial configset in place despite throwing an error but now the logic is pre-validation, thus we don't create the configset if it has forbidden stuff. An improvement!
The issue https://issues.apache.org/jira/browse/SOLR-16480 introduced
ZkMaintenanceUtils.isFileForbiddenInConfigSets. It's location poor as this is not a ZK specific matter, it's a ConfigSet matter. Moved to ConfigSetService.