[Freeipa-devel] [PATCH 0087] execute user-del pre-callback also during user preservation

[Martin Babinsky]

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] [PATCHES 0318 - 0320, 0323] installer: allow to modify dse.ldif during installation

[Martin Kosek]

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? 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] [PATCHES 0318 - 0320, 0323] installer: allow to modify dse.ldif during installation

[Jan Cholasta]

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.)



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] 0001 cert-show: Remove check if hostname != CN

[Petr Spacek]

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

[Martin Babinsky]

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

Re: [Freeipa-devel] [PATCHES 0318 - 0320, 0323] installer: allow to modify dse.ldif during installation

[Martin Basti]

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 0083] perform an unlimited search for reverse zones when adding DNS records

[Martin Babinsky]

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 

Re: [Freeipa-devel] [PATCHES 0318 - 0320, 0323] installer: allow to modify dse.ldif during installation

[Jan Cholasta]

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

[Freeipa-devel] [PATCH 0327] KRA: fix check if CA is installed on replica

[Martin Basti]

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

[Freeipa-devel] [PATCH 0328] DNSSEC CI: Wait until DS records are replicated

[Martin Basti]

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] [PATCHSET] Replica promotion patches

[Endi Sukma Dewata]

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