[ 
https://issues.apache.org/jira/browse/HADOOP-11335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14280013#comment-14280013
 ] 

Arun Suresh commented on HADOOP-11335:
--------------------------------------

Thanks for working this [~dian.fu]

I have taken an first pass at the latest patch.. my initial comments follow 
(will send out more comments once I do further reviews).

1) JavaKeyStoreProvider:
   * createKey() : I think it might be a bit odd that we have metadata with 
version == 0 
   * deleteKey() : Am sorry, I might be missing something but, I did not quite 
understand why we can't just delete the metadata from the cache when the key is 
deleted

2) KeyProvider:
   * The Metadata Class now has a dependency on {{KeyOpType}} which is defined 
in {{KeyProviderAuthorizationExtension}}. The Extension classes were meant to 
add functionality to a KeyProvider. It seems a bit weird that the KeyProvider 
class should have a dependency on an Extension.
   * Not very confortable adding setters to Metadata. I Guess the original 
implementaion conciously made a choice to not allow modification of metadata 
once it is created (except for the version)

3) KeyProviderExtension:
   * Do we really need the read and write locks ? The undelying KeyProvider 
should take care of the synchronization. (for eg. {{JavaKeyStoreProvider}}) 
does infact use write and read locks for createKey etc.. this would probably 
lead to unnecessary double locking. 

4) KeyProviderAuthorizationExtension:
   * Same as above.. do we really need the read and write locks ? I feel the 
Extension class should handle its own concurrency semantics

5) MetadataKeyAuthorizer
   * Remove commented code

Looking at the commented code in {{MetadataKeyAuthorizer}}, I see that you had 
initially toyed with having an extended {{MetadataWithACL}} class. Any reason 
why you did not pursue that design ? It seems to me like that could have been a 
way to probably avoid having to modify the {{JavaKeyStoreProvider}} and 
{{KeyProvider}}. One suggestion would have been to templatized {{KeyProvider}} 
like so :
{noformat}
  public class KeyProvider<M extends Metadata>
  ...
{noformat}
and have different implemenetation of a {{KeyProvider}} like :
{noformat}
  public classs KeyProviderWithACls extends KeyProvider<MetadataWithACLs>
  ...  
{noformat}  

> KMS ACL in meta data or database
> --------------------------------
>
>                 Key: HADOOP-11335
>                 URL: https://issues.apache.org/jira/browse/HADOOP-11335
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: kms
>    Affects Versions: 2.6.0
>            Reporter: Jerry Chen
>            Assignee: Dian Fu
>              Labels: Security
>         Attachments: HADOOP-11335.001.patch, HADOOP-11335.002.patch, KMS ACL 
> in metadata or database.pdf
>
>   Original Estimate: 504h
>  Remaining Estimate: 504h
>
> Currently Hadoop KMS has implemented ACL for keys and the per key ACL are 
> stored in the configuration file kms-acls.xml.
> The management of ACL in configuration file would not be easy in enterprise 
> usage and it is put difficulties for backup and recovery.
> It is ideal to store the ACL for keys in the key meta data similar to what 
> file system ACL does.  In this way, the backup and recovery that works on 
> keys should work for ACL for keys too.
> On the other hand, with the ACL in meta data, the ACL of each key can be 
> easily manipulate with API or command line tool and take effect instantly.  
> This is very important for enterprise level access control management.  This 
> feature can be addressed by separate JIRA. While with the configuration file, 
> these would be hard to provide.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to