Re: [Freeipa-devel] [PATCHES 0227-0229] Server upgrade: introduce ipa-server-upgrade command

[Jan Cholasta]

Hi,

Dne 15.4.2015 v 16:26 Martin Basti napsal(a):

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

Patches attached.

Also ipa-upgradeconfig part is called as a subprocess. This will be
removed after installer modifications.

This patch may cause temporal upgrade issues (corner cases), until
installer part will be finished.

If somebody will be hit by them, please use --skip-version-check for
ipactl and ipa-server-upgrade.


Regarding that option vs. --force: I think the common assumption is that 
--force ignores *all* non-fatal errors, but you break that assumption in 
ipactl. IMO --force should both ignore errors in service startup *and* 
skip version check, and a new option should be added to just ignore 
errors in service startup (e.g. --ignore-service-failures).


ipa-server-upgrade should probably also have --force, even if it does 
the same thing as --skip-version-check, again because --force is common.



This is a weird API:

+if data_upgrade.badsyntax:
+raise admintool.ScriptError(
+'Bad syntax detected in upgrade file(s).', 1)
+elif data_upgrade.upgradefailed:
+raise admintool.ScriptError('IPA upgrade failed.', 1)
+elif data_upgrade.modified:
+self.log.info('Data update complete')
+else:
+self.log.info('Data update complete, no data were modified')

Why does not IPAUpgrade raise errors instead?


+class IPAVersionError(Exception):
+pass
+
+class PlatformMismatchError(IPAVersionError):
+pass
+
+class DataUpgradeRequiredError(IPAVersionError):
+pass
+
+class DataInNewerVersionError(IPAVersionError):
+pass

I don't like the "IPA" in "IPAVersionError", it does not tell you much 
about what kind of version is that. Also data version errors should only 
tell you what is wrong, not how you fix it. IMO better names for these 
would be e.g. "UpgradeVersionError", "UpgradePlatformError", 
"UpgradeDataOlderVersionError", "UpgradeDataNewerVersionError". Similar 
for store_ipa_version and check_ipa_version.



Why is it not an error if there is no version in check_ipa_version? IMO 
it should, even if you then ignore the exception most of the time.



Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 0227-0229] Server upgrade: introduce ipa-server-upgrade command

[David Kupka]

On 04/16/2015 05:14 PM, Martin Basti wrote:

On 15/04/15 16:26, Martin Basti wrote:

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

Patches attached.

Also ipa-upgradeconfig part is called as a subprocess. This will be
removed after installer modifications.

This patch may cause temporal upgrade issues (corner cases), until
installer part will be finished.

If somebody will be hit by them, please use --skip-version-check for
ipactl and ipa-server-upgrade.




Updated patches attached.

--
Martin Basti


Hi,
thanks for the patches. Could you please split them correctly?
I mean patch 227 and 228. In patch 227 you add whole file 
ipa_server_upgrade.py and in patch 228 add forgotten import and change 
option description slightly.


Otherwise it works for me.

--
David Kupka

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] manage replication topology in the shared tree

[thierry bordaz]

On 04/13/2015 10:56 AM, Ludwig Krispenz wrote:

Hi,

in the attachment you find the latest state of the "topology plugin", 
it implements what is defined in the design page: 
http://www.freeipa.org/page/V4/Manage_replication_topology (which is 
also waiting for a reviewer)


It contains the plugin itself and  a core of ipa commands to manage a 
topology. to be really applicable, some work outside is required, eg 
the management of the domain level and a decision where the binddn 
group should be maintained.


Thanks,
Ludwig



Hello Ludwig,

Quite long review to do. So far I only looked at the startup phase and I 
have only few questions and comments.


In ipa_topo_start, do you need to get argc/argv as you are not using 
plugin-argxx attributes ?



topo_plugin_conf configuration parameters are not freed when the plugin 
is closed. Is it closed only at shutdown ?

Also I would initiatlize it to {NULL}.

In case the config does not contain any 
nsslapd-topo-plugin-shared-replica-root, I wonder if 
ipa_topo_apply_shared_config may crash as shared_replica_root will be NULL.
or at least in 
ipa_topo_apply_shared_replica_config/ipa_topo_util_get_replica_conf.


Also if nsslapd-topo-plugin-shared-replica-root contains an invalid root 
suffix (typo), topoRepl remains NULL and 
ipa_topo_util_get_replica_conf/ipa_topo_cfg_replica_add can crash.


In ipa_topo_util_segment_from_entry, if the config entry has no 
direction/left/right it will crash. Shouldn't it return an error if the 
config is invalid.


The update of domainLevel may start the plugin. If two mods update the 
domainLevel they could be done in parallele.



In ipa_topo_util_update_agmt_list, if there is a marked agmnt but no 
segment it deletes the agreement.
Is it possible there is a segment but no agmnt ? For example, if the 
server were stopped or crashed after the segment was created but before 
the local config was updated.



Hosts are taken from shared config tree (cn=masters,), is it 
possible to have a replica agreement to a host that is not under 
'cn=masters,'



thanks
thierry

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0030] use separate ccache filename for each IPA DNSSEC daemon

[Petr Spacek]

On 20.4.2015 17:02, Martin Babinsky wrote:
> The attached patch implements a request by Petr^2 Spacek during the review of
> my PATCHES 0015-0017, which are prerequisites of the patch and were pushed 
> today.
> 
> Petr wanted each DNSSEC daemon (ipa-dnskeysync-replica, ipa-dnskeysyncd, and
> ipa-ods-exporter) to have its own CCache file to simplify his life during
> debugging DNSSEC-related issues.

Obvious ACK. Thank you!

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES] 0688-0689 Remove Editable DN and DN component classes

[Jan Cholasta]

Dne 20.4.2015 v 17:13 Petr Viktorin napsal(a):

On 04/20/2015 10:24 AM, Jan Cholasta wrote:

Dne 16.4.2015 v 14:35 Petr Viktorin napsal(a):

On 04/16/2015 09:04 AM, Jan Cholasta wrote:

Hi,

Dne 10.4.2015 v 15:58 Petr Viktorin napsal(a):

The attached patches remove EditableDN, EditableRDN and EditableAVA.
They depend on Petr Voborník's patch 811 (performance: faster DN
implementation).


Mutable DNs are not very useful. When creating them it is easier to
work
with lists or generators, and needing to change DNs aside from
operations like `DN(new_rdn, original[1:])` is very rare -- I'd even
say
theoretical.
Mutable DNs are not hashable, so they can't be used as dist keys.
Storing them as "keys" in other structures (e.g. in a LDAPEntry) is
dangerous -- it's hard to reason about outside modifications.

The first patch removes the last use of EditableDN. I could be
convinced
it's not an improvement in elegance/readability, but I believe this is
the strongest case for EditableDN in IPA, and it doesn't justify
keeping
it.


