Rob Crittenden wrote:
Simo Sorce wrote:
On Fri, 2013-05-03 at 01:27 -0400, Nathaniel McCallum wrote:
All issues fixed unless noted below... The attached patches are tested
to work.
On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote:
- (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
(although I know slapi_ch_malloc() currently just aborts on failure, I
think that is plain wrong which is why I would prefer using
malloc/strdup, but well, I guess this is not something I am going to
care too much about for now).
Not fixed.
- Is the logic with auth_type_used correct ?
At least the name of the variable sounds misleading, because you set it
only if the authentication was successful but not set if it was 'used'
but was unsuccessful. Made me look at it multiple times to reconstuct
the logic. The var name could be better, however I also want a comment
that explain exactly how we are going to manage authentication and
fallbacks here as that logic is critical and is useful for anyone that
is going to have to change this code later in order not to break it.
The variable is now gone. I re-factored the section to make the logic
clearer and put a nice big comment up top.
- General question: how does this PRE_BIND plugin interact with
ipapwd_pre_bind() in the ipa-pwd-extop() plugin ?
Are you going to cause that plugin not to run by returning a result
directly in this function ?
Or is this plugin configured to run only after the previous one went
through ?
I ask because I do not see any ordering information in the cn=config
plugin configuration so it is not immediately clear to me.
That is a good question for Nathan since he wrote this part of the
code...
Continuing with otp.c:
- what does 'egress' mean ?
(can you just use 'done' as a standard label for exceptions ?)
Egress:
Noun - The action of going out of or leaving a place: "direct means of
access and egress".
Verb - Go out of or leave (a place).
In short: ingress means to enter and egress means to exit.
I have changed all 'egress' labels to 'done'.
- Is it ok to call PK11_DestroyContext() if ctx is NULL ?
Can't find much documentation but see #276314 / #276311 on
bugzilla.mozilla.org
I added if's for all of these just to be defensive.
- Can you please add a comment that describe which HMAC algorithm are
you using (or a reference to a RFC of it ?) Unfortunately NSS makes
thins a lot more cryptic than it should :(
Also adding comments before the various NSS invocation to explain what
they do would help, this code is obscure.
Unfortunately, that codes is mostly copy/paste from an NSS example of
how to do HMAC. It is just as unclear to me as it is to you. I have
added a link to the example with a note about me not understanding how
it works...
The good news is that it passes all the unit tests which use values
defined in the RFC. Also, valgrind reports no leaks or other errors. So
even if I don't know *how* it works, I do know that it does, in fact,
work.
Ok I took a deper look and now understand what it is doing.
I think it is implementing RFC 6234 HMAC, but can't say for sure.
The first NSS call creates a key container, the second initializes the
context and tells NSS which HMAC algorythm to use and what is the key.
Then the 3 calls simply (1) start the hmac calculation, (2) add in the
data to be signed and (3) extracts the final signature on the data to be
returned.
- Why do we have ipa_otp_hotp if we implement only totp ?
TOTP is a specialized case of HOTP. It is simply an alternative
mechanism for calculating the counter input to HOTP. Note that
ipa_otp_totp() is basically a one-liner. Since you *have* to implement
HOTP to get TOTP, you might as well expose the HOTP implementation for
future use.
Yeah I've seen that, looks a bit weird but makes perfect sense.
I do not have any more concerns on patch 1, so it's an ACK from me for
that one.
I haven't yet gone through the whole companion daemon patch :/
The otp ACIs one I think is wrong though, so still no full ack on the
whole patchset.
Simo.
This patch should fix things up.
rob
The ACIs to let a user manage their own OTP needed some tweaking. It was
loading in the wrong place for new installs and in both cases it lacked
read access to objectclass so nothing was actually being granted.
rob
>From 614e46cbce0672fe55ab23e1e95ef712a4749db4 Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Fri, 3 May 2013 15:27:13 -0400
Subject: [PATCH] Fix access control for OTP
---
install/share/default-aci.ldif | 10 ++++++++-
install/share/otp.ldif | 14 ------------
install/updates/40-otp.update | 11 ++-------
ipaserver/install/plugins/update_anonymous_aci.py | 27 ++++++++++++++++-------
4 files changed, 30 insertions(+), 32 deletions(-)
diff --git a/install/share/default-aci.ldif b/install/share/default-aci.ldif
index f173f79718e406e103cfe96026da041701382f7a..18881ece4c4ccd97701a1e5bf23459915b3194c4 100644
--- a/install/share/default-aci.ldif
+++ b/install/share/default-aci.ldif
@@ -3,7 +3,7 @@
dn: $SUFFIX
changetype: modify
add: aci
-aci: (target != "ldap:///idnsname=*,cn=dns,$SUFFIX";)(targetattr != "userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory || krbMKey || userPKCS12 || ipaNTHash || ipaNTTrustAuthOutgoing || ipaNTTrustAuthIncoming")(version 3.0; acl "Enable Anonymous access"; allow (read, search, compare) userdn = "ldap:///anyone";;)
+aci: (targetfilter = "(&(!(objectClass=ipaToken))(!(objectClass=ipatokenTOTP))(!(objectClass=ipatokenRadiusProxyUser))(!(objectClass=ipatokenRadiusConfiguration)))")(target != "ldap:///idnsname=*,cn=dns,$SUFFIX";)(targetattr != "userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory || krbMKey || userPKCS12 || ipaNTHash || ipaNTTrustAuthOutgoing || ipaNTTrustAuthIncoming")(version 3.0; acl "Enable Anonymous access"; allow (read, search, compare) userdn = "ldap:///anyone";;)
aci: (targetattr = "memberOf || memberHost || memberUser")(version 3.0; acl "No anonymous access to member information"; deny (read,search,compare) userdn != "ldap:///all";;)
aci: (targetattr != "userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory || krbMKey || krbPrincipalName || krbCanonicalName || krbUPEnabled || krbTicketPolicyReference || krbPrincipalExpiration || krbPasswordExpiration || krbPwdPolicyReference || krbPrincipalType || krbPwdHistory || krbLastPwdChange || krbPrincipalAliases || krbExtraData || krbLastSuccessfulAuth || krbLastFailedAuth || krbLoginFailedCount || ipaUniqueId || memberOf || serverHostName || enrolledBy || ipaNTHash")(version 3.0; acl "Admin can manage any entry"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";;)
aci: (targetattr = "userpassword || krbprincipalkey || sambalmpassword || sambantpassword")(version 3.0; acl "selfservice:Self can write own password"; allow (write) userdn="ldap:///self";;)
@@ -96,3 +96,11 @@ dn: cn=ipa,cn=etc,$SUFFIX
changetype: modify
add: aci
aci: (target="ldap:///cn=*,cn=ca_renewal,cn=ipa,cn=etc,$SUFFIX";)(targetattr="userCertificate")(version 3.0; acl "Modify CA Certificates for renewals"; allow(write) userdn = "ldap:///fqdn=$FQDN,cn=computers,cn=accounts,$SUFFIX";;)
+
+# Let users manage their own tokens
+dn: $SUFFIX
+changetype: modify
+add: aci
+aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs = "objectclass || ipatokenUniqueID || description || ipatokenOwner || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Users can read basic token info"; allow (read, search, compare) userattr = "ipatokenOwner#USERDN";)
+aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs = "ipatokenUniqueID || description || ipatokenOwner || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Users can write basic token info"; allow (write) userattr = "ipatokenOwner#USERDN";)
+aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "ipatokenOTPkey || ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPclockOffset || ipatokenTOTPtimeStep")(version 3.0; acl "Users can add TOTP token secrets"; allow (write, search) userattr = "ipatokenOwner#USERDN";)
diff --git a/install/share/otp.ldif b/install/share/otp.ldif
index 57f0d6b82139b3c2862ab48412bfe571adfcb386..aee10501bc8a2f469af06822493f680c27883b4b 100644
--- a/install/share/otp.ldif
+++ b/install/share/otp.ldif
@@ -20,17 +20,3 @@ nsslapd-pluginVendor: Red Hat, Inc.
nsslapd-pluginDescription: IPA OTP plugin
nsslapd-plugin-depends-on-type: database
nsslapd-basedn: $SUFFIX
-
-dn: $SUFFIX
-changetype: modify
-add: aci
-aci: (targetattrs = "ipatokenRadiusConfigLink || ipatokenRadiusUserName")(version 3.0; acl "RADIUS user configuration is priviledged"; deny (read,search,compare) 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";;)
-aci: (targetfilter = "(objectClass=ipatokenRadiusConfiguration)")(targetattrs = "cn || ipatokenRadiusServer || ipatokenRadiusSecret || description || ipatokenRadiusTimeout || ipatokenRadiusRetries || ipatokenUserMapAttribute")(version 3.0; acl "RADIUS configuration is priviledged"; deny (read,search,compare) userdn = "ldap:///all";;)
-aci: (targetfilter = "(objectClass=ipatokenRadiusConfiguration)")(targetattrs = "cn || ipatokenRadiusServer || ipatokenRadiusSecret || description || ipatokenRadiusTimeout || ipatokenRadiusRetries || ipatokenUserMapAttribute")(version 3.0; acl "Admins can manage RADIUS configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";;)
-aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs = "ipatokenUniqueID || description || ipatokenOwner || ipatokenDisabled || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Token configuration is priviledged"; deny (read,search,compare) userdn = "ldap:///all";;)
-aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs = "ipatokenUniqueID || description || ipatokenOwner || ipatokenDisabled || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Admins can manage token configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";;)
-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, write, search, compare) userattr = "ipatokenOwner#USERDN";)
-aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "ipatokenOTPkey || ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPclockOffset || ipatokenTOTPtimeStep")(version 3.0; acl "TOTP Token configuration is priviledged"; deny (read,search,compare) userdn = "ldap:///all";;)
-aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "ipatokenOTPkey || ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPclockOffset || ipatokenTOTPtimeStep")(version 3.0; acl "Admins can manage TOTP token configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";;)
-aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "ipatokenOTPkey || ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPclockOffset || ipatokenTOTPtimeStep")(version 3.0; acl "Users can add TOTP token secrets"; allow (write, search) userattr = "ipatokenOwner#USERDN";)
diff --git a/install/updates/40-otp.update b/install/updates/40-otp.update
index 78c286578fc80909332cb7a2aaae5a6b62358eb6..ff36c87a60c071efc3e2aaee59747635a2477740 100644
--- a/install/updates/40-otp.update
+++ b/install/updates/40-otp.update
@@ -4,13 +4,6 @@ default: objectClass: top
default: cn: otp
dn: $SUFFIX
-add: aci:'(targetattrs = "ipatokenRadiusConfigLink || ipatokenRadiusUserName")(version 3.0; acl "RADIUS user configuration is priviledged"; deny (read,search,compare) userdn = "ldap:///all";;)'
-add: 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";;)'
-add: aci:'(targetfilter = "(objectClass=ipatokenRadiusConfiguration)")(targetattrs = "cn || ipatokenRadiusServer || ipatokenRadiusSecret || description || ipatokenRadiusTimeout || ipatokenRadiusRetries || ipatokenUserMapAttribute")(version 3.0; acl "RADIUS configuration is priviledged"; deny (read,search,compare) userdn = "ldap:///all";;)'
-add: aci:'(targetfilter = "(objectClass=ipatokenRadiusConfiguration)")(targetattrs = "cn || ipatokenRadiusServer || ipatokenRadiusSecret || description || ipatokenRadiusTimeout || ipatokenRadiusRetries || ipatokenUserMapAttribute")(version 3.0; acl "Admins can manage RADIUS configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";;)'
-add: aci:'(targetfilter = "(objectClass=ipaToken)")(targetattrs = "ipatokenUniqueID || description || ipatokenOwner || ipatokenDisabled || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Token configuration is priviledged"; deny (read,search,compare) userdn = "ldap:///all";;)'
-add: aci:'(targetfilter = "(objectClass=ipaToken)")(targetattrs = "ipatokenUniqueID || description || ipatokenOwner || ipatokenDisabled || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Admins can manage token configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";;)'
-add: 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, write, search, compare) userattr = "ipatokenOwner#USERDN";)'
-add: aci:'(targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "ipatokenOTPkey || ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPclockOffset || ipatokenTOTPtimeStep")(version 3.0; acl "TOTP Token configuration is priviledged"; deny (read,search,compare) userdn = "ldap:///all";;)'
-add: aci:'(targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "ipatokenOTPkey || ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPclockOffset || ipatokenTOTPtimeStep")(version 3.0; acl "Admins can manage TOTP token configuration"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";;)'
+add: aci:'(targetfilter = "(objectClass=ipaToken)")(targetattrs = "objectclass || ipatokenUniqueID || description || ipatokenOwner || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Users can read basic token info"; allow (read, search, compare) userattr = "ipatokenOwner#USERDN";)'
+add: aci:'(targetfilter = "(objectClass=ipaToken)")(targetattrs = "ipatokenUniqueID || description || ipatokenOwner || ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel || ipatokenSerial")(version 3.0; acl "Users can write basic token info"; allow (write) userattr = "ipatokenOwner#USERDN";)'
add: aci:'(targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs = "ipatokenOTPkey || ipatokenOTPalgorithm || ipatokenOTPdigits || ipatokenTOTPclockOffset || ipatokenTOTPtimeStep")(version 3.0; acl "Users can add TOTP token secrets"; allow (write, search) userattr = "ipatokenOwner#USERDN";)'
diff --git a/ipaserver/install/plugins/update_anonymous_aci.py b/ipaserver/install/plugins/update_anonymous_aci.py
index 2b7446ad0e989c4d951e8fa0b22a481c4ef3f3b9..22c17c387ea926af494e7372a4da42cfe339edfb 100644
--- a/ipaserver/install/plugins/update_anonymous_aci.py
+++ b/ipaserver/install/plugins/update_anonymous_aci.py
@@ -20,8 +20,6 @@
from copy import deepcopy
from ipaserver.install.plugins import FIRST, LAST
from ipaserver.install.plugins.baseupdate import PostUpdate
-#from ipalib.frontend import Updater
-#from ipaserver.install.plugins import baseupdate
from ipalib import api
from ipalib.aci import ACI
from ipalib.plugins import aci
@@ -37,7 +35,9 @@ class update_anonymous_aci(PostUpdate):
aciname = u'Enable Anonymous access'
aciprefix = u'none'
ldap = self.obj.backend
-
+ targetfilter = '(&(!(objectClass=ipaToken))(!(objectClass=ipatokenTOTP))(!(objectClass=ipatokenRadiusProxyUser))(!(objectClass=ipatokenRadiusConfiguration)))'
+ filter = None
+
(dn, entry_attrs) = ldap.get_entry(api.env.basedn, ['aci'])
acistrs = entry_attrs.get('aci', [])
@@ -45,6 +45,9 @@ class update_anonymous_aci(PostUpdate):
rawaci = aci._find_aci_by_name(acilist, aciprefix, aciname)
attrs = rawaci.target['targetattr']['expression']
+ rawfilter = rawaci.target.get('targetfilter', None)
+ if rawfilter is not None:
+ filter = rawfilter['expression']
update_attrs = deepcopy(attrs)
@@ -54,12 +57,10 @@ class update_anonymous_aci(PostUpdate):
needed_attrs.append(attr)
update_attrs.extend(needed_attrs)
- if len(attrs) == len(update_attrs):
+ if (len(attrs) == len(update_attrs) and
+ filter == targetfilter):
root_logger.debug("Anonymous ACI already update-to-date")
return (False, False, [])
- else:
- root_logger.debug("New Anonymous ACI attributes needed: %s",
- needed_attrs)
for tmpaci in acistrs:
candidate = ACI(tmpaci)
@@ -67,7 +68,17 @@ class update_anonymous_aci(PostUpdate):
acistrs.remove(tmpaci)
break
- rawaci.target['targetattr']['expression'] = update_attrs
+ if len(attrs) != len(update_attrs):
+ root_logger.debug("New Anonymous ACI attributes needed: %s",
+ needed_attrs)
+
+ rawaci.target['targetattr']['expression'] = update_attrs
+
+ if filter != targetfilter:
+ root_logger.debug("New Anonymous ACI targetfilter needed.")
+
+ rawaci.set_target_filter(targetfilter)
+
acistrs.append(unicode(rawaci))
entry_attrs['aci'] = acistrs
--
1.8.2.1
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
