[Freeipa-devel] [PATCH 0049] Add support for protected tokens

[Jan Cholasta]

On 13.5.2014 19:12, Nathaniel McCallum wrote:
> On Tue, 2014-05-13 at 16:33 +0200, Jan Cholasta wrote:
>> On 12.5.2014 21:02, Nathaniel McCallum wrote:
>>> On Thu, 2014-05-08 at 13:51 -0400, Simo Sorce wrote:
>>>> On Thu, 2014-05-08 at 12:26 -0400, Nathaniel McCallum wrote:
>>>>> On Wed, 2014-05-07 at 11:17 -0400, Simo Sorce wrote:
>>>>>> On Wed, 2014-05-07 at 09:54 -0400, Dmitri Pal wrote:
>>>>>>> On 05/07/2014 09:05 AM, Nathaniel McCallum wrote:
>>>>>>>> On Wed, 2014-05-07 at 11:42 +0200, Jan Cholasta wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 6.5.2014 17:08, Nathaniel McCallum wrote:
>>>>>>>>>> On Tue, 2014-05-06 at 09:49 -0400, Nathaniel McCallum wrote:
>>>>>>>>>>> On Mon, 2014-05-05 at 12:42 -0400, Nathaniel McCallum wrote:
>>>>>>>>>>>> This also constitutes a rethinking of the token ACIs after the
>>>>>>>>>>>> introduction of SELFDN support.
>>>>>>>>>>>>
>>>>>>>>>>>> Admins, as before, have full access to all token permissions.
>>>>>>>>>>>>
>>>>>>>>>>>> Normal users have read/search/compare access to all of the non-secret
>>>>>>>>>>>> data for tokens assigned to them, whether protected or non-protected.
>>>>>>>>>>>> Users can add or delete non-protected tokens and modify most of their
>>>>>>>>>>>> metadata. However they cannot create, delete or modify protected tokens.
>>>>>>>>>>>> Regardless of whether the token is protected or not, users cannot change
>>>>>>>>>>>> a token's ownership or unique identity.
>>>>>>>>>>>>
>>>>>>>>>>>> In contrast, admins can create protected tokens. This protects the token
>>>>>>>>>>>> from deletion or modification when assigned to users. Additionally, when
>>>>>>>>>>>> a user account is deleted, the assigned non-protected tokens are deleted
>>>>>>>>>>>> but the protected tokens are merely orphaned. This permits the token to
>>>>>>>>>>>> be reassigned without having to recreate it. This last point is
>>>>>>>>>>>> particularly useful in the case of hardware tokens.
>>>>>>>>>>>>
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4228
>>>>>>>>>>>>
>>>>>>>>>>>> NOTE: This patch depends on my patch 0048.
>>>>>>>>>>> This new version makes ipatokenDisabled visible for token owners. It is
>>>>>>>>>>> also writable if the token is non-protected. This additionally fixes:
>>>>>>>>>>>
>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4259
>>>>>>>>>> This new version changes the way the default value of protected is setup
>>>>>>>>>> in accordance with the changes made for the review of my patch 0048.2.
>>>>>>>>>>
>>>>>>>>>> Nathaniel
>>>>>>>>> Is using the ipatokenprotected attribute the final design?
>>>>>>>> No. Alternate designs are welcome. The code is easy enough to modify.
>>>>>>>>
>>>>>>>>> I did not dig too deep into this, but I think it might be better to
>>>>>>>>> instead use the managedby attribute on a token to limit who can delete
>>>>>>>>> (or administer in other way) the token. On otptoken-add, managedby would
>>>>>>>>> be set to the "whoami" user DN, unless run with --protected, in which
>>>>>>>>> case managedby would be left empty. Then, when deleting a user, the
>>>>>>>>> token would be deleted only if the user manages the token.
>>>>>>>> It seems to me that the mechanics of this are roughly the same as
>>>>>>>> protected, just with a different syntax. The cost of this is more
>>>>>>>> complex ACIs. In particular, we'd have to use two userdn clauses (is
>>>>>>>> this possible?) instead of a simple filter. If there is a clear benefit,
>>>>>>>> we can justify the more obtuse syntax.
>>>>>>>
>>>>>>> We usually try not to create new attributes until it is fully justified.
>>>>>>> I would like Simo to chime in on this.
>>>>>>
>>>>>> I would also prefer to reuse existing attributes and mechanism if
>>>>>> possible and if it will not preclude future work.
>>>>>>
>>>>>> In this case, it "sounds" like managed-by has the appropriate meaning:
>>>>>> "who manages the token ?" -> myself, admin, other, none ?
>>>>>>
>>>>>> Nathaniel can you send 2 lines showing the difference in ACIs between
>>>>>> using managed-by vs a new attribute ?
>>>>>
>>>>> These are the ACIs using the protected mechanism:
>>>>>
>>>>> aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
>>>>> "objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
>>>>> ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
>>>>> || ipatokenSerial || ipatokenOwner || ipatokenProtected")(version 3.0;
>>>>> acl "Users can read basic token info"; allow (read, search, compare)
>>>>> userattr = "ipatokenOwner#USERDN";)
>>>>>
>>>>> aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs =
>>>>> "ipatokenOTPalgorithm || ipatokenOTPdigits ||
>>>>> ipatokenTOTPtimeStep")(version 3.0; acl "Users can see TOTP details";
>>>>> allow (read, search, compare) userattr = "ipatokenOwner#USERDN";)
>>>>>
>>>>> aci: (targetfilter = "(objectClass=ipatokenHOTP)")(targetattrs =
>>>>> "ipatokenOTPalgorithm || ipatokenOTPdigits")(version 3.0; acl "Users can
>>>>> see HOTP details"; allow (read, search, compare) userattr =
>>>>> "ipatokenOwner#USERDN";)
>>>>>
>>>>> aci: (targetfilter =
>>>>> "(&(objectClass=ipaToken)(!(ipatokenProtected=TRUE)))")(targetattrs =
>>>>> "description || ipatokenDisabled || ipatokenNotBefore ||
>>>>> ipatokenNotAfter || ipatokenVendor || ipatokenModel ||
>>>>> ipatokenSerial")(version 3.0; acl "Users can write basic token info";
>>>>> allow (write) userattr = "ipatokenOwner#USERDN";)
>>>>>
>>>>> aci: (target = "ldap:///ipatokenuniqueid=*,cn=otp,$SUFFIX")(targetfilter
>>>>> = "(&(objectClass=ipaToken)(!(ipatokenProtected=TRUE))))")(version 3.0;
>>>>> acl "Users can create and delete tokens"; allow (add, delete) userattr =
>>>>> "ipatokenOwner#SELFDN";)
>>>>>
>>>>> This is what they look like using managedBy (I have not tested this):
>>>>>
>>>>> aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
>>>>> "objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
>>>>> ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
>>>>> || ipatokenSerial || ipatokenOwner || ipatokenProtected")(version 3.0;
>>>>> acl "Users can read basic token info"; allow (read, search, compare)
>>>>> userattr = "ipatokenOwner#USERDN"; allow (read, search, compare)
>>>>> userattr = "managedBy#USERDN";)
>>>>>
>>>>> aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs =
>>>>> "ipatokenOTPalgorithm || ipatokenOTPdigits ||
>>>>> ipatokenTOTPtimeStep")(version 3.0; acl "Users can see TOTP details";
>>>>> allow (read, search, compare) userattr = "ipatokenOwner#USERDN"; allow
>>>>> (read, search, compare) userattr = "managedBy#USERDN";)
>>>>>
>>>>> aci: (targetfilter = "(objectClass=ipatokenHOTP)")(targetattrs =
>>>>> "ipatokenOTPalgorithm || ipatokenOTPdigits")(version 3.0; acl "Users can
>>>>> see HOTP details"; allow (read, search, compare) userattr =
>>>>> "ipatokenOwner#USERDN"; allow (read, search, compare) userattr =
>>>>> "managedBy#USERDN";)
>>>>>
>>>>> aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
>>>>> "description || ipatokenDisabled || ipatokenNotBefore ||
>>>>> ipatokenNotAfter || ipatokenVendor || ipatokenModel ||
>>>>> ipatokenSerial")(version 3.0; acl "Managers can write basic token info";
>>>>> allow (write) userattr = "managedBy#USERDN";)
>>>>>
>>>>> aci: (targetfilter = "(objectClass=ipaToken)")(version 3.0; acl
>>>>> "Managers can delete tokens"; allow (delete) userattr =
>>>>> "managedBy#USERDN";)
>>>>>
>>>>> aci: (target = "ldap:///ipatokenuniqueid=*,cn=otp,$SUFFIX")(targetfilter
>>>>> = "(objectClass=ipaToken)")(version 3.0; acl "Users can create
>>>>> self-managed tokens"; allow (add) userattr = "ipatokenOwner#SELFDN" and
>>>>> userattr = "managedBy#SELFDN";)
>>>>>
>>>>> In short:
>>>>> 1. Owner and manager get read, search and compare.
>>>>> 2. Manager gets write (to select attributes) and delete.
>>>>> 3. Users can create self-managed tokens for themselves only.
>>>>>
>>>>> The otptoken-add command should gain the following defaults:
>>>>> 1. The owner defaults to the user adding the token.
>>>>> 2. If owner == user adding token, managedBy defaults to owner.
>>>>> 3. Otherwise, managedBy defaults to None.
>>>>>
>>>>> This means that if neither owner nor managedBy are specified, the
>>>>> default is a self-owned, self-managed token. Likewise, if a different
>>>>> owner is specified, no manager is assumed.
>>>>>
>>>>> rcrit expresses worry that ipalib's ACI parser may not handle the above
>>>>> syntax. This will become clear during testing if we want this approach.
>>>>>
>>>>> Does this look sane?
>>>>
>>>> I am not entirely sure your ACI syntax is always right for the second
>>>> set. and perhaps we want to duplicate ACIs in some cases (once for owner
>>>> once for manager).
>>>>
>>>> I think the read ACIs do not need to list managedby ? Do we plan to have
>>>> a manager that is another regular user but not the owner nor an admin ?
>>>>
>>>> In any case I prefer the sytnax that uses managedby, as it has more
>>>> potential.
>>>
>>> Attached is a new version of the patch which implements the feature
>>> using managedBy instead of ipatokenProtected. One important thing needs
>>> to be said about this patch. I am not exposing managedBy in either the
>>> UI, the CLI or LDAP (ACI). Do we care about this? If yes, should I
>>> expose this similar to owner or as a relationship?
>>
>> I would expose it, as a relationship. (Note that ipatokenowner should
>> ideally be represented as a relationship too, but the framework does not
>> support 1-to-many relationships ATM.)
>
> So since this is a 1-to-many relationship we shouldn't expose it?
>
> Or should I do it like owner is currently done?

Do it like managedby is done in the host plugin (see 
"attribute_members", "relationships", etc.)

>
>>
>> Just curious, why are the ACIs divided like this:
>>
>>       aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
>> "objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
>> ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
>> || ipatokenSerial || ipatokenOwner")(version 3.0; acl "Users/managers
>> can read basic token info"; allow (read, search, compare) userattr =
>> "ipatokenOwner#USERDN" or userattr = "managedBy#USERDN";)
>>       aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs =
>> "ipatokenOTPalgorithm || ipatokenOTPdigits ||
>> ipatokenTOTPtimeStep")(version 3.0; acl "Users/managers can see TOTP
>> details"; allow (read, search, compare) userattr =
>> "ipatokenOwner#USERDN" or userattr = "managedBy#USERDN";)
>>       aci: (targetfilter = "(objectClass=ipatokenHOTP)")(targetattrs =
>> "ipatokenOTPalgorithm || ipatokenOTPdigits")(version 3.0; acl
>> "Users/managers can see HOTP details"; allow (read, search, compare)
>> userattr = "ipatokenOwner#USERDN" or userattr = "managedBy#USERDN";)
>>
>> IMHO you could make them less complex by dividing them like this:
>>
>>       aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
>> "objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
>> ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
>> || ipatokenSerial || ipatokenOwner || ipatokenOTPalgorithm ||
>> ipatokenOTPdigits || ipatokenTOTPtimeStep")(version 3.0; acl "Owner can
>> read token details"; allow (read, search, compare) userattr =
>> "ipatokenOwner#USERDN";)
>>       aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
>> "objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
>> ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
>> || ipatokenSerial || ipatokenOwner || ipatokenOTPalgorithm ||
>> ipatokenOTPdigits || ipatokenTOTPtimeStep")(version 3.0; acl "Managers
>> can read token details"; allow (read, search, compare) userattr =
>> "managedBy#USERDN";)
>
> The first set is organized by objectClass. The second by userattr. I
> have no strong opinion on this matter, though performance is probably a
> consideration. Do any DS guys want to chime in?

I would still like to know someone else's opinion on this, but if 
there's none, let's keep it your way.

>
>> Would it make sense to keep --protected as a flag in otptoken-add as a
>> shortcut for "entry_attrs['managedby'] = None"?
>
> I can't think of a use case for this. The only use case I *can* think of
> is an admin creating a non-protected token for a user.

OK.

>
>> Would it make sense to default managedby to the current bind DN in
>> otptoken-add, even if it's not a user DN? (Do we want to allow running
>> otptoken-add by hosts/services/other non-users?)
>
> No idea. Dmitri?

We can add this later if necessary.

>
>> Is orphaning a token when a user is deleted only if it is not managed by
>> any other users the intended behavior? It just seems sort of strange to
>> me, since it changes the token from unprotected to protected.
>
> I don't think that is the behavior. We orphan the token if the owner is
> not listed as a manager. If the owner is listed as a manager, we delete
> the token.
>
> Put another way, protected tokens are orphaned and unprotected tokens
> are deleted.
>
> If I didn't implement that, please point out my bug.

Sorry, my bad, your code is right. You can make it simpler, though:

     orphan = [x for x in token.get('managedby', []) if x == dn]

(The "len() == 0" check is not necessary and using list comprehensions 
makes the code more readable than using filter.)

>
> Nathaniel
>


-- 
Jan Cholasta