[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #2784: HDFS-15850. Superuser actions should be reported to external enforcers

2021-03-30 Thread GitBox 


xiaoyuyao commented on a change in pull request #2784:
URL: https://github.com/apache/hadoop/pull/2784#discussion_r604564623



##
File path: hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
##
@@ -310,4 +310,10 @@
   
   
 
+

Review comment:
   can we revert the formatting change in findbugsExcludeFile.xml as the 
assert should fix the spotbugs issue? 




-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #2784: HDFS-15850. Superuser actions should be reported to external enforcers

2021-03-29 Thread GitBox 


xiaoyuyao commented on a change in pull request #2784:
URL: https://github.com/apache/hadoop/pull/2784#discussion_r603641136



##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##
@@ -2316,7 +2317,7 @@ boolean truncate(String src, long newLength, String 
clientName,
 removeBlocks(toRemoveBlocks);
 toRemoveBlocks.clear();
   }
-  logAuditEvent(true, operationName, src, null, r.getFileStatus());
+  logAuditEvent(true, operationName, src, null, status);

Review comment:
   I think you can just add a assert(r != null); before return at 2325. 
That should be good enough. 




-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #2784: HDFS-15850. Superuser actions should be reported to external enforcers

2021-03-23 Thread GitBox 


xiaoyuyao commented on a change in pull request #2784:
URL: https://github.com/apache/hadoop/pull/2784#discussion_r599677401



##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeAttributeProvider.java
##
@@ -405,6 +405,30 @@ default void 
checkPermissionWithContext(AuthorizationContext authzContext)
   + "implement the checkPermissionWithContext(AuthorizationContext) "
   + "API.");
 }
+

Review comment:
   FSPermissionChecker#isSuperUser() is called in 
ContentSummaryComputationContext#checkPermission(), should we add a 
FSPermissionChecker#checkSuperuserPrivilege for auditing?




-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #2784: HDFS-15850. Superuser actions should be reported to external enforcers

2021-03-23 Thread GitBox 


xiaoyuyao commented on a change in pull request #2784:
URL: https://github.com/apache/hadoop/pull/2784#discussion_r599661862



##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##
@@ -7664,8 +7669,9 @@ void addCachePool(CachePoolInfo req, boolean 
logRetryCache)
 final String operationName = "addCachePool";
 checkOperation(OperationCategory.WRITE);
 String poolInfoStr = null;
+String poolName = req == null ? null : req.getPoolName();

Review comment:
   Line 7678 can reuse the poolName here. 




-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #2784: HDFS-15850. Superuser actions should be reported to external enforcers

2021-03-22 Thread GitBox 


xiaoyuyao commented on a change in pull request #2784:
URL: https://github.com/apache/hadoop/pull/2784#discussion_r599041257



##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##
@@ -7665,7 +7670,7 @@ void addCachePool(CachePoolInfo req, boolean 
logRetryCache)
 checkOperation(OperationCategory.WRITE);
 String poolInfoStr = null;
 try {
-  checkSuperuserPrivilege();
+  checkSuperuserPrivilege(operationName);

Review comment:
   Can we get it from other calls after Line 7670?
   String poolNameStr = "{poolName: " +
   (req == null ? null : req.getPoolName()) + "}";




-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #2784: HDFS-15850. Superuser actions should be reported to external enforcers

2021-03-19 Thread GitBox 


xiaoyuyao commented on a change in pull request #2784:
URL: https://github.com/apache/hadoop/pull/2784#discussion_r597962113



##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirAttrOp.java
##
@@ -82,15 +82,24 @@ static FileStatus setOwner(
 fsd.writeLock();
 try {
   iip = fsd.resolvePath(pc, src, DirOp.WRITE);
+  // Only the owner or super user can change the group or owner
   fsd.checkOwner(pc, iip);
-  if (!pc.isSuperUser()) {
-if (username != null && !pc.getUser().equals(username)) {
-  throw new AccessControlException("User " + pc.getUser()
-  + " is not a super user (non-super user cannot change owner).");
-}
-if (group != null && !pc.isMemberOfGroup(group)) {
-  throw new AccessControlException(
-  "User " + pc.getUser() + " does not belong to " + group);
+  // Only a super user can change ownership to a different user

Review comment:
   Can you rephrase the comments to make it clear what super user can do 
and what a non-super user owner can do w.r.t. SetOwner operation:
   superuser: change owner to a different user, change owner group to any group
   owner: can't change owner to a different user but can change owner group to 
different group that the user belongs to.

##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java
##
@@ -377,9 +377,10 @@ private void printDatanodeReplicaStatus(Block block,
*/
   public void fsck() throws AccessControlException {
 final long startTime = Time.monotonicNow();
+String operationName = "fsck";
 try {
   if(blockIds != null) {
-namenode.getNamesystem().checkSuperuserPrivilege();
+namenode.getNamesystem().checkSuperuserPrivilege(operationName);

Review comment:
   can we pass the path parameter to add more context for better auditing?

##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##
@@ -8869,15 +8870,19 @@ private ECTopologyVerifierResult 
getEcTopologyVerifierResultForEnabledPolicies()
 Arrays.asList(enabledEcPolicies));
   }
 
-  // This method logs operatoinName without super user privilege.
+  // This method logs operationName without super user privilege.
   // It should be called without holding FSN lock.
-  void checkSuperuserPrivilege(String operationName)
+  void checkSuperuserPrivilege(String operationName, String path)
   throws IOException {
-try {
-  checkSuperuserPrivilege();
-} catch (AccessControlException ace) {
-  logAuditEvent(false, operationName, null);
-  throw ace;
+if (isPermissionEnabled) {
+  try {
+FSPermissionChecker.setOperationType(operationName);
+FSPermissionChecker pc = getPermissionChecker();
+pc.checkSuperuserPrivilege(path);
+  } catch(AccessControlException ace){
+logAuditEvent(false, operationName, null);

Review comment:
   I know the existing code pass null here. Chould we include the path here 
instead of passing null? 

##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeAttributeProvider.java
##
@@ -405,6 +405,30 @@ default void 
checkPermissionWithContext(AuthorizationContext authzContext)
   + "implement the checkPermissionWithContext(AuthorizationContext) "
   + "API.");
 }
+
+/**
+ * Checks if the user belongs to superuser group.

Review comment:
   Can we rephrase "Checks if the user belongs to superuser group." to 
"Checks if the user is a superuser or belongs to superuser group."

##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##
@@ -7986,9 +7990,7 @@ EncryptionZone getEZForPath(final String srcArg)
 final String operationName = "listEncryptionZones";
 boolean success = false;
 checkOperation(OperationCategory.READ);
-final FSPermissionChecker pc = getPermissionChecker();
-FSPermissionChecker.setOperationType(operationName);
-checkSuperuserPrivilege(pc);
+checkSuperuserPrivilege(operationName);

Review comment:
   Can we add path info as the context for better auditing?

##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##
@@ -7692,7 +7697,7 @@ void modifyCachePool(CachePoolInfo req, boolean 
logRetryCache)
 String poolNameStr = "{poolName: " +
 (req == null ? null : req.getPoolName()) + "}";
 try {
-  checkSuperuserPrivilege();
+  checkSuperuserPrivilege(operationName);

Review comment:
   same as above.

##
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java