Re: [Freeipa-devel] [PATCH 0022-23] Coverity patches

[Stanislav Laznicka]

Reworded the commit messages so that they mention Coverity.

On 02/22/2016 07:18 AM, Jan Cholasta wrote:

On 2.2.2016 13:36, Stanislav Laznicka wrote:

On 02/01/2016 02:24 PM, Jan Cholasta wrote:

On 1.2.2016 12:11, Petr Spacek wrote:

On 1.2.2016 09:03, Jan Cholasta wrote:

Hi,

On 29.1.2016 15:49, Martin Basti wrote:



On 29.01.2016 15:49, Stanislav Laznicka wrote:

Reworded the commits so that they better reflect what's going on
in those.

On 01/29/2016 02:49 PM, Stanislav Laznicka wrote:

Hello,

I made some patches based on the Coverity report from 18.1.2016.

Cheers,
Standa




NACK, see my previous email


I don't think this deserves 9 patches, 1 would be sufficient enough.


I would rather have it is separate patches as these fixes are largely
not
related. It will make bisecting easier.


Most of the fixes are cosmetic, which makes them related, and the rest
are small isolated changes, so in reality it would hardly make
bisecting easier and only increase the overhead. In the past we
usually had put such fixes into a single patch and AFAIK nobody
complained so far.


Squeezed the changes into two new patches, then. One for the very
cosmetic changes, one for possible bugs.


OK.





Patch 0013:

1) I think this unreachable return is intentional, as indicated by
the comment:

-#we shouldn't get here
-return [UNKNOWN_ERROR]


I would use
assert False, "we shouldn't get here"
neither we nor Coverity are confused when we hit the code path one 
day.


UNKNOWN_ERROR would pop up somewhere else and it will be harder to
find out
why the hell the code behaves as mad. Traceback will clearly indicate
that
there is a problem with the 'switch'.


Sure, my point is that returning None is no better than returning
UNKNOWN_ERROR.


Added assert as suggested. There should still be no way of getting to
it, though.


Petr^2 Spacek


2) How is this dead code?

-if options.mode == 'validate_pot' or options.mode ==
'validate_po':
+if options.mode in ('validate_pot', 'validate_po'):

-elif options.mode == 'create_test' or 'test_gettext':
+elif options.mode in ('create_test', 'test_gettext'):



Patch 0014-0015: LGTM


Actually scratch that, patch 0015 breaks correct code.


The dead code appears in the 'else' branch as the latter of these two
conditions always evaluates to True. The first condition change is just
a cosmetic one so that both of the conditions look the same.


OK.



Also removed the changes made in patch 0015.



Patch 0016: The original code is in fact correct.


Point taken, removed the change.


Patch 0017: This will break Python 3. The two branches are
performing the same
action, but with different data types.


This might undergo further investigation in the future as there is no
way how "bytes" instance could become an argument of this function (as
suggested). Not even the newest Python 3 patches from pviktori mention
this code.


OK. (This is not what Coverity was complaining about, though.)



Patch 0018: LGTM


Patch 0019: IMO the original code is better (None has a __class__
too, you know).


Made it more "Coverity friendly" yet nice enough modification.


Patch 0020: LGTM


It seems that there actually is a check that checks whether the input is
correct. It is called ad-hoc but that might be the test feature.
Therefore just added an assert so that Coverity does not complain.


OK.



Patch 0021: Please use the original error messages (there are no
requests
being added to D-Bus, but to certmonger).


Honza





Added error messages that reflect the situation better, then.


Could you please mention Coverity in the commit messages, so that it's 
clear why are we doing these changes? Otherwise LGTM.




From 7e062d48fca49374eac27890c3cb6489764540b1 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 2 Feb 2016 12:50:26 +0100
Subject: [PATCH 1/2] Cosmetic changes to the code

Fixes some Coverity issues

ipadiscovery.py:  added assert should universe break
plugins/dns.py:   removed dead code
dnssec/ldapkeydb.py:  attribute assert in the proper object
test_automount_plugin.py: fixed possible close() on None
xmlrpc_test.py:   Coverity does not like accessing None.__class__

https://fedorahosted.org/freeipa/ticket/5661
---
 ipaclient/ipadiscovery.py | 2 +-
 ipalib/plugins/dns.py | 3 ---
 ipapython/dnssec/ldapkeydb.py | 2 +-
 ipatests/test_xmlrpc/test_automount_plugin.py | 2 ++
 ipatests/test_xmlrpc/xmlrpc_test.py   | 2 +-
 5 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/ipaclient/ipadiscovery.py b/ipaclient/ipadiscovery.py
index 45a71e190e56d33d51d37f16ae61a7b4c28df521..772add43a282838539b9b18dc60d3bba041bed1b 100644
--- a/ipaclient/ipadiscovery.py
+++ b/ipaclient/ipadiscovery.py
@@ -402,7 +402,7 @@ class IPADiscovery(object):
 return [0, thost, lrealms[0]]
 
 #we shouldn't get here
-

Re: [Freeipa-devel] [patch 0034] ipatests: extend permission plugin test with new expected output

[Milan Kubík]

On 02/18/2016 03:52 PM, Milan Kubík wrote:

On 02/15/2016 04:59 PM, Milan Kubík wrote:

Patch attached. Applies on ipa-4-3 as well.




Updated version of patch fixes test_old_permission_plugin as well.

--
Milan Kubik



Review bump.

--
Milan Kubik

-- 
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 0084-0086] CI: Add double circle topology

[David Kupka]

On 23/02/16 17:54, Martin Basti wrote:



On 23.02.2016 17:33, Martin Basti wrote:



On 23.02.2016 17:30, Martin Basti wrote:



On 18.02.2016 10:14, David Kupka wrote:

On 12/02/16 16:52, Martin Basti wrote:



On 12.02.2016 13:03, Milan Kubík wrote:

On 02/12/2016 10:59 AM, David Kupka wrote:

Sending one more topology test. This one creates a M groups
consisting N (N>=2) servers.
First two servers in each group are used to connect with nearest
four
groups and also with the other servers inside the group (when N>2).
Servers inside the group (not connecting to other groups) are
connected with each other.

The patch set needs freeipa-dkupka-8{1,2,3} applied.


ACK


I cannot apply patches, please rebase

[mbasti@dhcp129-96 freeipa-devel]$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
[mbasti@dhcp129-96 freeipa-devel]$ git am freeipa-dkupka-008* -3
Applying: CI: Add '2-connected' topology generator.
Applying: CI: Add simple replication test in 2-connected topology.
Applying: CI: Add test for 2-connected topology generator.
Applying: CI: Add double circle topology.
Applying: CI: Add replication test utilizing double-circle topology.
Applying: CI: Add test for double-circle topology generator.
error: invalid object 100644 e12d141391840cc7f9150a178875393a96dd469b
for 'ipatests/test_integration/test_topologies.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0006 CI: Add test for double-circle topology
generator.
The copy of the patch that failed is found in:
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am
--abort".


Martin^2


Git fails to apply patches because wrong version of
freeipa-dkupka-008{1,2,3} was pushed. Attached patches should fix it.


Sorry my bad.

ACK

Pushed to:
ipa-4-3: ffe3731ae7813fcc3246a83e37d62fc2754bb4ca
master: acdabba6ec0f68841f02c1e6ad65232de81bb505


New test:

Pushed to:
master: a1e582b33c42bcc8a708777afb975e7dc571ee3d
ipa-4-3: 2efa60637111e40a9ac9459d507d9f55a2ae301a


Revert?
IMO this will not work on python3

* Module ipatests.test_integration.test_topologies
ipatests/test_integration/test_topologies.py:124:
[W1638(range-builtin-not-iterating), test_topology_double_circle_topo]
range built-in referenced when not iterating)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:949: [W0110(deprecated-lambda),
double_circle_topo] map/filter on lambda could be replaced by
comprehension)
ipatests/test_integration/tasks.py:949:
[W1636(map-builtin-not-iterating), double_circle_topo] map built-in
referenced when not iterating)
ipatests/test_integration/tasks.py:949:
[W1637(zip-builtin-not-iterating), double_circle_topo] zip built-in
referenced when not iterating)



You can revert it and I will send fixed patches or you can just apply 
attached patch.



--
David Kupka
From a46ba01172e666d8afdac5e0605f32110138ea39 Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Wed, 24 Feb 2016 08:15:51 +0100
Subject: [PATCH] CI: Make double circle topology python3 compatible

---
 ipatests/test_integration/tasks.py   | 2 +-
 ipatests/test_integration/test_topologies.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 9d9a78bb10b463016c646aa921dde722e882da93..60e9e82391077ce6b997d0ed4ad4f2ff19f43d1e 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -946,7 +946,7 @@ def double_circle_topo(master, replicas, site_size=6):
 
 # split servers into sites
 it = [iter(servers)] * site_size
-sites = map(lambda x: (x[0], x[1], x[2:]), zip(*it))
+sites = [(x[0], x[1], x[2:]) for x in zip(*it)]
 num_sites = len(sites)
 
 for i in range(num_sites):
diff --git a/ipatests/test_integration/test_topologies.py b/ipatests/test_integration/test_topologies.py
index a0a1b9d6222779a9ce67b2fa8a29052747eae8f9..4bdff44a162e188c0549b6ecca12ae658839e5cd 100644
--- a/ipatests/test_integration/test_topologies.py
+++ b/ipatests/test_integration/test_topologies.py
@@ -121,7 +121,7 @@ def test_topology_two_connected():
 def test_topology_double_circle_topo():
 topo = tasks.get_topo('double-circle')
 assert topo == tasks.double_circle_topo
-assert list(topo('M', range(1, 30))) == [
+assert list(topo('M', [r for r in range(1, 30)])) == [
 ('M', 1),
 (1, 6),
 (1, 12),
-- 
2.5.0

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

Re: [Freeipa-devel] [PATCH 0011] Move freeipa certmonger helpers to libexecdir.

[David Kupka]

On 23/02/16 16:41, Rob Crittenden wrote:

David Kupka wrote:

On 23/02/16 10:14, Martin Kosek wrote:

On 02/23/2016 09:47 AM, David Kupka wrote:

On 22/02/16 16:15, Martin Kosek wrote:

On 02/22/2016 04:04 PM, Jan Cholasta wrote:

On 22.2.2016 15:56, David Kupka wrote:

On 22/02/16 07:28, Jan Cholasta wrote:

On 18.2.2016 10:10, David Kupka wrote:

On 19/01/16 16:10, David Kupka wrote:

On 19/01/16 14:38, Jan Cholasta wrote:

On 19.1.2016 14:26, Martin Kosek wrote:

On 01/19/2016 01:47 PM, David Kupka wrote:

I've polished the patch attached to #5586 by Timo Aaltonen.

Thanks for the patch. I've fixed the path in specfile and
removed
unused import
but otherwise it works, ACK.

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


Won't this break existing certmonger requests depending on
the old
path?


It will, I don't see any upgrade code.



# getcert list | grep '/usr/lib64/ipa/certmonger'
   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"auditSigningCert
cert-pki-ca"
   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"ocspSigningCert
cert-pki-ca"
   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"subsystemCert
cert-pki-ca"
   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"caSigningCert
cert-pki-ca"
   post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert
   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"Server-Cert
cert-pki-ca"
   post-save command:
/usr/lib64/ipa/certmonger/restart_dirsrv
RHEL72
   post-save command: /usr/lib64/ipa/certmonger/restart_httpd






You're right it will break the upgrade. I haven't noticed that
Server-Cert for DS and HTTPD are not handled by
certificate_renewal_update (ipaserver.install.server.upgrade)
where all
the other trackings are stopped and then configured again with the
paths.CERTMONGER_COMMAND_TEMPLATE already updated.

Thanks for the catch.



I've updated Timo's patch little more and added
start_tracking_certificates() for dsinstance and httpinstance.
Now the
upgrade works as expected.


The way the patches are split is kind of weird and apparently
confusing
(see the other thread). IMO there should be 2 patches: the first
should
add the ability to change DS and HTTP certmonger config during
upgrade
(i.e. the start_tracking_certificates() methods and
certificate_renewal_update() changes), the second should move the
helpers (i.e. the actual move and certificate_renewal_update()
version
bump).


Honza, do I understand it correctly that the code is OK but I did not
split it to the patches correctly?


Yes.


Before acking or pushing, can you please explain for me how the
upgrade of
certmonger tracking requests work? I want to make sure this is
right, so please
bear with me:

1) How does it edit existing tracking requests with the new helper
paths?

2) Does it go and try to edit the requests on every upgrade? Or is
there some
check that requests were updated?

Thanks,
Martin



Whole upgrade of renewal requests is done in
ipaserver/install/server/upgrade.py in certificate_renewal_upgrade().

First there is version of requests and if it's the same as in state file
upgrade is skipped.
Then every request is searched over certmonger's DBus interface and
if at least
one is not found it means that there was change in request
configuration. All
tracking requests are then stopped and started again with new
configuration.

So to answer you questions:
1) By stopping the old request with the old parameters (including
path) and
starting new with new parameters.

2) Only if version was bumped which happens only if some of the
requests changes.


Ah, so IIUC, if you bump the version, requests should be properly
updated. The
change looks fine then.



After discussion with Honza, we decided to drop the part comparing only
base names of pre- and post-save commands and use it as whole. I've also
split the patches so it's obvious what is going on.

Patches should be applied in this order:

freeipa-dkupka-0091.0


A cert could silently fail to be tracked in
start_tracking_certificates() if no serverid can be found.


In that case it also wouldn't be stopped. The behavior is the same as in 
existing stop_tracking_certificates(). Should we rather raise and stop 
the upgrade? I guess not but warning would be probably useful. What 
solution would you prefer, Rob?





freeipa-dkupka-0087.1
freeipa-dkupka-0088.1
freeipa-tjaalton-0011.2
freeipa-dkupka-0092.0






--
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 0420] Set BuildRequires to pylint 1.4

[Rob Crittenden]

