ctubbsii commented on code in PR #2994:
URL: https://github.com/apache/accumulo/pull/2994#discussion_r988341731
##########
server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java:
##########
@@ -308,6 +307,11 @@ private Map<String,String> conf(TCredentials credentials,
AccumuloConfiguration
@Override
public Map<String,String> getConfiguration(TInfo tinfo, TCredentials
credentials,
ConfigurationType type) throws TException {
+ if (!(security.isSystemUser(credentials) ||
security.hasSystemPermission(credentials,
+ credentials.getPrincipal(), SystemPermission.SYSTEM))) {
Review Comment:
Right, so that's equivalent to comparing the user principal. The class name
is basically the equivalent to the system user name.
But, the system user also has a password, and the authenticate method call
was removed from line 296. We need to be very careful we're not bypassing the
checking of the password for any RPC request that just masquerades as the
system user, but doesn't actually authenticate as the system user.
The code highlighted here seems to only check the system user principal, and
proceeds regardless of whether it authenticates. What I can't tell without
taking a deeper look, is if the user has already been previously authenticated
before reaching this line. If it has in every case, then this is fine... but if
it hasn't, we still need to do that.
--
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]