LGTM, but patch 688 needs to be rebased.


Here you go.


Regarding patch 688, it seems we are always replacing the suffix of the
DN, so I think we can simplify _dn_replace to:

 if not dn.endswith(old):
 raise ValueError('no replacement made')
 return DN(*dn[:-len(old)]) + new



Sure, here's a patches with this change.



Thanks, but it looks like you forgot to raise the ValueError.

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH 0030] use separate ccache filename for each IPA DNSSEC daemon

[Martin Babinsky]

The attached patch implements a request by Petr^2 Spacek during the 
review of my PATCHES 0015-0017, which are prerequisites of the patch and 
were pushed today.


Petr wanted each DNSSEC daemon (ipa-dnskeysync-replica, ipa-dnskeysyncd, 
and ipa-ods-exporter) to have its own CCache file to simplify his life 
during debugging DNSSEC-related issues.


--
Martin^3 Babinsky
From b4ceafcbb9cefe19121caf0e63cc09a30e2ef811 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Wed, 15 Apr 2015 15:20:00 +0200
Subject: [PATCH] use separate ccache filename for each IPA DNSSEC daemon

ipa-dnskeysyncd, ipa-dnskeysync-replica, and ipa-ods-exporter use a generic
'ccache' filename for credential storage, making debugging Kerberos-related
errors unnecessarily complicated. This patch renames the ccache files so that
each of these daemons now has its own credenital cache.
---
 daemons/dnssec/ipa-dnskeysync-replica | 2 +-
 daemons/dnssec/ipa-dnskeysyncd| 2 +-
 daemons/dnssec/ipa-ods-exporter   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/daemons/dnssec/ipa-dnskeysync-replica b/daemons/dnssec/ipa-dnskeysync-replica
index bcf9282153c9d105179d21237442598c1db530b5..c2c4c2725a9c46db4db04894a326ddf40e254eab 100755
--- a/daemons/dnssec/ipa-dnskeysync-replica
+++ b/daemons/dnssec/ipa-dnskeysync-replica
@@ -139,7 +139,7 @@ log.setLevel(level=logging.DEBUG)
 # Kerberos initialization
 PRINCIPAL = str('%s/%s' % (DAEMONNAME, ipalib.api.env.host))
 log.debug('Kerberos principal: %s', PRINCIPAL)
-ccache_filename = os.path.join(WORKDIR, 'ccache')
+ccache_filename = os.path.join(WORKDIR, 'ipa-dnskeysync-replica.ccache')
 ipautil.kinit_keytab(PRINCIPAL, paths.IPA_DNSKEYSYNCD_KEYTAB, ccache_filename)
 os.environ['KRB5CCNAME'] = ccache_filename
 log.debug('Got TGT')
diff --git a/daemons/dnssec/ipa-dnskeysyncd b/daemons/dnssec/ipa-dnskeysyncd
index b17c8d94e8a3c35a2aa29a1ce697ffdef654eeda..398f0076290c0d4bea48d15714505d46cd5ef5d4 100755
--- a/daemons/dnssec/ipa-dnskeysyncd
+++ b/daemons/dnssec/ipa-dnskeysyncd
@@ -65,7 +65,7 @@ log = root_logger
 # Kerberos initialization
 PRINCIPAL = str('%s/%s' % (DAEMONNAME, api.env.host))
 log.debug('Kerberos principal: %s', PRINCIPAL)
-ccache_filename = os.path.join(WORKDIR, 'ccache')
+ccache_filename = os.path.join(WORKDIR, 'ipa-dnskeysyncd.ccache')
 ipautil.kinit_keytab(PRINCIPAL, KEYTAB_FB, ccache_filename)
 os.environ['KRB5CCNAME'] = ccache_filename
 
diff --git a/daemons/dnssec/ipa-ods-exporter b/daemons/dnssec/ipa-ods-exporter
index 6d33b79bbddb59a8194107a7f2455e56637bad17..913b418af2806e2660a7db221e06394b501bbb18 100755
--- a/daemons/dnssec/ipa-ods-exporter
+++ b/daemons/dnssec/ipa-ods-exporter
@@ -399,7 +399,7 @@ ipalib.api.finalize()
 # Kerberos initialization
 PRINCIPAL = str('%s/%s' % (DAEMONNAME, ipalib.api.env.host))
 log.debug('Kerberos principal: %s', PRINCIPAL)
-ccache_name = os.path.join(WORKDIR, 'ccache')
+ccache_name = os.path.join(WORKDIR, 'ipa-ods-exporter.ccache')
 ipautil.kinit_keytab(PRINCIPAL, paths.IPA_ODS_EXPORTER_KEYTAB, ccache_name)
 os.environ['KRB5CCNAME'] = ccache_name
 log.debug('Got TGT')
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 424] install: Introduce installer framework ipapython.install

[Jan Cholasta]

Dne 20.4.2015 v 15:14 Martin Basti napsal(a):

On 17/04/15 16:15, Jan Cholasta wrote:

Dne 16.4.2015 v 16:46 Jan Cholasta napsal(a):

Hi,

the attached patch adds the basics of the new installer framework.

As a next step, I plan to convert the install scripts to use the
framework with their old code (the old code will be gradually ported to
the framework later).

(Note I didn't manage to write docstrings today, expect update
tomorrow.)


Added some docstrings.

Also updated the patch to reflect little brainstorming David and I had
this morning.



Honza





Hello, see comments bellow:

1) We started using new shorter License header in files:
#
# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
#


OK.



2) IMO this will not work, NoneType has no 'obj' attribute
+else:
+if isinstance(value, from_):
+value = None
+stack.append(value.obj)
+continue


Right.



3) Multiple inheritance. I do not like it much.
+class CompositeInstaller(Installer, CompositeConfigurator):


I guess you are antagonistic to multiple inheritance because of how 
other languages (like C++) do it. In Python it can be pretty elegant and 
is basis for e.g. the mixin design pattern.




Installer and CompositeConfigurator inherites from Configurator class,
and all of them implements _generator method.


Both of them call super()._generator(), so it's no problem (same for 
other methods).




If I understand correctly
(https://www.python.org/download/releases/2.3/mro/) the
Installer._generator method will be used in this case.
However in case when CompositeConfigurator has more levels (respectively
it is more specialized) of inheritance, it could take precedence and its
_generator method may be used instead.


The order of precedence is defined by the order of base classes in the 
class definition.




I'm afraid this may suddenly stop working.
Maybe I'm wrong, please fix me.


As long as you call the super class, it will work fine.



And Multiple inheritance is not easily readable, this is even a diamond
inheritance model.


Cooperative inheritance is used by design and IMHO is easily readable if 
you know how to read it. Every class defines a single bit of behavior. 
Without cooperative inheritance, it would have to be hardcoded and/or 
hacked around, which I wanted to avoid.


This blog post explains it nicely: 
.




--
Martin Basti



--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 424] install: Introduce installer framework ipapython.install

