[Freeipa-devel] [PATCH] krb 1.12's OTP-Over-RADIUS

Fri May 3 14:04:56 UTC 2013

On 05/01/2013 03:33 PM, Nathaniel McCallum wrote:
> Below is my first stab at ACLs. They don't actually work right, but I'm not sure what I've done wrong. The basic gist is that nobody gets any permissions by default. Admins get full permissions and users get limited permissions for their own tokens. Any help would be appreciated.

We have an ACI allowing read access to all attributes or trees that were not
forbidden:

aci: (target != "ldap:///idnsname=*,cn=dns,dc=idm,dc=lab,dc=bos,dc=redhat,dc=c
 om")(targetattr != "userPassword || krbPrincipalKey || sambaLMPassword || sam
 baNTPassword || passwordHistory || krbMKey || userPKCS12 || ipaNTHash || ipaN
 TTrustAuthOutgoing || ipaNTTrustAuthIncoming")(version 3.0; acl "Enable Anony
 mous access"; allow (read, search, compare) userdn = "ldap:///anyone";)

If you want to hide some attributes from regular users and only allow them to
be read by admins, you need to extend targetattr list. This can be done in
ipaserver/install/plugins/update_anonymous_aci.py.

> 
> Nathaniel
> 
> dn: $SUFFIX
> changetype: modify
> add: aci
> aci: (targetattrs = "ipatokenRadiusConfigLink || ipatokenRadiusUserName")(version 3.0; acl "RADIUS user configuration is priviledged"; deny (all) userdn = "ldap:///all";)
> aci: (targetattrs = "ipatokenRadiusConfigLink || ipatokenRadiusUserName")(version 3.0; acl "Admins can manage RADIUS user configuration"; allow (all) groupdn="ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)

deny rule will override the allow rule so this won't allow admins to do
anything. Couldn't we just add ipatokenRadiusConfigLink and
ipatokenRadiusUserName to the global ACI blacklist above? Then you could delete
both ACIs. Admins read&write access is already allowed by this ACI:

aci: (targetattr != "userPassword || krbPrincipalKey || sambaLMPassword || sam
 baNTPassword || passwordHistory || krbMKey || krbPrincipalName || krbCanonica
 lName || krbUPEnabled || krbTicketPolicyReference || krbPrincipalExpiration |
 | krbPasswordExpiration || krbPwdPolicyReference || krbPrincipalType || krbPw
 dHistory || krbLastPwdChange || krbPrincipalAliases || krbExtraData || krbLas
 tSuccessfulAuth || krbLastFailedAuth || krbLoginFailedCount || ipaUniqueId ||
  memberOf || serverHostName || enrolledBy || ipaNTHash")(version 3.0; acl "Ad
 min can manage any entry"; allow (all) groupdn = "ldap:///cn=admins,cn=groups
 ,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com";)

> aci: (targetfilter = "(objectClass=ipatokenRadiusConfiguration)")(targetattrs = "*")(version 3.0; acl "RADIUS configuration is priviledged"; deny (all) userdn = "ldap:///all";)
> aci: (targetfilter = "(objectClass=ipatokenRadiusConfiguration)")(targetattrs = "*")(version 3.0; acl "Admins can manage RADIUS configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)

This won't work from the reasons above. Maybe we should add

(targetfilter != "(objectClass=ipatokenRadiusConfiguration)")

to the global ACI?

> aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs = "*")(version 3.0; acl "Token configuration is priviledged"; deny (all) userdn = "ldap:///all";)
> aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs = "*")(version 3.0; acl "Admins can manage token configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)

We would just update global ACI.

> aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs = "ipatokenUniqueID || description || ipatokenOwner || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Users can read/add basic token info"; allow (read, add, search, compare) userattr = "ipatokenOwner#USERDN";)

Looks ok.

> aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "*")(version 3.0; acl "TOTP Token configuration is priviledged"; deny (all) userdn = "ldap:///all";)
> aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "*")(version 3.0; acl "Admins can manage TOTP token configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)

We would just update global ACI.

> aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "ipatokenOTPkey || ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPclockOffset || ipatokenTOTPtimeStep")(version 3.0; acl "Users can add TOTP token secrets"; allow (add, search) userattr = "ipatokenOwner#USERDN";)

Looks ok.

Rob, Simo - does this proposal seams reasonable?

Martin

