[
https://issues.apache.org/jira/browse/HADOOP-11335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14281352#comment-14281352
]
Dian Fu commented on HADOOP-11335:
----------------------------------
Hi [~asuresh], thanks a lot for your review and comments.
{quote}
1) JavaKeyStoreProvider:
* createKey() : I think it might be a bit odd that we have metadata with
version == 0
{quote}
OK, have updated the patch and version will no longer be equal to 0.
{quote}
* 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
{quote}
As we may store ACL in metadata and ACL shouldn't be deleted when the key is
deleted.
{quote}
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.
{quote}
Agree. Have moved {{KeyOpType}} to {{KeyProvider}}.
{quote}
* 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)
{quote}
Agree. Have removed the setters of Metadata.
{quote}
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
{quote}
The lock in {{JavaKeyStoreProvider}} is only used for key related operations,
such as {{createKey}}, {{deleteKey}}, etc. As the new added methods such as
{{createKeyAcl}}, {{deleteKeyAcl}} also need to read/write keystore and these
methods only exist in {{KeyProviderAuthorizationExtension}}, not in
{{KeyProvider}}. So we add lock in {{KeyProviderExtension}} and the lock in
{{KeyProviderAuthorizationExtension}} is inherited from
{{KeyProviderExtension}}.
{quote}
5) MetadataKeyAuthorizer
* Remove commented code
{quote}
Removed the commented code in latest patch.
{quote}
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}
{quote}
The method you suggested is a good method. I have tried to use this method but
I found there are some problems. For example, currently we create
{{KeyProviderAuthorizationExtension}} by wrapping a {{KeyProvider}} in it and
this {{KeyProvider}} should be created first. Then when we create
{{KeyProvider}}, we should know the type for {{Metadata}}, for example whether
it's {{Metadata}} or {{MetadataWithAcl}}. This is a little weird as whether ACL
is stored in Metadata should be controlled in
{{KeyProviderAuthorizationExtension}}.
I choose to solve this issue by modifying {{Metadata}} and it currently has two
elements {{MetadataForKey}} and {{MetadataForAcl}}. You can refer to the latest
patch (revision 003) for detailed implementation.
> 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,
> HADOOP-11335.003.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)