Lukas Slebodnik wrote:
> On (23/02/16 17:09), Martin Basti wrote:
>> We cannot guarantee that versions older than 1.4 will work with freeipa code.
>>
>> Patch attached.
> 
>>From a59e72a0b87231c0f2e0d737057550dd532feed7 Mon Sep 17 00:00:00 2001
>> From: Martin Basti 
>> Date: Tue, 23 Feb 2016 16:58:07 +0100
>> Subject: [PATCH] Set BuildRequires to pylint >= 1.4
>>
>> We can guarantee that only pylint 1.4 and newer will work
>>
>> https://fedorahosted.org/freeipa/ticket/5615
>> ---
>> freeipa.spec.in | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/freeipa.spec.in b/freeipa.spec.in
>> index 
>> 54a11bfc8cced643c19c29c5ada70bacf7540395..219c5ca2f13eaac14746ec4689ba611bbc6fc377
>>  100644
>> --- a/freeipa.spec.in
>> +++ b/freeipa.spec.in
>> @@ -76,7 +76,7 @@ BuildRequires:  python-netaddr
>> BuildRequires:  python-gssapi >= 1.1.2
>> BuildRequires:  python-rhsm
>> BuildRequires:  pyOpenSSL
>> -BuildRequires:  pylint >= 1.0
>> +BuildRequires:  pylint >= 1.4
> 
> I can build rpms even withour pylint and pylint is not executed
> anywhere in spec file. (in other words, my patch was rejected)
> Why does it need to be in BuildRequires?

pylint is part of the in-tree build process (make rpms). It is not
executed when building upstream packages.

rob

-- 
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] Locations design v2: LDAP schema & user interface

[Petr Spacek]

On 23.2.2016 18:14, Simo Sorce wrote:
>> > Petr Vobornik mentioned an important question:
>> > Should we care about non-IPA services?
>> > 
>> > IMHO it is a valid point. It complicates things a lot as soon as we start
>> > introducing 'locations per service'. It is certainly doable but I would 
>> > like
>> > to avoid it.
>> > 
>> > It seems easy enough to support custom services as long as there is only 
>> > one
>> > set of locations (which match IPA locations). It would be a management
>> > nightmare to support N parallel locations for distinct sets of services.
>> > 
>> > As far as I can tell, AD can live with only 1 set of locations and that 
>> > sounds
>> > reasonable thing to support in the IPA management interface to me.
> I think one set of Locations is fine, but we need to be able to assign
> services to locations independently from "servers" in some cases, I
> think.
> 
> Mostly because a server can have service 1 and 3 but not service 2 and
> another server can have service 2 and 3 but not 1.

Hmm, I do not follow. Where is the problem? You can assign both servers to the
location and get all three services available, the SRV records from all
servers will just combine.

We should have checks in IPA so it will not allow you to create a Frankenstein
IPA server which is missing LDAP or so but this will be needed only when we
decide to containerize - or when we allow to select individual services
instead of servers :-)

For custom services, you are on your own. Still, I would say that assigning
services (instead of servers) to location is making it *more* error prone as
there is bigger chance of omitting something (like selecting only service 1
from server A) and not the other way around.

What did I miss?

[...]

 > >> Priority groups are harder because they express metric based on:
 > >> * communication costs,
 > >> * fail-over requirements,
 > >> * other political requirements in given deployment.
 > >> These are hard things to see from layer 7.
 > >>
 > >> Theoretically we can provide ipa-advise plugin to generate some 
 > >> initial set of
 > >> groups but this is going to be complicated and error prone.
 > >>
 > >> E.g. we can use ICMP ping or LDAP base DN search timings and use some
 > >> clustering algorithm to create priority groups using measured values.
 > >> This could work if we use some smart-enough clustering algorithm (= AI
 > >> library). And of course, we would have to do measurements from at 
 > >> least one
 > >> server in each location to properly define groups for each location 
 > >> ...
 > >>
 > >> It is not that easy as it might seem and I do not see an easy 
 > >> solution.
 > >>
 > >>
 > >> Maybe we should take evolutional approach:
 > >> Implement this 'expert' UI which exposes groups & weights to the user 
 > >> first.
 > >> (It will be necessary for special cases anyway.) When this is done, 
 > >> we can
 > >> play with it, do some usability testing (we can ask RH IT to see if 
 > >> it makes
 > >> sense to them, for example.)
 > >>
 > >> Later we can extend this with a 'simple' variant of UI based on 
 > >> feedback or
 > >> add the generator). This does not even need to happen in the same 
 > >> release.
 > >>
 > >> IMHO it would be better to start with something and refine it later 
 > >> on because
 > >> right now we are just hand-waving and have no idea what users 
 > >> actually do and
 > >> want.
>>> > > 
>>> > > As long as we establish a proper CLI I am ok with implementing a very
>>> > > bare bone UI first and improving it only later.
>>> > > 
>>> > > Btw we probably want to have this information reported by the topology
>>> > > view, and used to automatically group servers there based on location,
>>> > > so I CCed Petr to see if there is anything that would make that job
>>> > > easier/harder.
>> > 
>> > 
>> > We were kicking ideas around the drawing board in the Brno office. Finally,
>> > after many iterations we arrived to this:
>> > 
>> > Wouldn't it be easier to implement concept of sites and links between 
>> > sites at
>> > the same time? (In the AD spirit.)
>> > 
>> > If we knew the locations/sites and links between them, we could compute
>> > priority groups etc. algorithmicaly. Then only remaining thing is weight,
>> > which can have default and admin do not have to touch it if not necessary.
> You would need to add weights to links, because just the fact there is a
> link tells you nothing about how big the link is between 2 locations, it

Oh, sure, I was thinking about link metric implicitly :-)


> also tells you nothing abut the number of clients in a location which
> may influence how you want to distribute them around.

Do you have an example in mind? It sounds weird to me that you want to
distribute clients outside of local site. If I understand you correctly it
means that the 

Re: [Freeipa-devel] [PATCH] 0008 Refactor test_sudocmdgroup_plugin, create SudoCmdGroupTracker

[Aleš Mareček]

NACK.


[root@master2 test_xmlrpc]# pep8 test_sudocmdgroup_plugin.py 
test_sudocmdgroup_plugin.py:26:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:70:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:76:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:84:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:90:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:98:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:104:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:166:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:180:80: E501 line too long (80 > 79 characters)
test_sudocmdgroup_plugin.py:186:80: E501 line too long (84 > 79 characters)
[root@master2 test_xmlrpc]# pep8 tracker/sudocmdgroup_plugin.py
tracker/sudocmdgroup_plugin.py:36:80: E501 line too long (82 > 79 characters)
tracker/sudocmdgroup_plugin.py:42:80: E501 line too long (82 > 79 characters)
tracker/sudocmdgroup_plugin.py:46:80: E501 line too long (85 > 79 characters)
tracker/sudocmdgroup_plugin.py:55:80: E501 line too long (82 > 79 characters)
tracker/sudocmdgroup_plugin.py:64:80: E501 line too long (82 > 79 characters)



- Original Message -
> From: "Filip Skola" 
> To: "Aleš Mareček" 
> Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> Sent: Monday, February 22, 2016 3:41:36 PM
> Subject: Re: [Freeipa-devel] [PATCH] 0008 Refactor test_sudocmdgroup_plugin, 
> create SudoCmdGroupTracker
> 
> Hi,
> 
> the test has been updated so it now uses the SudoCmdTracker (from the
> previous patch).
> 
> Filip
> 
> - Original Message -
> > NACK.
> > 
> > "create_sudocmd" and "delete_sudocmd" should be imported from Tracker, not
> > from the previous test (sudocmd_plugin).
> > 
> >   - alich -
> > 
> > - Original Message -
> > > From: "Filip Skola" 
> > > To: freeipa-devel@redhat.com
> > > Sent: Thursday, January 28, 2016 12:49:17 PM
> > > Subject: [Freeipa-devel] [PATCH] 0008 Refactor test_sudocmdgroup_plugin,
> > > create SudoCmdGroupTracker
> > > 
> > > Hi,
> > > 
> > > sending the next sudo patch. This one depends on the previous one
> > > (sudocmd_plugin).
> > > 
> > > Filip
> > > 
> > > --
> > > Manage your subscription for the Freeipa-devel mailing list:
> > > https://www.redhat.com/mailman/listinfo/freeipa-devel
> > > Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
> > 
> 

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

Re: [Freeipa-devel] [PATCH 0416-0419] fix broken configuration of sidgen and extdom plugins

[Martin Basti]

On 23.02.2016 17:31, Tomas Babej wrote:


On 02/23/2016 01:25 PM, Martin Basti wrote:


On 23.02.2016 13:02, Alexander Bokovoy wrote:

On Tue, 23 Feb 2016, Martin Basti wrote:

 From f2ae1bd129a1741500d2f3dcb86a0da553604d15 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Tue, 23 Feb 2016 10:37:47 +0100
Subject: [PATCH 4/4] fix upgrade: wait for proper DS socket after DS
restart

Restarting DS executed by upgrade plugin causes that upgrade frameworg
was waiting for not proper socket to be ready. This commit fix issue.

Please fix the commit message typos.


Fixed. Updated patches attached.

ACK.

Tomas

Pushed to master: 0accf8ccb64963954dbe7c137d23f52e5901ac4f
Pushed to ipa-4-3: 4734012c8063460f93f3b819a5bbcca797f6059e
Pushed to ipa-4-2: 63d8caf0d105f02decc0b5d865fedf6ad063bc1a

--
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] Locations design v2: LDAP schema & user interface

[Simo Sorce]

On Tue, 2016-02-23 at 18:04 +0100, Petr Spacek wrote:
> On 23.2.2016 15:19, Simo Sorce wrote:
> > On Tue, 2016-02-23 at 12:43 +0100, Petr Spacek wrote:
> >> On 23.2.2016 11:00, Jan Cholasta wrote:
> >>> Hi,
> >>>
> >>> On 19.2.2016 16:31, Simo Sorce wrote:
>  On Fri, 2016-02-19 at 08:58 +0100, Petr Spacek wrote:
> > On 4.2.2016 18:21, Petr Spacek wrote:
> >> On 3.2.2016 18:41, Petr Spacek wrote:
> >>> Hello,
> >>>
> >>> I've updated the design page
> >>> http://www.freeipa.org/page/V4/DNS_Location_Mechanism
> >>>
> >>> Namely it now contains 'Version 2'.
> >>
> >> Okay, here is the idea how we can make it flexible:
> >> http://www.freeipa.org/page/V4/DNS_Location_Mechanism#Implementation
> >
> > Hello,
> >
> > I'm thinking about LDAP schema for DNS locations.
> >
> > Purpose
> > ===
> > * Allow admins to define any number of locations.
> > * 1 DNS server advertises at most 1 location.
> > * 1 location generally contains set of services with different 
> > priorities and
> > weights (in DNS SRV terms).
> > * Express server & service priority for each defined location in a way 
> > which
> > is granular and flexible and ad the same time easy to manage.
> >
> >
> > Proposal
> > 
> > a) Container for locations
> > --
> > cn=locations,cn=ipa,cn=etc,dc=example,dc=com
> >
> >
> > b) 1 location
> > -
> > Attributes:
> > 2.16.840.1.113730.3.8.5.32 idnsLocationMember
> > Server/service assigned to a DNS Location. Usually used to define 'main'
> > servers for that location. Should it point to service DNs to be sure we 
> > have
> > smooth upgrade to containers?
> >>>
> >>> Services always live on a host (call it server or not), so IMO it makes 
> >>> sense
> >>> to point to servers.
> >>
> >> Fine with me. We just need something which will be able to accommodate
> >> containerization without upgrade headache.
> >>
> >> Do I undestand correctly that 1 container is going to have 1 host object 
> >> with
> >> one service object inside it?
> >>
> >> Like:
> >> cn=container
> >> - cn=DNS, cn=container
> >> ?
> > 
> > Do we think we will ever need to define different locations on a per
> > service basis ?
> 
> No, I hope that we will avoid this.
> 
> I suppose that when we containerize IPA things will get 'more interesting',
> but hopefully not in direction 'location per service'. If we have the
> possibility to have 1 DNS container and 2 CA containers attached to 1 LDAP
> container things will be complicated ...
> 
> I would expect that we will need some additional logic to ensure that one
> location advertises all the services (so e.g. LDAP is not missing in that
> particular location). I would not go beyond that.
> 
> > We based or hypothesis on the fact we only have one location and at most
> > different weights per services ?
> > Is there anything in here that will make it hard for us should we change
> > our mind in future ? (I think the single _tcp DNAME may be an
> > architectural issue anyway, but that could be resolved perhaps by moving
> > the location DNAME on a per service basis in future should we need it ?
> 
> Yes, that is a possible approach but I hope it will not be necessary.
> 
> Hopefully the logic for assigning containers/server to locations can be made
> smart enough to guarantee that we do not need more layers of CNAME/DNAME hacks
> or so.
> 
> The DNAME trick should be seen as a cheap way to emulate views without
> actually using views. If you want more fancy things, go and use views on
> full-featured DNS server ...
> 
> 
> > 2.16.840.1.113730.3.8.5.33 idnsBackupLocation
> > Pointer to another location. Sucks in all servers from that location as 
> > one
> > group with the same priority. Easy to use with _default location where 
> > all
> > 'other' servers are used as backup.
> >
> > These two attributes use sub-type priority and 
> > relativeweight.
> > This is the only way I could express all the information without need 
> > for
> > separate objects.
> >>>
> >>> I don't see the benefit here. What is wrong with separate objects? Why is 
> >>> it
> >>> necessary to reinvent the wheel and abuse attribute sub-types for this, 
> >>> losing
> >>> schema integrity checks provided by DS and making the implementation more
> >>> complex along the way?
> >>
> >> AFAIK Simo did not like separate objects because we could not use 
> >> referential
> >> integrity plugin to prune references to removed servers.
> >>
> >> This can surely be done in framework, I do not insist on subtypes.
> >>
> >>
> >> Talk is cheap, show me your schema :-)
> > 
> > I had a preference, but I m ok also with multiplle objects one per
> > server/service if we think this will make things easier to handle.
> > 
> > Given we are going to need a Server object 