[Martin Basti]

On 17/04/15 16:15, Jan Cholasta wrote:

Dne 16.4.2015 v 16:46 Jan Cholasta napsal(a):

Hi,

the attached patch adds the basics of the new installer framework.

As a next step, I plan to convert the install scripts to use the
framework with their old code (the old code will be gradually ported to
the framework later).

(Note I didn't manage to write docstrings today, expect update 
tomorrow.)


Added some docstrings.

Also updated the patch to reflect little brainstorming David and I had 
this morning.




Honza





Hello, see comments bellow:

1) We started using new shorter License header in files:
#
# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
#

2) IMO this will not work, NoneType has no 'obj' attribute
+else:
+if isinstance(value, from_):
+value = None
+stack.append(value.obj)
+continue

3) Multiple inheritance. I do not like it much.
+class CompositeInstaller(Installer, CompositeConfigurator):

Installer and CompositeConfigurator inherites from Configurator class, 
and all of them implements _generator method.


If I understand correctly 
(https://www.python.org/download/releases/2.3/mro/) the 
Installer._generator method will be used in this case.
However in case when CompositeConfigurator has more levels (respectively 
it is more specialized) of inheritance, it could take precedence and its 
_generator method may be used instead.


I'm afraid this may suddenly stop working.
Maybe I'm wrong, please fix me.

And Multiple inheritance is not easily readable, this is even a diamond 
inheritance model.


--
Martin Basti

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 001] Remove recommendation from ipa-adtrust-install

[Gabe Alford]

Ack from me.

Thanks,

Gabe

On Fri, Apr 10, 2015 at 7:35 AM, Thorsten Scherf  wrote:

>
>
> --
> Manage your subscription for the Freeipa-devel mailing list:
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
>
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0014] emit a more helpful error messages when CA configuration fails

[Martin Babinsky]

On 04/17/2015 03:56 PM, Martin Babinsky wrote:

On 03/05/2015 01:11 PM, Martin Babinsky wrote:

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



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



Nobody to review this?



Attaching updated patches, one for ipa-4-1 (no DogtagInstance) and one 
for master.


--
Martin^3 Babinsky
From 83a5b8aa57d40f5d293f91b9088c13a1efdbbd49 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 17 Apr 2015 17:27:55 +0200
Subject: [PATCH] point the users to PKI-related logs when CA configuration
 fails

This patch adds an error handler which prints out the paths to logs related to
configuration and installation of Dogtag/CA in the case of failure.

https://fedorahosted.org/freeipa/ticket/4900
---
 ipapython/dogtag.py |  6 ++
 ipaserver/install/cainstance.py | 19 +++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/ipapython/dogtag.py b/ipapython/dogtag.py
index 675d2a77fe30b9109c17089f129b189282ffa57b..78409dfda2e1620aab1d239b14d6925ae5b0aee6 100644
--- a/ipapython/dogtag.py
+++ b/ipapython/dogtag.py
@@ -55,7 +55,10 @@ class Dogtag10Constants(object):
 DESTROY_BINARY = paths.PKIDESTROY
 
 SERVER_ROOT = paths.VAR_LIB_PKI_DIR
+PKI_INSTALL_LOG = paths.PKI_CA_INSTALL_LOG
+PKI_UNINSTALL_LOG = paths.PKI_CA_UNINSTALL_LOG
 PKI_INSTANCE_NAME = 'pki-tomcat'
+PKI_LOG_TOP_LEVEL = os.path.join(paths.VAR_LOG_PKI_DIR, PKI_INSTANCE_NAME)
 PKI_ROOT = '%s/%s' % (SERVER_ROOT, PKI_INSTANCE_NAME)
 CRL_PUBLISH_PATH = paths.PKI_CA_PUBLISH_DIR
 CS_CFG_PATH = '%s/conf/ca/CS.cfg' % PKI_ROOT
@@ -88,7 +91,10 @@ class Dogtag9Constants(object):
 DESTROY_BINARY = paths.PKISILENT
 
 SERVER_ROOT = paths.VAR_LIB
+PKI_INSTALL_LOG = paths.PKI_CA_INSTALL_LOG
+PKI_UNINSTALL_LOG = paths.PKI_CA_UNINSTALL_LOG
 PKI_INSTANCE_NAME = 'pki-ca'
+PKI_LOG_TOP_LEVEL = paths.PKI_CA_LOG_DIR
 PKI_ROOT = '%s/%s' % (SERVER_ROOT, PKI_INSTANCE_NAME)
 CRL_PUBLISH_PATH = paths.PKI_CA_PUBLISH_DIR
 CS_CFG_PATH = '%s/conf/CS.cfg' % PKI_ROOT
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index cf80d17e04fc59d97ad02116ccfbd3f8bbc10823..54f2f6c53c0103786b3a866f76df8ed365f64788 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -669,8 +669,7 @@ class CAInstance(service.Service):
 try:
 ipautil.run(args, nolog=nolog)
 except ipautil.CalledProcessError, e:
-root_logger.critical("failed to configure ca instance %s" % e)
-raise RuntimeError('Configuration of CA failed')
+self.handle_setup_error(e)
 finally:
 os.remove(cfg_file)
 
@@ -820,8 +819,7 @@ class CAInstance(service.Service):
 
 ipautil.run(args, env={'PKI_HOSTNAME':self.fqdn}, nolog=nolog)
 except ipautil.CalledProcessError, e:
-root_logger.critical("failed to configure ca instance %s" % e)
-raise RuntimeError('Configuration of CA failed')
+self.handle_setup_error(e)
 
 if self.external == 1:
 print "The next step is to get %s signed by your CA and re-run %s as:" % (self.csr_file, sys.argv[0])
@@ -1764,6 +1762,19 @@ class CAInstance(service.Service):
 master_entry['ipaConfigString'].append('caRenewalMaster')
 self.admin_conn.update_entry(master_entry)
 
+def handle_setup_error(self, e):
+root_logger.critical("Failed to configure CA instance: %s"
+  % e)
+root_logger.critical("See the installation logs and the following "
+  "files/directories for more information:")
+logs = [self.dogtag_constants.PKI_INSTALL_LOG,
+self.dogtag_constants.PKI_LOG_TOP_LEVEL]
+
+for log in logs:
+root_logger.critical("  %s" % log)
+
+raise RuntimeError("CA configuration failed.")
+
 
 def replica_ca_install_check(config):
 if not config.setup_ca:
-- 
2.1.0

From e4cada419253dadeaaa5a051e36d54d1a4fca6ae Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 20 Apr 2015 12:34:38 +0200
Subject: [PATCH] point the users to PKI-related logs when CA configuration
 fails

This patch adds an error handler which prints out the paths to logs related to
configuration and installation of Dogtag/CA in the case of failure.

