Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 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) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 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) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP address(es), so you should use label Reverse zone(s) Fixed. 5) ipa-server-install --ip-address=10.16.78.105
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 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) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 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) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP address(es), so you should use label Reverse zone(s) Fixed. 5) ipa-server-install
Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file
On 09/02/2014 10:18 AM, Jan Cholasta wrote: Dne 27.8.2014 v 16:49 David Kupka napsal(a): On 08/27/2014 11:22 AM, Jan Cholasta wrote: Dne 26.8.2014 v 15:55 Rob Crittenden napsal(a): David Kupka wrote: On 08/26/2014 03:08 PM, Jan Cholasta wrote: Hi, Dne 26.8.2014 v 13:01 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4481 Doing this will break ipa-client-automount and ipa-certupdate, because they assume that api.env.host contains the hostname of the local system (which is the default value). It looked suspiciously simple so I could expect that there is some catch. There is obviously some confusion about what the option should represent (documentation says server hostname, code does client hostname), IMO we should resolve that first. Ok, are there any suggestions? What is the desired state? AIUI the server option is deprecated because it wasn't being used, not that it needed to be replaced. I believe that in most cases the server name is pulled from the xmlrpc_uri. Yes, that's what the ticket says: https://fedorahosted.org/freeipa/ticket/3071. Ok, adding 'host' entry with local host name. host has always meant the local host name. I think the man page is wrong. +1 Fixing the line in man page. rob ACK as long as this works for Nalin. I see Nalin is OK with the patch, I am not so OK. What should we do with the server option then? It is still being referred to as Deprecated in the man page. Should we then un-deprecate it as Honza suggested down the thread? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'
On 25/09/14 17:13, Martin Kosek wrote: On 09/25/2014 04:39 PM, Petr Viktorin wrote: On 09/25/2014 04:32 PM, Petr Spacek wrote: On 25.9.2014 10:31, Martin Basti wrote: On 24/09/14 16:24, Martin Basti wrote: On 24/09/14 16:05, Martin Basti wrote: On 23/09/14 17:45, Petr Vobornik wrote: On 25.8.2014 14:52, Martin Basti wrote: Patches attached. Ticket: https://fedorahosted.org/freeipa/ticket/4149 There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause the named service is stopped after deleting zone. Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138 Review of: http://www.redhat.com/archives/freeipa-devel/2014-September/msg00484.html 1. Please follow pep8 for the new code. # git diff HEAD~7 -U0 | pep8 --diff --ignore=E501 Produces 25 erros. Only E124 and E128 could be ignored if they are part of old code. I left there some pep8 errors. They don't decrease readability Patch 120: 3. This patch uses term 'deprecated' in a different meaning than a DeprecatedParam. It creates inconsistency - future confusion. IMHO this usage is correct since the usual understanding of deprecation is that the param is still usable but user should be prepared that it will be removed in a future. IMHO DeprecatedParam is badly designed but that's not an issue of this patch. I think we can leave this as is and create a ticket to rename DeprecatedParam e.g. to RemovedParam. What do you think? https://fedorahosted.org/freeipa/ticket/4566 5. You've removed 'idnssoamname' and 'force' from Web UI but dnszone-add precallback still uses these params. What is the intended purpose? User should use modify dialog in webUI for zones. Precallback fills default value for idnsmname from LDAP. with --force there will be no validation of user specified soa mname Purpose is a user should let IPA to fill mname with safe value. Patch 123: 10. In `normalize_zonemgr(zonemgr)`, if zonemgr contains '@' shouldn't it be normalized to contain '.' at the end? Or is it handled by bind-dyndb-ldap? Zone manager (SOA RNAME) can eb relative name, BIND will append zone name. Currently we cant validate if email address is reachable, it doestn matter if it is filled with nonexistent absolute name, or nonexistent relative name. Unrelated to this patch set: a. One is able to run: # ipa dnszone-remove-permission $zone multiple times and it always returns success Is it intentional? No, it isn't. I will inspect it and I will send additional patch b. Web UI doesn't have means to call dnszone-mod with --force option I'm not sure what you mean, it didn't do that before my patches. Updated patches attached I accidentally removed one line in previous patchset. Updated patches attached Sorry my IDE was too smart, and somehow added its configuration file to commit and I didn't notice it. Patches attached. ACK, it works for me. Replica installation and deletion properly adds and deletes records as necessary. I would defer further improvements to https://fedorahosted.org/freeipa/ticket/3343 Pushed to: ipa-4-1: b7e3a990369d85dfd12165891cf9142d669a0259 master: bc2eaa145637e1947449ee53548243ab22059805 I reopened the ticket, we missed update to DNS help page (ipa help dns): https://fedorahosted.org/freeipa/ticket/4149#comment:18 Martin Thanks! Patch attached. -- Martin Basti From 2ed3e111e31e29cb100479c4445e87f32c35c8a4 Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Fri, 26 Sep 2014 10:15:59 +0200 Subject: [PATCH] Remove --ip-address, --name-server otpions from DNS help Ticket: https://fedorahosted.org/freeipa/ticket/4149 --- ipalib/plugins/dns.py | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index 64dc6f4bd9e1ed93ac325bf66a2d4859b8b03fb9..df42c6bfe9d19c6530cc7e90663306f937aaede5 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -87,9 +87,7 @@ ipa dnsrecord-mod --mx-rec=0 mx.example.com. --mx-preference=1 EXAMPLES: ) + _( Add new zone: - ipa dnszone-add example.com --name-server=ns \\ - --admin-email=ad...@example.com \\ - --ip-address=192.0.2.1 + ipa dnszone-add example.com --admin-email=ad...@example.com ) + _( Add system permission that can be used for per-zone privilege delegation: ipa dnszone-add-permission example.com @@ -105,8 +103,7 @@ EXAMPLES: ipa dnszone-mod example.com --allow-transfer=192.0.2.0/24 ) + _( Add new reverse zone specified by network IP address: - ipa dnszone-add --name-from-ip=192.0.2.0/24 \\ - --name-server=ns.example.com. + ipa dnszone-add --name-from-ip=192.0.2.0/24 ) + _( Add second nameserver for example.com: ipa dnsrecord-add example.com @ --ns-rec=nameserver2.example.com -- 1.8.3.1 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'
On 09/26/2014 10:20 AM, Martin Basti wrote: On 25/09/14 17:13, Martin Kosek wrote: On 09/25/2014 04:39 PM, Petr Viktorin wrote: On 09/25/2014 04:32 PM, Petr Spacek wrote: On 25.9.2014 10:31, Martin Basti wrote: On 24/09/14 16:24, Martin Basti wrote: On 24/09/14 16:05, Martin Basti wrote: On 23/09/14 17:45, Petr Vobornik wrote: On 25.8.2014 14:52, Martin Basti wrote: Patches attached. Ticket: https://fedorahosted.org/freeipa/ticket/4149 There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause the named service is stopped after deleting zone. Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138 Review of: http://www.redhat.com/archives/freeipa-devel/2014-September/msg00484.html 1. Please follow pep8 for the new code. # git diff HEAD~7 -U0 | pep8 --diff --ignore=E501 Produces 25 erros. Only E124 and E128 could be ignored if they are part of old code. I left there some pep8 errors. They don't decrease readability Patch 120: 3. This patch uses term 'deprecated' in a different meaning than a DeprecatedParam. It creates inconsistency - future confusion. IMHO this usage is correct since the usual understanding of deprecation is that the param is still usable but user should be prepared that it will be removed in a future. IMHO DeprecatedParam is badly designed but that's not an issue of this patch. I think we can leave this as is and create a ticket to rename DeprecatedParam e.g. to RemovedParam. What do you think? https://fedorahosted.org/freeipa/ticket/4566 5. You've removed 'idnssoamname' and 'force' from Web UI but dnszone-add precallback still uses these params. What is the intended purpose? User should use modify dialog in webUI for zones. Precallback fills default value for idnsmname from LDAP. with --force there will be no validation of user specified soa mname Purpose is a user should let IPA to fill mname with safe value. Patch 123: 10. In `normalize_zonemgr(zonemgr)`, if zonemgr contains '@' shouldn't it be normalized to contain '.' at the end? Or is it handled by bind-dyndb-ldap? Zone manager (SOA RNAME) can eb relative name, BIND will append zone name. Currently we cant validate if email address is reachable, it doestn matter if it is filled with nonexistent absolute name, or nonexistent relative name. Unrelated to this patch set: a. One is able to run: # ipa dnszone-remove-permission $zone multiple times and it always returns success Is it intentional? No, it isn't. I will inspect it and I will send additional patch b. Web UI doesn't have means to call dnszone-mod with --force option I'm not sure what you mean, it didn't do that before my patches. Updated patches attached I accidentally removed one line in previous patchset. Updated patches attached Sorry my IDE was too smart, and somehow added its configuration file to commit and I didn't notice it. Patches attached. ACK, it works for me. Replica installation and deletion properly adds and deletes records as necessary. I would defer further improvements to https://fedorahosted.org/freeipa/ticket/3343 Pushed to: ipa-4-1: b7e3a990369d85dfd12165891cf9142d669a0259 master: bc2eaa145637e1947449ee53548243ab22059805 I reopened the ticket, we missed update to DNS help page (ipa help dns): https://fedorahosted.org/freeipa/ticket/4149#comment:18 Martin Thanks! Patch attached. ACK! Pushed to: master: 3f8cfdab269490e4935db7d296c3fc7f2fa552f5 ipa-4-1: 0f2eb65f008777ebdee9b35f5f69bada66066484 Martin ___ 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/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 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) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 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) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP address(es), so you should use label Reverse
Re: [Freeipa-devel] [PATCHES] 0633-0634 Move setting SELinux booleans to platform code; Set SELinux booleans when restoring
On 09/25/2014 11:34 AM, thierry bordaz wrote: On 09/25/2014 10:58 AM, Petr Viktorin wrote: On 09/24/2014 06:02 PM, thierry bordaz wrote: On 08/15/2014 10:40 PM, Petr Viktorin wrote: A fix for https://fedorahosted.org/freeipa/ticket/4157 This depends on my patches 0631-0632 (for backup/restore integration tests). Our setsebool code was repeated a few times. Instead of adding another copy, I refactored what we have into a platform task. I fixed two old setsebool tickets while I was at it: https://fedorahosted.org/freeipa/ticket/2519 https://fedorahosted.org/freeipa/ticket/2934 Since ipaplatform should not depend on ipalib, and I needed a new exception type, I added a new module, ipapython.errors. This might not be the best name, since it could be confused with ipalib.errors. Opinions welcome. As for the second patch: ideally, rather than what I do with `if 'ADTRUST' in self.backup_services`, we'd get the list of booleans directly from the *instance modules, or even tell the individual services to restore themselves. But, that refactoring looks like too much to do now. Filed easyfix: https://fedorahosted.org/freeipa/ticket/4571 The first patch looks good to me. Just a minor comment. The test and run of 'paths.SELINUXENABLED' is present several times in tasks.py and fedora. Does it worth to refactor it ? About the second patch, something I do not understand. restore_selinux_booleans resets the selinux boolean to the values that are taken from SELINUX_BOOLEAN_SETTINGS in the instance (http/ad) . Does that mean this dict has been updated with the original values (using 'backup_func' in set_selinux_booleans ?). This is restoring an IPA installation, not restoring the system to a pre-IPA state. The settings need to be the same as if IPA was being installed. OK thanks for the explanation. Is this an ACK? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 129 ipa-kdb: fix unit tests
On 09/24/2014 01:38 PM, Jakub Hrozek wrote: On Tue, Jul 22, 2014 at 05:24:51PM +0200, Sumit Bose wrote: Hi, it looks like the ipa-kdb unit test is broken. This patch tries to fix it. bye, Sumit ACK ... Pushed to: master: 757272a3f818e85e7f0b88060efbcd76d3a93f8b ipa-4-1: 5297cc9fa5f2b4890899247cc94057c19174f2e7 ipa-4-0: cfa2f2146d48f474bde9d11a516a51dddf4eeceb Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0637 upgradeinstance: Restore listeners on failure
On 09/25/2014 01:24 PM, Martin Kosek wrote: On 09/24/2014 10:43 AM, Martin Kosek wrote: On 08/22/2014 06:07 PM, Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/4499 Actually I wonder why we use backup_state/restore_state for these settings. Rob, was there a reason for not just always setting nsslapd-port: 389 and nsslapd-security: on? This works pretty nicely, I liked the service.py extension. My test output: # ipa-ldap-updater --upgrade Upgrading IPA: [1/10]: stopping directory server [2/10]: saving configuration [3/10]: disabling listeners [4/10]: starting directory server [5/10]: preparing server upgrade PRE_SCHEMA_UPDATE [6/10]: updating schema ... [7/10]: upgrading server [error] ValueError: Ha! [cleanup]: stopping directory server [cleanup]: restoring configuration # ipactl start # netstat -putna | grep 389 ... tcp6 0 0 10.16.78.147:38910.16.78.147:37490 ESTABLISHED 5956/ns-slapd So I am willing to ACK if there are no other objections. Martin I read the silence as no objections, so ACK. Pushed to: master: 9a188607fcf68721fc8c38c3c73ee02cac76b58a ipa-4-1: b333e7adc98ff7c5335fbc7ce1bde5b9dfb3f5ef Martin Just found (benign) display issue with this patch when --external-ca is used. See: https://fedorahosted.org/freeipa/ticket/4499#comment:4 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0633-0634 Move setting SELinux booleans to platform code; Set SELinux booleans when restoring
On 09/26/2014 11:23 AM, Martin Kosek wrote: On 09/25/2014 11:34 AM, thierry bordaz wrote: On 09/25/2014 10:58 AM, Petr Viktorin wrote: On 09/24/2014 06:02 PM, thierry bordaz wrote: On 08/15/2014 10:40 PM, Petr Viktorin wrote: A fix for https://fedorahosted.org/freeipa/ticket/4157 This depends on my patches 0631-0632 (for backup/restore integration tests). Our setsebool code was repeated a few times. Instead of adding another copy, I refactored what we have into a platform task. I fixed two old setsebool tickets while I was at it: https://fedorahosted.org/freeipa/ticket/2519 https://fedorahosted.org/freeipa/ticket/2934 Since ipaplatform should not depend on ipalib, and I needed a new exception type, I added a new module, ipapython.errors. This might not be the best name, since it could be confused with ipalib.errors. Opinions welcome. As for the second patch: ideally, rather than what I do with `if 'ADTRUST' in self.backup_services`, we'd get the list of booleans directly from the *instance modules, or even tell the individual services to restore themselves. But, that refactoring looks like too much to do now. Filed easyfix: https://fedorahosted.org/freeipa/ticket/4571 The first patch looks good to me. Just a minor comment. The test and run of 'paths.SELINUXENABLED' is present several times in tasks.py and fedora. Does it worth to refactor it ? About the second patch, something I do not understand. restore_selinux_booleans resets the selinux boolean to the values that are taken from SELINUX_BOOLEAN_SETTINGS in the instance (http/ad) . Does that mean this dict has been updated with the original values (using 'backup_func' in set_selinux_booleans ?). This is restoring an IPA installation, not restoring the system to a pre-IPA state. The settings need to be the same as if IPA was being installed. OK thanks for the explanation. Is this an ACK? Martin Ho sorry, yes for me it is a ACK. thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
On 09/23/2014 11:46 AM, Jan Cholasta wrote: Dne 6.8.2014 v 18:17 Jan Cholasta napsal(a): Dne 6.8.2014 v 14:43 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4447. +cert_group.add_option(--ca-key-algorithm, dest=ca_key_algorithm, + help=Key algorithm of the IPA CA certificate (default SHA256withRSA)) Why not set the default here rather than later? CA-related defaults should be internalized in CA-related code IMHO. Should the list of options be added to the man page as well? Sure, why not. Do we want to support the MD*-based signing algorithms? I'd think not. Since the reason this patch exists is to support old and/or broken external CAs, I would think yes, but I don't have a strong opinion on this. Turns out Dogtag does not like them, so I removed them. Seeing the context makes me wonder if we should eventually add options for CA key size and signing alg as well. rob Updated patch attached. I tested the patch (it works fine with Dogtag 10), but I got very confused. What CA option are we setting? Signing algorithm or Key Algorithm? I thought we are only setting Signing algorithm, but in that case: - --ca-key-algorithm option should rather read --ca-signing-key-algorithm - Dogtag9 update should only set --signing_algorithm and not --key_algorithm - man page should also be updated with proper explanation. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0633-0634 Move setting SELinux booleans to platform code; Set SELinux booleans when restoring
On 09/26/2014 11:57 AM, thierry bordaz wrote: On 09/26/2014 11:23 AM, Martin Kosek wrote: On 09/25/2014 11:34 AM, thierry bordaz wrote: On 09/25/2014 10:58 AM, Petr Viktorin wrote: On 09/24/2014 06:02 PM, thierry bordaz wrote: On 08/15/2014 10:40 PM, Petr Viktorin wrote: A fix for https://fedorahosted.org/freeipa/ticket/4157 This depends on my patches 0631-0632 (for backup/restore integration tests). Our setsebool code was repeated a few times. Instead of adding another copy, I refactored what we have into a platform task. I fixed two old setsebool tickets while I was at it: https://fedorahosted.org/freeipa/ticket/2519 https://fedorahosted.org/freeipa/ticket/2934 Since ipaplatform should not depend on ipalib, and I needed a new exception type, I added a new module, ipapython.errors. This might not be the best name, since it could be confused with ipalib.errors. Opinions welcome. As for the second patch: ideally, rather than what I do with `if 'ADTRUST' in self.backup_services`, we'd get the list of booleans directly from the *instance modules, or even tell the individual services to restore themselves. But, that refactoring looks like too much to do now. Filed easyfix: https://fedorahosted.org/freeipa/ticket/4571 The first patch looks good to me. Just a minor comment. The test and run of 'paths.SELINUXENABLED' is present several times in tasks.py and fedora. Does it worth to refactor it ? About the second patch, something I do not understand. restore_selinux_booleans resets the selinux boolean to the values that are taken from SELINUX_BOOLEAN_SETTINGS in the instance (http/ad) . Does that mean this dict has been updated with the original values (using 'backup_func' in set_selinux_booleans ?). This is restoring an IPA installation, not restoring the system to a pre-IPA state. The settings need to be the same as if IPA was being installed. OK thanks for the explanation. Is this an ACK? Martin Ho sorry, yes for me it is a ACK. thierry Ok, thanks. Pushed to: master: dea825fd9cdd36a6fa371b2a5e1d1f35c177c6ef ipa-4-1: 9b5436cbb9d0ab0c4b5a46885d108fda1cdf1b6d Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0116] Refactoring of service autobind
On 09/25/2014 03:06 PM, Martin Basti wrote: On 25/09/14 14:47, Jan Cholasta wrote: Dne 25.9.2014 v 10:51 Martin Basti napsal(a): On 19/09/14 14:30, Jan Cholasta wrote: Dne 19.9.2014 v 13:32 Martin Basti napsal(a): On 01/09/14 16:26, Martin Basti wrote: On 28/08/14 14:01, Jan Cholasta wrote: Hi, Dne 27.8.2014 v 15:22 Martin Basti napsal(a): Patch attached. 1) Please rename object_exists to entry_exists. 2) Use empty attribute list in get_entry() in object_exists/entry_exists. 3) Please update LDAPObject.get_dn_if_exists() to use object_exists/entry_exists. 4) I'm not a fan of how do_bind() is laid out, IMHO something like this would be better (untested): +def do_bind(self, dm_password=None, autobind=AUTOBIND_AUTO, timeout=DEFAULT_TIMEOUT): +if dm_password: +self.do_simple_bind(bindpw=dm_password, timeout=timeout) +return + +if autobind != AUTOBIND_DISABLED and os.getegid() == 0 and self.ldapi: +try: +# autobind +pw_name = pwd.getpwuid(os.geteuid()).pw_name +self.do_external_bind(pw_name, timeout=timeout) +return +except errors.NotFound: +if autobind == AUTOBIND_ENABLED: +# autobind was required and failed, raise +# exception that it failed +raise + +# Fall back +self.do_sasl_gssapi_bind(timeout=timeout) Honza 3) skipped as we discuss on IRC Updated patch attached ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Please review, this should be in 4.1 1) The patch need a rebase on top of current ipa-4-1. I can apply it (Am I doing something wrong?) 2) You can remove import pwd from service.py, it is no longer used there. 3) Are named constants for the autobind argument the right thing to do? It is a tri-state which can be expressed with None/True/False. (I'm just asking, I don't have a strong opinion on this.) As we discussed on IRC, using None/True/False, is not good approach. Updated patch attached ACK, but the patch still does not apply cleanly on ipa-4-1: $ git apply freeipa-mbasti-0116.3-Refactoring-of-autobind-object_exists.patch error: patch failed: ipaserver/install/service.py:20 error: ipaserver/install/service.py: patch does not apply Rebased patches attached Pushed to: master: 29ba9d9d26b92498902d40d71adae193308b5c92 ipa-4-1: 8e0f8bc7ad8e91bcf9e30e3cc8159838977348e6 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0118] Allow to disable service (in LDAP)
On 09/25/2014 05:14 PM, Jan Cholasta wrote: Dne 25.9.2014 v 16:15 Martin Basti napsal(a): On 22/09/14 19:30, Martin Basti wrote: On 19/09/14 14:47, Jan Cholasta wrote: Dne 19.9.2014 v 13:33 Martin Basti napsal(a): On 02/09/14 11:59, Martin Basti wrote: On 02/09/14 09:10, Jan Cholasta wrote: Hi, Dne 1.9.2014 v 16:57 Martin Basti napsal(a): This patch allows to disable service in LDAP to prevents service to be started by ipactl restart Required by DNSSEC Patch attached I don't think the extra argument in ldap_enable is necessary. It should enable the service no matter if the entry existed before or not. Similarly, in ldap_disable you should not raise an error when the entry is not found, because that already makes the service disabled. Honza Updated patch attached ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Please review, this should be in 4.1 -- Martin Basti 1) ipaConfigString is case-insensitive, so you need to look for the string enabledService in a case-insensitive way. 2) Don't say failed to enable ... when the service is already enabled. It is not a failure. Same for disabled. 3) Missing ipaConfigString is nothing to warn about. 4) You should log success in both ldap_enable/ldap_disable. 5) The except below update_entry should say except errors.EmptyModlist. Thank you, updated patch attached Updated patch attached. I modified ldap_disable to use search function. ACK. Pushed to: master: 66ce71f17a689bcad03022e3df6bbdf0fada2ab8 ipa-4-1: df9086c938963fdf59309cd8af69f4f5d8a7a34e Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file
On 09/26/2014 09:56 AM, Martin Kosek wrote:
On 09/02/2014 10:18 AM, Jan Cholasta wrote:
Dne 27.8.2014 v 16:49 David Kupka napsal(a):
On 08/27/2014 11:22 AM, Jan Cholasta wrote:
Dne 26.8.2014 v 15:55 Rob Crittenden napsal(a):
David Kupka wrote:
On 08/26/2014 03:08 PM, Jan Cholasta wrote:
Hi,
Dne 26.8.2014 v 13:01 David Kupka napsal(a):
https://fedorahosted.org/freeipa/ticket/4481
Doing this will break ipa-client-automount and ipa-certupdate,
because
they assume that api.env.host contains the hostname of the local
system
(which is the default value).
It looked suspiciously simple so I could expect that there is some
catch.
There is obviously some confusion about what the option should
represent
(documentation says server hostname, code does client hostname),
IMO we
should resolve that first.
Ok, are there any suggestions? What is the desired state?
AIUI the server option is deprecated because it wasn't being used, not
that it needed to be replaced. I believe that in most cases the server
name is pulled from the xmlrpc_uri.
Yes, that's what the ticket says:
https://fedorahosted.org/freeipa/ticket/3071.
Ok, adding 'host' entry with local host name.
host has always meant the local host name.
I think the man page is wrong.
+1
Fixing the line in man page.
rob
ACK as long as this works for Nalin.
I see Nalin is OK with the patch, I am not so OK. What should we do with
the server option then? It is still being referred to as Deprecated in
the man page. Should we then un-deprecate it as Honza suggested down the
thread?
Martin
Ok, changed man page. It no longer refer server as deprecated.
--
David Kupka
From 273d68f91dede5fccb70944b2a360865082bf276 Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Wed, 27 Aug 2014 16:02:35 +0200
Subject: [PATCH] Add 'host' setting into default.conf configuration file on
client. Fix description in man page.
'host' setting specifies local hostname not the hostname of IPA server.
https://fedorahosted.org/freeipa/ticket/4481
---
ipa-client/ipa-install/ipa-client-install | 5 +++--
ipa-client/man/default.conf.5 | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index b3da28df19654a2bf676fd7499057828394c9618..45e802207d06a64cc53c581445c9e897c5295c88 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -812,7 +812,7 @@ def uninstall(options, env):
return rv
-def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server):
+def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server, hostname):
ipaconf = ipaclient.ipachangeconf.IPAChangeConf(IPA Installer)
ipaconf.setOptionAssignment( = )
ipaconf.setSectionNameDelimiters(([,]))
@@ -825,6 +825,7 @@ def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server):
{'name':'realm', 'type':'option', 'value':cli_realm},
{'name':'domain', 'type':'option', 'value':cli_domain},
{'name':'server', 'type':'option', 'value':cli_server[0]},
+ {'name':'host', 'type':'option', 'value':hostname},
{'name':'xmlrpc_uri', 'type':'option', 'value':'https://%s/ipa/xml' % ipautil.format_netloc(cli_server[0])},
{'name':'enable_ra', 'type':'option', 'value':'True'}]
@@ -2473,7 +2474,7 @@ def install(options, env, fstore, statestore):
# Configure ipa.conf
if not options.on_master:
-configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server)
+configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server, hostname)
root_logger.info(Created /etc/ipa/default.conf)
api.bootstrap(context='cli_installer', debug=options.debug)
diff --git a/ipa-client/man/default.conf.5 b/ipa-client/man/default.conf.5
index c1ccf109e874907885fc3b51b63507c2b46b64ab..dbc8a5b4647439de4de7c01152d098eb0561e236 100644
--- a/ipa-client/man/default.conf.5
+++ b/ipa-client/man/default.conf.5
@@ -96,7 +96,7 @@ Specifies whether the CA is acting as an RA agent, such as when dogtag is being
Specifies whether an IPA client should attempt to fall back and try other services if the first connection fails.
.TP
.B host hostname
-Specifies the hostname of the IPA server. This value is used to construct URL values on the client and server.
+Specifies the local system hostname.
.TP
.B in_server boolean
Specifies whether requests should be forwarded to an IPA server or handled locally. This is used internally by IPA in a similar way as context. The same IPA framework is used by the ipa command\-line tool and the server. This setting tells the framework whether it should execute the command as if on the server or forward it via XML\-RPC to a remote server.
@@ -164,7 +164,7 @@ Specifies the length of time authentication credentials cached in
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
Dne 26.9.2014 v 12:02 Martin Kosek napsal(a): On 09/23/2014 11:46 AM, Jan Cholasta wrote: Dne 6.8.2014 v 18:17 Jan Cholasta napsal(a): Dne 6.8.2014 v 14:43 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4447. +cert_group.add_option(--ca-key-algorithm, dest=ca_key_algorithm, + help=Key algorithm of the IPA CA certificate (default SHA256withRSA)) Why not set the default here rather than later? CA-related defaults should be internalized in CA-related code IMHO. Should the list of options be added to the man page as well? Sure, why not. Do we want to support the MD*-based signing algorithms? I'd think not. Since the reason this patch exists is to support old and/or broken external CAs, I would think yes, but I don't have a strong opinion on this. Turns out Dogtag does not like them, so I removed them. Seeing the context makes me wonder if we should eventually add options for CA key size and signing alg as well. rob Updated patch attached. I tested the patch (it works fine with Dogtag 10), but I got very confused. What CA option are we setting? Signing algorithm or Key Algorithm? I thought we are only setting Signing algorithm, but in that case: We are setting key algorithm for the CA signing key. - --ca-key-algorithm option should rather read --ca-signing-key-algorithm If you want to emphasize that it is actually the algorithm used to sign the CA certificate, the option should read --ca-certificate-signature-algorithm, but I would rather stick to Dogtag terminology and keep the string key algorithm in the name. - Dogtag9 update should only set --signing_algorithm and not --key_algorithm It should not, because then *all* the certificates issued by the CA would use that algorithm, instead of just the CA certificate. - man page should also be updated with proper explanation. And that would be? Martin -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file
On 09/26/2014 01:37 PM, David Kupka wrote: On 09/26/2014 09:56 AM, Martin Kosek wrote: On 09/02/2014 10:18 AM, Jan Cholasta wrote: Dne 27.8.2014 v 16:49 David Kupka napsal(a): On 08/27/2014 11:22 AM, Jan Cholasta wrote: Dne 26.8.2014 v 15:55 Rob Crittenden napsal(a): David Kupka wrote: On 08/26/2014 03:08 PM, Jan Cholasta wrote: Hi, Dne 26.8.2014 v 13:01 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4481 Doing this will break ipa-client-automount and ipa-certupdate, because they assume that api.env.host contains the hostname of the local system (which is the default value). It looked suspiciously simple so I could expect that there is some catch. There is obviously some confusion about what the option should represent (documentation says server hostname, code does client hostname), IMO we should resolve that first. Ok, are there any suggestions? What is the desired state? AIUI the server option is deprecated because it wasn't being used, not that it needed to be replaced. I believe that in most cases the server name is pulled from the xmlrpc_uri. Yes, that's what the ticket says: https://fedorahosted.org/freeipa/ticket/3071. Ok, adding 'host' entry with local host name. host has always meant the local host name. I think the man page is wrong. +1 Fixing the line in man page. rob ACK as long as this works for Nalin. I see Nalin is OK with the patch, I am not so OK. What should we do with the server option then? It is still being referred to as Deprecated in the man page. Should we then un-deprecate it as Honza suggested down the thread? Martin Ok, changed man page. It no longer refer server as deprecated. LGTM. Pushed to: master: 89c4f1242558d725a1771dce444df5737e49289e ipa-4-1: d82bc63960c22783d8fb56bcca5e21825d9a02cc Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
Dne 24.9.2014 v 18:13 Jan Cholasta napsal(a):
Hi,
the attached patches fix https://fedorahosted.org/freeipa/ticket/4480
and https://fedorahosted.org/freeipa/ticket/4489.
(Note that design page for this is TBD.)
Honza
Polished the code up a bit and rebased on top of current ipa-4-1.
Updated patches attached.
--
Jan Cholasta
From a9226727d304203b76a422e0156eb314b6637475 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Wed, 24 Sep 2014 16:22:32 +0200
Subject: [PATCH 1/4] Add NSSDatabase.import_files method for importing files
in various formats
The files are accepted in PEM and DER certificate, PKCS#7 certificate chain,
PKCS#8 and raw private key and PKCS#12 formats.
---
ipaserver/install/certs.py | 174 +
1 file changed, 174 insertions(+)
diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py
index 4d508cd..bae9f31 100644
--- a/ipaserver/install/certs.py
+++ b/ipaserver/install/certs.py
@@ -195,6 +195,180 @@ class NSSDatabase(object):
raise RuntimeError(unknown error import pkcs#12 file %s %
pkcs12_filename)
+def import_files(self, files, db_password_filename, load_keys=False,
+ key_password=None, key_nickname=None):
+key_file = None
+extracted_key = None
+extracted_certs = ''
+
+for filename in files:
+if load_keys:
+# Try to import file as PKCS#12 file
+try:
+self.import_pkcs12(
+filename, db_password_filename, key_password)
+except RuntimeError:
+pass
+else:
+if key_file:
+raise RuntimeError(
+Can't load private key from both %s and %s %
+(key_file, filename))
+key_file = filename
+
+server_certs = self.find_server_certs()
+if key_nickname:
+for nickname, trust_flags in server_certs:
+if nickname == key_nickname:
+break
+else:
+raise RuntimeError(
+Server certificate \%s\ not found in %s %
+(key_nickname, filename))
+else:
+if len(server_certs) 1:
+raise RuntimeError(
+%s server certificates found in %s,
+expecting only one %
+(len(server_certs), filename))
+
+continue
+
+try:
+with open(filename, 'rb') as f:
+data = f.read()
+except IOError as e:
+raise RuntimeError(
+Failed to open %s: %s % (filename, e.strerror))
+
+# Try to load file as DER certificate
+try:
+x509.load_certificate(data, x509.DER)
+except NSPRError:
+pass
+else:
+data = x509.make_pem(base64.b64encode(data))
+extracted_certs += data + '\n'
+continue
+
+# Try to parse PEM file
+matches = re.finditer(
+r'-BEGIN (.+?)-(.*?)-END \1-', data, re.DOTALL)
+loaded = False
+for match in matches:
+body = match.group()
+label = match.group(1)
+line = len(data[:match.start() + 1].splitlines())
+
+if label in ('CERTIFICATE', 'X509 CERTIFICATE',
+ 'X.509 CERTIFICATE'):
+try:
+x509.load_certificate(match.group(2))
+except NSPRError as e:
+if label != 'CERTIFICATE':
+root_logger.warning(
+Skipping certificate in %s at line %s: %s,
+filename, line, e)
+continue
+else:
+extracted_certs += body + '\n'
+loaded = True
+continue
+
+if label in ('PKCS7', 'PKCS #7 SIGNED DATA', 'CERTIFICATE'):
+args = [
+paths.OPENSSL, 'pkcs7',
+'-print_certs',
+]
+try:
+stdout, stderr, rc = ipautil.run(args, stdin=body)
+except ipautil.CalledProcessError as e:
+if label == 'CERTIFICATE':
+root_logger.warning(
+Skipping certificate in %s at line %s: %s,
+
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
On 09/26/2014 01:41 PM, Jan Cholasta wrote: Dne 26.9.2014 v 12:02 Martin Kosek napsal(a): On 09/23/2014 11:46 AM, Jan Cholasta wrote: Dne 6.8.2014 v 18:17 Jan Cholasta napsal(a): Dne 6.8.2014 v 14:43 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4447. +cert_group.add_option(--ca-key-algorithm, dest=ca_key_algorithm, + help=Key algorithm of the IPA CA certificate (default SHA256withRSA)) Why not set the default here rather than later? CA-related defaults should be internalized in CA-related code IMHO. Should the list of options be added to the man page as well? Sure, why not. Do we want to support the MD*-based signing algorithms? I'd think not. Since the reason this patch exists is to support old and/or broken external CAs, I would think yes, but I don't have a strong opinion on this. Turns out Dogtag does not like them, so I removed them. Seeing the context makes me wonder if we should eventually add options for CA key size and signing alg as well. rob Updated patch attached. I tested the patch (it works fine with Dogtag 10), but I got very confused. What CA option are we setting? Signing algorithm or Key Algorithm? I thought we are only setting Signing algorithm, but in that case: We are setting key algorithm for the CA signing key. That did not made me any less confused... If I check for example fields from certificate details from my browser, I see 2 algorithms names: * Public Key Algorithm (RSA, ECC, ...) * Certificate Signature Algorithm (SHA-1 with RSA, SHA-256 with RSA, something with ECC) In that world, key algorithm should really refer to the key PKI algorithm, i.e. RSA, ECC, ... Signature algorithms is where hashes come to play. - --ca-key-algorithm option should rather read --ca-signing-key-algorithm If you want to emphasize that it is actually the algorithm used to sign the CA certificate, the option should read --ca-certificate-signature-algorithm, but I would rather stick to Dogtag terminology and keep the string key algorithm in the name. I still think for most people key algorithm refers to Public Key algorithm. Rob or Simo, what is your take on this? - Dogtag9 update should only set --signing_algorithm and not --key_algorithm It should not, because then *all* the certificates issued by the CA would use that algorithm, instead of just the CA certificate. Ok. - man page should also be updated with proper explanation. And that would be? That would be something specifically referring to singing. You can also add a note when the option can be used. Whether with --external-ca only or with any CA option. ___ 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/26/2014 10:30 AM, David Kupka wrote: On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 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) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 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) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP
Re: [Freeipa-devel] [PATCH][DOC] Update Solaris Documentation, add proxy agent, and profile
Hello, Just wondering if we have found a reviewer for this patch set? It would be nice to have this as a part of the docs. Thanks, Gabe On Wed, Apr 16, 2014 at 5:13 AM, Petr Spacek pspa...@redhat.com wrote: On 16.4.2014 05:01, Gabe Alford wrote: The following patches update the Solaris documentation and add a proxy agent/profile for Solaris. - Solaris documentation update https://fedorahosted.org/freeipa/ticket/3731 - Patch adds default Proxy Agent and default_secure profile through 20-nss_ldap.update when ipa-server-install is run. https://fedorahosted.org/freeipa/ticket/2561 Thank you Gabe! I think this chapter deserves review from a Solaris expert. Core IPA team doesn't have one so we need to find some victim ... :-) Sigbjorn, would you mind reviewing this patch set? Instructions how to build docs are here: http://www.freeipa.org/page/Contribute/Documentation Thank you very much! -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH][DOC] Update Solaris Documentation, add proxy agent, and profile
I saw a similar interest on freeipa-users list in FreeIPA 3.3 and Solaris 10 Client Integration: thread. Also note it is proposed to only have the guide maintained in the downstream guide in [Freeipa-users] What should we do with upstream guide? thread (I know you know about it). So it is likely the patch would only be applied either in downstream doc or would be even transferred in an article/howto on FreeIPA.org so that Solaris users can easily update it. Martin On 09/26/2014 03:08 PM, Gabe Alford wrote: Hello, Just wondering if we have found a reviewer for this patch set? It would be nice to have this as a part of the docs. Thanks, Gabe On Wed, Apr 16, 2014 at 5:13 AM, Petr Spacek pspa...@redhat.com mailto:pspa...@redhat.com wrote: On 16.4.2014 05:01, Gabe Alford wrote: The following patches update the Solaris documentation and add a proxy agent/profile for Solaris. - Solaris documentation update https://fedorahosted.org/__freeipa/ticket/3731 https://fedorahosted.org/freeipa/ticket/3731 - Patch adds default Proxy Agent and default_secure profile through 20-nss_ldap.update when ipa-server-install is run. https://fedorahosted.org/__freeipa/ticket/2561 https://fedorahosted.org/freeipa/ticket/2561 Thank you Gabe! I think this chapter deserves review from a Solaris expert. Core IPA team doesn't have one so we need to find some victim ... :-) Sigbjorn, would you mind reviewing this patch set? Instructions how to build docs are here: http://www.freeipa.org/page/__Contribute/Documentation http://www.freeipa.org/page/Contribute/Documentation Thank you very much! -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
From 81d3d673944fc61de4616b5572d24719637d1d50 Mon Sep 17 00:00:00 2001
From: Francesco Marella fmare...@gmx.com
Date: Fri, 26 Sep 2014 14:07:25 +0200
Subject: [PATCH] Refactor selinuxenabled check
Ticket: https://fedorahosted.org/freeipa/ticket/4571
---
ipaplatform/fedora/tasks.py | 44 ++--
1 file changed, 18 insertions(+), 26 deletions(-)
diff --git a/ipaplatform/fedora/tasks.py b/ipaplatform/fedora/tasks.py
index 9f4a76b8208cc78c330dc022730c4faac09995f9..439a045c96440342105bad7f9e587398a8894124 100644
--- a/ipaplatform/fedora/tasks.py
+++ b/ipaplatform/fedora/tasks.py
@@ -50,6 +50,20 @@ log = log_mgr.get_logger(__name__)
class FedoraTaskNamespace(BaseTaskNamespace):
+def check_enabled_selinux(self):
+
+Check if SELinux is enabled.
+
+if os.path.exists(paths.SELINUXENABLED):
+try:
+ipautil.run([paths.SELINUXENABLED])
+except ipautil.CalledProcessError:
+# selinuxenabled returns 1 if not enabled
+return False
+else:
+# No selinuxenabled, no SELinux
+return False
+
def restore_context(self, filepath, restorecon=paths.SBIN_RESTORECON):
restore security context on the file path
@@ -59,15 +73,8 @@ class FedoraTaskNamespace(BaseTaskNamespace):
ipautil.run() will do the logging.
-try:
-if os.path.exists(paths.SELINUXENABLED):
-ipautil.run([paths.SELINUXENABLED])
-else:
-# No selinuxenabled, no SELinux
-return
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
-return
+
+self.check_enabled_selinux()
if (os.path.exists(restorecon)):
ipautil.run([restorecon, filepath], raiseonerr=False)
@@ -82,15 +89,7 @@ class FedoraTaskNamespace(BaseTaskNamespace):
This function returns nothing but may raise a Runtime exception
if SELinux is enabled but restorecon is not available.
-try:
-if os.path.exists(paths.SELINUXENABLED):
-ipautil.run([paths.SELINUXENABLED])
-else:
-# No selinuxenabled, no SELinux
-return
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
-return
+self.check_enabled_selinux()
if not os.path.exists(restorecon):
raise RuntimeError('SELinux is enabled but %s does not exist.\n'
@@ -336,14 +335,7 @@ class FedoraTaskNamespace(BaseTaskNamespace):
return args
-if (os.path.exists(paths.SELINUXENABLED)):
-try:
-ipautil.run([paths.SELINUXENABLED])
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
-return False
-else:
-return False
+self.check_enabled_selinux()
updated_vars = {}
failed_vars = {}
--
2.1.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
Hello! Thanks for the patch! The new function is not one of the platform-independent tasks, and doesn't even use `self`, so you can define it as a module-level helper function. But more importantly, this won't work: the blocks you are replacing return from their functions. You'd need to use something like: if not selinux_enabled(): return instead of: self.check_enabled_selinux() -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
On 09/26/2014 03:35 PM, Francesco Marella wrote: ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello, I think that if we want to keep the same previous behaviour, then if 'self.check_enabled_selinux()' returns False, the caller should also return or return False. thanks theirry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH][DOC] Update Solaris Documentation, add proxy agent, and profile
If we can, let's try to push the doc patch 0014 downstream (except for patch 0015 as that patch is an ldif update). Gabe On Fri, Sep 26, 2014 at 7:16 AM, Martin Kosek mko...@redhat.com wrote: I saw a similar interest on freeipa-users list in FreeIPA 3.3 and Solaris 10 Client Integration: thread. Also note it is proposed to only have the guide maintained in the downstream guide in [Freeipa-users] What should we do with upstream guide? thread (I know you know about it). So it is likely the patch would only be applied either in downstream doc or would be even transferred in an article/howto on FreeIPA.org so that Solaris users can easily update it. Martin On 09/26/2014 03:08 PM, Gabe Alford wrote: Hello, Just wondering if we have found a reviewer for this patch set? It would be nice to have this as a part of the docs. Thanks, Gabe On Wed, Apr 16, 2014 at 5:13 AM, Petr Spacek pspa...@redhat.com mailto:pspa...@redhat.com wrote: On 16.4.2014 05:01, Gabe Alford wrote: The following patches update the Solaris documentation and add a proxy agent/profile for Solaris. - Solaris documentation update https://fedorahosted.org/__freeipa/ticket/3731 https://fedorahosted.org/freeipa/ticket/3731 - Patch adds default Proxy Agent and default_secure profile through 20-nss_ldap.update when ipa-server-install is run. https://fedorahosted.org/__freeipa/ticket/2561 https://fedorahosted.org/freeipa/ticket/2561 Thank you Gabe! I think this chapter deserves review from a Solaris expert. Core IPA team doesn't have one so we need to find some victim ... :-) Sigbjorn, would you mind reviewing this patch set? Instructions how to build docs are here: http://www.freeipa.org/page/__Contribute/Documentation http://www.freeipa.org/page/Contribute/Documentation Thank you very much! -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0637 upgradeinstance: Restore listeners on failure
On 09/26/2014 11:46 AM, Martin Kosek wrote:
On 09/25/2014 01:24 PM, Martin Kosek wrote:
On 09/24/2014 10:43 AM, Martin Kosek wrote:
On 08/22/2014 06:07 PM, Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/4499
Actually I wonder why we use backup_state/restore_state for these
settings.
Rob, was there a reason for not just always setting nsslapd-port:
389 and
nsslapd-security: on?
This works pretty nicely, I liked the service.py extension.
My test output:
# ipa-ldap-updater --upgrade
Upgrading IPA:
[1/10]: stopping directory server
[2/10]: saving configuration
[3/10]: disabling listeners
[4/10]: starting directory server
[5/10]: preparing server upgrade
PRE_SCHEMA_UPDATE
[6/10]: updating schema
...
[7/10]: upgrading server
[error] ValueError: Ha!
[cleanup]: stopping directory server
[cleanup]: restoring configuration
# ipactl start
# netstat -putna | grep 389
...
tcp6 0 0 10.16.78.147:38910.16.78.147:37490
ESTABLISHED
5956/ns-slapd
So I am willing to ACK if there are no other objections.
Martin
I read the silence as no objections, so ACK.
Pushed to:
master: 9a188607fcf68721fc8c38c3c73ee02cac76b58a
ipa-4-1: b333e7adc98ff7c5335fbc7ce1bde5b9dfb3f5ef
Martin
Just found (benign) display issue with this patch when --external-ca is
used. See:
https://fedorahosted.org/freeipa/ticket/4499#comment:4
Martin
You're right. This patch should fix that.
--
Petr³
From 93862732e9c05ce8b96e25daf4c71ae5a1f1fa82 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 26 Sep 2014 12:43:18 +0200
Subject: [PATCH] ipaserver.install.service: Don't show error message on
SystemExit(0)
Additional fix for: https://fedorahosted.org/freeipa/ticket/4499
---
ipaserver/install/service.py | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index d095fe041bec8098937bf5b99b301a41842ce53e..d2556dccc873d72d9209de379eca86a0b572da29 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -370,14 +370,15 @@ def run_step(message, method):
run_step(full_msg, method)
step += 1
except BaseException as e:
-# show the traceback, so it's not lost if the cleanup method fails
-root_logger.debug(%s % traceback.format_exc())
-self.print_msg(' [error] %s: %s' % (type(e).__name__, e))
+if not (isinstance(e, SystemExit) and e.code == 0):
+# show the traceback, so it's not lost if cleanup method fails
+root_logger.debug(%s % traceback.format_exc())
+self.print_msg(' [error] %s: %s' % (type(e).__name__, e))
-# run through remaining methods marked run_after_failure
-for message, method, run_after_failure in steps_iter:
-if run_after_failure:
-run_step( [cleanup]: %s % message, method)
+# run through remaining methods marked run_after_failure
+for message, method, run_after_failure in steps_iter:
+if run_after_failure:
+run_step( [cleanup]: %s % message, method)
raise
--
1.9.3
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
On 26/09/2014 15:41, Petr Viktorin wrote:
Hello! Thanks for the patch!
The new function is not one of the platform-independent tasks, and
doesn't even use `self`, so you can define it as a module-level helper
function.
But more importantly, this won't work: the blocks you are replacing
return from their functions. You'd need to use something like:
if not selinux_enabled():
return
instead of:
self.check_enabled_selinux()
From d7422bb4de03219ec8e8326b04bde4c5e7fe4b02 Mon Sep 17 00:00:00 2001
From: Francesco Marella fmare...@gmx.com
Date: Fri, 26 Sep 2014 14:07:25 +0200
Subject: [PATCH] Refactor selinuxenabled check
Ticket: https://fedorahosted.org/freeipa/ticket/4571
---
ipaplatform/fedora/tasks.py | 44
1 file changed, 20 insertions(+), 24 deletions(-)
diff --git a/ipaplatform/fedora/tasks.py b/ipaplatform/fedora/tasks.py
index 9f4a76b8208cc78c330dc022730c4faac09995f9..7387dc920ab04023dfa8a79fc417ff9091bc0a98 100644
--- a/ipaplatform/fedora/tasks.py
+++ b/ipaplatform/fedora/tasks.py
@@ -48,6 +48,21 @@ from ipaplatform.base.tasks import BaseTaskNamespace
log = log_mgr.get_logger(__name__)
+def selinux_enabled():
+
+Check if SELinux is enabled.
+
+if os.path.exists(paths.SELINUXENABLED):
+try:
+ipautil.run([paths.SELINUXENABLED])
+except ipautil.CalledProcessError:
+# selinuxenabled returns 1 if not enabled
+return False
+else:
+# No selinuxenabled, no SELinux
+return False
+
+
class FedoraTaskNamespace(BaseTaskNamespace):
def restore_context(self, filepath, restorecon=paths.SBIN_RESTORECON):
@@ -59,14 +74,8 @@ class FedoraTaskNamespace(BaseTaskNamespace):
ipautil.run() will do the logging.
-try:
-if os.path.exists(paths.SELINUXENABLED):
-ipautil.run([paths.SELINUXENABLED])
-else:
-# No selinuxenabled, no SELinux
-return
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
+
+if not selinux_enabled():
return
if (os.path.exists(restorecon)):
@@ -82,14 +91,7 @@ class FedoraTaskNamespace(BaseTaskNamespace):
This function returns nothing but may raise a Runtime exception
if SELinux is enabled but restorecon is not available.
-try:
-if os.path.exists(paths.SELINUXENABLED):
-ipautil.run([paths.SELINUXENABLED])
-else:
-# No selinuxenabled, no SELinux
-return
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
+if not selinux_enabled():
return
if not os.path.exists(restorecon):
@@ -336,14 +338,8 @@ class FedoraTaskNamespace(BaseTaskNamespace):
return args
-if (os.path.exists(paths.SELINUXENABLED)):
-try:
-ipautil.run([paths.SELINUXENABLED])
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
-return False
-else:
-return False
+if not selinux_enabled():
+return
updated_vars = {}
failed_vars = {}
--
2.1.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 314 Allow specifying key algorithm of the IPA CA cert in ipa-server-install
On Fri, 26 Sep 2014 13:54:34 +0200 Martin Kosek mko...@redhat.com wrote: I tested the patch (it works fine with Dogtag 10), but I got very confused. What CA option are we setting? Signing algorithm or Key Algorithm? I thought we are only setting Signing algorithm, but in that case: We are setting key algorithm for the CA signing key. That did not made me any less confused... If I check for example fields from certificate details from my browser, I see 2 algorithms names: * Public Key Algorithm (RSA, ECC, ...) * Certificate Signature Algorithm (SHA-1 with RSA, SHA-256 with RSA, something with ECC) In that world, key algorithm should really refer to the key PKI algorithm, i.e. RSA, ECC, ... Signature algorithms is where hashes come to play. - --ca-key-algorithm option should rather read --ca-signing-key-algorithm If you want to emphasize that it is actually the algorithm used to sign the CA certificate, the option should read --ca-certificate-signature-algorithm, but I would rather stick to Dogtag terminology and keep the string key algorithm in the name. I still think for most people key algorithm refers to Public Key algorithm. Rob or Simo, what is your take on this? If we are defining the signing algorithm the signing string should be somewhere in the option. Having just --key-algorithm is indeed confusing. 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] 0637 upgradeinstance: Restore listeners on failure
On 09/26/2014 04:14 PM, Petr Viktorin wrote: On 09/26/2014 11:46 AM, Martin Kosek wrote: On 09/25/2014 01:24 PM, Martin Kosek wrote: On 09/24/2014 10:43 AM, Martin Kosek wrote: On 08/22/2014 06:07 PM, Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/4499 Actually I wonder why we use backup_state/restore_state for these settings. Rob, was there a reason for not just always setting nsslapd-port: 389 and nsslapd-security: on? This works pretty nicely, I liked the service.py extension. My test output: # ipa-ldap-updater --upgrade Upgrading IPA: [1/10]: stopping directory server [2/10]: saving configuration [3/10]: disabling listeners [4/10]: starting directory server [5/10]: preparing server upgrade PRE_SCHEMA_UPDATE [6/10]: updating schema ... [7/10]: upgrading server [error] ValueError: Ha! [cleanup]: stopping directory server [cleanup]: restoring configuration # ipactl start # netstat -putna | grep 389 ... tcp6 0 0 10.16.78.147:38910.16.78.147:37490 ESTABLISHED 5956/ns-slapd So I am willing to ACK if there are no other objections. Martin I read the silence as no objections, so ACK. Pushed to: master: 9a188607fcf68721fc8c38c3c73ee02cac76b58a ipa-4-1: b333e7adc98ff7c5335fbc7ce1bde5b9dfb3f5ef Martin Just found (benign) display issue with this patch when --external-ca is used. See: https://fedorahosted.org/freeipa/ticket/4499#comment:4 Martin You're right. This patch should fix that. It does! ACK. Pushed to: master: f86618623964f9a97244ce08117c575b200a34af ipa-4-1: 540f4166e45752ae74f652f66eec9b92413a5d1e Martin ___ 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/26/2014 02:47 PM, David Kupka wrote: On 09/26/2014 10:30 AM, David Kupka wrote: On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 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) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 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) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.,
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
On 09/24/2014 06:13 PM, Jan Cholasta wrote:
Hi,
the attached patches fix https://fedorahosted.org/freeipa/ticket/4480
and https://fedorahosted.org/freeipa/ticket/4489.
(Note that design page for this is TBD.)
Honza
336:
Instead of
len(data[:match.start() + 1].splitlines())
you can do
data.count('\n', 0, match.start()) + 1
337:
The --external_cert_file and --external_ca_file options for
ipa-ca-install are removed, do we really want to do that? Shouldn't they
be deprecated instead?
Same for --external-ca-file in ipa-cacert-manage.
338: Looks OK
339: Looks OK
Could you add some docstrings to the functions you add? Sometimes it's
harder than necessary to decipher what they do and what the
arguments/return values mean exactly.
There is no user-visible documentation on what file types are
expected/supported. It would be good to add this to the man pages, or
the --help.
In external CA, the error message when specifying a certificate but not
the CA could be improved:
$ ipa-server-install --external_cert_file ~/p/Certificate_Authority_8.cer
...
CA certificate CN=Certificate Authority,O=IDM.LAB.ENG.BRQ.REDHAT.COM in
/home/pviktori/p/Certificate_Authority_8.cer is not valid:
(SEC_ERROR_UNKNOWN_ISSUER) Peer's Certificate issuer is not recognized.
For CA-less, I used a combination of files with which server
installation went well, but replica-install failed halfway through:
Console:
...
[16/36]: creating indices
[17/36]: enabling referential integrity plugin
[18/36]: configuring ssl for ds instance
[error] RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
Log tail:
2014-09-26T15:05:43Z DEBUG Starting external process
2014-09-26T15:05:43Z DEBUG args='/usr/bin/pk12util' '-d'
'/etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM/' '-i'
'/tmp/tmp2vEWX_ipa/realm_info/dscert.p12' '-k'
'/etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM//pwdfile.txt' '-v' '-w'
'/dev/stdin'
2014-09-26T15:05:43Z DEBUG Process finished, return code=17
2014-09-26T15:05:43Z DEBUG stdout=
2014-09-26T15:05:43Z DEBUG stderr=pk12util: PKCS12 decode not verified:
SEC_ERROR_BAD_PASSWORD: The security password entered is incorrect.
2014-09-26T15:05:43Z DEBUG Traceback (most recent call last):
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 370, in start_creation
run_step(full_msg, method)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 360, in run_step
method()
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
600, in __enable_ssl
trust_flags=trust_flags)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 1030, in create_from_pkcs12
self.import_pkcs12(pkcs12_fname, pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 971, in import_pkcs12
pkcs12_passwd=pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 191, in import_pkcs12
pkcs12_filename)
RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
2014-09-26T15:05:43Z DEBUG [error] RuntimeError: incorrect password
for pkcs#12 file /tmp/tmp2vEWX_ipa/realm_info/dscert.p12
2014-09-26T15:05:43Z DEBUG File
/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py,
line 644, in run_script
return_value = main_function()
File /sbin/ipa-replica-install, line 677, in main
ds = install_replica_ds(config)
File /sbin/ipa-replica-install, line 190, in install_replica_ds
ca_file=config.dir + /ca.crt,
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
354, in create_replica
self.start_creation(runtime=60)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 370, in start_creation
run_step(full_msg, method)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 360, in run_step
method()
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
600, in __enable_ssl
trust_flags=trust_flags)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 1030, in create_from_pkcs12
self.import_pkcs12(pkcs12_fname, pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 971, in import_pkcs12
pkcs12_passwd=pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 191, in import_pkcs12
pkcs12_filename)
2014-09-26T15:05:43Z DEBUG The ipa-replica-install command failed,
exception: RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
I'll attach the files for reference; the options for ipa-server-install
and ipa-replica-prepare were:
--http-cert-file=~/STAR.idm.lab.eng.brq.redhat.com_3.p12-nocacerts.p12
--http-cert-file
~/STAR.idm.lab.eng.brq.redhat.com_3.p12-allcerts-x509.pem --http-pin
12345678
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. When adding the CA certificates to the temporary database it will report that a failure occurred, but not the exception: +except CalledProcessError, e: +root_logger.info(Failed to add CA to temporary NSS database.) +return CLIENT_INSTALL_ERROR Granted, NSS errors are often obtuse, but should it at least DEBUG log it? 324, 325, 326: ACK 327: So the idea is to just mirror the certs and us the new db to know what was added? What if someone has the same nickname, different cert in the two databases? Is that too much of a corner case? 328, 329, 330, 331: ACK As an aside, do you know why the global certs are seen by mod_nss? libnssckbi.so is symlinked into the directory (certutil -L -d /etc/httpd/alias -h all will show all the certs). 332-335 I think the naming and/or comments can be improved. For example, there are now 3 *retrieve_cert commands, all of which do slightly different things. All have external handlers (via the oddly named profile), but some are called internally as well. This is rather complex code: a command passes a call onto certmonger which then exuecutes the IPA CA helper... More documentation would definitely help a newcomer figure out how renewal, CA retrieval, etc. works. Not to be too pedantic but there is a lot of this in dogtag-ipa-ca-renew-agent-submit: if variable: OR if not variable: Where variable defaults to None. Shouldn't the test be: if variable is [not] None: 340: ACK Through some combination of ipa-certupdate and ipa-cacert-manage I seem to have hosed things up: ipa: DEBUG: certmonger request is in state dbus.String(u'CA_UNREACHABLE', variant_level=1) ipa.ipaserver.install.ipa_cacert_manage.CACertManage: DEBUG: File /usr/lib/python2.7/site-packages/ipapython/admintool.py, line 171, in execute return_value = self.run() File /usr/lib/python2.7/site-packages/ipaserver/install/ipa_cacert_manage.py, line 118, in run rc = self.renew() File /usr/lib/python2.7/site-packages/ipaserver/install/ipa_cacert_manage.py, line 181, in renew return self.renew_self_signed(ca) File /usr/lib/python2.7/site-packages/ipaserver/install/ipa_cacert_manage.py, line 193, in renew_self_signed self.resubmit_request(ca, 'caCACert') File /usr/lib/python2.7/site-packages/ipaserver/install/ipa_cacert_manage.py, line 315, in resubmit_request please check the request manually % self.request_id) ipa.ipaserver.install.ipa_cacert_manage.CACertManage: DEBUG: The ipa-cacert-manage command failed, exception: ScriptError: Error resubmitting certmonger request '20140909142830', please check the request manually ipa.ipaserver.install.ipa_cacert_manage.CACertManage: ERROR: Error resubmitting certmonger request '20140909142830', please check the request manually Incidentally, IMHO it should include the command to execute to check the request manually. The CA is actually unreachable. Restarting it fixed things. I'll chalk this up to cosmic rays. Re-running ipa-cacert-manage renew was successful. I confirmed that the CA signing cert was updated in the dogtag database. I then ran ipa-certupdate to distribute this new CA cert around and none of the databases got the updated CA cert, nor did /etc/ipa/ca.crt. Still doing some functional testing. Unrelated to this: 9:freeipa-python-4.0.0GITf6875d4-0.# [100%] Update failed: Operations error: Seems to be related to this: 2014-09-25T19:28:22Z DEBUG - 2014-09-25T19:28:22Z DEBUG Final value after applying
Re: [Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
This should be the final one.
fm
On 26/09/2014 16:30, Francesco Marella wrote:
On 26/09/2014 15:41, Petr Viktorin wrote:
Hello! Thanks for the patch!
The new function is not one of the platform-independent tasks, and
doesn't even use `self`, so you can define it as a module-level
helper function.
But more importantly, this won't work: the blocks you are replacing
return from their functions. You'd need to use something like:
if not selinux_enabled():
return
instead of:
self.check_enabled_selinux()
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
From bf7fc14de136b3e57b210c32163b78e4162f0b29 Mon Sep 17 00:00:00 2001
From: Francesco Marella fmare...@gmx.com
Date: Fri, 26 Sep 2014 14:07:25 +0200
Subject: [PATCH] Refactor selinuxenabled check
Ticket: https://fedorahosted.org/freeipa/ticket/4571
---
ipaplatform/fedora/tasks.py | 45 +
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/ipaplatform/fedora/tasks.py b/ipaplatform/fedora/tasks.py
index 9f4a76b8208cc78c330dc022730c4faac09995f9..2e5ca714f85c3756caf17c054beb4a680c58a98d 100644
--- a/ipaplatform/fedora/tasks.py
+++ b/ipaplatform/fedora/tasks.py
@@ -48,6 +48,22 @@ from ipaplatform.base.tasks import BaseTaskNamespace
log = log_mgr.get_logger(__name__)
+def selinux_enabled():
+
+Check if SELinux is enabled.
+
+if os.path.exists(paths.SELINUXENABLED):
+try:
+ipautil.run([paths.SELINUXENABLED])
+return True
+except ipautil.CalledProcessError:
+# selinuxenabled returns 1 if not enabled
+return False
+else:
+# No selinuxenabled, no SELinux
+return False
+
+
class FedoraTaskNamespace(BaseTaskNamespace):
def restore_context(self, filepath, restorecon=paths.SBIN_RESTORECON):
@@ -59,14 +75,8 @@ class FedoraTaskNamespace(BaseTaskNamespace):
ipautil.run() will do the logging.
-try:
-if os.path.exists(paths.SELINUXENABLED):
-ipautil.run([paths.SELINUXENABLED])
-else:
-# No selinuxenabled, no SELinux
-return
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
+
+if not selinux_enabled():
return
if (os.path.exists(restorecon)):
@@ -82,14 +92,7 @@ class FedoraTaskNamespace(BaseTaskNamespace):
This function returns nothing but may raise a Runtime exception
if SELinux is enabled but restorecon is not available.
-try:
-if os.path.exists(paths.SELINUXENABLED):
-ipautil.run([paths.SELINUXENABLED])
-else:
-# No selinuxenabled, no SELinux
-return
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
+if not selinux_enabled():
return
if not os.path.exists(restorecon):
@@ -336,14 +339,8 @@ class FedoraTaskNamespace(BaseTaskNamespace):
return args
-if (os.path.exists(paths.SELINUXENABLED)):
-try:
-ipautil.run([paths.SELINUXENABLED])
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
-return False
-else:
-return False
+if not selinux_enabled():
+return
updated_vars = {}
failed_vars = {}
--
2.1.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
Petr Viktorin wrote:
On 09/24/2014 06:13 PM, Jan Cholasta wrote:
Hi,
the attached patches fix https://fedorahosted.org/freeipa/ticket/4480
and https://fedorahosted.org/freeipa/ticket/4489.
(Note that design page for this is TBD.)
Isn't this backwards then?
336:
Instead of
len(data[:match.start() + 1].splitlines())
you can do
data.count('\n', 0, match.start()) + 1
337:
The --external_cert_file and --external_ca_file options for
ipa-ca-install are removed, do we really want to do that? Shouldn't they
be deprecated instead?
+1
Same for --external-ca-file in ipa-cacert-manage.
+1
I can't say I'm a fan of forcing users to concatenate cert files.
338: Looks OK
339: Looks OK
Could you add some docstrings to the functions you add? Sometimes it's
harder than necessary to decipher what they do and what the
arguments/return values mean exactly.
There is no user-visible documentation on what file types are
expected/supported. It would be good to add this to the man pages, or
the --help.
I also wonder if the detection code should be changed. It basically now
tries a slew of different mechanisms one at a time rather than trying to
identify the type of file and using that one. It may not be possible in
all cases but you could at least start by looking for ^- to know it
is a text file and go from there, otherwise step through the binary formats.
In external CA, the error message when specifying a certificate but not
the CA could be improved:
$ ipa-server-install --external_cert_file ~/p/Certificate_Authority_8.cer
...
CA certificate CN=Certificate Authority,O=IDM.LAB.ENG.BRQ.REDHAT.COM in
/home/pviktori/p/Certificate_Authority_8.cer is not valid:
(SEC_ERROR_UNKNOWN_ISSUER) Peer's Certificate issuer is not recognized.
For CA-less, I used a combination of files with which server
installation went well, but replica-install failed halfway through:
Console:
...
[16/36]: creating indices
[17/36]: enabling referential integrity plugin
[18/36]: configuring ssl for ds instance
[error] RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
Log tail:
2014-09-26T15:05:43Z DEBUG Starting external process
2014-09-26T15:05:43Z DEBUG args='/usr/bin/pk12util' '-d'
'/etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM/' '-i'
'/tmp/tmp2vEWX_ipa/realm_info/dscert.p12' '-k'
'/etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM//pwdfile.txt' '-v' '-w'
'/dev/stdin'
2014-09-26T15:05:43Z DEBUG Process finished, return code=17
2014-09-26T15:05:43Z DEBUG stdout=
2014-09-26T15:05:43Z DEBUG stderr=pk12util: PKCS12 decode not verified:
SEC_ERROR_BAD_PASSWORD: The security password entered is incorrect.
2014-09-26T15:05:43Z DEBUG Traceback (most recent call last):
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 370, in start_creation
run_step(full_msg, method)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 360, in run_step
method()
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
600, in __enable_ssl
trust_flags=trust_flags)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 1030, in create_from_pkcs12
self.import_pkcs12(pkcs12_fname, pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 971, in import_pkcs12
pkcs12_passwd=pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 191, in import_pkcs12
pkcs12_filename)
RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
2014-09-26T15:05:43Z DEBUG [error] RuntimeError: incorrect password
for pkcs#12 file /tmp/tmp2vEWX_ipa/realm_info/dscert.p12
2014-09-26T15:05:43Z DEBUG File
/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py,
line 644, in run_script
return_value = main_function()
File /sbin/ipa-replica-install, line 677, in main
ds = install_replica_ds(config)
File /sbin/ipa-replica-install, line 190, in install_replica_ds
ca_file=config.dir + /ca.crt,
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
354, in create_replica
self.start_creation(runtime=60)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 370, in start_creation
run_step(full_msg, method)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 360, in run_step
method()
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
600, in __enable_ssl
trust_flags=trust_flags)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 1030, in create_from_pkcs12
self.import_pkcs12(pkcs12_fname, pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 971, in import_pkcs12
pkcs12_passwd=pkcs12_passwd)
File
Re: [Freeipa-devel] [PATCH] 749-754 webui: new ID views section
On 25.9.2014 19:07, Petr Vobornik wrote:
All issues will be done separately as already stated in other
sub-thread. I've removed issues which are discussed in the other
sub-thread.
2. The tab titles in the ID view details page are quite long, and the
User ID overrides and Group ID overrides labels aren't quite
appropriate because the ID view can override other attributes too. How
about using facet groups like in User Groups? For example:
- ID view applies to:
- Hosts
- ID view overrides:
- Users
- Groups
- Settings
Attached patch 758
7. Related to #6, there probably should be a tab in the Host details
page showing which ID views apply to it.
There is only a single view and yes, it would be good to add a property
there, linking it to the ID view tab, if possible.
Will add simple readonly field (link to view). It will be improved later
(based on ipa-4-1 priorities)
Attached patch 760
9. This probably requires server support. In the Apply to hosts
association dialog, if a host is already added it will still appear in
the dialog box. As a comparison, a User that has been added into a
User Group will not appear in the association dialog anymore.
Could be trivially filtered out on Web UI side.
Will be implemented.
Attached patch 759 - quite ugly hack but does the job
--
Petr Vobornik
From b3c3791b78d1364e59f65d11976d83d5f278a437 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Fri, 26 Sep 2014 17:21:00 +0200
Subject: [PATCH] webui: add link from host to idview
https://fedorahosted.org/freeipa/ticket/4535
---
install/ui/src/freeipa/host.js | 7 +++
install/ui/src/freeipa/util.js | 17 -
install/ui/src/freeipa/widget.js | 15 +++
install/ui/test/data/ipa_init.json | 1 +
ipalib/plugins/internal.py | 1 +
5 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/install/ui/src/freeipa/host.js b/install/ui/src/freeipa/host.js
index 692441f6b7bdb3dd96bafa87451955b8d5e3a3f4..5b886b6394e73533d73f0d1a3d800922e4ef3e4d 100644
--- a/install/ui/src/freeipa/host.js
+++ b/install/ui/src/freeipa/host.js
@@ -112,6 +112,13 @@ return {
$type: 'checkbox',
acl_param: 'krbticketflags',
flags: ['w_if_no_aci']
+},
+{
+name: 'ipaassignedidview',
+$type: 'link',
+label: '@i18n:objects.idview.ipaassignedidview',
+ui_formatter: 'dn',
+other_entity: 'idview'
}
]
},
diff --git a/install/ui/src/freeipa/util.js b/install/ui/src/freeipa/util.js
index 0032c8ace38c1c449589d558c6ee6cb3286bc54a..adebe2cdb397a1728d578c53a0f87a88f6f1f8e1 100644
--- a/install/ui/src/freeipa/util.js
+++ b/install/ui/src/freeipa/util.js
@@ -241,6 +241,13 @@ define([
return els;
}
+function get_val_from_dn(dn, pos) {
+if (!dn) return '';
+pos = pos === undefined ? 0 : pos;
+var val = dn.split(',')[pos].split('=')[1];
+return val;
+}
+
/**
* Module with utility functions
* @class
@@ -362,7 +369,15 @@ define([
* @param {string} text
* @return {Array} array of jQuery elements
*/
-beautify_message: beautify_message
+beautify_message: beautify_message,
+
+/**
+ * Return value of part of DN on specified position
+ * @param {string} dn Distinguished name
+ * @param {Number} [position=0] Zero-based DN part position
+ * @return {string}
+ */
+get_val_from_dn: get_val_from_dn
};
return util;
diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index c7a082b187e06221846487fbb137811b9edb5e55..b9ab82cbceff5a8cef12828b0d4006fea2cf578e 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -2457,6 +2457,20 @@ IPA.datetime_formatter = function(spec) {
};
/**
+ * Format DN into single pkey
+ * @class
+ * @extends IPA.formatter
+ */
+IPA.dn_formatter = function(spec) {
+
+var that = IPA.formatter(spec);
+that.format = function(value) {
+return util.get_val_from_dn(value);
+};
+return that;
+};
+
+/**
* Column for {@link IPA.table_widget}
*
* Handles value rendering.
@@ -6180,6 +6194,7 @@ exp.register = function() {
f.register('boolean', IPA.boolean_formatter);
f.register('boolean_status', IPA.boolean_status_formatter);
f.register('datetime', IPA.datetime_formatter);
+f.register('dn', IPA.dn_formatter);
};
phases.on('registration', exp.register);
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index 9b6a528e4e72a4efc586ca87a71c7e6f1eaa47a6..f40ff14dfb3ecae40e6921da29ce3e2916121268 100644
---
Re: [Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
Hello, When called from set_selinux_booleans, if not selinux_enabled, you may want to 'return False' rather than 'return'. Now it looks like callers of set_selinux_booleans do not check the returned value :-) thanks thierry On 09/26/2014 05:26 PM, Francesco Marella wrote: This should be the final one. fm On 26/09/2014 16:30, Francesco Marella wrote: On 26/09/2014 15:41, Petr Viktorin wrote: Hello! Thanks for the patch! The new function is not one of the platform-independent tasks, and doesn't even use `self`, so you can define it as a module-level helper function. But more importantly, this won't work: the blocks you are replacing return from their functions. You'd need to use something like: if not selinux_enabled(): return instead of: self.check_enabled_selinux() ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ 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 26/09/14 14:47, David Kupka wrote: On 09/26/2014 10:30 AM, David Kupka wrote: On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 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) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 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) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label
Re: [Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
On 26/09/2014 17:43, thierry bordaz wrote:
Hello,
When called from set_selinux_booleans, if not selinux_enabled, you
may want to 'return False' rather than 'return'.
Now it looks like callers of set_selinux_booleans do not check the
returned value :-)
thanks
thierry
On 09/26/2014 05:26 PM, Francesco Marella wrote:
This should be the final one.
fm
On 26/09/2014 16:30, Francesco Marella wrote:
On 26/09/2014 15:41, Petr Viktorin wrote:
Hello! Thanks for the patch!
The new function is not one of the platform-independent tasks, and
doesn't even use `self`, so you can define it as a module-level
helper function.
But more importantly, this won't work: the blocks you are replacing
return from their functions. You'd need to use something like:
if not selinux_enabled():
return
instead of:
self.check_enabled_selinux()
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
From 5463ce197addb741ff728be2f2f11609b71e9565 Mon Sep 17 00:00:00 2001
From: Francesco Marella fmare...@gmx.com
Date: Fri, 26 Sep 2014 14:07:25 +0200
Subject: [PATCH] Refactor selinuxenabled check
Ticket: https://fedorahosted.org/freeipa/ticket/4571
---
ipaplatform/fedora/tasks.py | 43 ---
1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/ipaplatform/fedora/tasks.py b/ipaplatform/fedora/tasks.py
index 9f4a76b8208cc78c330dc022730c4faac09995f9..d1d4abb97d84404cd65ade9ed411a6d1503c9125 100644
--- a/ipaplatform/fedora/tasks.py
+++ b/ipaplatform/fedora/tasks.py
@@ -48,6 +48,22 @@ from ipaplatform.base.tasks import BaseTaskNamespace
log = log_mgr.get_logger(__name__)
+def selinux_enabled():
+
+Check if SELinux is enabled.
+
+if os.path.exists(paths.SELINUXENABLED):
+try:
+ipautil.run([paths.SELINUXENABLED])
+return True
+except ipautil.CalledProcessError:
+# selinuxenabled returns 1 if not enabled
+return False
+else:
+# No selinuxenabled, no SELinux
+return False
+
+
class FedoraTaskNamespace(BaseTaskNamespace):
def restore_context(self, filepath, restorecon=paths.SBIN_RESTORECON):
@@ -59,14 +75,8 @@ class FedoraTaskNamespace(BaseTaskNamespace):
ipautil.run() will do the logging.
-try:
-if os.path.exists(paths.SELINUXENABLED):
-ipautil.run([paths.SELINUXENABLED])
-else:
-# No selinuxenabled, no SELinux
-return
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
+
+if not selinux_enabled():
return
if (os.path.exists(restorecon)):
@@ -82,14 +92,7 @@ class FedoraTaskNamespace(BaseTaskNamespace):
This function returns nothing but may raise a Runtime exception
if SELinux is enabled but restorecon is not available.
-try:
-if os.path.exists(paths.SELINUXENABLED):
-ipautil.run([paths.SELINUXENABLED])
-else:
-# No selinuxenabled, no SELinux
-return
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
+if not selinux_enabled():
return
if not os.path.exists(restorecon):
@@ -336,13 +339,7 @@ class FedoraTaskNamespace(BaseTaskNamespace):
return args
-if (os.path.exists(paths.SELINUXENABLED)):
-try:
-ipautil.run([paths.SELINUXENABLED])
-except ipautil.CalledProcessError:
-# selinuxenabled returns 1 if not enabled
-return False
-else:
+if not selinux_enabled():
return False
updated_vars = {}
--
2.1.1
___
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/26/2014 05:50 PM, Martin Basti wrote: On 26/09/14 14:47, David Kupka wrote: On 09/26/2014 10:30 AM, David Kupka wrote: On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 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) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 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) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.,
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/26/2014 05:50 PM, Martin Basti wrote: On 26/09/14 14:47, David Kupka wrote: On 09/26/2014 10:30 AM, David Kupka wrote: On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 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) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 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) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.,
Re: [Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
On 09/26/2014 05:53 PM, Francesco Marella wrote: On 26/09/2014 17:43, thierry bordaz wrote: Hello, When called from set_selinux_booleans, if not selinux_enabled, you may want to 'return False' rather than 'return'. Now it looks like callers of set_selinux_booleans do not check the returned value :-) thanks thierry On 09/26/2014 05:26 PM, Francesco Marella wrote: This should be the final one. fm On 26/09/2014 16:30, Francesco Marella wrote: On 26/09/2014 15:41, Petr Viktorin wrote: Hello! Thanks for the patch! The new function is not one of the platform-independent tasks, and doesn't even use `self`, so you can define it as a module-level helper function. But more importantly, this won't work: the blocks you are replacing return from their functions. You'd need to use something like: if not selinux_enabled(): return instead of: self.check_enabled_selinux() ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Thanks Francesco ! For me it is a ACK. thanks theirry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... When adding the CA certificates to the temporary database it will report that a failure occurred, but not the exception: +except CalledProcessError, e: +root_logger.info(Failed to add CA to temporary NSS database.) +return CLIENT_INSTALL_ERROR Granted, NSS errors are often obtuse, but should it at least DEBUG log it? It is already logged in ipautil.run. The exception only says that the command failed, there's no point in logging it. 324, 325, 326: ACK 327: So the idea is to just mirror the certs and us the new db to know what was added? Exactly. What if someone has the same nickname, different cert in the two databases? Is that too much of a corner case? IMO it is too much of a corner case, but I plan on adding a better diff/patch algorithm in the future if it turns out to be a problem. 328, 329, 330, 331: ACK As an aside, do you know why the global certs are seen by mod_nss? libnssckbi.so is symlinked into the directory (certutil -L -d /etc/httpd/alias -h all will show all the certs). I'm not sure why it is this way, but the module is linked to the NSS database: $ sudo modutil -list -dbdir /etc/httpd/alias Listing of PKCS #11 Modules --- 1. NSS Internal PKCS #11 Module slots: 2 slots attached status: loaded slot: NSS Internal Cryptographic Services token: NSS Generic Crypto Services slot: NSS User Private Key and Certificate Services token: NSS Certificate DB 2. Root Certs library name: /etc/httpd/alias/libnssckbi.so slots: 2 slots attached status: loaded slot: /etc/pki/ca-trust/source token: System Trust slot: /usr/share/pki/ca-trust-source token: Default Trust --- 332-335 I think the naming and/or comments can be improved. For example, there are now 3 *retrieve_cert commands, all of which do slightly different things. All have external handlers (via the oddly named profile), but some are called internally as well. I have spent quite some time trying to come up with good names for them, but I was not able to do so. Suggestions are welcome. This is rather complex code: a command passes a call onto certmonger which then exuecutes the IPA CA helper... More documentation would definitely help a newcomer figure out how renewal, CA retrieval, etc. works. OK, I'll try to add some. Not to be too pedantic but there is a lot of this in dogtag-ipa-ca-renew-agent-submit: if variable: OR if not variable: Where variable defaults to None. Shouldn't the test be: if variable is [not] None: This doesn't catch empty strings, which may occur in some of these checks. 340: ACK Through some combination of ipa-certupdate and ipa-cacert-manage I seem to have hosed things up: ipa: DEBUG: certmonger request is in state dbus.String(u'CA_UNREACHABLE', variant_level=1) ipa.ipaserver.install.ipa_cacert_manage.CACertManage: DEBUG: File /usr/lib/python2.7/site-packages/ipapython/admintool.py, line 171, in execute return_value = self.run() File /usr/lib/python2.7/site-packages/ipaserver/install/ipa_cacert_manage.py, line
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
Dne 26.9.2014 v 17:37 Rob Crittenden napsal(a):
Petr Viktorin wrote:
On 09/24/2014 06:13 PM, Jan Cholasta wrote:
Hi,
the attached patches fix https://fedorahosted.org/freeipa/ticket/4480
and https://fedorahosted.org/freeipa/ticket/4489.
(Note that design page for this is TBD.)
Isn't this backwards then?
336:
Instead of
len(data[:match.start() + 1].splitlines())
you can do
data.count('\n', 0, match.start()) + 1
Unfortunately '\n' is not good enough, we have to check for '\r\n' and
'\r' as well, hence the use of splitlines.
337:
The --external_cert_file and --external_ca_file options for
ipa-ca-install are removed, do we really want to do that? Shouldn't they
be deprecated instead?
+1
Same for --external-ca-file in ipa-cacert-manage.
+1
IMO it's OK to just remove them, as they were added during 4.1
development and are not available in any released version of IPA.
I can't say I'm a fan of forcing users to concatenate cert files.
All the --*-cert-file options may be given multiple times.
338: Looks OK
339: Looks OK
Could you add some docstrings to the functions you add? Sometimes it's
harder than necessary to decipher what they do and what the
arguments/return values mean exactly.
Sure.
There is no user-visible documentation on what file types are
expected/supported. It would be good to add this to the man pages, or
the --help.
Added.
I also wonder if the detection code should be changed. It basically now
tries a slew of different mechanisms one at a time rather than trying to
identify the type of file and using that one. It may not be possible in
all cases but you could at least start by looking for ^- to know it
is a text file and go from there, otherwise step through the binary formats.
Rearranged the code so that text files are tried first.
In external CA, the error message when specifying a certificate but not
the CA could be improved:
$ ipa-server-install --external_cert_file ~/p/Certificate_Authority_8.cer
...
CA certificate CN=Certificate Authority,O=IDM.LAB.ENG.BRQ.REDHAT.COM in
/home/pviktori/p/Certificate_Authority_8.cer is not valid:
(SEC_ERROR_UNKNOWN_ISSUER) Peer's Certificate issuer is not recognized.
Fixed.
For CA-less, I used a combination of files with which server
installation went well, but replica-install failed halfway through:
Console:
...
[16/36]: creating indices
[17/36]: enabling referential integrity plugin
[18/36]: configuring ssl for ds instance
[error] RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
Log tail:
2014-09-26T15:05:43Z DEBUG Starting external process
2014-09-26T15:05:43Z DEBUG args='/usr/bin/pk12util' '-d'
'/etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM/' '-i'
'/tmp/tmp2vEWX_ipa/realm_info/dscert.p12' '-k'
'/etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM//pwdfile.txt' '-v' '-w'
'/dev/stdin'
2014-09-26T15:05:43Z DEBUG Process finished, return code=17
2014-09-26T15:05:43Z DEBUG stdout=
2014-09-26T15:05:43Z DEBUG stderr=pk12util: PKCS12 decode not verified:
SEC_ERROR_BAD_PASSWORD: The security password entered is incorrect.
2014-09-26T15:05:43Z DEBUG Traceback (most recent call last):
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 370, in start_creation
run_step(full_msg, method)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 360, in run_step
method()
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
600, in __enable_ssl
trust_flags=trust_flags)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 1030, in create_from_pkcs12
self.import_pkcs12(pkcs12_fname, pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 971, in import_pkcs12
pkcs12_passwd=pkcs12_passwd)
File /usr/lib/python2.7/site-packages/ipaserver/install/certs.py,
line 191, in import_pkcs12
pkcs12_filename)
RuntimeError: incorrect password for pkcs#12 file
/tmp/tmp2vEWX_ipa/realm_info/dscert.p12
2014-09-26T15:05:43Z DEBUG [error] RuntimeError: incorrect password
for pkcs#12 file /tmp/tmp2vEWX_ipa/realm_info/dscert.p12
2014-09-26T15:05:43Z DEBUG File
/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py,
line 644, in run_script
return_value = main_function()
File /sbin/ipa-replica-install, line 677, in main
ds = install_replica_ds(config)
File /sbin/ipa-replica-install, line 190, in install_replica_ds
ca_file=config.dir + /ca.crt,
File
/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py, line
354, in create_replica
self.start_creation(runtime=60)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 370, in start_creation
run_step(full_msg, method)
File /usr/lib/python2.7/site-packages/ipaserver/install/service.py,
line 360, in run_step
method()
File
