[Freeipa-devel] [HELP] Regular users should not be able to add OTP tokens with custom name

[Martin Kosek]

On 10/13/2014 07:23 PM, Nathaniel McCallum wrote:
> On Mon, 2014-10-13 at 12:39 +0200, Martin Kosek wrote:
>> On 10/10/2014 05:43 PM, Nathaniel McCallum wrote:
>>> As a result of this ongoing conversation, I have opened two 389 bugs:
>>>
>>> 1. Post Read - https://fedorahosted.org/389/ticket/47924
>>> 2. UUID ACIs - https://fedorahosted.org/389/ticket/47925
>>>
>>> On Wed, 2014-10-08 at 17:46 -0400, Nathaniel McCallum wrote:
>>>> The background of this email is this bug:
>>>> https://fedorahosted.org/freeipa/ticket/4456
>>>>
>>>> Attached are two patches which solve this issue for admin users (not
>>>> very helpful, I know). They depend on this fix in 389:
>>>> https://fedorahosted.org/389/ticket/47920
>>>>
>>>> There are two outstanding issues:
>>>>
>>>> 1. 389 does not send the post read control for normal users. The
>>>> operation itself succeeds, but no control is sent.
>>>>
>>>> The relevant sections from the log are attached. 389 is denying access
>>>> to the following attributes (* = valid, ! = invalid):
>>>> ! objectClass
>>>> ! ipatokenOTPalgorithm
>>>> ! ipatokenOTPdigits
>>>> * ipatokenOTPkey
>>>> * ipatokenHOTPcounter
>>>> ! ipatokenOwner
>>>> ! managedBy
>>>> ! ipatokenUniqueID
>>>>
>>>> The ACIs allowing access to most of these attributes are here:
>>>> https://git.fedorahosted.org/cgit/freeipa.git/tree/install/share/default-aci.ldif#n90
>>>>
>>>> Note that I am able to query the entry just fine (including all the
>>>> above invalidly restricted attributes). Hence, I know the ACIs are
>>>> working just fine.
>>>>
>>>> Part of the strange thing is that in the post read control request, I
>>>> haven't indicated that I want *any* attributes returned (i.e. I want
>>>> just the DN). So I'm not sure why it is querying all the attributes. I
>>>> would suspect that the proper behavior would be to only check the ACIs
>>>> on attributes that will actually be returned.
>>>>
>>>> 2. The second patch (0002) modifies the ACI for normal user token
>>>> addition from this:
>>>>
>>>> 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";)
>>>>
>>>> To this:
>>>>
>>>> aci: (target = "ldap:///ipatokenuniqueid=autogenerate,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";)
>>>>
>>>> The idea is to allow users to create tokens which will be expanded by
>>>> the UUID plugin. Unfortunately, after the UUID is expanded, the ACIs are
>>>> checked. Since the expanded UUID no longer matches the ACI, the addition
>>>> is denied. Is this truly the correct behavior? I would think that the
>>>> ACIs would be checked before the UUID plugin, not after.
>>
>> Given the number of issues we have with this patch on the DS side, it is likely
>> we will need to go some simpler way for FreeIPA 4.1, in terms just of hiding
>> the option where appropriate.
> 
> We have solved the first DS issue via a sensible workaround. Attached
> are two new patches. These take into account your feedback below and
> incorporate the aforementioned workaround.
> 
> The only thing these patches lack is the tightening of the ACI (problem
> #2 in the first email). Merging these patches I believe provides
> benefits over our current code, even though we don't get the final
> benefit of forcing normal users to use autogeneration.
> 
> If we test/merge these now, then when the second DS problem is solved,
> we can simply update the ACI independently via a trivial second patch
> (s/\*/autogenerate/).

Ok. We can merge the patches if they at least improve the situation and prepare
us for the future.

>> Also, few comments to your current patch set (though the patches themselves
>> will probably not land in 4.1):
>>
>> Patch 0001:
>> - while it may work (after DS fixes), it adds PostRead for all our commands,
>> not just otptoken-add. I would rather have some attribute like
>> "read_dn_from_postread" on LDAPObject which defaults to False to make sure
>> there is no regression or performance hit with other LDAP calls.
> 
> In the new code, we actually get a performance gain as the manual post
> read is eliminated if the post read control is successful. Only one
> issue can arise, which is when the post read control evaluates ACIs in a
> different context than a normal manual read. This problem is well known
> and is trivial to fix (s/USERDN/SELFDN/).

That's my point - with such a big change, we can hit many unforeseen issues and
we are close to deadline. I would rather like to use the PostRead control only
in otptoken_add command. CCing Petr and Honza to chime in.

>> - Wider adoption of the PostRead call would be left for future, when we stop
>> doing post-execute LDAP read call and only rely on PostRead result.
> 
> This is already integrated.
> 
>> Patch 0002:
>> - "cn=IPA Token Unique IDs,cn=IPA UUID,cn=plugins,cn=config" should be created
>> in LDAP update files only. Currently it will not be created on upgrades.
> 
> Fixed.

I would recommend testing your patches before you send them. Just based on
reading the update file change I know it won't work - try yourselves :-)

> I believe the following patches are ready for review. Should we review
> on this thread? Or should I create separate threads for the patches?

This thread is fine. You will also need to bump the Requires in our spec file,
to get the fixed DS version.

Martin