https://fedorahosted.org/freeipa/ticket/4900
---
 ipapython/dogtag.py |  6 ++
 ipaserver/install/cainstance.py |  3 +--
 ipaserver/install/dogtaginstance.py | 17 ++---
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/ipapython/dogtag.py b/ipapython/dogtag.py
index 3d70bccfc32901ac884f5b412866d986a4087244..5dc532b7aa83586e7bc5a2904d01443d2979 100644
--- a/ipapython/dogtag.py
+++ b/ipapython/dogtag.py
@@ -55,7 +55,10 @@ class Dogtag10Constants(object):

Re: [Freeipa-devel] Splitting out ipaldap

[Petr Viktorin]

On 04/20/2015 08:30 AM, Jan Cholasta wrote:

Dne 16.4.2015 v 09:18 Petr Viktorin napsal(a):

On 04/15/2015 08:30 AM, Jan Cholasta wrote:

Dne 14.4.2015 v 19:21 Petr Viktorin napsal(a):

On 04/14/2015 06:18 PM, Jan Cholasta wrote:

Dne 14.4.2015 v 17:50 Petr Viktorin napsal(a):

On 04/14/2015 05:22 PM, Jan Cholasta wrote:

Hi,

Dne 14.4.2015 v 16:38 Petr Viktorin napsal(a):

Hello!

As some of you know, I'm looking to help porting FreeIPA to
Python 3.
One of the major dependencies holding this back is python-ldap,
which
hasn't been ported yet. Some preliminary porting patches by Raphaël
Barrois [0] are ready and have been sent to the python-ldap list.
The
python-ldap upstream has been very quiet about reviewing them so
far,
but they're something for me to test against, and maybe improve.

To make the testing easier, I'd like to split out "ipaldap" to a
stand-alone package, and port it to Python 3 first.
This split will make it easier to test (since I don't have to port
all
of IPA), and being able to use our generic LDAP wrappers in non-IPA
projects could maybe also invite some community participation.
Also,
ipaldap unit tests are somewhat lacking, so I'll help with that.
Packaging-wise, I want "ipaldap" to be on the same level as
"ipapython"
or "ipaserver"; additionally I want to release it on PyPI [1].


[...]

- Move ipapython.dn -> ipaldap.dn (keeping ipapython.dn importable
for
old scripts/plugins)


DNs are not strictly LDAP specific, so I would rather move
ipapython.dn
to a new ipautil package.


I'd rather not, at least until there's something that needs it (and
doesn't also depend on ipaldap). The scope of "ipautil" is far too
badly
defined for such a package to be useful.


IMO generic stuff should be in a package for generic stuff. I guess it
should originally have been ipapython, but it's too fused with ipalib
ATM, hence my proposal to use a new package.


No. Any vaguely defined collection of generic utilities needed in a
project is really a single-purpose package. Nobody likes pulling in a
bunch of unrelated stuff because of one particular thing they need, and
without a scope the amount of unnecessary stuff grows without bound.
I'd be OK with an "ipadn", if you can manage the overhead of a package.


IMO "ipadn" is just too specific. I guess we can use X.500 as scope,
since the basic types like DN or OID are defined in X.500, and put it in
"ipax500". Does that sound OK?


It might make sense conceptually, but do you have a use case? Some
software that would want to depend on python-ldap (since that's what DNs
depend on), but couldn't also bring in ipaldap?


I would rather get rid of the python-ldap dependency.

We talked about rewriting DN in C, because long-term we can't keep
working around the performance issues caused by DN being implemented in
Python.


Well all the more reason to not create a DN/x.500 library from the 
current code.



IMO we should do that for Python 3 and get rid of the
python-ldap dependency at the same time.


Removing the dependency, or rather making ipaldap depend on a new 
C-based DN/x.500 library instead, can be done at any time.

I don't think it should hold back porting to Python 3.


- Move CIDict to ipaldap.cidict. For Python 3, I'd really like to
replace this with something based on collections.MutableMapping,
since
the semantics of subclassing "dict" aren't very well defined.


I have WIP which does just that.


Could you send it?


Not yet unfortunately, CIDict removal is actually just a side
effect of
other changes, and it still needs a lot of work before it is sendable.


I was thinking the Python 3 boundary is a good point to switch, since
stuff will be breaking anyway. I can import the new one under py3, and
keep the old one for py2.



I'm a bit lost here, what do you mean by "new one" and "old one"?


Use the existing (old) CIDict under Python 2, and a new one based on
MutableMapping for all Python 3 code.


Wouldn't it be easier to use a custom MutableMapping for both? I can
code it now if you want, and replace it with my currently WIP stuff later.


I can also code it now. But I want to stay well away from breaking 
something on Python 2. A comment mentions isinstance(x, dict) being 
used, and I'd like to deal with cases like that as part of the porting.


--
Petr Viktorin

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 810 speed up indirect member processing

[Petr Vobornik]

On 04/20/2015 09:51 AM, Jan Cholasta wrote:

Dne 9.4.2015 v 13:56 Petr Vobornik napsal(a):

On 04/08/2015 10:21 AM, Jan Cholasta wrote:

Hi,

Dne 31.3.2015 v 12:11 Petr Vobornik napsal(a):

the old implementation tried to get all entries which are member of
group. That means also user. User can't have any members therefore this
costly processing was unnecessary.

New implementation reduces the search only to entries which can have
entries.

Also page size was removed to avoid paging by small pages(default size:
100) which is very slow for many members.

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

Useful to test with #809


1) To search for entries with members, you should search for entries
with the member attribute set ('(member=*)'), not for entries with some
arbitrary object class.


Replaced, new presence index added




2) I don't like how the search in get_memberindirect is limited to an
arbitrary hard-coded subtree. You should go through the object's
attribute_members to figure out which subtrees to search.



The subtree search was removed.



3) Since memberindirect and memberofindirect are not real attributes,
you must define their syntax in ipaldap before you cat set them using
.raw[], otherwise they will be decoded to wrong type.


Added.



4) The processing of memberof should be done even when memberofindirect
is not requested, otherwise its value will depend on whether
memberofindirect was requested or not.


True, but it's the same behavior as before. Could be changed in other
patch.


OK. Should we file a ticket?


AFAIK, memberof and memberofindirect are requested always together atm. 
Do we have a use case for this change? In any case, I've opened a ticket 
about more finer control of fetching members (as was discussed 
previously in triage and dev mtgs), it might be part of it.


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






5) I would prefer if all membership processing
(.convert_attribute_members() and .get_indirect_members()) was done in a
single LDAPObject method.


Now, as before, get_indirect_members is called before post callbacks and
convert_attribute_members after. If it should be combined, it should be
done separately.


OK, but at least move get_indirect_members to LDAPObject.



