date:20120905

On 09/05/2012 03:47 AM, Rob Crittenden wrote:
> Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On 08/30/2012 02:53 PM, Rob Crittenden wrote:
 Martin Kosek wrote:
> Current objectclass updates in a form of "replace" update instruction
> dependent on exact match of the old object class specification in the
> update instruction and the real value in LDAP. However, this
> approach is
> very error prone as object class definition can easily differ as for
> example because of unexpected X-ORIGIN value. Such objectclass update
> failures may lead to serious malfunctions later.
>
> Add new update instruction type "replaceoc" with the following format:
> replaceoc:OID:new
> This update instruction will always replace an objectclass with
> specified OID with the new definition.
>
> https://fedorahosted.org/freeipa/ticket/2440

 This works ok. Martin and I had a conversation in IRC about it.

 This moves from replacing a specific bit of schema with a new one, in
 all
 cases. I wonder if we should be more conservative and know what we're
 replacing
 in advance.

 rob

>>>
>>> You are right, I was too harsh when replacing the objectclasses. This
>>> would
>>> cause issues when LDAP update would be run on a replica with lower
>>> version and
>>> older objectclass definitions.
>>>
>>> I came up with an alternative solution and instead of always replacing
>>> the
>>> objectclass I rather reverted to old-OC:new-OC style which should be
>>> safer.
>>> Now, the LDAP updater always normalizes an objectclass before
>>> comparing it
>>> using python-ldap objectclass model. With this approach, objectclasses
>>> differing only in X-ORIGIN or white spaces should match and be updated.
>>>
>>> Martin
>>>
>>
>> NACK
>>
>> I think this:
>>
>> +for value in replaced_values:
>> +entry_values.remove(old)
>>
>> Should be:
>>
>> +entry_values.remove(value)

Right. Thanks for the fix. It only worked in my testing because I had no
objectclass update which would differ in X-ORIGIN or whitespaces or case. I
tried to mangle an update file and with this fix it did the update even if
X-ORIGIN and whitespaces differed.

>>
>> I'm still doing other testing but this is what I've found so far.
>>
>> rob
> 
> I did some more testing and it looks like this will do the trick.
> 
> I also found a place where the schema was left as unicode and causing it to
> blow up inside python-ldap. Here is the diff on my working instance:
> 
> diff -u  ipaserver/install/ldapupdate.py
> /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py
> --- ipaserver/install/ldapupdate.py 2012-09-04 16:59:33.210688723 -0400
> +++ /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py 
> 2012-09-04
> 21:47:01.583574375 -0400
> @@ -643,7 +643,7 @@
>  self.debug('replace: no match for replaced
> ObjectClass "%s"', old)
>  continue
>  for value in replaced_values:
> -entry_values.remove(old)
> +entry_values.remove(value)
>  else:
>  entry_values.remove(old)
>  entry_values.append(new)
> @@ -772,7 +772,11 @@
>  updated = False
>  changes = self.conn.generateModList(entry.origDataDict(),
> entry.toDict())
>  if (entry.dn == DN(('cn', 'schema'))):
> -updated = self.is_schema_updated(entry.toDict())
> +d = dict()
> +e = entry.toDict()
> +for k,v in e.items():
> +d[k] = [str(x) for x in v]
> +updated = self.is_schema_updated(d)
>  else:
>  if len(changes) >= 1:
>  updated = True
> 
> rob
> 

I did not hit this error during testing, but at least it won't harm. Sending an
updated patch.
>From 51593ec841218674e4d200513a06609cf0383558 Mon Sep 17 00:00:00 2001
From: Martin Kosek 
Date: Tue, 4 Sep 2012 13:18:54 +0200
Subject: [PATCH] Add safe updates for objectClasses

Current objectclass updates in a form of "replace" update instruction
dependent on exact match of the old object class specification in the
update instruction and the real value in LDAP. However, this approach is
very error prone as object class definition can easily differ as for
example because of unexpected X-ORIGIN value. Such objectclass update
failures may lead to serious malfunctions later.

When comparing the objectclasses, make sure we normalize them both
before we compare them to mitigate these kinds of errors. python-ldap's
objectclass model can be utilized to do the normalization part.

One objectclass update instruction was changed to do a rep

Re: [Freeipa-devel] [PATCH] 0077 Check direct/reverse hostname/address resolution in ipa-replica-install


On 09/04/2012 07:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:


https://fedorahosted.org/freeipa/ticket/2845


Shouldn't this also call verify_fqdn() on the local hostname and not
just the master? I think this would eventually fail in the conncheck but
what if that was skipped?

rob


A few lines above there is a call to get_host_name, which will call 
verify_fqdn.


--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

We allowed IP addresses without network specification which lead
to unexpected results when the zone was being created. We should rather
strictly require the prefix/netmask specifying the IP network that
the reverse zone should be created for. This is already done in
Web UI.

A unit test exercising this new validation was added.

https://fedorahosted.org/freeipa/ticket/2461
>From 6b1f1681d103ff73b61e77f672594458a6ed0fb5 Mon Sep 17 00:00:00 2001
From: Martin Kosek 
Date: Wed, 5 Sep 2012 09:56:27 +0200
Subject: [PATCH] Stricter IP network validator in dnszone-add command

We allowed IP addresses without network specification which lead
to unexpected results when the zone was being created. We should rather
strictly require the prefix/netmask specifying the IP network that
the reverse zone should be created for. This is already done in
Web UI.

A unit test exercising this new validation was added.

https://fedorahosted.org/freeipa/ticket/2461
---
 ipalib/plugins/dns.py| 10 +-
 tests/test_xmlrpc/test_dns_plugin.py | 16 
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 3987001f06dba1bcc5a311243e4f1fdcf83091c7..aa81f29e68deb4e6af7e8c44242deb3c0defbd0c 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -281,7 +281,15 @@ def _validate_ip4addr(ugettext, ipaddr):
 def _validate_ip6addr(ugettext, ipaddr):
 return _validate_ipaddr(ugettext, ipaddr, 6)
 
-def _validate_ipnet(ugettext, ipnet):
+def _validate_ipnet(ugettext, ipnet, require_prefix=True):
+if require_prefix:
+try:
+net = netaddr.IPAddress(ipnet)
+except (netaddr.AddrFormatError, ValueError):
+pass
+else:
+return _('netmask or subnet prefix specifying the IP network address is required')
+
 try:
 net = netaddr.IPNetwork(ipnet)
 except (netaddr.AddrFormatError, ValueError, UnboundLocalError):
diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py
index 2b6d53c0bb705ea96c80ff23149e0e049c439e39..c195aa169201a86ebaec45341c4559e309c68a2e 100644
--- a/tests/test_xmlrpc/test_dns_plugin.py
+++ b/tests/test_xmlrpc/test_dns_plugin.py
@@ -948,6 +948,22 @@ class test_dns(Declarative):
 error=u'invalid IP network format'),
 ),
 