Re: [Freeipa-devel] Locations design v2: LDAP schema & user interface

[Petr Spacek]

On 23.2.2016 15:19, Simo Sorce wrote:
> On Tue, 2016-02-23 at 12:43 +0100, Petr Spacek wrote:
>> On 23.2.2016 11:00, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 19.2.2016 16:31, Simo Sorce wrote:
 On Fri, 2016-02-19 at 08:58 +0100, Petr Spacek wrote:
> On 4.2.2016 18:21, Petr Spacek wrote:
>> On 3.2.2016 18:41, Petr Spacek wrote:
>>> Hello,
>>>
>>> I've updated the design page
>>> http://www.freeipa.org/page/V4/DNS_Location_Mechanism
>>>
>>> Namely it now contains 'Version 2'.
>>
>> Okay, here is the idea how we can make it flexible:
>> http://www.freeipa.org/page/V4/DNS_Location_Mechanism#Implementation
>
> Hello,
>
> I'm thinking about LDAP schema for DNS locations.
>
> Purpose
> ===
> * Allow admins to define any number of locations.
> * 1 DNS server advertises at most 1 location.
> * 1 location generally contains set of services with different priorities 
> and
> weights (in DNS SRV terms).
> * Express server & service priority for each defined location in a way 
> which
> is granular and flexible and ad the same time easy to manage.
>
>
> Proposal
> 
> a) Container for locations
> --
> cn=locations,cn=ipa,cn=etc,dc=example,dc=com
>
>
> b) 1 location
> -
> Attributes:
> 2.16.840.1.113730.3.8.5.32 idnsLocationMember
> Server/service assigned to a DNS Location. Usually used to define 'main'
> servers for that location. Should it point to service DNs to be sure we 
> have
> smooth upgrade to containers?
>>>
>>> Services always live on a host (call it server or not), so IMO it makes 
>>> sense
>>> to point to servers.
>>
>> Fine with me. We just need something which will be able to accommodate
>> containerization without upgrade headache.
>>
>> Do I undestand correctly that 1 container is going to have 1 host object with
>> one service object inside it?
>>
>> Like:
>> cn=container
>> - cn=DNS, cn=container
>> ?
> 
> Do we think we will ever need to define different locations on a per
> service basis ?

No, I hope that we will avoid this.

I suppose that when we containerize IPA things will get 'more interesting',
but hopefully not in direction 'location per service'. If we have the
possibility to have 1 DNS container and 2 CA containers attached to 1 LDAP
container things will be complicated ...

I would expect that we will need some additional logic to ensure that one
location advertises all the services (so e.g. LDAP is not missing in that
particular location). I would not go beyond that.

> We based or hypothesis on the fact we only have one location and at most
> different weights per services ?
> Is there anything in here that will make it hard for us should we change
> our mind in future ? (I think the single _tcp DNAME may be an
> architectural issue anyway, but that could be resolved perhaps by moving
> the location DNAME on a per service basis in future should we need it ?

Yes, that is a possible approach but I hope it will not be necessary.

Hopefully the logic for assigning containers/server to locations can be made
smart enough to guarantee that we do not need more layers of CNAME/DNAME hacks
or so.

The DNAME trick should be seen as a cheap way to emulate views without
actually using views. If you want more fancy things, go and use views on
full-featured DNS server ...


> 2.16.840.1.113730.3.8.5.33 idnsBackupLocation
> Pointer to another location. Sucks in all servers from that location as 
> one
> group with the same priority. Easy to use with _default location where all
> 'other' servers are used as backup.
>
> These two attributes use sub-type priority and 
> relativeweight.
> This is the only way I could express all the information without need for
> separate objects.
>>>
>>> I don't see the benefit here. What is wrong with separate objects? Why is it
>>> necessary to reinvent the wheel and abuse attribute sub-types for this, 
>>> losing
>>> schema integrity checks provided by DS and making the implementation more
>>> complex along the way?
>>
>> AFAIK Simo did not like separate objects because we could not use referential
>> integrity plugin to prune references to removed servers.
>>
>> This can surely be done in framework, I do not insist on subtypes.
>>
>>
>> Talk is cheap, show me your schema :-)
> 
> I had a preference, but I m ok also with multiplle objects one per
> server/service if we think this will make things easier to handle.
> 
> Given we are going to need a Server object in DNS anyway (so that things
> are self contained for non IPA use cases) then I think the referential
> integrity thing goes out the window.

Back to the drawing board! :-)


> [...]
> 
> Attributes:
> 2.16.840.1.113730.3.8.5.34 idnsAdvertisedLocation
> Pointer to a idnsLocation object. On DNS service object / external server.

Re: [Freeipa-devel] [PATCH 0084-0086] CI: Add double circle topology

[Martin Basti]

On 23.02.2016 17:33, Martin Basti wrote:



On 23.02.2016 17:30, Martin Basti wrote:



On 18.02.2016 10:14, David Kupka wrote:

On 12/02/16 16:52, Martin Basti wrote:



On 12.02.2016 13:03, Milan Kubík wrote:

On 02/12/2016 10:59 AM, David Kupka wrote:

Sending one more topology test. This one creates a M groups
consisting N (N>=2) servers.
First two servers in each group are used to connect with nearest 
four

groups and also with the other servers inside the group (when N>2).
Servers inside the group (not connecting to other groups) are
connected with each other.

The patch set needs freeipa-dkupka-8{1,2,3} applied.


ACK


I cannot apply patches, please rebase

[mbasti@dhcp129-96 freeipa-devel]$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
[mbasti@dhcp129-96 freeipa-devel]$ git am freeipa-dkupka-008* -3
Applying: CI: Add '2-connected' topology generator.
Applying: CI: Add simple replication test in 2-connected topology.
Applying: CI: Add test for 2-connected topology generator.
Applying: CI: Add double circle topology.
Applying: CI: Add replication test utilizing double-circle topology.
Applying: CI: Add test for double-circle topology generator.
error: invalid object 100644 e12d141391840cc7f9150a178875393a96dd469b
for 'ipatests/test_integration/test_topologies.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0006 CI: Add test for double-circle topology 
generator.

The copy of the patch that failed is found in:
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am 
--abort".



Martin^2


Git fails to apply patches because wrong version of 
freeipa-dkupka-008{1,2,3} was pushed. Attached patches should fix it.



Sorry my bad.

ACK

Pushed to:
ipa-4-3: ffe3731ae7813fcc3246a83e37d62fc2754bb4ca
master: acdabba6ec0f68841f02c1e6ad65232de81bb505


New test:

Pushed to:
master: a1e582b33c42bcc8a708777afb975e7dc571ee3d
ipa-4-3: 2efa60637111e40a9ac9459d507d9f55a2ae301a


Revert?
IMO this will not work on python3

* Module ipatests.test_integration.test_topologies
ipatests/test_integration/test_topologies.py:124: 
[W1638(range-builtin-not-iterating), test_topology_double_circle_topo] 
range built-in referenced when not iterating)

* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:949: [W0110(deprecated-lambda), 
double_circle_topo] map/filter on lambda could be replaced by comprehension)
ipatests/test_integration/tasks.py:949: 
[W1636(map-builtin-not-iterating), double_circle_topo] map built-in 
referenced when not iterating)
ipatests/test_integration/tasks.py:949: 
[W1637(zip-builtin-not-iterating), double_circle_topo] zip built-in 
referenced when not iterating)


--
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 0420] Set BuildRequires to pylint 1.4

[Lukas Slebodnik]

On (23/02/16 17:09), Martin Basti wrote:
>We cannot guarantee that versions older than 1.4 will work with freeipa code.
>
>Patch attached.

>From a59e72a0b87231c0f2e0d737057550dd532feed7 Mon Sep 17 00:00:00 2001
>From: Martin Basti 
>Date: Tue, 23 Feb 2016 16:58:07 +0100
>Subject: [PATCH] Set BuildRequires to pylint >= 1.4
>
>We can guarantee that only pylint 1.4 and newer will work
>
>https://fedorahosted.org/freeipa/ticket/5615
>---
> freeipa.spec.in | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/freeipa.spec.in b/freeipa.spec.in
>index 
>54a11bfc8cced643c19c29c5ada70bacf7540395..219c5ca2f13eaac14746ec4689ba611bbc6fc377
> 100644
>--- a/freeipa.spec.in
>+++ b/freeipa.spec.in
>@@ -76,7 +76,7 @@ BuildRequires:  python-netaddr
> BuildRequires:  python-gssapi >= 1.1.2
> BuildRequires:  python-rhsm
> BuildRequires:  pyOpenSSL
>-BuildRequires:  pylint >= 1.0
>+BuildRequires:  pylint >= 1.4

I can build rpms even withour pylint and pylint is not executed
anywhere in spec file. (in other words, my patch was rejected)
Why does it need to be in BuildRequires?

LS

-- 
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 0084-0086] CI: Add double circle topology

[Martin Basti]

On 23.02.2016 17:30, Martin Basti wrote:



On 18.02.2016 10:14, David Kupka wrote:

On 12/02/16 16:52, Martin Basti wrote:



On 12.02.2016 13:03, Milan Kubík wrote:

On 02/12/2016 10:59 AM, David Kupka wrote:

Sending one more topology test. This one creates a M groups
consisting N (N>=2) servers.
First two servers in each group are used to connect with nearest four
groups and also with the other servers inside the group (when N>2).
Servers inside the group (not connecting to other groups) are
connected with each other.

The patch set needs freeipa-dkupka-8{1,2,3} applied.


ACK


I cannot apply patches, please rebase

[mbasti@dhcp129-96 freeipa-devel]$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
[mbasti@dhcp129-96 freeipa-devel]$ git am freeipa-dkupka-008* -3
Applying: CI: Add '2-connected' topology generator.
Applying: CI: Add simple replication test in 2-connected topology.
Applying: CI: Add test for 2-connected topology generator.
Applying: CI: Add double circle topology.
Applying: CI: Add replication test utilizing double-circle topology.
Applying: CI: Add test for double-circle topology generator.
error: invalid object 100644 e12d141391840cc7f9150a178875393a96dd469b
for 'ipatests/test_integration/test_topologies.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0006 CI: Add test for double-circle topology generator.
The copy of the patch that failed is found in:
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Martin^2


Git fails to apply patches because wrong version of 
freeipa-dkupka-008{1,2,3} was pushed. Attached patches should fix it.



Sorry my bad.

ACK

Pushed to:
ipa-4-3: ffe3731ae7813fcc3246a83e37d62fc2754bb4ca
master: acdabba6ec0f68841f02c1e6ad65232de81bb505


New test:

Pushed to:
master: a1e582b33c42bcc8a708777afb975e7dc571ee3d
ipa-4-3: 2efa60637111e40a9ac9459d507d9f55a2ae301a

--
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 0416-0419] fix broken configuration of sidgen and extdom plugins

[Tomas Babej]

On 02/23/2016 01:25 PM, Martin Basti wrote:
> 
> 
> On 23.02.2016 13:02, Alexander Bokovoy wrote:
>> On Tue, 23 Feb 2016, Martin Basti wrote:
>>> From f2ae1bd129a1741500d2f3dcb86a0da553604d15 Mon Sep 17 00:00:00 2001
>>> From: Martin Basti 
>>> Date: Tue, 23 Feb 2016 10:37:47 +0100
>>> Subject: [PATCH 4/4] fix upgrade: wait for proper DS socket after DS
>>> restart
>>>
>>> Restarting DS executed by upgrade plugin causes that upgrade frameworg
>>> was waiting for not proper socket to be ready. This commit fix issue.
>> Please fix the commit message typos.
>>
> Fixed. Updated patches attached.

ACK.

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 0084-0086] CI: Add double circle topology

[Martin Basti]

On 18.02.2016 10:14, David Kupka wrote:

On 12/02/16 16:52, Martin Basti wrote:



On 12.02.2016 13:03, Milan Kubík wrote:

On 02/12/2016 10:59 AM, David Kupka wrote:

Sending one more topology test. This one creates a M groups
consisting N (N>=2) servers.
First two servers in each group are used to connect with nearest four
groups and also with the other servers inside the group (when N>2).
Servers inside the group (not connecting to other groups) are
connected with each other.

The patch set needs freeipa-dkupka-8{1,2,3} applied.


ACK


I cannot apply patches, please rebase

[mbasti@dhcp129-96 freeipa-devel]$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
[mbasti@dhcp129-96 freeipa-devel]$ git am freeipa-dkupka-008* -3
Applying: CI: Add '2-connected' topology generator.
Applying: CI: Add simple replication test in 2-connected topology.
Applying: CI: Add test for 2-connected topology generator.
Applying: CI: Add double circle topology.
Applying: CI: Add replication test utilizing double-circle topology.
Applying: CI: Add test for double-circle topology generator.
error: invalid object 100644 e12d141391840cc7f9150a178875393a96dd469b
for 'ipatests/test_integration/test_topologies.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0006 CI: Add test for double-circle topology generator.
The copy of the patch that failed is found in:
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Martin^2


Git fails to apply patches because wrong version of 
freeipa-dkupka-008{1,2,3} was pushed. Attached patches should fix it.



Sorry my bad.

ACK

Pushed to:
ipa-4-3: ffe3731ae7813fcc3246a83e37d62fc2754bb4ca
master: acdabba6ec0f68841f02c1e6ad65232de81bb505

--
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 0413] fix permission: Read Replication Agreements

[Martin Basti]

On 22.02.2016 09:00, Jan Cholasta wrote:

Hi,

On 17.2.2016 14:49, Martin Basti wrote:

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

Patch attached (for master, 4.3, 4.2)


1) All the replication agreement permission ACIs should be located in 
the same entry. Currently "Read Replication Agreements" is in 
"cn=config" and everything else in "cn=mapping tree,cn=config", so I 
guess "cn=mapping tree,cn=config" makes more sense.



2) Instead of literal DN('cn=permissions,cn=pbac'), use 
api.env.container_permissions.



