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

[Ade Lee]

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.
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-vakwetu-0295-Realm-allow-auth-instances-to-support-multiple-realm.patch
Type: text/x-patch
Size: 12979 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20160419/3d5643cf/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-vakwetu-0294-Realms-Address-comments-from-review.patch
Type: text/x-patch
Size: 33867 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20160419/3d5643cf/attachment-0001.bin>