Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, Petr Viktorin wrote:
On 04/10/2012 07:07 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
On 10.4.2012 16:00, Petr Viktorin wrote:
[snip]
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both --setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
Then we should validate *exactly* the same way, including not allowing
no_update attributes to be updated.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any objections?
Nope - as long as ticket 2754 is fixed.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0047 Do not use extra command options in ACI, permission, selfservice
On Thu, 2012-05-10 at 13:07 +0200, Petr Viktorin wrote: This is the second and likely the next-to-last part of disabling extra command options (after this it's just test fixes and turning the checking on). Part of the work for https://fedorahosted.org/freeipa/ticket/2509 This patch looks and works OK. I just think you missed a pkey_only attribute for aci_find command. pkey_only is being passed to aci_find command by selfservice_find and delegation_find commands and it would fail in the hardened tests because it is not defined in aci_find command as it is not based on LDAPSearch class but crud.Search. It just needs to be added to aci_find command and it should be fine. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1011 fix permission-find
On Fri, 2012-05-11 at 15:31 -0400, Rob Crittenden wrote: The permission-find command was failing for two reasons. The first was an overlap in the --name option and the primary key. The second was that aci's use a different attribute for name, aciname, so cn wasn't matching anything returning all entries. rob ACK. Pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On Mon, 2012-05-14 at 09:36 +0200, Martin Kosek wrote:
On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, Petr Viktorin wrote:
On 04/10/2012 07:07 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
On 10.4.2012 16:00, Petr Viktorin wrote:
[snip]
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both --setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
Then we should validate *exactly* the same way, including not allowing
no_update attributes to be updated.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any objections?
Nope - as long as ticket 2754 is fixed.
I just noticed we have the enable/disable command for DNS zone as well,
I somehow managed to overlook it... Sorry for noise, closing the ticket
2754.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1012 validate domain in installer
On Fri, 2012-05-11 at 16:03 -0400, Rob Crittenden wrote: Use our domain validator to validate the domain name we get in the installer. rob I found few issues with the patch: 1) The unexpected error is not very user friendly error message: # ipa-server-install ... Server host name [vm-109.idm.lab.bos.redhat.com]: The domain name has been calculated based on the host name. Please confirm the domain name [idm.lab.bos.redhat.com]: bar-.com Unexpected error - see ipaserver-install.log for details: only letters, numbers, and - are allowed. DNS label may not start or end with - I think we should make it consistent with hostname validation error message format: # ipa-server-install ... Server host name [vm-109.idm.lab.bos.redhat.com]: foo- Invalid hostname 'foo-', must be fully-qualified. 2) The error is also confusing when domain is passed as an option, user won't know what actually failed: # ipa-server-install --domain .. ... Server host name [vm-109.idm.lab.bos.redhat.com]: Unexpected error - see ipaserver-install.log for details: empty DNS label As for value passed via option, I think we should quit immediately and not in second or third interactive step - like we do for --zonemgr validation: # ipa-server-install --zonemgr=foo Usage: ipa-server-install [options] ipa-server-install: error: invalid zonemgr: missing address domain This applies both for --hostname and --domain options. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0047 Do not use extra command options in ACI, permission, selfservice
On Mon, 2012-05-14 at 10:00 +0200, Martin Kosek wrote: On Thu, 2012-05-10 at 13:07 +0200, Petr Viktorin wrote: This is the second and likely the next-to-last part of disabling extra command options (after this it's just test fixes and turning the checking on). Part of the work for https://fedorahosted.org/freeipa/ticket/2509 This patch looks and works OK. I just think you missed a pkey_only attribute for aci_find command. pkey_only is being passed to aci_find command by selfservice_find and delegation_find commands and it would fail in the hardened tests because it is not defined in aci_find command as it is not based on LDAPSearch class but crud.Search. It just needs to be added to aci_find command and it should be fine. Martin Petr³ noticed that pkey_only was already explicitly added to takes_options of aci_find command. The patch is OK then. ACK. Pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1013 implement permission/aci find by subtree
On Fri, 2012-05-11 at 16:34 -0400, Rob Crittenden wrote: permission-find --subtree wasn't implemented so always returned all entries (the option was ignored). rob I found the following 2 issues: 1) The following piece of code is over-complicated: +found = False +if kw['subtree'] == target: +found = True +if not found: +try: +results.remove(a) +except ValueError: +pass This would simpler and more readable: +if kw['subtree'] != target: +try: +results.remove(a) +except ValueError: +pass 2) I know we don't validate the subtree expression, but I wonder if we shouldn't make the subtree comparing at least case insensitive as DN is also not case sensitive. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] Build problems with FreeIPA v2.2.0
Hi, when i try to build FreeIPA v2.2.0 rpm's on Scientific Linux v6.2, i get the following errors (during make-lint phase): ipaserver/plugins/ldap2.py:176: [E1121, IPASimpleLDAPObject.rename_s] Too many positional arguments for function call ipa-client/ipa-install/ipa-client-install:742: [E1101, configure_sssd_conf] Instance of 'SSSDConfig' has no 'activate_service' member ipapython/nsslib.py:280: [E1121, NSSConnection.endheaders] Too many positional arguments for function call What might cause these errors ?. Br, -- -- Dan Airinen d...@crasman.fi Crasman Co Ltd - http://www.crasman.fi ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Build problems with FreeIPA v2.2.0
On Mon, May 14, 2012 at 01:11:49PM +0300, Dan Airinen wrote: ipa-client/ipa-install/ipa-client-install:742: [E1101, configure_sssd_conf] Instance of 'SSSDConfig' has no 'activate_service' member What version of the SSSD are you running? The activate_service method was added into the SSSD during the 1.8 beta phase. I suggest to also go with SSSD 1.8, it's going to be present in RHEL 6.3 anyway. That said, ipa-client should require SSSD-1.8 if it uses newly added features. The other errors seem to be unrelated. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Build problems with FreeIPA v2.2.0
Thanks Jakub!, manually updated sssd RPM's to 1.8.3 version. And that error was now fixed. Still problems with the other two errors tho. On ma, 2012-05-14 at 12:30 +0200, Jakub Hrozek wrote: On Mon, May 14, 2012 at 01:11:49PM +0300, Dan Airinen wrote: ipa-client/ipa-install/ipa-client-install:742: [E1101, configure_sssd_conf] Instance of 'SSSDConfig' has no 'activate_service' member What version of the SSSD are you running? The activate_service method was added into the SSSD during the 1.8 beta phase. I suggest to also go with SSSD 1.8, it's going to be present in RHEL 6.3 anyway. That said, ipa-client should require SSSD-1.8 if it uses newly added features. The other errors seem to be unrelated. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- -- Dan Airinen d...@crasman.fi Crasman Co Ltd - http://www.crasman.fi ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
The final part of rejecting unknown Command arguments: enable the
validation, add tests.
Also fix up things that were changed since the previous patches.
https://fedorahosted.org/freeipa/ticket/2509
--
Petr³
From 64496d35b5483b8b237282dd157388f10e72beda Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 17 Apr 2012 12:42:35 -0400
Subject: [PATCH] Fail on unknown Command options
When unknown keyword arguments are passed to a Command, raise an
error instead of ignoring them.
Options used when IPA calls its commands internally are listed
in a new Command attribute called internal_options, and allowed.
Previous patches (0b01751c, c45174d6, c5689e7f) made IPA not use
unknown keyword arguments in its own commands and tests, but since
that some violations were reintroduced in permission_find and tests.
Fix those.
Tests included; both a frontend unittest and a XML-RPC test via the
ping plugin (which was untested previously).
https://fedorahosted.org/freeipa/ticket/2509
---
ipalib/frontend.py| 13 ++--
ipalib/plugins/aci.py |2 ++
ipalib/plugins/automount.py |4 +++
ipalib/plugins/permission.py | 17 +-
tests/test_cmdline/test_cli.py|6 ++--
tests/test_ipalib/test_frontend.py|5 +++
tests/test_xmlrpc/test_host_plugin.py |2 +-
tests/test_xmlrpc/test_netgroup_plugin.py |6 ++--
tests/test_xmlrpc/test_ping_plugin.py | 52 +
9 files changed, 89 insertions(+), 18 deletions(-)
create mode 100644 tests/test_xmlrpc/test_ping_plugin.py
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index e8a84eabe0ccb981eabcc6b5d5e89f80c4135fe8..7c63b27ddb41e751692e47ebc334cb699606a74e 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -29,7 +29,8 @@
from output import Output, Entry, ListOfEntries
from text import _, ngettext
-from errors import ZeroArgumentError, MaxArgumentError, OverlapError, RequiresRoot, VersionError, RequirementError
+from errors import (ZeroArgumentError, MaxArgumentError, OverlapError,
+RequiresRoot, VersionError, RequirementError, OptionError)
from errors import InvocationError
from constants import TYPE_ERROR
from ipapython.version import API_VERSION
@@ -404,6 +405,8 @@ class Command(HasParam):
output_params = Plugin.finalize_attr('output_params')
has_output_params = tuple()
+internal_options = tuple()
+
msg_summary = None
msg_truncated = _('Results are truncated, try a more specific search')
@@ -520,7 +523,13 @@ def __args_2_params(self, values):
def __options_2_params(self, options):
for name in self.params:
if name in options:
-yield (name, options[name])
+yield (name, options.pop(name))
+# If any options remain, they are either internal or unknown
+unused_keys = set(options).difference(self.internal_options)
+unused_keys.discard('version')
+if unused_keys:
+raise OptionError(_('Unknown option: %(option)s'),
+option=unused_keys.pop())
def args_options_2_entry(self, *args, **options):
diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index b0be26f5c44e51d91af3bdf7d0a94c0a14f8fe4a..8476f1826561f8f00522b0b19f057d493bec2611 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -610,6 +610,8 @@ class aci_mod(crud.Update):
takes_options = (_prefix_option,)
+internal_options = ['rename']
+
msg_summary = _('Modified ACI %(value)s')
def execute(self, aciname, **kw):
diff --git a/ipalib/plugins/automount.py b/ipalib/plugins/automount.py
index ac9c9769f26b31631322e725721ecaf470b80f1c..a42c53b6fcada362a47657c8d6ea3f652240b4d4 100644
--- a/ipalib/plugins/automount.py
+++ b/ipalib/plugins/automount.py
@@ -787,6 +787,8 @@ class automountkey_add(LDAPCreate):
msg_summary = _('Added automount key %(value)s')
+internal_options = ['description', 'add_operation']
+
def pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
options.pop('add_operation', None)
options.pop('description', None)
@@ -910,6 +912,8 @@ class automountkey_mod(LDAPUpdate):
msg_summary = _('Modified automount key %(value)s')
+internal_options = ['newautomountkey']
+
takes_options = LDAPUpdate.takes_options + (
IA5Str('newautomountinformation?',
cli_name='newinfo',
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index ff38f852da9a6d133ae0b4c7a77c74fcd4a23913..c532e16f4e29066708ac2c0d6ee1d4de204cb631 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -359,10 +359,16 @@ class permission_find(LDAPSearch):
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.pop('pkey_only', False):
return
+
+# There is an option/param overlap: cn
[Freeipa-devel] [PATCH] 0051 Check for empty/single value parameters before calling callbacks
Pre-callbacks were called before a few validation steps, leading to
internal errors if the pre-callback relied on valid data.
https://fedorahosted.org/freeipa/ticket/2701
Regression test included.
--
Petr³
From f74699ea8e765f5aba197287a00bf2d0e0c83d03 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 10 May 2012 11:03:41 -0400
Subject: [PATCH] Check for empty/single value parameters before calling
callbacks
https://fedorahosted.org/freeipa/ticket/2701
---
ipalib/plugins/baseldap.py |5 +++--
tests/test_xmlrpc/test_config_plugin.py |8
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 895ec682ac2ee1d6b57e48711e22c75cb5f05105..2851f0f270d9e2bdba4780cc7bf308a76e180fd2 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -1255,18 +1255,19 @@ def execute(self, *keys, **options):
set(self.obj.default_attributes + entry_attrs.keys())
)
+_check_single_value_attrs(self.params, entry_attrs)
+_check_empty_attrs(self.obj.params, entry_attrs)
+
for callback in self.PRE_CALLBACKS:
if hasattr(callback, 'im_self'):
dn = callback(
ldap, dn, entry_attrs, attrs_list, *keys, **options
)
else:
dn = callback(
self, ldap, dn, entry_attrs, attrs_list, *keys, **options
)
-_check_single_value_attrs(self.params, entry_attrs)
-_check_empty_attrs(self.obj.params, entry_attrs)
ldap.get_schema()
_check_limit_object_class(self.api.Backend.ldap2.schema.attribute_types(self.obj.limit_object_classes), entry_attrs.keys(), allow_only=True)
_check_limit_object_class(self.api.Backend.ldap2.schema.attribute_types(self.obj.disallow_object_classes), entry_attrs.keys(), allow_only=False)
diff --git a/tests/test_xmlrpc/test_config_plugin.py b/tests/test_xmlrpc/test_config_plugin.py
index fbe389106601c1cef7f4689d7d3f85d80694d34a..da549bfb3efb56b05546ba32e7ce57414a586160 100644
--- a/tests/test_xmlrpc/test_config_plugin.py
+++ b/tests/test_xmlrpc/test_config_plugin.py
@@ -21,6 +21,7 @@
Test the `ipalib/plugins/config.py` module.
+from ipalib import errors
from xmlrpc_test import Declarative, fuzzy_digits, fuzzy_uuid
class test_config(Declarative):
@@ -52,4 +53,11 @@ class test_config(Declarative):
),
),
+dict(
+desc='Try to remove ipausersearchfields',
+command=('config_mod', [],
+dict(delattr=u'ipausersearchfields=uid,givenname,sn,telephonenumber,ou,title')),
+expected=errors.RequirementError(name='ipausersearchfields'),
+),
+
]
--
1.7.10.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
Martin Kosek wrote:
On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, Petr Viktorin wrote:
On 04/10/2012 07:07 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
On 10.4.2012 16:00, Petr Viktorin wrote:
[snip]
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both --setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
Then we should validate *exactly* the same way, including not allowing
no_update attributes to be updated.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any objections?
Nope - as long as ticket 2754 is fixed.
Martin
I think those are there so they don't show up for the -mod command since
we have a separate interface for doing it.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Build problems with FreeIPA v2.2.0
Dan Airinen wrote: Thanks Jakub!, manually updated sssd RPM's to 1.8.3 version. And that error was now fixed. Still problems with the other two errors tho. What version of python-nss and python-ldap do you have installed? rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0020] Separate LDAP result from LDAP connection, fix deadlock.
On 05/11/2012 12:26 PM, Adam Tkac wrote:
On Mon, May 07, 2012 at 02:49:07PM +0200, Petr Spacek wrote:
Hello,
this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/66:
Plugin deadlocks during new zone load when connections == 1.
It fixes structural problem, when LDAP query result was tied with
LDAP connection up. It wasn't possible to release connection and
work with query result after that.
Described deadlock is consequence of this problematic design.
Now LDAP connection is separated from LDAP result. Next planed patch
will avoid manual connection management, so possibility of
deadlock should be next to zero.
Petr^2 Spacek
Hello Peter,
good work, please check my comments below.
Regards, Adam
From 8ee1fd607531ef71369e99c9228456baea45b65d Mon Sep 17 00:00:00 2001
From: Petr Spacekpspa...@redhat.com
Date: Mon, 7 May 2012 12:51:09 +0200
Subject: [PATCH] Separate LDAP result from LDAP connection, fix deadlock.
https://fedorahosted.org/bind-dyndb-ldap/ticket/66
Signed-off-by: Petr Spacekpspa...@redhat.com
Hello Adam,
thanks for ideas/improvements!
Reworked patch is attached. I did all proposed changes except this one:
@ ldap_psearch_watcher:
restart:
(... snip ...)
soft_err:
-
- ldap_msgfree(conn-result);
- ldap_entrylist_destroy(conn-mctx,
- conn-ldap_entries);
+ ;
Empty label soft_err: is useless, please remove it and use continue; on
appropriate places;
I think continue in this place can lead to memory leak, so I removed
soft_err by other way.
Petr^2 Spacek
From 4c8c8c1ec217d3219a0380f6baec4b41248d Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Mon, 7 May 2012 12:51:09 +0200
Subject: [PATCH] Separate LDAP result from LDAP connection, fix deadlock.
https://fedorahosted.org/bind-dyndb-ldap/ticket/66
Signed-off-by: Petr Spacek pspa...@redhat.com
---
src/ldap_helper.c | 269 -
1 files changed, 162 insertions(+), 107 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 304c37296f8f3a428c4c72b45fe4154b2c9e954c..dae3e28804b602ca91d813c4d68d258e7065608f 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -108,6 +108,7 @@
* must acquire the semaphore and the lock.
*/
+typedef struct ldap_qresult ldap_qresult_t;
typedef struct ldap_connection ldap_connection_t;
typedef struct ldap_pool ldap_pool_t;
typedef struct ldap_auth_pair ldap_auth_pair_t;
@@ -188,28 +189,28 @@ struct ldap_connection {
ld_string_t *query_string;
LDAP *handle;
- LDAPMessage *result;
LDAPControl *serverctrls[2]; /* psearch/NULL or NULL/NULL */
int msgid;
/* Parsing. */
isc_lex_t *lex;
isc_buffer_t rdata_target;
unsigned char *rdata_target_mem;
- /* Cache. */
- ldap_entrylist_t ldap_entries;
-
/* For reconnection logic. */
isc_time_t next_reconnect;
unsigned int tries;
+};
- /* Temporary stuff. */
- LDAPMessage *entry;
- BerElement *ber;
- char *attribute;
- char **values;
- char *dn;
+/**
+ * Result from single LDAP query.
+ */
+struct ldap_qresult {
+ isc_mem_t *mctx;
+ LDAPMessage *result;
+
+ /* Cache. */
+ ldap_entrylist_t ldap_entries;
};
/*
@@ -271,9 +272,10 @@ static int handle_connection_error(ldap_instance_t *ldap_inst,
ldap_connection_t *ldap_conn, isc_boolean_t force,
isc_result_t *result);
static isc_result_t ldap_query(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
- const char *base,
- int scope, char **attrs, int attrsonly, const char *filter, ...);
-static void ldap_query_free(ldap_connection_t *ldap_conn);
+ ldap_qresult_t **ldap_qresultp, const char *base, int scope, char **attrs,
+ int attrsonly, const char *filter, ...);
+static isc_result_t ldap_query_create(isc_mem_t *mctx, ldap_qresult_t **ldap_qresultp);
+static void ldap_query_free(isc_boolean_t prepare_reuse, ldap_qresult_t **ldap_qresultp);
/* Functions for writing to LDAP. */
static isc_result_t ldap_modify_do(ldap_connection_t *ldap_conn, const char *dn,
@@ -1095,6 +1097,8 @@ refresh_zones_from_ldap(ldap_instance_t *ldap_inst)
{
isc_result_t result = ISC_R_SUCCESS;
ldap_connection_t *ldap_conn = NULL;
+ ldap_qresult_t *ldap_config_qresult = NULL;
+ ldap_qresult_t *ldap_zones_qresult = NULL;
int zone_count = 0;
ldap_entry_t *entry;
dns_rbt_t *rbt = NULL;
@@ -1119,31 +1123,31 @@ refresh_zones_from_ldap(ldap_instance_t *ldap_inst)
log_debug(2, refreshing list of zones for %s, ldap_inst-db_name);
+ /* Query for configuration and zones from LDAP and release LDAP connection
+ * before processing them. It prevents deadlock in situations where
+ * ldap_parse_zoneentry() requests another connection. */
CHECK(ldap_pool_getconnection(ldap_inst-pool, ldap_conn));
-
- CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(ldap_inst-base),
+ CHECK(ldap_query(ldap_inst, ldap_conn, ldap_config_qresult,
Re: [Freeipa-devel] Build problems with FreeIPA v2.2.0
On 05/14/2012 06:11 AM, Dan Airinen wrote: Hi, when i try to build FreeIPA v2.2.0 rpm's on Scientific Linux v6.2, i get the following errors (during make-lint phase): ipaserver/plugins/ldap2.py:176: [E1121, IPASimpleLDAPObject.rename_s] Too many positional arguments for function call ipa-client/ipa-install/ipa-client-install:742: [E1101, configure_sssd_conf] Instance of 'SSSDConfig' has no 'activate_service' member ipapython/nsslib.py:280: [E1121, NSSConnection.endheaders] Too many positional arguments for function call What might cause these errors ?. incompatible versions, what version of python do you have, what version of python-ldap? -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 124-132 Inconsistent ways to show/change entry status
On 5/10/2012 7:19 AM, Petr Vobornik wrote: Updated patch attached. See comments below. On 05/08/2012 04:23 AM, Endi Sukma Dewata wrote: Try adding a very long DNS zone, then open the zone. Compare the breadcrumbs in the DNS Resource Records page and in the Settings page, in my case the second one is a bit longer. I think the length of the breadcrumb should be calculated differently. I remade how breadcrumb is limited. Now it tries to use as much space available with keeping low complexity level of calculation. It still uses an average but now the average is calculated in two phases in order to neglect a presence of short keys. ACK. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 136 Correction of nested search facets tab labels
On 5/10/2012 5:29 AM, Petr Vobornik wrote: Nested search facets were using 'search' tab label instead of their nested entity name. This patch is fixing that regression. https://fedorahosted.org/freeipa/ticket/2744 ACK. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 133 Action list for user password
On 5/3/2012 3:59 AM, Petr Vobornik wrote: Currently the user password is shown as follows in the details page: Password: Reset Password This is inconsistent with the rest of the page because the 'Reset Password' is an action, not the value of the password. Now password is shown as follows: Password: *** (if set) Password: (if not set) Reset password action was moved to Action List. https://fedorahosted.org/freeipa/ticket/2248 Note: I will address enabling/disabling 'reset password' option in action list in ticket #2318. Just for the record: On 5/11/2012 11:36 AM, Petr Vobornik wrote: On 05/08/2012 01:47 AM, Endi Sukma Dewata wrote: 10. The 'Action List' in ticket #2248 for the password reset is actually different than the action list for Enable/Disable. I was proposing to create a small panel under the 'Account Settings' section, probably something like this: --- Account Settings +-+ User login: admin | Actions: | Password: | Reset password | Password expiration: +-+ UID: GID: This panel might better be called 'Action Panel' to distinguish from the dropdown 'Action List' on the top. The reason for this panel is that a Password Reset only affects an aspect of the user, not the entire object like Enable/Disable (although that can also be argued differently), so the action should be placed where it's relevant, in this case near the Password field. The same concept will be used for ticket #2250, #2251, #2252. I'll consider my patch #133 NACKed and first create the action panels. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1012 validate domain in installer
Martin Kosek wrote:
On Fri, 2012-05-11 at 16:03 -0400, Rob Crittenden wrote:
Use our domain validator to validate the domain name we get in the
installer.
rob
I found few issues with the patch:
1) The unexpected error is not very user friendly error message:
# ipa-server-install
...
Server host name [vm-109.idm.lab.bos.redhat.com]:
The domain name has been calculated based on the host name.
Please confirm the domain name [idm.lab.bos.redhat.com]: bar-.com
Unexpected error - see ipaserver-install.log for details:
only letters, numbers, and - are allowed. DNS label may not start or
end with -
I think we should make it consistent with hostname validation error
message format:
# ipa-server-install
...
Server host name [vm-109.idm.lab.bos.redhat.com]: foo-
Invalid hostname 'foo-', must be fully-qualified.
2) The error is also confusing when domain is passed as an option, user
won't know what actually failed:
# ipa-server-install --domain ..
...
Server host name [vm-109.idm.lab.bos.redhat.com]:
Unexpected error - see ipaserver-install.log for details:
empty DNS label
As for value passed via option, I think we should quit immediately and
not in second or third interactive step - like we do for --zonemgr
validation:
# ipa-server-install --zonemgr=foo
Usage: ipa-server-install [options]
ipa-server-install: error: invalid zonemgr: missing address domain
This applies both for --hostname and --domain options.
Martin
Done.
There is a separate ticket to validate hostname in the parser. It is a
bit more complicated since it depends on other options so I punted on that.
rob
From 6f306111a727b481838b77fbc4bf6a80b7b0cc89 Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Fri, 11 May 2012 15:56:43 -0400
Subject: [PATCH] Call the domain validator on the user-provided domain name.
Wrap printing exceptions in unicode() to do Gettext conversion.
https://fedorahosted.org/freeipa/ticket/2196
---
install/tools/ipa-server-install | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index 1dd02ba870a02e902c4c345d9f5802ef09f78a7a..a6a98159bb7e44db8a1c37e2d55920d294d953c4 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -62,6 +62,7 @@ from ipapython.config import IPAOptionParser
from ipalib.dn import DN
from ipalib.x509 import load_certificate_from_file, load_certificate_chain_from_file
from ipalib.constants import DNS_ZONE_REFRESH
+from ipalib.util import validate_domain_name
from ipapython import services as ipaservices
from ipapython.ipa_log_manager import *
@@ -96,6 +97,17 @@ def subject_callback(option, opt_str, value, parser):
raise OptionValueError('Invalid subject base format: %s' % str(e))
parser.values.subject = str(dn) # may as well normalize it
+def domain_callback(option, opt_str, value, parser):
+
+Validate the domain name
+
+try:
+validate_domain_name(value)
+except ValueError, e:
+parser.error(invalid domain: + unicode(e))
+
+parser.values.zonemgr = value
+
def validate_dm_password(password):
if len(password) 8:
raise ValueError(Password must be at least 8 characters long)
@@ -117,6 +129,9 @@ def parse_options():
basic_group.add_option(-r, --realm, dest=realm_name,
help=realm name)
basic_group.add_option(-n, --domain, dest=domain_name,
+ action=callback,
+ callback=domain_callback,
+ type=string,
help=domain name)
basic_group.add_option(-p, --ds-password, dest=dm_password,
sensitive=True, help=admin password)
@@ -727,6 +742,10 @@ def main():
domain_name = options.domain_name
domain_name = domain_name.lower()
+try:
+validate_domain_name(domain_name)
+except ValueError, e:
+sys.exit(Invalid domain name: %s % unicode(e))
ip = get_server_ip_address(host_name, fstore, options.unattended, options)
ip_address = str(ip)
@@ -1108,9 +1127,9 @@ try:
except Exception, e:
success = False
if uninstalling:
-message = Unexpected error - see ipaserver-uninstall.log for details:\n %s % str(e)
+message = Unexpected error - see ipaserver-uninstall.log for details:\n %s % unicode(e)
else:
-message = Unexpected error - see ipaserver-install.log for details:\n %s % str(e)
+message = Unexpected error - see ipaserver-install.log for details:\n %s % unicode(e)
print message
message = str(e)
for str in traceback.format_tb(sys.exc_info()[2]):
--
1.7.10.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1013 implement permission/aci find by subtree
Martin Kosek wrote:
On Fri, 2012-05-11 at 16:34 -0400, Rob Crittenden wrote:
permission-find --subtree wasn't implemented so always returned all
entries (the option was ignored).
rob
I found the following 2 issues:
1) The following piece of code is over-complicated:
+found = False
+if kw['subtree'] == target:
+found = True
+if not found:
+try:
+results.remove(a)
+except ValueError:
+pass
This would simpler and more readable:
+if kw['subtree'] != target:
+try:
+results.remove(a)
+except ValueError:
+pass
2) I know we don't validate the subtree expression, but I wonder if we
shouldn't make the subtree comparing at least case insensitive as DN is
also not case sensitive.
Martin
Yeah, much simpler. Fixed both.
rob
From df3d0d26a4cbc534bb49338ca7eb8261b9fe2787 Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Fri, 11 May 2012 16:15:58 -0400
Subject: [PATCH] Implement permission/aci find by subtree
https://fedorahosted.org/freeipa/ticket/2321
---
ipalib/plugins/aci.py | 13 -
tests/test_xmlrpc/test_permission_plugin.py | 41 +++
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index f0b81f48af1f9fbf8ab267a3d4b113c328ab1170..51f6568ceb47408a1876e1f4dd03387fc2e9c9b3 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -840,7 +840,18 @@ class aci_find(crud.Search):
a.target['targetfilter']['expression'] != kw['filter']:
results.remove(a)
-# TODO: searching by: subtree
+if kw.get('subtree'):
+for a in acis:
+if 'target' in a.target:
+target = a.target['target']['expression']
+else:
+results.remove(a)
+continue
+if kw['subtree'].lower() != target.lower():
+try:
+results.remove(a)
+except ValueError:
+pass
acis = []
for result in results:
diff --git a/tests/test_xmlrpc/test_permission_plugin.py b/tests/test_xmlrpc/test_permission_plugin.py
index 14cfcbc784624011d067eaa8a1400b5c4f6fe963..f54e20462c3c26a0a1cd98a0a75a310bacfc62de 100644
--- a/tests/test_xmlrpc/test_permission_plugin.py
+++ b/tests/test_xmlrpc/test_permission_plugin.py
@@ -478,6 +478,47 @@ class test_permission(Declarative):
dict(
+desc='Change %r to a subtree type' % permission1_renamed_ucase,
+command=(
+'permission_mod', [permission1_renamed_ucase], dict(subtree=u'ldap:///cn=*,cn=test,cn=accounts,%s' % api.env.basedn, type=None)
+),
+expected=dict(
+value=permission1_renamed_ucase,
+summary=u'Modified permission %s' % permission1_renamed_ucase,
+result=dict(
+dn=lambda x: DN(x) == permission1_renamed_ucase_dn,
+cn=[permission1_renamed_ucase.lower()],
+member_privilege=[privilege1],
+subtree=u'ldap:///cn=*,cn=test,cn=accounts,%s' % api.env.basedn,
+permissions=[u'write'],
+memberof=u'ipausers',
+),
+),
+),
+
+
+dict(
+desc='Search for %r using --subtree' % permission1,
+command=('permission_find', [], {'subtree': 'ldap:///cn=*,cn=test,cn=accounts,%s' % api.env.basedn}),
+expected=dict(
+count=1,
+truncated=False,
+summary=u'1 permission matched',
+result=[
+{
+'dn':lambda x: DN(x) == permission1_renamed_ucase_dn,
+'cn':[permission1_renamed_ucase.lower()],
+'member_privilege':[privilege1],
+'subtree':u'ldap:///cn=*,cn=test,cn=accounts,%s' % api.env.basedn,
+'permissions':[u'write'],
+'memberof':u'ipausers',
+},
+],
+),
+),
+
+
+dict(
desc='Delete %r' % permission1_renamed_ucase,
command=('permission_del', [permission1_renamed_ucase], {}),
expected=dict(
--
1.7.10.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] Adding indices and permissions to FreeIPA
Hi, I am currently working on adding DHCP support, so that FreeIPA can control an ISC-DHCP server. As part of this, I need to add a number of indices to 389ds, as well as a number of permissions (ACIs) and groups to manage these. Is there a specific way to add these? Should they be added as part of the DHCP feature installation process, or should they be part of the base server install? Sincerely, William Brown pgp.mit.edu http://pgp.mit.edu:11371/pks/lookup?op=vindexsearch=0x3C0AC6DAB2F928A2 signature.asc Description: Message signed with OpenPGP using GPGMail ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] New beaker server in mountain view
At long last, a beaker server is now up and running here in Mountain View. The beaker server is able to control nearly all of the machines here at the mountain view location. Login to it here: http://hammer1.dsdev.sjc.redhat.com/bkr/ You should be able to login with your kerberos username and password. If you would like to use beaker through the cli, follow these steps: 1. make sure you have beaker client installed and configured for use with the wesford lab controller. 2. Download and install this RPM: https://engineering.redhat.com/trac/ipa-tests/browser/trunk/ipa-tests/beaker/ipa-server/shared/bkr-hammer-1.0-1.noarch.rpm 3. you should be able to use bkr-hammer just as you would use the bkr command. I may need to add you to some beaker groups to allow you to use all of the machines here. There is only a small number of tests in this beaker server. I tried to add all of the ipa tasks. Feel free to add any missing tasks that you need. Or, as me to add them, and I'm move them into this lab controller. There is also a reduced set of rhel builds as there isn't enough bandwidth here to sync the nightlies here. I'll keep this server syned up with the latest snapshots. Look at the currently available distros here: http://hammer1.dsdev.sjc.redhat.com/bkr/distros/ Give it a try, feedback is welcome. Michael- ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
