You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2019/05/03 11:51:01 UTC

[GitHub] [hadoop] hadoop-yetus commented on a change in pull request #664: [HADOOP-14951] Make the KMSACLs implementation customizable, with an …

hadoop-yetus commented on a change in pull request #664: [HADOOP-14951] Make the KMSACLs implementation customizable, with an …
URL: https://github.com/apache/hadoop/pull/664#discussion_r280745641
 
 

 ##########
 File path: hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSACLs.java
 ##########
 @@ -65,269 +66,105 @@ public String getBlacklistConfigKey() {
     }
   }
 
-  public static final String ACL_DEFAULT = AccessControlList.WILDCARD_ACL_VALUE;
-
-  public static final int RELOADER_SLEEP_MILLIS = 1000;
-
-  private volatile Map<Type, AccessControlList> acls;
-  private volatile Map<Type, AccessControlList> blacklistedAcls;
-  @VisibleForTesting
-  volatile Map<String, HashMap<KeyOpType, AccessControlList>> keyAcls;
-  @VisibleForTesting
-  volatile Map<KeyOpType, AccessControlList> defaultKeyAcls = new HashMap<>();
-  @VisibleForTesting
-  volatile Map<KeyOpType, AccessControlList> whitelistKeyAcls = new HashMap<>();
-  private ScheduledExecutorService executorService;
-  private long lastReload;
-
-  KMSACLs(Configuration conf) {
-    if (conf == null) {
-      conf = loadACLs();
-    }
-    setKMSACLs(conf);
-    setKeyACLs(conf);
-  }
-
-  public KMSACLs() {
-    this(null);
-  }
-
-  private void setKMSACLs(Configuration conf) {
-    Map<Type, AccessControlList> tempAcls = new HashMap<Type, AccessControlList>();
-    Map<Type, AccessControlList> tempBlacklist = new HashMap<Type, AccessControlList>();
-    for (Type aclType : Type.values()) {
-      String aclStr = conf.get(aclType.getAclConfigKey(), ACL_DEFAULT);
-      tempAcls.put(aclType, new AccessControlList(aclStr));
-      String blacklistStr = conf.get(aclType.getBlacklistConfigKey());
-      if (blacklistStr != null) {
-        // Only add if blacklist is present
-        tempBlacklist.put(aclType, new AccessControlList(blacklistStr));
-        LOG.info("'{}' Blacklist '{}'", aclType, blacklistStr);
-      }
-      LOG.info("'{}' ACL '{}'", aclType, aclStr);
-    }
-    acls = tempAcls;
-    blacklistedAcls = tempBlacklist;
-  }
-
-  @VisibleForTesting
-  void setKeyACLs(Configuration conf) {
-    Map<String, HashMap<KeyOpType, AccessControlList>> tempKeyAcls =
-        new HashMap<String, HashMap<KeyOpType,AccessControlList>>();
-    Map<String, String> allKeyACLS =
-        conf.getValByRegex(KMSConfiguration.KEY_ACL_PREFIX_REGEX);
-    for (Map.Entry<String, String> keyAcl : allKeyACLS.entrySet()) {
-      String k = keyAcl.getKey();
-      // this should be of type "key.acl.<KEY_NAME>.<OP_TYPE>"
-      int keyNameStarts = KMSConfiguration.KEY_ACL_PREFIX.length();
-      int keyNameEnds = k.lastIndexOf(".");
-      if (keyNameStarts >= keyNameEnds) {
-        LOG.warn("Invalid key name '{}'", k);
-      } else {
-        String aclStr = keyAcl.getValue();
-        String keyName = k.substring(keyNameStarts, keyNameEnds);
-        String keyOp = k.substring(keyNameEnds + 1);
-        KeyOpType aclType = null;
-        try {
-          aclType = KeyOpType.valueOf(keyOp);
-        } catch (IllegalArgumentException e) {
-          LOG.warn("Invalid key Operation '{}'", keyOp);
-        }
-        if (aclType != null) {
-          // On the assumption this will be single threaded.. else we need to
-          // ConcurrentHashMap
-          HashMap<KeyOpType,AccessControlList> aclMap =
-              tempKeyAcls.get(keyName);
-          if (aclMap == null) {
-            aclMap = new HashMap<KeyOpType, AccessControlList>();
-            tempKeyAcls.put(keyName, aclMap);
-          }
-          aclMap.put(aclType, new AccessControlList(aclStr));
-          LOG.info("KEY_NAME '{}' KEY_OP '{}' ACL '{}'",
-              keyName, aclType, aclStr);
-        }
-      }
-    }
-    keyAcls = tempKeyAcls;
-
-    final Map<KeyOpType, AccessControlList> tempDefaults = new HashMap<>();
-    final Map<KeyOpType, AccessControlList> tempWhitelists = new HashMap<>();
-    for (KeyOpType keyOp : KeyOpType.values()) {
-      parseAclsWithPrefix(conf, KMSConfiguration.DEFAULT_KEY_ACL_PREFIX,
-          keyOp, tempDefaults);
-      parseAclsWithPrefix(conf, KMSConfiguration.WHITELIST_KEY_ACL_PREFIX,
-          keyOp, tempWhitelists);
-    }
-    defaultKeyAcls = tempDefaults;
-    whitelistKeyAcls = tempWhitelists;
-  }
+  /**
+   * First Check if user is in ACL for the KMS operation, if yes, then return
+   * true if user is not present in any configured blacklist for the operation.
+   * 
+   * @param keyOperationType
+   *          KMS Operation
+   * @param ugi
+   *          UserGroupInformation of user
+   * @param key
+   *          the key name
+   * @return true is user has access
+   */
+  public abstract boolean hasAccess(Type keyOperationType,
+      UserGroupInformation ugi, String key);
 
+  
   /**
-   * Parse the acls from configuration with the specified prefix. Currently
-   * only 2 possible prefixes: whitelist and default.
-   *
-   * @param conf The configuration.
-   * @param prefix The prefix.
-   * @param keyOp The key operation.
-   * @param results The collection of results to add to.
+   * This is called by the KeyProvider to check if the given user is
+   * authorized to perform the specified operation on the given acl name.
+   * @param aclName name of the key ACL
+   * @param ugi User's UserGroupInformation
+   * @param opType Operation Type 
+   * @return true if user has access to the aclName and opType else false
    */
-  private void parseAclsWithPrefix(final Configuration conf,
-      final String prefix, final KeyOpType keyOp,
-      Map<KeyOpType, AccessControlList> results) {
-    String confKey = prefix + keyOp;
-    String aclStr = conf.get(confKey);
-    if (aclStr != null) {
-      if (keyOp == KeyOpType.ALL) {
-        // Ignore All operation for default key and whitelist key acls
-        LOG.warn("Invalid KEY_OP '{}' for {}, ignoring", keyOp, prefix);
-      } else {
-        if (aclStr.equals("*")) {
-          LOG.info("{} for KEY_OP '{}' is set to '*'", prefix, keyOp);
-        }
-        results.put(keyOp, new AccessControlList(aclStr));
-      }
-    }
-  }
+  @Override
+  public abstract boolean hasAccessToKey(String aclName,
+      UserGroupInformation ugi, KeyOpType opType);
 
+  /**
+   * 
+   * @param aclName ACL name
+   * @param opType Operation Type
+   * @return true if AclName exists else false 
 
 Review comment:
   whitespace:end of line
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

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