Moved
--
Petr Vobornik
From 68164aa2a5acba14dbfd6bb6d0f2b45cadf6a503 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Tue, 31 Mar 2015 10:59:37 +0200
Subject: [PATCH] speed up indirect member processing

the old implementation tried to get all entries which are member of group.
That means also user. User can't have any members therefore this costly
processing was unnecessary.

New implementation reduces the search only to entries which have members.

Also page size was removed to avoid paging by small pages(default size: 100)
which is very slow for many members.

https://fedorahosted.org/freeipa/ticket/4947
---
 install/updates/20-indices.update |  2 +-
 ipalib/plugins/baseldap.py| 72 +++
 ipalib/plugins/host.py|  2 +-
 ipalib/plugins/role.py|  8 ++--
 ipapython/ipaldap.py  |  2 +
 ipaserver/plugins/ldap2.py| 90 ---
 6 files changed, 81 insertions(+), 95 deletions(-)

diff --git a/install/updates/20-indices.update b/install/updates/20-indices.update
index a8a432d9c28a7fb4ca74582e36d4c39fd98df2cf..a9ec9f9eb9bcc228dcbb7eba99879ce5a8251e80 100644
--- a/install/updates/20-indices.update
+++ b/install/updates/20-indices.update
@@ -27,7 +27,7 @@ default:nsSystemIndex: false
 only:nsIndexType: eq,pres,sub
 
 dn: cn=member,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
-only:nsIndexType: eq,sub
+only:nsIndexType: eq,pres,sub
 
 dn: cn=uniquemember,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
 only:nsIndexType: eq,sub
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index ca4e54fd269aba9fdcf234f6450382fb7daabe42..b06b570cbc353aa903e5b2218932b8c6e59ce31b 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -663,6 +663,67 @@ class LDAPObject(Object):
 new_attr.append(new_value)
 break
 
+def get_indirect_members(self, entry_attrs, attrs_list):
+if 'memberindirect' in attrs_list:
+self.get_memberindirect(entry_attrs)
+if 'memberofindirect' in attrs_list:
+self.get_memberofindirect(entry_attrs)
+
+def get_memberindirect(self, group_entry):
+"""
+Get indirect members
+"""
+
+mo_filter = self.backend.make_filter({'memberof': group_entry.dn})
+filter = self.backend.combine_filters(
+('(member=*)', mo_filter), self.backend.MATCH_ALL)
+try:
+result, truncated = self.backend.find_entries(
+base_dn=self.api.env.basedn,
+filter=filter,
+attrs_list=['member'],
+size_limit=-1, # paged search will get ev

Re: [Freeipa-devel] [PATCH 0029] suppress errors arising from deleting non-existent files during client uninstall

[Martin Babinsky]

On 04/20/2015 10:32 AM, Martin Basti wrote:

On 17/04/15 14:11, Martin Babinsky wrote:

On 04/17/2015 12:41 PM, Martin Babinsky wrote:

On 04/17/2015 12:36 PM, Martin Basti wrote:

On 17/04/15 12:33, Martin Babinsky wrote:

On 04/17/2015 12:04 PM, Martin Basti wrote:

On 15/04/15 15:53, Martin Babinsky wrote:

On 04/14/2015 04:24 PM, Martin Basti wrote:

On 14/04/15 16:12, Martin Basti wrote:

On 14/04/15 14:25, Martin Babinsky wrote:

This patch addresses https://fedorahosted.org/freeipa/ticket/4966

The noise during rollback/uninstall is caused mainly by
unsuccessful
attempts to remove files that do not exist anymore. These errors
are
now logged at debug level and do not pop-up to stdout/stderr.




Hello, thank you for the patch.

1)
The option add_warning is quite unclear to me. It does not show
warning but error. I suggest something like, show_hint,
show_user_action, or something show_additional_..., or
promt_manual_removal

Martin^2



Continue...

2)

 if file_exists(preferences_fname):
 try:
 os.remove(preferences_fname)
 except OSError as e:
 log_file_removal_error(e, preferences_fname,
True)

In this case file not found error should never happen.

Could you remove the 'if file_exists' part and handle just
exception?


I just reverted this bit to original form in order to not fix
something that isn't broken. Is that ok?

3)
this is inconsistent with change above, choose one style please:

 if os.path.exists(ca_file):
 try:
 os.unlink(ca_file)
 except OSError, e:
 root_logger.error(
 "Failed to remove '%s': %s", ca_file, e)

--
Martin Basti



Attaching updated patch.


thanks,

just one nitpick, can you move the new function into installutils, it
can be used in different scripts not just in ipaclient.



I'm not sure if it is a good idea as installutils is a part for
freeipa-server package.

Placing it there would create an unnecessary dependency of
freeipa-client on freeipa-server because of a single function.


you are right, I do not why I thought that ipa-client-install uses
installutils.

ACK


self-NACK, I will try to rewrite the patch in a slightly less dumb way.

Sorry for the confusion.



Attaching updated patch which does the same but using a wrapper around
os.remove().

Jan suggested to keep the new function in 'ipa-client-install' and
move it around when we do installer re#$%@^ing.

Is that ok?


It looks better, ACK.


Jan NACKed your ACK.

Attaching updated patch.

--
Martin^3 Babinsky
From 965f349be7941ac6edf2f1faccf7b4e717dacb0d Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Tue, 14 Apr 2015 13:55:33 +0200
Subject: [PATCH] suppress errors arising from deleting non-existent files
 during client uninstall

When rolling back partially configured IPA client a number of OSErrors pop up
due to uninstaller trying to remove files that do not exist anymore. This
patch supresses these errors while keeping them in log as debug messages.

https://fedorahosted.org/freeipa/ticket/4966
---
 ipa-client/ipa-install/ipa-client-install | 40 +--
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 3f9a7419a10ddcb4618e80789a06a05058d1e8a4..57c2d9481461a1db8b00c897d8a37f6a6e583cf0 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -224,6 +224,25 @@ def logging_setup(options):
 console_format='%(message)s')
 
 
+def remove_file(filename):
+"""
+Deletes a file. If the file does not exist (OSError 2) does nothing.
+Otherwise logs an error message and instructs the user to remove the
+offending file manually
+:param filename: name of the file to be removed
+"""
+
+try:
+os.remove(filename)
+except OSError as e:
+if e.errno == 2:
+return
+
+root_logger.error("Failed to remove file %s: %s", filename, e)
+root_logger.error('Please remove %s manually, as it can cause '
+  'subsequent installation to fail.', filename)
+
+
 def log_service_error(name, action, error):
 root_logger.error("%s failed to %s: %s", name, action, str(error))
 
@@ -529,10 +548,7 @@ def uninstall(options, env):
  os.path.join(ipa_db.secdir, 'key3.db'),
  os.path.join(ipa_db.secdir, 'secmod.db'),
  os.path.join(ipa_db.secdir, 'pwdfile.txt')):
