Skip to content

Added property to restrict bulk import source file locations - #6564

Open
dlmarion wants to merge 6 commits into
apache:2.1from
dlmarion:bulk-import-allowed-dir-prop
Open

dlmarion wants to merge 6 commits into
apache:2.1from
dlmarion:bulk-import-allowed-dir-prop

Conversation

@dlmarion

Copy link
Copy Markdown
Contributor

No description provided.

@dlmarion dlmarion added this to the 2.1.7 milestone Sep 24, 2026
@dlmarion dlmarion self-assigned this Sep 24, 2026
Comment thread core/src/main/java/org/apache/accumulo/core/conf/Property.java Outdated
Co-authored-by: Amanda Villarreal <48224036+Amemeda@users.noreply.github.com>
@dlmarion
dlmarion requested a review from ctubbsii October 15, 2026 15:32
+ " 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).*$",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +543 to +544
final String allowedLocations =
context.getTableConfiguration(tableId).get(Property.GENERAL_BULK_SOURCE_DIRS);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +546 to +549
if (allowedLocations == null || allowedLocations.isBlank()
|| Pattern.matches(allowedLocations, dir)) {
locationAllowed = true;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it so the default is to allow. I don't think that's secure. The default should be to reject.

@dlmarion
dlmarion requested a review from ctubbsii October 20, 2026 18:27
@dlmarion

Copy link
Copy Markdown
Contributor Author

ITs passed successfully

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants