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

[Ludwig Krispenz]

On 05/13/2014 04:33 PM, 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.)
>
>
> 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";)
do you mean aci: (targetfilter = 
"(|(objectClass=ipaToken)(objectClass=ipatokenTOTP)(objectClass=ipatokenHOTP))") 
or are the attrs like ipatokenOTPdigits also in the ipatoken objectclass ?
>
>
> Would it make sense to keep --protected as a flag in otptoken-add as a 
> shortcut for "entry_attrs['managedby'] = None"?
>
>
> 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?)
>
>
> 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.
>