Re: [Pki-devel] [PATCH] 285 - 293 Patches for fine grained authz in the KRA
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