3) IMO the removal of managed permission attributes could be a little 
bit more robust. You should check that the original entry contains all 
the required values before touching it (objectclass=ipapermissionv2, 
ipapermissiontype=V2, ipapermissiontype=MANAGED) and remove only the 
values that need to be removed, instead of just overwriting everything.



Honza


Updated patch attached.
From 4332e5d4b52d13b0d08d57691b77bc892999303b Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 4 Feb 2016 16:23:40 +0100
Subject: [PATCH] fix permission: Read Replication Agreements

This permission cannot be MANAGED permission because it is located in
nonreplicating part of the LDAP tree.

As side effect, the particular ACI has not been created on all replicas.

This commit makes Read Replication Agreements non managed permission and
also fix missing ACI on replicas.

https://fedorahosted.org/freeipa/ticket/5631
---
 ACI.txt|   2 -
 install/share/delegation.ldif  |   9 ++
 install/share/replica-acis.ldif|   5 +
 install/updates/20-aci.update  |   4 +-
 install/updates/90-post_upgrade_plugins.update |   1 +
 .../install/plugins/update_managed_permissions.py  | 133 +++--
 6 files changed, 90 insertions(+), 64 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index bbc2e660cc0a549421069f2f569f8b1dbf42c724..24cb332ce6e10c82a5bfab76d084fb6c0277800d 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -388,8 +388,6 @@ dn: cn=Domain Level,cn=ipa,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "createtimestamp || entryusn || ipadomainlevel || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipadomainlevelconfig)")(version 3.0;acl "permission:System: Read Domain Level";allow (compare,read,search) userdn = "ldap:///all;;)
 dn: cn=masters,cn=ipa,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || entryusn || ipaconfigstring || modifytimestamp || objectclass")(targetfilter = "(objectclass=nscontainer)")(version 3.0;acl "permission:System: Read IPA Masters";allow (compare,read,search) groupdn = "ldap:///cn=System: Read IPA Masters,cn=permissions,cn=pbac,dc=ipa,dc=example";)
-dn: cn=config
-aci: (targetattr = "cn || createtimestamp || description || entryusn || modifytimestamp || nsds50ruv || nsds5beginreplicarefresh || nsds5debugreplicatimeout || nsds5flags || nsds5replicaabortcleanruv || nsds5replicaautoreferral || nsds5replicabackoffmax || nsds5replicabackoffmin || nsds5replicabinddn || nsds5replicabindmethod || nsds5replicabusywaittime || nsds5replicachangecount || nsds5replicachangessentsincestartup || nsds5replicacleanruv || nsds5replicacleanruvnotified || nsds5replicacredentials || nsds5replicaenabled || nsds5replicahost || nsds5replicaid || nsds5replicalastinitend || nsds5replicalastinitstart || nsds5replicalastinitstatus || nsds5replicalastupdateend || nsds5replicalastupdatestart || nsds5replicalastupdatestatus || nsds5replicalegacyconsumer || nsds5replicaname || nsds5replicaport || nsds5replicaprotocoltimeout || nsds5replicapurgedelay || nsds5replicareferral || nsds5replicaroot || nsds5replicasessionpausetime || nsds5replicastripattrs || nsds5replicatedattributelist || nsds5replicatedattributelisttotal || nsds5replicatimeout || nsds5replicatombstonepurgeinterval || nsds5replicatransportinfo || nsds5replicatype || nsds5replicaupdateinprogress || nsds5replicaupdateschedule || nsds5task || nsds7directoryreplicasubtree || nsds7dirsynccookie || nsds7newwingroupsyncenabled || nsds7newwinusersyncenabled || nsds7windowsdomain || nsds7windowsreplicasubtree || nsruvreplicalastmodified || nsstate || objectclass || onewaysync || winsyncdirectoryfilter || winsyncinterval || winsyncmoveaction || winsyncsubtreepair || winsyncwindowsfilter")(targetfilter = "(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0;acl "permission:System: Read Replication Agreements";allow (compare,read,search) groupdn = "ldap:///cn=System: Read Replication Agreements,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=replication,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || entryusn || modifytimestamp || nsds5flags || nsds5replicaabortcleanruv || nsds5replicaautoreferral || nsds5replicabackoffmax || nsds5replicabackoffmin || nsds5replicabinddn || 

Re: [Freeipa-devel] [PATCH 0414] py3: remove usage of iteritems

[Martin Basti]

On 23.02.2016 17:13, Martin Babinsky wrote:

On 02/17/2016 04:54 PM, Martin Basti wrote:

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

Patch attached.




ACK


Pushed to:
master: 697072cac9d94ebd006d6a0f595d7956d38d58b9
ipa-4-3: 93582ac57533b2c2bb3079da2ebc48b67b2c1655

--
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 0414] py3: remove usage of iteritems

[Martin Babinsky]

On 02/17/2016 04:54 PM, Martin Basti wrote:

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

Patch attached.




ACK

--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCH 0415] Pylint: disable new pylint checks

[Martin Basti]

On 19.02.2016 13:00, David Kupka wrote:

On 17/02/16 18:02, Martin Basti wrote:

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

Disable unwanted or false positive checks.

New checks cannot be disabled inline because pylint < 1.5 will report
them as errors, thus must be disable globally until we decide to move
completely to pytlint 1.5 (Fedora 25?)

Patches attached.



Works for me, ACK.

All tree branches tested on F23 with pylint 1.4.3-3


Pushed to master: ddda062d58fd9c3c6b4edbea0afb236fc26024ba
Pushed to ipa-4-3: 963ce7f117b0cd69590327350b893e755cc38856
Pushed to ipa-4-2: 9136d72ae1feff15a80f162d06a837e7f8cee7ea

--
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 0135] upgrade: unconditional import of certificate profiles into LDAP

[Jan Cholasta]

On 23.2.2016 09:55, Martin Babinsky wrote:

On 02/23/2016 07:43 AM, Fraser Tweedale wrote:

On Tue, Feb 23, 2016 at 07:32:31AM +0100, Jan Cholasta wrote:

On 23.2.2016 06:40, Fraser Tweedale wrote:

On Mon, Feb 22, 2016 at 02:03:49PM +0100, Martin Babinsky wrote:

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

--
Martin^3 Babinsky


Thanks for the patch.  Conditional ACK.

Patch is tested and works, but I am wary about checking for
substring match against RemoteRetrieveError reason string (see hunk
below).  It would be better to carry the status code as an attribute
of RemoteRetrieveError and check whether it is 409.


+except errors.RemoteRetrieveError as e:
+if "Profile already exists" in e.reason:
+root_logger.debug("Profile '{}' already
present".format(
+profile_id))
+else:
+root_logger.error("Error migrating profile '{}':
{}".format(
+profile_id, e))
+return


If you agree, we can file a ticket and I am happy for these patches
to be merged as-is.  The scope of changing RemoteRetrieveError is
larger than #5682 so it makes sense to do it separately, and just
for master branch.


I don't think this is the right approach. create_profile() should raise
DuplicateEntry rather than RemoteRetrieveError if the profile already
exists, which can then be properly handled in _create_dogtag_profile().


I am happy with that.



Attaching updated patch. Unfortunately, REST API does report only a
super-generic error code 400: Bad request when adding profile which
already exists, which is not very useful for us (I will open a ticket
for Dogtag for this).

After consultation with Jan, we decided to log the error at debug level
and be done with it (for the moment).


Thanks, ACK.

Pushed to:
master: 2c3b0b1bcd972e6beec4691c03830f37dd27e199
ipa-4-2: 704319c3eaf74e0531dd2aa1e5880db7b6ab830c
ipa-4-3: e0ce7e37636969af56a4fa2b1068ae97f1058bdd

--
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 0011] Move freeipa certmonger helpers to libexecdir.

[Rob Crittenden]

David Kupka wrote:
> On 23/02/16 10:14, Martin Kosek wrote:
>> On 02/23/2016 09:47 AM, David Kupka wrote:
>>> On 22/02/16 16:15, Martin Kosek wrote:
 On 02/22/2016 04:04 PM, Jan Cholasta wrote:
> On 22.2.2016 15:56, David Kupka wrote:
>> On 22/02/16 07:28, Jan Cholasta wrote:
>>> On 18.2.2016 10:10, David Kupka wrote:
 On 19/01/16 16:10, David Kupka wrote:
> On 19/01/16 14:38, Jan Cholasta wrote:
>> On 19.1.2016 14:26, Martin Kosek wrote:
>>> On 01/19/2016 01:47 PM, David Kupka wrote:
 I've polished the patch attached to #5586 by Timo Aaltonen.

 Thanks for the patch. I've fixed the path in specfile and
 removed
 unused import
 but otherwise it works, ACK.

 https://fedorahosted.org/freeipa/ticket/5586
>>>
>>> Won't this break existing certmonger requests depending on
>>> the old
>>> path?
>>
>> It will, I don't see any upgrade code.
>>
>>>
>>> # getcert list | grep '/usr/lib64/ipa/certmonger'
>>>   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
>>> "auditSigningCert
>>> cert-pki-ca"
>>>   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
>>> "ocspSigningCert
>>> cert-pki-ca"
>>>   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
>>> "subsystemCert
>>> cert-pki-ca"
>>>   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
>>> "caSigningCert
>>> cert-pki-ca"
>>>   post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert
>>>   pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>   post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
>>> "Server-Cert
>>> cert-pki-ca"
>>>   post-save command:
>>> /usr/lib64/ipa/certmonger/restart_dirsrv
>>> RHEL72
>>>   post-save command: /usr/lib64/ipa/certmonger/restart_httpd
>>>
>>
>>
>
> You're right it will break the upgrade. I haven't noticed that
> Server-Cert for DS and HTTPD are not handled by
> certificate_renewal_update (ipaserver.install.server.upgrade)
> where all
> the other trackings are stopped and then configured again with the
> paths.CERTMONGER_COMMAND_TEMPLATE already updated.
>
> Thanks for the catch.
>

 I've updated Timo's patch little more and added
 start_tracking_certificates() for dsinstance and httpinstance.
 Now the
 upgrade works as expected.
>>>
>>> The way the patches are split is kind of weird and apparently
>>> confusing
>>> (see the other thread). IMO there should be 2 patches: the first
>>> should
>>> add the ability to change DS and HTTP certmonger config during
>>> upgrade
>>> (i.e. the start_tracking_certificates() methods and
>>> certificate_renewal_update() changes), the second should move the
>>> helpers (i.e. the actual move and certificate_renewal_update()
>>> version
>>> bump).
>>>
>> Honza, do I understand it correctly that the code is OK but I did not
>> split it to the patches correctly?
>
> Yes.

 Before acking or pushing, can you please explain for me how the
 upgrade of
 certmonger tracking requests work? I want to make sure this is
 right, so please
 bear with me:

 1) How does it edit existing tracking requests with the new helper
 paths?

 2) Does it go and try to edit the requests on every upgrade? Or is
 there some
 check that requests were updated?

 Thanks,
 Martin

>>>
>>> Whole upgrade of renewal requests is done in
>>> ipaserver/install/server/upgrade.py in certificate_renewal_upgrade().
>>>
>>> First there is version of requests and if it's the same as in state file
>>> upgrade is skipped.
>>> Then every request is searched over certmonger's DBus interface and
>>> if at least
>>> one is not found it means that there was change in request
>>> configuration. All
>>> tracking requests are then stopped and started again with new
>>> configuration.
>>>
>>> So to answer you questions:
>>> 1) By stopping the old request with the old parameters (including
>>> path) and
>>> starting new with new parameters.
>>>
>>> 2) Only if version was bumped which happens only if some of the
>>> requests changes.
>>
>> Ah, so IIUC, if you bump the version, requests should be properly
>> updated. The
>> change 

Re: [Freeipa-devel] Locations design v2: LDAP schema & user interface

[Petr Vobornik]

On 02/19/2016 04:31 PM, Simo Sorce wrote:

On Fri, 2016-02-19 at 08:58 +0100, Petr Spacek wrote:

On 4.2.2016 18:21, Petr Spacek wrote:

On 3.2.2016 18:41, Petr Spacek wrote:

Hello,

I've updated the design page
http://www.freeipa.org/page/V4/DNS_Location_Mechanism

Namely it now contains 'Version 2'.


Okay, here is the idea how we can make it flexible:
http://www.freeipa.org/page/V4/DNS_Location_Mechanism#Implementation


Hello,

I'm thinking about LDAP schema for DNS locations.

Purpose
===
* Allow admins to define any number of locations.
* 1 DNS server advertises at most 1 location.
* 1 location generally contains set of services with different priorities and
weights (in DNS SRV terms).
* Express server & service priority for each defined location in a way which
is granular and flexible and ad the same time easy to manage.


Proposal

a) Container for locations
--
cn=locations,cn=ipa,cn=etc,dc=example,dc=com


b) 1 location
-
Attributes:
2.16.840.1.113730.3.8.5.32 idnsLocationMember
Server/service assigned to a DNS Location. Usually used to define 'main'
servers for that location. Should it point to service DNs to be sure we have
smooth upgrade to containers?

2.16.840.1.113730.3.8.5.33 idnsBackupLocation
Pointer to another location. Sucks in all servers from that location as one
group with the same priority. Easy to use with _default location where all
'other' servers are used as backup.

These two attributes use sub-type priority and relativeweight.
This is the only way I could express all the information without need for
separate objects.


Object classes:
2.16.840.1.113730.3.8.6.7  idnsLocation
MAY ( idnsLocationMember $ idnsBackupLocation )


1st example:
Location CZ:
- servers czserver1, czserver2
- priority=1
- relative weight = 50 % each
- if both CZ servers fail, use servers in location UK as backup (priority 2)
- if all CZ and UK servers fail, use servers in location US as backup
(priority 3) - servers on the other continent are used only as option of last
resort
DN: cn=cz,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
objectClass: idnsLocation
idnsLocationMember;priority1;relativeweight50:
cn=czserver1,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsLocationMember;priority1;relativeweight50:
cn=czserver2,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority2: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority3: cn=us,cn=locations,cn=ipa,cn=etc,dc=example,dc=com