+
+dict(
+desc='Try to create a reverse zone from IP without prefix',
+command=(
+'dnszone_add', [], {
+'name_from_ip': u'10.0.0.1',
+'idnssoamname': dnszone1_mname,
+'idnssoarname': dnszone1_rname,
+'ip_address' : u'1.2.3.4',
+}
+),
+expected=errors.ValidationError(name='name_from_ip',
+error=u'netmask or subnet prefix specifying the IP network address is required'),
+),
+
+
 dict(
 desc='Create reverse from IP %s zone using name_from_ip option' % revdnszone1_ip,
 command=(
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH] 205 Reflect API change of SSH store in Web UI

2012-09-05 Thread Petr Vobornik

Format of ipasshpubkey in users and hosts changed from BYTES to STR. Web 
UI no longer gets the value as base64 encoded string in an object.


Label was changed to reflect that the key don't have to be plain base64 
encoded blob.


https://fedorahosted.org/freeipa/ticket/2989

Note: freeipa-jcholast-83-openssh-style-public-keys.patch should be applied
--
Petr Vobornik
From decfc911e3e37c7c21768a8cda6d741783e79a9d Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Wed, 5 Sep 2012 09:27:21 +0200
Subject: [PATCH] Reflect API change of SSH store in Web UI

Format of ipasshpubkey in users and hosts changed from BYTES to STR. Web UI no longer gets the value as base64 encoded string in a object.

Label was changed to reflect that the key don't have to be plain base64 encoded blob.

https://fedorahosted.org/freeipa/ticket/2989
---
 install/ui/field.js|  2 +-
 install/ui/test/data/ipa_init.json |  2 +-
 install/ui/test/data/user_details_refresh.json | 12 
 install/ui/test/data/user_mod.json | 12 
 ipalib/plugins/internal.py |  2 +-
 5 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/install/ui/field.js b/install/ui/field.js
index 8e2d116c0fec3f1a372c49b0c22822af4cedbf0c..49b9de084f10f9826f6e23b0d63b6e605b401057 100644
--- a/install/ui/field.js
+++ b/install/ui/field.js
@@ -649,7 +649,7 @@ IPA.sshkeys_field = function(spec) {
 if (keys[i] === '') continue;
 
 var value = {
-key: keys[i].__base64__,
+key: keys[i],
 fingerprint: fingerprints[i]
 };
 values.push(value);
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index ff4c7489d235d0be7a2f1c98371921023a75cfa2..6a3bb02d8a982eecd53fb7c0d3d7192532f6e769 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -405,7 +405,7 @@
 },
 "sshkeystore": {
 "keys": "SSH public keys",
-"set_dialog_help": "Base-64 encoded SSH public key:",
+"set_dialog_help": "SSH public key:",
 "set_dialog_title": "Set SSH key",
 "show_set_key": "Show/Set key",
 "status_mod_ns": "Modified: key not set",
diff --git a/install/ui/test/data/user_details_refresh.json b/install/ui/test/data/user_details_refresh.json
index 98270f3e6c5581d3dd8ba61bfac17e7baecc..43df0cba3235b89f874c65bb1f5d0a171392c90e 100644
--- a/install/ui/test/data/user_details_refresh.json
+++ b/install/ui/test/data/user_details_refresh.json
@@ -109,12 +109,8 @@
 "/home/kfrog"
 ],
 "ipasshpubkey": [
-{
-"__base64__": "B3NzaC1kc3MAAACBAJQdBmxjnYdDzpGPzAD2kkoRwGbPIqVsu6mR0eyLjMJafQPbOkrgFpIwxAhb519FHAr0olSOklwq5Pvkm7rSpnoXDAEoVlW5jNCj33NrYkVJTAmtsa4ihZIqz7KJYkYifQyBxy8nYScAlEu1a1k9xfbRT8ZAmiCqgd6klWaKZXHBFQCd2wJW3Rb7kclRPbKaRF1hhdW+hwAAAIBr9ZozKls1lAkqMZuhGgnvlFg5J7DXKRY6iju1VfMc5HiY1mq197Qq2PVi4iQ1Ach3Pvj/MZGLhz/SyIsH59Uugl21KeQjk2HHt/ZLL1PrJvm9LmX3Fv2E8lQNnEyPaA2Ngf6O0bbzii41lp1F6wo3xQLJCLldljcG+wskRf5RrgAAAIAKTDWct9ES5BZADb3TfBAng2aeeojg6rDhS6i0WYPtna2hjCdRaCnnXM079JVcYBq3IK7mFpINnqnn252Mr9OqGsu/92gglfpTpXq2Lzkvd2dbcyTPQmNBM7+KFlUOZ4hWb4c5CVLBwILccL7QakwSHRYlRhCsDyKbI+2qPGwrXg=="
-},
-{
-"__base64__": "B3NzaC1yc2EDAQABAAABAQCaoWIiTBqTqsWnAHFUfOUNY65sk2KuEp798ESyDWmOuJbjQNHG0grl8d5D9/OMuDvSHAthkTR4rXn1QAMm+Geh63XhS+nnEpzinPKloMWyF8wsNEw117TX2mA8wzeea1HDcqd0v4YKyU5FAHm+6hOALB9mrAnRZ4WoTY6NWtm3aoRge1D26pOuLRyNFB5Etwa7cmJqaK8EeHN0XNPIbaAP1JNVe46wyYvMlE/yhGYS9qW36OJpQBJGk9nYqHT3/6ai2w3TQ8k5puvAwBcCe0wYhBE9o8c5PmImIfa8lZ3Oo2gHIiOyA9gyHv3MBqUxXBC6PmmYHEr6y/wnYnfSHF+N"
-}
+"ssh-rsa B3NzaC1yc2EDAQABAAABAQCaoWIiTBqTqsWnAHFUfOUNY65sk2KuEp798ESyDWmOuJbjQNHG0grl8d5D9/OMuDvSHAthkTR4rXn1QAMm+Geh63XhS+nnEpzinPKloMWyF8wsNEw117TX2mA8wzeea1HDcqd0v4YKyU5FAHm+6hOALB9mrAnRZ4WoTY6NWtm3aoRge1D26pOuLRyNFB5Etwa7cmJqaK8EeHN0XNPIbaAP1JNVe46wyYvMlE/yhGYS9qW36OJpQBJGk9nYqHT3/6ai2w3TQ8k5puvAwBcCe0wYhBE9o8c5PmImIfa8lZ3Oo2gHIiOyA9gyHv3MBqUxXBC6PmmYHEr6y/wnYnfSHF+N",
+"ssh-dss B3NzaC1kc3MAAACBAJQdBmxjnYdDzpGPzAD2kkoRwGbPIqVsu6mR0eyLjMJafQPbOkrgFpIwxAhb519FHAr0olSOklwq5Pvkm7rSpnoXDAEoVlW5jNCj33NrYkVJTAmtsa4ihZIqz7KJYkYifQyBxy8nYScAlEu1a1k9xfbRT8ZAmiCqgd6klWaKZXHBFQCd2wJW3Rb7kclRPbKaRF1hhdW+hwAAAIBr9ZozKls1lAkqMZuhGgnvlFg5J7DXKRY6iju1VfMc5HiY1mq197Qq2PVi4iQ1Ach3Pvj/MZGLhz/SyIsH59Uugl21KeQjk2HHt/ZLL1PrJvm9LmX3Fv2E8lQNnEyPaA2Ngf6O0bbzii41lp1F6wo3xQLJCLldljcG+wskRf5RrgAAAIAKTDWct9ES5BZADb3TfBAng2aeeojg6rDhS6i0WYPtna2hjCdRaCnnXM079JVcYBq3IK7mFpINnqnn252Mr9OqGsu/9

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

2012-09-05 Thread Jan Cholasta


Dne 5.9.2012 10:04, Martin Kosek napsal(a):

We allowed IP addresses without network specification which lead
to unexpected results when the zone was being created. We should rather
strictly require the prefix/netmask specifying the IP network that
the reverse zone should be created for. This is already done in
Web UI.

A unit test exercising this new validation was added.

https://fedorahosted.org/freeipa/ticket/2461



I don't like this much. I would suggest using CheckedIPAddress and not 
forcing the user to enter the prefix length instead.


CheckedIPAddress uses a sensible default prefix length if one is not 
specified (class-based for IPv4, /64 for IPv6) as opposed to IPNetwork 
(/32 for IPv4, /128 for IPv6 - this causes the erroneous reverse zones 
to be created as described in the ticket).


Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 300-301 Fix DNS SOA serial parameters boundaries

This works well, but please see some comments below.

On 09/04/2012 04:22 PM, Martin Kosek wrote:

To test, simply run the following command:

  ipa dnszone-mod example.com --serial=4294967295

This should work well on patched server&client. Web UI should work too as it
reads the max limit dynamically.

Please put this in the test suite.

---
[PATCH 2/2] Fix DNS SOA serial parameters boundaries:

Set correct boundaries for DNS SOA serial parameters (see RFC 1035,
2181).

[PATCH 1/2] Transfer long numbers over XMLRPC

Numeric parameters in ipalib were limited by XMLRPC boundaries for
integer (2^31-1) which is too low for some LDAP attributes like DNS
SOA serial field.

Transfer numbers which are not in XMLRPC boundary as a string and not
as a number to workaround this limitation. Int parameter had to be
updated to also accept Python's long type as valid int type.

freeipa-mkosek-300-transfer-long-numbers-over-xmlrpc.patch

From 8782015a17b130c5ebae8b014a7241810b10dedd Mon Sep 17 00:00:00 2001

From: Martin Kosek
Date: Tue, 4 Sep 2012 15:49:26 +0200
Subject: [PATCH 1/2] Transfer long numbers over XMLRPC

Numeric parameters in ipalib were limited by XMLRPC boundaries for
integer (2^31-1) which is too low for some LDAP attributes like DNS
SOA serial field.

Transfer numbers which are not in XMLRPC boundary as a string and not
as a number to workaround this limitation. Int parameter had to be
updated to also accept Python's long type as valid int type.

https://fedorahosted.org/freeipa/ticket/2568
---
  ipalib/parameters.py | 12 ++--
  ipalib/rpc.py|  5 -
  2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 
de0d14faf08d1ab79c99e65dab9cc08f406e3a1d..21e30356b2a351bf7a3be7d47d7fabf0130cf6d4
 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -1077,7 +1077,7 @@ class Number(Param):
  """
  if type(value) is self.type:
  return value
-if type(value) in (unicode, int, float):
+if type(value) in (unicode, int, long, float):

PEP8 says that "Object type comparisons should always use isinstance() 
instead of comparing types directly".
It would be nice to change the old code whenever it is touched. It's 
also in a few more places in the patch.

diff --git a/ipalib/rpc.py b/ipalib/rpc.py
index 
d1764e3e30492d5855450398e86689bfcad7aa39..85239ac65903acf447a4d971cce70f819979ce8d
 100644
--- a/ipalib/rpc.py
+++ b/ipalib/rpc.py
@@ -94,6 +95,8 @@ def xml_wrap(value):
  if type(value) is Decimal:
  # transfer Decimal as a string
  return unicode(value)
+if isinstance(value, (int, long)) and (value < MININT or value > MAXINT):
+return unicode(value)
  if isinstance(value, DN):
  return str(value)
  assert type(value) in (unicode, int, float, bool, NoneType)

`long` should also be included here.

>>> api.Command.user_find(uidnumber=151100)
{'count': 1, 'truncated': False, 'result': ({...},), 'summary': u'1 user 
matched'}

>>> api.Command.user_find(uidnumber=151100 + 0L)
Traceback (most recent call last):
  File "", line 1, in 
...
  File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 102, in 
xml_wrap

assert type(value) in (unicode, int, float, bool, NoneType)
AssertionError

--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command


On 09/05/2012 11:30 AM, Jan Cholasta wrote:

Dne 5.9.2012 10:04, Martin Kosek napsal(a):

We allowed IP addresses without network specification which lead
to unexpected results when the zone was being created. We should rather
strictly require the prefix/netmask specifying the IP network that
the reverse zone should be created for. This is already done in
Web UI.

A unit test exercising this new validation was added.

https://fedorahosted.org/freeipa/ticket/2461



I don't like this much. I would suggest using CheckedIPAddress and not forcing
the user to enter the prefix length instead.

CheckedIPAddress uses a sensible default prefix length if one is not specified
(class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for IPv4,
/128 for IPv6 - this causes the erroneous reverse zones to be created as
described in the ticket).


Hello,

I don't like automatic netmask guessing. I have met class-based guessing in 
Windows (XP?) and I was forced to overwrite default mask all the time ...


IMHO there is no "sensible default prefix" in real world. I sitting on network 
with /23 prefix right now. Also, I have never seen 10.x network with /8 prefix.


--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 300-301 Fix DNS SOA serial parameters boundaries

On 09/05/2012 12:14 PM, Petr Viktorin wrote:

This works well, but please see some comments below.

On 09/04/2012 04:22 PM, Martin Kosek wrote:

To test, simply run the following command:

  ipa dnszone-mod example.com --serial=4294967295

This should work well on patched server&client. Web UI should work too
as it
reads the max limit dynamically.

Please put this in the test suite.

---
[PATCH 2/2] Fix DNS SOA serial parameters boundaries:

Set correct boundaries for DNS SOA serial parameters (see RFC 1035,
2181).

[PATCH 1/2] Transfer long numbers over XMLRPC

Numeric parameters in ipalib were limited by XMLRPC boundaries for
integer (2^31-1) which is too low for some LDAP attributes like DNS
SOA serial field.

Transfer numbers which are not in XMLRPC boundary as a string and not
as a number to workaround this limitation. Int parameter had to be
updated to also accept Python's long type as valid int type.

freeipa-mkosek-300-transfer-long-numbers-over-xmlrpc.patch

From 8782015a17b130c5ebae8b014a7241810b10dedd Mon Sep 17 00:00:00 2001

From: Martin Kosek
Date: Tue, 4 Sep 2012 15:49:26 +0200
Subject: [PATCH 1/2] Transfer long numbers over XMLRPC

Numeric parameters in ipalib were limited by XMLRPC boundaries for
integer (2^31-1) which is too low for some LDAP attributes like DNS
SOA serial field.

Transfer numbers which are not in XMLRPC boundary as a string and not
as a number to workaround this limitation. Int parameter had to be
updated to also accept Python's long type as valid int type.

https://fedorahosted.org/freeipa/ticket/2568
---
  ipalib/parameters.py | 12 ++--
  ipalib/rpc.py|  5 -
  2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index
de0d14faf08d1ab79c99e65dab9cc08f406e3a1d..21e30356b2a351bf7a3be7d47d7fabf0130cf6d4
100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -1077,7 +1077,7 @@ class Number(Param):
  """
  if type(value) is self.type:
  return value
-if type(value) in (unicode, int, float):
+if type(value) in (unicode, int, long, float):

PEP8 says that "Object type comparisons should always use isinstance()
instead of comparing types directly".
It would be nice to change the old code whenever it is touched. It's
also in a few more places in the patch.

diff --git a/ipalib/rpc.py b/ipalib/rpc.py
index
d1764e3e30492d5855450398e86689bfcad7aa39..85239ac65903acf447a4d971cce70f819979ce8d
100644
--- a/ipalib/rpc.py
+++ b/ipalib/rpc.py
@@ -94,6 +95,8 @@ def xml_wrap(value):
  if type(value) is Decimal:
  # transfer Decimal as a string
  return unicode(value)
+if isinstance(value, (int, long)) and (value < MININT or value >
MAXINT):
+return unicode(value)
  if isinstance(value, DN):
  return str(value)
  assert type(value) in (unicode, int, float, bool, NoneType)

`long` should also be included here.

 >>> api.Command.user_find(uidnumber=151100)
{'count': 1, 'truncated': False, 'result': ({...},), 'summary': u'1 user
matched'}
 >>> api.Command.user_find(uidnumber=151100 + 0L)
Traceback (most recent call last):
   File "", line 1, in 
...
   File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 102, in
xml_wrap
 assert type(value) in (unicode, int, float, bool, NoneType)
AssertionError

One more thing: please update the VERSION file.

--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

On 09/05/2012 12:22 PM, Petr Spacek wrote:
> On 09/05/2012 11:30 AM, Jan Cholasta wrote:
>> Dne 5.9.2012 10:04, Martin Kosek napsal(a):
>>> We allowed IP addresses without network specification which lead
>>> to unexpected results when the zone was being created. We should rather
>>> strictly require the prefix/netmask specifying the IP network that
>>> the reverse zone should be created for. This is already done in
>>> Web UI.
>>>
>>> A unit test exercising this new validation was added.
>>>
>>> https://fedorahosted.org/freeipa/ticket/2461
>>>
>>
>> I don't like this much. I would suggest using CheckedIPAddress and not 
>> forcing
>> the user to enter the prefix length instead.
>>
>> CheckedIPAddress uses a sensible default prefix length if one is not 
>> specified
>> (class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for IPv4,
>> /128 for IPv6 - this causes the erroneous reverse zones to be created as
>> described in the ticket).
>>
> Hello,
> 
> I don't like automatic netmask guessing. I have met class-based guessing in
> Windows (XP?) and I was forced to overwrite default mask all the time ...
> 
> IMHO there is no "sensible default prefix" in real world. I sitting on network
> with /23 prefix right now. Also, I have never seen 10.x network with /8 
> prefix.
> 

+1 I would rather force user to choose the netmask and receive an expectable
result that to be confused with CIDR-based (for IPv6) values.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

2012-09-05 Thread Jan Cholasta


Dne 5.9.2012 12:22, Petr Spacek napsal(a):

On 09/05/2012 11:30 AM, Jan Cholasta wrote:

Dne 5.9.2012 10:04, Martin Kosek napsal(a):

We allowed IP addresses without network specification which lead
to unexpected results when the zone was being created. We should rather
strictly require the prefix/netmask specifying the IP network that
the reverse zone should be created for. This is already done in
Web UI.

A unit test exercising this new validation was added.

https://fedorahosted.org/freeipa/ticket/2461



I don't like this much. I would suggest using CheckedIPAddress and not
forcing
the user to enter the prefix length instead.

CheckedIPAddress uses a sensible default prefix length if one is not
specified
(class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for
IPv4,
/128 for IPv6 - this causes the erroneous reverse zones to be created as
described in the ticket).


Hello,

I don't like automatic netmask guessing. I have met class-based guessing
in Windows (XP?) and I was forced to overwrite default mask all the time
...


If there was no guessing, you would have to write the netmask anyway, so 
I don't see any harm in guessing here.




IMHO there is no "sensible default prefix" in real world. I sitting on
network with /23 prefix right now. Also, I have never seen 10.x network
with /8 prefix.



While this might be true for IPv4 in some cases, /64 is perfectly 
sensible for IPv6. Also, I have never seen 192.168.x.x network with 
non-/24 prefix.


Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

On 09/05/2012 12:36 PM, Jan Cholasta wrote:
> Dne 5.9.2012 12:22, Petr Spacek napsal(a):
>> On 09/05/2012 11:30 AM, Jan Cholasta wrote:
>>> Dne 5.9.2012 10:04, Martin Kosek napsal(a):
 We allowed IP addresses without network specification which lead
 to unexpected results when the zone was being created. We should rather
 strictly require the prefix/netmask specifying the IP network that
 the reverse zone should be created for. This is already done in
 Web UI.

 A unit test exercising this new validation was added.

 https://fedorahosted.org/freeipa/ticket/2461

>>>
>>> I don't like this much. I would suggest using CheckedIPAddress and not
>>> forcing
>>> the user to enter the prefix length instead.
>>>
>>> CheckedIPAddress uses a sensible default prefix length if one is not
>>> specified
>>> (class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for
>>> IPv4,
>>> /128 for IPv6 - this causes the erroneous reverse zones to be created as
>>> described in the ticket).
>>>
>> Hello,
>>
>> I don't like automatic netmask guessing. I have met class-based guessing
>> in Windows (XP?) and I was forced to overwrite default mask all the time
>> ...
> 
> If there was no guessing, you would have to write the netmask anyway, so I
> don't see any harm in guessing here.
> 
>>
>> IMHO there is no "sensible default prefix" in real world. I sitting on
>> network with /23 prefix right now. Also, I have never seen 10.x network
>> with /8 prefix.
>>
> 
> While this might be true for IPv4 in some cases, /64 is perfectly sensible for
> IPv6. Also, I have never seen 192.168.x.x network with non-/24 prefix.
> 
> Honza
> 

While this may be true for 192.168.x.x, it does not apply for 10.x.x.x networks
as Petr already pointed out. I don't think that there will be many people
expecting that a reverse zone of 10.0.0.0/24 would be created.

And since FreeIPA is mainly deployed to internal networks, I assume this will
be the case of most users.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

On Wed, Sep 05, 2012 at 12:48:22PM +0200, Martin Kosek wrote:
> On 09/05/2012 12:36 PM, Jan Cholasta wrote:
> > Dne 5.9.2012 12:22, Petr Spacek napsal(a):
> >> On 09/05/2012 11:30 AM, Jan Cholasta wrote:
> >>> Dne 5.9.2012 10:04, Martin Kosek napsal(a):
>  We allowed IP addresses without network specification which lead
>  to unexpected results when the zone was being created. We should rather
>  strictly require the prefix/netmask specifying the IP network that
>  the reverse zone should be created for. This is already done in
>  Web UI.
> 
>  A unit test exercising this new validation was added.
> 
>  https://fedorahosted.org/freeipa/ticket/2461
> 
> >>>
> >>> I don't like this much. I would suggest using CheckedIPAddress and not
> >>> forcing
> >>> the user to enter the prefix length instead.
> >>>
> >>> CheckedIPAddress uses a sensible default prefix length if one is not
> >>> specified
> >>> (class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for
> >>> IPv4,
> >>> /128 for IPv6 - this causes the erroneous reverse zones to be created as
> >>> described in the ticket).
> >>>
> >> Hello,
> >>
> >> I don't like automatic netmask guessing. I have met class-based guessing
> >> in Windows (XP?) and I was forced to overwrite default mask all the time
> >> ...
> > 
> > If there was no guessing, you would have to write the netmask anyway, so I
> > don't see any harm in guessing here.
> > 
> >>
> >> IMHO there is no "sensible default prefix" in real world. I sitting on
> >> network with /23 prefix right now. Also, I have never seen 10.x network
> >> with /8 prefix.
> >>
> > 
> > While this might be true for IPv4 in some cases, /64 is perfectly sensible 
> > for
> > IPv6. Also, I have never seen 192.168.x.x network with non-/24 prefix.
> > 
> > Honza
> > 
> 
> While this may be true for 192.168.x.x, it does not apply for 10.x.x.x 
> networks
> as Petr already pointed out. I don't think that there will be many people
> expecting that a reverse zone of 10.0.0.0/24 would be created.

+1 for explicit netmasks.

Although 192.168.X.0/24 is common for home networks, it's not common for
company networks. When company use 192.168.0.0/16 network, it is nearly always
splitted into something with for example 255.255.240.0 netmask because 255
machines in one network is too low number.

Regards, Adam

-- 
Adam Tkac, Red Hat, Inc.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

2012-09-05 Thread Jan Cholasta


Dne 5.9.2012 12:48, Martin Kosek napsal(a):

On 09/05/2012 12:36 PM, Jan Cholasta wrote:

Dne 5.9.2012 12:22, Petr Spacek napsal(a):

On 09/05/2012 11:30 AM, Jan Cholasta wrote:

Dne 5.9.2012 10:04, Martin Kosek napsal(a):

We allowed IP addresses without network specification which lead
to unexpected results when the zone was being created. We should rather
strictly require the prefix/netmask specifying the IP network that
the reverse zone should be created for. This is already done in
Web UI.

A unit test exercising this new validation was added.

https://fedorahosted.org/freeipa/ticket/2461



I don't like this much. I would suggest using CheckedIPAddress and not
forcing
the user to enter the prefix length instead.

CheckedIPAddress uses a sensible default prefix length if one is not
specified
(class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for
IPv4,
/128 for IPv6 - this causes the erroneous reverse zones to be created as
described in the ticket).


Hello,

I don't like automatic netmask guessing. I have met class-based guessing
in Windows (XP?) and I was forced to overwrite default mask all the time
...


If there was no guessing, you would have to write the netmask anyway, so I
don't see any harm in guessing here.



IMHO there is no "sensible default prefix" in real world. I sitting on
network with /23 prefix right now. Also, I have never seen 10.x network
with /8 prefix.



While this might be true for IPv4 in some cases, /64 is perfectly sensible for
IPv6. Also, I have never seen 192.168.x.x network with non-/24 prefix.

Honza



While this may be true for 192.168.x.x, it does not apply for 10.x.x.x networks
as Petr already pointed out. I don't think that there will be many people
expecting that a reverse zone of 10.0.0.0/24 would be created.


And they would be correct, because the default prefix length for a class 
A network is /8, not /24.




And since FreeIPA is mainly deployed to internal networks, I assume this will
be the case of most users.

Martin



OK, but what about IPv6? Correct me if I'm wrong, but the prefix length 
is going to be /64 99% of the time for IPv6.


The installer uses /24 for IPv4 addresses and /64 for IPv6 addresses, 
maybe this should be used as a default here as well.


Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0050] Fix memory leak in configuration with multiple LDAP instances

On Tue, Aug 14, 2012 at 04:00:21PM +0200, Petr Spacek wrote:
> Hello,
> 
> this patch fixes $SUBJ$.
> 
> Adam, please double-check correctness of this change.
> 
> I had two assumptions:
> - all locking is done inside dns_db_(un)register() functions
> - LDAP instances are decommissioned before dynamic_driver_destroy() call

Ack

> From e314eb7da7bfbbb2ae9d4ce1252d886c9a744e7f Mon Sep 17 00:00:00 2001
> From: Petr Spacek 
> Date: Tue, 14 Aug 2012 15:53:42 +0200
> Subject: [PATCH] Fix memory leak in configuration with multiple LDAP
>  instances.
> 
> Signed-off-by: Petr Spacek 
> ---
>  src/ldap_driver.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ldap_driver.c b/src/ldap_driver.c
> index 
> d958d15bdebe5e89dc4948655ffba655073d53e0..117215ae480cdcabb2977037af9a89bb25578243
>  100644
> --- a/src/ldap_driver.c
> +++ b/src/ldap_driver.c
> @@ -1278,6 +1278,7 @@ isc_result_t
>  dynamic_driver_init(isc_mem_t *mctx, const char *name, const char * const 
> *argv,
>   dns_dyndb_arguments_t *dyndb_args)
>  {
> + dns_dbimplementation_t *ldapdb_imp_new = NULL;
>   isc_result_t result;
>  
>   REQUIRE(name != NULL);
> @@ -1305,11 +1306,12 @@ dynamic_driver_init(isc_mem_t *mctx, const char 
> *name, const char * const *argv,
>   }
>  
>   /* Register new DNS DB implementation. */
> - ldapdb_imp = NULL;
>   result = dns_db_register(ldapdb_impname, &ldapdb_create, NULL, mctx,
> -  &ldapdb_imp);
> +  &ldapdb_imp_new);
>   if (result != ISC_R_SUCCESS && result != ISC_R_EXISTS)
>   return result;
> + else if (result == ISC_R_SUCCESS)
> + ldapdb_imp = ldapdb_imp_new;
>  
>   /* Finally, create the instance. */
>   result = manager_create_db_instance(mctx, name, argv, dyndb_args);
> -- 
> 1.7.11.2
> 


-- 
Adam Tkac, Red Hat, Inc.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

On Wed, Sep 05, 2012 at 01:02:35PM +0200, Jan Cholasta wrote:
> Dne 5.9.2012 12:48, Martin Kosek napsal(a):
> >On 09/05/2012 12:36 PM, Jan Cholasta wrote:
> >>Dne 5.9.2012 12:22, Petr Spacek napsal(a):
> >>>On 09/05/2012 11:30 AM, Jan Cholasta wrote:
> Dne 5.9.2012 10:04, Martin Kosek napsal(a):
> >We allowed IP addresses without network specification which lead
> >to unexpected results when the zone was being created. We should rather
> >strictly require the prefix/netmask specifying the IP network that
> >the reverse zone should be created for. This is already done in
> >Web UI.
> >
> >A unit test exercising this new validation was added.
> >
> >https://fedorahosted.org/freeipa/ticket/2461
> >
> 
> I don't like this much. I would suggest using CheckedIPAddress and not
> forcing
> the user to enter the prefix length instead.
> 
> CheckedIPAddress uses a sensible default prefix length if one is not
> specified
> (class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for
> IPv4,
> /128 for IPv6 - this causes the erroneous reverse zones to be created as
> described in the ticket).
> 
> >>>Hello,
> >>>
> >>>I don't like automatic netmask guessing. I have met class-based guessing
> >>>in Windows (XP?) and I was forced to overwrite default mask all the time
> >>>...
> >>
> >>If there was no guessing, you would have to write the netmask anyway, so I
> >>don't see any harm in guessing here.
> >>
> >>>
> >>>IMHO there is no "sensible default prefix" in real world. I sitting on
> >>>network with /23 prefix right now. Also, I have never seen 10.x network
> >>>with /8 prefix.
> >>>
> >>
> >>While this might be true for IPv4 in some cases, /64 is perfectly sensible 
> >>for
> >>IPv6. Also, I have never seen 192.168.x.x network with non-/24 prefix.
> >>
> >>Honza
> >>
> >
> >While this may be true for 192.168.x.x, it does not apply for 10.x.x.x 
> >networks
> >as Petr already pointed out. I don't think that there will be many people
> >expecting that a reverse zone of 10.0.0.0/24 would be created.
> 
> And they would be correct, because the default prefix length for a
> class A network is /8, not /24.
> 
> >
> >And since FreeIPA is mainly deployed to internal networks, I assume this will
> >be the case of most users.
> >
> >Martin
> >
> 
> OK, but what about IPv6? Correct me if I'm wrong, but the prefix
> length is going to be /64 99% of the time for IPv6.

You are right, IPv6 networks could have default /64 prefix. However as I wrote
in different mail, I don't recommend to use default IPv4 prefix at all because
FreeIPA targets for company environments where /24 is not so common, not for
home environments.

> The installer uses /24 for IPv4 addresses and /64 for IPv6
> addresses, maybe this should be used as a default here as well.

Regards, Adam

-- 
Adam Tkac, Red Hat, Inc.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

On 09/05/2012 01:06 PM, Adam Tkac wrote:
> On Wed, Sep 05, 2012 at 01:02:35PM +0200, Jan Cholasta wrote:
>> Dne 5.9.2012 12:48, Martin Kosek napsal(a):
>>> On 09/05/2012 12:36 PM, Jan Cholasta wrote:
 Dne 5.9.2012 12:22, Petr Spacek napsal(a):
> On 09/05/2012 11:30 AM, Jan Cholasta wrote:
>> Dne 5.9.2012 10:04, Martin Kosek napsal(a):
>>> We allowed IP addresses without network specification which lead
>>> to unexpected results when the zone was being created. We should rather
>>> strictly require the prefix/netmask specifying the IP network that
>>> the reverse zone should be created for. This is already done in
>>> Web UI.
>>>
>>> A unit test exercising this new validation was added.
>>>
>>> https://fedorahosted.org/freeipa/ticket/2461
>>>
>>
>> I don't like this much. I would suggest using CheckedIPAddress and not
>> forcing
>> the user to enter the prefix length instead.
>>
>> CheckedIPAddress uses a sensible default prefix length if one is not
>> specified
>> (class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for
>> IPv4,
>> /128 for IPv6 - this causes the erroneous reverse zones to be created as
>> described in the ticket).
>>
> Hello,
>
> I don't like automatic netmask guessing. I have met class-based guessing
> in Windows (XP?) and I was forced to overwrite default mask all the time
> ...

 If there was no guessing, you would have to write the netmask anyway, so I
 don't see any harm in guessing here.

>
> IMHO there is no "sensible default prefix" in real world. I sitting on
> network with /23 prefix right now. Also, I have never seen 10.x network
> with /8 prefix.
>

 While this might be true for IPv4 in some cases, /64 is perfectly sensible 
 for
 IPv6. Also, I have never seen 192.168.x.x network with non-/24 prefix.

 Honza

>>>
>>> While this may be true for 192.168.x.x, it does not apply for 10.x.x.x 
>>> networks
>>> as Petr already pointed out. I don't think that there will be many people
>>> expecting that a reverse zone of 10.0.0.0/24 would be created.
>>
>> And they would be correct, because the default prefix length for a
>> class A network is /8, not /24.
>>
>>>
>>> And since FreeIPA is mainly deployed to internal networks, I assume this 
>>> will
>>> be the case of most users.
>>>
>>> Martin
>>>
>>
>> OK, but what about IPv6? Correct me if I'm wrong, but the prefix
>> length is going to be /64 99% of the time for IPv6.
> 
> You are right, IPv6 networks could have default /64 prefix. However as I wrote
> in different mail, I don't recommend to use default IPv4 prefix at all because
> FreeIPA targets for company environments where /24 is not so common, not for
> home environments.

I think now most of us agree on that automatic prefixes does not make much
sense for IPv4. But do we really want to introduce different behavior in this
case for IPv4+IPv6 network validation and allow automatic prefix only for IPv6
only?

> 
>> The installer uses /24 for IPv4 addresses and /64 for IPv6
>> addresses, maybe this should be used as a default here as well.

Yeah, it would make sense to do this change also for installer to make them
consistent.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0051-0052] Log successful reconnection to LDAP server

On Wed, Aug 15, 2012 at 01:20:08PM +0200, Petr Spacek wrote:
> Hello,
> 
> this two patches solves upstream ticket
> https://fedorahosted.org/bind-dyndb-ldap/ticket/71
> "Log successful reconnect"
> 
> Patch 51:
> Adds log_info(): logging facility with log level INFO.

Ack.

> Patch 52:
> Logs successful reconnection to LDAP server.
> 
> LDAP connection error handling was modified:
> Errors are handled exclusively by handle_connection_error() now.
> 
> Direct calls to ldap_connect() and ldap_reconnect() should be avoided.

Nack, please check my comments below.

Regards, Adam

> From 15286f0793d3666845e6b03b565d49f135b115ff Mon Sep 17 00:00:00 2001
> From: Petr Spacek 
> Date: Wed, 15 Aug 2012 12:05:56 +0200
> Subject: [PATCH] Add log_info(): logging facility with log level INFO.
> 
> Signed-off-by: Petr Spacek 
> ---
>  src/log.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/log.h b/src/log.h
> index 
> 898639be144dbf6049a1440493c3358e01a5c2dd..9062a4e80786b5bab806d6c9ceba1d1a9917a3e2
>  100644
> --- a/src/log.h
> +++ b/src/log.h
> @@ -43,6 +43,9 @@
>  #define log_error(format, ...)   \
>   log_write(GET_LOG_LEVEL(ISC_LOG_ERROR), format, ##__VA_ARGS__)
>  
> +#define log_info(format, ...)\
> + log_write(GET_LOG_LEVEL(ISC_LOG_INFO), format, ##__VA_ARGS__)
> +
>  #define log_debug(level, format, ...)\
>   log_write(GET_LOG_LEVEL(level), format, ##__VA_ARGS__)
>  
> -- 
> 1.7.11.2
> 

> From 06f03d8b602656dc9581abc675c943d6fa6a6db2 Mon Sep 17 00:00:00 2001
> From: Petr Spacek 
> Date: Wed, 15 Aug 2012 12:57:32 +0200
> Subject: [PATCH] Log successful reconnection to LDAP server.
> 
> LDAP connection error handling was modified:
> Errors are handled exclusively by handle_connection_error() now.
> 
> Direct calls to ldap_connect() and ldap_reconnect() should be avoided.
> 
> https://fedorahosted.org/bind-dyndb-ldap/ticket/71
> 
> Signed-off-by: Petr Spacek 
> ---
>  src/ldap_helper.c | 37 -
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 
> cc7003a6cdcd2d27404fec936623ed8a3e8fa7f8..798aeadfef27d7071a1dd4133b7f08a21918ef78
>  100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -1734,7 +1734,7 @@ ldap_query(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn,
>* successful
>* TODO: handle this case inside ldap_pool_getconnection()?
>*/
> - CHECK(ldap_connect(ldap_inst, ldap_conn, ISC_FALSE));
> + CHECK(handle_connection_error(ldap_inst, ldap_conn, ISC_FALSE));
>   }
>  
>  retry:
> @@ -1903,14 +1903,16 @@ ldap_connect(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn,
>   int ret;
>   int version;
>   struct timeval timeout;
> + isc_result_t result;

I would recommend to be consistent here and use "isc_result_t result = 
ISC_R_FAILURE"

> + REQUIRE(ldap_inst != NULL);
>   REQUIRE(ldap_conn != NULL);
>  
>   ret = ldap_initialize(&ld, str_buf(ldap_inst->uri));
>   if (ret != LDAP_SUCCESS) {
>   log_error("LDAP initialization failed: %s",
> ldap_err2string(ret));
> - goto cleanup;
> + CHECK(ISC_R_FAILURE);

Please use goto here, as done in rest of code.

>   }
>  
>   version = LDAP_VERSION3;
> @@ -1932,21 +1934,22 @@ ldap_connect(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn,
>   if (ldap_conn->handle != NULL)
>   ldap_unbind_ext_s(ldap_conn->handle, NULL, NULL);
>   ldap_conn->handle = ld;
> + ld = NULL; /* prevent double-unbind from ldap_reconnect() and cleanup: 
> */
>  
> - return ldap_reconnect(ldap_inst, ldap_conn, force);
> + CHECK(ldap_reconnect(ldap_inst, ldap_conn, force));
> + return result;
>  
>  cleanup:
> -
>   if (ld != NULL)
>   ldap_unbind_ext_s(ld, NULL, NULL);
>   
>   /* Make sure handle is NULL. */
>   if (ldap_conn->handle != NULL) {
>   ldap_unbind_ext_s(ldap_conn->handle, NULL, NULL);
>   ldap_conn->handle = NULL;
>   }
>  
> - return ISC_R_FAILURE;
> + return result;
>  }
>  
>  static isc_result_t
> @@ -2064,12 +2067,18 @@ handle_connection_error(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn
>  {
>   int ret;
>   int err_code;
> + isc_result_t result = ISC_R_FAILURE;
>  
> - ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE,
> -   (void *)&err_code);
> + REQUIRE(ldap_conn != NULL);

...

> - if (ret != LDAP_OPT_SUCCESS) {
> - log_error("handle_connection_error failed to obtain ldap error 
> code");
> + if (ldap_conn->handle != NULL) {
> + ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE,
> +   (void *)&err_code);
> + if (ret != LDAP_OPT_SUCCE

Re: [Freeipa-devel] [PATCH 0053] Use richer set of return codes for LDAP connection error handling code

On Wed, Aug 15, 2012 at 01:23:45PM +0200, Petr Spacek wrote:
> Hello,
> 
> current code return very generic ISC_R_FAILURE code in nearly all (error) 
> cases.
> 
> This patch distinguishes between different LDAP errors and returns
> richer set of return codes from LDAP connection error handling code.
> 
> It should lead to clearer log messages.
> 
> Petr^2 Spacek

Ack

> From 15d6b38c9eda5b05d799c145ede8341f359e8633 Mon Sep 17 00:00:00 2001
> From: Petr Spacek 
> Date: Wed, 15 Aug 2012 13:01:48 +0200
> Subject: [PATCH] Use richer set of return codes for LDAP connection error
>  handling code.
> 
> It should lead to clear log messages.
> 
> Signed-off-by: Petr Spacek 
> ---
>  src/ldap_helper.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 
> 798aeadfef27d7071a1dd4133b7f08a21918ef78..da083d2e65032e650cfbbeb863262e0141403407
>  100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -1971,7 +1971,7 @@ ldap_reconnect(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn,
>   result = isc_time_now(&now);
>   time_cmp = isc_time_compare(&now, &ldap_conn->next_reconnect);
>   if (result == ISC_R_SUCCESS && time_cmp < 0)
> - return ISC_R_FAILURE;
> + return ISC_R_SOFTQUOTA;
>   }
>  
>   /* If either bind_dn or the password is not set, we will use
> @@ -2050,6 +2050,8 @@ force_reconnect:
>   return ISC_R_NOPERM;
>   case LDAP_SERVER_DOWN:
>   return ISC_R_NOTCONNECTED;
> + case LDAP_TIMEOUT:
> + return ISC_R_TIMEDOUT;
>   default:
>   return ISC_R_FAILURE;
>   }
> @@ -2085,13 +2087,16 @@ handle_connection_error(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn
>   switch (err_code) {
>   case LDAP_NO_SUCH_OBJECT:
>   ldap_conn->tries = 0;
> - return ISC_R_SUCCESS;
> + result = ISC_R_SUCCESS;
> + break;
>   case LDAP_TIMEOUT:
>   log_error("LDAP query timed out. Try to adjust \"timeout\" 
> parameter");
> + result = ISC_R_TIMEDOUT;
>   break;
>   case LDAP_INVALID_DN_SYNTAX:
>   case LDAP_INVALID_SYNTAX:
>   log_bug("Invalid syntax in handle_connection_error indicates a 
> bug");
> + result = ISC_R_UNEXPECTEDTOKEN;
>   break;
>   default:
>   /* Try to reconnect on other errors. */
> -- 
> 1.7.11.2
> 


-- 
Adam Tkac, Red Hat, Inc.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 300-301 Fix DNS SOA serial parameters boundaries

On 09/05/2012 12:26 PM, Petr Viktorin wrote:
> On 09/05/2012 12:14 PM, Petr Viktorin wrote:
>> This works well, but please see some comments below.
>>
>> On 09/04/2012 04:22 PM, Martin Kosek wrote:
>>> To test, simply run the following command:
>>>
>>>   ipa dnszone-mod example.com --serial=4294967295
>>>
>>> This should work well on patched server&client. Web UI should work too
>>> as it
>>> reads the max limit dynamically.
>>
>> Please put this in the test suite.

Done.

>>
>>> ---
>>> [PATCH 2/2] Fix DNS SOA serial parameters boundaries:
>>>
>>> Set correct boundaries for DNS SOA serial parameters (see RFC 1035,
>>> 2181).
>>>
>>>
>>> [PATCH 1/2] Transfer long numbers over XMLRPC
>>>
>>> Numeric parameters in ipalib were limited by XMLRPC boundaries for
>>> integer (2^31-1) which is too low for some LDAP attributes like DNS
>>> SOA serial field.
>>>
>>> Transfer numbers which are not in XMLRPC boundary as a string and not
>>> as a number to workaround this limitation. Int parameter had to be
>>> updated to also accept Python's long type as valid int type.
>>>
>>>
>>> freeipa-mkosek-300-transfer-long-numbers-over-xmlrpc.patch
>>>
>>>
 From 8782015a17b130c5ebae8b014a7241810b10dedd Mon Sep 17 00:00:00 2001
>>> From: Martin Kosek
>>> Date: Tue, 4 Sep 2012 15:49:26 +0200
>>> Subject: [PATCH 1/2] Transfer long numbers over XMLRPC
>>>
>>> Numeric parameters in ipalib were limited by XMLRPC boundaries for
>>> integer (2^31-1) which is too low for some LDAP attributes like DNS
>>> SOA serial field.
>>>
>>> Transfer numbers which are not in XMLRPC boundary as a string and not
>>> as a number to workaround this limitation. Int parameter had to be
>>> updated to also accept Python's long type as valid int type.
>>>
>>> https://fedorahosted.org/freeipa/ticket/2568
>>> ---
>>>   ipalib/parameters.py | 12 ++--
>>>   ipalib/rpc.py|  5 -
>>>   2 files changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/ipalib/parameters.py b/ipalib/parameters.py
>>> index
>>> de0d14faf08d1ab79c99e65dab9cc08f406e3a1d..21e30356b2a351bf7a3be7d47d7fabf0130cf6d4
>>>
>>> 100644
>>> --- a/ipalib/parameters.py
>>> +++ b/ipalib/parameters.py
>>> @@ -1077,7 +1077,7 @@ class Number(Param):
>>>   """
>>>   if type(value) is self.type:
>>>   return value
>>> -if type(value) in (unicode, int, float):
>>> +if type(value) in (unicode, int, long, float):
>>
>>
>> PEP8 says that "Object type comparisons should always use isinstance()
>> instead of comparing types directly".
>> It would be nice to change the old code whenever it is touched. It's
>> also in a few more places in the patch.
>>

I considered doing this when I was developing the patch, but I did not want to
mix type/isinstance in the same code. Now, when I experimented and tried to
replace it in a larger scope, there were unexpected issues. Like bool type
suddenly passing an isinstance(value, (int, long)) test and causing issues 
later.

So I skipped this part (as we discussed off-list).

>>> diff --git a/ipalib/rpc.py b/ipalib/rpc.py
>>> index
>>> d1764e3e30492d5855450398e86689bfcad7aa39..85239ac65903acf447a4d971cce70f819979ce8d
>>>
>>> 100644
>>> --- a/ipalib/rpc.py
>>> +++ b/ipalib/rpc.py
>>> @@ -94,6 +95,8 @@ def xml_wrap(value):
>>>   if type(value) is Decimal:
>>>   # transfer Decimal as a string
>>>   return unicode(value)
>>> +if isinstance(value, (int, long)) and (value < MININT or value >
>>> MAXINT):
>>> +return unicode(value)
>>>   if isinstance(value, DN):
>>>   return str(value)
>>>   assert type(value) in (unicode, int, float, bool, NoneType)
>>
>> `long` should also be included here.
>>
>>  >>> api.Command.user_find(uidnumber=151100)
>> {'count': 1, 'truncated': False, 'result': ({...},), 'summary': u'1 user
>> matched'}
>>  >>> api.Command.user_find(uidnumber=151100 + 0L)
>> Traceback (most recent call last):
>>File "", line 1, in 
>> ...
>>File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 102, in
>> xml_wrap
>>  assert type(value) in (unicode, int, float, bool, NoneType)
>> AssertionError
>>
>>
>>
>>
> 
> One more thing: please update the VERSION file.
> 

I did not want to do this because I just messed with validation, but yeah, I
can do that.

Fixed patches attached.

Martin
>From 965fa27cf32dcb209a291945e9eb342c88fd18dd Mon Sep 17 00:00:00 2001
From: Martin Kosek 
Date: Tue, 4 Sep 2012 15:49:26 +0200
Subject: [PATCH 1/2] Transfer long numbers over XMLRPC

Numeric parameters in ipalib were limited by XMLRPC boundaries for
integer (2^31-1) which is too low for some LDAP attributes like DNS
SOA serial field.

Transfer numbers which are not in XMLRPC boundary as a string and not
as a number to workaround this limitation. Int parameter had to be
updated to also accept Python's long type as valid int type.

https://fedorahosted.org/freeipa/ticket/2568
---
 ipalib/parameters.py | 12 ++--
 ipalib/rpc.py|  7 +++

Re: [Freeipa-devel] [PATCH 0050] Fix memory leak in configuration with multiple LDAP instances


On 09/05/2012 01:02 PM, Adam Tkac wrote:

On Tue, Aug 14, 2012 at 04:00:21PM +0200, Petr Spacek wrote:

Hello,

this patch fixes $SUBJ$.

Adam, please double-check correctness of this change.

I had two assumptions:
- all locking is done inside dns_db_(un)register() functions
- LDAP instances are decommissioned before dynamic_driver_destroy() call


Ack


Pushed to master: 
https://fedorahosted.org/bind-dyndb-ldap/changeset/f390c944336657c604941278b9647449b69c9a8c


Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

On 09/05/2012 01:18 PM, Martin Kosek wrote:

>You are right, IPv6 networks could have default /64 prefix. However as I wrote
>in different mail, I don't recommend to use default IPv4 prefix at all because
>FreeIPA targets for company environments where /24 is not so common, not for
>home environments.

+1

I think now most of us agree on that automatic prefixes does not make much
sense for IPv4. But do we really want to introduce different behavior in this
case for IPv4+IPv6 network validation and allow automatic prefix only for IPv6
only?

+1

Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0011] Make sure selinuxusemap behaves consistently to HBAC rule

On 09/03/2012 05:12 PM, Tomas Babej wrote:
> Hi,
> 
> Both selinuxusermap-add and selinuxusermap-mod commands now behave
> consistently in not allowing user/host category or user/host members
> and HBAC rule being set at the same time. Also adds a bunch of unit
> tests that check this behaviour.
> 
> https://fedorahosted.org/freeipa/ticket/2983
> 
> Tomas
> 

I found few issues with this patch:

1) Patch needs a rebase

2) Patch does not expect attributes to be set to None, i.e. to be left empty or
to be deleted, e.g.:

# ipa selinuxusermap-add foo --selinuxuser=guest_u:s0 --usercat=all --hbacrule=
ipa: ERROR: HBAC rule and local members cannot both be set

# ipa selinuxusermap-add foo --selinuxuser=guest_u:s0 --usercat=all

Added SELinux User Map "foo"

  Rule name: foo
  SELinux User: guest_u:s0
  User category: all
  Enabled: TRUE

# ipa selinuxusermap-mod foo --usercat= --hbacrule=
ipa: ERROR: HBAC rule and local members cannot both be set

# ipa selinuxusermap-mod foo --usercat=
---
Modified SELinux User Map "foo"
---
  Rule name: foo
  SELinux User: guest_u:s0
  Enabled: TRUE

# ipa selinuxusermap-mod foo --hbacrule=foo
---
Modified SELinux User Map "foo"
---
  Rule name: foo
  SELinux User: guest_u:s0
  HBAC Rule: foo
  Enabled: TRUE

# ipa selinuxusermap-mod foo --hbacrule= --usercat=all
ipa: ERROR: HBAC rule and local members cannot both be set

All these validation failures are not valid.

3) Additionally, I think it would be more readable and less error prone that if
instead of this blob:

+are_local_members_to_be_set  = 'usercategory' in _entry_attrs or \
+   'hostcategory' in _entry_attrs or \
+   'memberuser' in _entry_attrs or \
+   'memberhost' in _entry_attrs

You would use something like that:

are_local_members_to_be_set  = any(attr in _entry_attrs
   for attr in ('usercategory',
'hostcategory',
'memberuser',
'memberhost'))

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 0077 Check direct/reverse hostname/address resolution in ipa-replica-install


Petr Viktorin wrote:

On 09/04/2012 07:44 PM, Rob Crittenden wrote:

Petr Viktorin wrote:


https://fedorahosted.org/freeipa/ticket/2845


Shouldn't this also call verify_fqdn() on the local hostname and not
just the master? I think this would eventually fail in the conncheck but
what if that was skipped?

rob


A few lines above there is a call to get_host_name, which will call
verify_fqdn.



Ok, well I tested this by commenting out the host's entry in /etc/hosts 
and removing it from DNS, and it failed at the conn check.


I suppose I could have nscd still running, I'll take another look.

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] ipadb_iterate(): handle match_entry == NULL

On 08/21/2012 03:06 PM, Simo Sorce wrote:
> - Original Message -
>> - Original Message -
>>> Hi,
>>>
>>> there was an issue reported yesterday on #freeipa
>>> (https://fedorahosted.org/freeipa/ticket/3011). It is easy to
>>> reproduce
>>> 'kdb5_util dump' just core dumps. The attached patch adds a
>>> parameter
>>> check to the call where the core dump occured and fixes the reason
>>> why
>>> the parameter was invalid.
>>>
>>> Please note that 'kdb5_util dump' will return 'kdb5_util: error
>>> performing Kerberos version 5 release 1.8 dump (Plugin does not
>>> support
>>> the operation)' with the patch applied, because
>>> ipadb_iterate_pwd_policy() is not implemented.
>>
>> Can you open a ticket on this ? We used the dump utility in the past
>> to perform bulk master key changes in the past and we do want to
>> have the option open bot for backup purposes and other reasons.
> 
> Also obvious ack to the patch.
> 
> Simo.
> 

Verified that this patch removes the segfault.

ACK#2. Pushed to master, ipa-3-0.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

2012-09-05 Thread Simo Sorce

On Wed, 2012-09-05 at 11:30 +0200, Jan Cholasta wrote:
> Dne 5.9.2012 10:04, Martin Kosek napsal(a):
> > We allowed IP addresses without network specification which lead
> > to unexpected results when the zone was being created. We should rather
> > strictly require the prefix/netmask specifying the IP network that
> > the reverse zone should be created for. This is already done in
> > Web UI.
> >
> > A unit test exercising this new validation was added.
> >
> > https://fedorahosted.org/freeipa/ticket/2461
> >
> 
> I don't like this much. I would suggest using CheckedIPAddress and not 
> forcing the user to enter the prefix length instead.
> 
> CheckedIPAddress uses a sensible default prefix length if one is not 
> specified (class-based for IPv4, /64 for IPv6)

IPv4 classes were already dead and not relevant last century Jan, so
class-based netmask is really useless, if we want to use a default for
ipv4 I would use /24 for any address, that's the simplest guess you can
make ,a nd will still be often wrongt, but certainly less wrong than
using the outdated 'class' concept.

Simo.

>  as opposed to IPNetwork 
> (/32 for IPv4, /128 for IPv6 - this causes the erroneous reverse zones 
> to be created as described in the ticket).
> 
> Honza
> 


-- 
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] 298 Add safe updates for objectClasses

On 09/05/2012 09:22 AM, Martin Kosek wrote:
> On 09/05/2012 03:47 AM, Rob Crittenden wrote:
>> Rob Crittenden wrote:
>>> Martin Kosek wrote:
 On 08/30/2012 02:53 PM, Rob Crittenden wrote:
> Martin Kosek wrote:
>> Current objectclass updates in a form of "replace" update instruction
>> dependent on exact match of the old object class specification in the
>> update instruction and the real value in LDAP. However, this
>> approach is
>> very error prone as object class definition can easily differ as for
>> example because of unexpected X-ORIGIN value. Such objectclass update
>> failures may lead to serious malfunctions later.
>>
>> Add new update instruction type "replaceoc" with the following format:
>> replaceoc:OID:new
>> This update instruction will always replace an objectclass with
>> specified OID with the new definition.
>>
>> https://fedorahosted.org/freeipa/ticket/2440
>
> This works ok. Martin and I had a conversation in IRC about it.
>
> This moves from replacing a specific bit of schema with a new one, in
> all
> cases. I wonder if we should be more conservative and know what we're
> replacing
> in advance.
>
> rob
>

 You are right, I was too harsh when replacing the objectclasses. This
 would
 cause issues when LDAP update would be run on a replica with lower
 version and
 older objectclass definitions.

 I came up with an alternative solution and instead of always replacing
 the
 objectclass I rather reverted to old-OC:new-OC style which should be
 safer.
 Now, the LDAP updater always normalizes an objectclass before
 comparing it
 using python-ldap objectclass model. With this approach, objectclasses
 differing only in X-ORIGIN or white spaces should match and be updated.

 Martin

>>>
>>> NACK
>>>
>>> I think this:
>>>
>>> +for value in replaced_values:
>>> +entry_values.remove(old)
>>>
>>> Should be:
>>>
>>> +entry_values.remove(value)
> 
> Right. Thanks for the fix. It only worked in my testing because I had no
> objectclass update which would differ in X-ORIGIN or whitespaces or case. I
> tried to mangle an update file and with this fix it did the update even if
> X-ORIGIN and whitespaces differed.
> 
>>>
>>> I'm still doing other testing but this is what I've found so far.
>>>
>>> rob
>>
>> I did some more testing and it looks like this will do the trick.
>>
>> I also found a place where the schema was left as unicode and causing it to
>> blow up inside python-ldap. Here is the diff on my working instance:
>>
>> diff -u  ipaserver/install/ldapupdate.py
>> /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py
>> --- ipaserver/install/ldapupdate.py 2012-09-04 16:59:33.210688723 -0400
>> +++ /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py 
>> 2012-09-04
>> 21:47:01.583574375 -0400
>> @@ -643,7 +643,7 @@
>>  self.debug('replace: no match for replaced
>> ObjectClass "%s"', old)
>>  continue
>>  for value in replaced_values:
>> -entry_values.remove(old)
>> +entry_values.remove(value)
>>  else:
>>  entry_values.remove(old)
>>  entry_values.append(new)
>> @@ -772,7 +772,11 @@
>>  updated = False
>>  changes = self.conn.generateModList(entry.origDataDict(),
>> entry.toDict())
>>  if (entry.dn == DN(('cn', 'schema'))):
>> -updated = self.is_schema_updated(entry.toDict())
>> +d = dict()
>> +e = entry.toDict()
>> +for k,v in e.items():
>> +d[k] = [str(x) for x in v]
>> +updated = self.is_schema_updated(d)
>>  else:
>>  if len(changes) >= 1:
>>  updated = True
>>
>> rob
>>
> 
> I did not hit this error during testing, but at least it won't harm. Sending 
> an
> updated patch.
> 

Sending a slightly updated patch which now makes sure that we always pass
objectclass of str (and not unicode) type to ObjectClass model. This should
make it more bullet-proof.

Martin
>From 22c4d3343039644eca5a2d5838b3149a1ff6807b Mon Sep 17 00:00:00 2001
From: Martin Kosek 
Date: Tue, 4 Sep 2012 13:18:54 +0200
Subject: [PATCH] Add safe updates for objectClasses

Current objectclass updates in a form of "replace" update instruction
dependent on exact match of the old object class specification in the
update instruction and the real value in LDAP. However, this approach is
very error prone as object class definition can easily differ as for
example because of unexpected

[Freeipa-devel] [PATCH 0013] Remove user-unfriendly "u" character from error messages

2012-09-05 Thread Tomas Babej


Hi,

User-unfriendly errors were caused by re-raising errors
from external python module netaddr.

https://fedorahosted.org/freeipa/ticket/2588

Tomas
>From 34f3da391a8e070b29640b0ecdfed6db81b86ce2 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Wed, 5 Sep 2012 09:03:18 -0400
Subject: [PATCH] Remove user-unfriendly "u" character from error messages

User-unfriendly errors were caused by re-raising errors
from external python module netaddr.

https://fedorahosted.org/freeipa/ticket/2588
---
 ipapython/ipautil.py | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index a212aa6efc02023d323ba196e9e93bc6c49a1129..bc355b3aba84b95f7a204e4114b8b04c0987ebdd 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -105,18 +105,19 @@ class CheckedIPAddress(netaddr.IPAddress):
 try:
 try:
 addr = netaddr.IPAddress(addr, flags=self.netaddr_ip_flags)
-except netaddr.AddrFormatError:
+except netaddr.AddrFormatError as e:
 # netaddr.IPAddress doesn't handle zone indices in textual
 # IPv6 addresses. Try removing zone index and parse the
 # address again.
+msg = e.message.replace(" u'"," '") # remove the unicode sign
 if not isinstance(addr, basestring):
-raise
+raise netaddr.AddrFormatError(msg)
 addr, sep, foo = addr.partition('%')
 if sep != '%':
-raise
+raise netaddr.AddrFormatError(msg)
 addr = netaddr.IPAddress(addr, flags=self.netaddr_ip_flags)
 if addr.version != 6:
-raise
+raise netaddr.AddrFormatError(msg)
 except ValueError:
 net = netaddr.IPNetwork(addr, flags=self.netaddr_ip_flags)
 if not parse_netmask:
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0013] Remove user-unfriendly "u" character from error messages


On 09/05/2012 03:19 PM, Tomas Babej wrote:

Hi,

User-unfriendly errors were caused by re-raising errors
from external python module netaddr.

https://fedorahosted.org/freeipa/ticket/2588

Tomas


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel



I don't agree with this approach. Raising another module's errors in our 
code is ugly, and will break if netaddr changes. The arguments to pass 
to the exceptions are undocumented (see 
http://packages.python.org/netaddr/api.html#custom-exceptions). The 
wording of error messages in libraries can usually change at any time, 
and is intended for developers, not end users.


This should be either fixed upstream (unlikely, using the repr() of the 
argument is a sane thing to do at their side), or we should pass 
bytestrings to netaddr (a possible quick fix, not sure it it'll work), 
or, ideally, we should raise IPA's own errors.


--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0051-0052] Log successful reconnection to LDAP server


On 09/05/2012 01:29 PM, Adam Tkac wrote:

On Wed, Aug 15, 2012 at 01:20:08PM +0200, Petr Spacek wrote:

Hello,

this two patches solves upstream ticket
https://fedorahosted.org/bind-dyndb-ldap/ticket/71
"Log successful reconnect"

Patch 51:
 Adds log_info(): logging facility with log level INFO.


Ack.


Patch 52:
 Logs successful reconnection to LDAP server.

 LDAP connection error handling was modified:
 Errors are handled exclusively by handle_connection_error() now.

 Direct calls to ldap_connect() and ldap_reconnect() should be avoided.


Nack, please check my comments below.


Thanks for your comments! Corrected patches are attached + I replied in-line.



Regards, Adam



 From 06f03d8b602656dc9581abc675c943d6fa6a6db2 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Wed, 15 Aug 2012 12:57:32 +0200
Subject: [PATCH] Log successful reconnection to LDAP server.

LDAP connection error handling was modified:
Errors are handled exclusively by handle_connection_error() now.

Direct calls to ldap_connect() and ldap_reconnect() should be avoided.

https://fedorahosted.org/bind-dyndb-ldap/ticket/71

Signed-off-by: Petr Spacek 
---
  src/ldap_helper.c | 37 -
  1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 
cc7003a6cdcd2d27404fec936623ed8a3e8fa7f8..798aeadfef27d7071a1dd4133b7f08a21918ef78
 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1734,7 +1734,7 @@ ldap_query(ldap_instance_t *ldap_inst, ldap_connection_t 
*ldap_conn,
 * successful
 * TODO: handle this case inside ldap_pool_getconnection()?
 */
-   CHECK(ldap_connect(ldap_inst, ldap_conn, ISC_FALSE));
+   CHECK(handle_connection_error(ldap_inst, ldap_conn, ISC_FALSE));
}

  retry:
@@ -1903,14 +1903,16 @@ ldap_connect(ldap_instance_t *ldap_inst, 
ldap_connection_t *ldap_conn,
int ret;
int version;
struct timeval timeout;
+   isc_result_t result;


I would recommend to be consistent here and use "isc_result_t result = 
ISC_R_FAILURE"
My fault, corrected. It is definitely better to initialize it to avoid 
surprises in a future.



+   REQUIRE(ldap_inst != NULL);
REQUIRE(ldap_conn != NULL);

ret = ldap_initialize(&ld, str_buf(ldap_inst->uri));
if (ret != LDAP_SUCCESS) {
log_error("LDAP initialization failed: %s",
  ldap_err2string(ret));
-   goto cleanup;
+   CHECK(ISC_R_FAILURE);


Please use goto here, as done in rest of code.

My intent was to replace
result = ISC_R_...;
goto cleanup;
with
CHECK(ISC_R_...);
to make it more readable. I don't like "alone" goto, because "result" value 
can change after code changes in future.


Well, CHECK() wasn't good choice for this. I introduced CLEANUP_WITH() macro 
to distinguish jump with constant result from CHECK(function_call()) cases.


Attached patch 0052a adds CLEANUP_WITH() macro.

This distinguishing should allow to realize failure injection to CHECK() 
macros (in far future :-).



}

version = LDAP_VERSION3;
@@ -1932,21 +1934,22 @@ ldap_connect(ldap_instance_t *ldap_inst, 
ldap_connection_t *ldap_conn,
if (ldap_conn->handle != NULL)
ldap_unbind_ext_s(ldap_conn->handle, NULL, NULL);
ldap_conn->handle = ld;
+   ld = NULL; /* prevent double-unbind from ldap_reconnect() and cleanup: 
*/

-   return ldap_reconnect(ldap_inst, ldap_conn, force);
+   CHECK(ldap_reconnect(ldap_inst, ldap_conn, force));
+   return result;

  cleanup:
-
if (ld != NULL)
ldap_unbind_ext_s(ld, NULL, NULL);

/* Make sure handle is NULL. */
if (ldap_conn->handle != NULL) {
ldap_unbind_ext_s(ldap_conn->handle, NULL, NULL);
ldap_conn->handle = NULL;
}

-   return ISC_R_FAILURE;
+   return result;
  }

  static isc_result_t
@@ -2064,12 +2067,18 @@ handle_connection_error(ldap_instance_t *ldap_inst, 
ldap_connection_t *ldap_conn
  {
int ret;
int err_code;
+   isc_result_t result = ISC_R_FAILURE;

-   ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE,
- (void *)&err_code);
+   REQUIRE(ldap_conn != NULL);


...


-   if (ret != LDAP_OPT_SUCCESS) {
-   log_error("handle_connection_error failed to obtain ldap error 
code");
+   if (ldap_conn->handle != NULL) {
+   ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE,
+ (void *)&err_code);
+   if (ret != LDAP_OPT_SUCCESS) {
+   log_error("handle_connection_error failed to obtain ldap 
error code");
+   }
+   }
+   if (ldap_conn->handle == NULL || ret != LDAP_OPT_SUCCESS) {
goto reconn

Re: [Freeipa-devel] [PATCH 0042] Flush zones and RRs cache when handling persistent search reconnection


On 08/27/2012 02:40 PM, Dmitri Pal wrote:

On 08/15/2012 05:18 AM, Simo Sorce wrote:

- Original Message -

On 08/14/2012 08:25 PM, Simo Sorce wrote:

See man ldap_result, the entries return with type
LDAP_RES_SEARCH_ENTRY, the last message is instead
LDAP_RES_SEARCH_RESULT which tells you the searc is complete.

This last message is never sent for a persistent search of course
(IIRC :-).

Right, it is what I tried to say with "there is no SearchResultDone
LDAP message".

I see.


http://tools.ietf.org/html/draft-smith-psearch-ldap-01#page-3
section 4 paragraph b).

This patch deals with persistent search only. Non-persistent search
scenarios
have cache records with limited TTL value.


But in case of a persistent search you should never need to flush
as entries are valid until you see a change.

Unfortunately, cache flush is necessary after persistent search
reconnection.
Records can change in meanwhile...

Example:
1. BIND started, cache was populated.
2. Persistent search connection was lost
3. Record was deleted (but Entry Change Notification wasn't
delivered)
4. Persistent search connection was re-established - there is no
information
about deleted record/zone, BIND still sees old data in the cache.

For this reason https://fedorahosted.org/bind-dyndb-ldap/ticket/44
was filled.

What I missed?

You missed nothing, I did.

Howeve I have an idea to handle this situation smartly.

We can issue 2 searches on 2 separate connections.

The first search will be a persistent search, however it will be started with a 
filter that has an additional element.
Before the persistent search we do a rootdse search and find either the higher 
modifyTimestamp or the higher entryUSN or equivalent, depending on what is 
available in the server. Worst case we do a ase search of the DNS tree and pick 
that modifyTimestamp.
Then we start the persistent search with (&(entryUSN>%d)) or 
(&(modifyTimestamp>=%d) (note modifyTimestamp requires >= due to low 
resolution).

On the other connection we start instead a nornmal search. This normal search 
will terminate with a 'done' result, so you know that once that search is 
finished you can flush any cache that has not been re-validated. While you get 
any changes that are occurring live on the persistent search connection.

This way we should be able to not loose any update and still know when we got 
all results and drop unvalidated caches.



What is the status of this proposal?


Oh, this mail got lost in my inbox, sorry.

I created ticket:
https://fedorahosted.org/bind-dyndb-ldap/ticket/86
"Improve psearch cache flush reliability"

I will return back to this proposal after resolving more actual issues.

Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0013] Remove user-unfriendly "u" character from error messages

2012-09-05 Thread Tomas Babej


On 09/05/2012 03:42 PM, Petr Viktorin wrote:

On 09/05/2012 03:19 PM, Tomas Babej wrote:

Hi,

User-unfriendly errors were caused by re-raising errors
from external python module netaddr.

https://fedorahosted.org/freeipa/ticket/2588

Tomas


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel



I don't agree with this approach. Raising another module's errors in 
our code is ugly, and will break if netaddr changes. The arguments to 
pass to the exceptions are undocumented (see 
http://packages.python.org/netaddr/api.html#custom-exceptions). The 
wording of error messages in libraries can usually change at any time, 
and is intended for developers, not end users.


This should be either fixed upstream (unlikely, using the repr() of 
the argument is a sane thing to do at their side), or we should pass 
bytestrings to netaddr (a possible quick fix, not sure it it'll work), 
or, ideally, we should raise IPA's own errors.


Well, this particular fix wouldn't have broken anything, since it was 
raising the same error that the except clause in which the raising 
occured caught. However, I changed this to StandardError, since the 
error message is extracted and packed into ValidationError during 
further validation and therefore simple format message is suitable.


Tomas
>From e39ef6e03cff06869649fa190b7a19211d886aa3 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Wed, 5 Sep 2012 09:03:18 -0400
Subject: [PATCH] Remove user-unfriendly "u" character from error messages

User-unfriendly errors were caused by re-raising errors
from external python module netaddr.

https://fedorahosted.org/freeipa/ticket/2588
---
 ipapython/ipautil.py | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index a212aa6efc02023d323ba196e9e93bc6c49a1129..f7e0a0cd0ffb7b874ac74240e828350508e9f268 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -50,6 +50,7 @@ from ipapython.ipa_log_manager import *
 from ipapython import ipavalidate
 from ipapython import config
 from ipapython.dn import DN
+from ipalib import errors
 
 try:
 from subprocess import CalledProcessError
@@ -105,18 +106,19 @@ class CheckedIPAddress(netaddr.IPAddress):
 try:
 try:
 addr = netaddr.IPAddress(addr, flags=self.netaddr_ip_flags)
-except netaddr.AddrFormatError:
+except netaddr.AddrFormatError as e:
 # netaddr.IPAddress doesn't handle zone indices in textual
 # IPv6 addresses. Try removing zone index and parse the
 # address again.
+msg = e.message.replace(" u'"," '") # remove the unicode sign
 if not isinstance(addr, basestring):
-raise
+raise StandardError(msg)
 addr, sep, foo = addr.partition('%')
 if sep != '%':
-raise
+raise StandardError(msg)
 addr = netaddr.IPAddress(addr, flags=self.netaddr_ip_flags)
 if addr.version != 6:
-raise
+raise StandardError(msg)
 except ValueError:
 net = netaddr.IPNetwork(addr, flags=self.netaddr_ip_flags)
 if not parse_netmask:
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0051-0052] Log successful reconnection to LDAP server

On Wed, Sep 05, 2012 at 03:53:36PM +0200, Petr Spacek wrote:
> On 09/05/2012 01:29 PM, Adam Tkac wrote:
> >On Wed, Aug 15, 2012 at 01:20:08PM +0200, Petr Spacek wrote:
> >>Hello,
> >>
> >>this two patches solves upstream ticket
> >>https://fedorahosted.org/bind-dyndb-ldap/ticket/71
> >>"Log successful reconnect"
> >>
> >>Patch 51:
> >> Adds log_info(): logging facility with log level INFO.
> >
> >Ack.
> >
> >>Patch 52:
> >> Logs successful reconnection to LDAP server.
> >>
> >> LDAP connection error handling was modified:
> >> Errors are handled exclusively by handle_connection_error() now.
> >>
> >> Direct calls to ldap_connect() and ldap_reconnect() should be avoided.
> >
> >Nack, please check my comments below.
> 
> Thanks for your comments! Corrected patches are attached + I replied in-line.

Ack

-- 
Adam Tkac, Red Hat, Inc.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH] 209 Fixed problem while deleting entry with unsaved changes

2012-09-05 Thread Petr Vobornik

While deleting an entry it now resets a facet if there are unsaved 
changes. It prevents pop up of various error dialogs when UI tries to 
redirect to search page after successful delete.


https://fedorahosted.org/freeipa/ticket/3047
--
Petr Vobornik
From eb8b01780c2da205b13f95a275c56269320db6c4 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Wed, 5 Sep 2012 16:55:57 +0200
Subject: [PATCH] Fixed problem while deleting entry with unsaved changes

While deleting an entry it now resets a facet if there are unsaved changes. It prevents pop up of various error dialogs when UI tries to redirect to search page after successful delete.

https://fedorahosted.org/freeipa/ticket/3047
---
 install/ui/details.js | 9 +
 install/ui/ipa.js | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/install/ui/details.js b/install/ui/details.js
index 883bb06bfd56ffd800f1ce60e229bc0c1f4ec1bb..13a895da99f9be24ca2f7c2ab65ba744c51192f8 100644
--- a/install/ui/details.js
+++ b/install/ui/details.js
@@ -1150,6 +1150,8 @@ IPA.object_action = function(spec) {
 return IPA.confirm(msg);
 };
 
+that.object_execute_action = that.execute_action;
+
 return that;
 };
 
@@ -1191,6 +1193,13 @@ IPA.delete_action = function(spec) {
 
 var that = IPA.object_action(spec);
 
+that.execute_action = function(facet, on_success, on_error) {
+
+if (facet.is_dirty()) facet.reset();
+
+that.object_execute_action(facet, on_success, on_error);
+};
+
 that.on_success = function(facet, data, text_status, xhr) {
 
 IPA.notify_success(data.result.summary);
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index 7df4deed3f1b6e064a832ff3a2a0ba91d1538da7..23c9933dfb97cb39a932f78d235fdaf844e42b7c 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -2042,6 +2042,8 @@ IPA.confirm = function(msg) {
 
 IPA.notify_success = function(message) {
 
+if (!message) return; // don't show undefined, null and such
+
 function destroy_timeout() {
 if (IPA.notify_success.timeout) window.clearTimeout(IPA.notify_success.timeout);
 }
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH] 303 Add range safety check for range_mod and range_del

range_mod and range_del command could easily create objects with
ID which is suddenly out of specified range. This could cause issues
in trust scenarios where range objects are used for computation of
remote IDs.

Add validator for both commands to check if there is any object with
ID in the range which would become out-of-range as a pre_callback.
Also add unit tests testing this new validator.

https://fedorahosted.org/freeipa/ticket/2919
>From a1f80e487fcfadaf5c32bffbf1c0ee7799267e06 Mon Sep 17 00:00:00 2001
From: Martin Kosek 
Date: Wed, 5 Sep 2012 12:28:42 +0200
Subject: [PATCH] Add range safety check for range_mod and range_del

range_mod and range_del command could easily create objects with
ID which is suddenly out of specified range. This could cause issues
in trust scenarios where range objects are used for computation of
remote IDs.

Add validator for both commands to check if there is any object with
ID in the range which would become out-of-range as a pre_callback.
Also add unit tests testing this new validator.

https://fedorahosted.org/freeipa/ticket/2919
---
 ipalib/plugins/range.py|  81 ++
 tests/test_xmlrpc/test_range_plugin.py | 151 +++--
 2 files changed, 225 insertions(+), 7 deletions(-)

diff --git a/ipalib/plugins/range.py b/ipalib/plugins/range.py
index 95b00b39bc64723829a5d17ed22590199d1d516f..cc0c127531bb608fbe22b3332660e6412c28747d 100644
--- a/ipalib/plugins/range.py
+++ b/ipalib/plugins/range.py
@@ -91,6 +91,58 @@ class range(LDAPObject):
 if not options.get('all', False) or options.get('pkey_only', False):
 entry_attrs.pop('objectclass', None)
 
+def check_ids_in_modified_range(self, old_base, old_size, new_base, new_size):
+if new_base is None and new_size is None:
+# nothing to check
+return
+if new_base is None:
+new_base = old_base
+if new_size is None:
+new_size = old_size
+old_interval = (old_base, old_base + old_size - 1)
+new_interval = (new_base, new_base + new_size - 1)
+checked_intervals = []
+low_diff = new_interval[0] - old_interval[0]
+if low_diff > 0:
+checked_intervals.append(
+(old_interval[0], min(old_interval[1], new_interval[0] - 1)))
+high_diff = old_interval[1] - new_interval[1]
+if high_diff > 0:
+checked_intervals.append(
+(max(old_interval[0], new_interval[1] + 1), old_interval[1]))
+
+if not checked_intervals:
+# range is equal or covers the entire old range, nothing to check
+return
+
+ldap = self.backend
+id_filter_base = ["(objectclass=posixAccount)",
+  "(objectclass=posixGroup)",
+  "(objectclass=ipaIDObject)"]
+id_filter_ids = []
+
+for id_low, id_high in checked_intervals:
+id_filter_ids.append("(&(uidNumber>=%(low)d)(uidNumber<=%(high)d))"
+ % dict(low=id_low, high=id_high))
+id_filter_ids.append("(&(gidNumber>=%(low)d)(gidNumber<=%(high)d))"
+ % dict(low=id_low, high=id_high))
+id_filter = ldap.combine_filters(
+[ldap.combine_filters(id_filter_base, "|"),
+  ldap.combine_filters(id_filter_ids, "|")],
+"&")
+
+try:
+(objects, truncated) = ldap.find_entries(filter=id_filter,
+attrs_list=['uid', 'cn'],
+base_dn=DN(api.env.container_accounts, api.env.basedn))
+except errors.NotFound:
+# no objects in this range found, allow the command
+pass
+else:
+raise errors.ValidationError(name="ipabaseid,ipaidrangesize",
+error=_('range modification leaving objects with ID out '
+'of the defined range is not allowed'))
+
 class range_add(LDAPCreate):
 __doc__ = _('Add new ID range.')
 
@@ -121,6 +173,18 @@ class range_del(LDAPDelete):
 
 msg_summary = _('Deleted ID range "%(value)s"')
 
+def pre_callback(self, ldap, dn, *keys, **options):
+try:
+(old_dn, old_attrs) = ldap.get_entry(dn, ['ipabaseid', 'ipaidrangesize'])
+except errors.NotFound:
+self.obj.handle_not_found(*keys)
+
+old_base_id = int(old_attrs.get('ipabaseid', [0])[0])
+old_range_size = int(old_attrs.get('ipaidrangesize', [0])[0])
+self.obj.check_ids_in_modified_range(
+old_base_id, old_range_size, 0, 0)
+return dn
+
 class range_find(LDAPSearch):
 __doc__ = _('Search for ranges.')
 
@@ -161,6 +225,23 @@ class range_mod(LDAPUpdate):
 def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
 assert isinstance(dn, DN)
 attrs_list.append('objectcl

Re: [Freeipa-devel] [PATCH 0051-0052] Log successful reconnection to LDAP server


On 09/05/2012 04:37 PM, Adam Tkac wrote:

On Wed, Sep 05, 2012 at 03:53:36PM +0200, Petr Spacek wrote:

On 09/05/2012 01:29 PM, Adam Tkac wrote:

On Wed, Aug 15, 2012 at 01:20:08PM +0200, Petr Spacek wrote:

Hello,

this two patches solves upstream ticket
https://fedorahosted.org/bind-dyndb-ldap/ticket/71
"Log successful reconnect"

Patch 51:
 Adds log_info(): logging facility with log level INFO.


Ack.


Pushed to master:
https://fedorahosted.org/bind-dyndb-ldap/changeset/3d5310d29f709f83fabb33f54356efa2acf5582d




Patch 52:
 Logs successful reconnection to LDAP server.

 LDAP connection error handling was modified:
 Errors are handled exclusively by handle_connection_error() now.

 Direct calls to ldap_connect() and ldap_reconnect() should be avoided.


Nack, please check my comments below.


Thanks for your comments! Corrected patches are attached + I replied in-line.


Ack


Pushed to master:
https://fedorahosted.org/bind-dyndb-ldap/changeset/ea9c3287f9e01dd932116e39de6cb592862f7591
(CLEANUP_WITH macro)

and

https://fedorahosted.org/bind-dyndb-ldap/e89735d969dc79d632f2c7776f5e88b934ccce57
(patch itself)

Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0053] Use richer set of return codes for LDAP connection error handling code


On 09/05/2012 01:33 PM, Adam Tkac wrote:

On Wed, Aug 15, 2012 at 01:23:45PM +0200, Petr Spacek wrote:

Hello,

current code return very generic ISC_R_FAILURE code in nearly all (error) cases.

This patch distinguishes between different LDAP errors and returns
richer set of return codes from LDAP connection error handling code.

It should lead to clearer log messages.

Petr^2 Spacek


Ack


Pushed to master:
https://fedorahosted.org/bind-dyndb-ldap/changeset/64310eb7c9a5c0489c82f2f31627874bf7c6b230

Petr^2 Spacek




 From 15d6b38c9eda5b05d799c145ede8341f359e8633 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Wed, 15 Aug 2012 13:01:48 +0200
Subject: [PATCH] Use richer set of return codes for LDAP connection error
  handling code.

It should lead to clear log messages.



___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 199 Permissions: select only applicable options on type change


On 9/3/2012 5:59 AM, Petr Vobornik wrote:

Updated patch attached.


ACK.

--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 197 Fixed search in HBAC test


On 9/3/2012 6:28 AM, Petr Vobornik wrote:

b) force refresh when searching with unchanged filter


I did (b). Updated patch attached.

I don't want to implement 'expiration date' at the moment. It's too
widespread change. Maybe in FreeIPA 3.2.


ACK.

--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 203 Notify success on add, delete and update


On 9/3/2012 8:35 AM, Petr Vobornik wrote:

Notification of success was added to:
  * details facet: update
  * association facet and association widget: add, delete items
  * attribute facet: delete items (notification of add should be handled
in entity adder dialog)
  * sudo rule: add, remove option
  * dnsrecord: add, update, delete

https://fedorahosted.org/freeipa/ticket/2977


ACK. Some minor issues below:

1. The notification for operations involving multiple entries are a bit 
inconsistent. In the search facet deleting entries will generate 
"Selected entries were deleted." Enabling/disabling entries will 
generate " items were enabled/disabled". In the association facet, 
HBAC/sudo rules, adding/deleting entries will generate "Items 
added/removed." For sudo options it's "Option/Option(s) added/removed."


2. The notification for operations involving a single entry are a bit 
consistent too, but I think this is an existing issue. If you add an 
entry the notification will say " successfully added". It 
doesn't show the primary key which may be generated by the server. As a 
comparison if you delete an entry from the details page it will say 
"Deleted  ''", and for update operation " 
 updated."


3. Also existing issue, if you add using "Add and Add Another" the 
notification doesn't fade away, does it matter?


Feel free to address this separately.

--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 1041 pull in cachememsize logging


Rob Crittenden wrote:

389-ds-base added logging if the entry cache is smaller than the
database so users will know they need to tune their DS install. Set this
as the minimum for IPA.

rob


Rebased patch.

rob

>From 131a95cf91bf1026f7afb2aa73251c92fc7e9822 Mon Sep 17 00:00:00 2001
From: Rob Crittenden 
Date: Mon, 20 Aug 2012 15:07:38 -0400
Subject: [PATCH] Set minimum of 389-ds-base to 1.2.11.8 to pick up cache
 warning.

If the DB is bigger than nsslapd-cachememsize then a warning will be
logged by 389-ds-base.

https://fedorahosted.org/freeipa/ticket/2739
---
 freeipa.spec.in | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index c371e9ded093abef6bc7298c7ddd3b39fe020f44..0a213329036b162fac1cf242d976da95d4f26ba7 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -101,7 +101,7 @@ Requires: %{name}-client = %{version}-%{release}
 Requires: %{name}-admintools = %{version}-%{release}
 Requires: %{name}-server-selinux = %{version}-%{release}
 %if 0%{?fedora} >= 17
-Requires(pre): 389-ds-base >= 1.2.11.5-1
+Requires(pre): 389-ds-base >= 1.2.11.8-1
 %else
 Requires(pre): 389-ds-base >= 1.2.10.10-1
 %endif
@@ -752,6 +752,14 @@ fi
 %ghost %attr(0644,root,apache) %config(noreplace) %{_sysconfdir}/ipa/ca.crt
 
 %changelog
+* Mon Aug 20 2012 Rob Crittenden  - 2.99.0-44
+- Set min for 389-ds-base to 1.2.11.11-1 on F17+ to pull in updated
+  RUV code and nsslapd-readonly schema.
+
+* Mon Aug 20 2012 Rob Crittenden  - 2.99.0-43
+- Set min for 389-ds-base to 1.2.11.9-1 on F17+ to pull in warning about
+  low nsslapd-cachememsize.
+
 * Mon Aug 20 2012 Tomas Babej  - 2.99.0-42
 - Add samba4-winbind to build dependencies for AD server-side code
 
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 1031 run cleanallruv task

Rob Crittenden wrote:

Martin Kosek wrote:

On 07/05/2012 08:39 PM, Rob Crittenden wrote:

Martin Kosek wrote:

On 07/03/2012 04:41 PM, Rob Crittenden wrote:

Deleting a replica can leave a replication vector (RUV) on the
other servers.
This can confuse things if the replica is re-added, and it also
causes the
server to calculate changes against a server that may no longer exist.

389-ds-base provides a new task that self-propogates itself to all
available
replicas to clean this RUV data.

This patch will create this task at deletion time to hopefully
clean things up.

It isn't perfect. If any replica is down or unavailable at the time
the
cleanruv task fires, and then comes back up, the old RUV data may be
re-propogated around.

To make things easier in this case I've added two new commands to
ipa-replica-manage. The first lists the replication ids of all the
servers we
have a RUV for. Using this you can call clean_ruv with the
replication id of a
server that no longer exists to try the cleanallruv step again.

This is quite dangerous though. If you run cleanruv against a
replica id that
does exist it can cause a loss of data. I believe I've put in
enough scary
warnings about this.

rob

Good work there, this should make cleaning RUVs much easier than
with the
previous version.

This is what I found during review:

1) list_ruv and clean_ruv command help in man is quite lost. I think
it would
help if we for example have all info for commands indented. This way
user could
simply over-look the new commands in the man page.

2) I would rename new commands to clean-ruv and list-ruv to make them
consistent with the rest of the commands (re-initialize, force-sync).

3) It would be nice to be able to run clean_ruv command in an
unattended way
(for better testing), i.e. respect --force option as we already do for
ipa-replica-manage del. This fix would aid test automation in the
future.

4) (minor) The new question (and the del too) does not react too
well for
CTRL+D:

# ipa-replica-manage clean_ruv 3 --force
Clean the Replication Update Vector for
vm-055.idm.lab.bos.redhat.com:389

Cleaning the wrong replica ID will cause that server to no
longer replicate so it may miss updates while the process
is running. It would need to be re-initialized to maintain
consistency. Be very careful.
Continue to clean? [no]: unexpected error:

5) Help for clean_ruv command without a required parameter is quite
confusing
as it reports that command is wrong and not the parameter:

# ipa-replica-manage clean_ruv
Usage: ipa-replica-manage [options]

It seems you just forgot to specify the error message in the command
definition

6) When the remote replica is down, the clean_ruv command fails with an
unexpected error:

[root@vm-086 ~]# ipa-replica-manage clean_ruv 5
Clean the Replication Update Vector for
vm-055.idm.lab.bos.redhat.com:389

/var/log/dirsrv/slapd-IDM-LAB-BOS-REDHAT-COM/errors:
[04/Jul/2012:06:28:16 -0400] NSMMReplicationPlugin -
cleanAllRUV_task: failed
to connect to replagreement connection
(cn=meTovm-055.idm.lab.bos.redhat.com,cn=replica,

cn=dc\3Didm\2Cdc\3Dlab\2Cdc\3Dbos\2Cdc\3Dredhat\2Cdc\3Dcom,cn=mapping
tree,cn=config), error 105
[04/Jul/2012:06:28:16 -0400] NSMMReplicationPlugin -
cleanAllRUV_task: replica
(cn=meTovm-055.idm.lab.
bos.redhat.com,cn=replica,cn=dc\3Didm\2Cdc\3Dlab\2Cdc\3Dbos\2Cdc\3Dredhat\2Cdc\3Dcom,cn=mapping

tree, cn=config) has not been cleaned. You will need to rerun the
CLEANALLRUV task on this replica.
[04/Jul/2012:06:28:16 -0400] NSMMReplicationPlugin -
cleanAllRUV_task: Task
failed (1)

In this case I think we should inform user that the command failed,
possibly
because of disconnected replicas and that they could enable the
replicas and
try again.

7) (minor) "pass" is now redundant in replication.py:
+except ldap.INSUFFICIENT_ACCESS:
+# We can't make the server we're removing read-only but
+# this isn't a show-stopper
+root_logger.debug("No permission to switch replica to
read-only,
continuing anyway")
+pass

I think this addresses everything.

rob

Thanks, almost there! I just found one more issue which needs to be fixed
before we push:

# ipa-replica-manage del vm-055.idm.lab.bos.redhat.com --force
Directory Manager password:

Unable to connect to replica vm-055.idm.lab.bos.redhat.com, forcing
removal
Failed to get data from 'vm-055.idm.lab.bos.redhat.com': {'desc': "Can't
contact LDAP server"}
Forcing removal on 'vm-086.idm.lab.bos.redhat.com'

There were issues removing a connection: %d format

[Freeipa-devel] [PATCH] 1053 support 389-ds posix-winsync plugin

Add support for the 389-ds posix winsync plugin. This plugin will sync 
the POSIX attributes from AD. We need to avoid trying to re-add them in 
our plugin.


rob
>From 2e1648eb60dfee7b0e3cbee679457f5e5c0fb4d0 Mon Sep 17 00:00:00 2001
From: Rob Crittenden 
Date: Fri, 31 Aug 2012 15:11:20 -0400
Subject: [PATCH] Support the new Winsync POSIX API.

This will sync down the POSIX attributes from AD so we need to be careful
to not mess with them when they are already set. This includes
uidNumber, gidNumber, homeDirectory, loginShell and gecos.

http://port389.org/wiki/WinSync_Posix
http://port389.org/wiki/Windows_Sync_Plugin_API#Version_3_API_functions

https://fedorahosted.org/freeipa/ticket/3007
---
 .../ipa-slapi-plugins/ipa-winsync/ipa-winsync.c| 56 +++---
 install/updates/10-config.update   |  5 ++
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-winsync/ipa-winsync.c b/daemons/ipa-slapi-plugins/ipa-winsync/ipa-winsync.c
index 612d607b87a99c171f04838f53b876473f4efb9b..235b37ac366ed5deff34da345f58cb464f3e8097 100644
--- a/daemons/ipa-slapi-plugins/ipa-winsync/ipa-winsync.c
+++ b/daemons/ipa-slapi-plugins/ipa-winsync/ipa-winsync.c
@@ -64,6 +64,8 @@
 #include 
 #include "plstr.h"
 
+static int ipa_winsync_precedence = 0;
+
 static void
 sync_acct_disable(
 void *cbdata, /* the usual domain config data */
@@ -268,7 +270,7 @@ ipa_winsync_pre_ds_add_user_cb(void *cbdata, const Slapi_Entry *rawentry,
 if (!type) {
 continue; /* should never happen */
 }
-
+
 if (!slapi_entry_attr_find(ds_entry, type, &e_attr) && e_attr) {
 /* already has attribute - add missing values */
 Slapi_Value *sv = NULL;
@@ -276,6 +278,14 @@ ipa_winsync_pre_ds_add_user_cb(void *cbdata, const Slapi_Entry *rawentry,
 for (ii = slapi_attr_first_value(attr, &sv); ii != -1;
  ii = slapi_attr_next_value(attr, ii, &sv))
 {
+if (!PL_strcasecmp(type, "uidNumber") ||
+!PL_strcasecmp(type, "gidNumber")) {
+LOG("--> ipa_winsync_pre_ds_add_user_cb -- "
+"skipping [%s] for new entry [%s]\n",
+type, slapi_entry_get_dn_const(ds_entry));
+/* uid or gid already set in AD, skip it */
+continue;
+}
 if (!slapi_entry_attr_has_syntax_value(ds_entry, type, sv)) {
 /* attr-value sv not found in ds_entry; add it */
 LOG("--> ipa_winsync_pre_ds_add_user_cb -- "
@@ -290,6 +300,9 @@ ipa_winsync_pre_ds_add_user_cb(void *cbdata, const Slapi_Entry *rawentry,
 slapi_attr_get_valueset(attr, &svs); /* makes a copy */
 slapi_entry_add_valueset(ds_entry, type, svs);
 slapi_valueset_free(svs); /* free the copy */
+LOG("--> ipa_winsync_pre_ds_add_user_cb -- "
+"adding attr [%s] to new entry [%s]\n",
+type, slapi_entry_get_dn_const(ds_entry));
 }
 }
 
@@ -383,8 +396,11 @@ ipa_winsync_pre_ds_add_user_cb(void *cbdata, const Slapi_Entry *rawentry,
 
 /* add a loginShell if we have a default */
 if (ipaconfig->login_shell) {
-slapi_entry_attr_set_charptr(ds_entry, "loginShell",
- ipaconfig->login_shell);
+type = "loginShell";
+if (slapi_entry_attr_find(ds_entry, type, &e_attr) || !e_attr) {
+slapi_entry_attr_set_charptr(ds_entry, "loginShell",
+ ipaconfig->login_shell);
+}
 }
 
 sync_acct_disable(cbdata, rawentry, ds_entry, ACCT_DISABLE_TO_DS,
@@ -545,6 +561,12 @@ ipa_winsync_destroy_agmt_cb(void *cbdata, const Slapi_DN *ds_subtree,
 return;
 }
 
+static int
+ipa_winsync_precedence_cb(void)
+{
+return ipa_winsync_precedence;
+}
+
 static void *ipa_winsync_api[] = {
 NULL, /* reserved for api broker use, must be zero */
 ipa_winsync_agmt_init,
@@ -565,7 +587,20 @@ static void *ipa_winsync_api[] = {
 ipa_winsync_can_add_entry_to_ad_cb,
 ipa_winsync_begin_update_cb,
 ipa_winsync_end_update_cb,
-ipa_winsync_destroy_agmt_cb
+ipa_winsync_destroy_agmt_cb,
+NULL,
+NULL,
+NULL,
+NULL,
+NULL,
+NULL,
+NULL,
+NULL,
+NULL,
+NULL,
+NULL,
+NULL,
+ipa_winsync_precedence_cb
 };
 
 /**
@@ -602,7 +637,7 @@ ipa_winsync_plugin_start(Slapi_PBlock *pb)
 
 LOG("--> ipa_winsync_plugin_start -- begin\n");
 
-	if( slapi_apib_register(WINSYNC_v1_0_GUID, ipa_winsync_api) ) {
+	if( slapi_apib_register(WINSYNC_v3_0_GUID, ipa_winsync_api) ) {
 LOG_FATAL("<-- ipa_winsync_plugin_start -- failed to register winsync api -- end\n");
 return -1;
 	}
@@ -626,7 +661,7 @@ ipa_winsync_plugin_close(Slapi_PBlock *pb)
 {
 LOG("--> ipa_winsync_plugin_close -- begin\n");
 
-	slapi_api

Re: [Freeipa-devel] [PATCH] 1053 support 389-ds posix-winsync plugin

2012-09-05 Thread Rich Megginson


On 09/05/2012 12:08 PM, Rob Crittenden wrote:
Add support for the 389-ds posix winsync plugin. This plugin will sync 
the POSIX attributes from AD. We need to avoid trying to re-add them 
in our plugin.

ack


rob


___
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] 204 Update of confirmation of actions


On 9/3/2012 11:05 AM, Petr Vobornik wrote:

This patch is changing confirmation of actions according to ticket
#3035, see the ticket description.

It does following changes:
  * Confirmation of update action was removed.
  * Action lists resets to first action (which is usually a NOP: '--
select action --') on change of displayed entry.
  * New confirmation dialog was implemented. It is used for action
confirmation. It is used in IPA.action to replace the call of
window.confirm(message). The old call is a modal window which blocks all
JS functionality and has different style than other dialogs in Web UI.
The new one has same design and doesn't block background operations.

  https://fedorahosted.org/freeipa/ticket/3035


Some issues:

1. None of the confirmation dialogs have a default button. If you hit 
Enter it will do nothing (except #2 below). All buttons are greyed out 
until you hover with mouse or focus with Tab. Is this intentional? I 
think usually a confirmation dialog would have a default button.


2. In the Users search facet the confirmation dialog doesn't show 
default button either, but if you hit Enter it will execute the 
operation. This is inconsistent with #1.


--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 298 Add safe updates for objectClasses


Martin Kosek wrote:

On 09/05/2012 09:22 AM, Martin Kosek wrote:

On 09/05/2012 03:47 AM, Rob Crittenden wrote:

Rob Crittenden wrote:

Martin Kosek wrote:

On 08/30/2012 02:53 PM, Rob Crittenden wrote:

Martin Kosek wrote:

Current objectclass updates in a form of "replace" update instruction
dependent on exact match of the old object class specification in the
update instruction and the real value in LDAP. However, this
approach is
very error prone as object class definition can easily differ as for
example because of unexpected X-ORIGIN value. Such objectclass update
failures may lead to serious malfunctions later.

Add new update instruction type "replaceoc" with the following format:
replaceoc:OID:new
This update instruction will always replace an objectclass with
specified OID with the new definition.

https://fedorahosted.org/freeipa/ticket/2440


This works ok. Martin and I had a conversation in IRC about it.

This moves from replacing a specific bit of schema with a new one, in
all
cases. I wonder if we should be more conservative and know what we're
replacing
in advance.

rob



You are right, I was too harsh when replacing the objectclasses. This
would
cause issues when LDAP update would be run on a replica with lower
version and
older objectclass definitions.

I came up with an alternative solution and instead of always replacing
the
objectclass I rather reverted to old-OC:new-OC style which should be
safer.
Now, the LDAP updater always normalizes an objectclass before
comparing it
using python-ldap objectclass model. With this approach, objectclasses
differing only in X-ORIGIN or white spaces should match and be updated.

Martin



NACK

I think this:

+for value in replaced_values:
+entry_values.remove(old)

Should be:

+entry_values.remove(value)


Right. Thanks for the fix. It only worked in my testing because I had no
objectclass update which would differ in X-ORIGIN or whitespaces or case. I
tried to mangle an update file and with this fix it did the update even if
X-ORIGIN and whitespaces differed.



I'm still doing other testing but this is what I've found so far.

rob


I did some more testing and it looks like this will do the trick.

I also found a place where the schema was left as unicode and causing it to
blow up inside python-ldap. Here is the diff on my working instance:

diff -u  ipaserver/install/ldapupdate.py
/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py
--- ipaserver/install/ldapupdate.py 2012-09-04 16:59:33.210688723 -0400
+++ /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py 2012-09-04
21:47:01.583574375 -0400
@@ -643,7 +643,7 @@
  self.debug('replace: no match for replaced
ObjectClass "%s"', old)
  continue
  for value in replaced_values:
-entry_values.remove(old)
+entry_values.remove(value)
  else:
  entry_values.remove(old)
  entry_values.append(new)
@@ -772,7 +772,11 @@
  updated = False
  changes = self.conn.generateModList(entry.origDataDict(),
entry.toDict())
  if (entry.dn == DN(('cn', 'schema'))):
-updated = self.is_schema_updated(entry.toDict())
+d = dict()
+e = entry.toDict()
+for k,v in e.items():
+d[k] = [str(x) for x in v]
+updated = self.is_schema_updated(d)
  else:
  if len(changes) >= 1:
  updated = True

rob



I did not hit this error during testing, but at least it won't harm. Sending an
updated patch.



Sending a slightly updated patch which now makes sure that we always pass
objectclass of str (and not unicode) type to ObjectClass model. This should
make it more bullet-proof.

Martin




ACK, pushed to master and ipa-3-0

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] Patch to allow IPA to work with dogtag 10 on f18


Martin Kosek wrote:

On 08/31/2012 04:53 PM, Petr Viktorin wrote:

On 08/28/2012 03:40 PM, Petr Viktorin wrote:

On 08/17/2012 06:04 PM, Ade Lee wrote:

On Fri, 2012-08-17 at 09:34 -0400, Ade Lee wrote:

On Thu, 2012-08-16 at 18:45 +0200, Martin Kosek wrote:

On 08/16/2012 01:28 PM, Ade Lee wrote:

Patch attached this time.  I should know better than to do this in the
middle of the night ..

On Thu, 2012-08-16 at 09:12 +0200, Martin Kosek wrote:

On 08/16/2012 07:53 AM, Ade Lee wrote:

On Wed, 2012-08-15 at 23:41 -0400, Ade Lee wrote:

On Wed, 2012-08-15 at 16:34 +0200, Martin Kosek wrote:

On 08/15/2012 03:54 PM, Ade Lee wrote:

On Wed, 2012-08-15 at 13:24 +0200, Martin Kosek wrote:

On 08/08/2012 10:05 PM, Ade Lee wrote:

Hi,

Dogtag 10 is being released on f18, and has a number of
changes that
will affect IPA.  In particular, the following changes will
affect
current IPA code.

* The directory layout of the dogtag instance has changed.
Instead of
using separate tomcat instances to host different
subsystems, the
standard dogtag installation will allow one to install a CA.
KRA, OCSP
and TKS within the same instance.  There have been
corresponding changes
in the directory layout, as well as the default instance name
(pki-tomcat instead of pki-ca), and startup daemon
(pki-tomcatd, instead
of pki-cad, pki-krad etc.)

* The default instance will use only four ports (HTTPS,
HTTP, AJP and
tomcat shutdown port) rather than the 6 previously used.
The default
ports will be changed to the standard tomcat ports.  As
these ports are
local to the ipa server machine, this should not cause too much
disruption.

* There is a new single step installer written in python.
(pkispawn/destroy) vs. pkicreate/pkisilent/pkiremove.

* Dogtag 10 runs on tomcat7 - with a new corresponding
version of
tomcatjss.

The attached patch integrates all the above changes in IPA
installation
and maintenance code.  Once the patch is applied, users will
be able to:

1. run ipa-server-install to completion on f18 with dogtag 10.
2. install a new replica on f18 on dogtag 10.
3. upgrade an f17 machine with an existing IPA instance to
f18/ dogtag
10 - and have that old-style dogtag instance continue to run
correctly.
This will require the installation of the latest version of
tomcatjss as
well as the installation of tomcat6.  The old-style instance
will
continue to use tomcat6.
4. in addition, the new cert renewal code has been patched
and should
continue to work.

What is not yet completed / supported:

1. Installation with an external CA is not yet completed in
the new
installer.  We plan to complete this soon.

2. There is some IPA upgrade code that has not yet been touched
(install/tools/ipa-upgradeconfig).

3. A script needs to be written to allow admins to convert
their
old-style dogtag instances to new style instances, as well
as code to
periodically prompt admins to do this.

4. Installation of old-style instances using
pkicreate/pkisilent on
dogtag 10 will no longer be supported, and will be disabled
soon.

5.  The pki-selinux policy has been updated to reflect these
changes,
but is still in flux.  In fact, it is our intention to place
the dogtag
selinux policy in the base selinux policy for f18.  In the
meantime, it
may be necessary to run installs in permissive mode.

The dogtag 10 code will be released shortly into f18.  Prior
to that
though, we have placed the new dogtag 10 and tomcatjss code
in a
developer repo that is located at
http://nkinder.fedorapeople.org/dogtag-devel/

Testing can be done on both f18 and f17 - although the
target platform -
and the only platform for which official builds will be
created is f18.

Thanks,
Ade



Hi Ade,

Thanks for the patch, I started with review and integration
tests (currently
running on Fedora 17 with Nathan's repo).

Installation on single master was smooth, it worked just
fine, even with
enforced SELinux, without any error - kudos to you and the
whole dogtag team.
The resulting logs and the structure of your log directory
seems improved. I
believe that the brand new Python installers will make it
easier to debug
issues with dogtag installation when they come.  When I tried
our unit tests or
some simple cert operation, it worked fine as well.

Now the bad news, or rather few issues and suggestions I found:

1) As we already discussed on IRC, tomcat 7 was not pulled
automatically on
Fedora 17 when I updated pki-ca, you somewhere miss a Requires.


We have a dogtag patch that is currently in review that will
address
this.  Once this is in, tomcatjss >=7.0.0 will be required for
f17+,
rather than f18+


2) I had installed IPA with dogtag10 on master. However, CA
installation on a
replica (ipa-ca-install) with dogtag9 failed - is this
expectable?


Yes.  The current IPA patch is designed to work with dogtag 10
only,
which will be officially available on f18+.  So if you update
to dogtag
10, you must have this patch and visa versa.  We probably need
to add
the relevant requires to IPA to enforce

Re: [Freeipa-devel] [PATCH] Patch to allow IPA to work with dogtag 10 on f18

2012-09-05 Thread Nalin Dahyabhai

On Wed, Aug 29, 2012 at 08:48:32AM -0400, Ade Lee wrote:
> Incidentally, I ran this in permmissive selinux mode.  The following
> rules are required to be added:
> 
> #= certmonger_t ==
> corenet_tcp_connect_http_cache_port(certmonger_t)
> files_read_var_lib_symlinks(certmonger_t)

On my system, "semanage port -l" shows me:
 http_cache_port_t  tcp  8080, 8118, 10001-10010

Are these ports already labeled this way for Dogtag, or is it a
coincidental overlap with some other package?  If it's an overlap,
it might be better to switch to using ports which aren't already labeled
for use in policy that applies to some other package.

If not, please open a bug against the selinux-policy component to get
these accesses added to the set that's allowed by the default policy.

Nalin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 83 Use OpenSSH-style public keys as the preferred format of SSH public keys

Jan Cholasta wrote:

Hi,

this patch changes the format of the sshpubkey parameter to the format
used by OpenSSH (see sshd(8)).

Public keys in the old format (raw RFC 4253 blob) are automatically
converted to OpenSSH-style public keys. OpenSSH-style public keys are
now stored in LDAP.

Changed sshpubkeyfp to be an output parameter, as that is what it
actually is.

Allow parameter normalizers to be used on values of any type, not just
unicode, so that public key blobs (which are str) can be normalized to
OpenSSH-style public keys.

Note that you need a SSSD build including

(SSSD 1.9.0beta7-to-be) in order to make OpenSSH integration actually
work with OpenSSH-style public keys.

Honza

NACK.

I think a bunch of tests are needed for this.

Because you abstracted out the pubkey class it should be straightforward 
to add a bunch of class-based unit tests on it.

There are also no user or host-based tests, either for adding or 
managing keys.

I tested backwards compatibility with 2.2 and the initial tests are mixed.

I installed 2.2 and created a 3.0 clone from it, including your patch.

I added a user in 3.0 with a key and it added ok, but on the 2.2 side it 
returns the entire base64 encoded blob of key type, key and comment, 
which I presume is unusable. At least things don't blow up.

The reverse works fine. An old-style key added to 2.2 appears to work 
fine in 3.0, we just lack a comment.

On the 2.2 server:

$ ipa user-show tuser1 --all | grep -i ssh
  Base-64 encoded SSH public key: 
c3NoLXJzYSBBQUFBQjNOemFDMXljMkVBQUFBREFRQUJBQUFCQVFDNUQyRTI2dHU5YXM2cHhlUVlSdUgzelYyUDUzMjFpR1U5aC9XNElpd0tGSGlOc2p5cXFyemhCUFB3am83dGlYRDlHbUo1M25KS21OTGd0K01XUnFTZEx2R0V3NjM3SkVTWEpGL0VWeUxvZEFWRGltdXFRVkNLWjBRcm1kYjErRUg1VGRrd3ByOExyd0g1a0RzMEVpcGc2c0xoRUZ5NzMvaXNjRkJqcmk0NGxSU1BZNXFHTWFLOVE0cjY1WFEyaytlZ1RDQnBNZnc0b0J6Mzh0ZHVEVVE2bW9XNFhQSnhZeWJ3MGFDMnRUK2RBOU42WndFSFZXREUzdzg0bHRHa0JRZFRaKzViRnBFdlladm9PbkZXdDlNZFIzYVd6UklnY1o5VDlySDFFT2Z3eE5zWVRCLzRjTmg3dS9adGxnMVV0Z1VteWN3TkpMTUYrMTNzNTl2OFFpSFogcmNyaXRAZWRzZWwuZ3JleW9hay5jb20=

$ python
Python 2.7.3 (default, Jul 24 2012, 10:05:38)
[GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import base64
>>> s = 
'c3NoLXJzYSBBQUFBQjNOemFDMXljMkVBQUFBREFRQUJBQUFCQVFDNUQyRTI2dHU5YXM2cHhlUVlSdUgzelYyUDUzMjFpR1U5aC9XNElpd0tGSGlOc2p5cXFyemhCUFB3am83dGlYRDlHbUo1M25KS21OTGd0K01XUnFTZEx2R0V3NjM3SkVTWEpGL0VWeUxvZEFWRGltdXFRVkNLWjBRcm1kYjErRUg1VGRrd3ByOExyd0g1a0RzMEVpcGc2c0xoRUZ5NzMvaXNjRkJqcmk0NGxSU1BZNXFHTWFLOVE0cjY1WFEyaytlZ1RDQnBNZnc0b0J6Mzh0ZHVEVVE2bW9XNFhQSnhZeWJ3MGFDMnRUK2RBOU42WndFSFZXREUzdzg0bHRHa0JRZFRaKzViRnBFdlladm9PbkZXdDlNZFIzYVd6UklnY1o5VDlySDFFT2Z3eE5zWVRCLzRjTmg3dS9adGxnMVV0Z1VteWN3TkpMTUYrMTNzNTl2OFFpSFogcmNyaXRAZWRzZWwuZ3JleW9hay5jb20='

>>> base64.b64decode(s)
'ssh-rsa 
B3NzaC1yc2EDAQABAAABAQC5D2E26tu9as6pxeQYRuH3zV2P5321iGU9h/W4IiwKFHiNsjyqqrzhBPPwjo7tiXD9GmJ53nJKmNLgt+MWRqSdLvGEw637JESXJF/EVyLodAVDimuqQVCKZ0Qrmdb1+EH5Tdkwpr8LrwH5kDs0Eipg6sLhEFy73/iscFBjri44lRSPY5qGMaK9Q4r65XQ2k+egTCBpMfw4oBz38tduDUQ6moW4XPJxYybw0aC2tT+dA9N6ZwEHVWDE3w84ltGkBQdTZ+5bFpEvYZvoOnFWt9MdR3aWzRIgcZ9T9rH1EOfwxNsYTB/4cNh7u/Ztlg1UtgUmycwNJLMF+13s59v8QiHZ 
rc...@edsel.greyoak.com'

Now show an old style key:

$ ipa user-show tuser2 --all | grep -i ssh
  Base-64 encoded SSH public key: 
B3NzaC1yc2EDAQABAAABAQCbRLyizFGyfucNRnHpWdUG8dBD7W2PfvTQ42k+LmAdUFudTytO89oTRXcVEYMDL42OyRth12JRMUjYTEmFwo9a9Mb7cP8+bo7N2lV4iCB0CUybcZARF0MV6NeYhhWlC9DV40nkqs3Goe8X8tMPXn/HZn8Rz33703w8K/G6STnN0txhAT4tY7D3e0DA9UY87wNnpJ7dXoJqMXRv2dRgmUnGih/8cLHypyxBoLoL8qR9cWxAf/Cs+qQmsk15lzIGQUAJwwXBBjbnXKwykEeHjTHsvjd7zzC1cWtz5Zz/8aop7AsVwaBqb9u+5dVOMxdzLGD24NKTjhtG86ADU4Mpnlb5

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] Patch to allow IPA to work with dogtag 10 on f18

2012-09-05 Thread Ade Lee

On Wed, 2012-09-05 at 16:43 -0400, Nalin Dahyabhai wrote:
> On Wed, Aug 29, 2012 at 08:48:32AM -0400, Ade Lee wrote:
> > Incidentally, I ran this in permmissive selinux mode.  The following
> > rules are required to be added:
> > 
> > #= certmonger_t ==
> > corenet_tcp_connect_http_cache_port(certmonger_t)
> > files_read_var_lib_symlinks(certmonger_t)
> 
> On my system, "semanage port -l" shows me:
>  http_cache_port_t  tcp  8080, 8118, 10001-10010
> 
> Are these ports already labeled this way for Dogtag, or is it a
> coincidental overlap with some other package?  If it's an overlap,
> it might be better to switch to using ports which aren't already labeled
> for use in policy that applies to some other package.
> 
We have specifically chosen to use what would be the default ports for
tomcat.  These ports are already labeled as you have described above.
We have adjusted our selinux policy to handle that.  In fact, we are now
extending a tomcat selinux domain provided by the system policies, and
this tomcat domain allows access to those ports.
  
> If not, please open a bug against the selinux-policy component to get
> these accesses added to the set that's allowed by the default policy.
> 
I can open a bug.

> Nalin


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 205 Reflect API change of SSH store in Web UI


On 9/5/2012 3:14 AM, Petr Vobornik wrote:

Format of ipasshpubkey in users and hosts changed from BYTES to STR. Web
UI no longer gets the value as base64 encoded string in an object.

Label was changed to reflect that the key don't have to be plain base64
encoded blob.

https://fedorahosted.org/freeipa/ticket/2989

Note: freeipa-jcholast-83-openssh-style-public-keys.patch should be applied


ACK.

Possible improvements:

1. Right now if you click 'Add' SSH public key you'd have to click 
'Show/Set key' to enter the value. We probably could make it such that 
when you click 'Add' it will open the input dialog immediately. This way 
we can avoid an incomplete state where a slot for a new key is added but 
it's empty.


2. If we do #1 the 'New: key set/not set' label can be changed to 'New 
key'. The 'Modified' can be changed to 'Modified key'.


3. The 'Show/Set key' probably can be changed to 'View/Edit' to be more 
consistent with host/service certificate.


--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 206-208 Fixed number parameters MIN boundary validation in Web UI


On 9/5/2012 9:08 AM, Petr Vobornik wrote:

Integers were missing most of minimum checks and Decimals boundaries
weren't checked at all in Web UI.

First part is done in ipalib, second in Web UI.

1) [PATCH] 206 Fixed metadata serialization of Numbers and DNs:
There were following problems:
1. DNs and Decimals weren't properly serialized. Serialization output
was object with empty __base64__ attribute. It was fixed by converting
them to string.
2. numberical values equal to 0 were excluded from metadata. It broke
many of minvalue checks in Web UI. Now excluding only None and False
values as initially intended.


ACK.


2)[PATCH] 207 Added decimal checks to metadata validator:
Metadata validator didn't have check for decimal values. It was added.


ACK.


3)[PATCH] 208 Generated metadata for testing updated
Testing metadata needs to be updated because of fix in json serialization.


ACK.

--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] Patch to allow IPA to work with dogtag 10 on f18

2012-09-05 Thread Nalin Dahyabhai

On Wed, Sep 05, 2012 at 05:08:12PM -0400, Ade Lee wrote:
> On Wed, 2012-09-05 at 16:43 -0400, Nalin Dahyabhai wrote:
> > On Wed, Aug 29, 2012 at 08:48:32AM -0400, Ade Lee wrote:
> > > Incidentally, I ran this in permmissive selinux mode.  The following
> > > rules are required to be added:
> > > 
> > > #= certmonger_t ==
> > > corenet_tcp_connect_http_cache_port(certmonger_t)
> > > files_read_var_lib_symlinks(certmonger_t)
> > 
> > On my system, "semanage port -l" shows me:
> >  http_cache_port_t  tcp  8080, 8118, 10001-10010
> > 
> > Are these ports already labeled this way for Dogtag, or is it a
> > coincidental overlap with some other package?  If it's an overlap,
> > it might be better to switch to using ports which aren't already labeled
> > for use in policy that applies to some other package.
> 
> We have specifically chosen to use what would be the default ports for
> tomcat.  These ports are already labeled as you have described above.
> We have adjusted our selinux policy to handle that.  In fact, we are now
> extending a tomcat selinux domain provided by the system policies, and
> this tomcat domain allows access to those ports.

My thinking, based on the name, is that the policy expects this set of
ports to be used by squid, and actual HTTP caches, rather than arbitrary
servlet containers.  But then I suppose the policy maintainer will know
better.  Please CC me on the policy bug so that I can keep an eye on it.

Thanks,

Nalin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] Patch to allow IPA to work with dogtag 10 on f18

2012-09-05 Thread Simo Sorce

On Wed, 2012-09-05 at 17:08 -0400, Ade Lee wrote:
> On Wed, 2012-09-05 at 16:43 -0400, Nalin Dahyabhai wrote:
> > On Wed, Aug 29, 2012 at 08:48:32AM -0400, Ade Lee wrote:
> > > Incidentally, I ran this in permmissive selinux mode.  The following
> > > rules are required to be added:
> > > 
> > > #= certmonger_t ==
> > > corenet_tcp_connect_http_cache_port(certmonger_t)
> > > files_read_var_lib_symlinks(certmonger_t)
> > 
> > On my system, "semanage port -l" shows me:
> >  http_cache_port_t  tcp  8080, 8118, 10001-10010
> > 
> > Are these ports already labeled this way for Dogtag, or is it a
> > coincidental overlap with some other package?  If it's an overlap,
> > it might be better to switch to using ports which aren't already labeled
> > for use in policy that applies to some other package.
> > 
> We have specifically chosen to use what would be the default ports for
> tomcat.  These ports are already labeled as you have described above.
> We have adjusted our selinux policy to handle that.  In fact, we are now
> extending a tomcat selinux domain provided by the system policies, and
> this tomcat domain allows access to those ports.
>   
> > If not, please open a bug against the selinux-policy component to get
> > these accesses added to the set that's allowed by the default policy.
> > 
> I can open a bug.

Ade, how will the selinux policy be handled in an upgrade scenario ?
If I understand correctly you are dropping custom selinux policies from
dogtag 10 and relying on system policy going forward, so what will keep
the right labels for the old ports in an upgrade scenario ?
Or will the rpm upgrade also change ports ? Is this properly handled on
the ipa part yet ?

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] 209 Fixed problem while deleting entry with unsaved changes