Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
On 08/26/2015 05:42 PM, Martin Basti wrote: On 08/26/2015 02:53 PM, Oleg Fayans wrote: Hi, No more short links :) On 08/26/2015 11:50 AM, Tomas Babej wrote: On 08/26/2015 11:44 AM, Oleg Fayans wrote: Hi Martin, On 08/20/2015 11:18 AM, Martin Basti wrote: On 08/20/2015 10:26 AM, Martin Basti wrote: On 08/19/2015 04:17 PM, Martin Basti wrote: I got this: https://paste.fedoraproject.org/256746/43999380/ FYI replica install failure. (I will retest it, but I'm pretty sure that it was clean VM, test for some reason install client first) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 295, in decorated func(installer) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 319, in install_check sys.exit(IPA client is already configured on this system.\n 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, exception: SystemExit: IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. 2015-08-19T14:14:15Z ERROR IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. Confirm I got same error. Fixed. It was a regex error. On 08/19/2015 09:00 AM, Oleg Fayans wrote: Hi Martin, As discussed, here is a new version with pep8-related fixes On 08/14/2015 10:44 AM, Oleg Fayans wrote: Hi Martin, Already noticed that. Implemented the named groups as Tomas advised. Added the third test for http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica On 08/13/2015 05:06 PM, Martin Basti wrote: On 08/11/2015 03:36 PM, Oleg Fayans wrote: Hi Martin, On 08/11/2015 02:02 PM, Martin Basti wrote: NACK, comments inline. On 11/08/15 13:25, Oleg Fayans wrote: Hi Martin, Thanks for the review! On 08/10/2015 07:08 PM, Martin Basti wrote: Thank you for patch, I have a few nitpicks: 1) On 10/08/15 13:05, Oleg Fayans wrote: +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) Why do you add the name of method in docstring? My bad, fixed. still there +tokenize_topologies(command_output) +takes an output of `ipa topologysegment-find` and returns an array of Fixed, sorry. 2) +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) +creates a topology segment. The first argument is a node to run the command on +The first 3 arguments should be objects of class Host +Returns a hash object containing segment's name, leftnode, rightnode information + I would prefer to add assert there instead of just document that a Host object is needed assert(isinstance(master, Host)) ... Fixed. Created a decorator that checks the type of arguments This does not scale well. If we will want to add new argument that is not host object, you need change it again. Agreed. Modified the decorator so that you can specify a slice of arguments to be checked and a class to compare to. This does scale :) This might be used as function with specified variables that have to be host objects 3) +def destroy_segment(master, segment_name): + +destroy_segment(master, segment_name) +Destroys topology segment. First argument should be object of class Host Instead of description of params as first, second etc., you may use following: +def destroy_segment(master, segment_name): + +Destroys topology segment. +:param master: reference to master object of class Host +:param segment: name fo segment and eventually this in other methods +:returns: Lorem ipsum sit dolor mit amet +:raises NotFound: if segment is not found Fixed 4) cls.replicas[:len(cls.replicas) - 1], I suggest cls.replicas[:-1] In [2]: a = [1, 2, 3, 4, 5] In [3]: a[:-1] Out[3]: [1, 2, 3, 4] Fixed 5) Why re.findall() and then you just use the first result? 'leftnode': self.leftnode_re.findall(i)[0] Isn't just re.search() enough? leftnode_re.search(value).group(1) in fact leftnode_re.findall(string)[0] and leftnode_re.search(string).group(1), Are equally bad from the readability point of view. The first one is even shorter a bit, so why change? :) It depends on point of view, because when I reviewed it yesterday my brain raises exception that you are trying to add multiple values to single value attribute, because you used findall, I expected that you need multiple values, and then I saw that index [0] at the end, and I was pretty confused what are you trying to achieve. And because findall is not effective in case when you need to find just one occurrence. I got it. Fixed. Python 3 nitpick: I'm not sure if time when we should
Re: [Freeipa-devel] [PATCH 477] spec file: Add Requires(pre) on selinux-policy
On Wed, 26 Aug 2015, Jan Cholasta wrote:
On 25.8.2015 21:25, Martin Kosek wrote:
On 08/25/2015 09:22 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 08:59 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 05:37 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 04:37 PM, Jan Cholasta wrote:
On 25.8.2015 14:50, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
On 25.8.2015 14:23, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
Hi,
the attached patch fixes
https://fedorahosted.org/freeipa/ticket/5256.
Honza
--
Jan Cholasta
From 216be8de30747f80f490d4e91a7cca4af3e767d6 Mon Sep 17
00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Tue, 25 Aug 2015 14:14:25 +0200
Subject: [PATCH] spec file: Add Requires(pre) on selinux-policy
This prevents ipa-server-upgrade failures on SELinux AVCs
because of
old
selinux-policy version.
https://fedorahosted.org/freeipa/ticket/5256
---
freeipa.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index cba91fe..fd73cda 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -139,6 +139,7 @@ Requires: systemd-units = 38
Requires(pre): shadow-utils
Requires(pre): systemd-units
Requires(post): systemd-units
+Requires(pre): selinux-policy = %{selinux_policy_version}
Requires: selinux-policy = %{selinux_policy_version}
Requires(post): selinux-policy-base
Requires: slapi-nis = 0.54.2-1
If we have it in Requires(pre), we don't need it in Requires, as
Requires(pre) is a superset of guarantees that Requires gives
you.
Martin (CCed) told me Requires(pre) does not imply Requires.
See http://rpm.org/api/4.4.2.2/tsort.html (available since 2007):
Since the only way out of a dependency loop is to snip the loop
somewhere, rpm uses hints from Requires: dependencies to
distinguish
co-requisite (these are not needed to install, only to use, a
package)
from pre-requisite (these are guaranteed to be installed before
the
package that includes the dependency) relations.
Requires(pre) ensures that selinux-policy of specific version is
installed before pre scripts of freeipa-server would run, be
it in the
same transaction or in a previous one.
Hmm, ipa-server-upgrade is run in posttrans. Should the
Requires(pre)
be changed to Required(posttrans)?
I don't think there is posttrans target. Perhaps, we can just
make sure
Requires(post) is enough.
OK, let's try that. Updated patch attached.
Will this really make a difference? I thought the problem is
caused by
selinux-policy being installed after freeipa-server package
upgrade. We
already
have Requires on selinux-policy, so I am not sure what is actually
changed by
this patch.
The change is that with Requires(pre) or Requires(post) we are
guaranteed that selinux-policy is installed and available before
our pre
or post scriptlets are run. With Requires only we are not
guaranteed to
be installed after selinux-policy, only that it would be available as
part of the same transaction we are installed in.
We don't really need to have Requires(pre) because we don't rely on
selinux-policy being available in pre scriptlet. Forcing
Requires(pre)
doesn't help anyone else (rpm/yum/dnf need to solve dependency
loops and
we are only complicating with Requires(pre) if we don't actually need
it). Thus, choosing Require(post) is more correct from distribution
point of view.
Sure, but given that FreeIPA upgrade is run in the posttrans phase:
%posttrans server
# This must be run in posttrans so that updates from previous
# execution that may no longer be shipped are not applied.
/usr/sbin/ipa-server-upgrade --quiet /dev/null || :
I am now not sure how Requires(pre) or Requires(post) help here, in
all
cases, the right selinux-policy should be there before all the
posttrans
scripts are being run.
I've looked at the rpm source code and here is the list of all
supported
requires/dependencies types:
https://github.com/rpm-software-management/rpm/blob/140744377b019e0de81d76d0931c32228d2ed57e/build/rpmfc.c#L1056
Requires(posttrans) is there so we could use this one too but it was
added only in 4.12-alpha which means it is missing in RHEL/CentOS 7,
for
example, as they are only up to 4.11.
Maybe the new selinux-policy is required for certmonger itself or
some other
event during upgrade?
No, I don't think so. However, we cannot set Requires(posttrans), thus
we should be using closest target before it, i.e. Requires(post).
Thank you, but I think I still did not get an answer for my question.
IIUC, the rough rpm process with regards to freeipa-server package
upgrade,
it should be in this order:
_
|
v
RPM installs some dependencies of freeipa-server
|
V
RPM installs Requires(pre) of freeipa-server
freeipa-server pre scriptlet runs
|
v
RPM installs freeipa-server
|
v
RPM installs Requires(post) of freeipa-server
Re: [Freeipa-devel] kra an ca instance installation
On 25.8.2015 17:50, Simo Sorce wrote:
On Tue, 2015-08-25 at 16:22 +0200, Jan Cholasta wrote:
On 25.8.2015 16:09, Simo Sorce wrote:
On Tue, 2015-08-25 at 15:52 +0200, Jan Cholasta wrote:
On 25.8.2015 15:37, Simo Sorce wrote:
On Tue, 2015-08-25 at 11:34 +0200, Jan Cholasta wrote:
On 25.8.2015 11:25, Jan Cholasta wrote:
On 24.8.2015 19:51, Simo Sorce wrote:
Why do we have cainstance.py and ca.py and krainstance.py and kra.py in
ipaserver/install when you always need both files to do anything around
installation of the ca ?
Is there a motivation ?
Or can I simply provide a patch to remove the ca.py and kra.py files an
unify all code in the proper *instance.py file ?
ca.py and kra.py are the proper files ready to be migrated to the new
install framework, cainstance.py and krainstance.py will be removed.
... once the migration is done. (Hit send button too fast.)
The motivation is that *instance.py do not provide a uniform interface,
have a lot of redundant and duplicate stuff and are generally unfit for
any further extension.
I have been changing only the instance files, so we are going in
different directions.
I don't see how, {ca,kra}instance.py code is called from {ca,kra}.py.
{ca,kra}.py contains all the code that is actually needed to install
CA/KRA that is not in {ca,kra}instance.py and was previously scattered
around ipa-{server,replica,ca,kra}-install.
I do not really care what file we are going into, but there is a lot of
code in the installer now that does not tell the user a step is being
done, while instances do that through the step interface.
This code was always there, we only moved it to a single location. See
git history.
The step interface is also a very good way to let someone that read the
code see what is going on and follow each step.
Are you proposing to stop going through the instances steps ? I found
the current way kra and ca installation is setup basically a regression,
it took me a *lot* longer that it should be needed to follow through all
the steps that are really taken.
Again, we only moved the code around, and I don't think we created any
regressions.
Ok, so the plan is just to move the CAInstance class and code *as is*
from cainstance.py to ca.py ?I guess I am confused about what is your
plan around this exactly.
The code in CAInstance will be gradually migrated to a new class in
ca.py which uses the new install framework. We started with the
top-level ipa-server-install and ipa-replica-install code, which is
still not done, but you can see it for reference on how it will look
like (ipaserver/install/server/*.py, especially the classes at the
bottom of the files).
Well I had to modify server/replicainstall.py quite radically, I had to
create a new set of promote_check and promote functions there. So we are
back to duplicated code, sorry (no way around it).
It's OK, there is still a lot of duplicate code in server/replica
install anyway, we deduplicated only the CA, KRA and DNS install code so
far.
However the functions in server/replicainstall.py still use the
instances mostly for all components *except* for kra and ca where there
is really confusing code calling unnecessarily instances multiple times
and fooling around with stuff.
If you think there are unnecessary calls I think you are reading the
code wrong.
I do not really understand what you mean
by migrating from CAInstance to what's in server/*install.py because in
there all instances are used for the individual components, so I am now
scratching my head.
What I mean is that the code to install a component will be wrapped in a
class similar to Server or Replica classes in these files. There will
still be steps like in CAInstance, but the rest will be different. I
don't think you have seen the PoC from 5 months ago:
https://www.redhat.com/archives/freeipa-devel/2015-March/msg00374.html.
The code in ca.py especially is really confusing, I rewrote it once to
eliminate duplications then decided to simply not touch it in my branch
(and threw away the modifications) because it is too confusing and I did
not want to risk regressions. So I created a simple create_replica()
function in the CA instance that does all that is needed.
Before ca.py, you would have to rewrite the same confusing code in
ipa-server-install, ipa-replica-install and ipa-ca-install. I don't see
how that's better.
I guess I will just keep ignoring the confusion and try to come up with
something that works but I'd really like to understand what is the
endgame there. If you want to replace instances (why?), what will you
replace it with ?
With something that does not mix install with service management,
maximizes code reuse and makes related code colocated, provides
introspection on configuration options, allows componentization and
idempotent execution. See the PoC linked above.
Simo.
--
Jan Cholasta
--
Manage your subscription for the Freeipa-devel mailing list:
Re: [Freeipa-devel] [PATCH 477] spec file: Add Requires(pre) on selinux-policy
On 26.8.2015 08:16, Alexander Bokovoy wrote:
On Wed, 26 Aug 2015, Jan Cholasta wrote:
On 25.8.2015 21:25, Martin Kosek wrote:
On 08/25/2015 09:22 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 08:59 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 05:37 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 04:37 PM, Jan Cholasta wrote:
On 25.8.2015 14:50, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
On 25.8.2015 14:23, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
Hi,
the attached patch fixes
https://fedorahosted.org/freeipa/ticket/5256.
Honza
--
Jan Cholasta
From 216be8de30747f80f490d4e91a7cca4af3e767d6 Mon Sep 17
00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Tue, 25 Aug 2015 14:14:25 +0200
Subject: [PATCH] spec file: Add Requires(pre) on
selinux-policy
This prevents ipa-server-upgrade failures on SELinux AVCs
because of
old
selinux-policy version.
https://fedorahosted.org/freeipa/ticket/5256
---
freeipa.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index cba91fe..fd73cda 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -139,6 +139,7 @@ Requires: systemd-units = 38
Requires(pre): shadow-utils
Requires(pre): systemd-units
Requires(post): systemd-units
+Requires(pre): selinux-policy = %{selinux_policy_version}
Requires: selinux-policy = %{selinux_policy_version}
Requires(post): selinux-policy-base
Requires: slapi-nis = 0.54.2-1
If we have it in Requires(pre), we don't need it in
Requires, as
Requires(pre) is a superset of guarantees that Requires gives
you.
Martin (CCed) told me Requires(pre) does not imply Requires.
See http://rpm.org/api/4.4.2.2/tsort.html (available since
2007):
Since the only way out of a dependency loop is to snip the loop
somewhere, rpm uses hints from Requires: dependencies to
distinguish
co-requisite (these are not needed to install, only to use, a
package)
from pre-requisite (these are guaranteed to be installed before
the
package that includes the dependency) relations.
Requires(pre) ensures that selinux-policy of specific
version is
installed before pre scripts of freeipa-server would run, be
it in the
same transaction or in a previous one.
Hmm, ipa-server-upgrade is run in posttrans. Should the
Requires(pre)
be changed to Required(posttrans)?
I don't think there is posttrans target. Perhaps, we can just
make sure
Requires(post) is enough.
OK, let's try that. Updated patch attached.
Will this really make a difference? I thought the problem is
caused by
selinux-policy being installed after freeipa-server package
upgrade. We
already
have Requires on selinux-policy, so I am not sure what is actually
changed by
this patch.
The change is that with Requires(pre) or Requires(post) we are
guaranteed that selinux-policy is installed and available before
our pre
or post scriptlets are run. With Requires only we are not
guaranteed to
be installed after selinux-policy, only that it would be
available as
part of the same transaction we are installed in.
We don't really need to have Requires(pre) because we don't rely on
selinux-policy being available in pre scriptlet. Forcing
Requires(pre)
doesn't help anyone else (rpm/yum/dnf need to solve dependency
loops and
we are only complicating with Requires(pre) if we don't actually
need
it). Thus, choosing Require(post) is more correct from distribution
point of view.
Sure, but given that FreeIPA upgrade is run in the posttrans phase:
%posttrans server
# This must be run in posttrans so that updates from previous
# execution that may no longer be shipped are not applied.
/usr/sbin/ipa-server-upgrade --quiet /dev/null || :
I am now not sure how Requires(pre) or Requires(post) help here, in
all
cases, the right selinux-policy should be there before all the
posttrans
scripts are being run.
I've looked at the rpm source code and here is the list of all
supported
requires/dependencies types:
https://github.com/rpm-software-management/rpm/blob/140744377b019e0de81d76d0931c32228d2ed57e/build/rpmfc.c#L1056
Requires(posttrans) is there so we could use this one too but it was
added only in 4.12-alpha which means it is missing in RHEL/CentOS 7,
for
example, as they are only up to 4.11.
Maybe the new selinux-policy is required for certmonger itself or
some other
event during upgrade?
No, I don't think so. However, we cannot set Requires(posttrans),
thus
we should be using closest target before it, i.e. Requires(post).
Thank you, but I think I still did not get an answer for my question.
IIUC, the rough rpm process with regards to freeipa-server package
upgrade,
it should be in this order:
_
|
v
RPM installs some dependencies of freeipa-server
|
V
RPM installs Requires(pre) of freeipa-server
freeipa-server pre scriptlet runs
|
v
RPM installs freeipa-server
|
v
RPM
Re: [Freeipa-devel] [PATCH 477] spec file: Add Requires(pre) on selinux-policy
On 25.8.2015 21:25, Martin Kosek wrote:
On 08/25/2015 09:22 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 08:59 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 05:37 PM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Martin Kosek wrote:
On 08/25/2015 04:37 PM, Jan Cholasta wrote:
On 25.8.2015 14:50, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
On 25.8.2015 14:23, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
Hi,
the attached patch fixes
https://fedorahosted.org/freeipa/ticket/5256.
Honza
--
Jan Cholasta
From 216be8de30747f80f490d4e91a7cca4af3e767d6 Mon Sep 17
00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Tue, 25 Aug 2015 14:14:25 +0200
Subject: [PATCH] spec file: Add Requires(pre) on selinux-policy
This prevents ipa-server-upgrade failures on SELinux AVCs
because of
old
selinux-policy version.
https://fedorahosted.org/freeipa/ticket/5256
---
freeipa.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index cba91fe..fd73cda 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -139,6 +139,7 @@ Requires: systemd-units = 38
Requires(pre): shadow-utils
Requires(pre): systemd-units
Requires(post): systemd-units
+Requires(pre): selinux-policy = %{selinux_policy_version}
Requires: selinux-policy = %{selinux_policy_version}
Requires(post): selinux-policy-base
Requires: slapi-nis = 0.54.2-1
If we have it in Requires(pre), we don't need it in Requires, as
Requires(pre) is a superset of guarantees that Requires gives
you.
Martin (CCed) told me Requires(pre) does not imply Requires.
See http://rpm.org/api/4.4.2.2/tsort.html (available since 2007):
Since the only way out of a dependency loop is to snip the loop
somewhere, rpm uses hints from Requires: dependencies to
distinguish
co-requisite (these are not needed to install, only to use, a
package)
from pre-requisite (these are guaranteed to be installed before
the
package that includes the dependency) relations.
Requires(pre) ensures that selinux-policy of specific version is
installed before pre scripts of freeipa-server would run, be
it in the
same transaction or in a previous one.
Hmm, ipa-server-upgrade is run in posttrans. Should the
Requires(pre)
be changed to Required(posttrans)?
I don't think there is posttrans target. Perhaps, we can just
make sure
Requires(post) is enough.
OK, let's try that. Updated patch attached.
Will this really make a difference? I thought the problem is
caused by
selinux-policy being installed after freeipa-server package
upgrade. We
already
have Requires on selinux-policy, so I am not sure what is actually
changed by
this patch.
The change is that with Requires(pre) or Requires(post) we are
guaranteed that selinux-policy is installed and available before
our pre
or post scriptlets are run. With Requires only we are not
guaranteed to
be installed after selinux-policy, only that it would be available as
part of the same transaction we are installed in.
We don't really need to have Requires(pre) because we don't rely on
selinux-policy being available in pre scriptlet. Forcing
Requires(pre)
doesn't help anyone else (rpm/yum/dnf need to solve dependency
loops and
we are only complicating with Requires(pre) if we don't actually need
it). Thus, choosing Require(post) is more correct from distribution
point of view.
Sure, but given that FreeIPA upgrade is run in the posttrans phase:
%posttrans server
# This must be run in posttrans so that updates from previous
# execution that may no longer be shipped are not applied.
/usr/sbin/ipa-server-upgrade --quiet /dev/null || :
I am now not sure how Requires(pre) or Requires(post) help here, in
all
cases, the right selinux-policy should be there before all the
posttrans
scripts are being run.
I've looked at the rpm source code and here is the list of all
supported
requires/dependencies types:
https://github.com/rpm-software-management/rpm/blob/140744377b019e0de81d76d0931c32228d2ed57e/build/rpmfc.c#L1056
Requires(posttrans) is there so we could use this one too but it was
added only in 4.12-alpha which means it is missing in RHEL/CentOS 7,
for
example, as they are only up to 4.11.
Maybe the new selinux-policy is required for certmonger itself or
some other
event during upgrade?
No, I don't think so. However, we cannot set Requires(posttrans), thus
we should be using closest target before it, i.e. Requires(post).
Thank you, but I think I still did not get an answer for my question.
IIUC, the rough rpm process with regards to freeipa-server package
upgrade,
it should be in this order:
_
|
v
RPM installs some dependencies of freeipa-server
|
V
RPM installs Requires(pre) of freeipa-server
freeipa-server pre scriptlet runs
|
v
RPM installs freeipa-server
|
v
RPM installs Requires(post) of freeipa-server
freeipa-server post scriptlet runs
|
v
RPM installs
Re: [Freeipa-devel] [PATCH 0066] ipactl: Do not start/stop/restart single service multiple times
On 08/26/2015 03:16 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/5248 ACK Pushed to: master: 59cc54b6dce29e32e81bfaad25ff13794092d782 ipa-4-2: 21cdcbd9a6b6a82d39d40b91a64d4d9b4d7e4e7d -- Petr Vobornik -- 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] How to support Designate?
On 08/25/2015 09:08 AM, Petr Spacek wrote: On 8.7.2015 19:56, Rich Megginson wrote: On 07/08/2015 10:11 AM, Petr Spacek wrote: Assuming that Designate wants to own DNS and be Primary Master, it would be awesome if they could support standard DNS UPDATE protocol (RFC 2136) alongside their own JSON API. The JSON API is superset of DNS UPDATE protocol because it allows to add zones but still, standard protocol would mean that standard client (possibly guest OS inside VM) can update its records without any OpenStack dependency, which is very much desirable. The use case here is to allow the guest OS to publish it's SSH key (which was generated inside the VM after first boot) to prevent Man in the middle attacks. I'm working on a different approach for guest OS registration. This involves a Nova hook/plugin: * build_instance pre-hook to generate an OTP and call ipa host-add with the OTP - add OTP to new host metadata - add ipa-client-registration script to new host cloud-init * new instance calls script - will wait for OTP to become available in metadata, then call ipa-client-install with OTP * Floating IP is assigned to VM - Nova hook will call dnsrecord-add with new IP https://github.com/richm/rdo-vm-factory/tree/master/rdo-ipa-nova The same goes for all other sorts of DANE/DNSSEC data or service discovery using DNS, where a guest/container running a distributed service can publish it's existence in DNS. DNS UPDATE supports GSS(API) for authentication via RFC 3007 and that is widely supported, too. So DNS UPDATE is my biggest wish :-) Ok. There was a Designate blueprint for such a feature, but I can't find it and neither can the Designate guys. There is a mention of nsupdate in the minidns blueprint, but that's about it. The fact that Designate upstream can't find the bp suggests that this is not a high priority for them and will not likely implement it on their own i.e. we would have to contribute this feature. If Designate had such a feature, how would this help us integrate FreeIPA with Designate? It would greatly simplify integration with FreeIPA. There is a plan to support DNS updates as described in RFC 2136 to push updates from FreeIPA servers to external DNS servers, so we could use the same code to integrate with AD Designate at the same time. (I'm sorry for the delay, it somehow slipped through the cracks.) For Designate for our use cases, we want IPA to be the authoritative source of DNS data. When a client wants to read data from Designate, that data should somehow come from IPA. I don't think Designate has any sort of proxy or pass-through feature, so the data would have be sync'd from IPA. If IPA supports being a server for AXFR/IXFR, Designate could be changed to support AXFR/IXFR client side, then would just be a slave of IPA. If IPA does not support zone transfers, then we would need some sort of initial sync of data from IPA to Designate (I wrote such a script for Designate (https://github.com/openstack/designate/blob/master/contrib/ipaextractor.py). Then, perhaps some sort of proxy/service that would poll for changes (syncrepl?) in IPA, then submit those changes to Designate (using Designate REST API, or DNS UPDATE when Designate mDNS supports it). When a client wants to update data in Designate, we need to somehow get that data into IPA. The only way Designate pushes data out currently is via AXFR, which doesn't work for IPA to be a direct slave of Designate. What might work is to have an agent that gets the AXFR, then somehow converts that into IPA updates. This would only work if the volume of updates is fairly low. If Designate supported IXFR it would be much better. -- 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 0066] ipactl: Do not start/stop/restart single service multiple times
On 08/26/2015 03:16 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/5248 +def deduplicate(lst): +new_lst = [] +s = set(lst) +for i in lst: +if i in s: +s.remove(i) +new_lst.append(i) + +return new_lst + Imho, this method deserves a docstring or at least a comment. It is not entrirely clear from the name, that its job is to remove the duplicates while preserving the order of the entries. Anyway, deduplication can be implemented in a more readable way: from collections import OrderedDict sample_list = [3,2,1,2,1,5,3] OrderedDict.fromkeys(sample_list).keys() [3, 2, 1, 5] Tomas -- 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 0002] Port from python-krbV to python-gssapi
On 25.8.2015 21:00, Simo Sorce wrote:
On Tue, 2015-08-25 at 20:45 +0200, Michael Šimáček wrote:
On 2015-08-25 18:43, Robbie Harwood wrote:
Jan Cholasta jchol...@redhat.com writes:
On 25.8.2015 12:46, Michael Šimáček wrote:
On 2015-08-25 12:38, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Michael Šimáček wrote:
On 2015-08-24 20:29, Robbie Harwood wrote:
Michael Šimáček msima...@redhat.com writes:
On 2015-08-24 17:49, Simo Sorce wrote:
On Mon, 2015-08-24 at 17:18 +0200, Michael Šimáček wrote:
On 2015-08-24 14:50, Jan Cholasta wrote:
Fixed. python-gssapi has a display_as method that could pull the
name
from it, but it doesn't work in current version, therefore using
partition to split on '@'
It's actually a bug in MIT Krb5, as we noted in your bug[0]. So this:
-user = api.Command.user_show(unicode(principal[0]))['result']
+user =
api.Command.user_show(principal.partition('@')[0])['result']
is working around a bug in specific Kerberos versions. If people are
okay with merging such code, then I guess this is fine; I would
personally not do so because there is not a clear point at which it can
be removed. At the very least, we should wait until we see what
versions of krb5 MIT is going to fix.
Otherwise, looks good.
[0]: https://github.com/pythongssapi/python-gssapi/issues/79
python-krbV migration is blocking support for Python 3. The bug
doesn't have any fix upstream yet and there are two bugs actually, the
second one is in python-gssapi, which I've just reported [1]. Waiting
for two bugs to be fixed could be detrimental to py3 migration as we
don't have much time left. And I'm no longer sure that display_as
I don't buy this.
We have plenty of time for solving these bugs. Remember, that Samba
DCE RPC bindings aren't migrated to Python 3 either and will not be
before release of Samba 4.4. For Samba 4.3 it is simply too late.
So we are still far away from full Python3 migration for FreeIPA and
waiting for solving these two bugs is OK.
If fixing them solves anything at all. I planned to use
display_as(NameType.user), but when trying it on Name object with
name_type set (which doesn't trigger the segfault), it doesn't seem to
work either. I get:
gssapi.raw.exceptions.OperationUnavailableError: Major (1048576): The
operation or option is not available or unsupported, Minor (0): Unknown
error
Robbie, can you clarify whether display_as could be actually used to get
the first component of the principal reliably?
display_as should behave in accordance with its docs; anything else is a
bug report, which you filed. I don't know what you're asking me for
beyond that.
Why I mentioned display_as at all is that I initially assumed it could
be used for this, but it was only an assumption because I couldn't get
around the segfault. Later on, the cause of the segfault was found and I
was able to try the method and I found out that it probably cannot be
used for this purpose (i. e. extracting the first component of the
principal) regardless of the two bugs. How I thought it would be used:
import gssapi
cred = gssapi.Credentials()
user = cred.name.display_as(gssapi.NameType.user)
What I got:
gssapi.raw.exceptions.OperationUnavailableError: Major (1048576): The
operation or option is not available or unsupported, Minor (0): Unknown
error
This seems more like the method is not intended to be used this way. So
I'm asking you whether it is a bug or whether there is another way to do
it. Otherwise display_as cannot be used here.
As I have written in the other thread, we use principal.split('@') in
other parts of IPA, so principal.partition('@') should be OK as well.
This patch works for me, so ACK.
Unless there are any further objections, I would like to push it.
I think the newest iteration of this
user =
api.Command.user_show(principal.partition('@')[0].partition('/')[0])['result']
is even worse, but if it is decided to merge, then hopefully we can be
rid of it quickly.
It is splitting a string of known format in a way that is used in other
places of freeipa. What is specifically so bad about it? What do you
suggest as an alternative?
Given display_as() currently does not work for you go ahead with this
code. We'll revisit display_as later once we figure out more about the
bug that makes it fail.
OK.
Pushed to master: aad73fad601f576dd83b758f4448839b4e8e87df
--
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 0051] IPA server and replica installers can accept options from config file
On 30.7.2015 08:55, Jan Cholasta wrote: Dne 29.7.2015 v 17:43 Petr Vobornik napsal(a): On 07/29/2015 05:13 PM, Martin Babinsky wrote: On 07/29/2015 01:25 PM, Jan Cholasta wrote: Dne 29.7.2015 v 12:20 Martin Babinsky napsal(a): Initial attempt to implement https://fedorahosted.org/freeipa/ticket/4517 Some points to discuss: 1.) name of the config entries: currently the option names are derived from CLI options but have underscores in them instead of dashes. Maybe keeping the CLI option names also for config entries will make it easier for the user to transfer their CLI options from scripts to config files. NACK. There is no point in generating config names from CLI names, which are generated from knob names - use knob names directly. The problem is that in some cases the cli_name does not map directly to knob name, leading in different naming of CLI options and config entries, confusion and mayhem. What works for CLI may not work for config files and vice versa. For example, this works for CLI: --no-ntp --no-forwarders --forwarder 1.2.3.4 --forwarder 5.6.7.8 but this works better in config file: ntp = False forwarders = forwarders = 1.2.3.4, 5.6.7.8 Personally I would say that Honza's approach is more eye-candy but IMHO *not* more usable - and I prefer usability. Alexander's approach requires no other documentation that `ipa-server-install --help` or even better just copypasting arguments users already have in scripts to a file. In this case I believe that using anything incompatible with CLI arguments is not worth because it requires yet another layer of documentation to make it usable. BTW GnuPG does the very same thing as Alexander mentioned with .gnupg/gpg.conf, i.e. the config file simply accepts all options from command line, with the same names and parameters - and that that is it. Petr^2 Spacek These are some offenders from `ipaserver/install/server.py`: http://fpaste.org/249424/18226114/ On the other hand, this can be an incentive to finally put an end to inconsistent option/knob naming across server/replica/etc. installers. Yes please. If the names are different than cli names, then they should be made discoverable somehow or be documented. IMHO documenting them is easy. 2.) Config sections: there is currently only one valid section named '[global]' in accordance with the format of 'default.conf'. Should we have separate sections equivalent to option groups in CLI (e.g. [basic], [certificate system], [dns])? No, because they would have to be maintained forever. For example, some options are in wrong sections and we wouldn't be able to move them. I'm also more inclined to a single section, at least for now since we are pressed for time with this RFE. That's not to say that we should ditch Alexander's idea about separate sections with overrides for different hosts. We should consider it as a future enhancement to this feature once the basic plumbing is in place. Right. 3.) Handling of unattended mode when specifying a config file: Currently there is no connection between --config-file and unattended mode. So when you run ipa-server-install using config file, you still get asked for missing stuff. Should '--config-file' automatically imply '--unattended'? The behavior should be the same as if you specified the options on the command line. So no, --config-file should not imply --unattended. That sound reasonable. the code behaves this way already so no changes here. There are probably other issues to discuss. Feel free to write email/ping me on IRC. (I haven't looked at the patch yet.) Please take a look at it ASAP. I am on PTO tomorrow and on Friday, but I will find time to work at it in the evening if you send me you comments. 1) IMO the option should be in the top-level option section, not in a separate group (use parser.add_option()). Also maybe rename it to --config, AFAIK that's what is usually used. A short name (-c?) would be nice too. Nitpick: if the option is named --config-file, dest should be config_file, to make it easier to look it up in the code. 2) Please don't duplicate the knob retrieval code, store knobs in a list and pass that as an argument to parse_config_file. 3) I'm not sure about using newline as a list separator. I don't know about other IPA components, but SSSD in particular uses commas, maybe we should be consistent with that? 4) Booleans should be assignable either True or False, i.e. do not use _parse_knob to parse them. Honza -- 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 477] spec file: Add Requires(pre) on selinux-policy
On Wed, 26 Aug 2015, Jan Pazdziora wrote:
On Tue, Aug 25, 2015 at 03:50:04PM +0300, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
On 25.8.2015 14:23, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
+Requires(pre): selinux-policy = %{selinux_policy_version}
Requires: selinux-policy = %{selinux_policy_version}
If we have it in Requires(pre), we don't need it in Requires, as
Requires(pre) is a superset of guarantees that Requires gives you.
Martin (CCed) told me Requires(pre) does not imply Requires.
See http://rpm.org/api/4.4.2.2/tsort.html (available since 2007):
Since the only way out of a dependency loop is to snip the loop
somewhere, rpm uses hints from Requires: dependencies to distinguish
co-requisite (these are not needed to install, only to use, a package)
from pre-requisite (these are guaranteed to be installed before the
package that includes the dependency) relations.
However, this section seems to only apply to loop resolution. Note
that
http://www.rpm.org/wiki/PackagerDocs/MoreOnDependencies
says about Requires(pre)
* It ensures that the package providing /usr/sbin/useradd is
installed before this package. In presence of dependency
loops, scriptlet dependencies are the only way to ensure
correct install order.
* If there are no other dependencies on the package providing
/usr/sbin/useradd, that package is permitted to be removed
from the system after installation(!)
It's a fairly common mistake to replace legacy PreReq
dependencies with Requires(pre), but this is not the
same, due to the latter point above!
So I'd say that Requires(pre) does not imply Requires and if we only
do Requires(pre): selinux-policy = %{selinux_policy_version}, after
the installation, anybody can downgrade the selinux-policy package.
Heck, even in that ipa-server upgrading transaction, there could be
a selinux-policy downgrade operation, which would leave the newer
version for ipa-server's pre but install older version of
selinux-policy after it's done with ipa-server.
Yes, it's just a theoretical situation but we should not shortcut
Requires with Requires(pre), it might teach people reading the .spec
files bad habits.
Well, in that case having both Requires and Requires(post) is a
necessity, it seems.
--
/ Alexander Bokovoy
--
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 0051] IPA server and replica installers can accept options from config file
On 26.8.2015 10:51, Petr Spacek wrote:
On 30.7.2015 08:55, Jan Cholasta wrote:
Dne 29.7.2015 v 17:43 Petr Vobornik napsal(a):
On 07/29/2015 05:13 PM, Martin Babinsky wrote:
On 07/29/2015 01:25 PM, Jan Cholasta wrote:
Dne 29.7.2015 v 12:20 Martin Babinsky napsal(a):
Initial attempt to implement
https://fedorahosted.org/freeipa/ticket/4517
Some points to discuss:
1.) name of the config entries: currently the option names are derived
from CLI options but have underscores in them instead of dashes. Maybe
keeping the CLI option names also for config entries will make it
easier
for the user to transfer their CLI options from scripts to config
files.
NACK. There is no point in generating config names from CLI names, which
are generated from knob names - use knob names directly.
The problem is that in some cases the cli_name does not map directly to
knob name, leading in different naming of CLI options and config
entries, confusion and mayhem.
What works for CLI may not work for config files and vice versa. For example,
this works for CLI:
--no-ntp
--no-forwarders
--forwarder 1.2.3.4 --forwarder 5.6.7.8
but this works better in config file:
ntp = False
forwarders =
forwarders = 1.2.3.4, 5.6.7.8
Personally I would say that Honza's approach is more eye-candy but IMHO *not*
more usable - and I prefer usability. Alexander's approach requires no other
documentation that `ipa-server-install --help` or even better just
copypasting arguments users already have in scripts to a file.
In this case I believe that using anything incompatible with CLI arguments is
not worth because it requires yet another layer of documentation to make it
usable.
BTW GnuPG does the very same thing as Alexander mentioned with
.gnupg/gpg.conf, i.e. the config file simply accepts all options from command
line, with the same names and parameters - and that that is it.
Sorry, but no. The installers are supposed to be callable from many
different kinds of often incompatible environments. How exactly would
you imagine e.g. a Python API to look like given it should use the same
conventions as CLI? Like this:
server_install(['no_ntp', ('forwarder', '1.2.3.4'), ('forwarder',
'5.6.7.8')])
? I would very much prefer if it looked like this:
server_install(ntp=False, forwarders=['1.2.3.4', '5.6.7.8'])
which is pretty much the same I suggested for config files and better
reflects the semantics of setting configuration options.
Petr^2 Spacek
These are some offenders from `ipaserver/install/server.py`:
http://fpaste.org/249424/18226114/
On the other hand, this can be an incentive to finally put an end to
inconsistent option/knob naming across server/replica/etc. installers.
Yes please.
If the names are different than cli names, then they should be made
discoverable somehow or be documented.
IMHO documenting them is easy.
2.) Config sections: there is currently only one valid section named
'[global]' in accordance with the format of 'default.conf'. Should we
have separate sections equivalent to option groups in CLI (e.g.
[basic],
[certificate system], [dns])?
No, because they would have to be maintained forever. For example, some
options are in wrong sections and we wouldn't be able to move them.
I'm also more inclined to a single section, at least for now since we
are pressed for time with this RFE.
That's not to say that we should ditch Alexander's idea about separate
sections with overrides for different hosts. We should consider it as a
future enhancement to this feature once the basic plumbing is in place.
Right.
3.) Handling of unattended mode when specifying a config file:
Currently there is no connection between --config-file and unattended
mode. So when you run ipa-server-install using config file, you still
get asked for missing stuff. Should '--config-file' automatically imply
'--unattended'?
The behavior should be the same as if you specified the options on the
command line. So no, --config-file should not imply --unattended.
That sound reasonable. the code behaves this way already so no changes
here.
There are probably other issues to discuss. Feel free to write
email/ping me on IRC.
(I haven't looked at the patch yet.)
Please take a look at it ASAP. I am on PTO tomorrow and on Friday, but I
will find time to work at it in the evening if you send me you comments.
1) IMO the option should be in the top-level option section, not in a separate
group (use parser.add_option()).
Also maybe rename it to --config, AFAIK that's what is usually used.
A short name (-c?) would be nice too.
Nitpick: if the option is named --config-file, dest should be config_file,
to make it easier to look it up in the code.
2) Please don't duplicate the knob retrieval code, store knobs in a list and
pass that as an argument to parse_config_file.
3) I'm not sure about using newline as a list separator. I don't know about
other IPA components, but SSSD in
Re: [Freeipa-devel] dnssec tests fail due to KeyError at is_record_signed method
On 08/25/2015 08:59 PM, Oleg Fayans wrote: Hi Martin, As I was running the dnssec integration tests, I noticed that 4 out of 5 tests fail with the assumption of the dns zone being signed. Here is the stdout of the tests: http://pastebin.test.redhat.com/307944 The failure occurs in the is_record_signed method: it catches KeyError calling the ans.response.find_rrset method (line 37 at test_integration/test_dnssec.py). Could you take a look at it? Thanks I will take a look -- 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 477] spec file: Add Requires(pre) on selinux-policy
On Tue, Aug 25, 2015 at 10:22:42PM +0300, Alexander Bokovoy wrote: The flow above is not correct. Each scriptlet of the package is executed when package is installed. In particular, there is no period of waiting until end of whole transaction to start executing %posttrans scriptlet of a specific package. RPM only guarantees you that %posttrans scriptlet is executed as the last thing of this package intall, after all %post/%postun scriptlets were executed for this package and all triggers for affected packages were executed. This went against my undertanding of %posttrans, so I asked rpm maintainer and he confirmed that %posttrans scriptlets are executed as the last thing in the transaction. I might be misunderstanding what you say but I'd say that there actually is a period of waiting / postponing execution of %posttrans scriptlets of all packages until the end of the transaction. -- Jan Pazdziora Senior Principal Software Engineer, Identity Management Engineering, Red Hat -- 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 0301] Fix lint error related to python3 migration
On 08/26/2015 10:43 AM, Jan Cholasta wrote: On 26.8.2015 10:37, Martin Basti wrote: Master branch is broken, this patch fixes it. Patch attached. This is not a pylint error, but an actual bug. Sorry, I used wrong words, because It was detected by pylint. -- 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] 0696-0710 More modernization
On 25.8.2015 15:05, Petr Viktorin wrote: On 08/25/2015 12:39 PM, Christian Heimes wrote: On 2015-08-24 17:31, Petr Viktorin wrote: 0701.2-Use-Python3-compatible-dict-method-names NACK Why are you replacing iteritems() with items() instead of using six.iteritems()? It looks cleaner, and it will be easier to clean up after six is dropped. Also, the performance difference is negligible if the whole thing is iterated over. (On small dicts, which are the majority of what iteritems was used on, items() is actually a bit faster on my machine.) Right, for small dicts the speed difference is negligible and favors the items() over iteritems(). For medium sized and large dicts the iterators are faster and consume less memory. I'm preferring iterator methods everywhere because I don't have to worry about dict sizes. 0710.2-Modernize-use-of-range NACK Please use six.moves.range. It defaults to xrange() in Python 2. Why? What is the benefit of xrange in these situations? Like with iteritems in 0701, when iterating over the whole thing, the performance difference is negligible. I don't think a few microseconds outside of tight loops are worth the verbosity. It's the same reasoning as in 0701. As long as you have a small range, it doesn't make a difference. For large ranges the additional memory usage can be problematic. In all above cases the iterator methods and generator functions are a safer choice. A malicious user can abuse the non-iterative methods for DoS attacks. As long as the input can't be controlled by a user and the range/dict/set/list is small, the non-iterative methods are fine. You have to verify every location, though. Keep in mind that for dicts, all the data is in memory already (in the dict). Are you worried about DoS from an extra list of references to existing objects? I'm usually too busy with other stuff (aka lazy) to verify these preconditions... I changed one questionable use of range. The other ones are: ipalib/plugins/dns.py: for i in range(len(relative_record_name) (max. ~255, verified by DNSName) ipaserver/install/dnskeysyncinstance.py: for _ in range(0, 16)) (16) ipaserver/install/ipa_otptoken_import.py: for i in range(1, blocks + 1): (about 7) ipaserver/install/ipa_otptoken_import.py: for k in range(mac.digest_size): (16) ipatests/data.py: for d in range(256)) (256) Plus tests, where it's rarely above 10. I have just pushed Michael's python-krbV removal patch, which conflicts with your patches. Could you please rebase them on top of current master? -- 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 0301] Fix lint error related to python3 migration
Master branch is broken, this patch fixes it. Patch attached. From ab510bec2eb53b6edaff61bc37fcd8a9aea7648f Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Wed, 26 Aug 2015 10:34:25 +0200 Subject: [PATCH] Fix lint error cause by python3 migration --- ipalib/plugins/vault.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py index 483da5f0e9d2674c8d856543214d4baba7876a04..d06b63d68a27ee0522f636504188852570033a42 100644 --- a/ipalib/plugins/vault.py +++ b/ipalib/plugins/vault.py @@ -989,7 +989,7 @@ class vault_mod(PKQuery, Local): else: backend = self.api.Backend.rpcclient if not backend.isconnected(): -backend.connect(ccache=krbV.default_context().default_ccache()) +backend.connect() # determine the vault type based on parameters specified if vault_type: -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0301] Fix lint error related to python3 migration
On 2015-08-26 10:43, Jan Cholasta wrote: On 26.8.2015 10:37, Martin Basti wrote: Master branch is broken, this patch fixes it. Patch attached. This is not a pylint error, but an actual bug. Yes, commit e46d9236d19f714b67fdf2865f19146c3016f46d (yesterday evening) added another reference to krbV after my patch for migration from krbV was reviewed. This patch looks ok to me, I'd just change the commit message (to Remove leftover krbV reference or something like that). Thanks, Michael -- 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] 914 webui: add option to establish bidirectional trust
On 08/25/2015 05:19 PM, Petr Vobornik wrote: https://fedorahosted.org/freeipa/ticket/5259 ACK. -- 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] First part of integration tests for Topology Plugin
Hi Martin, On 08/20/2015 11:18 AM, Martin Basti wrote: On 08/20/2015 10:26 AM, Martin Basti wrote: On 08/19/2015 04:17 PM, Martin Basti wrote: I got this: https://paste.fedoraproject.org/256746/43999380/ FYI replica install failure. (I will retest it, but I'm pretty sure that it was clean VM, test for some reason install client first) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 295, in decorated func(installer) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 319, in install_check sys.exit(IPA client is already configured on this system.\n 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, exception: SystemExit: IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. 2015-08-19T14:14:15Z ERROR IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. Confirm I got same error. Fixed. It was a regex error. On 08/19/2015 09:00 AM, Oleg Fayans wrote: Hi Martin, As discussed, here is a new version with pep8-related fixes On 08/14/2015 10:44 AM, Oleg Fayans wrote: Hi Martin, Already noticed that. Implemented the named groups as Tomas advised. Added the third test for http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica On 08/13/2015 05:06 PM, Martin Basti wrote: On 08/11/2015 03:36 PM, Oleg Fayans wrote: Hi Martin, On 08/11/2015 02:02 PM, Martin Basti wrote: NACK, comments inline. On 11/08/15 13:25, Oleg Fayans wrote: Hi Martin, Thanks for the review! On 08/10/2015 07:08 PM, Martin Basti wrote: Thank you for patch, I have a few nitpicks: 1) On 10/08/15 13:05, Oleg Fayans wrote: +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) Why do you add the name of method in docstring? My bad, fixed. still there +tokenize_topologies(command_output) +takes an output of `ipa topologysegment-find` and returns an array of Fixed, sorry. 2) +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) +creates a topology segment. The first argument is a node to run the command on +The first 3 arguments should be objects of class Host +Returns a hash object containing segment's name, leftnode, rightnode information + I would prefer to add assert there instead of just document that a Host object is needed assert(isinstance(master, Host)) ... Fixed. Created a decorator that checks the type of arguments This does not scale well. If we will want to add new argument that is not host object, you need change it again. Agreed. Modified the decorator so that you can specify a slice of arguments to be checked and a class to compare to. This does scale :) This might be used as function with specified variables that have to be host objects 3) +def destroy_segment(master, segment_name): + +destroy_segment(master, segment_name) +Destroys topology segment. First argument should be object of class Host Instead of description of params as first, second etc., you may use following: +def destroy_segment(master, segment_name): + +Destroys topology segment. +:param master: reference to master object of class Host +:param segment: name fo segment and eventually this in other methods +:returns: Lorem ipsum sit dolor mit amet +:raises NotFound: if segment is not found Fixed 4) cls.replicas[:len(cls.replicas) - 1], I suggest cls.replicas[:-1] In [2]: a = [1, 2, 3, 4, 5] In [3]: a[:-1] Out[3]: [1, 2, 3, 4] Fixed 5) Why re.findall() and then you just use the first result? 'leftnode': self.leftnode_re.findall(i)[0] Isn't just re.search() enough? leftnode_re.search(value).group(1) in fact leftnode_re.findall(string)[0] and leftnode_re.search(string).group(1), Are equally bad from the readability point of view. The first one is even shorter a bit, so why change? :) It depends on point of view, because when I reviewed it yesterday my brain raises exception that you are trying to add multiple values to single value attribute, because you used findall, I expected that you need multiple values, and then I saw that index [0] at the end, and I was pretty confused what are you trying to achieve. And because findall is not effective in case when you need to find just one occurrence. I got it. Fixed. Python 3 nitpick: I'm not sure if time when we should enforce python 2/3 compability already comes, but just for record: instead of open(file, 'r'), please use io.open(file, 'r') (import io before) Done. 1) +# empty comment here (several times) Removed NACK you changed it wrong
Re: [Freeipa-devel] [PATCH 477] spec file: Add Requires(pre) on selinux-policy
On Tue, Aug 25, 2015 at 02:18:29PM +0200, Jan Cholasta wrote:
Hi,
the attached patch fixes https://fedorahosted.org/freeipa/ticket/5256.
Honza
--
Jan Cholasta
From 216be8de30747f80f490d4e91a7cca4af3e767d6 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Tue, 25 Aug 2015 14:14:25 +0200
Subject: [PATCH] spec file: Add Requires(pre) on selinux-policy
This prevents ipa-server-upgrade failures on SELinux AVCs because of old
selinux-policy version.
https://fedorahosted.org/freeipa/ticket/5256
---
freeipa.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index cba91fe..fd73cda 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -139,6 +139,7 @@ Requires: systemd-units = 38
Requires(pre): shadow-utils
Requires(pre): systemd-units
Requires(post): systemd-units
+Requires(pre): selinux-policy = %{selinux_policy_version}
What is the core issue with
https://fedorahosted.org/freeipa/ticket/5256
? I undestand that we need new selinux-policy, but what does that
policy change?
I ask because if it's about labelling of files installed by rpm, the
(pre) might not help because rpm did not reload the file contexts
mid-transaction
https://bugzilla.redhat.com/show_bug.cgi?id=505066#c9
and I'm not sure things have changed since RHEL 5.
--
Jan Pazdziora
Senior Principal Software Engineer, Identity Management Engineering, Red Hat
--
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 0301] Fix lint error related to python3 migration
On 26.8.2015 10:37, Martin Basti wrote: Master branch is broken, this patch fixes it. Patch attached. This is not a pylint error, but an actual bug. -- 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] 915 vault: change default vault type to symmetric
On 08/25/2015 07:21 PM, Petr Vobornik wrote: On 08/25/2015 06:29 PM, Petr Vobornik wrote: https://fedorahosted.org/freeipa/ticket/5251 Attaching new rebased version with help text amended. ACK Pushed to: master: 19dd2ed758210e859a5b0085de558cf13ba09104 ipa-4-2: e247babc1ad211f7c939029cfb57eb6f4fbd79ab -- 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] First part of integration tests for Topology Plugin
On 08/26/2015 11:44 AM, Oleg Fayans wrote: Hi Martin, On 08/20/2015 11:18 AM, Martin Basti wrote: On 08/20/2015 10:26 AM, Martin Basti wrote: On 08/19/2015 04:17 PM, Martin Basti wrote: I got this: https://paste.fedoraproject.org/256746/43999380/ FYI replica install failure. (I will retest it, but I'm pretty sure that it was clean VM, test for some reason install client first) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 295, in decorated func(installer) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 319, in install_check sys.exit(IPA client is already configured on this system.\n 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, exception: SystemExit: IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. 2015-08-19T14:14:15Z ERROR IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. Confirm I got same error. Fixed. It was a regex error. On 08/19/2015 09:00 AM, Oleg Fayans wrote: Hi Martin, As discussed, here is a new version with pep8-related fixes On 08/14/2015 10:44 AM, Oleg Fayans wrote: Hi Martin, Already noticed that. Implemented the named groups as Tomas advised. Added the third test for http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica On 08/13/2015 05:06 PM, Martin Basti wrote: On 08/11/2015 03:36 PM, Oleg Fayans wrote: Hi Martin, On 08/11/2015 02:02 PM, Martin Basti wrote: NACK, comments inline. On 11/08/15 13:25, Oleg Fayans wrote: Hi Martin, Thanks for the review! On 08/10/2015 07:08 PM, Martin Basti wrote: Thank you for patch, I have a few nitpicks: 1) On 10/08/15 13:05, Oleg Fayans wrote: +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) Why do you add the name of method in docstring? My bad, fixed. still there +tokenize_topologies(command_output) +takes an output of `ipa topologysegment-find` and returns an array of Fixed, sorry. 2) +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) +creates a topology segment. The first argument is a node to run the command on +The first 3 arguments should be objects of class Host +Returns a hash object containing segment's name, leftnode, rightnode information + I would prefer to add assert there instead of just document that a Host object is needed assert(isinstance(master, Host)) ... Fixed. Created a decorator that checks the type of arguments This does not scale well. If we will want to add new argument that is not host object, you need change it again. Agreed. Modified the decorator so that you can specify a slice of arguments to be checked and a class to compare to. This does scale :) This might be used as function with specified variables that have to be host objects 3) +def destroy_segment(master, segment_name): + +destroy_segment(master, segment_name) +Destroys topology segment. First argument should be object of class Host Instead of description of params as first, second etc., you may use following: +def destroy_segment(master, segment_name): + +Destroys topology segment. +:param master: reference to master object of class Host +:param segment: name fo segment and eventually this in other methods +:returns: Lorem ipsum sit dolor mit amet +:raises NotFound: if segment is not found Fixed 4) cls.replicas[:len(cls.replicas) - 1], I suggest cls.replicas[:-1] In [2]: a = [1, 2, 3, 4, 5] In [3]: a[:-1] Out[3]: [1, 2, 3, 4] Fixed 5) Why re.findall() and then you just use the first result? 'leftnode': self.leftnode_re.findall(i)[0] Isn't just re.search() enough? leftnode_re.search(value).group(1) in fact leftnode_re.findall(string)[0] and leftnode_re.search(string).group(1), Are equally bad from the readability point of view. The first one is even shorter a bit, so why change? :) It depends on point of view, because when I reviewed it yesterday my brain raises exception that you are trying to add multiple values to single value attribute, because you used findall, I expected that you need multiple values, and then I saw that index [0] at the end, and I was pretty confused what are you trying to achieve. And because findall is not effective in case when you need to find just one occurrence. I got it. Fixed. Python 3 nitpick: I'm not sure if time when we should enforce python 2/3 compability already comes, but just for record: instead of open(file, 'r'),
Re: [Freeipa-devel] [PATCH 0051] IPA server and replica installers can accept options from config file
On 26.8.2015 11:48, Jan Cholasta wrote:
On 26.8.2015 10:51, Petr Spacek wrote:
On 30.7.2015 08:55, Jan Cholasta wrote:
Dne 29.7.2015 v 17:43 Petr Vobornik napsal(a):
On 07/29/2015 05:13 PM, Martin Babinsky wrote:
On 07/29/2015 01:25 PM, Jan Cholasta wrote:
Dne 29.7.2015 v 12:20 Martin Babinsky napsal(a):
Initial attempt to implement
https://fedorahosted.org/freeipa/ticket/4517
Some points to discuss:
1.) name of the config entries: currently the option names are derived
from CLI options but have underscores in them instead of dashes. Maybe
keeping the CLI option names also for config entries will make it
easier
for the user to transfer their CLI options from scripts to config
files.
NACK. There is no point in generating config names from CLI names, which
are generated from knob names - use knob names directly.
The problem is that in some cases the cli_name does not map directly to
knob name, leading in different naming of CLI options and config
entries, confusion and mayhem.
What works for CLI may not work for config files and vice versa. For
example,
this works for CLI:
--no-ntp
--no-forwarders
--forwarder 1.2.3.4 --forwarder 5.6.7.8
but this works better in config file:
ntp = False
forwarders =
forwarders = 1.2.3.4, 5.6.7.8
Personally I would say that Honza's approach is more eye-candy but IMHO *not*
more usable - and I prefer usability. Alexander's approach requires no other
documentation that `ipa-server-install --help` or even better just
copypasting arguments users already have in scripts to a file.
In this case I believe that using anything incompatible with CLI arguments is
not worth because it requires yet another layer of documentation to make it
usable.
BTW GnuPG does the very same thing as Alexander mentioned with
.gnupg/gpg.conf, i.e. the config file simply accepts all options from command
line, with the same names and parameters - and that that is it.
Sorry, but no. The installers are supposed to be callable from many different
kinds of often incompatible environments. How exactly would you imagine e.g. a
Python API to look like given it should use the same conventions as CLI? Like
this:
server_install(['no_ntp', ('forwarder', '1.2.3.4'), ('forwarder',
'5.6.7.8')])
? I would very much prefer if it looked like this:
server_install(ntp=False, forwarders=['1.2.3.4', '5.6.7.8'])
which is pretty much the same I suggested for config files and better reflects
the semantics of setting configuration options.
I'm just saying that:
1. API user-interface on CLI are not the same, so there is no need to
strictly use the same names in API and CLI (which we apparently do not do,
compare --help and internal knobs).
2. User interface self-consistency (CLI options vs. configuration file) is
more important that consistency between config file and API.
Petr^2 Spacek
These are some offenders from `ipaserver/install/server.py`:
http://fpaste.org/249424/18226114/
On the other hand, this can be an incentive to finally put an end to
inconsistent option/knob naming across server/replica/etc. installers.
Yes please.
If the names are different than cli names, then they should be made
discoverable somehow or be documented.
IMHO documenting them is easy.
2.) Config sections: there is currently only one valid section named
'[global]' in accordance with the format of 'default.conf'. Should we
have separate sections equivalent to option groups in CLI (e.g.
[basic],
[certificate system], [dns])?
No, because they would have to be maintained forever. For example, some
options are in wrong sections and we wouldn't be able to move them.
I'm also more inclined to a single section, at least for now since we
are pressed for time with this RFE.
That's not to say that we should ditch Alexander's idea about separate
sections with overrides for different hosts. We should consider it as a
future enhancement to this feature once the basic plumbing is in place.
Right.
3.) Handling of unattended mode when specifying a config file:
Currently there is no connection between --config-file and unattended
mode. So when you run ipa-server-install using config file, you still
get asked for missing stuff. Should '--config-file' automatically imply
'--unattended'?
The behavior should be the same as if you specified the options on the
command line. So no, --config-file should not imply --unattended.
That sound reasonable. the code behaves this way already so no changes
here.
There are probably other issues to discuss. Feel free to write
email/ping me on IRC.
(I haven't looked at the patch yet.)
Please take a look at it ASAP. I am on PTO tomorrow and on Friday, but I
will find time to work at it in the evening if you send me you comments.
1) IMO the option should be in the top-level option section, not in a
separate
group (use parser.add_option()).
Also maybe rename it to
Re: [Freeipa-devel] [PATCH 0301] Fix: Remove leftover krbV reference
On 08/26/2015 11:19 AM, Michael Šimáček wrote: On 2015-08-26 10:43, Jan Cholasta wrote: On 26.8.2015 10:37, Martin Basti wrote: Master branch is broken, this patch fixes it. Patch attached. This is not a pylint error, but an actual bug. Yes, commit e46d9236d19f714b67fdf2865f19146c3016f46d (yesterday evening) added another reference to krbV after my patch for migration from krbV was reviewed. This patch looks ok to me, I'd just change the commit message (to Remove leftover krbV reference or something like that). Thanks, Michael Fixed. updated patch attached. From a5733f8ff5623ef10b4c740226fd5834fab1a6db Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Wed, 26 Aug 2015 10:34:25 +0200 Subject: [PATCH] Fix: Remove leftover krbV reference --- ipalib/plugins/vault.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py index 483da5f0e9d2674c8d856543214d4baba7876a04..d06b63d68a27ee0522f636504188852570033a42 100644 --- a/ipalib/plugins/vault.py +++ b/ipalib/plugins/vault.py @@ -989,7 +989,7 @@ class vault_mod(PKQuery, Local): else: backend = self.api.Backend.rpcclient if not backend.isconnected(): -backend.connect(ccache=krbV.default_context().default_ccache()) +backend.connect() # determine the vault type based on parameters specified if vault_type: -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0051] IPA server and replica installers can accept options from config file
On 26.8.2015 12:00, Petr Spacek wrote:
On 26.8.2015 11:48, Jan Cholasta wrote:
On 26.8.2015 10:51, Petr Spacek wrote:
On 30.7.2015 08:55, Jan Cholasta wrote:
Dne 29.7.2015 v 17:43 Petr Vobornik napsal(a):
On 07/29/2015 05:13 PM, Martin Babinsky wrote:
On 07/29/2015 01:25 PM, Jan Cholasta wrote:
Dne 29.7.2015 v 12:20 Martin Babinsky napsal(a):
Initial attempt to implement
https://fedorahosted.org/freeipa/ticket/4517
Some points to discuss:
1.) name of the config entries: currently the option names are derived
from CLI options but have underscores in them instead of dashes. Maybe
keeping the CLI option names also for config entries will make it
easier
for the user to transfer their CLI options from scripts to config
files.
NACK. There is no point in generating config names from CLI names, which
are generated from knob names - use knob names directly.
The problem is that in some cases the cli_name does not map directly to
knob name, leading in different naming of CLI options and config
entries, confusion and mayhem.
What works for CLI may not work for config files and vice versa. For example,
this works for CLI:
--no-ntp
--no-forwarders
--forwarder 1.2.3.4 --forwarder 5.6.7.8
but this works better in config file:
ntp = False
forwarders =
forwarders = 1.2.3.4, 5.6.7.8
Personally I would say that Honza's approach is more eye-candy but IMHO *not*
more usable - and I prefer usability. Alexander's approach requires no other
documentation that `ipa-server-install --help` or even better just
copypasting arguments users already have in scripts to a file.
In this case I believe that using anything incompatible with CLI arguments is
not worth because it requires yet another layer of documentation to make it
usable.
BTW GnuPG does the very same thing as Alexander mentioned with
.gnupg/gpg.conf, i.e. the config file simply accepts all options from command
line, with the same names and parameters - and that that is it.
Sorry, but no. The installers are supposed to be callable from many different
kinds of often incompatible environments. How exactly would you imagine e.g. a
Python API to look like given it should use the same conventions as CLI? Like
this:
server_install(['no_ntp', ('forwarder', '1.2.3.4'), ('forwarder',
'5.6.7.8')])
? I would very much prefer if it looked like this:
server_install(ntp=False, forwarders=['1.2.3.4', '5.6.7.8'])
which is pretty much the same I suggested for config files and better reflects
the semantics of setting configuration options.
I'm just saying that:
1. API user-interface on CLI are not the same, so there is no need to
strictly use the same names in API and CLI (which we apparently do not do,
compare --help and internal knobs).
2. User interface self-consistency (CLI options vs. configuration file) is
more important that consistency between config file and API.
User interface is not necessarily only CLI and config files and I would
prefer not to mutilate the user interface in general with CLI specifics.
If you want 100% CLI compatibility you can do the following and don't
bother with any new code in IPA at all:
$ echo --no-ntp options
$ echo --forwarder 1.2.3.4 options
$ echo --forwarder 5.6.7.8 options
$ ipa-server-install $(cat options)
Interface consistency is important in any case, and providing it in one
place just to sacrifice it in other place does not really improve anything.
Petr^2 Spacek
These are some offenders from `ipaserver/install/server.py`:
http://fpaste.org/249424/18226114/
On the other hand, this can be an incentive to finally put an end to
inconsistent option/knob naming across server/replica/etc. installers.
Yes please.
If the names are different than cli names, then they should be made
discoverable somehow or be documented.
IMHO documenting them is easy.
2.) Config sections: there is currently only one valid section named
'[global]' in accordance with the format of 'default.conf'. Should we
have separate sections equivalent to option groups in CLI (e.g.
[basic],
[certificate system], [dns])?
No, because they would have to be maintained forever. For example, some
options are in wrong sections and we wouldn't be able to move them.
I'm also more inclined to a single section, at least for now since we
are pressed for time with this RFE.
That's not to say that we should ditch Alexander's idea about separate
sections with overrides for different hosts. We should consider it as a
future enhancement to this feature once the basic plumbing is in place.
Right.
3.) Handling of unattended mode when specifying a config file:
Currently there is no connection between --config-file and unattended
mode. So when you run ipa-server-install using config file, you still
get asked for missing stuff. Should '--config-file' automatically imply
'--unattended'?
The behavior should be the same as if you specified the options on the
command line. So no,
Re: [Freeipa-devel] [PATCH] 913 fix missing information in object metadata
On 08/25/2015 04:54 PM, Petr Vobornik wrote: Missing 'required' values in takes_params causes Web UI to treat required fields as optional. Regression caused by ba0a1c6b33e2519a48754602413c8379fb1f0ff1 https://fedorahosted.org/freeipa/ticket/5258 ACK Pushed to: master: d01f18d4417e3073f31981dedfc8a200b3a42eb9 ipa-4-2: 42e8ab8c39c24fa9dca2d7a6dc4f75b7ae6e8b9a -- 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] 914 webui: add option to establish bidirectional trust
On 08/26/2015 11:28 AM, Tomas Babej wrote: On 08/25/2015 05:19 PM, Petr Vobornik wrote: https://fedorahosted.org/freeipa/ticket/5259 ACK. Pushed to: master: d7b096486e05defc1de2cc3c9f5582061b8e4060 ipa-4-2: b1f1dcaab3c2b4799ef12a417a9998d7556496af -- 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] kra an ca instance installation
On Wed, 2015-08-26 at 09:06 +0200, Jan Cholasta wrote:
On 25.8.2015 17:50, Simo Sorce wrote:
On Tue, 2015-08-25 at 16:22 +0200, Jan Cholasta wrote:
On 25.8.2015 16:09, Simo Sorce wrote:
On Tue, 2015-08-25 at 15:52 +0200, Jan Cholasta wrote:
On 25.8.2015 15:37, Simo Sorce wrote:
On Tue, 2015-08-25 at 11:34 +0200, Jan Cholasta wrote:
On 25.8.2015 11:25, Jan Cholasta wrote:
On 24.8.2015 19:51, Simo Sorce wrote:
Why do we have cainstance.py and ca.py and krainstance.py and kra.py
in
ipaserver/install when you always need both files to do anything
around
installation of the ca ?
Is there a motivation ?
Or can I simply provide a patch to remove the ca.py and kra.py files
an
unify all code in the proper *instance.py file ?
ca.py and kra.py are the proper files ready to be migrated to the new
install framework, cainstance.py and krainstance.py will be removed.
... once the migration is done. (Hit send button too fast.)
The motivation is that *instance.py do not provide a uniform interface,
have a lot of redundant and duplicate stuff and are generally unfit for
any further extension.
I have been changing only the instance files, so we are going in
different directions.
I don't see how, {ca,kra}instance.py code is called from {ca,kra}.py.
{ca,kra}.py contains all the code that is actually needed to install
CA/KRA that is not in {ca,kra}instance.py and was previously scattered
around ipa-{server,replica,ca,kra}-install.
I do not really care what file we are going into, but there is a lot of
code in the installer now that does not tell the user a step is being
done, while instances do that through the step interface.
This code was always there, we only moved it to a single location. See
git history.
The step interface is also a very good way to let someone that read the
code see what is going on and follow each step.
Are you proposing to stop going through the instances steps ? I found
the current way kra and ca installation is setup basically a regression,
it took me a *lot* longer that it should be needed to follow through all
the steps that are really taken.
Again, we only moved the code around, and I don't think we created any
regressions.
Ok, so the plan is just to move the CAInstance class and code *as is*
from cainstance.py to ca.py ?I guess I am confused about what is your
plan around this exactly.
The code in CAInstance will be gradually migrated to a new class in
ca.py which uses the new install framework. We started with the
top-level ipa-server-install and ipa-replica-install code, which is
still not done, but you can see it for reference on how it will look
like (ipaserver/install/server/*.py, especially the classes at the
bottom of the files).
Well I had to modify server/replicainstall.py quite radically, I had to
create a new set of promote_check and promote functions there. So we are
back to duplicated code, sorry (no way around it).
It's OK, there is still a lot of duplicate code in server/replica
install anyway, we deduplicated only the CA, KRA and DNS install code so
far.
However the functions in server/replicainstall.py still use the
instances mostly for all components *except* for kra and ca where there
is really confusing code calling unnecessarily instances multiple times
and fooling around with stuff.
If you think there are unnecessary calls I think you are reading the
code wrong.
I do not really understand what you mean
by migrating from CAInstance to what's in server/*install.py because in
there all instances are used for the individual components, so I am now
scratching my head.
What I mean is that the code to install a component will be wrapped in a
class similar to Server or Replica classes in these files. There will
still be steps like in CAInstance, but the rest will be different. I
don't think you have seen the PoC from 5 months ago:
https://www.redhat.com/archives/freeipa-devel/2015-March/msg00374.html.
I had not seen this, thanks.
The code in ca.py especially is really confusing, I rewrote it once to
eliminate duplications then decided to simply not touch it in my branch
(and threw away the modifications) because it is too confusing and I did
not want to risk regressions. So I created a simple create_replica()
function in the CA instance that does all that is needed.
Before ca.py, you would have to rewrite the same confusing code in
ipa-server-install, ipa-replica-install and ipa-ca-install. I don't see
how that's better.
I guess I will just keep ignoring the confusion and try to come up with
something that works but I'd really like to understand what is the
endgame there. If you want to replace instances (why?), what will you
replace it with ?
With something that does not mix install with service management,
maximizes code reuse and makes related code colocated,
Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
Hi, No more short links :) On 08/26/2015 11:50 AM, Tomas Babej wrote: On 08/26/2015 11:44 AM, Oleg Fayans wrote: Hi Martin, On 08/20/2015 11:18 AM, Martin Basti wrote: On 08/20/2015 10:26 AM, Martin Basti wrote: On 08/19/2015 04:17 PM, Martin Basti wrote: I got this: https://paste.fedoraproject.org/256746/43999380/ FYI replica install failure. (I will retest it, but I'm pretty sure that it was clean VM, test for some reason install client first) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 295, in decorated func(installer) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 319, in install_check sys.exit(IPA client is already configured on this system.\n 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, exception: SystemExit: IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. 2015-08-19T14:14:15Z ERROR IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. Confirm I got same error. Fixed. It was a regex error. On 08/19/2015 09:00 AM, Oleg Fayans wrote: Hi Martin, As discussed, here is a new version with pep8-related fixes On 08/14/2015 10:44 AM, Oleg Fayans wrote: Hi Martin, Already noticed that. Implemented the named groups as Tomas advised. Added the third test for http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica On 08/13/2015 05:06 PM, Martin Basti wrote: On 08/11/2015 03:36 PM, Oleg Fayans wrote: Hi Martin, On 08/11/2015 02:02 PM, Martin Basti wrote: NACK, comments inline. On 11/08/15 13:25, Oleg Fayans wrote: Hi Martin, Thanks for the review! On 08/10/2015 07:08 PM, Martin Basti wrote: Thank you for patch, I have a few nitpicks: 1) On 10/08/15 13:05, Oleg Fayans wrote: +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) Why do you add the name of method in docstring? My bad, fixed. still there +tokenize_topologies(command_output) +takes an output of `ipa topologysegment-find` and returns an array of Fixed, sorry. 2) +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) +creates a topology segment. The first argument is a node to run the command on +The first 3 arguments should be objects of class Host +Returns a hash object containing segment's name, leftnode, rightnode information + I would prefer to add assert there instead of just document that a Host object is needed assert(isinstance(master, Host)) ... Fixed. Created a decorator that checks the type of arguments This does not scale well. If we will want to add new argument that is not host object, you need change it again. Agreed. Modified the decorator so that you can specify a slice of arguments to be checked and a class to compare to. This does scale :) This might be used as function with specified variables that have to be host objects 3) +def destroy_segment(master, segment_name): + +destroy_segment(master, segment_name) +Destroys topology segment. First argument should be object of class Host Instead of description of params as first, second etc., you may use following: +def destroy_segment(master, segment_name): + +Destroys topology segment. +:param master: reference to master object of class Host +:param segment: name fo segment and eventually this in other methods +:returns: Lorem ipsum sit dolor mit amet +:raises NotFound: if segment is not found Fixed 4) cls.replicas[:len(cls.replicas) - 1], I suggest cls.replicas[:-1] In [2]: a = [1, 2, 3, 4, 5] In [3]: a[:-1] Out[3]: [1, 2, 3, 4] Fixed 5) Why re.findall() and then you just use the first result? 'leftnode': self.leftnode_re.findall(i)[0] Isn't just re.search() enough? leftnode_re.search(value).group(1) in fact leftnode_re.findall(string)[0] and leftnode_re.search(string).group(1), Are equally bad from the readability point of view. The first one is even shorter a bit, so why change? :) It depends on point of view, because when I reviewed it yesterday my brain raises exception that you are trying to add multiple values to single value attribute, because you used findall, I expected that you need multiple values, and then I saw that index [0] at the end, and I was pretty confused what are you trying to achieve. And because findall is not effective in case when you need to find just one occurrence. I got it. Fixed. Python 3 nitpick: I'm not sure if time when we should enforce python 2/3 compability already comes, but just for record: instead of open(file, 'r'), please use io.open(file,
Re: [Freeipa-devel] [PATCH] 916 vault: add vault container commands
On 08/25/2015 08:04 PM, Petr Vobornik wrote:
adds commands:
* vaultcontainer-show [--service service|--user user ]
* vaultcontainer-add-owner
[--service service|--user user ]
[--users users] [--groups groups] [--services services]
* vaultcontainer-remove-owner
[--service service|--user user ]
[--users users] [--groups groups] [--services services]
https://fedorahosted.org/freeipa/ticket/5250
Use cases:
1. When user/service is deleted, associated vault container looses
owner. There was no API command to set the owner.
2. Change owner of container by admin to manage access.
Show command was added to show current owners.
Find command was not added, should it be?
There is also a design for vault container ownership handling created by
Endi - it's for future Vault 2.0.
http://www.freeipa.org/page/V4/Password_Vault_2.0#Adding_container_owner
This patch has a different API than the proposed - different way of
specifying the container. The design page uses path e.g. /users/foobar.
This patch uses the same way as vaults e.g. --user=foobar. This means
that the implementation in this patch cannot manage ownership of parent
vault containers e.g. cn=users,cn=vaults,cn=kra,$SUFFIX.
Do we want to go with this approach in 4.2?
Attaching also new path which removes setting of owner which doesn't
exist so that integrity is OK and that it is consistent with removing of
user.
Updated patch attached - output fix.
--
Petr Vobornik
From 7ef2a5580f06103d803e2ec2e85315a40d5e8666 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 26 Aug 2015 13:00:05 +0200
Subject: [PATCH] vault: set vaultcontainer owner only if exists
To be consistent with situation when owner(e.g. user) is deleted.
https://fedorahosted.org/freeipa/ticket/5250
---
ipalib/plugins/vault.py | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index 0026ac11ed79c5aaf8de7d8ad13778284d00496b..da1a58cfb77932e8a907725eb88f9f5c6df023c9 100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -422,6 +422,12 @@ class vault(LDAPObject):
entries = []
+# check that owner exists
+try:
+self.backend.get_entry(owner_dn, [])
+except errors.NotFound:
+owner_dn = None
+
while dn:
assert dn.endswith(container_dn)
@@ -431,9 +437,11 @@ class vault(LDAPObject):
{
'objectclass': ['ipaVaultContainer'],
'cn': rdn['cn'],
-'owner': [owner_dn],
})
+if owner_dn:
+entry['owner'] = [owner_dn]
+
# if entry can be added, return
try:
self.backend.add_entry(entry)
--
2.4.3
From 846851d30caaf3da56b5f97f7023f147c34515da Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Tue, 25 Aug 2015 19:56:00 +0200
Subject: [PATCH] vault: add vault container commands
adds commands:
* vaultcontainer-show [--service service|--user user ]
* vaultcontainer-add-owner
[--service service|--user user ]
[--users users] [--groups groups] [--services services]
* vaultcontainer-remove-owner
[--service service|--user user ]
[--users users] [--groups groups] [--services services]
https://fedorahosted.org/freeipa/ticket/5250
---
API.txt | 40 +++
VERSION | 4 +-
ipalib/plugins/vault.py | 180 ++--
3 files changed, 216 insertions(+), 8 deletions(-)
diff --git a/API.txt b/API.txt
index afd5017bee2bc1eed54497ccd504b92619ff7a58..c45d332528b0cf5aa6b125b9c58cd3b3b8c970dc 100644
--- a/API.txt
+++ b/API.txt
@@ -5668,6 +5668,46 @@ option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
+command: vaultcontainer_add_owner
+args: 0,9,3
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('group*', alwaysask=True, cli_name='groups', csv=True)
+option: Flag('no_members', autofill=True, default=False, exclude='webui')
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('service?')
+option: Str('services', alwaysask=True, cli_name='services', csv=True, multivalue=True, required=False)
+option: Str('user*', alwaysask=True, cli_name='users', csv=True)
+option: Str('username?', cli_name='user')
+option: Str('version?', exclude='webui')
+output: Output('completed', type 'int', None)
+output: Output('failed', type 'dict', None)
+output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+command: vaultcontainer_remove_owner
+args: 0,9,3
+option:
[Freeipa-devel] [PATCH] 919 vault: fix vault tests after default type change
vault test should no longer hang on interactive prompt. Doesn't fix
other issues in vault tests.
https://fedorahosted.org/freeipa/ticket/5251
--
Petr Vobornik
From f288a9bd33a72a86a50bbe4a990b0a2e30d1599f Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 26 Aug 2015 13:03:22 +0200
Subject: [PATCH] vault: fix vault tests after default type change
https://fedorahosted.org/freeipa/ticket/5251
---
ipatests/test_xmlrpc/test_vault_plugin.py | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/ipatests/test_xmlrpc/test_vault_plugin.py b/ipatests/test_xmlrpc/test_vault_plugin.py
index 18032e287668e57b84e3fb528f02b98ebf0c90ab..495512dac687afaee0c94c620c2b504df424c246 100644
--- a/ipatests/test_xmlrpc/test_vault_plugin.py
+++ b/ipatests/test_xmlrpc/test_vault_plugin.py
@@ -156,7 +156,9 @@ class test_vault_plugin(Declarative):
'command': (
'vault_add',
[vault_name],
-{},
+{
+'ipavaulttype': u'standard',
+},
),
'expected': {
'value': vault_name,
@@ -257,6 +259,7 @@ class test_vault_plugin(Declarative):
'vault_add',
[vault_name],
{
+'ipavaulttype': u'standard',
'service': service_name,
},
),
@@ -366,6 +369,7 @@ class test_vault_plugin(Declarative):
'vault_add',
[vault_name],
{
+'ipavaulttype': u'standard',
'shared': True
},
),
@@ -475,6 +479,7 @@ class test_vault_plugin(Declarative):
'vault_add',
[vault_name],
{
+'ipavaulttype': u'standard',
'username': user_name,
},
),
@@ -583,7 +588,9 @@ class test_vault_plugin(Declarative):
'command': (
'vault_add',
[standard_vault_name],
-{},
+{
+'ipavaulttype': u'standard',
+},
),
'expected': {
'value': standard_vault_name,
--
2.4.3
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 919 vault: fix vault tests after default type change
On 08/26/2015 01:24 PM, Petr Vobornik wrote: vault test should no longer hang on interactive prompt. Doesn't fix other issues in vault tests. https://fedorahosted.org/freeipa/ticket/5251 ACK Pushed to: master: 9b0a01930bcefda1f37d7de147fed0856c28296f ipa-4-2: 91de475fd9d4499c05052e74bd2918569da4f269 -- 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] 0040 certprofile: prevent rename (modrdn)
On 08/25/2015 04:19 PM, Simo Sorce wrote:
On Tue, 2015-08-25 at 21:49 +1000, Fraser Tweedale wrote:
On Tue, Aug 25, 2015 at 01:39:42PM +0300, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Petr Vobornik wrote:
On 08/25/2015 07:37 AM, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Fraser Tweedale wrote:
The attached patch fixes
https://fedorahosted.org/freeipa/ticket/5247.
Thanks,
Fraser
From 2cb4ab6eeedccc3471ed9bf983add4687ecd5c1a Mon Sep 17 00:00:00 2001
From: Fraser Tweedale ftwee...@redhat.com
Date: Mon, 24 Aug 2015 20:25:10 -0400
Subject: [PATCH] certprofile: prevent rename (modrdn)
Fixes: https://fedorahosted.org/freeipa/ticket/5247
---
ipalib/plugins/certprofile.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/ipalib/plugins/certprofile.py
b/ipalib/plugins/certprofile.py
index
007cc543406b7e5705fd7474f3685cd6a9ce6aca..a0ffa38608400860994c771e4eba81304ead27be
100644
--- a/ipalib/plugins/certprofile.py
+++ b/ipalib/plugins/certprofile.py
@@ -323,8 +323,9 @@ class certprofile_mod(LDAPUpdate):
def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys,
**options):
ca_enabled_check()
# Once a profile id is set it cannot be changed
-if 'cn' in entry_attrs:
-raise errors.ACIError(info=_('cn is immutable'))
+if 'rename' in options or 'cn' in entry_attrs:
+raise errors.ProtectedEntryError(label='certprofile',
key=keys[0],
+reason=_('Certificate profiles cannot be renamed'))
if 'file' in options:
with self.api.Backend.ra_certprofile as profile_api:
profile_api.disable_profile(keys[0])
ACK
can't we fix it by removing `rdn_is_primary_key = True`?
That would also remove the --rename option. Yes it's an API change but if
rename is forbidden than the option should not be even there, just the
result error will different.
Well, that is another option, yes. Perhaps even a better one -- we have
plenty of places where rdn_is_primary_key is not actually used.
I filed a ticket for this: https://fedorahosted.org/freeipa/ticket/5254
There are a bunch of commands that have this situation - not just
certprofile - so if we're going to break API in one place IMO we
should do them all at once.
Why do we need to break the API ?
Just deny it.
Simo.
OK, original patch pushed to:
master: 5c7d6a6a31daca8bf92d85d8c1895279be84c888
ipa-4-2: d943bf09799e6faf2dd83f630bcfd6f99575c5c8
--
Petr Vobornik
--
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 0301] Fix: Remove leftover krbV reference
On 26.8.2015 12:13, Martin Basti wrote: On 08/26/2015 11:19 AM, Michael Šimáček wrote: On 2015-08-26 10:43, Jan Cholasta wrote: On 26.8.2015 10:37, Martin Basti wrote: Master branch is broken, this patch fixes it. Patch attached. This is not a pylint error, but an actual bug. Yes, commit e46d9236d19f714b67fdf2865f19146c3016f46d (yesterday evening) added another reference to krbV after my patch for migration from krbV was reviewed. This patch looks ok to me, I'd just change the commit message (to Remove leftover krbV reference or something like that). Thanks, Michael Fixed. updated patch attached. ACK. Pushed to master: 14a87632e5df30dd2b44a1ca49e6b308d2249c70 -- 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] 376 Removed clear text passwords from KRA install log.
On 08/22/2015 08:17 AM, Alexander Bokovoy wrote:
On Fri, 21 Aug 2015, Endi Sukma Dewata wrote:
The ipa-kra-install tool has been modified to use password files
instead of clear text passwords when invoking pki tool such that
the passwords are no longer visible in ipaserver-kra-install.log.
https://fedorahosted.org/freeipa/ticket/5246
--
Endi S. Dewata
From 545de89d5b8992469335415d209b6f04be6918ed Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Sat, 22 Aug 2015 01:14:16 +0200
Subject: [PATCH] Removed clear text passwords from KRA install log.
The ipa-kra-install tool has been modified to use password files
instead of clear text passwords when invoking pki tool such that
the passwords are no longer visible in ipaserver-kra-install.log.
https://fedorahosted.org/freeipa/ticket/5246
---
ipaplatform/base/paths.py| 2 ++
ipaserver/install/krainstance.py | 16
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index
0dd3c7fda3020264a1ace8f2d13557cfddf18c2d..5c8f25d6ef85fab2b9b30a660cd1c0360dbe9931
100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -343,6 +343,8 @@ class BasePathNamespace(object):
SLAPD_INSTANCE_SOCKET_TEMPLATE = /var/run/slapd-%s.socket
ALL_SLAPD_INSTANCE_SOCKETS = /var/run/slapd-*.socket
ADMIN_CERT_PATH = '/root/.dogtag/pki-tomcat/ca_admin.cert'
+KRA_NSSDB_PASSWORD_FILE =
/root/.dogtag/pki-tomcat/kra/password.conf
+KRA_PKCS12_PASSWORD_FILE =
/root/.dogtag/pki-tomcat/kra/pkcs12_password.conf
ACK.
Pushed to:
master: 8676364ae8260a5894b0b0c2af8e81b10aeaba6b
ipa-4-2: 4e474c5a20b91d4eed75f514f801b40f1f291e65
For the record, these files are created by pki-spawn early in the
creation of security databases for CA deployment. The second file isnt
created
if CA is deployed with HSM option (the databases are in hardware then) but
then the first one is created for HSM and thus both of them are in use.
We don't support deployment with HSM backend yet, but the code covers
both cases.
In future it would be good to actually source these values from
/etc/pki/default.cfg:
pki_client_password_conf=%(pki_client_subsystem_dir)s/password.conf
pki_client_pkcs12_password_conf=%(pki_client_subsystem_dir)s/pkcs12_password.conf
but right now this would mean need to use dogtag's Python helpers from
pki.server.deployment.pkiparser.PKIConfigParser.read_pki_configuration_file()
to do
actual sourcing of the config file but right now PKIConfigParser use
assumes it is actually parsing the command line options/arguments before
using its methods:
from pki.server.deployment.pkiparser import PKIConfigParser
cfg = PKIConfigParser('IPA CA', '')
cfg.init_config()
Traceback (most recent call last):
File stdin, line 1, in module
File
/usr/lib/python2.7/site-packages/pki/server/deployment/pkiparser.py,
line 196, in init_config
'pki_subsystem_type': config.pki_subsystem.lower(),
AttributeError: 'NoneType' object has no attribute 'lower'
--
Petr Vobornik
--
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 0065] vault: Limit size of data stored in vault
https://fedorahosted.org/freeipa/ticket/5231
--
David Kupka
From f86f4f89d1083c1474d8c470ae3b0f85ed1eb6bb Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Wed, 26 Aug 2015 14:11:21 +0200
Subject: [PATCH] vault: Limit size of data stored in vault
https://fedorahosted.org/freeipa/ticket/5231
---
ipalib/constants.py | 2 ++
ipalib/plugins/vault.py | 20 ++--
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/ipalib/constants.py b/ipalib/constants.py
index 53c3106cdd16fef0eba42a70518f7633b3fd95d1..5b83c7177987e95e101b2050aabfbc53d18e1b8d 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -239,3 +239,5 @@ SID_ANCHOR_PREFIX = ':SID:'
MIN_DOMAIN_LEVEL = 0
MAX_DOMAIN_LEVEL = 1
+
+MAX_VAULT_DATA_SIZE = 2**20 # = 1 MB
diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index 6b7d188f4c024cc6709faff33af942559ce4f8ec..4eb67438df0bb7ba3f22398d717b0be354edf893 100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -47,7 +47,7 @@ from ipalib.plugins.baseldap import LDAPObject, LDAPCreate, LDAPDelete,\
from ipalib.request import context
from ipalib.plugins.user import split_principal
from ipalib.plugins.service import normalize_principal
-from ipalib import _, ngettext
+from ipalib import _, ngettext, constants
from ipaplatform.paths import paths
from ipapython.dn import DN
from ipapython.nsslib import current_dbdir
@@ -1227,10 +1227,26 @@ class vault_archive(PKQuery, Local):
raise errors.MutuallyExclusiveError(
reason=_('Input data specified multiple times'))
+elif data:
+if len(data) constants.MAX_VAULT_DATA_SIZE:
+raise errors.ValidationError(name=data, error=_(Size of data
+ exceeds the limit. Current vault data size limit is
+ %(limit)d B) % {'limit': constants.MAX_VAULT_DATA_SIZE})
+
elif input_file:
+try:
+stat = os.stat(input_file)
+except IOError as exc:
+raise errors.ValidationError(name=in, error=_(Cannot read
+ file '%(filename)s': %(exc)s) % {'filename': input_file,
+'exc': exc[1]})
+if stat.st_size constants.MAX_VAULT_DATA_SIZE:
+raise errors.ValidationError(name=in, error=_(Size of data
+ exceeds the limit. Current vault data size limit is
+ %(limit)d B) % {'limit': constants.MAX_VAULT_DATA_SIZE})
data = validated_read('in', input_file, mode='rb')
-elif not data:
+else:
data = ''
if self.api.env.in_server:
--
2.4.3
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0065] vault: Limit size of data stored in vault
On 08/26/2015 03:56 PM, David Kupka wrote: On 26/08/15 15:45, Petr Vobornik wrote: On 08/26/2015 02:13 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/5231 Attaching updated patch. With changes discussed offline. Changes works for me, ACK. (with the changes it is also ACK from me) Pushed to: master: 02ab34c60b5e624ef0653a473316633a5618b07c ipa-4-2: 9fc82bc66992eaa5daeed80e366e10986a8583d8 Not related to the patch: This patch limits the size to 1MB instead of proposed 10MB. Testing showed that even 10MB raises a MemoryError in archive_encrypted_data which is AFAIK a KraClient method - Endi, this sounds as something which should be also handled in PKI. Especially when it happens the subsequent vault-archive command ends with HTTPError: 503 Server Error: Service Unavailable. After restart of pki, subsequent vault-archive with 1M file took about 4mins (in vault_retrieve_internal). Next archive command with 1M file took only 18s. 10k file took 9s. Why is it so slow? -- Petr Vobornik -- 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] 0004 Fix user tracker to reflect new user-del message
Fix for user tracker in ipatests/test_xmlrpc/test_user_plugin.py so that it reflects recently changed message of user-del command. Lenka From 226ea47939160ef3a164a1e8f979f52f10a7d83e Mon Sep 17 00:00:00 2001 From: Lenka Doudova ldoud...@redhat.com Date: Wed, 26 Aug 2015 16:16:43 +0200 Subject: [PATCH] Fix user tracker to reflect new user-del message --- ipatests/test_xmlrpc/test_user_plugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipatests/test_xmlrpc/test_user_plugin.py b/ipatests/test_xmlrpc/test_user_plugin.py index 1809f3d0a106a6a099500ca76d88a36758bc063d..b4355261f45087631b2509c70f4408a40e541922 100644 --- a/ipatests/test_xmlrpc/test_user_plugin.py +++ b/ipatests/test_xmlrpc/test_user_plugin.py @@ -1969,7 +1969,7 @@ class UserTracker(Tracker): def finish(): with raises_exact(errors.NotFound( -reason=u'no such entry')): +reason=u'%s: user not found' % self.uid)): del_command() request.addfinalizer(finish) -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0066] ipactl: Do not start/stop/restart single service multiple times
https://fedorahosted.org/freeipa/ticket/5248 -- David Kupka From 349e8ada21526cb704d9d876a151aaa2764970f8 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 26 Aug 2015 15:10:16 +0200 Subject: [PATCH] ipactl: Do not start/stop/restart single service multiple times In case multiple services are provided by single system daemon it is not needed to start/stop/restart it mutiple time. https://fedorahosted.org/freeipa/ticket/5248 --- install/tools/ipactl | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/install/tools/ipactl b/install/tools/ipactl index f37c4e02c44d57c93a88541192a8477cc6033a40..5782d4c424fb98c08e55614c71f3abaa6e776a68 100755 --- a/install/tools/ipactl +++ b/install/tools/ipactl @@ -45,6 +45,16 @@ def check_IPA_configuration(): raise IpactlError(IPA is not configured + (see man pages of ipa-server-install for help), 6) +def deduplicate(lst): +new_lst = [] +s = set(lst) +for i in lst: +if i in s: +s.remove(i) +new_lst.append(i) + +return new_lst + def is_dirsrv_debugging_enabled(): Check the 389-ds instance to see if debugging is enabled. @@ -283,6 +293,7 @@ def ipa_start(options): # no service to start return +svc_list = deduplicate(svc_list) for svc in svc_list: svchandle = services.service(svc) try: @@ -321,6 +332,7 @@ def ipa_stop(options): finally: raise IpactlError() +svc_list = deduplicate(svc_list) for svc in reversed(svc_list): svchandle = services.service(svc) try: @@ -398,6 +410,7 @@ def ipa_restart(options): if len(old_svc_list) != 0: # we need to definitely stop some services +old_svc_list = deduplicate(old_svc_list) for svc in reversed(old_svc_list): svchandle = services.service(svc) try: @@ -422,7 +435,7 @@ def ipa_restart(options): if len(svc_list) != 0: # there are services to restart - +svc_list = deduplicate(svc_list) for svc in svc_list: svchandle = services.service(svc) try: @@ -444,6 +457,7 @@ def ipa_restart(options): if len(new_svc_list) != 0: # we still need to start some services +new_svc_list = deduplicate(new_svc_list) for svc in new_svc_list: svchandle = services.service(svc) try: @@ -494,6 +508,7 @@ def ipa_status(options): if len(svc_list) == 0: return +svc_list = deduplicate(svc_list) for svc in svc_list: svchandle = services.service(svc) try: -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0065] vault: Limit size of data stored in vault
On 08/26/2015 02:13 PM, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/5231
Attaching updated patch. With changes discussed offline.
Not related to the patch:
This patch limits the size to 1MB instead of proposed 10MB. Testing
showed that even 10MB raises a MemoryError in archive_encrypted_data
which is AFAIK a KraClient method - Endi, this sounds as something which
should be also handled in PKI.
Especially when it happens the subsequent vault-archive command ends
with HTTPError: 503 Server Error: Service Unavailable. After restart of
pki, subsequent vault-archive with 1M file took about 4mins (in
vault_retrieve_internal). Next archive command with 1M file took only 18s.
10k file took 9s.
Why is it so slow?
--
Petr Vobornik
From c08848ad37010fa72e774305837db49a078ef5ea Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Wed, 26 Aug 2015 14:11:21 +0200
Subject: [PATCH] vault: Limit size of data stored in vault
https://fedorahosted.org/freeipa/ticket/5231
---
ipalib/plugins/vault.py | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index da1a58cfb77932e8a907725eb88f9f5c6df023c9..3f23c57be830fe85369bfc19e0b93581ded4115a 100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -236,6 +236,8 @@ def validated_read(argname, filename, mode='r', encoding=None):
register = Registry()
+MAX_VAULT_DATA_SIZE = 2**20 # = 1 MB
+
vaultcontainer_options = (
Str(
'service?',
@@ -1242,10 +1244,28 @@ class vault_archive(PKQuery, Local):
raise errors.MutuallyExclusiveError(
reason=_('Input data specified multiple times'))
+elif data:
+if len(data) MAX_VAULT_DATA_SIZE:
+raise errors.ValidationError(name=data, error=_(
+Size of data exceeds the limit. Current vault data size
+limit is %(limit)d B)
+% {'limit': MAX_VAULT_DATA_SIZE})
+
elif input_file:
+try:
+stat = os.stat(input_file)
+except OSError as exc:
+raise errors.ValidationError(name=in, error=_(
+Cannot read file '%(filename)s': %(exc)s)
+% {'filename': input_file, 'exc': exc[1]})
+if stat.st_size MAX_VAULT_DATA_SIZE:
+raise errors.ValidationError(name=in, error=_(
+Size of data exceeds the limit. Current vault data size
+limit is %(limit)d B)
+% {'limit': MAX_VAULT_DATA_SIZE})
data = validated_read('in', input_file, mode='rb')
-elif not data:
+else:
data = ''
if self.api.env.in_server:
--
2.4.3
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 477] spec file: Add Requires(pre) on selinux-policy
On Tue, Aug 25, 2015 at 03:50:04PM +0300, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
On 25.8.2015 14:23, Alexander Bokovoy wrote:
On Tue, 25 Aug 2015, Jan Cholasta wrote:
+Requires(pre): selinux-policy = %{selinux_policy_version}
Requires: selinux-policy = %{selinux_policy_version}
If we have it in Requires(pre), we don't need it in Requires, as
Requires(pre) is a superset of guarantees that Requires gives you.
Martin (CCed) told me Requires(pre) does not imply Requires.
See http://rpm.org/api/4.4.2.2/tsort.html (available since 2007):
Since the only way out of a dependency loop is to snip the loop
somewhere, rpm uses hints from Requires: dependencies to distinguish
co-requisite (these are not needed to install, only to use, a package)
from pre-requisite (these are guaranteed to be installed before the
package that includes the dependency) relations.
However, this section seems to only apply to loop resolution. Note
that
http://www.rpm.org/wiki/PackagerDocs/MoreOnDependencies
says about Requires(pre)
* It ensures that the package providing /usr/sbin/useradd is
installed before this package. In presence of dependency
loops, scriptlet dependencies are the only way to ensure
correct install order.
* If there are no other dependencies on the package providing
/usr/sbin/useradd, that package is permitted to be removed
from the system after installation(!)
It's a fairly common mistake to replace legacy PreReq
dependencies with Requires(pre), but this is not the
same, due to the latter point above!
So I'd say that Requires(pre) does not imply Requires and if we only
do Requires(pre): selinux-policy = %{selinux_policy_version}, after
the installation, anybody can downgrade the selinux-policy package.
Heck, even in that ipa-server upgrading transaction, there could be
a selinux-policy downgrade operation, which would leave the newer
version for ipa-server's pre but install older version of
selinux-policy after it's done with ipa-server.
Yes, it's just a theoretical situation but we should not shortcut
Requires with Requires(pre), it might teach people reading the .spec
files bad habits.
--
Jan Pazdziora
Senior Principal Software Engineer, Identity Management Engineering, Red Hat
--
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 0006] Fixed installation failures
Hi Martin, On 08/26/2015 09:58 AM, Martin Basti wrote: On 08/25/2015 06:41 PM, Oleg Fayans wrote: Hi Martin, On 08/24/2015 04:35 PM, Martin Basti wrote: On 08/24/2015 12:55 PM, Oleg Fayans wrote: Hi all. The current issue [1] effectively blocks testing of 4.2 branch. Here is (one of the possible) solution, that proved to work. [1] https://www.redhat.com/archives/freeipa-devel/2015-August/msg00085.html The patch needs rebase for ipa-4-2 branch. What is the best way to do it? git checkout --track origin/ipa-4-2 And then commit the same changes again? I usually do cherrypick, or just apply patch on top of ipa-4-2 branch with threeway merge (git am -3 option) I had discussion, yesterday, with Honza, and maybe this is not the correct fix. We are in hurry today with RHEL 7.2, but later I may send updated patch Cool, I'll wait then. -- Oleg Fayans Quality Engineer FreeIPA team RedHat. -- 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] import krbV error
On 26.8.2015 15:29, Martin Basti wrote:
I got following error during rpm install.
2015-08-26T13:25:40Z ERROR IPA server upgrade failed: Inspect
/var/log/ipaupgrade.log and run command ipa-server-upgrade manually.
2015-08-26T13:25:40Z DEBUG File
/usr/lib/python2.7/site-packages/ipapython/admintool.py, line 171, in
execute
return_value = self.run()
File
/usr/lib/python2.7/site-packages/ipaserver/install/ipa_server_upgrade.py,
line 44, in run
api.finalize()
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line 651,
in finalize
self.__do_if_not_done('load_plugins')
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line 365,
in __do_if_not_done
getattr(self, name)()
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line 529,
in load_plugins
self.import_plugins(module)
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line 567,
in import_plugins
module = importlib.import_module(name)
File /usr/lib64/python2.7/importlib/__init__.py, line 37, in
import_module
__import__(name)
File /usr/lib/python2.7/site-packages/ipalib/plugins/kerberos.py,
line 30, in module
import krbV
2015-08-26T13:25:40Z DEBUG The ipa-server-upgrade command failed,
exception: ImportError: No module named krbV
2015-08-26T13:25:40Z ERROR Unexpected error - see
/var/log/ipaupgrade.log for details:
ImportError: No module named krbV
ipalib/plugins/kerberos.py was removed in git master. I guess that on
your system it is a leftover from previous install. Remove the file and
everything should be fine.
--
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] import krbV error
On 08/26/2015 03:29 PM, Martin Basti wrote:
I got following error during rpm install.
2015-08-26T13:25:40Z ERROR IPA server upgrade failed: Inspect
/var/log/ipaupgrade.log and run command ipa-server-upgrade manually.
2015-08-26T13:25:40Z DEBUG File
/usr/lib/python2.7/site-packages/ipapython/admintool.py, line 171,
in execute
return_value = self.run()
File
/usr/lib/python2.7/site-packages/ipaserver/install/ipa_server_upgrade.py,
line 44, in run
api.finalize()
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line
651, in finalize
self.__do_if_not_done('load_plugins')
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line
365, in __do_if_not_done
getattr(self, name)()
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line
529, in load_plugins
self.import_plugins(module)
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line
567, in import_plugins
module = importlib.import_module(name)
File /usr/lib64/python2.7/importlib/__init__.py, line 37, in
import_module
__import__(name)
File /usr/lib/python2.7/site-packages/ipalib/plugins/kerberos.py,
line 30, in module
import krbV
2015-08-26T13:25:40Z DEBUG The ipa-server-upgrade command failed,
exception: ImportError: No module named krbV
2015-08-26T13:25:40Z ERROR Unexpected error - see
/var/log/ipaupgrade.log for details:
ImportError: No module named krbV
You can ignore this, error is probably on my side.
--
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] import krbV error
I got following error during rpm install.
2015-08-26T13:25:40Z ERROR IPA server upgrade failed: Inspect
/var/log/ipaupgrade.log and run command ipa-server-upgrade manually.
2015-08-26T13:25:40Z DEBUG File
/usr/lib/python2.7/site-packages/ipapython/admintool.py, line 171, in
execute
return_value = self.run()
File
/usr/lib/python2.7/site-packages/ipaserver/install/ipa_server_upgrade.py,
line 44, in run
api.finalize()
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line 651,
in finalize
self.__do_if_not_done('load_plugins')
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line 365,
in __do_if_not_done
getattr(self, name)()
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line 529,
in load_plugins
self.import_plugins(module)
File /usr/lib/python2.7/site-packages/ipalib/plugable.py, line 567,
in import_plugins
module = importlib.import_module(name)
File /usr/lib64/python2.7/importlib/__init__.py, line 37, in
import_module
__import__(name)
File /usr/lib/python2.7/site-packages/ipalib/plugins/kerberos.py,
line 30, in module
import krbV
2015-08-26T13:25:40Z DEBUG The ipa-server-upgrade command failed,
exception: ImportError: No module named krbV
2015-08-26T13:25:40Z ERROR Unexpected error - see
/var/log/ipaupgrade.log for details:
ImportError: No module named krbV
--
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 0065] vault: Limit size of data stored in vault
On 26/08/15 15:45, Petr Vobornik wrote: On 08/26/2015 02:13 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/5231 Attaching updated patch. With changes discussed offline. Changes works for me, ACK. Not related to the patch: This patch limits the size to 1MB instead of proposed 10MB. Testing showed that even 10MB raises a MemoryError in archive_encrypted_data which is AFAIK a KraClient method - Endi, this sounds as something which should be also handled in PKI. Especially when it happens the subsequent vault-archive command ends with HTTPError: 503 Server Error: Service Unavailable. After restart of pki, subsequent vault-archive with 1M file took about 4mins (in vault_retrieve_internal). Next archive command with 1M file took only 18s. 10k file took 9s. Why is it so slow? -- 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] 375 Added mechanism to copy vault secrets.
On 8/20/2015 2:08 AM, Endi Sukma Dewata wrote:
On 8/19/2015 4:20 AM, Martin Basti wrote:
On 08/16/2015 05:29 PM, Endi Sukma Dewata wrote:
The vault-add and vault-archive commands have been modified to
optionally retrieve a secret from a source vault, then re-archive
the secret into the new/existing target vault.
https://fedorahosted.org/freeipa/ticket/5223
I cannot apply this patch.
Rebased. It depends on patch #371-2.
Rebased due to other changes in vault.
--
Endi S. Dewata
From 676b2043a390e6e68772837cf46e222aeda9da78 Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Sat, 15 Aug 2015 16:17:47 +0200
Subject: [PATCH] Added mechanism to copy vault secrets.
The vault-add and vault-archive commands have been modified to
optionally retrieve a secret from a source vault, then re-archive
the secret into the new/existing target vault.
https://fedorahosted.org/freeipa/ticket/5223
---
API.txt | 20 ++-
VERSION | 4 +-
ipalib/plugins/vault.py | 213 --
ipatests/test_xmlrpc/test_vault_plugin.py | 143
4 files changed, 306 insertions(+), 74 deletions(-)
diff --git a/API.txt b/API.txt
index
afd5017bee2bc1eed54497ccd504b92619ff7a58..c883271af4ff84f82c623208567f114265c3ce60
100644
--- a/API.txt
+++ b/API.txt
@@ -5405,7 +5405,7 @@ output: Output('result', type 'bool', None)
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
command: vault_add
-args: 1,14,3
+args: 1,22,3
arg: Str('cn', attribute=True, cli_name='name', maxlength=255,
multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True,
required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False,
exclude='webui')
@@ -5419,6 +5419,14 @@ option: Flag('raw', autofill=True, cli_name='raw',
default=False, exclude='webui
option: Str('service?')
option: Str('setattr*', cli_name='setattr', exclude='webui')
option: Flag('shared?', autofill=True, default=False)
+option: Str('source_password?')
+option: Str('source_password_file?')
+option: Bytes('source_private_key?')
+option: Str('source_private_key_file?')
+option: Str('source_service?')
+option: Flag('source_shared?', autofill=True, default=False)
+option: Str('source_user?')
+option: Str('source_vault?')
option: Str('username?', cli_name='user')
option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an
LDAP entry', domain='ipa', localedir=None))
@@ -5474,7 +5482,7 @@ output: Output('completed', type 'int', None)
output: Output('failed', type 'dict', None)
output: Entry('result', type 'dict', Gettext('A dictionary representing an
LDAP entry', domain='ipa', localedir=None))
command: vault_archive
-args: 1,11,3
+args: 1,19,3
arg: Str('cn', attribute=True, cli_name='name', maxlength=255,
multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True,
required=True)
option: Flag('all', autofill=True, cli_name='all', default=False,
exclude='webui')
option: Bytes('data?')
@@ -5485,6 +5493,14 @@ option: Str('password_file?', cli_name='password_file')
option: Flag('raw', autofill=True, cli_name='raw', default=False,
exclude='webui')
option: Str('service?')
option: Flag('shared?', autofill=True, default=False)
+option: Str('source_password?')
+option: Str('source_password_file?')
+option: Bytes('source_private_key?')
+option: Str('source_private_key_file?')
+option: Str('source_service?')
+option: Flag('source_shared?', autofill=True, default=False)
+option: Str('source_user?')
+option: Str('source_vault?')
option: Str('username?', cli_name='user')
option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an
LDAP entry', domain='ipa', localedir=None))
diff --git a/VERSION b/VERSION
index
d3073e52ee022cc08b74953222a5040929ded60f..e3cfaa91f03fc6f4d9f5084809a8f74af333c8ef
100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
# #
IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=154
-# Last change: pvoborni - change default vault type to 'symmetric'
+IPA_API_VERSION_MINOR=155
+# Last change: edewata - Added mechanism to copy vault secrets.
diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index
ff6c22c646e9784b2fa1a6464f0749cb1ce86b50..a625746ab067d915e71504e971eefb0d0222ff77
100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -255,6 +255,78 @@ vault_options = (
),
)
+source_vault_options = (
+Str(
+'source_vault?',
+doc=_('Name of the source service vault'),
+),
+Str(
+'source_service?',
+doc=_('Service name of the source
[Freeipa-devel] [PATCH] Updated no of legacy permission in ipatests
Hi All,
This patch fixes bug - https://fedorahosted.org/freeipa/ticket/5264
Thanks,
Abhijeet Kasurde
From 4492ddd0e07c5c82e8fbe2d7ae88e5fb0bce5be0 Mon Sep 17 00:00:00 2001
From: Abhijeet Kasurde akasu...@redhat.com
Date: Thu, 27 Aug 2015 10:31:35 +0530
Subject: [PATCH] Updated number of legacy permission in ipatests
Since IPA 4.2 has an additional permission
Request Certificate ignoring CA ACLs, the number of legacy
permission in testcase is updated from 8 to 9.
https://fedorahosted.org/freeipa/ticket/5264
Signed-off-by: Abhijeet Kasurde akasu...@redhat.com
---
ipatests/test_xmlrpc/test_permission_plugin.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipatests/test_xmlrpc/test_permission_plugin.py b/ipatests/test_xmlrpc/test_permission_plugin.py
index c899c428edfcd19c2e7f538cdc38b693e11c8715..fd69ff25dcded412c78c38ccffd0d39413ea6943 100644
--- a/ipatests/test_xmlrpc/test_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_permission_plugin.py
@@ -2849,7 +2849,7 @@ def check_legacy_results(results):
legacy_permissions = [p for p in results
if not p.get('ipapermissiontype')]
print legacy_permissions
-assert len(legacy_permissions) == 8, len(legacy_permissions)
+assert len(legacy_permissions) == 9, len(legacy_permissions)
return True
class test_permission_legacy(Declarative):
--
2.4.3
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCHES 478-479] cert renewal: KRA agent certificate update
Hi,
the attached patches fix https://fedorahosted.org/freeipa/ticket/5253.
Honza
--
Jan Cholasta
From 5600a357d3dbed0524acd766a2ce19b20e58235e Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Thu, 27 Aug 2015 07:23:39 +0200
Subject: [PATCH 1/2] cert renewal: Include KRA users in Dogtag LDAP update
https://fedorahosted.org/freeipa/ticket/5253
---
ipaserver/install/cainstance.py | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 60c41b6..c8b834f 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -1575,7 +1575,7 @@ def update_people_entry(dercert):
Returns True or False
-base_dn = DN(('ou','People'), ('o','ipaca'))
+base_dn = DN(('o', 'ipaca'))
serial_number = x509.get_serial_number(dercert, datatype=x509.DER)
subject = x509.get_subject(dercert, datatype=x509.DER)
issuer = x509.get_issuer(dercert, datatype=x509.DER)
@@ -1591,9 +1591,14 @@ def update_people_entry(dercert):
conn = ldap2.ldap2(api, ldap_uri=dogtag_uri)
conn.connect(autobind=True)
-db_filter = conn.make_filter(
-{'description': ';%s;%s' % (issuer, subject)},
-exact=False, trailing_wildcard=False)
+db_filter = conn.combine_filters(
+[
+conn.make_filter({'objectClass': 'inetOrgPerson'}),
+conn.make_filter(
+{'description': ';%s;%s' % (issuer, subject)},
+exact=False, trailing_wildcard=False),
+],
+conn.MATCH_ALL)
try:
entries = conn.get_entries(base_dn, conn.SCOPE_SUBTREE, db_filter)
except errors.NotFound:
--
2.4.3
From 8aacd2e5e318d53b9db3d28ab615f49e57dc7cf2 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Thu, 27 Aug 2015 07:37:24 +0200
Subject: [PATCH 2/2] cert renewal: Automatically update KRA agent PEM file
https://fedorahosted.org/freeipa/ticket/5253
---
install/restart_scripts/renew_ra_cert | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/install/restart_scripts/renew_ra_cert b/install/restart_scripts/renew_ra_cert
index 24b8ba4..4337e7a 100644
--- a/install/restart_scripts/renew_ra_cert
+++ b/install/restart_scripts/renew_ra_cert
@@ -29,7 +29,7 @@ import traceback
from ipapython import ipautil
from ipalib import api
-from ipaserver.install import certs, cainstance
+from ipaserver.install import certs, cainstance, krainstance
from ipaplatform import services
from ipaplatform.paths import paths
@@ -60,6 +60,16 @@ def _main():
# Load it into dogtag
cainstance.update_people_entry(dercert)
+
+kra = krainstance.KRAInstance(api.env.realm)
+if kra.is_installed():
+# export ipaCert with private key for client authentication
+args = [/usr/bin/pki,
+-d, paths.HTTPD_ALIAS_DIR,
+-C, paths.ALIAS_PWDFILE_TXT,
+client-cert-show, ipaCert,
+--client-cert, paths.KRA_AGENT_PEM]
+ipautil.run(args)
finally:
shutil.rmtree(tmpdir)
--
2.4.3
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] changes in preparation of replica promotion work
On 25.8.2015 20:43, Simo Sorce wrote: On Wed, 2015-08-05 at 11:24 -0400, Simo Sorce wrote: On Wed, 2015-08-05 at 08:20 +0200, Jan Cholasta wrote: Hi, Dne 31.7.2015 v 12:46 Simo Sorce napsal(a): I've been carrying these patches in my tree for a while, I think it is time to put them in master as they stand on their own. Simo. Patch 530: ACK Patch 531: ACK Patch 532: The methods should be static methods: @staticmethod def setOption(name, value): ... Care to explain why ? @staticmethod is not used anywhere else in that file. Rebased patches on master, made requested change +1 more patch. Simo. Patch 532: ACK Patch 533: ACK Pushed to master: f57b687241fbc92d1138507210e87e9de465c507 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] [PATCH] First part of integration tests for Topology Plugin
On 08/26/2015 02:53 PM, Oleg Fayans wrote: Hi, No more short links :) On 08/26/2015 11:50 AM, Tomas Babej wrote: On 08/26/2015 11:44 AM, Oleg Fayans wrote: Hi Martin, On 08/20/2015 11:18 AM, Martin Basti wrote: On 08/20/2015 10:26 AM, Martin Basti wrote: On 08/19/2015 04:17 PM, Martin Basti wrote: I got this: https://paste.fedoraproject.org/256746/43999380/ FYI replica install failure. (I will retest it, but I'm pretty sure that it was clean VM, test for some reason install client first) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 295, in decorated func(installer) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 319, in install_check sys.exit(IPA client is already configured on this system.\n 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, exception: SystemExit: IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. 2015-08-19T14:14:15Z ERROR IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. Confirm I got same error. Fixed. It was a regex error. On 08/19/2015 09:00 AM, Oleg Fayans wrote: Hi Martin, As discussed, here is a new version with pep8-related fixes On 08/14/2015 10:44 AM, Oleg Fayans wrote: Hi Martin, Already noticed that. Implemented the named groups as Tomas advised. Added the third test for http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica On 08/13/2015 05:06 PM, Martin Basti wrote: On 08/11/2015 03:36 PM, Oleg Fayans wrote: Hi Martin, On 08/11/2015 02:02 PM, Martin Basti wrote: NACK, comments inline. On 11/08/15 13:25, Oleg Fayans wrote: Hi Martin, Thanks for the review! On 08/10/2015 07:08 PM, Martin Basti wrote: Thank you for patch, I have a few nitpicks: 1) On 10/08/15 13:05, Oleg Fayans wrote: +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) Why do you add the name of method in docstring? My bad, fixed. still there +tokenize_topologies(command_output) +takes an output of `ipa topologysegment-find` and returns an array of Fixed, sorry. 2) +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) +creates a topology segment. The first argument is a node to run the command on +The first 3 arguments should be objects of class Host +Returns a hash object containing segment's name, leftnode, rightnode information + I would prefer to add assert there instead of just document that a Host object is needed assert(isinstance(master, Host)) ... Fixed. Created a decorator that checks the type of arguments This does not scale well. If we will want to add new argument that is not host object, you need change it again. Agreed. Modified the decorator so that you can specify a slice of arguments to be checked and a class to compare to. This does scale :) This might be used as function with specified variables that have to be host objects 3) +def destroy_segment(master, segment_name): + +destroy_segment(master, segment_name) +Destroys topology segment. First argument should be object of class Host Instead of description of params as first, second etc., you may use following: +def destroy_segment(master, segment_name): + +Destroys topology segment. +:param master: reference to master object of class Host +:param segment: name fo segment and eventually this in other methods +:returns: Lorem ipsum sit dolor mit amet +:raises NotFound: if segment is not found Fixed 4) cls.replicas[:len(cls.replicas) - 1], I suggest cls.replicas[:-1] In [2]: a = [1, 2, 3, 4, 5] In [3]: a[:-1] Out[3]: [1, 2, 3, 4] Fixed 5) Why re.findall() and then you just use the first result? 'leftnode': self.leftnode_re.findall(i)[0] Isn't just re.search() enough? leftnode_re.search(value).group(1) in fact leftnode_re.findall(string)[0] and leftnode_re.search(string).group(1), Are equally bad from the readability point of view. The first one is even shorter a bit, so why change? :) It depends on point of view, because when I reviewed it yesterday my brain raises exception that you are trying to add multiple values to single value attribute, because you used findall, I expected that you need multiple values, and then I saw that index [0] at the end, and I was pretty confused what are you trying to achieve. And because findall is not effective in case when you need to find just one occurrence. I got it. Fixed. Python 3 nitpick: I'm not sure if time when we should enforce python 2/3 compability already comes, but just
