dlmarion commented on issue #88:
URL: https://github.com/apache/accumulo-access/issues/88#issuecomment-3643844342
Looking at the 2.1 Accumulo code, I don't think my concerns about issues on
upgrade are valid. Based on the information below, the allowed characters in
the Accumulo 2.1 Authorizations and ColumnVisibility are limited to `a-z`,
`A-Z`, `0-9`, `_`, `-`, `:`, `.`, and `/`.
1. In 2.1, there is **no** validation that I can find on Authorizations
set on a User via the client API
2. In 2.1, there **is** validation when creating a new ColumnVisibility
that the Authorizations is a valid auth character (See uses of
[Authorizations.isValidAuthChar(c)](https://github.com/apache/accumulo/blob/2.1/core/src/main/java/org/apache/accumulo/core/security/Authorizations.java#L90).
3. The
[ColumnVisibility(byte[])](https://github.com/apache/accumulo/blob/2.1/core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java#L504)
ctor is called from the system
[VisibilityFilter](https://github.com/apache/accumulo/blob/2.1/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/system/VisibilityFilter.java#L83),
which ends up calling `ColumnVisibility.validate`, which ends up calling
`Authorizations.isValidAuthChar`. This check ensures that data ingested by the
various mechanisms still have a ColumnVisibility using the set of allowed
characters.
Looking at the 4.0 code:
1. The valid set of Authorizations chars are still there, and still used
in the ColumnVisibilityParser, but that is only called if the user calls
`ColumnVisibility.flatten()`, or `ColumnVisibility.getParseTree()`. I think
this is a **bug** that will throw an Exception when parse is called. Either the
Authorizations list of valid chars needs to be removed and we need to enforce
valid Authorization characters in the ColumnVisibility constructor.
2. The
[VisibilityFilter](https://github.com/apache/accumulo/blob/main/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/system/VisibilityFilter.java#L87)
passes the ColumnVisiblity bytes to the AccessEvaluatorImpl.canAccess method
which parses the AccessExpression.
With my concerns about existing systems squashed, then I think we are at the
following place:
1. I think we have some consensus to change the SPECIFICATION such that
only valid UTF-8 characters can be used, even within quotes
2. This means that any AccessExpression created using this library will
validate compliance with the specification. This means that
ParserEvaluator.parseAccessExpression will need to change.
3. We likely have some work to do still in Accumulo 4.0. We need to fix
the bug with ColumnVisibilityParser and we may want to add some validation when
Authorizations are added to a user.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