> -
> 
> ----- Original Message -----
> From: "Nathaniel McCallum" <npmccallum at redhat.com>
> To: "Rob Crittenden" <rcritten at redhat.com>
> Cc: freeipa-devel at redhat.com
> Sent: Tuesday, April 30, 2013 5:46:33 PM
> Subject: Re: [Freeipa-devel] [PATCH] krb 1.12's OTP-Over-RADIUS
> 
> On Fri, 2013-04-26 at 18:30 -0400, Rob Crittenden wrote:
>> Nathaniel McCallum wrote:
>>> On Fri, 2013-04-12 at 17:39 -0400, Nathaniel McCallum wrote:
>>>> On Fri, 2013-04-12 at 11:53 -0400, Nathaniel McCallum wrote:
>>>>> On Fri, 2013-04-12 at 11:34 -0400, Nathaniel McCallum wrote:
>>>>>> On Thu, 2013-04-11 at 14:48 -0400, Rob Crittenden wrote:
>>>>>>> Nathaniel McCallum wrote:
>>>>>>>> On Wed, 2013-04-10 at 15:35 -0400, Rob Crittenden wrote:
>>>>>>>>> I'm not sure how I'd test it if I got it built.
>>>>>>>>
>>>>>>>> I'm working on this. I hope to have a clear answer next week. Bear with
>>>>>>>> me...
>>>>>>>>
>>>>>>>>> Overall looks really good.
>>>>>>>>
>>>>>>>> I've split up the patch into multiple commits. I've also added .update
>>>>>>>> files and a patch for ipa-kdb to feed krb5 the right user string.
>>>>>>>>
>>>>>>>> https://github.com/npmccallum/freeipa/commits/otp
>>>>>>>>
>>>>>>>> Please take a look. I *think* I've got everything worked out so far with
>>>>>>>> the exception of bug numbers / urls. Should every patch have a separate
>>>>>>>> bug and a link to the design page?
>>>>>>>
>>>>>>> The ticket should go into every commit. I'd probably put the design link
>>>>>>> there too, just for completeness. Future bug fixes, et all aren't going
>>>>>>> to require the design page, but since these commits are all related to
>>>>>>> the initial feature it will be nice to have.
>>>>>>>
>>>>>>> You can have multiple patches on the same ticket/bug.
>>>>>>
>>>>>> https://github.com/npmccallum/freeipa/commits/otp
>>>>>>
>>>>>> All four commits now have bug numbers and design page links. I'm adding
>>>>>> the design page link to the tickets as we speak.
>>>>>>
>>>>>> Remaining issues (AFAICS):
>>>>>> 1. The daemon (ipa-otpd) runs as root and binds anonymously
>>>>>> 2. ipatokenRadiusSecret is readable by an anonymous bind
>>>>> 3. ipatokenT?OTP.* are readable by an anonymous bind
>>>>>
>>>>> In the case of both #2 and #3, only admins should have RW. ipa-otpd
>>>>> needs read access to ipatokenRadiusSecret. The DS bind plugin below (#2)
>>>>> needs read access to ipatokenT?OTP.*.
>>>>>
>>>>>> Outstanding pieces:
>>>>>> 1. CLI tool -- https://fedorahosted.org/freeipa/ticket/3368
>>>>>> 2. DS bind plugin -- https://fedorahosted.org/freeipa/ticket/3367
>>>>>> 3. UI -- https://fedorahosted.org/freeipa/ticket/3369
>>>>>> 4. Self Service UI -- https://fedorahosted.org/freeipa/ticket/3370
>>>>>>
>>>>>> #1 and #2 are within the scope of F19 and should hopefully land shortly
>>>>>> (in separate commits). #3 and #4 are probably $nextrelease.
>>>>>>
>>>>
>>>> FYI - Here is an RPM with all of the code so far:
>>>> http://koji.fedoraproject.org/koji/taskinfo?taskID=5247029
>>>
>>> Updated RPMs, containing the new 389DS bind plugin and build for F19,
>>> are here:
>>> http://koji.fedoraproject.org/koji/taskinfo?taskID=5270926
>>>
>>> Nathaniel
>>
>> BuildRequires needed for whatever provides krad.h
> 
> It is there already. I updated the spec file to have a hard dependency
> on krb5 >= 1.11.
> 
>> A bunch of the files declare functions in each other. Is it cleaner to 
>> put these into an include file? I'm gathering that this will always be 
>> self-contained, so maybe this is ok.
> 
> Most of the functions are only needed in one other file. I thought this
> was cleaner. I'm also open to suggestions on how to better split the
> files so there are less includes...
> 
>> In entry_attr_get_berval() is it worth pointing out that there is no 
>> need to free the value, or is that assumed because it uses 
>> slapi_value_get_berval()?
> 
> I would hope it is assumed by const.
> 
>> If we detect that there is clock drift should we log it? Will we ever 
>> try to report to the client (e.g. future enhancement)?
> 
> There are lots of clock related enhancements that should be low-hanging
> fruit.
> 
>> I wonder if the NSPR-version of some functions should be used since this 
>> is running inside 389-ds, like PL_strcasecmp for strcasecmp()
> 
> I did not fix this since strcasecmp() is available everywhere.
> 
>> ops.c:
>>
>> pedantic: lack of spacing between if and parens
> 
> Fixed.
> 
>> sha384 is an allowed type only in otp.c. Is that needed?
> 
> The RFC doesn't specify sha384 support. However, adding support for it
> is somewhat obvious. In the spirit of "being liberal in what you
> receive, strict in what you send" I added sha384 support in auth.c.
> 
> Nathaniel
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> 