Location UK:
- servers ukserver1, ukserver2
- priority=1
- server ukserver1 is a new beefy machine so it can handle 3 times more load
than ukserver2, thus relative weights 75 % and 25 %
- if both UK servers fail, use servers in location CZ as backup (priority 2)
- if all CZ and UK servers fail, use servers in location US as backup
(priority 3) - servers on the other continent are used only as option of last
resort
DN: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
objectClass: idnsLocation
idnsLocationMember;priority1;relativeweight3:
cn=ukserver1,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsLocationMember;priority1;relativeweight1:
cn=ukserver2,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority2: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority3: cn=us,cn=locations,cn=ipa,cn=etc,dc=example,dc=com

Location US:
- servers usserver1, usserver2
- priority=1
- relative weight = 50 % each
- if both US servers fail, use servers in location CZ and UK as backup
(priority 2) - it is over ocean anyway, so US clients will not make any
difference between CZ and UK locations
DN: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
objectClass: idnsLocation
idnsLocationMember;priority1;relativeweight50:
cn=ukserver1,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsLocationMember;priority1;relativeweight50:
cn=ukserver2,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority2: cn=cz,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority2: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com


Resulting DNS SRV records (generated by FreeIPA framework). Please note that
only numbers in SRV records matter only relatively. Priorities work as group,
weights are relative only inside the group. Absolute values above are used
only in algorithm which generates SRV records:
Location CZ:
_kerberos._udp SRV 1 50 czserver1
_kerberos._udp SRV 1 50 czserver2
_kerberos._udp SRV 2 75 ukserver1
_kerberos._udp SRV 2 25 ukserver1
_kerberos._udp SRV 3 50 usserver1
_kerberos._udp SRV 3 50 usserver2

Location UK:
_kerberos._udp SRV 1 75 ukserver1
_kerberos._udp SRV 1 25 ukserver1
_kerberos._udp SRV 2 50 czserver1
_kerberos._udp SRV 2 50 czserver2
_kerberos._udp SRV 3 50 usserver1
_kerberos._udp SRV 3 50 usserver2

Location US:
_kerberos._udp SRV 1 50 usserver1
_kerberos._udp SRV 1 50 usserver2
_kerberos._udp SRV 2 250 czserver1
_kerberos._udp SRV 2 250 czserver2
_kerberos._udp SRV 2 

Re: [Freeipa-devel] [PATCH 0012] Use HTTPD_USER in dogtaginstance.py

[Martin Basti]

On 19.01.2016 13:48, David Kupka wrote:

I've converted the diff attached to #5587 by Timo Aaltonen.
Works for me, ACK.

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



Patch has been pushed to master, original email is probably lost.

67c367d0db194d9afa56ecda34dafb46758d99b5 Use HTTPD_USER in dogtaginstance.py

Also patch had not been pushed to ipa-4-3, fixed:
Pushed to ipa-4-3: 073fdeb57465044bbce981672027253c2dd44c0f
-- 
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] Refactor test_hostgroup_plugin

[Martin Basti]

On 22.12.2015 11:57, Filip Skola wrote:

And also sending refactored hostgroup plugin test.

F



bump for review
-- 
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] 811 performance: faster DN implementation

[Martin Basti]

On 22.12.2015 16:59, Petr Spacek wrote:

On 14.4.2015 19:32, Petr Vobornik wrote:

Pushed to master: 11bd9d96f191066f7ba760549f00179c128a9787


Please be so kind and fix naming. AFAIK the patch refers to 'openldap' DN
format but in fact it is python-ldap-ishm. It will surely confuse next
generation of FreeIPA developers :-)


Will be in separate patch.

Petr, Santa found out that you did not send the patch! You are risking
Christmas without any presents ... :-)


Bump for patch.

--
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] 0048 Decode HTTP reason phrase as iso-8859-1

[Jan Cholasta]

On 13.1.2016 08:12, Jan Cholasta wrote:

On 8.1.2016 11:56, Fraser Tweedale wrote:

On Thu, Jan 07, 2016 at 08:00:51PM +1000, Fraser Tweedale wrote:

On Thu, Jan 07, 2016 at 07:56:15AM +0100, Jan Cholasta wrote:

Hi,

On 6.1.2016 05:26, Fraser Tweedale wrote:

Happy new year, all.

The attached patch fixes a unicode decode error triggered in some
locales, which causes failure of installation (and probably other
oprations, if locale is changed under an existing server).

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


It seems like this fixes only part of the issue - the installer
won't crash
anymore. But what happens if the reason phrase uses characters which
are not
in iso-8859-1 (e.g. "č", a character commonly used in Czech)?
Shouldn't we
always specify the encoding in requests, so that Dogtag does not
have to
guess?


In this case it will not throw an exception, but it will decode
nonsense.  However, in my investigation just now of how Tomcat
decides what to send in the reason phrase, it turns out that in
future releases they will not send a reason phrase[1] at all!

[1]
https://github.com/apache/tomcat/commit/707ab1c77f3bc189e1c3f29b641506db4c8bce37


(Nice to know about this in advance - I will not be surprised if
some clients break)

I'll cut a new patch tomorrow that just ignores the reason phrase
rather than trying to decode and log it.  All the info is in the
status code, after all.

Thanks for reviewing,
Fraser


Promised new patch attached - removes the reason phrase and updates
call sites accordingly.


Thanks, ACK.

Pushed to master: fe94222873c4df5118e93cebe7e9d69439266ba0


and to:
ipa-4-2: 0aedaf10e1e3fe48df5f57ad0f25c80b91c2eb45
ipa-4-3: 45f7762a9229ff867700432413c434e61fdddc0d

--
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] [TESTS][PATCH 0010] WebUI tests - ID views

[Lenka Doudova]

Hi,

attached is patch providing missing test coverage for ID views in webUI.

Lenka
From 5940a3e7f63b7b6360a28fd52ba6c7df65e4ea98 Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Fri, 19 Feb 2016 14:59:19 +0100
Subject: [PATCH] WebUI test: ID views

Provides missing test coverage for ID views web UI.
---
 ipatests/test_webui/data_idviews.py |  20 ++
 ipatests/test_webui/test_idviews.py | 127 
 ipatests/test_webui/ui_driver.py|  48 --
 3 files changed, 189 insertions(+), 6 deletions(-)
 create mode 100644 ipatests/test_webui/data_idviews.py
 create mode 100644 ipatests/test_webui/test_idviews.py

diff --git a/ipatests/test_webui/data_idviews.py b/ipatests/test_webui/data_idviews.py
new file mode 100644
index ..9d62f33fe47dcf2cbde525d687ddd80c50b367e0
--- /dev/null
+++ b/ipatests/test_webui/data_idviews.py
@@ -0,0 +1,20 @@
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+ENTITY = 'idview'
+USER_FACET = 'idoverrideuser'
+GROUP_FACET = 'idoverridegroup'
+HOST_FACET = 'appliedtohosts'
+
+PKEY = 'itest-view'
+DATA = {
+'pkey': PKEY,
+'add': [
+('textbox', 'cn', PKEY),
+('textarea', 'description', 'Description of ID view'),
+],
+'mod': [
+('textarea', 'description', 'Different description'),
+],
+}
diff --git a/ipatests/test_webui/test_idviews.py b/ipatests/test_webui/test_idviews.py
new file mode 100644
index ..b7f5c312eb2a5a7680b55126ae91446b5010a5b8
--- /dev/null
+++ b/ipatests/test_webui/test_idviews.py
@@ -0,0 +1,127 @@
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+from ipatests.test_webui.ui_driver import UI_driver
+from ipatests.test_webui.ui_driver import screenshot
+import ipatests.test_webui.data_idviews as idview
+import ipatests.test_webui.data_user as user
+import ipatests.test_webui.data_group as group
+import ipatests.test_webui.data_hostgroup as hostgroup
+from ipatests.test_webui.test_host import host_tasks, ENTITY as HOST_ENTITY
+import pytest
+
+DATA_USER = {
+'pkey': user.PKEY,
+'add': [
+('combobox', 'ipaanchoruuid', user.PKEY),
+('textbox', 'uid', 'iduser'),
+('textbox', 'gecos', 'id user'),
+('textbox', 'uidnumber', 1),
+('textbox', 'gidnumber', 1),
+('textbox', 'loginshell', 'shell'),
+('textbox', 'homedirectory', 'home'),
+('textarea', 'description', 'desc'),
+],
+'mod': [
+('textbox', 'uid', 'moduser'),
+('textbox', 'uidnumber', 3),
+],
+}
+
+DATA_GROUP = {
+'pkey': group.PKEY,
+'add': [
+('combobox', 'ipaanchoruuid', group.PKEY),
+('textbox', 'cn', 'idgroup'),
+('textbox', 'gidnumber', 2),
+('textarea', 'description', 'desc'),
+],
+'mod': [
+('textbox', 'cn', 'modgroup'),
+('textbox', 'gidnumber', 3),
+],
+}
+
+
+@pytest.mark.tier1
+class test_idviews(UI_driver):
+
+@screenshot
+def test_crud(self):
+"""
+Basic CRUD: ID view
+"""
+self.init_app()
+self.basic_crud(
+idview.ENTITY, idview.DATA, default_facet=idview.USER_FACET)
+
+@screenshot
+def test_overrides(self):
+"""
+User and group overrides
+"""
+self.init_app()
+
+self.add_record(user.ENTITY, user.DATA, navigate=False)
+self.add_record(group.ENTITY, group.DATA)
+self.add_record(idview.ENTITY, idview.DATA)
+
+self.navigate_to_record(idview.PKEY)
+parent_entity = 'idview'
+
+# user override
+self.add_record(parent_entity, DATA_USER, facet=idview.USER_FACET)
+self.navigate_to_record(user.PKEY)
+self.mod_record(idview.USER_FACET, DATA_USER)
+self.delete_action(idview.ENTITY, user.PKEY)
+
+# group override
+self.navigate_to_record(idview.PKEY)
+self.switch_to_facet(idview.GROUP_FACET)
+self.add_record(parent_entity, DATA_GROUP, facet=idview.GROUP_FACET)
+self.navigate_to_record(group.PKEY)
+self.mod_record(idview.GROUP_FACET, DATA_GROUP)
+self.delete_action(idview.ENTITY, group.PKEY)
+
+# cleanup
+self.delete(idview.ENTITY, [idview.DATA])
+self.delete(user.ENTITY, [user.DATA])
+self.delete(group.ENTITY, [group.DATA])
+
+@screenshot
+def test_hosts(self):
+"""
+Apply to hosts and host groups
+"""
+self.init_app()
+host = host_tasks()
+host.setup(self.driver, self.config)
+
+self.add_record(HOST_ENTITY, host.data)
+self.add_record(hostgroup.ENTITY, hostgroup.DATA)
+self.navigate_to_record(hostgroup.PKEY)
+self.add_associations([host.pkey])
+self.add_record(idview.ENTITY, idview.DATA)
+
+self.navigate_to_record(idview.PKEY)
+

Re: [Freeipa-devel] Locations design v2: LDAP schema & user interface

[Simo Sorce]

On Tue, 2016-02-23 at 12:43 +0100, Petr Spacek wrote:
> On 23.2.2016 11:00, Jan Cholasta wrote:
> > Hi,
> > 
> > On 19.2.2016 16:31, Simo Sorce wrote:
> >> On Fri, 2016-02-19 at 08:58 +0100, Petr Spacek wrote:
> >>> On 4.2.2016 18:21, Petr Spacek wrote:
>  On 3.2.2016 18:41, Petr Spacek wrote:
> > Hello,
> >
> > I've updated the design page
> > http://www.freeipa.org/page/V4/DNS_Location_Mechanism
> >
> > Namely it now contains 'Version 2'.
> 
>  Okay, here is the idea how we can make it flexible:
>  http://www.freeipa.org/page/V4/DNS_Location_Mechanism#Implementation
> >>>
> >>> Hello,
> >>>
> >>> I'm thinking about LDAP schema for DNS locations.
> >>>
> >>> Purpose
> >>> ===
> >>> * Allow admins to define any number of locations.
> >>> * 1 DNS server advertises at most 1 location.
> >>> * 1 location generally contains set of services with different priorities 
> >>> and
> >>> weights (in DNS SRV terms).
> >>> * Express server & service priority for each defined location in a way 
> >>> which
> >>> is granular and flexible and ad the same time easy to manage.
> >>>
> >>>
> >>> Proposal
> >>> 
> >>> a) Container for locations
> >>> --
> >>> cn=locations,cn=ipa,cn=etc,dc=example,dc=com
> >>>
> >>>
> >>> b) 1 location
> >>> -
> >>> Attributes:
> >>> 2.16.840.1.113730.3.8.5.32 idnsLocationMember
> >>> Server/service assigned to a DNS Location. Usually used to define 'main'
> >>> servers for that location. Should it point to service DNs to be sure we 
> >>> have
> >>> smooth upgrade to containers?
> > 
> > Services always live on a host (call it server or not), so IMO it makes 
> > sense
> > to point to servers.
> 
> Fine with me. We just need something which will be able to accommodate
> containerization without upgrade headache.
> 
> Do I undestand correctly that 1 container is going to have 1 host object with
> one service object inside it?
> 
> Like:
> cn=container
> - cn=DNS, cn=container
> ?

Do we think we will ever need to define different locations on a per
service basis ?
We based or hypothesis on the fact we only have one location and at most
different weights per services ?
Is there anything in here that will make it hard for us should we change
our mind in future ? (I think the single _tcp DNAME may be an
architectural issue anyway, but that could be resolved perhaps by moving
the location DNAME on a per service basis in future should we need it ?

> >>> 2.16.840.1.113730.3.8.5.33 idnsBackupLocation
> >>> Pointer to another location. Sucks in all servers from that location as 
> >>> one
> >>> group with the same priority. Easy to use with _default location where all
> >>> 'other' servers are used as backup.
> >>>
> >>> These two attributes use sub-type priority and 
> >>> relativeweight.
> >>> This is the only way I could express all the information without need for
> >>> separate objects.
> > 
> > I don't see the benefit here. What is wrong with separate objects? Why is it
> > necessary to reinvent the wheel and abuse attribute sub-types for this, 
> > losing
> > schema integrity checks provided by DS and making the implementation more
> > complex along the way?
> 
> AFAIK Simo did not like separate objects because we could not use referential
> integrity plugin to prune references to removed servers.
> 
> This can surely be done in framework, I do not insist on subtypes.
> 
> 
> Talk is cheap, show me your schema :-)