-try:
-os.remove(filename)
-except OSError, e:
-root_logger.error("Failed to remove %s: %s", filename, e)
+remove_file(filename)
 
 for nickname, trust_flags in ipa_certs:
 while sys_db.has_nickname(nickname):
@@ -760,25 +776,13 @@ def uninstall(options, env):
 'to its pre-installat

Re: [Freeipa-devel] [PATCH 0029] suppress errors arising from deleting non-existent files during client uninstall

[Martin Basti]

On 17/04/15 14:11, Martin Babinsky wrote:

On 04/17/2015 12:41 PM, Martin Babinsky wrote:

On 04/17/2015 12:36 PM, Martin Basti wrote:

On 17/04/15 12:33, Martin Babinsky wrote:

On 04/17/2015 12:04 PM, Martin Basti wrote:

On 15/04/15 15:53, Martin Babinsky wrote:

On 04/14/2015 04:24 PM, Martin Basti wrote:

On 14/04/15 16:12, Martin Basti wrote:

On 14/04/15 14:25, Martin Babinsky wrote:

This patch addresses https://fedorahosted.org/freeipa/ticket/4966

The noise during rollback/uninstall is caused mainly by
unsuccessful
attempts to remove files that do not exist anymore. These errors
are
now logged at debug level and do not pop-up to stdout/stderr.




Hello, thank you for the patch.

1)
The option add_warning is quite unclear to me. It does not show
warning but error. I suggest something like, show_hint,
show_user_action, or something show_additional_..., or
promt_manual_removal

Martin^2



Continue...

2)

 if file_exists(preferences_fname):
 try:
 os.remove(preferences_fname)
 except OSError as e:
 log_file_removal_error(e, preferences_fname,
True)

In this case file not found error should never happen.

Could you remove the 'if file_exists' part and handle just 
exception?



I just reverted this bit to original form in order to not fix
something that isn't broken. Is that ok?

3)
this is inconsistent with change above, choose one style please:

 if os.path.exists(ca_file):
 try:
 os.unlink(ca_file)
 except OSError, e:
 root_logger.error(
 "Failed to remove '%s': %s", ca_file, e)

--
Martin Basti



Attaching updated patch.


thanks,

just one nitpick, can you move the new function into installutils, it
can be used in different scripts not just in ipaclient.



I'm not sure if it is a good idea as installutils is a part for
freeipa-server package.

Placing it there would create an unnecessary dependency of
freeipa-client on freeipa-server because of a single function.


you are right, I do not why I thought that ipa-client-install uses
installutils.

ACK


self-NACK, I will try to rewrite the patch in a slightly less dumb way.

Sorry for the confusion.



Attaching updated patch which does the same but using a wrapper around 
os.remove().


Jan suggested to keep the new function in 'ipa-client-install' and 
move it around when we do installer re#$%@^ing.


Is that ok?


It looks better, ACK.

--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

[Jan Cholasta]

Dne 20.4.2015 v 10:06 Martin Babinsky napsal(a):

On 04/20/2015 09:48 AM, Jan Cholasta wrote:

Dne 15.4.2015 v 15:17 Martin Babinsky napsal(a):

On 04/13/2015 02:16 PM, Martin Babinsky wrote:

On 04/09/2015 03:38 PM, Jan Cholasta wrote:



Some comments:

Patch 15:

1) The functions should be as similar as possible:

 a) kinit_password() should have a 'ccache_path' argument
instead of
passing the path in KRB5CCNAME in the 'env' argument.

 b) I don't think kinit_password() should have the 'env'
argument at
all. You can always call kinit with LC_ALL=C and set other
variables in
os.environ if you want.

 c) The arguments should have the same ordering.

 d) Either set KRB5CCNAME in both kinit_keytab() and
kinit_password() or in none of them.

 e) Either rename armor_ccache to armor_ccache_path or ccache_path
to ccache.


I have done some reordering of parameters in both functions so they are
very similar now and the parameter ordering should make more sense (at
least to me).

Neither of them sets KRB5CCNAME env. variable since I think that it is
not a very good practice and the developer should be responsible for
pointing to correct CCache path. Jan agrees with this and the other
patches are updated accordingly.


2) Space before comma in docstring:

+Given a ccache_path , keytab file and a principal kinit as that
user.


3) I would prefer if the default value of 'armor_ccache' in
kinit_password() was None.


Fixed.


Patch 16:

1) The callback should not be named 'validate_kinit_attempts_option',
but rather 'kinit_attempts_callback', as it doesn't just validate the
value.


Fixed.


2) Why is there the sys.maxint upper bound on --kinit-attempts
again? A
comment with explanation would be nice.


It actually doesn't make much sense to have such upper bound, so I have
removed it from the check and updated the error message accordingly.


Patch 17:

1) Is there a reason for the ccache filename changes in DNSSEC code?


That was Petr Spacek's request since a sane naming of persistent
Ccaches
makes debugging of Kerberos-related errors a bit easier for him.

Attaching updated patches.





Jan had some further suggestions so I am attaching updated patches which
should reflect them.

He also recommended to split the naming changes of DNSSEC daemon
credential caches to a separate patch, so I will submit them later when
this patchset is pushed.



ACK. The patches need to be rebased on top of ipa-4-1 though.



Right, attaching rebased patches.



Thanks.

Pushed to:
master: 3d2feac0e416c66ba37eee53ef5b3833c2c3e414
ipa-4-1: 0ca8254959f3566c322eb7b20c6d6522814d78d1

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES] 0688-0689 Remove Editable DN and DN component classes

[Jan Cholasta]

Dne 16.4.2015 v 14:35 Petr Viktorin napsal(a):

On 04/16/2015 09:04 AM, Jan Cholasta wrote:

Hi,

Dne 10.4.2015 v 15:58 Petr Viktorin napsal(a):

The attached patches remove EditableDN, EditableRDN and EditableAVA.
They depend on Petr Voborník's patch 811 (performance: faster DN
implementation).


Mutable DNs are not very useful. When creating them it is easier to work
with lists or generators, and needing to change DNs aside from
operations like `DN(new_rdn, original[1:])` is very rare -- I'd even say
theoretical.
Mutable DNs are not hashable, so they can't be used as dist keys.
Storing them as "keys" in other structures (e.g. in a LDAPEntry) is
dangerous -- it's hard to reason about outside modifications.

The first patch removes the last use of EditableDN. I could be convinced
it's not an improvement in elegance/readability, but I believe this is
the strongest case for EditableDN in IPA, and it doesn't justify keeping
it.


LGTM, but patch 688 needs to be rebased.


Here you go.


Regarding patch 688, it seems we are always replacing the suffix of the 
DN, so I think we can simplify _dn_replace to:


if not dn.endswith(old):
raise ValueError('no replacement made')
return DN(*dn[:-len(old)]) + new

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] User life cycle: How to update 60basev3.ldif

[thierry bordaz]

On 04/20/2015 09:10 AM, Petr Spacek wrote:

On 17.4.2015 17:16, thierry bordaz wrote:

Hello,

User life cycle uses a new DS aci right: moddn. This right comes
with two new target keywords (target_to and target_from).
permission plugins should support those new target keywords and so
those attributes need to be defined in the schema 60basev3.ldif.

When adding new attributes in that schema, I should pick new OIDs.
Is it ok to pick the next ones available in ds-oids/08-FreeIPA.txt

I would say that these ACI-related attributes should be assigned from DS-core
OID arc (assuming that this ACI is not FreeIPA-specific and will be available
in core 389 DS).

Thanks Petr.
I wanted to allow new FreeIPA specific attributes to the definition of 
'ipaPermissionV2' objectclass.

So it is Freeipa-core OIDs.

My understanding is that when the patch on 60baseV3.ldif will be 
reviewed, I will consequently update/push 08-FreeIPA.txt changes.


thanks
thierry



(rhanana) and what is review process for changes in 08-FreeIPA.txt ?

Use your best judgment :-) Definitions will be reviewed later when LDIF with
it is sent as a patch to freeipa-devel list.



--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

[Martin Babinsky]

On 04/20/2015 09:48 AM, Jan Cholasta wrote:

Dne 15.4.2015 v 15:17 Martin Babinsky napsal(a):

On 04/13/2015 02:16 PM, Martin Babinsky wrote:

On 04/09/2015 03:38 PM, Jan Cholasta wrote:



Some comments:

Patch 15:

1) The functions should be as similar as possible:

 a) kinit_password() should have a 'ccache_path' argument
instead of
passing the path in KRB5CCNAME in the 'env' argument.

 b) I don't think kinit_password() should have the 'env'
argument at
all. You can always call kinit with LC_ALL=C and set other variables in
os.environ if you want.

 c) The arguments should have the same ordering.

 d) Either set KRB5CCNAME in both kinit_keytab() and
kinit_password() or in none of them.

 e) Either rename armor_ccache to armor_ccache_path or ccache_path
to ccache.


I have done some reordering of parameters in both functions so they are
very similar now and the parameter ordering should make more sense (at
least to me).

Neither of them sets KRB5CCNAME env. variable since I think that it is
not a very good practice and the developer should be responsible for
pointing to correct CCache path. Jan agrees with this and the other
patches are updated accordingly.


2) Space before comma in docstring:

+Given a ccache_path , keytab file and a principal kinit as that
user.


3) I would prefer if the default value of 'armor_ccache' in
kinit_password() was None.


Fixed.


Patch 16:

1) The callback should not be named 'validate_kinit_attempts_option',
but rather 'kinit_attempts_callback', as it doesn't just validate the
value.


Fixed.


2) Why is there the sys.maxint upper bound on --kinit-attempts again? A
comment with explanation would be nice.


It actually doesn't make much sense to have such upper bound, so I have
removed it from the check and updated the error message accordingly.


Patch 17:

1) Is there a reason for the ccache filename changes in DNSSEC code?


That was Petr Spacek's request since a sane naming of persistent Ccaches
makes debugging of Kerberos-related errors a bit easier for him.

Attaching updated patches.





Jan had some further suggestions so I am attaching updated patches which
should reflect them.

He also recommended to split the naming changes of DNSSEC daemon
credential caches to a separate patch, so I will submit them later when
this patchset is pushed.



ACK. The patches need to be rebased on top of ipa-4-1 though.



Right, attaching rebased patches.

--
Martin^3 Babinsky
From 221023c2680b5f4e7895cad9d969a6e81c38518d Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 16 Mar 2015 16:28:54 +0100
Subject: [PATCH 1/3] ipautil: new functions kinit_keytab and kinit_password

kinit_keytab replaces kinit_hostprincipal and performs Kerberos auth using
keytab file. Function is also able to repeat authentication multiple times
before giving up and raising Krb5Error.

kinit_password wraps kinit auth using password and also supports FAST
authentication using httpd armor ccache.
---
 ipapython/ipautil.py | 71 +++-
 1 file changed, 54 insertions(+), 17 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 24c090f91acf7af0dc6026cdc403dabcb019cc6d..5111c983ac40f7be64d2bc7088bea3855d79dd25 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1185,27 +1185,64 @@ def wait_for_open_socket(socket_name, timeout=0):
 else:
 raise e
 
-def kinit_hostprincipal(keytab, ccachedir, principal):
+
+def kinit_keytab(principal, keytab, ccache_name, attempts=1):
+"""
+Given a ccache_path, keytab file and a principal kinit as that user.
+
+The optional parameter 'attempts' specifies how many times the credential
+initialization should be attempted in case of non-responsive KDC.
 """
-Given a ccache directory and a principal kinit as that user.
+errors_to_retry = {krbV.KRB5KDC_ERR_SVC_UNAVAILABLE,
+   krbV.KRB5_KDC_UNREACH}
+root_logger.debug("Initializing principal %s using keytab %s"
+  % (principal, keytab))
+root_logger.debug("using ccache %s" % ccache_name)
+for attempt in range(1, attempts + 1):
+try:
+krbcontext = krbV.default_context()
+ktab = krbV.Keytab(name=keytab, context=krbcontext)
+princ = krbV.Principal(name=principal, context=krbcontext)
+ccache = krbV.CCache(name=ccache_name, context=krbcontext,
+ primary_principal=princ)
+ccache.init(princ)
+ccache.init_creds_keytab(keytab=ktab, principal=princ)
+root_logger.debug("Attempt %d/%d: success"
+  % (attempt, attempts))
+return
+except krbV.Krb5Error as e:
+if e.args[0] not in errors_to_retry:
+raise
+root_logger.debug("Attempt %d/%d: failed: %s"
+  % (attempt, attempts, e))
+if

Re: [Freeipa-devel] [PATCH] 809 speed up convert_attribute_members

[Jan Cholasta]

Dne 9.4.2015 v 13:56 Petr Vobornik napsal(a):

On 04/02/2015 09:47 AM, Jan Cholasta wrote:

Hi,

Dne 31.3.2015 v 12:11 Petr Vobornik napsal(a):

A workaround to avoid usage of slow LDAPEntry._sync_attr #4946.

I originally wanted to avoid DN processing as well but we can't do that
because of DNs which are encoded - e.g. contains '+' or ','. Therefore
patch 811 - faster DN implementation is very useful. Also patch 809 is
useful to avoid high load of 389.

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



1)

+dn = container_dns.get(ldap_obj_name, None)
+if not dn:
+ldap_obj = self.api.Object[ldap_obj_name]
+dn = DN(ldap_obj.container_dn, api.env.basedn)
+container_dns[ldap_obj_name] = dn
+return dn

 a) The second argument of .get() is None by default

 b) "not dn" matches None as well as empty DNs, use "dn is not None"
