9:47 p.m.
Some comments inline, although most of this was discussed on #irc.
I have added two additional patches which are to be applied on top
of 258=293.
294: This patch fixes the problems identified in this review. In
particular:
Review comments addressed:
1. when archiving or generating keys, realm is checked
2. when no plugin is found for a realm, access is denied.
3. rename mFoo to foo for new variables.
4. add chaining of exceptions
5. remove attributes from KeyArchivalRequest etc. when realm is
null
6. Add more detail to denial in BasicGroupAuthz
295 - Adds the ability for authz plugins to support multiple realms.
In particular, the authorize() command has been extended to allow
the realm to be passed in, and the ACL plugins have been modified
to account for the realm.
Please review,
Thanks,
Ade
On Mon, 2016-04-18 at 18:28 -0500, Endi Sukma Dewata wrote:
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?
Done - page updated. This actually makes things simpler. As things
were before, it was possible to create a secret - and then not be able
to list it if you were not part of the realm. You could retrieve it by
ID (because you owned it), but not list it.
With the above requirement in place, this confusing scenario no longer
exists.
2. In the design could you specify which command generates which
key/request ID? It would make it easier to understand the example.
Done
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.
Done
5. The null assignments for BasicGroupAuthz's fields are
redundant.
Done
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");
}
Added debug statement
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;
Incorrect - as we discussed.
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.
Done
9. In setRealm() in KeyArchivalRequest/KeyGenerationRequest we
probably
want to remove the attribute if realm is null.
Done
10. With #9 it's no longer necessary to check if realm is null
in
KeyClient:
if (realm != null) {
data.setRealm(realm);
}
Done
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.
Done
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.