[
https://issues.apache.org/jira/browse/HADOOP-4348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12655426#action_12655426
]
Sanjay Radia commented on HADOOP-4348:
--------------------------------------
Went over your patch. Some feedback:
1) ConnectionHeader should have as javadoc since it is part of the protocol.
2) It is bizarre for ConnectionHeader to be configurable!!
Server side implementation details should not leak into protocol types.
I see why you have done it. Change getProtocol to return a string and
convert to the class name outside.
3) The Phantom "authorize" call.
3a) Add comments to the status enum:
In particular
FATAL(-1) // occurs if the connection authorization fails.
// Only occurs with callid of -1.
3b) You use call id of -1 for authorize call.
Define a constant for AUTHORIZE_CONNECTION_CALLID = -1
3c) Given that FATAL only occurs for specific speail Phantom calls,
it would be cleaner to for you to deduce that authorization failed
by writing your code as:
{code}
if (state == Status.FATAL.state) {
if (id == AUTHORIZE_CONNECTION_CALLID) {
// authorization failed
} else If (id == PROTOCOL_CHECK_CALLID ) {
// protocol does not match ( - see my Item-10 below)
.. .other phantom calls
}
{code}
3d) Somewhere in the connection establishment code or receiveResponse,
you need to add a comment that when connection is established,
a failed authorization check will respond with a call-response
for the Phantom callid of AUTHORIZE_CONNECTION_CALLID.
Consider the following:
authorization could be done as part of creating the proxy on the client side
and that the proxy creation should fail if authorization fails. In your code
there is no real AUTHORIZE_CONNECTION_CALL. Only a reply to the call when the
call fails. Doing a real AUTHORIZE_CONNECTION_CALL when proxy is created would
be conceptually cleaner and should be considered; its counter argument is that
it would add latency to the first call.
If we decide to do this perhaps it should be a separate jira.
4) Coding Style: ipc.server.java
Move the following variable to above readAndProcess()
-- it uses the authFailedCall variable declared *afterwards*:
Call authFailedCall = new Call(-1, null, null);
5) Following comment in code is incorrect.
{code}
/// Reads the connection header following version and
/// authorizes the connection
private void processHeader() throws IOException {
{code}
The authorization is not done in this processHeader(); it is done by the
callee after header is processed. Please fix comment.
(Actually it would be cleaner for processHeader() to do the authorization and
throw the
exception. If you decide to do this call it processHeaderAndAuthorize().
6. ipc.server.authorize()
- should this be an abstract method with no default implementation?
7. Add java doc for the params for the ipc.server.call()
8. You deprecated two call() methods in ipc.server.
Why not remove these deprecated methods since IPC/RPC is an hadoop
internal/private interface
9. Move your new method setupResponse to the Call class.
10. On server side check the class name (protocol) against any base class of
the server-instance when the connection header is received.
- if protocol does not match then reply FATAL and close the connection.
(see my Item-3c above)
11. On the IPC/RPC server side you eliminated the need for UGI by using
the JASS subject. This is good.
File jiras to similarly use JAAS in other parts of the code.
11a) File jira remove UGI on rpc/ipc client side (ie use JASS instead)
- None of the getProxy should have a UGI parameter
- the getProxy should get it via login
(perhaps only if if there is no thread subject ?? not sure)
11b) File jira for fixing the calls to UGI in our various servers (NN, DN, JT
etc)
- getCurrentUGI should be private, add public getCurrentUser() &
getCurrentGroup()
12. Add javadoc in securityUtil to say that it is for service level
authorization.
Or is it more general than that?.
Methods like authorize apply only to service level access not to file
or object level access. Hence method names should have the words that
indicate
that is related to "service level authorization".
13. service.java - what is the servicekey (is it serviceName?).
I don't understand what getPermission is? Is it the permission granted for the
service?
Your comment is a little confusing.
14. HDFSPolicyProvider and MRPolicyProvider.
14a) Add java doc which says
- this the default policy provider for the... and that these are
the only protocols that will be allowed access.
- the acl in the acl policy file will specify the list of users/groups
allowed access.
14b) I think we can do without these (you can do this in a new jira if you
like)
- add a parameter to getProxy( existing parameters, protocolList);
protocolList is an array (or a var args) that is a triple
{className, version#, protocolKey}
15) Move the doAs in securityUtil.authorize() to the IPC/RPC layer
(it belongs there and will match the other doAs in the ipc/rpc layer).
Hence you can remove the authorize() method from the securityUtil.
In the Item-14b jira when we change the rpc.getServer() to add the rpc
interfaces list,
move the setPolicy to inside the rpc layer. This will then simplify all
server code
that create rpc servers.
16) SecurityUtil.getSubject()
create the hash set with right number of entries (user plus number of groups)
- note this is called on every rpc, should be reasonably efficient.
Question - should this be a static method be on the UGI class instead?
17) You added a method to dfsAdmin called refreshAuthorizationPolicy().
Some points
- Refresh is about refreshing the policy file which contains the the acls;
correct?
-if so change the name of method to refreshServiceAcls or something like
that.
- also change the method RefreshAuthorizationPolicyProtocol.refresh() to
refreshServiceAcls()
- refresh is too general a name for method inside a mixin interface.
18) General comment: make sure the that all the classes/interfaces you added to
src/core have good javadoc so that folks know when and how to use them.
> Adding service-level authorization to Hadoop
> --------------------------------------------
>
> Key: HADOOP-4348
> URL: https://issues.apache.org/jira/browse/HADOOP-4348
> Project: Hadoop Core
> Issue Type: New Feature
> Components: security
> Reporter: Kan Zhang
> Assignee: Arun C Murthy
> Fix For: 0.20.0
>
> Attachments: HADOOP-4348_0_20081022.patch,
> HADOOP-4348_1_20081201.patch, HADOOP-4348_2_20081202.patch,
> HADOOP-4348_3_20081204.patch, HADOOP-4348_4_20081205.patch,
> HADOOP-4348_5_20081206.patch, HADOOP-4348_6_20081209.patch,
> jaas_service_v1.patch, jaas_service_v2.patch, jaas_service_v3.patch,
> ServiceLevelAuthorization.pdf, ServiceLevelAuthorization.pdf
>
>
> Service-level authorization is the initial checking done by a Hadoop service
> to find out if a connecting client is a pre-defined user of that service. If
> not, the connection or service request will be declined. This feature allows
> services to limit access to a clearly defined group of users. For example,
> service-level authorization allows "world-readable" files on a HDFS cluster
> to be readable only by the pre-defined users of that cluster, not by anyone
> who can connect to the cluster. It also allows a M/R cluster to define its
> group of users so that only those users can submit jobs to it.
> Here is an initial list of requirements I came up with.
> 1. Users of a cluster is defined by a flat list of usernames and groups.
> A client is a user of the cluster if and only if her username is listed in
> the flat list or one of her groups is explicitly listed in the flat list.
> Nested groups are not supported.
> 2. The flat list is stored in a conf file and pushed to every cluster
> node so that services can access them.
> 3. Services will monitor the modification of the conf file periodically
> (5 mins interval by default) and reload the list if needed.
> 4. Checking against the flat list is done as early as possible and before
> any other authorization checking. Both HDFS and M/R clusters will implement
> this feature.
> 5. This feature can be switched off and is off by default.
> I'm aware of interests in pulling user data from LDAP. For this JIRA, I
> suggest we implement it using a conf file. Additional data sources may be
> supported via new JIRA's.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
