You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2026/09/24 21:52:12 UTC
[PR] Added property to restrict bulk import source file locations [accumulo]
dlmarion opened a new pull request, #6564:
URL: https://github.com/apache/accumulo/pull/6564
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added property to restrict bulk import source file locations [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on code in PR #6564:
URL: https://github.com/apache/accumulo/pull/6564#discussion_r4126617899
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -313,6 +313,12 @@ public enum Property {
+ " 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).*$",
+ PropertyType.STRING,
+ "A regular expressions where source bulk import files for the table may located."
Review Comment:
```suggestion
"A regular expression where source bulk import files for the table may be located."
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added property to restrict bulk import source file locations [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #6564:
URL: https://github.com/apache/accumulo/pull/6564#issuecomment-6346480867
ITs passed successfully
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added property to restrict bulk import source file locations [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6564:
URL: https://github.com/apache/accumulo/pull/6564#discussion_r4318691424
##########
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java:
##########
@@ -537,7 +538,21 @@ protected boolean canDeleteRange(TCredentials c, TableId tableId, String tableNa
protected boolean canBulkImport(TCredentials c, TableId tableId, String tableName, String dir,
String failDir, NamespaceId namespaceId) throws ThriftSecurityException {
authenticate(c);
- return hasTablePermission(c, tableId, namespaceId, TablePermission.BULK_IMPORT, false);
+ final boolean tablePerm =
+ hasTablePermission(c, tableId, namespaceId, TablePermission.BULK_IMPORT, false);
+ final String allowedLocations =
+ context.getTableConfiguration(tableId).get(Property.GENERAL_BULK_SOURCE_DIRS);
+ boolean locationAllowed = false;
+ if (allowedLocations == null || allowedLocations.isBlank()
+ || Pattern.matches(allowedLocations, dir)) {
+ locationAllowed = true;
+ }
Review Comment:
This makes it so the default is to allow. I don't think that's secure. The default should be to reject.
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -313,6 +313,12 @@ public enum Property {
+ " 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).*$",
Review 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:
```json
{}
```
* example 1:
```json
{"user1": "hdfs://path/to/user1/data/.*"}
```
* example 2:
```json
{"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`):
```json
{"(.*)": "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.
##########
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java:
##########
@@ -537,7 +538,21 @@ protected boolean canDeleteRange(TCredentials c, TableId tableId, String tableNa
protected boolean canBulkImport(TCredentials c, TableId tableId, String tableName, String dir,
String failDir, NamespaceId namespaceId) throws ThriftSecurityException {
authenticate(c);
- return hasTablePermission(c, tableId, namespaceId, TablePermission.BULK_IMPORT, false);
+ final boolean tablePerm =
+ hasTablePermission(c, tableId, namespaceId, TablePermission.BULK_IMPORT, false);
+ final String allowedLocations =
+ context.getTableConfiguration(tableId).get(Property.GENERAL_BULK_SOURCE_DIRS);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org