[Freeipa-devel] [PATCH 0049] Add support for protected tokens
Ludwig Krispenz
lkrispen at redhat.com
Wed Jun 11 09:55:05 UTC 2014
On 06/11/2014 11:32 AM, Jan Cholasta wrote:
> On 6.6.2014 19:04, Nathaniel McCallum wrote:
>> On Thu, 2014-06-05 at 08:45 +0200, Jan Cholasta wrote:
>>> On 28.5.2014 22:44, Nathaniel McCallum wrote:
>>>> On Mon, 2014-05-26 at 16:57 +0200, Jan Cholasta wrote:
>>>>> 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.)
>>>>
>>>> The attached version fixes all the above issues. Two issues that may
>>>> remain:
>>>> 1. There is no option to set managedBy during otptoken-add or
>>>> otptoken-mod. This is probably okay.
>>>
>>> Yes. I guess this bit is not needed anymore:
>>>
>>> # If owner was not specified, default to the person adding
>>> this token.
>>> - if 'ipatokenowner' not in entry_attrs:
>>> + # If managedby was not specified, attempt a sensible default.
>>> + if 'ipatokenowner' not in entry_attrs or 'managedby' not in
>>> entry_attrs:
>>> result =
>>> self.api.Command.user_find(whoami=True)['result']
>>> if result:
>>> cur_uid = result[0]['uid'][0]
>>> - entry_attrs.setdefault('ipatokenowner', cur_uid)
>>> + prev_uid = entry_attrs.setdefault('ipatokenowner',
>>> cur_uid)
>>> + if cur_uid == prev_uid:
>>> + entry_attrs.setdefault('managedby',
>>> result[0]['dn'])
>>
>> This code is still needed. Read the patch description. And, FYI,
>> 'managedby' is correct in this case, not 'managedby_user'. I tested.
>
> Right. I don't even know why I thought otherwise.
>
>>
>>>> 2. I can't figure out how to get the framework to actually show
>>>> managedBy in command output (like otptoken-show). This means you can
>>>> add/remove relationships using otptoken-add-managedby and
>>>> otptoken-remove-managedby, but you can't actually see the list of
>>>> managers. What am I missing?
>>>
>>> In the hbacrule or selinuxusermap plugins it is done by adding an
>>> "invisible" param to the object plugin, like this:
>>>
>>> Str('managedby_user?',
>>> label=_('Manager'),
>>> flags=['no_create', 'no_update', 'no_search'],
>>> ),
>>
>> Done.
>>
>>>>
>>>> Also, it would be helpful if someone with DS expertise could answer
>>>> the
>>>> question about the performance of the ACI structure options as listed
>>>> above.
>>
>> We still need this. Ludwig?
>
> +1
sorry, I had missed this, look into it now
>
>>
>>> You should update the code in user_del to use managedby_user instead of
>>> managedby, otherwise it won't really work.
>>
>> Done.
>>
>
> Thanks, ACK after the ACI issue is dealt with.
>
More information about the Freeipa-devel
mailing list