[Freeipa-devel] [PATCH 0087] execute user-del pre-callback also during user preservation
fixes tickets: https://fedorahosted.org/freeipa/ticket/5362 https://fedorahosted.org/freeipa/ticket/5372 Upon discussion with Simo we decided that OTP tokens should be orphaned/deleted also during the user preservation. -- Martin^3 Babinsky From 635754c3773b1db5550fe19ad6f0a84e84d36459 Mon Sep 17 00:00:00 2001 From: Martin Babinsky Date: Fri, 16 Oct 2015 19:16:46 +0200 Subject: [PATCH] execute user-del pre-callback also during user preservation user preservation code was not using the pre-callback function which did check whether a protected member is being deleted and facilitated the orphaning/deletion of OTP tokens owner/managed by the user. https://fedorahosted.org/freeipa/ticket/5362 https://fedorahosted.org/freeipa/ticket/5372 --- ipalib/plugins/user.py | 7 +++ 1 file changed, 7 insertions(+) diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py index e3397f03d9130efaeae3fbc710bfce668f13..31d7b1d6decadd5bf5d4cf148d15f4426fc77f49 100644 --- a/ipalib/plugins/user.py +++ b/ipalib/plugins/user.py @@ -618,6 +618,10 @@ class user_del(baseuser_del): except errors.NotFound: self.obj.handle_not_found(pkey) +for callback in self.get_callbacks('pre'): +dn = callback(self, ldap, dn, [pkey], **options) +assert isinstance(dn, DN) + # start to move the entry to Delete container self._exc_wrapper(pkey, options, ldap.move_entry)(dn, delete_dn, del_old=True) @@ -673,6 +677,9 @@ class user_del(baseuser_del): check_protected_member(keys[-1]) +if dn.endswith(DN(self.obj.delete_container_dn, api.env.basedn)): +return dn + # Delete all tokens owned and managed by this user. # Orphan all tokens owned but not managed by this user. owner = self.api.Object.user.get_primary_key_from_dn(dn) -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHSET] Replica promotion patches
On 10/15/2015 9:54 AM, Simo Sorce wrote: 3) ipa-ca-install fails with: Traceback (most recent call last): File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line 445, in start_creation run_step(full_msg, method) File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line 435, in run_step method() File "/usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py", line 631, in __spawn_instance DogtagInstance.spawn_instance(self, cfg_file) File "/usr/lib/python2.7/site-packages/ipaserver/install/dogtaginstance.py", line 185, in spawn_instance self.handle_setup_error(e) File "/usr/lib/python2.7/site-packages/ipaserver/install/dogtaginstance.py", line 448, in handle_setup_error raise RuntimeError("%s configuration failed." % self.subsystem) RuntimeError: CA configuration failed. I guess I'm hitting the authentication bug in Dogtag. It is supposed to be fixed in pki-core-10.2.6-10, but is it fixed in pki-core-10.2.7-0.2? We might need a new 10.2.7 build. I am not sure which version has it fixed, Endi ? PKI ticket #1580 was fixed in pki-core-10.2.6-10 for F23 and F24. We never released a pki-core-10.2.7. I suppose that is a custom build? -- Endi S. Dewata -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0328] DNSSEC CI: Wait until DS records are replicated
Drill command sometimes fail on replica due to long replication time, this patch adding check that waits until record is replicated. Patch attached. From bce95f9d16fd3a920ca50952be75022d89682699 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Fri, 16 Oct 2015 14:09:37 +0200 Subject: [PATCH] DNSSEC CI: wait until DS records is replicated In some cases replication may take much more time than we expected. This patch adds explicit cech if DS records has been replicated. --- ipatests/test_integration/test_dnssec.py | 6 ++ 1 file changed, 6 insertions(+) diff --git a/ipatests/test_integration/test_dnssec.py b/ipatests/test_integration/test_dnssec.py index 66e67a6efbe1db767f8b7102d2928be775e723af..5d6acb7cc0d843929fd23e5a07f37803cbfdb792 100644 --- a/ipatests/test_integration/test_dnssec.py +++ b/ipatests/test_integration/test_dnssec.py @@ -350,6 +350,12 @@ class TestInstallDNSSECFirst(IntegrationTest): self.master.run_command(args) +# wait until DS records it replicated +assert wait_until_record_is_signed( +self.replicas[0].ip, example_test_zone, self.log, timeout=100, +rtype="DS" +), "No DS record of '%s' returned from replica" % example_test_zone + # extract DSKEY from root zone ans = resolve_with_dnssec(self.master.ip, root_zone, self.log, rtype="DNSKEY") -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0012-0019] CA ACL tracker and functional test
On 09/30/2015 02:47 PM, Martin Basti wrote: On 09/24/2015 02:49 PM, Milan Kubík wrote: Hi all, an update for CA ACL tests! I, with help from M. Babinsky, managed to find a way how to change the identity during acceptance cest run, which allows to test CA ACLs (and perhaps other areas with some form of access controll). This allowed me to write a test for CA ACLs and certificate profiles that checks if the ACL/profile is being used and enforced. The first several tests are based on Fraser's blogpost using SMIME profile [1]. The master and ipa-4-2 branches diverged a bit, so I had to change two commits when rebasing to ipa-4-2 branch. Commits should be applied in the order (including rebased patches I sent in an earlier email): master: * 12 - 17 ipa-4-2: * 18, 13 - 15, 19, 17 For convenience: patches on top of master: https://github.com/apophys/freeipa/tree/acl-profile-functional patches on top of ipa-4-2: https://github.com/apophys/freeipa/tree/acl-42 [1]: https://blog-ftweedal.rhcloud.com/2015/08/user-certificates-and-custom-profiles-with-freeipa-4-2/ Cheers, Milan NACK 0) rpm file does not contain test_xmlrpc/data directory, please modify setup.py.in. 1) Code contains to much todo for my taste. 2) Please do not use filter function, use dict comprehension. Hi, updated patches and the numbering mess somehow curbed. The patches are rebased on top of current master and ipa-4-2. 0) fixed by 0021 1) docs for tracker extended, added more test cases 2) changed -- Milan Kubik From 42df41de0b621392e113140602c8c9fd10c7d719 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milan=20Kub=C3=ADk?= Date: Fri, 17 Jul 2015 14:42:23 +0200 Subject: [PATCH 1/6] ipatests: add fuzzy instances for CA ACL DN and RDN https://fedorahosted.org/freeipa/ticket/57 --- ipatests/test_xmlrpc/xmlrpc_test.py | 8 1 file changed, 8 insertions(+) diff --git a/ipatests/test_xmlrpc/xmlrpc_test.py b/ipatests/test_xmlrpc/xmlrpc_test.py index 80638e2efdd9d7ff07fd89688397acb7d44654cd..5880a4c6f2d9bae23f296aa6386f5705f18eea29 100644 --- a/ipatests/test_xmlrpc/xmlrpc_test.py +++ b/ipatests/test_xmlrpc/xmlrpc_test.py @@ -77,6 +77,14 @@ fuzzy_sudocmddn = Fuzzy( '(?i)ipauniqueid=%s,cn=sudocmds,cn=sudo,%s' % (uuid_re, api.env.basedn) ) +# Matches caacl dn +fuzzy_caacldn = Fuzzy( +'(?i)ipauniqueid=%s,cn=caacls,cn=ca,%s' % (uuid_re, api.env.basedn) +) + +# Matches fuzzy ipaUniqueID DN group (RDN) +fuzzy_ipauniqueid = Fuzzy('(?i)ipauniqueid=%s' % uuid_re) + # Matches a hash signature, not enforcing length fuzzy_hash = Fuzzy('^([a-f0-9][a-f0-9]:)+[a-f0-9][a-f0-9]$', type=six.string_types) -- 2.6.1 From 57fcb2f7e5e4a30963c698cf41aae0a6bab45d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milan=20Kub=C3=ADk?= Date: Tue, 30 Jun 2015 17:00:18 +0200 Subject: [PATCH 2/6] ipatests: Add initial CAACLTracker implementation The patch implements the tracker for CA ACL feature. The basic CRUD checkers has been implemented. The methods for adding and removing the association of the resources with the ACL do not have the check methods. These will be provided as a separate test suite. https://fedorahosted.org/freeipa/ticket/57 --- ipatests/test_xmlrpc/objectclasses.py | 5 + ipatests/test_xmlrpc/test_caacl_plugin.py | 378 ++ 2 files changed, 383 insertions(+) create mode 100644 ipatests/test_xmlrpc/test_caacl_plugin.py diff --git a/ipatests/test_xmlrpc/objectclasses.py b/ipatests/test_xmlrpc/objectclasses.py index 1cd77c7f885fe408d0d9d48fc6d8284900c91b7f..134a08803f3abca1124c4d26274d9e3fc981b941 100644 --- a/ipatests/test_xmlrpc/objectclasses.py +++ b/ipatests/test_xmlrpc/objectclasses.py @@ -217,3 +217,8 @@ certprofile = [ u'top', u'ipacertprofile', ] + +caacl = [ +u'ipaassociation', +u'ipacaacl' +] diff --git a/ipatests/test_xmlrpc/test_caacl_plugin.py b/ipatests/test_xmlrpc/test_caacl_plugin.py new file mode 100644 index ..6cf835b229f70797e32bcfd2309cfa7be5732f51 --- /dev/null +++ b/ipatests/test_xmlrpc/test_caacl_plugin.py @@ -0,0 +1,378 @@ +# +# Copyright (C) 2015 FreeIPA Contributors see COPYING for license +# + +""" +Test the `ipalib.plugins.caacl` module. +""" + +import os + +import pytest + +from ipapython import ipautil +from ipalib import errors, x509 +from ipapython.dn import DN +from ipatests.test_xmlrpc.ldaptracker import Tracker +from ipatests.test_xmlrpc.xmlrpc_test import (XMLRPC_test, fuzzy_caacldn, + fuzzy_uuid, fuzzy_ipauniqueid, + raises_exact) +from ipatests.test_xmlrpc import objectclasses +from ipatests.util import assert_deepequal + + +class CAACLTracker(Tracker): +"""Tracker class for CA ACL LDAP object. + +The class implements methods required by the base class +to help with basic CRUD operations. + +Methods fo
[Freeipa-devel] [PATCH 0327] KRA: fix check if CA is installed on replica
https://fedorahosted.org/freeipa/ticket/5345 Patch attached. From 5538700dba81cbc4bc64485f7790dfc72148b4f8 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Thu, 15 Oct 2015 16:43:59 +0200 Subject: [PATCH] KRA: fix check that CA is installed https://fedorahosted.org/freeipa/ticket/5345 --- ipaserver/install/ipa_kra_install.py | 30 +++--- ipaserver/install/kra.py | 5 + 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/ipaserver/install/ipa_kra_install.py b/ipaserver/install/ipa_kra_install.py index ef2b2f9857c313f68bdc58bf8d3d15bf42a0debd..b07d1ffcf471189ce373ec630ee9f93a1c995077 100644 --- a/ipaserver/install/ipa_kra_install.py +++ b/ipaserver/install/ipa_kra_install.py @@ -31,7 +31,6 @@ from ipapython.dn import DN from ipaserver.install import krainstance from ipaserver.install import installutils from ipaserver.install.installutils import create_replica_config -from ipaserver.install import dogtaginstance from ipaserver.install import kra @@ -122,28 +121,21 @@ class KRAInstaller(KRAInstall): def validate_options(self, needs_root=True): super(KRAInstaller, self).validate_options(needs_root=True) -if self.options.unattended and self.options.password is None: -self.option_parser.error( -"Directory Manager password must be specified using -p" -" in unattended mode" -) - -self.installing_replica = dogtaginstance.is_installing_replica("KRA") - -if self.installing_replica: -if not self.args: -self.option_parser.error("A replica file is required.") -if len(self.args) > 1: -self.option_parser.error("Too many arguments provided") - +self.installing_replica = False +if len(self.args) > 1: +self.option_parser.error("Too many arguments provided") +elif len(self.args) == 1: +self.installing_replica = True self.replica_file = self.args[0] if not ipautil.file_exists(self.replica_file): self.option_parser.error( "Replica file %s does not exist" % self.replica_file) -else: -if self.args: -self.option_parser.error("Too many parameters provided. " - "No replica file is required.") + +if self.options.unattended and self.options.password is None: +self.option_parser.error( +"Directory Manager password must be specified using -p" +" in unattended mode" +) def ask_for_options(self): super(KRAInstaller, self).ask_for_options() diff --git a/ipaserver/install/kra.py b/ipaserver/install/kra.py index f3a0fe5c6945039315839af53c7b6f5649f67cd7..c78cd287262b13fc687b7dce038e4e482ccd65fe 100644 --- a/ipaserver/install/kra.py +++ b/ipaserver/install/kra.py @@ -49,6 +49,11 @@ def install_check(api, replica_config, options): for nickname in kra_cert_nicknames): raise RuntimeError("Missing KRA certificates, please create a " "new replica file.") +else: +if api.Command.kra_is_enabled()['result']: +raise RuntimeError( +"KRA is already installed on the master system. A replica " +"file is required.") def install(api, replica_config, options): -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0318 - 0320, 0323] installer: allow to modify dse.ldif during installation
On 16.10.2015 10:31, Martin Basti wrote: On 16.10.2015 10:05, Martin Babinsky wrote: On 10/16/2015 08:47 AM, Jan Cholasta wrote: On 16.10.2015 08:42, Martin Kosek wrote: On 10/16/2015 06:00 AM, Jan Cholasta wrote: On 15.10.2015 19:47, Martin Basti wrote: On 15.10.2015 18:47, Martin Basti wrote: On 15.10.2015 18:36, Martin Babinsky wrote: On 10/15/2015 04:50 PM, Martin Basti wrote: On 14.10.2015 16:10, Martin Basti wrote: On 14.10.2015 15:51, Martin Babinsky wrote: On 10/13/2015 06:38 PM, Martin Basti wrote: On 12.10.2015 12:30, Martin Babinsky wrote: On 10/08/2015 05:58 PM, Martin Basti wrote: The attached patches fix following tickets: https://fedorahosted.org/freeipa/ticket/4949 https://fedorahosted.org/freeipa/ticket/4048 https://fedorahosted.org/freeipa/ticket/1930 With these patches, an administrator can specify LDIF file that contains modifications to be applied to dse.ldif right after creation of DS instance. Hi, Functionally the paches work as expected. However I have following nitpicks: in patch 318: 1.) there is a typo in ModifyLDIF class docstring: +Operations keep the order in whihc were specified per DN. in patch 320: 1.) you should use 'os.path.join' to construct FS paths: -dse_filename = '%s/%s' % ( +dse_filename = os.path.join( paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE % self.serverid, -'dse.ldif', +'dse.ldif' ) 2.) IIUC the 'config_ldif_file' knob in BaseServer holds the path to LDIF containing the mod operations to dse.ldif. However, the knob name sounds like the option accepts the path of dse.ldif itself. I propose to rename the knob to something more in-line with the supposed function, like 'dse_mods_file'. Updated patches + CI test attached Patches work as expected and CI tests are OK. However it seems that warning level messages are not logged to installer output but only to log file (*waves hand* magic). Maybe it would be a good idea to raise the message level to "error", so that it is immediately obvious to the user that his DSE mods contain an error and cannot be parsed. Also you have a typo in the commit message of patch 320 (s/chages/changes/). Updated patches attached. Rebased patches for master branch. ACK Pushed to master: 5233165ce7062bb7aa649bf95a029103c375207b Pushed to ipa-4-2: 94412b81c1c09b56ee2900942a1b804f21c264c5 These tickets are not for ipa-4-2, reverted a17936a1aad139236f18f0e5ad0b066f7747ca60 Can we use a better option name? --dirsrv-config-mods sounds weird, as you need to specify a file, not mods... +1. maybe --dirsrv-config-ldif? --dirsrv-config-file is most consistent with other options which accept files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.) This, however, may be confusing to user since '--dirsrv-config-file' may evoke a feeling that it consumes *whole new* dse.ldif while in reality it is only a few custom mods to directory server configuration. I agree, it expects only file containing modifications in LDIF format, 'config-file' suffix may be confusing Sorry, but this does not make any sense. Why would anyone think they are supposed to specify a complete dse.ldif? Is it written somewhere that DS config file == dse.ldif? I don't think so. And, if you use --help, you will see exactly what the option does right away. What is actually confusing is inventing a "smart" name instead of making it consistent with everything else. Speaking about the option, I saw some unescaped "-"'s in the man page updates: +.TP +\fB\-\-dirsrv-config-mods\fR +The path to LDIF file that will be used to modify configuration of dse.ldif during installation of the directory server instance Martin -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0083] perform an unlimited search for reverse zones when adding DNS records
On 10/14/2015 04:05 PM, Petr Spacek wrote: On 14.10.2015 14:13, Martin Babinsky wrote: On 10/13/2015 04:00 PM, Petr Spacek wrote: On 13.10.2015 13:37, Martin Babinsky wrote: On 10/13/2015 09:36 AM, Petr Spacek wrote: On 12.10.2015 16:35, Martin Babinsky wrote: https://fedorahosted.org/freeipa/ticket/5200 --- ipalib/plugins/dns.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index 84086f4c77d02922f237937d58031cc42d55410e..c36345faecfb9db7abced1c6bd72ddcf93473a74 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -537,7 +537,8 @@ def get_reverse_zone(ipaddr, prefixlen=None): if prefixlen is None: revzone = None -result = api.Command['dnszone_find']()['result'] +result = api.Command['dnszone_find'](sizelimit=0)['result'] + NACK, this just increases the limit because LDAP server will enforce a per-user limit. for zone in result: zonename = zone['idnsname'][0] if (revdns.is_subdomain(zonename.make_absolute()) and Generic solution should use dns.resolver.zone_for_name() to find DNS zone matching the reverse name. This should also implicitly cover CNAME/DNAME redirections per RFC2317. Using DNS implicitly means that a zone will always be found (at least the root zone :-). For this reason I would change final error message reason=_('DNS reverse zone for IP address %(addr)s not found') to something like: reason=_('DNS reverse zone %(zone)s for IP address %(addr)s is not managed by this server') These changes should fix it without adding other artificial limitation. Thank you for clarification Petr. Attaching the reworked patch. -- Martin^3 Babinsky freeipa-mbabinsk-0083.1-perform-more-robust-search-for-reverse-zones-when-ad.patch From 463903f4ea4f74efeb02dbb705ca0a54fab81366 Mon Sep 17 00:00:00 2001 From: Martin Babinsky Date: Mon, 12 Oct 2015 16:24:50 +0200 Subject: [PATCH 1/3] perform more robust search for reverse zones when adding DNS record Instead of searching for all zones to identify the correct reverse zone, we will first ask the resolver to return the name of zone that should contain the desired record and then see if IPA manages this zone. https://fedorahosted.org/freeipa/ticket/5200 --- ipalib/plugins/dns.py | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index 84086f4c77d02922f237937d58031cc42d55410e..e3c6ce46168f8b6af607a61af1e3e431cfee32c8 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -531,25 +531,35 @@ def add_forward_record(zone, name, str_address): pass # the entry already exists and matches def get_reverse_zone(ipaddr, prefixlen=None): +""" +resolve the reverse zone for IP address and see if it is managed by IPA +server +:param ipaddr: host IP address +:param prefixlen: network prefix length +:return: tuple containing name of the reverse zone and the name of the +record +""" ip = netaddr.IPAddress(str(ipaddr)) revdns = DNSName(unicode(ip.reverse_dns)) +resolved_zone = unicode(dns.resolver.zone_for_name(revdns)) if prefixlen is None: revzone = None +result = api.Command['dnszone_find'](resolved_zone)['result'] -result = api.Command['dnszone_find']()['result'] for zone in result: zonename = zone['idnsname'][0] if (revdns.is_subdomain(zonename.make_absolute()) and (revzone is None or zonename.is_subdomain(revzone))): revzone = zonename Oh, wait, this might blow up if there is a disabled DNS zone which is deeper than the one returned from DNS query. Damn, we have opened a Pandora box! ipaserver/install/bindinstance.py has own implementation of get_reverse_zone() + additional menthods find_reverse_zone(). These are using get_reverse_zone_default() from ipalib/util.py which is duplicating code in 'if prefixlen is not None' branch. And of course, are not correct in light of this change. My expectation would be that disabled zones should be ignored... So we should get rid of this mess in the loop and use dnszone_show() which is called at the end. And fix the other places, preferably by deleting duplicate implementations. I would expect that you can store (converted) output of dns.resolver.zone_for_name(revdns) into revzone and delete whole 'if prefixlen is None' branch. else: +# if prefixlen is specified, determine the zone name for the network +# prefix if ip.version == 4: pos = 4 - prefixlen / 8 elif ip.version == 6: pos = 32 - prefixlen / 4 -items = ip.reverse_dns.split('.') -revzone = DNSName(items[pos:]) +revzone = revdns[pos:] Hmm, and this logic will breaks CNAME/DNAME (RFC2317) handling ...
Re: [Freeipa-devel] [PATCHES 0318 - 0320, 0323] installer: allow to modify dse.ldif during installation
On 16.10.2015 10:05, Martin Babinsky wrote: On 10/16/2015 08:47 AM, Jan Cholasta wrote: On 16.10.2015 08:42, Martin Kosek wrote: On 10/16/2015 06:00 AM, Jan Cholasta wrote: On 15.10.2015 19:47, Martin Basti wrote: On 15.10.2015 18:47, Martin Basti wrote: On 15.10.2015 18:36, Martin Babinsky wrote: On 10/15/2015 04:50 PM, Martin Basti wrote: On 14.10.2015 16:10, Martin Basti wrote: On 14.10.2015 15:51, Martin Babinsky wrote: On 10/13/2015 06:38 PM, Martin Basti wrote: On 12.10.2015 12:30, Martin Babinsky wrote: On 10/08/2015 05:58 PM, Martin Basti wrote: The attached patches fix following tickets: https://fedorahosted.org/freeipa/ticket/4949 https://fedorahosted.org/freeipa/ticket/4048 https://fedorahosted.org/freeipa/ticket/1930 With these patches, an administrator can specify LDIF file that contains modifications to be applied to dse.ldif right after creation of DS instance. Hi, Functionally the paches work as expected. However I have following nitpicks: in patch 318: 1.) there is a typo in ModifyLDIF class docstring: +Operations keep the order in whihc were specified per DN. in patch 320: 1.) you should use 'os.path.join' to construct FS paths: -dse_filename = '%s/%s' % ( +dse_filename = os.path.join( paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE % self.serverid, -'dse.ldif', +'dse.ldif' ) 2.) IIUC the 'config_ldif_file' knob in BaseServer holds the path to LDIF containing the mod operations to dse.ldif. However, the knob name sounds like the option accepts the path of dse.ldif itself. I propose to rename the knob to something more in-line with the supposed function, like 'dse_mods_file'. Updated patches + CI test attached Patches work as expected and CI tests are OK. However it seems that warning level messages are not logged to installer output but only to log file (*waves hand* magic). Maybe it would be a good idea to raise the message level to "error", so that it is immediately obvious to the user that his DSE mods contain an error and cannot be parsed. Also you have a typo in the commit message of patch 320 (s/chages/changes/). Updated patches attached. Rebased patches for master branch. ACK Pushed to master: 5233165ce7062bb7aa649bf95a029103c375207b Pushed to ipa-4-2: 94412b81c1c09b56ee2900942a1b804f21c264c5 These tickets are not for ipa-4-2, reverted a17936a1aad139236f18f0e5ad0b066f7747ca60 Can we use a better option name? --dirsrv-config-mods sounds weird, as you need to specify a file, not mods... +1. maybe --dirsrv-config-ldif? --dirsrv-config-file is most consistent with other options which accept files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.) This, however, may be confusing to user since '--dirsrv-config-file' may evoke a feeling that it consumes *whole new* dse.ldif while in reality it is only a few custom mods to directory server configuration. I agree, it expects only file containing modifications in LDIF format, 'config-file' suffix may be confusing Speaking about the option, I saw some unescaped "-"'s in the man page updates: +.TP +\fB\-\-dirsrv-config-mods\fR +The path to LDIF file that will be used to modify configuration of dse.ldif during installation of the directory server instance Martin -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0001 cert-show: Remove check if hostname != CN
On 15.10.2015 17:28, Jan Orel wrote: > diff --git a/ipalib/plugins/cert.py b/ipalib/plugins/cert.py > index e459320..55f9484 100644 > --- a/ipalib/plugins/cert.py > +++ b/ipalib/plugins/cert.py > @@ -625,9 +625,12 @@ class cert_show(VirtualCommand): > result['md5_fingerprint'] = > unicode(nss.data_to_hex(nss.md5_digest(cert.der_data), 64)[0]) > result['sha1_fingerprint'] = > unicode(nss.data_to_hex(nss.sha1_digest(cert.der_data), 64)[0]) > if hostname: > -# If we have a hostname we want to verify that the subject > -# of the certificate matches it, otherwise raise an error > -if hostname != cert.subject.common_name:#pylint: > disable=E1101 > +# If we have a hostname we want to verify that we can > +# write to the usercertificate attr of the target > +ldap = self.api.Backend.ldap2 > +entry = ldap.find_entry_by_attr("cn", cert.subject.common_name, > +"ipahost", base_dn=api.env.basedn) > +if not ldap.can_write(entry.dn, 'usercertificate'): > raise acierr > > return dict(result=result) I can't say anything about correctness of the change itself but it would be good to add explanatory error message to acierr, when you are at it. Something like 'Insufficient permissions for write to userCertificate attribute of $DN entry' or so. Thanks! -- Petr^2 Spacek -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0318 - 0320, 0323] installer: allow to modify dse.ldif during installation
On 10/16/2015 08:47 AM, Jan Cholasta wrote: On 16.10.2015 08:42, Martin Kosek wrote: On 10/16/2015 06:00 AM, Jan Cholasta wrote: On 15.10.2015 19:47, Martin Basti wrote: On 15.10.2015 18:47, Martin Basti wrote: On 15.10.2015 18:36, Martin Babinsky wrote: On 10/15/2015 04:50 PM, Martin Basti wrote: On 14.10.2015 16:10, Martin Basti wrote: On 14.10.2015 15:51, Martin Babinsky wrote: On 10/13/2015 06:38 PM, Martin Basti wrote: On 12.10.2015 12:30, Martin Babinsky wrote: On 10/08/2015 05:58 PM, Martin Basti wrote: The attached patches fix following tickets: https://fedorahosted.org/freeipa/ticket/4949 https://fedorahosted.org/freeipa/ticket/4048 https://fedorahosted.org/freeipa/ticket/1930 With these patches, an administrator can specify LDIF file that contains modifications to be applied to dse.ldif right after creation of DS instance. Hi, Functionally the paches work as expected. However I have following nitpicks: in patch 318: 1.) there is a typo in ModifyLDIF class docstring: +Operations keep the order in whihc were specified per DN. in patch 320: 1.) you should use 'os.path.join' to construct FS paths: -dse_filename = '%s/%s' % ( +dse_filename = os.path.join( paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE % self.serverid, -'dse.ldif', +'dse.ldif' ) 2.) IIUC the 'config_ldif_file' knob in BaseServer holds the path to LDIF containing the mod operations to dse.ldif. However, the knob name sounds like the option accepts the path of dse.ldif itself. I propose to rename the knob to something more in-line with the supposed function, like 'dse_mods_file'. Updated patches + CI test attached Patches work as expected and CI tests are OK. However it seems that warning level messages are not logged to installer output but only to log file (*waves hand* magic). Maybe it would be a good idea to raise the message level to "error", so that it is immediately obvious to the user that his DSE mods contain an error and cannot be parsed. Also you have a typo in the commit message of patch 320 (s/chages/changes/). Updated patches attached. Rebased patches for master branch. ACK Pushed to master: 5233165ce7062bb7aa649bf95a029103c375207b Pushed to ipa-4-2: 94412b81c1c09b56ee2900942a1b804f21c264c5 These tickets are not for ipa-4-2, reverted a17936a1aad139236f18f0e5ad0b066f7747ca60 Can we use a better option name? --dirsrv-config-mods sounds weird, as you need to specify a file, not mods... +1. maybe --dirsrv-config-ldif? --dirsrv-config-file is most consistent with other options which accept files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.) This, however, may be confusing to user since '--dirsrv-config-file' may evoke a feeling that it consumes *whole new* dse.ldif while in reality it is only a few custom mods to directory server configuration. Speaking about the option, I saw some unescaped "-"'s in the man page updates: +.TP +\fB\-\-dirsrv-config-mods\fR +The path to LDIF file that will be used to modify configuration of dse.ldif during installation of the directory server instance Martin -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code