Repository navigation
Conversation
Co-authored-by: Amanda Villarreal <48224036+Amemeda@users.noreply.github.com>
| + " user-implementations of pluggable Accumulo features, such as the balancer" | ||
| + " or volume chooser.", | ||
| "2.0.0"), | ||
| GENERAL_BULK_SOURCE_DIRS("general.bulk.allowed.source.dir.pattern", "^(?!.*/accumulo/tables).*$", |
There was a problem hiding this comment.
I think a better default is ^$ (though, the ^ and $ are redundant here, since this is using match, which matches the entire string by default), because the pattern here won't protect against specifying all files that are in use by Accumulo (RFiles are specified by absolute path, so there's no requirement that existing files be in a directory that matches this negative pattern).
Also, even if an admin could configure a pattern to protect against specifying another user's table data, it won't protect against a user specifying another user's data that is outside of an Accumulo table. For that, I think we need per-user config. Can still use one property, though:
- default:
{}- example 1:
{"user1": "hdfs://path/to/user1/data/.*"}- example 2:
{"user1": "hdfs://path/to/user1/data/.*", "user2": "hdfs://path/to/user2/data/.*"}We could also allow regex usernames, with optional capture groups to use for injection into the pattern, so the bulk source directory pattern can be derived from the username, as in:
- example 3 (for paths like
/home/<user>/data):
{"(.*)": "hdfs://path/to/home/\\1/data/.*"} jshell> String userRegex = "^(?<group>\\w+)_(?<name>\\w+)$";
jshell> String allowedPattern = "hdfs://[^/]*/home/${name}/data/${group}/.*";
jshell> String derivedPattern = "admin_root".replaceAll(userRegex, allowedPattern);
derivedPattern ==> "hdfs://[^/]*/home/root/data/admin/.*"
However, that might be overkill.
The configuration should be validated to ensure a user isn't listed twice, or if regex in the username is supported, should be well-defined, to match on the first or last matching username pattern.
| final String allowedLocations = | ||
| context.getTableConfiguration(tableId).get(Property.GENERAL_BULK_SOURCE_DIRS); |
There was a problem hiding this comment.
General properties are not per-table. Should read from the system config instead of the table config. The end result is the same, but the code will be less confusing.
| if (allowedLocations == null || allowedLocations.isBlank() | ||
| || Pattern.matches(allowedLocations, dir)) { | ||
| locationAllowed = true; | ||
| } |
There was a problem hiding this comment.
This makes it so the default is to allow. I don't think that's secure. The default should be to reject.
|
ITs passed successfully |
No description provided.