I had a preference, but I m ok also with multiplle objects one per
server/service if we think this will make things easier to handle.

Given we are going to need a Server object in DNS anyway (so that things
are self contained for non IPA use cases) then I think the referential
integrity thing goes out the window.

[...]

> >>> Attributes:
> >>> 2.16.840.1.113730.3.8.5.34 idnsAdvertisedLocation
> >>> Pointer to a idnsLocation object. On DNS service object / external server.
> >>> Single-valued.
> > 
> > IMO this should be attribute of server rather than service,
> > given that
> > idnsLocationMember points to servers rather than services.
> 
> The main reason is why idnsAdvertisedLocation is tied to DNS service is that a
> IPA server without a DNS server cannot advertise anything, so the attribute
> does not make sense on all server objects.
> 
> Also, the attribute can be (in future) used on external DNS server. The
> external server is not going to be an IPA server, it will be just
> representation of a DNS endpoint. Likely this external DNS server is not going
> to reside in cn=masters at all. It might be in cn=dns so somewhere else.

If possible I'd tie it to the DNS server's DNS object, or a new object
in the DNS hierarchy.
We may want to have locations for servers that are not IPA Server at
all, like the preferred local XMPP server or other things like that.

> So it seemed to me that it would be good to tie this to 'DNS endpoint' object
> instead of IPA server object.
> 
> 

Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin

[Aleš Mareček]

NACK.
Some little changes still required:
 * fixing the pep8 errors
 * fixing the wrong comment

[root@master2 freeipa]# pep8 ipatests/test_xmlrpc/test_sudocmd_plugin.py
ipatests/test_xmlrpc/test_sudocmd_plugin.py:94:80: E501 line too long (87 > 79 
characters)
ipatests/test_xmlrpc/test_sudocmd_plugin.py:97:80: E501 line too long (87 > 79 
characters)
ipatests/test_xmlrpc/test_sudocmd_plugin.py:134:80: E501 line too long (80 > 79 
characters)
  
[root@master2 freeipa]# pep8 ipatests/test_xmlrpc/tracker/sudocmd_plugin.py 
ipatests/test_xmlrpc/tracker/sudocmd_plugin.py:14:80: E501 line too long (81 > 
79 characters)

[root@master2 freeipa]# grep 'Class for' 
ipatests/test_xmlrpc/tracker/sudocmd_plugin.py
""" Class for host plugin like tests """


- Original Message -
> From: "Filip Skola" 
> To: "Aleš Mareček" 
> Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> Sent: Monday, February 22, 2016 1:59:43 PM
> Subject: Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
> 
> Hi,
> 
> sudocmd tracker has been created.
> 
> Filip
> 
> - Original Message -
> > NACK.
> > 
> > "create_sudocmd" and "delete_sudocmd" should be placed in Tracker. So this
> > patch should create the tracker as well.
> > 
> > - Original Message -
> > > From: "Filip Skola" 
> > > To: freeipa-devel@redhat.com
> > > Sent: Monday, January 25, 2016 3:57:25 PM
> > > Subject: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
> > > 
> > > Hello,
> > > 
> > > attaching refactored sudocmd_plugin.
> > > 
> > > Filip
> > > --
> > > Manage your subscription for the Freeipa-devel mailing list:
> > > https://www.redhat.com/mailman/listinfo/freeipa-devel
> > > Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
> > 
> 

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

Re: [Freeipa-devel] [PATCH 0011] Move freeipa certmonger helpers to libexecdir.

[David Kupka]

On 23/02/16 10:14, Martin Kosek wrote:

On 02/23/2016 09:47 AM, David Kupka wrote:

On 22/02/16 16:15, Martin Kosek wrote:

On 02/22/2016 04:04 PM, Jan Cholasta wrote:

On 22.2.2016 15:56, David Kupka wrote:

On 22/02/16 07:28, Jan Cholasta wrote:

On 18.2.2016 10:10, David Kupka wrote:

On 19/01/16 16:10, David Kupka wrote:

On 19/01/16 14:38, Jan Cholasta wrote:

On 19.1.2016 14:26, Martin Kosek wrote:

On 01/19/2016 01:47 PM, David Kupka wrote:

I've polished the patch attached to #5586 by Timo Aaltonen.

Thanks for the patch. I've fixed the path in specfile and removed
unused import
but otherwise it works, ACK.

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


Won't this break existing certmonger requests depending on the old
path?


It will, I don't see any upgrade code.



# getcert list | grep '/usr/lib64/ipa/certmonger'
  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"auditSigningCert
cert-pki-ca"
  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"ocspSigningCert
cert-pki-ca"
  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"subsystemCert
cert-pki-ca"
  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"caSigningCert
cert-pki-ca"
  post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert
  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"Server-Cert
cert-pki-ca"
  post-save command: /usr/lib64/ipa/certmonger/restart_dirsrv
RHEL72
  post-save command: /usr/lib64/ipa/certmonger/restart_httpd






You're right it will break the upgrade. I haven't noticed that
Server-Cert for DS and HTTPD are not handled by
certificate_renewal_update (ipaserver.install.server.upgrade) where all
the other trackings are stopped and then configured again with the
paths.CERTMONGER_COMMAND_TEMPLATE already updated.

Thanks for the catch.



I've updated Timo's patch little more and added
start_tracking_certificates() for dsinstance and httpinstance. Now the
upgrade works as expected.


The way the patches are split is kind of weird and apparently confusing
(see the other thread). IMO there should be 2 patches: the first should
add the ability to change DS and HTTP certmonger config during upgrade
(i.e. the start_tracking_certificates() methods and
certificate_renewal_update() changes), the second should move the
helpers (i.e. the actual move and certificate_renewal_update() version
bump).


Honza, do I understand it correctly that the code is OK but I did not
split it to the patches correctly?


Yes.


Before acking or pushing, can you please explain for me how the upgrade of
certmonger tracking requests work? I want to make sure this is right, so please
bear with me:

1) How does it edit existing tracking requests with the new helper paths?

2) Does it go and try to edit the requests on every upgrade? Or is there some
check that requests were updated?

Thanks,
Martin



Whole upgrade of renewal requests is done in
ipaserver/install/server/upgrade.py in certificate_renewal_upgrade().

First there is version of requests and if it's the same as in state file
upgrade is skipped.
Then every request is searched over certmonger's DBus interface and if at least
one is not found it means that there was change in request configuration. All
tracking requests are then stopped and started again with new configuration.

So to answer you questions:
1) By stopping the old request with the old parameters (including path) and
starting new with new parameters.

2) Only if version was bumped which happens only if some of the requests 
changes.


Ah, so IIUC, if you bump the version, requests should be properly updated. The
change looks fine then.



After discussion with Honza, we decided to drop the part comparing only 
base names of pre- and post-save commands and use it as whole. I've also 
split the patches so it's obvious what is going on.


Patches should be applied in this order:

freeipa-dkupka-0091.0
freeipa-dkupka-0087.1
freeipa-dkupka-0088.1
freeipa-tjaalton-0011.2
freeipa-dkupka-0092.0

--
David Kupka
From 3e43c00c9d90752c28b5e81ddb7827ba00f12eba Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Wed, 17 Feb 2016 15:18:04 +0100
Subject: [PATCH 2/5] dsinstance: add start_tracking_certificates method

Configure certmonger to start tracing certificate for DS.

https://fedorahosted.org/freeipa/ticket/5586
---
 ipaserver/install/dsinstance.py | 10 ++
 ipaserver/install/server/upgrade.py | 19 +--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 

Re: [Freeipa-devel] [PATCH 0416-0419] fix broken configuration of sidgen and extdom plugins

[Martin Basti]

On 23.02.2016 13:02, Alexander Bokovoy wrote:

On Tue, 23 Feb 2016, Martin Basti wrote:

From f2ae1bd129a1741500d2f3dcb86a0da553604d15 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Tue, 23 Feb 2016 10:37:47 +0100
Subject: [PATCH 4/4] fix upgrade: wait for proper DS socket after DS 
restart


Restarting DS executed by upgrade plugin causes that upgrade frameworg
was waiting for not proper socket to be ready. This commit fix issue.

Please fix the commit message typos.


Fixed. Updated patches attached.
From 779ca0e230252d40bb275539b4cc512d0460a056 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 18 Feb 2016 19:59:50 +0100
Subject: [PATCH 1/4] upgrade: fix config of sidgen and extdom plugins

During upgrade to IPA 4.2, literally "$SUFFIX" value was added to
configuration of sidgen and extdom plugins. This cause that SID are not properly configured.

Upgrade must fix "$SUFFIX" to reals suffix DN, and run sidgen task
against IPA domain (if exists).

All trusts added when plugins configuration was broken must be re-added.

https://fedorahosted.org/freeipa/ticket/5665
---
 install/updates/90-post_upgrade_plugins.update |   2 +
 ipaserver/install/dsinstance.py|  12 +-
 ipaserver/install/plugins/adtrust.py   | 153 +
 ipaserver/install/server/upgrade.py|   4 +-
 4 files changed, 163 insertions(+), 8 deletions(-)

diff --git a/install/updates/90-post_upgrade_plugins.update b/install/updates/90-post_upgrade_plugins.update
index 5642021ad93cf336db2d872bf3ef6db99b5ffa46..9c9ee160fffedbc8e8d59705169e6cf08ddc9779 100644
--- a/install/updates/90-post_upgrade_plugins.update
+++ b/install/updates/90-post_upgrade_plugins.update
@@ -5,6 +5,8 @@
 plugin: update_ca_topology
 plugin: update_dnszones
 plugin: update_dns_limits
+plugin: update_sigden_extdom_broken_config
+plugin: update_sids
 plugin: update_default_range
 plugin: update_default_trust_view
 plugin: update_ca_renewal_master
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 2905c31c64c4fa8bd034d2cdf6ce5d90ecfea091..f474e189a47f945b7c91cba4ccc17266b9c7e430 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -1054,9 +1054,9 @@ class DsInstance(service.Service):
 """
 Add sidgen directory server plugin configuration if it does not already exist.
 """
-self._ldap_mod('ipa-sidgen-conf.ldif', self.sub_dict)
+self.add_sidgen_plugin(self.sub_dict['SUFFIX'])
 
-def add_sidgen_plugin(self):
+def add_sidgen_plugin(self, suffix):
 """
 Add sidgen plugin configuration only if it does not already exist.
 """
@@ -1064,7 +1064,7 @@ class DsInstance(service.Service):
 try:
 self.admin_conn.get_entry(dn)
 except errors.NotFound:
-self._add_sidgen_plugin()
+self._ldap_mod('ipa-sidgen-conf.ldif', dict(SUFFIX=suffix))
 else:
 root_logger.debug("sidgen plugin is already configured")
 
@@ -1072,9 +1072,9 @@ class DsInstance(service.Service):
 """
 Add directory server configuration for the extdom extended operation.
 """
-self._ldap_mod('ipa-extdom-extop-conf.ldif', self.sub_dict)
+self.add_extdom_plugin(self.sub_dict['SUFFIX'])
 
-def add_extdom_plugin(self):
+def add_extdom_plugin(self, suffix):
 """
 Add extdom configuration if it does not already exist.
 """
@@ -1082,7 +1082,7 @@ class DsInstance(service.Service):
 try:
 self.admin_conn.get_entry(dn)
 except errors.NotFound:
-self._add_extdom_plugin()
+self._ldap_mod('ipa-extdom-extop-conf.ldif', dict(SUFFIX=suffix))
 else:
 root_logger.debug("extdom plugin is already configured")
 
diff --git a/ipaserver/install/plugins/adtrust.py b/ipaserver/install/plugins/adtrust.py
index df9412adba833251cc1c70d7d72ebebbdc77c2a4..5b81b2efd5be63f45b02b8d357f89ec5ba142170 100644
--- a/ipaserver/install/plugins/adtrust.py
+++ b/ipaserver/install/plugins/adtrust.py
@@ -21,6 +21,8 @@ from ipalib import api, errors
 from ipalib import Updater
 from ipapython.dn import DN
 from ipapython.ipa_log_manager import root_logger
+from ipaserver.install import sysupgrade
+
 
 DEFAULT_ID_RANGE_SIZE = 20
 
@@ -161,5 +163,156 @@ class update_default_trust_view(Updater):
 
 return False, [update]
 
+
+class update_sigden_extdom_broken_config(Updater):
+"""Fix configuration of sidgen and extdom plugins
+
+Upgrade to IPA 4.2+ cause that sidgen and extdom plugins have improperly
+configured basedn.
+
+All trusts which have been added when config was broken must to be
+re-added manually.
+
+https://fedorahosted.org/freeipa/ticket/5665
+"""
+
+sidgen_config_dn = DN("cn=IPA SIDGEN,cn=plugins,cn=config")
+extdom_config_dn = 

Re: [Freeipa-devel] [PATCH 0416-0419] fix broken configuration of sidgen and extdom plugins

[Alexander Bokovoy]

On Tue, 23 Feb 2016, Martin Basti wrote:

From f2ae1bd129a1741500d2f3dcb86a0da553604d15 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Tue, 23 Feb 2016 10:37:47 +0100
Subject: [PATCH 4/4] fix upgrade: wait for proper DS socket after DS restart

Restarting DS executed by upgrade plugin causes that upgrade frameworg
was waiting for not proper socket to be ready. This commit fix issue.

Please fix the commit message typos.

--
/ 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 0416-0419] fix broken configuration of sidgen and extdom plugins

[Martin Basti]

On 22.02.2016 20:11, Martin Basti wrote:



On 22.02.2016 19:15, Martin Basti wrote:



On 22.02.2016 17:05, Martin Basti wrote:



On 19.02.2016 15:02, Alexander Bokovoy wrote:

On Fri, 19 Feb 2016, Petr Vobornik wrote:

On 02/19/2016 11:12 AM, Alexander Bokovoy wrote:

On Fri, 19 Feb 2016, Martin Basti wrote:

WIP patch attached

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


Comments inline.


+# we need to run sidgen task
+sidgen_task_dn = 
DN("cn=sidgen,cn=ipa-sidgen-task,cn=tasks,"

+"cn=config")
+sidgen_tasks_attr = {
+"objectclass": ["top", "extensibleObject"],
+"cn": ["sidgen"],
+"delay": [0],
+"nsslapd-basedn": [self.api.env.basedn],
+}

May be you are better to name this task more uniquely?
Something like 'cn=generate domain sid,cn=...'?


+
+task_entry = ldap.make_entry(sidgen_task_dn,
+ **sidgen_tasks_attr)
+try:
+ldap.add_entry(task_entry)
+except errors.DuplicateEntry:
+self.log.debug("sidgen task already created")
+else:
+self.log.debug("sidgen task has been created")
There could be multiple tasks running in parallel, that's why it 
could

be good to use a different and unique name.

+# we have to check all trusts domains which have been 
added

after the
+# upgrade that caused bug was done.
+
+base_dn = DN(self.api.env.container_adtrusts,
self.api.env.basedn)
+trust_domain_entries, truncated = ldap.find_entries(
+base_dn=base_dn,
+scope=ldap.SCOPE_ONELEVEL,
+attrs_list=[attr_name, "cn"],
+)
+
+if truncated:
+self.log.warning("update_sids: Search results were
truncated")
+
+for entry in trust_domain_entries:
+if entry.single_value[attr_name] is None:
+domain = entry.single_value["cn"]
+self.log.error(
+"Your trust to %s is broken. Please 
re-create it

by "
+"running 'ipa trust-add' again", domain)
+
+sysupgrade.set_upgrade_state('sidgen', 'update_sids', 
False)

+return False, ()
This part looks fine. Basically, a similar check needs to be 
added to

trust_find, trust_show, and may be trust_add.



Why trust-add?

I'm not a big fan of cluttering existing commands(find, show, mod) 
with logic to fix one upgrade bug. But I understand a need to 
communicate it somehow.


Would it make sense to move such logic to a separate command, e.g. 
trust-check/trust-verify?  The command can do additional check in 
a future.
No. What is the value of trust-add if it says you 'Trust 
established and

verified' while in reality it didn't? This specific issue is very easy
to catch.


Patch attached, patch with warning will land soon.



Updated patches attached


I fixed except clause in 416, added patch with user warnings, IMO to 
have separate search is the cleanest solution here, command is not 
used often, I would like to avoid any hacks in find and show command


Patches attached.

I will look on ldapsearch timeouts after restarting DS tomorrow.



Updated patches attached + new patch fixing timeout error

From 779ca0e230252d40bb275539b4cc512d0460a056 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 18 Feb 2016 19:59:50 +0100
Subject: [PATCH 1/4] upgrade: fix config of sidgen and extdom plugins

During upgrade to IPA 4.2, literally "$SUFFIX" value was added to
configuration of sidgen and extdom plugins. This cause that SID are not properly configured.

Upgrade must fix "$SUFFIX" to reals suffix DN, and run sidgen task
against IPA domain (if exists).

All trusts added when plugins configuration was broken must be re-added.

https://fedorahosted.org/freeipa/ticket/5665
---
 install/updates/90-post_upgrade_plugins.update |   2 +
 ipaserver/install/dsinstance.py|  12 +-
 ipaserver/install/plugins/adtrust.py   | 153 +
 ipaserver/install/server/upgrade.py|   4 +-
 4 files changed, 163 insertions(+), 8 deletions(-)

diff --git a/install/updates/90-post_upgrade_plugins.update b/install/updates/90-post_upgrade_plugins.update
index 5642021ad93cf336db2d872bf3ef6db99b5ffa46..9c9ee160fffedbc8e8d59705169e6cf08ddc9779 100644
--- a/install/updates/90-post_upgrade_plugins.update
+++ b/install/updates/90-post_upgrade_plugins.update
@@ -5,6 +5,8 @@
 plugin: update_ca_topology
 plugin: update_dnszones
 plugin: update_dns_limits
+plugin: update_sigden_extdom_broken_config
+plugin: update_sids
 plugin: update_default_range
 plugin: update_default_trust_view
 plugin: update_ca_renewal_master
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 2905c31c64c4fa8bd034d2cdf6ce5d90ecfea091..f474e189a47f945b7c91cba4ccc17266b9c7e430 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -1054,9 +1054,9 @@ class 

Re: [Freeipa-devel] Locations design v2: LDAP schema & user interface

[Petr Spacek]

On 23.2.2016 11:00, Jan Cholasta wrote:
> Hi,
> 
> On 19.2.2016 16:31, Simo Sorce wrote:
>> On Fri, 2016-02-19 at 08:58 +0100, Petr Spacek wrote:
>>> On 4.2.2016 18:21, Petr Spacek wrote:
 On 3.2.2016 18:41, Petr Spacek wrote:
> Hello,
>
> I've updated the design page
> http://www.freeipa.org/page/V4/DNS_Location_Mechanism
>
> Namely it now contains 'Version 2'.

 Okay, here is the idea how we can make it flexible:
 http://www.freeipa.org/page/V4/DNS_Location_Mechanism#Implementation
>>>
>>> Hello,
>>>
>>> I'm thinking about LDAP schema for DNS locations.
>>>
>>> Purpose
>>> ===
>>> * Allow admins to define any number of locations.
>>> * 1 DNS server advertises at most 1 location.
>>> * 1 location generally contains set of services with different priorities 
>>> and
>>> weights (in DNS SRV terms).
>>> * Express server & service priority for each defined location in a way which
>>> is granular and flexible and ad the same time easy to manage.
>>>
>>>
>>> Proposal
>>> 
>>> a) Container for locations
>>> --
>>> cn=locations,cn=ipa,cn=etc,dc=example,dc=com
>>>
>>>
>>> b) 1 location
>>> -
>>> Attributes:
>>> 2.16.840.1.113730.3.8.5.32 idnsLocationMember
>>> Server/service assigned to a DNS Location. Usually used to define 'main'
>>> servers for that location. Should it point to service DNs to be sure we have
>>> smooth upgrade to containers?
> 
> Services always live on a host (call it server or not), so IMO it makes sense
> to point to servers.

Fine with me. We just need something which will be able to accommodate
containerization without upgrade headache.

Do I undestand correctly that 1 container is going to have 1 host object with
one service object inside it?

Like:
cn=container
- cn=DNS, cn=container
?

>>> 2.16.840.1.113730.3.8.5.33 idnsBackupLocation
>>> Pointer to another location. Sucks in all servers from that location as one
>>> group with the same priority. Easy to use with _default location where all
>>> 'other' servers are used as backup.
>>>
>>> These two attributes use sub-type priority and 
>>> relativeweight.
>>> This is the only way I could express all the information without need for
>>> separate objects.
> 
> I don't see the benefit here. What is wrong with separate objects? Why is it
> necessary to reinvent the wheel and abuse attribute sub-types for this, losing
> schema integrity checks provided by DS and making the implementation more
> complex along the way?

AFAIK Simo did not like separate objects because we could not use referential
integrity plugin to prune references to removed servers.

This can surely be done in framework, I do not insist on subtypes.


Talk is cheap, show me your schema :-)

>>> Object classes:
>>> 2.16.840.1.113730.3.8.6.7  idnsLocation
>>> MAY ( idnsLocationMember $ idnsBackupLocation )
>>>
>>>
>>> 1st example:
>>> Location CZ:
>>> - servers czserver1, czserver2
>>> - priority=1
>>> - relative weight = 50 % each
>>> - if both CZ servers fail, use servers in location UK as backup (priority 2)
>>> - if all CZ and UK servers fail, use servers in location US as backup
>>> (priority 3) - servers on the other continent are used only as option of 
>>> last
>>> resort
>>> DN: cn=cz,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
>>> objectClass: idnsLocation
>>> idnsLocationMember;priority1;relativeweight50:
>>> cn=czserver1,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
>>> idnsLocationMember;priority1;relativeweight50:
>>> cn=czserver2,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
>>> idnsBackupLocation;priority2:
>>> cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
>>> idnsBackupLocation;priority3:
>>> cn=us,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
>>>
>>> Location UK:
>>> - servers ukserver1, ukserver2
>>> - priority=1
>>> - server ukserver1 is a new beefy machine so it can handle 3 times more load
>>> than ukserver2, thus relative weights 75 % and 25 %
>>> - if both UK servers fail, use servers in location CZ as backup (priority 2)
>>> - if all CZ and UK servers fail, use servers in location US as backup
>>> (priority 3) - servers on the other continent are used only as option of 
>>> last
>>> resort
>>> DN: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
>>> objectClass: idnsLocation
>>> idnsLocationMember;priority1;relativeweight3:
>>> cn=ukserver1,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
>>> idnsLocationMember;priority1;relativeweight1:
>>> cn=ukserver2,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
>>> idnsBackupLocation;priority2:
>>> cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
>>> idnsBackupLocation;priority3:
>>> cn=us,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
>>>
>>> Location US:
>>> - servers usserver1, usserver2
>>> - priority=1
>>> - relative weight = 50 % each
>>> - if both US servers fail, use servers in location CZ and UK as backup
>>> (priority 2) - it is over ocean anyway, so US clients will not make any
>>> difference between CZ and UK 

Re: [Freeipa-devel] Locations design v2: LDAP schema & user interface

[Jan Cholasta]

Hi,

On 19.2.2016 16:31, Simo Sorce wrote:

On Fri, 2016-02-19 at 08:58 +0100, Petr Spacek wrote:

On 4.2.2016 18:21, Petr Spacek wrote:

On 3.2.2016 18:41, Petr Spacek wrote:

Hello,

I've updated the design page
http://www.freeipa.org/page/V4/DNS_Location_Mechanism

Namely it now contains 'Version 2'.


Okay, here is the idea how we can make it flexible:
http://www.freeipa.org/page/V4/DNS_Location_Mechanism#Implementation


Hello,

I'm thinking about LDAP schema for DNS locations.

Purpose
===
* Allow admins to define any number of locations.
* 1 DNS server advertises at most 1 location.
* 1 location generally contains set of services with different priorities and
weights (in DNS SRV terms).
* Express server & service priority for each defined location in a way which
is granular and flexible and ad the same time easy to manage.


Proposal

a) Container for locations
--
cn=locations,cn=ipa,cn=etc,dc=example,dc=com


b) 1 location
-
Attributes:
2.16.840.1.113730.3.8.5.32 idnsLocationMember
Server/service assigned to a DNS Location. Usually used to define 'main'
servers for that location. Should it point to service DNs to be sure we have
smooth upgrade to containers?


Services always live on a host (call it server or not), so IMO it makes 
sense to point to servers.




2.16.840.1.113730.3.8.5.33 idnsBackupLocation
Pointer to another location. Sucks in all servers from that location as one
group with the same priority. Easy to use with _default location where all
'other' servers are used as backup.

These two attributes use sub-type priority and relativeweight.
This is the only way I could express all the information without need for
separate objects.


I don't see the benefit here. What is wrong with separate objects? Why 
is it necessary to reinvent the wheel and abuse attribute sub-types for 
this, losing schema integrity checks provided by DS and making the 
implementation more complex along the way?





Object classes:
2.16.840.1.113730.3.8.6.7  idnsLocation
MAY ( idnsLocationMember $ idnsBackupLocation )


1st example:
Location CZ:
- servers czserver1, czserver2
- priority=1
- relative weight = 50 % each
- if both CZ servers fail, use servers in location UK as backup (priority 2)
- if all CZ and UK servers fail, use servers in location US as backup
(priority 3) - servers on the other continent are used only as option of last
resort
DN: cn=cz,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
objectClass: idnsLocation
idnsLocationMember;priority1;relativeweight50:
cn=czserver1,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsLocationMember;priority1;relativeweight50:
cn=czserver2,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority2: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority3: cn=us,cn=locations,cn=ipa,cn=etc,dc=example,dc=com

Location UK:
- servers ukserver1, ukserver2
- priority=1
- server ukserver1 is a new beefy machine so it can handle 3 times more load
than ukserver2, thus relative weights 75 % and 25 %
- if both UK servers fail, use servers in location CZ as backup (priority 2)
- if all CZ and UK servers fail, use servers in location US as backup
(priority 3) - servers on the other continent are used only as option of last
resort
DN: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
objectClass: idnsLocation
idnsLocationMember;priority1;relativeweight3:
cn=ukserver1,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsLocationMember;priority1;relativeweight1:
cn=ukserver2,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority2: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority3: cn=us,cn=locations,cn=ipa,cn=etc,dc=example,dc=com

Location US:
- servers usserver1, usserver2
- priority=1
- relative weight = 50 % each
- if both US servers fail, use servers in location CZ and UK as backup
(priority 2) - it is over ocean anyway, so US clients will not make any
difference between CZ and UK locations
DN: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
objectClass: idnsLocation
idnsLocationMember;priority1;relativeweight50:
cn=ukserver1,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsLocationMember;priority1;relativeweight50:
cn=ukserver2,cn=masters,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority2: cn=cz,cn=locations,cn=ipa,cn=etc,dc=example,dc=com
idnsBackupLocation;priority2: cn=uk,cn=locations,cn=ipa,cn=etc,dc=example,dc=com


Resulting DNS SRV records (generated by FreeIPA framework). Please note that
only numbers in SRV records matter only relatively. Priorities work as group,
weights are relative only inside the group. Absolute values above are used
only in algorithm which generates SRV records:
Location CZ:
_kerberos._udp SRV 1 50 czserver1
_kerberos._udp SRV 1 50 czserver2
_kerberos._udp SRV 2 75 ukserver1
_kerberos._udp SRV 2 25 ukserver1
_kerberos._udp SRV 3 50 usserver1
_kerberos._udp SRV 3 50 usserver2

Location UK:

Re: [Freeipa-devel] [PATCH] 946 webui: fixed showing of success message after password change on login

[Petr Vobornik]

On 02/18/2016 03:58 PM, Pavel Vomacka wrote:



On 02/18/2016 03:56 PM, Petr Vobornik wrote:

On 01/20/2016 06:42 PM, Petr Vobornik wrote:

similar issue and cause as in
https://fedorahosted.org/freeipa/ticket/5567

root cause is that binding triggers validation which clears messages in
validation summary. Maybe it could be refactored in a future to not use
  the same validation summary field for API calls and fields.

If you think it is actually ticket #5567 (could be in some point of
view) then feel free to push also to 4.2 and 4.3 branch.




bump for review

Ack. Works as expected.


Pushed to master: b9c27b672218c30d669d085b5a57045711542fb9

--
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] 947 webui: use API call ca_is_enabled instead of enable_ra env variable.

[Petr Vobornik]

On 02/18/2016 03:59 PM, Pavel Vomacka wrote:



On 02/18/2016 03:56 PM, Petr Vobornik wrote:

On 01/20/2016 07:02 PM, Petr Vobornik wrote:

To be consistent with backend code.

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




bump for review


Ack.



Pushed to:
master: 31f42bc2e1e931b4c7dec9bf89eb94c844397ea2
ipa-4-3: 814f20100dec5f465b23eb051873ede05480b676




--
Pavel^3 Vomacka

--
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 0011] Move freeipa certmonger helpers to libexecdir.

[Martin Kosek]

On 02/23/2016 09:47 AM, David Kupka wrote:
> On 22/02/16 16:15, Martin Kosek wrote:
>> On 02/22/2016 04:04 PM, Jan Cholasta wrote:
>>> On 22.2.2016 15:56, David Kupka wrote:
 On 22/02/16 07:28, Jan Cholasta wrote:
> On 18.2.2016 10:10, David Kupka wrote:
>> On 19/01/16 16:10, David Kupka wrote:
>>> On 19/01/16 14:38, Jan Cholasta wrote:
 On 19.1.2016 14:26, Martin Kosek wrote:
> On 01/19/2016 01:47 PM, David Kupka wrote:
>> I've polished the patch attached to #5586 by Timo Aaltonen.
>>
>> Thanks for the patch. I've fixed the path in specfile and removed
>> unused import
>> but otherwise it works, ACK.
>>
>> https://fedorahosted.org/freeipa/ticket/5586
>
> Won't this break existing certmonger requests depending on the old
> path?

 It will, I don't see any upgrade code.

>
> # getcert list | grep '/usr/lib64/ipa/certmonger'
>  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
> "auditSigningCert
> cert-pki-ca"
>  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
> "ocspSigningCert
> cert-pki-ca"
>  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
> "subsystemCert
> cert-pki-ca"
>  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
> "caSigningCert
> cert-pki-ca"
>  post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert
>  pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>  post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
> "Server-Cert
> cert-pki-ca"
>  post-save command: /usr/lib64/ipa/certmonger/restart_dirsrv
> RHEL72
>  post-save command: /usr/lib64/ipa/certmonger/restart_httpd
>


>>>
>>> You're right it will break the upgrade. I haven't noticed that
>>> Server-Cert for DS and HTTPD are not handled by
>>> certificate_renewal_update (ipaserver.install.server.upgrade) where all
>>> the other trackings are stopped and then configured again with the
>>> paths.CERTMONGER_COMMAND_TEMPLATE already updated.
>>>
>>> Thanks for the catch.
>>>
>>
>> I've updated Timo's patch little more and added
>> start_tracking_certificates() for dsinstance and httpinstance. Now the
>> upgrade works as expected.
>
> The way the patches are split is kind of weird and apparently confusing
> (see the other thread). IMO there should be 2 patches: the first should
> add the ability to change DS and HTTP certmonger config during upgrade
> (i.e. the start_tracking_certificates() methods and
> certificate_renewal_update() changes), the second should move the
> helpers (i.e. the actual move and certificate_renewal_update() version
> bump).
>
 Honza, do I understand it correctly that the code is OK but I did not
 split it to the patches correctly?
>>>
>>> Yes.
>>
>> Before acking or pushing, can you please explain for me how the upgrade of
>> certmonger tracking requests work? I want to make sure this is right, so 
>> please
>> bear with me:
>>
>> 1) How does it edit existing tracking requests with the new helper paths?
>>
>> 2) Does it go and try to edit the requests on every upgrade? Or is there some
>> check that requests were updated?
>>
>> Thanks,
>> Martin
>>
> 
> Whole upgrade of renewal requests is done in
> ipaserver/install/server/upgrade.py in certificate_renewal_upgrade().
> 
> First there is version of requests and if it's the same as in state file
> upgrade is skipped.
> Then every request is searched over certmonger's DBus interface and if at 
> least
> one is not found it means that there was change in request configuration. All
> tracking requests are then stopped and started again with new configuration.
> 
> So to answer you questions:
> 1) By stopping the old request with the old parameters (including path) and
> starting new with new parameters.
> 
> 2) Only if version was bumped which happens only if some of the requests 
> changes.

Ah, so IIUC, if you bump the version, requests should be properly updated. The
change looks fine then.

-- 
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] [TESTS][PATCH 0009] WebUI tests fix

[Petr Vobornik]

On 02/19/2016 02:53 PM, Lenka Doudova wrote:



On 02/19/2016 10:51 AM, Petr Vobornik wrote:

On 02/16/2016 10:10 AM, Lenka Doudova wrote:



On 02/11/2016 11:13 AM, Lenka Doudova wrote:

Hi all,

most of webUI tests fail with

AssertionError: Can't click on checkbox label: table.table
Message: Element is not clickable at point (37, 340.338964844).
Other element would receive the click:



The problem seems to be that the tests attempt to click on a checkbox
label that is no longer there. Since the checkbox is clickable
directly, I changed the code accordingly. Most of the tests should now
proceed successfully.

Lenka




Attaching updated patch with minor fix.

Lenka




would ACK  but the commit message is so generic that it doesn't say
anything.

Also the description in the mail should be in commit message to be
usable in a future.


Fix attached, hope this is better.

Lenka


ACK

Pushed to master: a3f8e8e71f95c145cbf2e1917bd71c6bedee11d4

--
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 0135] upgrade: unconditional import of certificate profiles into LDAP

[Martin Babinsky]

On 02/23/2016 07:43 AM, Fraser Tweedale wrote:

On Tue, Feb 23, 2016 at 07:32:31AM +0100, Jan Cholasta wrote:

On 23.2.2016 06:40, Fraser Tweedale wrote:

On Mon, Feb 22, 2016 at 02:03:49PM +0100, Martin Babinsky wrote:

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

--
Martin^3 Babinsky


Thanks for the patch.  Conditional ACK.

Patch is tested and works, but I am wary about checking for
substring match against RemoteRetrieveError reason string (see hunk
below).  It would be better to carry the status code as an attribute
of RemoteRetrieveError and check whether it is 409.


+except errors.RemoteRetrieveError as e:
+if "Profile already exists" in e.reason:
+root_logger.debug("Profile '{}' already present".format(
+profile_id))
+else:
+root_logger.error("Error migrating profile '{}': {}".format(
+profile_id, e))
+return


If you agree, we can file a ticket and I am happy for these patches
to be merged as-is.  The scope of changing RemoteRetrieveError is
larger than #5682 so it makes sense to do it separately, and just
for master branch.


I don't think this is the right approach. create_profile() should raise
DuplicateEntry rather than RemoteRetrieveError if the profile already
exists, which can then be properly handled in _create_dogtag_profile().


I am happy with that.



Attaching updated patch. Unfortunately, REST API does report only a 
super-generic error code 400: Bad request when adding profile which 
already exists, which is not very useful for us (I will open a ticket 
for Dogtag for this).


After consultation with Jan, we decided to log the error at debug level 
and be done with it (for the moment).


--
Martin^3 Babinsky
From e75576971f9639825e92b234b688cb56e09cb707 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 22 Feb 2016 13:35:41 +0100
Subject: [PATCH] upgrade: unconditional import of certificate profiles into
 LDAP

During IPA server upgrade, the migration of Dogtag profiles into LDAP
backend was bound to the update of CS.cfg which enabled the LDAP profile
subsystem. If the subsequent profile migration failed, the subsequent
upgrades were not executing the migration code leaving CA subsystem in
broken state. Therefore the migration code path should be executed
regardless of the status of the main Dogtag config file.

https://fedorahosted.org/freeipa/ticket/5682
---
 ipaserver/install/cainstance.py | 8 ++--
 ipaserver/install/server/upgrade.py | 4 +++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 369902ad04b197c9e9516503c1f81c4de1ef153b..1a98c438786ae7dad208212fff23e3a760c95b3c 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -1807,7 +1807,6 @@ def migrate_profiles_to_ldap(dogtag_constants):
 continue
 class_id = match.group(1)
 
-root_logger.info("Migrating profile '%s' to LDAP", profile_id)
 with open(filename) as f:
 profile_data = f.read()
 if profile_data[-1] != '\n':
@@ -1824,7 +1823,12 @@ def _create_dogtag_profile(profile_id, profile_data):
 # import the profile
 try:
 profile_api.create_profile(profile_data)
-except errors.RemoteRetrieveError:
+root_logger.info("Profile '%s' successfully migrated to LDAP",
+ profile_id)
+except errors.RemoteRetrieveError as e:
+root_logger.debug("Error migrating '{}': {}".format(
+profile_id, e))
+
 # conflicting profile; replace it if we are
 # installing IPA, but keep it for upgrades
 if api.env.context == 'installer':
diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py
index 0a46635979497f8028465c2295b22485fd9c0279..258d976c83844f89c1a939303b685fd6565b79e5 100644
--- a/ipaserver/install/server/upgrade.py
+++ b/ipaserver/install/server/upgrade.py
@@ -336,7 +336,9 @@ def ca_enable_ldap_profile_subsystem(ca):
 separator='=')
 
 ca.restart(dogtag.configured_constants().PKI_INSTANCE_NAME)
-cainstance.migrate_profiles_to_ldap(caconfig)
+
+root_logger.info('[Migrating certificate profiles to LDAP]')
+cainstance.migrate_profiles_to_ldap(caconfig)
 
 return needs_update
 
-- 
2.5.0

From 148d44fc744c92fc13ce0ac3544e924c7099ff05 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 22 Feb 2016 13:35:41 +0100
Subject: [PATCH] upgrade: unconditional import of certificate profiles into
 LDAP

During IPA server upgrade, the migration of Dogtag profiles into LDAP
backend was bound to the update of CS.cfg which enabled the LDAP profile
subsystem. If the subsequent profile migration failed, the subsequent
upgrades were not executing the migration code leaving CA subsystem 

Re: [Freeipa-devel] [PATCH 0011] Move freeipa certmonger helpers to libexecdir.

[David Kupka]

On 22/02/16 16:15, Martin Kosek wrote:

On 02/22/2016 04:04 PM, Jan Cholasta wrote:

On 22.2.2016 15:56, David Kupka wrote:

On 22/02/16 07:28, Jan Cholasta wrote:

On 18.2.2016 10:10, David Kupka wrote:

On 19/01/16 16:10, David Kupka wrote:

On 19/01/16 14:38, Jan Cholasta wrote:

On 19.1.2016 14:26, Martin Kosek wrote:

On 01/19/2016 01:47 PM, David Kupka wrote:

I've polished the patch attached to #5586 by Timo Aaltonen.

Thanks for the patch. I've fixed the path in specfile and removed
unused import
but otherwise it works, ACK.

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


Won't this break existing certmonger requests depending on the old
path?


It will, I don't see any upgrade code.



# getcert list | grep '/usr/lib64/ipa/certmonger'
 pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
 post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"auditSigningCert
cert-pki-ca"
 pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
 post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"ocspSigningCert
cert-pki-ca"
 pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
 post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"subsystemCert
cert-pki-ca"
 pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
 post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"caSigningCert
cert-pki-ca"
 post-save command: /usr/lib64/ipa/certmonger/renew_ra_cert
 pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
 post-save command: /usr/lib64/ipa/certmonger/renew_ca_cert
"Server-Cert
cert-pki-ca"
 post-save command: /usr/lib64/ipa/certmonger/restart_dirsrv
RHEL72
 post-save command: /usr/lib64/ipa/certmonger/restart_httpd






You're right it will break the upgrade. I haven't noticed that
Server-Cert for DS and HTTPD are not handled by
certificate_renewal_update (ipaserver.install.server.upgrade) where all
the other trackings are stopped and then configured again with the
paths.CERTMONGER_COMMAND_TEMPLATE already updated.

Thanks for the catch.



I've updated Timo's patch little more and added
start_tracking_certificates() for dsinstance and httpinstance. Now the
upgrade works as expected.


The way the patches are split is kind of weird and apparently confusing
(see the other thread). IMO there should be 2 patches: the first should
add the ability to change DS and HTTP certmonger config during upgrade
(i.e. the start_tracking_certificates() methods and
certificate_renewal_update() changes), the second should move the
helpers (i.e. the actual move and certificate_renewal_update() version
bump).


Honza, do I understand it correctly that the code is OK but I did not
split it to the patches correctly?


Yes.


Before acking or pushing, can you please explain for me how the upgrade of
certmonger tracking requests work? I want to make sure this is right, so please
bear with me:

1) How does it edit existing tracking requests with the new helper paths?

2) Does it go and try to edit the requests on every upgrade? Or is there some
check that requests were updated?

Thanks,
Martin



Whole upgrade of renewal requests is done in 
ipaserver/install/server/upgrade.py in certificate_renewal_upgrade().


First there is version of requests and if it's the same as in state file 
upgrade is skipped.
Then every request is searched over certmonger's DBus interface and if 
at least one is not found it means that there was change in request 
configuration. All tracking requests are then stopped and started again 
with new configuration.


So to answer you questions:
1) By stopping the old request with the old parameters (including path) 
and starting new with new parameters.


2) Only if version was bumped which happens only if some of the requests 
changes.


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