(it's not that there could be empty DNs here, but let's not give a
potential reader the wrong idea)

 c) It would be better to catch KeyError rather than call .get() and
check the result:

 try:
 dn = container_dns[ldap_obj_name]
 except KeyError:
 dn = ...
 container_dns[ldap_obj_name] = dn


Changed




2) Does get_new_attr() actually provide any speed up? Unless I'm missing
something, it just mirrors the virtual member attributes already readily
available from entry_attrs in new_attrs.


Yes, a bit. With 30K members and my vm get_new_attr takes ~ 0.114s.
setdefault takes ~ 0.686s which is about 7-10% of the entire
convert_attribute_members. Pure dict is faster.




3) get_container_dn() and get_new_attr() do not need to be functions,
since each is called just from a single spot.


Changed




4) "memberdn = DN(member)" could be one for loop up.



Changed



Here's what I ended up with trying to fix the above (untested):

 for attr in self.attribute_members:
 try:
 value = entry_attrs.raw[attr]
 except KeyError:
 continue
 del entry_attrs[attr]

 ldap_objs = {}
 for ldap_obj_name in self.attribute_members[attr]:
 ldap_obj = self.api.Object[ldap_obj_name]
 container_dn = DN(ldap_obj.container_dn, api.env.basedn)
 ldap_objs[container_dn] = ldap_obj

 for member in value:
 memberdn = DN(member)
 try:
 ldap_obj = ldap_objs[DN(*memberdn[1:])]
 except KeyError:
 continue

 new_attr = '%s_%s' % (attr, ldap_obj.name)
 new_value = ldap_obj.get_primary_key_from_dn(memberdn)
 entry_attrs.setdefault(new_attr, []).append(new_value)


Without any modifications the code is ~ 2.3x slower than mine. In patch
811 DN's slice, __hash__ and __eq__ functions are optimized.


Thanks, ACK.

Pushed to master: e4930b3235e5d61d227a7e43d30a8feb7f35664d

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 810 speed up indirect member processing

[Jan Cholasta]

Dne 9.4.2015 v 13:56 Petr Vobornik napsal(a):

On 04/08/2015 10:21 AM, Jan Cholasta wrote:

Hi,

Dne 31.3.2015 v 12:11 Petr Vobornik napsal(a):

the old implementation tried to get all entries which are member of
group. That means also user. User can't have any members therefore this
costly processing was unnecessary.

New implementation reduces the search only to entries which can have
entries.

Also page size was removed to avoid paging by small pages(default size:
100) which is very slow for many members.

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

Useful to test with #809


1) To search for entries with members, you should search for entries
with the member attribute set ('(member=*)'), not for entries with some
arbitrary object class.


Replaced, new presence index added




2) I don't like how the search in get_memberindirect is limited to an
arbitrary hard-coded subtree. You should go through the object's
attribute_members to figure out which subtrees to search.



The subtree search was removed.



3) Since memberindirect and memberofindirect are not real attributes,
you must define their syntax in ipaldap before you cat set them using
.raw[], otherwise they will be decoded to wrong type.


Added.



4) The processing of memberof should be done even when memberofindirect
is not requested, otherwise its value will depend on whether
memberofindirect was requested or not.


True, but it's the same behavior as before. Could be changed in other
patch.


OK. Should we file a ticket?






5) I would prefer if all membership processing
(.convert_attribute_members() and .get_indirect_members()) was done in a
single LDAPObject method.


Now, as before, get_indirect_members is called before post callbacks and
convert_attribute_members after. If it should be combined, it should be
done separately.


OK, but at least move get_indirect_members to LDAPObject.






Honza




--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

[Jan Cholasta]

Dne 15.4.2015 v 15:17 Martin Babinsky napsal(a):

On 04/13/2015 02:16 PM, Martin Babinsky wrote:

On 04/09/2015 03:38 PM, Jan Cholasta wrote:



Some comments:

Patch 15:

1) The functions should be as similar as possible:

 a) kinit_password() should have a 'ccache_path' argument instead of
passing the path in KRB5CCNAME in the 'env' argument.

 b) I don't think kinit_password() should have the 'env' argument at
all. You can always call kinit with LC_ALL=C and set other variables in
os.environ if you want.

 c) The arguments should have the same ordering.

 d) Either set KRB5CCNAME in both kinit_keytab() and
kinit_password() or in none of them.

 e) Either rename armor_ccache to armor_ccache_path or ccache_path
to ccache.


I have done some reordering of parameters in both functions so they are
very similar now and the parameter ordering should make more sense (at
least to me).

Neither of them sets KRB5CCNAME env. variable since I think that it is
not a very good practice and the developer should be responsible for
pointing to correct CCache path. Jan agrees with this and the other
patches are updated accordingly.


2) Space before comma in docstring:

+Given a ccache_path , keytab file and a principal kinit as that
user.


3) I would prefer if the default value of 'armor_ccache' in
kinit_password() was None.


Fixed.


Patch 16:

1) The callback should not be named 'validate_kinit_attempts_option',
but rather 'kinit_attempts_callback', as it doesn't just validate the
value.


Fixed.


2) Why is there the sys.maxint upper bound on --kinit-attempts again? A
comment with explanation would be nice.


It actually doesn't make much sense to have such upper bound, so I have
removed it from the check and updated the error message accordingly.


Patch 17:

1) Is there a reason for the ccache filename changes in DNSSEC code?


That was Petr Spacek's request since a sane naming of persistent Ccaches
makes debugging of Kerberos-related errors a bit easier for him.

Attaching updated patches.





Jan had some further suggestions so I am attaching updated patches which
should reflect them.

He also recommended to split the naming changes of DNSSEC daemon
credential caches to a separate patch, so I will submit them later when
this patchset is pushed.



ACK. The patches need to be rebased on top of ipa-4-1 though.

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] User life cycle: How to update 60basev3.ldif

[Petr Spacek]

On 17.4.2015 17:16, thierry bordaz wrote:
> Hello,
> 
>User life cycle uses a new DS aci right: moddn. This right comes
>with two new target keywords (target_to and target_from).
>permission plugins should support those new target keywords and so
>those attributes need to be defined in the schema 60basev3.ldif.
> 
>When adding new attributes in that schema, I should pick new OIDs.
>Is it ok to pick the next ones available in ds-oids/08-FreeIPA.txt
I would say that these ACI-related attributes should be assigned from DS-core
OID arc (assuming that this ACI is not FreeIPA-specific and will be available
in core 389 DS).

>(rhanana) and what is review process for changes in 08-FreeIPA.txt ?

Use your best judgment :-) Definitions will be reviewed later when LDIF with
it is sent as a patch to freeipa-devel list.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code