[Pki-devel] [PATCH] 285 - 293 Patches for fine grained authz in the KRA

Endi Sukma Dewata edewata at redhat.com
Mon Apr 18 23:28:31 UTC 2016


On 4/18/2016 12:09 PM, Ade Lee wrote:
> As promised, wiki documentation for this feature provided below:
>
> http://pki.fedoraproject.org/wiki/Kra_authz_realm
>
> Ade
>
> On Sat, 2016-04-16 at 17:24 -0400, Ade Lee wrote:
>> This is the main series of patches that implements fine grained
>> authorization in the KRA as described in :
>>
>>   https://pagure.io/test_dogtag_designs/pull-request/5
>>
>> I'll be moving this design to the wiki and adding some additional
>> documentation and test scripts shortly.
>>
>> More to come including :
>> 1. authz for the modify method in the Key service.
>> 2. new VLV indexes
>> 3. database migration script
>> 4. Man page updates
>> 5. Python unit tests for the Python CLI changes
>>
>> Please review,
>>
>> Thanks,
>> Ade

Here are some initial questions/comments (I have not tested the code):

1. According to the design agent1 is not in barbican realm but it can 
create secrets in that realm. So agent1 is like a non-agent user in 
barbican realm, but right now we don't really have a regular user role 
in KRA. Should we, at least for now, require realm membership to 
create/access the secrets in the realm?

2. In the design could you specify which command generates which 
key/request ID? It would make it easier to understand the example.

3. To simplify the terminology, can we call the non-barbican realm the 
"default/common" realm? So all agents belong to the default realm and 
they can access common secrets.

4. Let's remove the "m" prefix for the newly added fields in 
ARequestRecord, KeyRecord, and BasicGroupAuthz.

5. The null assignments for BasicGroupAuthz's fields are redundant.

6. To help troubleshooting the exception in BasicGroupAuthz we should 
clarify why the access was denied.

   if (!group.isMember(user)) {
       throw new EAuthzAccessDenied("Access denied");
   }

7. As mentioned in #1, we probably should validate the ownership only 
after we validate the realm membership.

   // if record owner == requester, SUCCESS
   if ((owner != null) &&
      owner.equals(authToken.getInString(IAuthToken.USER_ID))) return;

8. This code is a bit risky since a typo will allow any agent to access 
the secrets in the realm.

   String mgrName = getAuthzManagerByRealm(realm);
   // if no authz manager for this realm, SUCCESS by default
   if (mgrName == null) return;

I think if realm is specified it must have a corresponding plugin.

9. In setRealm() in KeyArchivalRequest/KeyGenerationRequest we probably 
want to remove the attribute if realm is null.

10. With #9 it's no longer necessary to check if realm is null in KeyClient:

   if (realm != null) {
       data.setRealm(realm);
   }

11. The original exception should be chained to help troubleshooting:

   } catch (EDBRecordNotFoundException e) {
       throw new KeyNotFoundException(keyId);
   }

   } catch (EAuthzAccessDenied e) {
       throw new UnauthorizedException("Not authorized to get request");
   }

There are a few of these in some of the patches.

12. It's probably an existing code, but I think we can remove the 
try-catch block:

   IRequest request = null;
   try {
       request = queue.findRequest(new RequestId(requestId));
   } catch (EBaseException e) {
   }

I think the issues that require some consideration are #1, #7, an d#8. 
If we agree on those I'd ACK the patches. The others are minor.

-- 
Endi S. Dewata




More information about the Pki-devel mailing list