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.

Thanks, Pushed to master. On Wed, 2016-04-20 at 15:23 -0500, Endi Sukma Dewata wrote:

On 4/19/2016 9:47 PM, Ade Lee wrote:

...

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,

ACK.