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

Nathaniel McCallum npmccallum at redhat.com
Mon Oct 13 17:23:27 UTC 2014


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/).

> 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/).

> - 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 believe the following patches are ready for review. Should we review
on this thread? Or should I create separate threads for the patches?

Nathaniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Use-post-read-control-on-add-operations.patch
Type: text/x-patch
Size: 5071 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141013/b13ac8a1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Use-UUID-plugin-to-generate-ipaTokenUniqueIDs.patch
Type: text/x-patch
Size: 10781 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141013/b13ac8a1/attachment-0001.bin>


More information about the Freeipa-devel mailing list