Re: [Freeipa-devel] [PATCH] [dyndb] Fix error handling in configure_view() to prevent deadlocks
On 17.9.2014 20:04, Tomas Hozza wrote:
On Tue 16 Sep 2014 07:32:39 PM CEST, Petr Spacek wrote:
Hello,
attached patches fix
https://bugzilla.redhat.com/show_bug.cgi?id=1142150
https://bugzilla.redhat.com/show_bug.cgi?id=1142152
... and improve related error messages.
I will push it to https://github.com/spacekpe/bind-dynamic_db if you are okay
with it.
I think there is a mistake in the first patch:
0001-Fix-error-handling-in-configure_view-to-prevent-dead.patch
diff --git a/lib/dns/dynamic_db.c b/lib/dns/dynamic_db.c
index
bf831617b391778ec540b2a5ca0df341937f2427..30c56a65c7227497c3e772c3e1b58ff49eacbd35
100644
--- a/lib/dns/dynamic_db.c
+++ b/lib/dns/dynamic_db.c
@@ -280,16 +280,24 @@ dns_dyndb_arguments_create(isc_mem_t *mctx)
}
void
-dns_dyndb_arguments_destroy(isc_mem_t *mctx, dns_dyndb_arguments_t
*args)
+dns_dyndb_arguments_destroy(isc_mem_t *mctx, dns_dyndb_arguments_t
**argsp)
{
+ dns_dyndb_arguments_t *args;
+
REQUIRE(args != NULL);
args is not initialized here. I think it should be argsp
Good point! I will fix that. Do I have your ACK then? :-)
--
Petr^2 Spacek
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] [dyndb] Fix error handling in configure_view() to prevent deadlocks
On Thu 18 Sep 2014 08:49:05 AM CEST, Petr Spacek wrote:
On 17.9.2014 20:04, Tomas Hozza wrote:
On Tue 16 Sep 2014 07:32:39 PM CEST, Petr Spacek wrote:
Hello,
attached patches fix
https://bugzilla.redhat.com/show_bug.cgi?id=1142150
https://bugzilla.redhat.com/show_bug.cgi?id=1142152
... and improve related error messages.
I will push it to https://github.com/spacekpe/bind-dynamic_db if you are
okay
with it.
I think there is a mistake in the first patch:
0001-Fix-error-handling-in-configure_view-to-prevent-dead.patch
diff --git a/lib/dns/dynamic_db.c b/lib/dns/dynamic_db.c
index
bf831617b391778ec540b2a5ca0df341937f2427..30c56a65c7227497c3e772c3e1b58ff49eacbd35
100644
--- a/lib/dns/dynamic_db.c
+++ b/lib/dns/dynamic_db.c
@@ -280,16 +280,24 @@ dns_dyndb_arguments_create(isc_mem_t *mctx)
}
void
-dns_dyndb_arguments_destroy(isc_mem_t *mctx, dns_dyndb_arguments_t
*args)
+dns_dyndb_arguments_destroy(isc_mem_t *mctx, dns_dyndb_arguments_t
**argsp)
{
+dns_dyndb_arguments_t *args;
+
REQUIRE(args != NULL);
args is not initialized here. I think it should be argsp
Good point! I will fix that. Do I have your ACK then? :-)
Yes, It worked with the change I proposed.
So ACK with the fix ;)
Regards,
--
Tomas Hozza
Software Engineer - EMEA ENG Developer Experience
PGP: 1D9F3C2D
Red Hat Inc. http://cz.redhat.com
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] [dyndb] Fix error handling in configure_view() to prevent deadlocks
On 18.9.2014 09:12, Tomas Hozza wrote:
On Thu 18 Sep 2014 08:49:05 AM CEST, Petr Spacek wrote:
On 17.9.2014 20:04, Tomas Hozza wrote:
On Tue 16 Sep 2014 07:32:39 PM CEST, Petr Spacek wrote:
Hello,
attached patches fix
https://bugzilla.redhat.com/show_bug.cgi?id=1142150
https://bugzilla.redhat.com/show_bug.cgi?id=1142152
... and improve related error messages.
I will push it to https://github.com/spacekpe/bind-dynamic_db if you are okay
with it.
I think there is a mistake in the first patch:
0001-Fix-error-handling-in-configure_view-to-prevent-dead.patch
diff --git a/lib/dns/dynamic_db.c b/lib/dns/dynamic_db.c
index
bf831617b391778ec540b2a5ca0df341937f2427..30c56a65c7227497c3e772c3e1b58ff49eacbd35
100644
--- a/lib/dns/dynamic_db.c
+++ b/lib/dns/dynamic_db.c
@@ -280,16 +280,24 @@ dns_dyndb_arguments_create(isc_mem_t *mctx)
}
void
-dns_dyndb_arguments_destroy(isc_mem_t *mctx, dns_dyndb_arguments_t
*args)
+dns_dyndb_arguments_destroy(isc_mem_t *mctx, dns_dyndb_arguments_t
**argsp)
{
+dns_dyndb_arguments_t *args;
+
REQUIRE(args != NULL);
args is not initialized here. I think it should be argsp
Good point! I will fix that. Do I have your ACK then? :-)
Yes, It worked with the change I proposed.
So ACK with the fix ;)
Thanks, pushed to dyndb_bind9_9 branch:
e3616f0922a45fdb5942013318bbf3eef0415bf0
7fc676122191c567511a0a6eaa6f1558ffbecac5
--
Petr^2 Spacek
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
https://fedorahosted.org/freeipa/ticket/4421
--
David Kupka
From 77faaa3c7887550b493f86f90f654da8e1f42eee Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Tue, 2 Sep 2014 16:11:55 +0200
Subject: [PATCH] Allow multiple krbprincipalnames.
Allow user to specify multiple krbprincipalnames and krbcanonicalname.
User must have IT specialist role or Host Administrators privilege
assigned.
https://fedorahosted.org/freeipa/ticket/4421
---
ACI.txt| 4 +++-
API.txt| 2 +-
ipalib/plugins/host.py | 21 +
3 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/ACI.txt b/ACI.txt
index 1e6bec0ece554fb2457fae0462c0c673a9b24e41..2a83af6ffba230f2d6ba5ec521652dc2312ce6d0 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -85,7 +85,9 @@ aci: (targetattr = businesscategory || cn || createtimestamp || description ||
dn: cn=computers,cn=accounts,dc=ipa,dc=example
aci: (targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add Hosts;allow (add) groupdn = ldap:///cn=System: Add Hosts,cn=permissions,cn=pbac,dc=ipa,dc=example;)
dn: cn=computers,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = krbprincipalname)(targetfilter = ((!(krbprincipalname=*))(objectclass=ipahost)))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;)
+aci: (targetattr = krbcanonicalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add krbCanonicalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbCanonicalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;)
+dn: cn=computers,cn=accounts,dc=ipa,dc=example
+aci: (targetattr = krbprincipalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;)
dn: cn=computers,cn=accounts,dc=ipa,dc=example
aci: (targetattr = enrolledby || objectclass)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Enroll a Host;allow (write) groupdn = ldap:///cn=System: Enroll a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;)
dn: cn=computers,cn=accounts,dc=ipa,dc=example
diff --git a/API.txt b/API.txt
index bbd0f507b2faeec0239920cdcff28fe25d618e02..ef1f70397e7685161821a98a92ea575aa6eff532 100644
--- a/API.txt
+++ b/API.txt
@@ -1884,7 +1884,7 @@ option: Str('description', attribute=True, autofill=False, cli_name='desc', mult
option: Bool('ipakrbokasdelegate', attribute=False, autofill=False, cli_name='ok_as_delegate', multivalue=False, required=False)
option: Bool('ipakrbrequirespreauth', attribute=False, autofill=False, cli_name='requires_pre_auth', multivalue=False, required=False)
option: Str('ipasshpubkey', attribute=True, autofill=False, cli_name='sshpubkey', csv=True, multivalue=True, required=False)
-option: Str('krbprincipalname?', attribute=True, cli_name='principalname')
+option: Str('krbprincipalname?', attribute=True, cli_name='principalname', multivalue=True)
option: Str('l', attribute=True, autofill=False, cli_name='locality', multivalue=False, required=False)
option: Str('macaddress', attribute=True, autofill=False, cli_name='macaddress', csv=True, multivalue=True, pattern='^([a-fA-F0-9]{2}[:|\\-]?){5}[a-fA-F0-9]{2}$', required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 570bbe56aa0a315a031051f9a895702ba7c35076..53c2b95b44063049d64e8ad96bd5e52f1e1cab48 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -303,16 +303,17 @@ class host(LDAPObject):
# This will let it be added if the client ends up enrolling with
# an administrator instead.
'ipapermright': {'write'},
-'ipapermtargetfilter': [
-'(objectclass=ipahost)',
-'(!(krbprincipalname=*))',
-],
'ipapermdefaultattr': {'krbprincipalname'},
'replaces': [
'(target = ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX;)(targetfilter = (!(krbprincipalname=*)))(targetattr = krbprincipalname)(version 3.0;acl permission:Add krbPrincipalName to a host; allow (write) groupdn = ldap:///cn=Add krbPrincipalName to a host,cn=permissions,cn=pbac,$SUFFIX;)',
],
'default_privileges': {'Host Administrators', 'Host Enrollment'},
},
+'System: Add krbCanonicalName to a Host': {
+'ipapermright': {'write'},
+'ipapermdefaultattr': {'krbcanonicalname'},
+'default_privileges': {'Host Administrators'},
+},
'System: Enroll a Host': {
'ipapermright': {'write'},
'ipapermdefaultattr': {'objectclass', 'enrolledby'},
@@ -761,6 +762,7 @@ class host_mod(LDAPUpdate):
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 10:19 AM, David Kupka wrote:
+'System: Add krbCanonicalName to a Host': {
+'ipapermright': {'write'},
+'ipapermdefaultattr': {'krbcanonicalname'},
+'default_privileges': {'Host Administrators'},
+},
Would it make sense to add the krbCanonicalName to System: Add
krbPrincipalName to a Host permission as they are semantically connected? I.e.
having one ACI without the other does not make much sense?
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
On 15.9.2014 21:08, Nathaniel McCallum wrote: On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: This prevents any local attempt at rapid token code replay. If two token codes hit the system at roughly the same moment, only the first write will succeed. All subsequent authentications will fail. This obviates the need for an OTP authentication lock. https://fedorahosted.org/freeipa/ticket/4493 I still need a review of this. This is targeted for 4.1. Nathaniel Works fine with HTOP but fails for new TOTP tokens. New TOTP token doesn't have a watermark attribute set so there is nothing to delete and therefore standard login procedure fails on writeattr call (libotp.c:223). -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/17/2014 07:25 AM, David Kupka wrote: On 09/16/2014 06:09 PM, Martin Basti wrote: On 16/09/14 15:59, David Kupka wrote: On 09/12/2014 07:24 PM, Martin Basti wrote: snip / Be careful, reviewed on friday! :-) 1) whitespace error + pep8 error patch:76: trailing whitespace. # there is reverse zone for every ip address warning: 1 line adds whitespace errors. ./ipaserver/install/bindinstance.py:640:9: E265 block comment should start with '# ' Stupid mistake, sorry. 2) (server install) +for ip in ip_addresses: +for rev_zone in reverse_zones: +if bindinstance.verify_reverse_zone(rev_zone, str(ip)): +break +else: +sys.exit(1) Please add there error message instead 1 to exit function You are right, it's better to say what's wrong. 3) (server install) Code above, bindinstance.verify_reverse_zone(rev_zone, str(ip)): IMHO this will cause errors (not tested) try: rev-zones: [10.10.10.in-addr.arpa., 16.172.in-addr.arpa.] ip_addreses: [10.10.10.1, 172.16.0.1] it should be any() of zone can handle ip I indented the else branch more that I wanted. 4) (replica-install) I'm not sure about behavior of combination ip addresses, and reverse zones, In original code, if user specify reverse zone, the ip address must match. In patched version, you just add new zones for ip-addresses which doesn't math the user zones. IMO we should check if all addresses belongs to user specified zones, otherwise raise an error. But I'm open to suggestions. The behavior now basically is: Check if IP address matches any specified zone a. if yes we are good b. else search if there is existing zone that can be used ba. if yes we are good bb. else ask user for zone or create default if --unattended 5) Code for ipa-replica-install, and ipa-server-install, differs in parts where we expecting same behavior - ip addresses and reverse zones The behavoir now should be almost the same. The only difference is that when installing replica we need to search for existing reverse zones as they could be added during on another server. 6) https://engineering.redhat.com/irc-services.txt+# there is at least one ip address in every zone +if options.reverse_zones: +for reverse_zone in options.reverse_zones: +for ip in config.ips: -- A +if bindinstance.verify_reverse_zone(reverse_zone, ip): + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) +break +else: +sys.exit(1) * +# there is reverse zone for every ip address +for ip in config.ips: +for reverse_zone in options.reverse_zones: --- B +if bindinstance.verify_reverse_zone(reverse_zone, ip): +if reverse_zone not in reverse_zones: + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) +break +else: C +reverse_zone = bindinstance.find_reverse_zone(ip) +if reverse_zone is None and not options.no_reverse: +reverse_zone = util.get_reverse_zone_default(ip) - D1 +if not options.unattended and bindinstance.create_reverse(): - D +reverse_zone = bindinstance.read_reverse_zone(reverse_zone, ip) + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) - D2 You added all possible zones in step A, B step is not needed. If user doesn't specify reverse_zones you can just jump to C step *add error message C: elif not options.no_reverse D: you asked user if wants to create zone, but you don't care about answer, and in step D2 append zone from D1 note: --no-reverse and --reverse-zones cant be used together, you can use it in new code, test it before cycle Rewritten. 7) # Always use force=True as named is not set up yet add_zone(self.domain, self.zonemgr, dns_backup=self.dns_backup, -ns_hostname=api.env.host, ns_ip_address=nameserver_ip_address, -force=True) + ns_hostname=api.env.host, ns_ip_address=None, force=True) +#for ns_ip_address in nameserver_ip_address: +#add_zone(self.domain, self.zonemgr, dns_backup=self.dns_backup, +#ns_hostname=api.env.host, ns_ip_address=ns_ip_address, +#force=True) Please remove commented section I forget to clean the trash, thanks. You can remove 'ns_ip_address=None' from function 8) +if set(options.ip_addresses) = set(ips): +ips =
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On Thu, 18 Sep 2014 16:28:19 +0200 Martin Kosek mko...@redhat.com wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? I think also both the code and the tests are missing to ensure that the krbPrincipalName *also* *always* lists the krbCanonicalName. I think with the current code you can end up in a situation where you can have a value in KrbCanonicalName and completely different values in KrbPrincipalName. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 04:28 PM, Martin Kosek wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? To allow additional krbPrincipalNames. This behavior is requested by the ticket. Martin -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 04:40 PM, Simo Sorce wrote: On Thu, 18 Sep 2014 16:28:19 +0200 Martin Kosek mko...@redhat.com wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? I think also both the code and the tests are missing to ensure that the krbPrincipalName *also* *always* lists the krbCanonicalName. I think with the current code you can end up in a situation where you can have a value in KrbCanonicalName and completely different values in KrbPrincipalName. I didn't realize that there is such requirement although it's logical. I will fix it, thanks. Simo. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) 2) Typo? There is no IP address matching reverze_zone %s. -^^ 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) 4) Ask framework gurus, if installutils module is better place for function above -- Martin Basti ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0066] Make ipatokenTOTPwatermark a required attribute
This makes ipatokenTOTPwatermark have exactly the same semantics as
ipatokenHOTPcounter.
NOTE: This patch includes an update plugin which will update existing
token objects. This should be low impact since it only updates TOTP
tokens which have never been used. TOTP tokens which have already been
used should already have ipatokenTOTPwatermark set.
From 30581e26faaebbc2fad3c1d80303f0a6ce3ad8cf Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum npmccal...@redhat.com
Date: Thu, 18 Sep 2014 13:45:46 -0400
Subject: [PATCH] Make ipatokenTOTPwatermark a required attribute
This makes ipatokenTOTPwatermark have exactly the same semantics as
ipatokenHOTPcounter.
---
install/share/70ipaotp.ldif| 2 +-
ipalib/plugins/otptoken.py | 10 -
ipaserver/install/plugins/Makefile.am | 1 +
ipaserver/install/plugins/update_totp_otptokens.py | 49 ++
4 files changed, 60 insertions(+), 2 deletions(-)
create mode 100644 ipaserver/install/plugins/update_totp_otptokens.py
diff --git a/install/share/70ipaotp.ldif b/install/share/70ipaotp.ldif
index bc95556682ef65ba375aa2f3cab6f53621641b3f..b35ddc7796559df4588d8d140f01ef049ec12bd2 100644
--- a/install/share/70ipaotp.ldif
+++ b/install/share/70ipaotp.ldif
@@ -25,7 +25,7 @@ attributeTypes: (2.16.840.1.113730.3.8.16.1.20 NAME 'ipatokenUserMapAttribute' D
attributeTypes: (2.16.840.1.113730.3.8.16.1.21 NAME 'ipatokenHOTPcounter' DESC 'HOTP counter' EQUALITY integerMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.27 SINGLE-VALUE X-ORIGIN 'IPA OTP')
attributeTypes: (2.16.840.1.113730.3.8.16.1.22 NAME 'ipatokenTOTPwatermark' DESC 'TOTP watermark' EQUALITY integerMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.27 SINGLE-VALUE X-ORIGIN 'IPA OTP')
objectClasses: (2.16.840.1.113730.3.8.16.2.1 NAME 'ipaToken' SUP top ABSTRACT DESC 'Abstract token class for tokens' MUST (ipatokenUniqueID) MAY (description $ managedBy $ ipatokenOwner $ ipatokenDisabled $ ipatokenNotBefore $ ipatokenNotAfter $ ipatokenVendor $ ipatokenModel $ ipatokenSerial) X-ORIGIN 'IPA OTP')
-objectClasses: (2.16.840.1.113730.3.8.16.2.2 NAME 'ipatokenTOTP' SUP ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $ ipatokenTOTPtimeStep) MAY (ipatokenTOTPwatermark) X-ORIGIN 'IPA OTP')
+objectClasses: (2.16.840.1.113730.3.8.16.2.2 NAME 'ipatokenTOTP' SUP ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $ ipatokenTOTPtimeStep $ ipatokenTOTPwatermark) X-ORIGIN 'IPA OTP')
objectClasses: (2.16.840.1.113730.3.8.16.2.3 NAME 'ipatokenRadiusProxyUser' SUP top AUXILIARY DESC 'Radius Proxy User' MAY (ipatokenRadiusConfigLink $ ipatokenRadiusUserName) X-ORIGIN 'IPA OTP')
objectClasses: (2.16.840.1.113730.3.8.16.2.4 NAME 'ipatokenRadiusConfiguration' SUP top STRUCTURAL DESC 'Proxy Radius Configuration' MUST (cn $ ipatokenRadiusServer $ ipatokenRadiusSecret) MAY (description $ ipatokenRadiusTimeout $ ipatokenRadiusRetries $ ipatokenUserMapAttribute) X-ORIGIN 'IPA OTP')
objectClasses: (2.16.840.1.113730.3.8.16.2.5 NAME 'ipatokenHOTP' SUP ipaToken STRUCTURAL DESC 'HOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenHOTPcounter) X-ORIGIN 'IPA OTP')
diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 1bd85d4b952dc51ea800ed37c49b3c50aeb31492..37b505d09e49bc7f7a46a3e6cc69beeceff8e5c4 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -62,7 +62,7 @@ EXAMPLES:
register = Registry()
TOKEN_TYPES = {
-u'totp': ['ipatokentotpclockoffset', 'ipatokentotptimestep'],
+u'totp': ['ipatokentotpclockoffset', 'ipatokentotptimestep', 'ipatokentotpwatermark'],
u'hotp': ['ipatokenhotpcounter']
}
@@ -237,6 +237,14 @@ class otptoken(LDAPObject):
minvalue=0,
flags=('no_update'),
),
+Int('ipatokentotpwatermark',
+cli_name='counter',
+label=_('Counter'),
+default=0,
+autofill=True,
+minvalue=0,
+flags=('no_output', 'no_option'),
+),
)
diff --git a/ipaserver/install/plugins/Makefile.am b/ipaserver/install/plugins/Makefile.am
index 7cf0495131b2108ee78a79758cee42ec344652c7..afc78c4241d7e35664f1c1feba6e1c3c21dd45d0 100644
--- a/ipaserver/install/plugins/Makefile.am
+++ b/ipaserver/install/plugins/Makefile.am
@@ -9,6 +9,7 @@ app_PYTHON = \
dns.py \
updateclient.py \
update_services.py \
+ update_totp_otptokens.py \
update_anonymous_aci.py \
update_pacs.py \
ca_renewal_master.py \
diff --git a/ipaserver/install/plugins/update_totp_otptokens.py b/ipaserver/install/plugins/update_totp_otptokens.py
new file mode 100644
index ..eeb9d55fd1a4ade25366de1e2afb7f2fb69f644f
--- /dev/null
+++ b/ipaserver/install/plugins/update_totp_otptokens.py
@@
Re: [Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
On Thu, 2014-09-18 at 14:00 +0200, Petr Vobornik wrote: On 15.9.2014 21:08, Nathaniel McCallum wrote: On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: This prevents any local attempt at rapid token code replay. If two token codes hit the system at roughly the same moment, only the first write will succeed. All subsequent authentications will fail. This obviates the need for an OTP authentication lock. https://fedorahosted.org/freeipa/ticket/4493 I still need a review of this. This is targeted for 4.1. Nathaniel Works fine with HTOP but fails for new TOTP tokens. New TOTP token doesn't have a watermark attribute set so there is nothing to delete and therefore standard login procedure fails on writeattr call (libotp.c:223). I have fixed this by making ipatokenTOTPwatermark a required attribute (MAY - MUST). I did this in a separate patch (0066) because I thought it was cleaner. https://www.redhat.com/archives/freeipa-devel/2014-September/msg00386.html There is no change to this patch, but it now depends on my patch 0066 (linked above). Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0066] Make ipatokenTOTPwatermark a required attribute
On Thu, 2014-09-18 at 13:56 -0400, Nathaniel McCallum wrote: This makes ipatokenTOTPwatermark have exactly the same semantics as ipatokenHOTPcounter. NOTE: This patch includes an update plugin which will update existing token objects. This should be low impact since it only updates TOTP tokens which have never been used. TOTP tokens which have already been used should already have ipatokenTOTPwatermark set. FYI, this patch is now a prerequisite of my patch 0062. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0066] Make ipatokenTOTPwatermark a required attribute
On Thu, 18 Sep 2014 13:56:44 -0400 Nathaniel McCallum npmccal...@redhat.com wrote: -objectClasses: (2.16.840.1.113730.3.8.16.2.2 NAME 'ipatokenTOTP' SUP ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $ ipatokenTOTPtimeStep) MAY (ipatokenTOTPwatermark) X-ORIGIN 'IPA OTP') +objectClasses: (2.16.840.1.113730.3.8.16.2.2 NAME 'ipatokenTOTP' SUP ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $ ipatokenTOTPtimeStep $ ipatokenTOTPwatermark) X-ORIGIN 'IPA OTP') NACK, you cannot move from MAY to MUST. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
On Thu, 18 Sep 2014 13:59:34 -0400 Nathaniel McCallum npmccal...@redhat.com wrote: On Thu, 2014-09-18 at 14:00 +0200, Petr Vobornik wrote: On 15.9.2014 21:08, Nathaniel McCallum wrote: On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: This prevents any local attempt at rapid token code replay. If two token codes hit the system at roughly the same moment, only the first write will succeed. All subsequent authentications will fail. This obviates the need for an OTP authentication lock. https://fedorahosted.org/freeipa/ticket/4493 I still need a review of this. This is targeted for 4.1. Nathaniel Works fine with HTOP but fails for new TOTP tokens. New TOTP token doesn't have a watermark attribute set so there is nothing to delete and therefore standard login procedure fails on writeattr call (libotp.c:223). I have fixed this by making ipatokenTOTPwatermark a required attribute (MAY - MUST). I did this in a separate patch (0066) because I thought it was cleaner. This can easily break stuff, and is not allowed, sorry you need to find a way that will not cause objects, even temporarily to be incomplete. (think of a replica getting the new schema while an older one pushes the object via replication) Simo. https://www.redhat.com/archives/freeipa-devel/2014-September/msg00386.html There is no change to this patch, but it now depends on my patch 0066 (linked above). Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0066] Make ipatokenTOTPwatermark a required attribute
On Thu, 2014-09-18 at 14:18 -0400, Simo Sorce wrote: On Thu, 18 Sep 2014 13:56:44 -0400 Nathaniel McCallum npmccal...@redhat.com wrote: -objectClasses: (2.16.840.1.113730.3.8.16.2.2 NAME 'ipatokenTOTP' SUP ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $ ipatokenTOTPtimeStep) MAY (ipatokenTOTPwatermark) X-ORIGIN 'IPA OTP') +objectClasses: (2.16.840.1.113730.3.8.16.2.2 NAME 'ipatokenTOTP' SUP ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $ ipatokenTOTPtimeStep $ ipatokenTOTPwatermark) X-ORIGIN 'IPA OTP') NACK, you cannot move from MAY to MUST. This is precisely what we have been discussing on IRC today. The consensus was that this was acceptable because of the update plugin and the rarity of the state in which a token would not have ipatokenTOTPwatermark set (the token has to be created an never used). Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0066] Make ipatokenTOTPwatermark a required attribute
On Thu, 18 Sep 2014 14:22:07 -0400 Nathaniel McCallum npmccal...@redhat.com wrote: On Thu, 2014-09-18 at 14:18 -0400, Simo Sorce wrote: On Thu, 18 Sep 2014 13:56:44 -0400 Nathaniel McCallum npmccal...@redhat.com wrote: -objectClasses: (2.16.840.1.113730.3.8.16.2.2 NAME 'ipatokenTOTP' SUP ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $ ipatokenTOTPtimeStep) MAY (ipatokenTOTPwatermark) X-ORIGIN 'IPA OTP') +objectClasses: (2.16.840.1.113730.3.8.16.2.2 NAME 'ipatokenTOTP' SUP ipaToken STRUCTURAL DESC 'TOTP Token Type' MUST (ipatokenOTPkey $ ipatokenOTPalgorithm $ ipatokenOTPdigits $ ipatokenTOTPclockOffset $ ipatokenTOTPtimeStep $ ipatokenTOTPwatermark) X-ORIGIN 'IPA OTP') NACK, you cannot move from MAY to MUST. This is precisely what we have been discussing on IRC today. The consensus was that this was acceptable because of the update plugin and the rarity of the state in which a token would not have ipatokenTOTPwatermark set (the token has to be created an never used). Sorry I was not around, but it is never acceptable, as it may cause replication failures. This has been a long (albeit perhaps unspoken) rule in changing schema in FreeIPA. Existing objectlasses can *never* gain new MUST attributes. This rule is rigid and is non-negotiable. Sorry. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
Martin Kosek wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? Yes, this is exactly the change I was referring to. Permission changes within a plugin now translate automatically to ACI changes. Sorry I wasn't clearer. This ACI gets replaced with a much simpler one and I'm not 100% sure it will work with all enrollments: -aci: (targetattr = krbprincipalname)(targetfilter = ((!(krbprincipalname=*))(objectclass=ipahost)))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) +aci: (targetattr = krbprincipalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) The first one restricts writing the attribute only if it isn't already set. The second lets it be changed unconditionally. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On Thu, 18 Sep 2014 14:57:45 -0400 Rob Crittenden rcrit...@redhat.com wrote: Martin Kosek wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? Yes, this is exactly the change I was referring to. Permission changes within a plugin now translate automatically to ACI changes. Sorry I wasn't clearer. This ACI gets replaced with a much simpler one and I'm not 100% sure it will work with all enrollments: -aci: (targetattr = krbprincipalname)(targetfilter = ((!(krbprincipalname=*))(objectclass=ipahost)))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) +aci: (targetattr = krbprincipalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) The first one restricts writing the attribute only if it isn't already set. The second lets it be changed unconditionally. Yeah this is wrong indeed, the point of the ACI is to allow setting the principal only when it is not already set, which is the OTP enrollment case. But if krbprincipal is set then this specific permission should not grant rights to change it. At least this was my understanding. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 09:11 PM, Simo Sorce wrote: On Thu, 18 Sep 2014 14:57:45 -0400 Rob Crittenden rcrit...@redhat.com wrote: Martin Kosek wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? Yes, this is exactly the change I was referring to. Permission changes within a plugin now translate automatically to ACI changes. Sorry I wasn't clearer. This ACI gets replaced with a much simpler one and I'm not 100% sure it will work with all enrollments: -aci: (targetattr = krbprincipalname)(targetfilter = ((!(krbprincipalname=*))(objectclass=ipahost)))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) +aci: (targetattr = krbprincipalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) The first one restricts writing the attribute only if it isn't already set. The second lets it be changed unconditionally. Yeah this is wrong indeed, the point of the ACI is to allow setting the principal only when it is not already set, which is the OTP enrollment case. But if krbprincipal is set then this specific permission should not grant rights to change it. At least this was my understanding. Simo. Right. It seems to me we should add keep this permission intact and add a new permission allowing adding krbPrincipalName aliases. This would allow writing both krbPrincipalName and krbCanonicalName. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
