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

[Martin Kosek]

On 06/16/2014 09:17 AM, Jan Cholasta wrote:
> On 13.6.2014 21:59, Nathaniel McCallum wrote:
>> On Wed, 2014-06-11 at 12:43 -0400, Nathaniel McCallum wrote:
>>> On Wed, 2014-06-11 at 12:12 +0200, Ludwig Krispenz wrote:
>>>> 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 ?
>>>>>
>>>
>>> Ludwig,
>>>
>>> objectClasses:  (2.16.840.1.113730.3.8.16.2.1  NAME 'ipaToken' SUP top
>>> ABSTRACT DESC 'Abstract token class for tokens' MUST (ipatokenUniqueID)
>>> MAY (description $ ipatokenOwner $ ipatokenDisabled $ ipatokenNotBefore
>>> $ ipatokenNotAfter $ ipatokenVendor $ ipatokenModel $ ipatokenSerial)
>>> X-ORIGIN 'IPA OTP')
>>>
>>> objectClasses:  (2.16.840.1.113730.3.8.16.2.2  NAME 'ipatokenTOTP' SUP
>>> ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $
>>> ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $
>>> ipatokenTOTPtimeStep) X-ORIGIN 'IPA OTP')
>>>
>>> objectClasses:  (2.16.840.1.113730.3.8.16.2.5  NAME 'ipatokenHOTP' SUP
>>> ipaToken STRUCTURAL DESC 'HOTP Token Type' MUST (ipatokenOTPkey $
>>> ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenHOTPcounter) X-ORIGIN
>>> 'IPA OTP')
>>
>> Ludwig / Jan,
>>
>> I'd like to propose that we move ahead and merge this patch since the
>> only outstanding item is the question of performance for the ACIs.
>>
>> Nathaniel
>>
> 
> I'm fine with that.
> 

Ok, good. Pushed 0049a.2 to master: 98851256f94efe55b873f01aa46b2cdcda4a3efb

Martin