Re: [Freeipa-devel] [PATCH] 0059..0064 Lightweight sub-CAs

2016-06-14 Thread Jan Cholasta 

On 15.6.2016 04:02, Fraser Tweedale wrote:

On Tue, Jun 14, 2016 at 03:21:24PM +0200, Martin Babinsky wrote:

On 06/14/2016 04:55 AM, Fraser Tweedale wrote:

On Tue, Jun 14, 2016 at 02:19:27AM +1000, Fraser Tweedale wrote:

On Mon, Jun 13, 2016 at 04:35:54PM +0200, Martin Babinsky wrote:


Hi Fraser,

during functional review I found the following issues:

1.)

If I create a CAACL rule tied to a specific sub-CA let's say for user
certificate issuance:

"""
ipa caacl-show user_cert_issuance
  Enabled: TRUE
  User category: all
  CAs: user_sub_ca
  Profiles: caIPAuserCert
  ACL name: user_cert_issuance

"""

I can still happily request certificate for a user using root-CA:

"""
 ipa cert-request cert.csr --principal jdoe --ca ipa
  Certificate: MIID9j.../Ov8mkjFA==
  Subject: CN=jdoe,O=IPA.TEST
  Issuer: CN=Certificate Authority,O=IPA.TEST
  ...
"""

should not this be denied by CA-ACL rule?

The default IPA CAACL rule is like this:

"""
ipa caacl-show hosts_services_caIPAserviceCert
  Enabled: TRUE
  Host category: all
  Service category: all
  Profiles: caIPAserviceCert
  ACL name: hosts_services_caIPAserviceCert

"""

so the default rule should not allow users to request certs at all.


Yes, these should be denied.  Looking into it.


Were you using 'admin' account to request the cert?  admin has
permission 'Request Certificate ignoring CA ACLs' via the
'Certificate Manager' privilege.

If so, please try again with less privileges (e.g. self-service as
jdoe).



You were right when I was requesting certs as user principals themselves
everything worked as expected.

Regarding usb-CAs not present on replica, both machines run the following
version of dogtag CA:

pki-ca-10.3.2-3.fc24.noarch

Here are the last 256 lines of the debug log:

http://paste.fedoraproject.org/378512/65828332


Thanks for that.  It seems there are two issues.  The first one is:

Unable to read key retriever class from CS.cfg: Property
features.authority.keyRetrieverClass missing value

This happens only on replica, until the first restart of Dogtag
after installation.  I have attached a patch to fix it.

The second issue is:

Failed to update certificate
  

This needs to be fixed in Dogtag, however, the error is not
triggered if the key retrieval succeeds when the replica first
observes the addition of a new CA.  The attached patch does help in
this regard, so apply it and I hope it will see you through the rest
of the functional testing of my patches... I hope.

Thank you for testing!

Cheers,
Fraser


Rebased patches attached (VERSION conflicts; no other changes).



Thanks Fraser,

the certificate issuance by sub-CA now works also on replica but I must
first restart PKI for it to pick up stuff from master CA.

If I set up a replica first and then create a sub-CA on master Dogtag picks
this up and uses the new sub-CA and its key for signing without restart.

This seems to be pure Dogtag issue, however and I concluded that IPA side
works ok.

If no one objects then I would give functional ACK to your patches and
document/track the Dogtag issue in a separate ticket.


Pushed to master: f0915e61986f545ad9b282fa90a4b1d0538829c5




Thanks Martin,

The Dogtag ticket is https://fedorahosted.org/pki/ticket/2359.  Fix
is merged and will go out in 10.3.3 this week or early next week.


Once released, please send a patch bumping the minimum version in our 
spec file. I'm leaving https://fedorahosted.org/freeipa/ticket/4559 open 
until the patch is merged.


--
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] 0059..0064 Lightweight sub-CAs

2016-06-14 Thread Fraser Tweedale 
On Tue, Jun 14, 2016 at 03:21:24PM +0200, Martin Babinsky wrote:
> On 06/14/2016 04:55 AM, Fraser Tweedale wrote:
> > On Tue, Jun 14, 2016 at 02:19:27AM +1000, Fraser Tweedale wrote:
> > > On Mon, Jun 13, 2016 at 04:35:54PM +0200, Martin Babinsky wrote:
> > > > > > > 
> > > > > > > Hi Fraser,
> > > > > > > 
> > > > > > > during functional review I found the following issues:
> > > > > > > 
> > > > > > > 1.)
> > > > > > > 
> > > > > > > If I create a CAACL rule tied to a specific sub-CA let's say for 
> > > > > > > user
> > > > > > > certificate issuance:
> > > > > > > 
> > > > > > > """
> > > > > > > ipa caacl-show user_cert_issuance
> > > > > > >   Enabled: TRUE
> > > > > > >   User category: all
> > > > > > >   CAs: user_sub_ca
> > > > > > >   Profiles: caIPAuserCert
> > > > > > >   ACL name: user_cert_issuance
> > > > > > > 
> > > > > > > """
> > > > > > > 
> > > > > > > I can still happily request certificate for a user using root-CA:
> > > > > > > 
> > > > > > > """
> > > > > > >  ipa cert-request cert.csr --principal jdoe --ca ipa
> > > > > > >   Certificate: MIID9j.../Ov8mkjFA==
> > > > > > >   Subject: CN=jdoe,O=IPA.TEST
> > > > > > >   Issuer: CN=Certificate Authority,O=IPA.TEST
> > > > > > >   ...
> > > > > > > """
> > > > > > > 
> > > > > > > should not this be denied by CA-ACL rule?
> > > > > > > 
> > > > > > > The default IPA CAACL rule is like this:
> > > > > > > 
> > > > > > > """
> > > > > > > ipa caacl-show hosts_services_caIPAserviceCert
> > > > > > >   Enabled: TRUE
> > > > > > >   Host category: all
> > > > > > >   Service category: all
> > > > > > >   Profiles: caIPAserviceCert
> > > > > > >   ACL name: hosts_services_caIPAserviceCert
> > > > > > > 
> > > > > > > """
> > > > > > > 
> > > > > > > so the default rule should not allow users to request certs at 
> > > > > > > all.
> > > > > > > 
> > > > > > Yes, these should be denied.  Looking into it.
> > > > > > 
> > > > > Were you using 'admin' account to request the cert?  admin has
> > > > > permission 'Request Certificate ignoring CA ACLs' via the
> > > > > 'Certificate Manager' privilege.
> > > > > 
> > > > > If so, please try again with less privileges (e.g. self-service as
> > > > > jdoe).
> > > > > 
> > > > 
> > > > You were right when I was requesting certs as user principals themselves
> > > > everything worked as expected.
> > > > 
> > > > Regarding usb-CAs not present on replica, both machines run the 
> > > > following
> > > > version of dogtag CA:
> > > > 
> > > > pki-ca-10.3.2-3.fc24.noarch
> > > > 
> > > > Here are the last 256 lines of the debug log:
> > > > 
> > > > http://paste.fedoraproject.org/378512/65828332
> > > > 
> > > Thanks for that.  It seems there are two issues.  The first one is:
> > > 
> > > Unable to read key retriever class from CS.cfg: Property
> > > features.authority.keyRetrieverClass missing value
> > > 
> > > This happens only on replica, until the first restart of Dogtag
> > > after installation.  I have attached a patch to fix it.
> > > 
> > > The second issue is:
> > > 
> > > Failed to update certificate
> > >   
> > > 
> > > This needs to be fixed in Dogtag, however, the error is not
> > > triggered if the key retrieval succeeds when the replica first
> > > observes the addition of a new CA.  The attached patch does help in
> > > this regard, so apply it and I hope it will see you through the rest
> > > of the functional testing of my patches... I hope.
> > > 
> > > Thank you for testing!
> > > 
> > > Cheers,
> > > Fraser
> > > 
> > Rebased patches attached (VERSION conflicts; no other changes).
> > 
> 
> Thanks Fraser,
> 
> the certificate issuance by sub-CA now works also on replica but I must
> first restart PKI for it to pick up stuff from master CA.
> 
> If I set up a replica first and then create a sub-CA on master Dogtag picks
> this up and uses the new sub-CA and its key for signing without restart.
> 
> This seems to be pure Dogtag issue, however and I concluded that IPA side
> works ok.
> 
> If no one objects then I would give functional ACK to your patches and
> document/track the Dogtag issue in a separate ticket.
> 
Thanks Martin,

The Dogtag ticket is https://fedorahosted.org/pki/ticket/2359.  Fix
is merged and will go out in 10.3.3 this week or early next week.

Cheers,
Fraser

-- 
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 0159-0160] emancipate IPA NTP service into role

2016-06-14 Thread Martin Babinsky 

On 06/14/2016 05:06 PM, Martin Basti wrote:



On 12.06.2016 17:37, Martin Babinsky wrote:

These two patches turn oft-neglected ntp service into a full fledged
role whose status can be queried centrally. They should also enable
generation of location-specific _ntp._udp records.

Please note that NTP is LDAP-enabled by additional call after DS
instance is configured. I was not feeling confident by swapping NTP
and DS configuration steps as I was afraid it will break things. If
not, I will happily update the patch accordingly.

https://fedorahosted.org/freeipa/ticket/5815
https://fedorahosted.org/freeipa/ticket/5826




Hello, I have a few comments:

Patch: 159
1)
+if ntp.is_configured():
+ntp.ldap_enable('NTP', fqdn, None, base_dn)
+ntp.enable()

All ipa services are in disabled state, ipactl starts them according
configuration in LDAP
IMO it should be something like:
ntp.disable()
if running:
ntp.start()

2)
could you upgrade NTP only once in upgrade.py? Use sysupgrade state

3)
+'NTP': ('ntpd', 42),
I prefer 45, it is easier to put any service before NTP if needed
without huge renumbering


Patch 160: LGTM

Martin^2




Right, attaching updated patches.

--
Martin^3 Babinsky
From f62e8fc696b6c5f2b9fe4fd226c54a0511f64d17 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Sun, 12 Jun 2016 17:02:09 +0200
Subject: [PATCH 1/2] Add NTP to the list of services stored in IPA masters
 LDAP subtree

IPA masters can be configured as NTP servers but the status of this service
can not be determined centrally from querying relevant LDAP subtree. This
patch makes IPA master and replica publish the newly configured NTP service in
their service container during installation.

If the master was configured as NTP server, the NTP service entry will be
created upon upgrade.

https://fedorahosted.org/freeipa/ticket/5815
https://fedorahosted.org/freeipa/ticket/5826
---
 ipaserver/install/ntpinstance.py   | 22 +-
 ipaserver/install/server/install.py|  3 +++
 ipaserver/install/server/replicainstall.py |  5 +
 ipaserver/install/server/upgrade.py|  3 +++
 ipaserver/install/service.py   |  1 +
 5 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/ipaserver/install/ntpinstance.py b/ipaserver/install/ntpinstance.py
index 8b0f0e5395dae3c058fc31bd8914741e4d158830..2cac7baf136cabd38ee7d5fda6ff8e917988b340 100644
--- a/ipaserver/install/ntpinstance.py
+++ b/ipaserver/install/ntpinstance.py
@@ -19,6 +19,7 @@
 #
 
 from ipaserver.install import service
+from ipaserver.install import sysupgrade
 from ipapython import sysrestore
 from ipapython import ipautil
 from ipaplatform.constants import constants
@@ -28,9 +29,28 @@ from ipapython.ipa_log_manager import root_logger
 NTPD_OPTS_VAR = constants.NTPD_OPTS_VAR
 NTPD_OPTS_QUOTE = constants.NTPD_OPTS_QUOTE
 
+NTP_EXPOSED_IN_LDAP = 'exposed_in_ldap'
+
+
+def ntp_ldap_enable(fqdn, base_dn, realm):
+ntp = NTPInstance(realm=realm)
+is_exposed_in_ldap = sysupgrade.get_upgrade_state(
+'ntp', NTP_EXPOSED_IN_LDAP)
+
+was_running = ntp.is_running()
+
+if ntp.is_configured() and not is_exposed_in_ldap:
+ntp.ldap_enable('NTP', fqdn, None, base_dn)
+sysupgrade.set_upgrade_state('ntp', NTP_EXPOSED_IN_LDAP, True)
+
+if was_running:
+ntp.start()
+
+
 class NTPInstance(service.Service):
-def __init__(self, fstore=None):
+def __init__(self, realm=None, fstore=None):
 service.Service.__init__(self, "ntpd", service_desc="NTP daemon")
+self.realm = realm
 
 if fstore:
 self.fstore = fstore
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index c36421fbd9e7ab9046ede57fef3dfa3ed2045b1d..930cca7b31ca06c04ab92deff49b6a4f198c2b6e 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -740,6 +740,9 @@ def install(installer):
idstart=options.idstart, idmax=options.idmax,
subject_base=options.subject,
hbac_allow=not options.no_hbac_allow)
+
+ntpinstance.ntp_ldap_enable(host_name, ds.suffix, realm_name)
+
 else:
 ds = dsinstance.DsInstance(fstore=fstore,
domainlevel=options.domainlevel)
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 6c0ad6939111317e7510662d2953d89ee34cd8f9..f597880471eb3710ebc7163f771d4e6dc9f1e3d6 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -780,6 +780,8 @@ def install(installer):
 # Configure dirsrv
 ds = install_replica_ds(config, options, ca_enabled, remote_api)
 
+ntpinstance.ntp_ldap_enable(config.host_name, ds.suffix, api.env.realm)
+
 # Always try to install DNS records

Re: [Freeipa-devel] [PATCH] 0031 webui: add ability to review certificate request dialog

2016-06-14 Thread Petr Vobornik 
On 04/27/2016 09:25 AM, Pavel Vomacka wrote:
> Hi
> 
> please review the attached patch.
> 
> Fixes this ticket: https://fedorahosted.org/freeipa/ticket/5652
> 
> -- 
> Pavel^3 Vomacka
> 
> 

ACK

master:
* 8135651abb857fbe489a1de8aacad3747d7d5cc9 Add ability to review cert
request dialog
-- 
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] 0045-47: webui: Sub-CAs

2016-06-14 Thread Petr Vobornik 
On 06/14/2016 10:17 AM, Pavel Vomacka wrote:
> 
> 
> On 06/14/2016 06:42 AM, Fraser Tweedale wrote:
>> On Mon, Jun 13, 2016 at 07:48:58PM +0200, Pavel Vomacka wrote:
>>>
>>> On 06/13/2016 06:55 AM, Fraser Tweedale wrote:
 On Fri, Jun 10, 2016 at 04:34:33PM +0200, Pavel Vomacka wrote:
> Hello,
>
> please review these new patches which add WebUI for Sub-CAs.
>
> https://fedorahosted.org/freeipa/ticket/5939
>
 Hi Pavel, I have reviewed the functionality of the patches.
 Functionality is good - a few minor comments below.
>>> Hello, thank you for review.
 Patch 45:

 1) In the main `Certificate Authorities' table, `Subject DN' is
 showing the DN of the IPA object, instead of the Subject DN.
>>> Fixed.
 2) In the `Certificate Authorities' detail table, there is an
 unlabelled row showing the DN of the IPA object.  IMO we do not need
 to show this value at all.
>>> The field removed.
 Patch 46:

 3) I see a FIXME in certificate.js.  The behaviour (default to IPA
 CA / 'ipa') is OK.  Alternatively, you could allow the user to not
 specify a CA (this will allow the default - currently 'ipa' - to be
 controlled by server).
>>> FIXME comment removed.
 Patch 47:

 4) For backwards compatibility, a CA ACL without any specified CAs
 (and not cacat=all) implies the 'ipa' CA.  It would be good to
 indicate this in the UI somehow, or include a notice to explain.
>>> I added tooltip next to the checkbox on adder dialog and also note
>>> above the
>>> table with CAs in CA ACL details view.
>>>
>> The message has a small typo: "specificed" should be "specified"
>> (the typo occurrs in both places).
>>
>> Once this is fixed, ACK.
> Updated patches attached.

Also ACK from Web UI internals perspective.

Patch 0045-2: ACK

Patch 0046-2: ACK, could be split into two patches but don't bother...

Patch 0047-3: ACK

-- 
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] 0036-38 webui: Server roles

2016-06-14 Thread Petr Vobornik 
On 06/09/2016 05:39 PM, Pavel Vomacka wrote:
> 
> 
> On 06/08/2016 04:09 PM, Petr Vobornik wrote:
>> On 06/05/2016 07:22 PM, Pavel Vomacka wrote:
>>>
>>> On 06/03/2016 03:10 PM, Petr Vobornik wrote:
 On 06/02/2016 01:40 PM, Pavel Vomacka wrote:
> Hello,
>
> please review my patches which add webui for server roles.
>
 Did not test yet. I'm waiting for rebase of backend.

 Patch 36: ACK (assuming it works when ^^ is available)

 Patch 37:

 1. typo: 'overriden' - twice
>>> Fixed.
 2. 'create_column_link' is a bad name for the method. The method
 doesn't
 create a column link. It is a link's click handler. So the name should
 be e.g. on_column_link_click
>>> Yes, this is better. Fixed.
 Patch 38:

 1. in serverroles_nested_search_facet wouldn't it be better to override
 only get_refresh_command_options and maybe get_refresh_command_args
 instead of full create_refresh_command?

>>> Fixed.
>>>
>>> Attached the whole patchset with edited patches.
>>>
>>> -- 
>>> Pavel^3 Vomacka
>> 1. Following line breaks navigation:
>> that.on_column_link_click(value, entity);
>> it should be:
>> return that.on_column_link_click(value, entity);
>>
> Attached patches are rebased and updated according to new Server Role
> patches.

ACK for all 4 patches.

pushed to master:
* 95c61c6a0b7d97a9f78fc5b83f38ce2b43cbebc4 Association table can be read
only
* 72fe7e3294fd2f0acdab161180609e9868c4a943 Extend table facet
* 1eb57600185f96e61d0894148a5f50870173c7cd Add server roles on topology page
* 31faf1c21d5635edfa9da23005a1942452a0248c Search facet can be without
search field

-- 
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 0043] Stop uninstaller from failing if a service can't be started

2016-06-14 Thread Stanislav Laznicka 

On 06/14/2016 09:25 AM, Stanislav Laznicka wrote:

On 06/13/2016 02:51 PM, Martin Babinsky wrote:

On 06/07/2016 10:14 AM, Stanislav Laznicka wrote:

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



Umm, wouldn't it be better to augment the `Service.start()/restart()` 
methods themselves with parameters that will suppress exception 
raising and log an error instead of copy-pasting try: ... except: 
blocks?


A good point. SystemdService start()/restart() methods will need 
augmenting as well (signerd service) but I suppose that's about it for 
this issue. I'll send a patch updated accordingly.


Actually, adding an argument to SystemdService's start()/restart() 
methods turned out to be very, very bad idea for this purpose (it would 
end up with way worse copy-paste than the original patch). Attached is 
probably the most minimal solution.
From 33f9b79a68431b54e3a047f3f4a67dec5554803c Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 14 Jun 2016 17:17:37 +0200
Subject: [PATCH] Uninstaller won't fail if service can't be started

https://fedorahosted.org/freeipa/ticket/5775
---
 ipaserver/install/bindinstance.py|  2 +-
 ipaserver/install/httpinstance.py|  2 +-
 ipaserver/install/krbinstance.py |  2 +-
 ipaserver/install/ntpinstance.py |  2 +-
 ipaserver/install/odsexporterinstance.py |  7 ++-
 ipaserver/install/opendnssecinstance.py  |  2 +-
 ipaserver/install/service.py | 28 +++-
 7 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 78e75359266bbefe7954242b98922272fb0c9194..b9b9a7d7f3bef5fb461182e23a4b45d39a3f0405 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -1273,7 +1273,7 @@ class BindInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 self.named_regular.unmask()
 if named_regular_enabled:
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index 00f890175ae583f485797da6f913a7f83b302df3..cd81bbed3830e239b6cf50f765f176547ee788fa 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -549,7 +549,7 @@ class HTTPInstance(service.Service):
 self.print_msg('WARNING: ' + str(e))
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 # disabled by default, by ldap_enable()
 if enabled:
diff --git a/ipaserver/install/krbinstance.py b/ipaserver/install/krbinstance.py
index f560a6ec4c2e4ce931cc1552976db5900a3fa5cd..61d02f9fd346d7b78e5d8682038d9b1a1a149c74 100644
--- a/ipaserver/install/krbinstance.py
+++ b/ipaserver/install/krbinstance.py
@@ -405,7 +405,7 @@ class KrbInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 self.kpasswd = KpasswdInstance()
 self.kpasswd.uninstall()
diff --git a/ipaserver/install/ntpinstance.py b/ipaserver/install/ntpinstance.py
index 8b0f0e5395dae3c058fc31bd8914741e4d158830..8b2b1c1ec5102355eb6461bcfa5b9985ba02bc12 100644
--- a/ipaserver/install/ntpinstance.py
+++ b/ipaserver/install/ntpinstance.py
@@ -183,4 +183,4 @@ class NTPInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
diff --git a/ipaserver/install/odsexporterinstance.py b/ipaserver/install/odsexporterinstance.py
index e9f7bf853d98237aa19aace384b8ff7021c3a85a..4625df81988ba654ac844f93812fc6802b48dee8 100644
--- a/ipaserver/install/odsexporterinstance.py
+++ b/ipaserver/install/odsexporterinstance.py
@@ -193,7 +193,12 @@ class ODSExporterInstance(service.Service):
 signerd_service.enable()
 
 if signerd_running:
-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start '{svcname}': {err}"
+  .format(svcname=signerd_service.service_name,
+  err=e))
 
 installutils.remove_keytab(paths.IPA_ODS_EXPORTER_KEYTAB)
 installutils.remove_ccache(ccache_path=paths.IPA_ODS_EXPORTER_CCACHE)
diff --git a/ipaserver/install/opendnssecinstance.py b/ipaserver/install/opendnssecinstance.py
index f0c512ba04129d08b5874f58c7a25620f7435b2a..c4de0eb4614c235981840db0041a62f4db42ccef 100644
--- a/ipaserver/install/opendnssecinstance.py
+++ b/ipaserver/install/opendnssecinstance.py
@@ -386,4 +386,4 @@ class OpenDNSSECInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
diff --git a/ipaserver/install/service.py

[Freeipa-devel] [PATCH 0424-0426] Fix subtle bugs in event processing

2016-06-14 Thread Petr Spacek 
Hello,

these three bugs were found accidentally while analyzing requirements for
https://fedorahosted.org/bind-dyndb-ldap/ticket/125

All three should go to master before the release because they were source of
subtle bugs.

-- 
Petr^2 Spacek
From 578e72ffa221f320acfa1a4f7eadb8d97996476f Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 14 Jun 2016 15:16:41 +0200
Subject: [PATCH] Rework hack for synchronous event processing to make it
 reliable.

Previously a pointer to event was used as unique ID which was a terrible
idea. The address was sometimes reused by allocator. Address re-use
lead to false positives in event tracing logic and thus some events
were processed asynchronously even if synchronous processing was requested.

https://fedorahosted.org/bind-dyndb-ldap/ticket/125
---
 src/ldap_helper.c | 32 +---
 src/syncrepl.c| 29 -
 src/syncrepl.h|  5 +++--
 src/types.h   | 14 ++
 4 files changed, 46 insertions(+), 34 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 5264e2d75c27864055ab1c3836b4ffaf98270af7..4610a79cb89f270af1bac799c3e17662267eecc0 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -198,18 +198,6 @@ const ldap_auth_pair_t supported_ldap_auth[] = {
 	{ AUTH_INVALID, NULL		},
 };
 
-#define LDAPDB_EVENT_SYNCREPL_UPDATE	(LDAPDB_EVENTCLASS + 1)
-
-typedef struct ldap_syncreplevent ldap_syncreplevent_t;
-struct ldap_syncreplevent {
-	ISC_EVENT_COMMON(ldap_syncreplevent_t);
-	isc_mem_t *mctx;
-	char *dbname;
-	char *prevdn;
-	int chgtype;
-	ldap_entry_t *entry;
-};
-
 extern const settings_set_t settings_default_set;
 
 /** Local configuration file */
@@ -3696,7 +3684,7 @@ update_zone(isc_task_t *task, isc_event_t *event)
 cleanup:
 	if (inst != NULL) {
 		sync_concurr_limit_signal(inst->sctx);
-		sync_event_signal(inst->sctx, event);
+		sync_event_signal(inst->sctx, pevent);
 		if (dns_name_dynamic())
 			dns_name_free(, inst->mctx);
 	}
@@ -3732,7 +3720,7 @@ update_config(isc_task_t * task, isc_event_t *event)
 cleanup:
 	if (inst != NULL) {
 		sync_concurr_limit_signal(inst->sctx);
-		sync_event_signal(inst->sctx, event);
+		sync_event_signal(inst->sctx, pevent);
 	}
 	if (result != ISC_R_SUCCESS)
 		log_error_r("update_config (syncrepl) failed for %s. "
@@ -3764,7 +3752,7 @@ update_serverconfig(isc_task_t * task, isc_event_t *event)
 cleanup:
 	if (inst != NULL) {
 		sync_concurr_limit_signal(inst->sctx);
-		sync_event_signal(inst->sctx, event);
+		sync_event_signal(inst->sctx, pevent);
 	}
 	if (result != ISC_R_SUCCESS)
 		log_error_r("update_serverconfig (syncrepl) failed for %s. "
@@ -4069,15 +4057,15 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
 	isc_result_t result = ISC_R_SUCCESS;
 	ldap_syncreplevent_t *pevent = NULL;
 	ldap_entry_t *entry = NULL;
-	isc_event_t *wait_event = NULL;
 	dns_name_t *zone_name = NULL;
 	dns_zone_t *zone_ptr = NULL;
 	char *dn = NULL;
 	char *dbname = NULL;
 	isc_mem_t *mctx = NULL;
 	isc_taskaction_t action = NULL;
 	isc_task_t *task = NULL;
 	sync_state_t sync_state;
+	isc_boolean_t synchronous;
 
 	REQUIRE(entryp != NULL);
 	entry = *entryp;
@@ -4113,10 +4101,12 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
 			isc_task_attach(inst->task, );
 			result = ISC_R_SUCCESS;
 		}
+		synchronous = ISC_FALSE;
 	} else {
 		/* For configuration object and zone object use single task
 		 * to make sure that the exclusive mode actually works. */
 		isc_task_attach(inst->task, );
+		synchronous = ISC_TRUE;
 	}
 	REQUIRE(task != NULL);
 
@@ -4168,23 +4158,19 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t **entryp, int chgtype)
 	pevent->prevdn = NULL;
 	pevent->chgtype = chgtype;
 	pevent->entry = entry;
-	wait_event = (isc_event_t *)pevent;
-	isc_task_send(task, (isc_event_t **));
-	*entryp = NULL; /* event handler will deallocate the LDAP entry */
 
 	/* Lock syncrepl queue to prevent zone, config and resource records
 	 * from racing with each other. */
-	if (action == update_zone || action == update_config
-	|| action == update_serverconfig)
-		CHECK(sync_event_wait(inst->sctx, wait_event));
+	CHECK(sync_event_send(inst->sctx, task, , synchronous));
+	*entryp = NULL; /* event handler will deallocate the LDAP entry */
 
 cleanup:
 	if (zone_ptr != NULL)
 		dns_zone_detach(_ptr);
 	if (result != ISC_R_SUCCESS)
 		log_error_r("syncrepl_update failed for %s",
 			ldap_entry_logname(entry));
-	if (wait_event == NULL) {
+	if (pevent != NULL) {
 		/* Event was not sent */
 		sync_concurr_limit_signal(inst->sctx);
 
diff --git a/src/syncrepl.c b/src/syncrepl.c
index 1117358e9a1ac3de89f8c1504dae87ca1eaaa80a..00796448b4e5121a8cc5cb2e623b959553bb6bcb 100644
--- a/src/syncrepl.c
+++ b/src/syncrepl.c
@@ -91,10 +91,11 @@ struct sync_ctx {
 	isc_condition_t			cond;	/**< for signal when task_cnt == 0 */
 	sync_state_t			state;
 	ldap_instance_t			*inst;
-	isc_event_t

Re: [Freeipa-devel] [PATCH 0159-0160] emancipate IPA NTP service into role

2016-06-14 Thread Martin Basti 



On 12.06.2016 17:37, Martin Babinsky wrote:
These two patches turn oft-neglected ntp service into a full fledged 
role whose status can be queried centrally. They should also enable 
generation of location-specific _ntp._udp records.


Please note that NTP is LDAP-enabled by additional call after DS 
instance is configured. I was not feeling confident by swapping NTP 
and DS configuration steps as I was afraid it will break things. If 
not, I will happily update the patch accordingly.


https://fedorahosted.org/freeipa/ticket/5815
https://fedorahosted.org/freeipa/ticket/5826




Hello, I have a few comments:

Patch: 159
1)
+if ntp.is_configured():
+ntp.ldap_enable('NTP', fqdn, None, base_dn)
+ntp.enable()

All ipa services are in disabled state, ipactl starts them according 
configuration in LDAP

IMO it should be something like:
ntp.disable()
if running:
ntp.start()

2)
could you upgrade NTP only once in upgrade.py? Use sysupgrade state

3)
+'NTP': ('ntpd', 42),
I prefer 45, it is easier to put any service before NTP if needed 
without huge renumbering



Patch 160: LGTM

Martin^2
-- 
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] 0206 adtrust optimize forest root LDAP filter

2016-06-14 Thread Alexander Bokovoy 

On Tue, 07 Jun 2016, Alexander Bokovoy wrote:

Hi,

`ipa trust-find' command should only show trusted forest root domains

The child domains should be visible via

  ipa trustdomain-find forest.root

The difference between forest root (or external domain) and child
domains is that root domain gets ipaIDObject class to allow assigning a
POSIX ID to the object. This POSIX ID is used by Samba when an Active
Directory domain controller connects as forest trusted domain object.

Child domains can only talk to IPA via forest root domain, thus they
don't need POSIX ID for their TDOs. This allows us a way to
differentiate objects for the purpose of 'trust-find' /
'trustdomain-find' commands.

Fixes https://fedorahosted.org/freeipa/ticket/5942


This patch needs review.

--
/ 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 0042] Removed dead code from LDAPRemoveReverseMember

2016-06-14 Thread Jan Cholasta 

On 14.6.2016 16:35, Martin Basti wrote:



On 14.06.2016 16:37, Jan Cholasta wrote:

On 14.6.2016 16:29, Martin Basti wrote:



On 08.06.2016 14:17, Stanislav Laznicka wrote:

On 06/07/2016 10:42 AM, Martin Basti wrote:



On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:



On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

On 06/03/2016 02:19 PM, Martin Basti wrote:


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the
same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that
every
reverse member command always acts like --all was specified.


I'm really afraid, what can happen if we put attr_list into
get_entry()
instead of '*', because this code were there for 4 years and I don't
feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?


It's a bug and should be fixed. The fix is easy so I see no point in
postponing it. I see no reason to be really afraid, I'm pretty sure
that removing the objectclass attribute (which is invisible in the
CLI anyway) from the output of all the 4 commands that use this code
won't break anything.



Ok

It seems that tests expect objectClass to be always returned in
derived methods. Is that expected behavior? If so, please see the
attached patch. I wonder if the keys of the passed options should make
it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?


I still don't think that we should use that attrs list, because
according git history, attrs_list was really used only with code that
was removed, and as you can see Standa had add extra objectclass
attribute there


So the assumption here is that if it's in git history, it's not a bug?

The extra object class should *not* be included by default.

(Also I don't see any reason to have this split into 2 patches.)


The first one is revert the patch that removes attr_list, and was pushed.


That does not make it any more special than any other commit.





My 2c
Martin^2








--
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 0042] Removed dead code from LDAPRemoveReverseMember

2016-06-14 Thread Martin Basti 



On 14.06.2016 16:37, Jan Cholasta wrote:

On 14.6.2016 16:29, Martin Basti wrote:



On 08.06.2016 14:17, Stanislav Laznicka wrote:

On 06/07/2016 10:42 AM, Martin Basti wrote:



On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:



On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

On 06/03/2016 02:19 PM, Martin Basti wrote:


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the
same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that 
every

reverse member command always acts like --all was specified.


I'm really afraid, what can happen if we put attr_list into
get_entry()
instead of '*', because this code were there for 4 years and I don't
feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?


It's a bug and should be fixed. The fix is easy so I see no point in
postponing it. I see no reason to be really afraid, I'm pretty sure
that removing the objectclass attribute (which is invisible in the
CLI anyway) from the output of all the 4 commands that use this code
won't break anything.



Ok

It seems that tests expect objectClass to be always returned in
derived methods. Is that expected behavior? If so, please see the
attached patch. I wonder if the keys of the passed options should make
it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?


I still don't think that we should use that attrs list, because
according git history, attrs_list was really used only with code that
was removed, and as you can see Standa had add extra objectclass
attribute there


So the assumption here is that if it's in git history, it's not a bug?

The extra object class should *not* be included by default.

(Also I don't see any reason to have this split into 2 patches.)


The first one is revert the patch that removes attr_list, and was pushed.



My 2c
Martin^2





--
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 0042] Removed dead code from LDAPRemoveReverseMember

2016-06-14 Thread Jan Cholasta 

On 14.6.2016 16:29, Martin Basti wrote:



On 08.06.2016 14:17, Stanislav Laznicka wrote:

On 06/07/2016 10:42 AM, Martin Basti wrote:



On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:



On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

On 06/03/2016 02:19 PM, Martin Basti wrote:


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the
same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that every
reverse member command always acts like --all was specified.


I'm really afraid, what can happen if we put attr_list into
get_entry()
instead of '*', because this code were there for 4 years and I don't
feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?


It's a bug and should be fixed. The fix is easy so I see no point in
postponing it. I see no reason to be really afraid, I'm pretty sure
that removing the objectclass attribute (which is invisible in the
CLI anyway) from the output of all the 4 commands that use this code
won't break anything.



Ok

It seems that tests expect objectClass to be always returned in
derived methods. Is that expected behavior? If so, please see the
attached patch. I wonder if the keys of the passed options should make
it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?


I still don't think that we should use that attrs list, because
according git history, attrs_list was really used only with code that
was removed, and as you can see Standa had add extra objectclass
attribute there


So the assumption here is that if it's in git history, it's not a bug?

The extra object class should *not* be included by default.

(Also I don't see any reason to have this split into 2 patches.)



My 2c
Martin^2



--
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 0042] Removed dead code from LDAPRemoveReverseMember

2016-06-14 Thread Martin Basti 



On 08.06.2016 14:17, Stanislav Laznicka wrote:

On 06/07/2016 10:42 AM, Martin Basti wrote:



On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:



On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

On 06/03/2016 02:19 PM, Martin Basti wrote:


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the 
same

code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that every
reverse member command always acts like --all was specified.

I'm really afraid, what can happen if we put attr_list into 
get_entry()

instead of '*', because this code were there for 4 years and I don't
feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?


It's a bug and should be fixed. The fix is easy so I see no point in 
postponing it. I see no reason to be really afraid, I'm pretty sure 
that removing the objectclass attribute (which is invisible in the 
CLI anyway) from the output of all the 4 commands that use this code 
won't break anything.




Ok
It seems that tests expect objectClass to be always returned in 
derived methods. Is that expected behavior? If so, please see the 
attached patch. I wonder if the keys of the passed options should make 
it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?


I still don't think that we should use that attrs list, because 
according git history, attrs_list was really used only with code that 
was removed, and as you can see Standa had add extra objectclass 
attribute there


My 2c
Martin^2

--
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 0041] Increase nsslapd-db-locks

2016-06-14 Thread Martin Basti 



On 09.06.2016 12:42, Stanislav Laznicka wrote:

On 06/07/2016 08:56 AM, thierry bordaz wrote:



On 06/06/2016 07:23 PM, Martin Basti wrote:




On 03.06.2016 13:38, Stanislav Laznicka wrote:

Hello,

The attached patch implements solution to 
https://fedorahosted.org/freeipa/ticket/5914. The patch is rather 
hacky as nsslapd-db-locks requires to be modified when DS is not 
running although I accept proposals for better solution.


Standa


LGTM and works for me, but I want ack from thierry because of the value

* value set by this patch is 100K, it is okay or should be rather 
50K as is mentioned in the ticket
* should be upgrade for this change, or should be this done only for 
new installations? (patch solves new installations only and I don't 
think that we should change it by upgrade, users may have already 
tuned DS)


Martin^2

Hello,

This value also depends of the data. For example adding groups with 
10K members spikes the dblock consumption up to 40K because of 
membership computation during the update.
The size of the entries can also contribute if for example we have 
several entries per page or they are too large and fit on several pages.


IMHO we should support out of the box groups with 10K, so 
'nsslapd-db-locks: 5' looks good to me.
However it could be documented why it can consum so much lock and how 
to tune it.


thanks
thierry

Thank you, Thierry. Reduced the 100k locks to 50k locks in the patch 
(attached).


ACK, I will push it later

--
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] [IMPORTANT] regression in 389-ds-base-1.3.5.5-1.fc24 breaks replica install

2016-06-14 Thread Martin Babinsky 

Hi list,

389-ds-base-1.3.5.5-1.fc24 package in Fedora updates-testing repo 
contains a fix for https://fedorahosted.org/389/ticket/48755 which most 
likely breaks FreeIPA replica installation during total update.


Please downgrade this package (and 389-ds-base-libs) to the latest 
working version (389-ds-base-1.3.5.4-1.fc24.x86_64). If you already 
encountered the issue, give negative karma to the build here:


https://bodhi.fedoraproject.org/updates/FEDORA-2016-5c965fe227

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

[Freeipa-devel] Using JSON for tlog config files

2016-06-14 Thread Nikolai Kondrashov 

Hi everyone,

Although this was mentioned several times before, I'd like to bring additional
attention to the idea of using config files written in JSON for tlog, because
there were some concerns over that being appropriate.

Tlog is a terminal I/O recording package [1], with primary purpose of sending
the recordings as JSON-formatted log messages to ElasticSearch. For the
purpose of reading what it wrote, it links with json-c.

At this moment both of tlog programs (tlog-rec and tlog-play) expect their
global configuration to be in JSON, with comments allowed. See the default
configurations attached. Plus, tlog-rec accepts an environment variable,
containing the whole, or a part of the configuration to (partially) override
the global one. That is also in JSON. Tlog uses the same json-c to parse all
of these. Internally, tlog uses json-c structures to pass around and merge the
configurations.

The question is, should the global tlog configuration, located in
/etc/tlog/tlog-rec.conf and /etc/tlog/tlog-play.conf, be in JSON, or should it
be something else?

The cons I heard so far:

* Administrators don't expect to find JSON in /etc.
* JSON is a fragile format.

The pros I'd like to present:

* Administrators setting up tlog are expected to also be familiar with
  ElasticSearch, which tlog targets as the storage. ElasticSearch speaks
  JSON exclusively and is configured using either YAML or JSON. So, the
  administrators should be largely familiar with it.

* Although JSON uses explicit and rigid syntax, such as quoting and
  prohibited trailing commas, it is still easy to read, and its
  specification is succint and easy to learn: http://json.org/

* Tlog is already linked with json-c, to read what it wrote, and
  reusing it for configuration reading avoids adding another dependency.

Overall, I consider the present situation a good compromise between
smaller/simpler code and reduced dependencies vs. familiarity and ease of
editing and reading for administrators.

The alternatives presented so far are YAML and INI. I'll list each of their
pros and cons, as I see them.

YAML

Pros

* Has a subset of syntax (sufficient for our purposes), which is easy to
  read and write, doesn't require quoting, not critical to commas and
  other separators.
* Has official specification.

Cons

* Requires additional dependency to be used in tlog.
* Only one implementation in C.
* Uses significant whitespace, which is easier to overlook than explicit
  syntax.
* *Sometimes* requires quoting to enforce value type, which is easy to
  overlook. E.g. an all-digits string requires quoting, otherwise it is
  considered a number.
* Although well-defined, specification is long and complicated:
  http://www.yaml.org/spec/1.2/spec.html This makes it hard to fully
  understand the language and be proficient at it.
* In an attempt to make the language as human-readable as possible, made
  it actually harder for humans to write, in some cases.
* Has too many features, complicating parsers, leading to harder to use
  APIs, and more bugs.

INI

Pros

* Familiar, already used by sister projects: SSSD, Kerberos, Samba, etc.
* Light, simple syntax

Cons

* Requires additional dependency to be used in tlog.
* No official specification, lots of variance in the field:
  https://en.wikipedia.org/wiki/INI_file#Varying_features
  This requires explicit description of the actually used syntax in
  the program manuals. I.e. it cannot simply link to a specification.
  Administrators have to discover which flavor to use. This will become
  worse if we'll implement storing (a subset of) tlog-rec configuration
  in LDAP verbatim, as suggested so far, because the documentation for the
  format will be less discoverable for the person editing the directory.
* Cannot be written without newlines, in a single line. This will make
  overriding configuration with an environment variable in tlog-rec harder
  to use. I.e. the environment variable value will have to contain
  newlines, or instead refer to a file containing the configuration.
* No escaping for special characters, multiline value support is patchy
  (not present at all in dinglibs). This will limit the ways to specify
  the recording notice presented to the users at the start of tlog-rec.


Your own pros/cons, and suggestions for other formats to use are welcome!
Thank you for your attention.

Nick

[1]: https://github.com/Scribery/tlog
//
// Tlog-play system-wide configuration. See tlog-play.conf(5) for details.
// This file uses JSON format with both C and C++ comments allowed.
//
{
// The type of "log reader" to use for retrieving log messages. The chosen
// reader needs to be configured using its own dedicated parameters.
// "reader" : "file",

// File reader parameters
"file": {
//

Re: [Freeipa-devel] [PATCH] 0059..0064 Lightweight sub-CAs

2016-06-14 Thread Martin Babinsky 

On 06/14/2016 04:55 AM, Fraser Tweedale wrote:

On Tue, Jun 14, 2016 at 02:19:27AM +1000, Fraser Tweedale wrote:

On Mon, Jun 13, 2016 at 04:35:54PM +0200, Martin Babinsky wrote:


Hi Fraser,

during functional review I found the following issues:

1.)

If I create a CAACL rule tied to a specific sub-CA let's say for user
certificate issuance:

"""
ipa caacl-show user_cert_issuance
  Enabled: TRUE
  User category: all
  CAs: user_sub_ca
  Profiles: caIPAuserCert
  ACL name: user_cert_issuance

"""

I can still happily request certificate for a user using root-CA:

"""
 ipa cert-request cert.csr --principal jdoe --ca ipa
  Certificate: MIID9j.../Ov8mkjFA==
  Subject: CN=jdoe,O=IPA.TEST
  Issuer: CN=Certificate Authority,O=IPA.TEST
  ...
"""

should not this be denied by CA-ACL rule?

The default IPA CAACL rule is like this:

"""
ipa caacl-show hosts_services_caIPAserviceCert
  Enabled: TRUE
  Host category: all
  Service category: all
  Profiles: caIPAserviceCert
  ACL name: hosts_services_caIPAserviceCert

"""

so the default rule should not allow users to request certs at all.


Yes, these should be denied.  Looking into it.


Were you using 'admin' account to request the cert?  admin has
permission 'Request Certificate ignoring CA ACLs' via the
'Certificate Manager' privilege.

If so, please try again with less privileges (e.g. self-service as
jdoe).



You were right when I was requesting certs as user principals themselves
everything worked as expected.

Regarding usb-CAs not present on replica, both machines run the following
version of dogtag CA:

pki-ca-10.3.2-3.fc24.noarch

Here are the last 256 lines of the debug log:

http://paste.fedoraproject.org/378512/65828332


Thanks for that.  It seems there are two issues.  The first one is:

Unable to read key retriever class from CS.cfg: Property
features.authority.keyRetrieverClass missing value

This happens only on replica, until the first restart of Dogtag
after installation.  I have attached a patch to fix it.

The second issue is:

Failed to update certificate
  

This needs to be fixed in Dogtag, however, the error is not
triggered if the key retrieval succeeds when the replica first
observes the addition of a new CA.  The attached patch does help in
this regard, so apply it and I hope it will see you through the rest
of the functional testing of my patches... I hope.

Thank you for testing!

Cheers,
Fraser


Rebased patches attached (VERSION conflicts; no other changes).



Thanks Fraser,

the certificate issuance by sub-CA now works also on replica but I must 
first restart PKI for it to pick up stuff from master CA.


If I set up a replica first and then create a sub-CA on master Dogtag 
picks this up and uses the new sub-CA and its key for signing without 
restart.


This seems to be pure Dogtag issue, however and I concluded that IPA 
side works ok.


If no one objects then I would give functional ACK to your patches and 
document/track the Dogtag issue in a separate ticket.


--
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] bind-dyndb-ldap 10.0 development status (related to FreeIPA 4.4)

2016-06-14 Thread Petr Spacek 
On 11.6.2016 21:22, Petr Spacek wrote:
> Hello,
> 
> bind-dyndb-ldap 10.0 alpha 1 is available for testing (finally).
> 
> AFAIK it implements all the critical functionality for FreeIPA 4.4, namely
> RecordGenerator & default TTL support necessary for FreeIPA DNS locations.
> 
> 
> Limitations
> ===
> BIND has to be reloaded ("rndc reload" at least) after each change in server's
> config or zone's default TTL.
> 
> In case of FreeIPA it means that server-mod command which touches server's DNS
> location has to be followed by "rndc reload" on the affected replica.
> 
> 
> Outlook
> ===
> I'm looking for a solution for quite a while now but it is an asynchronous
> parallel event hell.
> 
> We will probably end up with big hammer like "reconnect to LDAP and re-parse
> everything". Most likely it will be error prone and racy (think about DNS
> updates in the middle of re-synchronization) but any fine-grained approaches
> seem to be even more fragile and even racier. Yuck.

Detailed analysis revealed that even 'reconnect to LDAP and re-parse
everything' hammer is unreliable and we do not have enough time to fix it.
Fixing it would require major refactoring of bind-dyndb-ldap internals.


Following alternative was considered, too:
Extend ipa-dnskeysyncd (daemon in Python) to trigger named restart when
particular attributes in LDAP are modified.

Unfortunately automatic restart could easily cause global DNS failure if e.g.
a script was used to modify locations on multiple IPA servers.

The main problem is that bind-dyndb-ldap start (and also shutdown) takes some
time if the LDAP database is not empty. During that time request are either
not answered at all or are being answered with NXDOMAIN answer.

For database with 3 DNS zones, 1 small (primary IPA domain) and 2 filled with
64k A records (1 record / name) it takes ~ 20 seconds to load the data and 60
seconds to shutdown the BIND. I.e. one restart means ~ 80 seconds of outage.

Plan

For this reason me and Petr Vobornik decided against automatic restart. Next
version of FreeIPA will issue a warning that admin has to manually restart
named on affected servers.


Comments are welcome.

Petr^2 Spacek


> Implemented designs
> ===
> - https://fedorahosted.org/bind-dyndb-ldap/wiki/Design/RecordGenerator
> - https://fedorahosted.org/bind-dyndb-ldap/wiki/Design/PerServerConfigInLDAP
> 
> Fixed tickets
> =
> - https://fedorahosted.org/bind-dyndb-ldap/ticket/126
> - https://fedorahosted.org/bind-dyndb-ldap/ticket/162
> - https://fedorahosted.org/bind-dyndb-ldap/ticket/70
> - https://fedorahosted.org/bind-dyndb-ldap/ticket/164
> - https://fedorahosted.org/bind-dyndb-ldap/ticket/165
> - https://fedorahosted.org/bind-dyndb-ldap/ticket/146
> 
> COPR packages
> =
> https://copr.fedorainfracloud.org/coprs/pspacek/bind-dyndb-ldap/build/339004/
> 
> SRPM
> 
> https://pspacek.fedorapeople.org/bind-dyndb-ldap/bind-dyndb-ldap-10.0-0.1alpha.fc23.src.rpm
> 
> Git branch
> ==
> https://github.com/pspacek/bind-dyndb-ldap/tree/server_config_in_ldap4
> 
> Git commit
> ==
> 6722382b2344fd5acd6ba9fa858c139c16e3de99
> 
> 
> Enjoy.
> 


-- 
Petr^2 Spacek

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

[Freeipa-devel] [Design Review Request] V4/Automatic_Certificate_Request_Generation

2016-06-14 Thread Ben Lipton 

Hello all,

I have written up a design proposal for making certificate requests 
easier to generate when using alternate certificate profiles: 
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation. 
The use case for this is described in 
https://fedorahosted.org/freeipa/ticket/4899. I will be working on 
implementing this design over the next couple of months. If you have the 
time and interest, please take a look and share any comments or concerns 
that you have.


Thanks!

Ben

--
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] 0020 Enable password change extop to apply on virtual entry like the entry in compat tree

2016-06-14 Thread Alexander Bokovoy 

On Tue, 14 Jun 2016, thierry bordaz wrote:

From ac6c0617f618fc609df93dc18ec25255484b533d Mon Sep 17 00:00:00 2001
From: Thierry Bordaz 
Date: Fri, 10 Jun 2016 15:34:40 +0200
Subject: [PATCH] ipapwd_extop should use TARGET_DN defined by a pre-extop
plugin

ipapwd_extop allows to update the password on a specific entry, identified by 
its DN.
It can be usefull to support virtual DN in the extop so that update of a 
virtual entry
would land into the proper real entry.

If a pre-extop sets the TARGET_DN, ipapwd_extop sets ORIGINAL_DN with the value
of TARGET_DN, instead of using the original one (in the ber req)

https://fedorahosted.org/freeipa/ticket/5946
---
.../ipa-pwd-extop/ipa_pwd_extop.c  | 36 +-
1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c 
b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
index 440e221..3c2c44f 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
@@ -207,8 +207,10 @@ static int ipapwd_chpwop(Slapi_PBlock *pb, struct 
ipapwd_krbcfg *krbcfg)
char *attrlist[] = {"*", "passwordHistory", NULL };
struct ipapwd_data pwdata;
int is_krb, is_smb, is_ipant;
-char *principal = NULL;
+   char *principal = NULL;
Slapi_PBlock *chpwop_pb = NULL;
+   Slapi_DN *target_sdn = NULL;
+   char *target_dn = NULL;

/* Get the ber value of the extended operation */
slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_VALUE, _value);
@@ -327,14 +329,32 @@ parse_req_done:
}
}

-/* Determine the target DN for this operation */
-/* Did they give us a DN ? */
-   if (dn == NULL || *dn == '\0') {
-   /* Get the DN from the bind identity on this connection */
-   dn = slapi_ch_strdup(bindDN);
-   LOG_TRACE("Missing userIdentity in request, "
-  "using the bind DN instead.\n");
+   /* Determine the target DN for this operation */
+   slapi_pblock_get(pb, SLAPI_TARGET_SDN, _sdn);
+   if (target_sdn != NULL) {
+   /* If there is a TARGET_DN we are consuming it */
+   slapi_pblock_set(pb, SLAPI_TARGET_SDN, NULL);
+   target_dn = slapi_sdn_get_ndn(target_sdn);
}
+   if (target_dn == NULL || *target_dn == '\0') {
+   /* Did they give us a DN ? */
+   if (dn == NULL || *dn == '\0') {
+   /* Get the DN from the bind identity on this connection 
*/
+   dn = slapi_ch_strdup(bindDN);
+   LOG_TRACE("Missing userIdentity in request, "
+   "using the bind DN instead.\n");
+   }
+   LOG_TRACE("extop dn %s (from ber)\n", dn ? dn : "");
+   } else {
+   /* At this point if SLAPI_TARGET_SDN was set that means
+* that a SLAPI_PLUGIN_PRE_EXTOP_FN plugin sets it
+* So take this one rather that the raw one that is in the ber
+*/
+   LOG_TRACE("extop dn %s was translated to %s\n", dn ? dn : 
"", target_dn);
+   slapi_ch_free_string();
+   dn = slapi_ch_strdup(target_dn);
+   }
+   slapi_sdn_free(_sdn);
Looks good now. 


ACK for the patch, the testing will come once slapi-nis patches will be
available.

Thanks.

--
/ 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 0501] Revert: switch /usr/bin/ipa to python3

2016-06-14 Thread Martin Basti 



On 14.06.2016 13:05, Martin Babinsky wrote:

On 06/14/2016 11:56 AM, Martin Basti wrote:



On 14.06.2016 10:14, Martin Basti wrote:




On 10.06.2016 10:57, Martin Basti wrote:



On 10.06.2016 06:17, Jan Cholasta wrote:

On 9.6.2016 20:57, Martin Basti wrote:
Py3 support was enabled prematurely, attached patches removes 
python3

from /usr/bin/ipa


Notes:

* ipa 4.3.x won't have enabled py3

* master (ipa 4.4+) will have disabled py3 temporarily


NACK. you reverted this bit wrong:

-%if 0%{?with_python3}
-Requires: python3-ipaclient = %{version}-%{release}
-%else
 Requires: python2-ipaclient = %{version}-%{release}
-%endif
+Requires: %{name}-client-common = %{version}-%{release}
+Requires: python2-ipalib = %{version}-%{release}



Shame on me,

updated patch attached




self NACK, even with reverted patch, /usr/bin/ipa has still configured
python3, it's a magic something is still putting python3 into ipa




Extra patch was added

now it should work

Martin^2





Thanks, ACK.


master:

* ee08f3e237cb3668c06307aad74e47eb20080474 Revert "Switch /usr/bin/ipa 
to Python 3"

* 5760cc918247746a4e55adc118bdbd9ffb01b78f Use python2 for ipa cli

ipa-4-3:

* b3024fb879a6b841822df1e15fe1c6218c87a4bb Revert "Switch /usr/bin/ipa 
to Python 3"

* 64f078b8b21314000ddf7e95c441570254e249f7 Use python2 for ipa cli

--
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 0501] Revert: switch /usr/bin/ipa to python3

2016-06-14 Thread Martin Babinsky 

On 06/14/2016 11:56 AM, Martin Basti wrote:



On 14.06.2016 10:14, Martin Basti wrote:




On 10.06.2016 10:57, Martin Basti wrote:



On 10.06.2016 06:17, Jan Cholasta wrote:

On 9.6.2016 20:57, Martin Basti wrote:

Py3 support was enabled prematurely, attached patches removes python3
from /usr/bin/ipa


Notes:

* ipa 4.3.x won't have enabled py3

* master (ipa 4.4+) will have disabled py3 temporarily


NACK. you reverted this bit wrong:

-%if 0%{?with_python3}
-Requires: python3-ipaclient = %{version}-%{release}
-%else
 Requires: python2-ipaclient = %{version}-%{release}
-%endif
+Requires: %{name}-client-common = %{version}-%{release}
+Requires: python2-ipalib = %{version}-%{release}



Shame on me,

updated patch attached




self NACK, even with reverted patch, /usr/bin/ipa has still configured
python3, it's a magic something is still putting python3 into ipa




Extra patch was added

now it should work

Martin^2





Thanks, 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 0494] Bump required version of pki-ca and pki-kra due bug in parsing '%' in DM password

2016-06-14 Thread Martin Basti 



On 02.06.2016 09:26, Martin Basti wrote:

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


Patch attached




You can ignore this patch, dogtag version has been bumped by different 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] 0020 Enable password change extop to apply on virtual entry like the entry in compat tree

2016-06-14 Thread thierry bordaz 



On 06/13/2016 05:06 PM, Alexander Bokovoy wrote:

On Mon, 13 Jun 2016, thierry bordaz wrote:

From fff11869d8cf3dfe98471e018c10926fc23b13da Mon Sep 17 00:00:00 2001

From: Thierry Bordaz 
Date: Fri, 10 Jun 2016 15:34:40 +0200
Subject: [PATCH] ipapwd_extop should use TARGET_DN defined by a 
pre-extop

plugin

ipapwd_extop allows to update the password on a specific entry, 
identified by its DN.
It can be usefull to support virtual DN in the extop so that update 
of a virtual entry

would land into the proper real entry.

If a pre-extop sets the TARGET_DN, ipapwd_extop sets ORIGINAL_DN with 
the value

of TARGET_DN, instead of using the original one (in the ber req)

https://fedorahosted.org/freeipa/ticket/5946
---
.../ipa-pwd-extop/ipa_pwd_extop.c  | 33 
--

1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c 
b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c

index 440e221..10fff30 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
@@ -207,8 +207,10 @@ static int ipapwd_chpwop(Slapi_PBlock *pb, 
struct ipapwd_krbcfg *krbcfg)

char *attrlist[] = {"*", "passwordHistory", NULL };
struct ipapwd_data pwdata;
int is_krb, is_smb, is_ipant;
-char *principal = NULL;
+char *principal = NULL;
Slapi_PBlock *chpwop_pb = NULL;
+Slapi_DN *target_sdn = NULL;
+char *target_dn = NULL;

/* Get the ber value of the extended operation */
slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_VALUE, _value);
@@ -327,13 +329,28 @@ parse_req_done:
}
}

- /* Determine the target DN for this operation */
- /* Did they give us a DN ? */
-if (dn == NULL || *dn == '\0') {
- /* Get the DN from the bind identity on this connection */
-dn = slapi_ch_strdup(bindDN);
-LOG_TRACE("Missing userIdentity in request, "
-  "using the bind DN instead.\n");
+/* Determine the target DN for this operation */
+slapi_pblock_get(pb, SLAPI_TARGET_SDN, _sdn);
+target_dn = slapi_sdn_get_ndn(target_sdn);
+if (target_dn) {
can you please use the same style for writing comparisons as the file 
using already?

 if (!(target_dn == NULL || *target_dn == '\0')) { ... }


+/* At this point if SLAPI_TARGET_SDN was set that means
+ * that a SLAPI_PLUGIN_PRE_EXTOP_FN plugin sets it
+ * So take this one rather that the raw one that is in the ber
+ */
+LOG_TRACE("extop dn %s was translated to %s\n", dn ? dn : 
"", target_dn);

+slapi_ch_free_string();
+dn = slapi_ch_strdup(target_dn);
+slapi_sdn_free(_sdn);
+slapi_pblock_set(pb, SLAPI_TARGET_SDN, NULL);
+} else {
+/* Did they give us a DN ? */
+if (dn == NULL || *dn == '\0') {
+/* Get the DN from the bind identity on this connection */
+dn = slapi_ch_strdup(bindDN);
+LOG_TRACE("Missing userIdentity in request, "
+"using the bind DN instead.\n");
+}
+LOG_TRACE("extop dn %s (from ber)\n", dn ? dn : "");
}

 if (slapi_pblock_set( pb, SLAPI_ORIGINAL_TARGET, dn )) {
--
2.5.0





Changing the comparison style

>From ac6c0617f618fc609df93dc18ec25255484b533d Mon Sep 17 00:00:00 2001
From: Thierry Bordaz 
Date: Fri, 10 Jun 2016 15:34:40 +0200
Subject: [PATCH] ipapwd_extop should use TARGET_DN defined by a pre-extop
 plugin

ipapwd_extop allows to update the password on a specific entry, identified by its DN.
It can be usefull to support virtual DN in the extop so that update of a virtual entry
would land into the proper real entry.

If a pre-extop sets the TARGET_DN, ipapwd_extop sets ORIGINAL_DN with the value
of TARGET_DN, instead of using the original one (in the ber req)

https://fedorahosted.org/freeipa/ticket/5946
---
 .../ipa-pwd-extop/ipa_pwd_extop.c  | 36 +-
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
index 440e221..3c2c44f 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
@@ -207,8 +207,10 @@ static int ipapwd_chpwop(Slapi_PBlock *pb, struct ipapwd_krbcfg *krbcfg)
 	char *attrlist[] = {"*", "passwordHistory", NULL };
 	struct ipapwd_data pwdata;
 	int is_krb, is_smb, is_ipant;
-char *principal = NULL;
+	char *principal = NULL;
 	Slapi_PBlock *chpwop_pb = NULL;
+	Slapi_DN *target_sdn = NULL;
+	char *target_dn = NULL;
 
 	/* Get the ber value of the extended operation */
 	slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_VALUE, _value);
@@ -327,14 +329,32 @@ parse_req_done:
 		}
 	}
 
-	 /* Determine the target DN for this operation */
-	 /* Did they

Re: [Freeipa-devel] [PATCH 0501] Revert: switch /usr/bin/ipa to python3

2016-06-14 Thread Martin Basti 



On 14.06.2016 10:14, Martin Basti wrote:




On 10.06.2016 10:57, Martin Basti wrote:



On 10.06.2016 06:17, Jan Cholasta wrote:

On 9.6.2016 20:57, Martin Basti wrote:

Py3 support was enabled prematurely, attached patches removes python3
from /usr/bin/ipa


Notes:

* ipa 4.3.x won't have enabled py3

* master (ipa 4.4+) will have disabled py3 temporarily


NACK. you reverted this bit wrong:

-%if 0%{?with_python3}
-Requires: python3-ipaclient = %{version}-%{release}
-%else
 Requires: python2-ipaclient = %{version}-%{release}
-%endif
+Requires: %{name}-client-common = %{version}-%{release}
+Requires: python2-ipalib = %{version}-%{release}



Shame on me,

updated patch attached




self NACK, even with reverted patch, /usr/bin/ipa has still configured 
python3, it's a magic something is still putting python3 into ipa





Extra patch was added

now it should work

Martin^2


From 305eaa7589b80a6ac4cf847e15b03be35a6a04f6 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Tue, 14 Jun 2016 11:41:25 +0200
Subject: [PATCH] Use python2 for ipa cli

Switch 'ipa' command to py3 has been done prematurely, this commit sets python2 as interpreter for ipa cli.

https://fedorahosted.org/freeipa/ticket/5638
---
 freeipa.spec.in | 5 +
 1 file changed, 5 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index f51fec4ebf15ab73a4c4fe1c51647ce4709ee38b..522f537e9b9c51e1c09f7c045e640076e5f0d626 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -739,6 +739,11 @@ make client-install DESTDIR=%{buildroot}
 (cd ipaclient && %{__python3} setup.py install --root %{buildroot})
 %endif # with_python3
 
+# Switch shebang of /usr/bin/ipa
+# XXX: ipa cli is not stable enough for enabling py3 support, keep it in py2
+# in any case
+sed -i -e'1s/python\(3\|$\)/python2/' %{buildroot}%{_bindir}/ipa
+
 %find_lang %{gettext_domain}
 
 mkdir -p %{buildroot}%{_usr}/share/ipa
-- 
2.5.5

From c7988f133de6d5d9a84e02e6d8b7f060173f757d Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 9 Jun 2016 20:37:55 +0200
Subject: [PATCH] Revert "Switch /usr/bin/ipa to Python 3"

This reverts commit 1ebd8334bc7da95f1edd64fc930e9cd6e3650534.

Switch 'ipa' command to py3 has been done prematurely, thus this commit
reverts it from IPA 4.3.2 and temporarily from master because it is
blocker for developing of the new features.

https://fedorahosted.org/freeipa/ticket/5638
---
 freeipa.spec.in | 11 ---
 ipa |  2 +-
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index ebd910a3df287a0154624391ddb54567cbb47364..c884990c3d158d4ae5bb84d869de4fde0915809b 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -421,11 +421,7 @@ Summary: IPA administrative tools
 Group: System Environment/Base
 BuildArch: noarch
 Requires: %{name}-client-common = %{version}-%{release}
-%if 0%{?with_python3}
-Requires: python3-ipalib = %{version}-%{release}
-%else
 Requires: python2-ipalib = %{version}-%{release}
-%endif
 Requires: python-ldap
 
 Provides: %{alt_name}-admintools = %{version}
@@ -741,13 +737,6 @@ make client-install DESTDIR=%{buildroot}
 (cd ipapython && make PYTHON=%{__python3} IPA_VERSION_IS_GIT_SNAPSHOT=no %{?_smp_mflags} DESTDIR=%{buildroot} install)
 (cd ipaplatform && %{__python3} setup.py install --root %{buildroot})
 (cd ipaclient && %{__python3} setup.py install --root %{buildroot})
-
-# Switch shebang of /usr/bin/ipa
-# XXX: This script is installed with ipaserver. When all of ipaserver is
-# built with Python 3, this will no longer be necessary (as long as the py3
-# version is installed after the py2 version, so it overwrites /usr/bin/ipa)
-sed -i -e'1s/python\(2\|$\)/python3/' %{buildroot}%{_bindir}/ipa
-
 %endif # with_python3
 
 %find_lang %{gettext_domain}
diff --git a/ipa b/ipa
index 338105d7ec57897078c4e94cf5959259565624d2..64ceea49732bb11c4d69cf353d1a2d183e58981a 100755
--- a/ipa
+++ b/ipa
@@ -1,4 +1,4 @@
-#!/usr/bin/python3
+#!/usr/bin/python2
 
 # Authors:
 #   Jason Gerard DeRose 
-- 
2.5.5

From 606a8ed11692980d578c23d5bb70d10cd84f490d Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 10 Jun 2016 10:38:55 +0200
Subject: [PATCH] Revert "Switch /usr/bin/ipa to Python 3"

This reverts commit 1ebd8334bc7da95f1edd64fc930e9cd6e3650534.

Switch 'ipa' command to py3 has been done prematurely, thus this commit
reverts it from IPA 4.3.2 and temporarily from master because it is
blocker for developing of the new features.

https://fedorahosted.org/freeipa/ticket/5638
---
 freeipa.spec.in | 11 ---
 ipa |  2 +-
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 58344695cae08fa1baf2123a13eb99f3fbc6d496..f51fec4ebf15ab73a4c4fe1c51647ce4709ee38b 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -421,11 +421,7 @@ installed on every client machine.
 Summary: IPA administrative tools
 Group: System Environment/Base

[Freeipa-devel] [PATCHES 551-552, 623-624] cert: add owner information, allow search by certificate

2016-06-14 Thread Jan Cholasta 

On 21.4.2016 09:11, Jan Cholasta wrote:

On 6.4.2016 15:46, Pavel Vomacka wrote:



On 03/16/2016 01:50 PM, Jan Cholasta wrote:

Hi,

the attached patches implement the server-side part of
.

Honza


Hi,

thank you for the patches. I tested them and they work well. But I would
like to ask you whether would be possible to extend the response of
'basecert_find' method and probably also 'basecert_show' response. I
think of these information:

1) information whether the certificate is issued by our CA or not.


You can check for that by comparing the issuer name of the certificate
to "CN=Certificate Authority,$SUBJECT_BASE". You can get subject base
from config-show.



2) this probably wouldn't be possible (as we discussed), but I rather
write it too - the information about revocation reason. The same as the
'cert_show' provides.


Added --check-revocation flag to request this information. Currently it
works only on certificates issued by our CA.



3) MD5 and SHA1 fingerprints as the 'cert_show' method returns


Added, also included SHA-256.



Thank you again.


Updated patches attached.


Updated and rebased patches attached. Requires Fraser's sub-CA patches.

--
Jan Cholasta
From ef0b52cd5f3943443cd43b64c712dc47a5ebbfd6 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 16 Mar 2016 13:09:11 +0100
Subject: [PATCH 1/4] ldap: fix handling of binary data in search filters

This fixes a UnicodeDecodeError when passing non-UTF-8 binary data to
LDAPClient.make_filter() and friends.

https://fedorahosted.org/freeipa/ticket/5381
---
 ipapython/ipaldap.py | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 410ddae..23405c6 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -1211,7 +1211,12 @@ class LDAPClient(object):
 ]
 return self.combine_filters(flts, rules)
 elif value is not None:
-value = ldap.filter.escape_filter_chars(value_to_utf8(value))
+if isinstance(value, bytes):
+if six.PY3:
+value = value.decode('raw_unicode_escape')
+else:
+value = value_to_utf8(value)
+value = ldap.filter.escape_filter_chars(value)
 if not exact:
 template = '%s'
 if leading_wildcard:
-- 
2.7.4

From a88e4a85383b1f1834c807c44f4694c496ac6819 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Tue, 14 Jun 2016 06:29:18 +0200
Subject: [PATCH 2/4] cert: add object plugin

Implement cert as an object with methods rather than a bunch of loosely
related commands.

https://fedorahosted.org/freeipa/ticket/5381
---
 API.txt   |  62 +++---
 VERSION   |   4 +-
 ipaclient/plugins/cert.py |   6 +-
 ipaserver/plugins/cert.py | 518 +-
 4 files changed, 328 insertions(+), 262 deletions(-)

diff --git a/API.txt b/API.txt
index 741c643..f01d3c1 100644
--- a/API.txt
+++ b/API.txt
@@ -730,25 +730,27 @@ output: Entry('result')
 output: Output('summary', type=[, ])
 output: PrimaryKey('value')
 command: cert_find
-args: 0,19,4
+args: 1,20,4
+arg: Str('criteria?')
 option: Flag('all', autofill=True, cli_name='all', default=False)
-option: Str('cacn?', autofill=False, cli_name='ca')
+option: Str('cacn?', cli_name='ca')
 option: Flag('exactly?', autofill=True, default=False)
-option: Str('issuedon_from?', autofill=False)
-option: Str('issuedon_to?', autofill=False)
-option: Str('issuer?', autofill=False)
+option: DateTime('issuedon_from?', autofill=False)
+option: DateTime('issuedon_to?', autofill=False)
+option: DNParam('issuer?', autofill=False)
 option: Int('max_serial_number?', autofill=False)
 option: Int('min_serial_number?', autofill=False)
+option: Flag('pkey_only?', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
-option: Int('revocation_reason?', autofill=False)
-option: Str('revokedon_from?', autofill=False)
-option: Str('revokedon_to?', autofill=False)
-option: Int('sizelimit?', default=100)
+option: Int('revocation_reason?', autofill=False, default=0)
+option: DateTime('revokedon_from?', autofill=False)
+option: DateTime('revokedon_to?', autofill=False)
+option: Int('sizelimit?')
 option: Str('subject?', autofill=False)
-option: Str('validnotafter_from?', autofill=False)
-option: Str('validnotafter_to?', autofill=False)
-option: Str('validnotbefore_from?', autofill=False)
-option: Str('validnotbefore_to?', autofill=False)
+option: DateTime('validnotafter_from?', autofill=False)
+option: DateTime('validnotafter_to?', autofill=False)
+option: DateTime('validnotbefore_from?', autofill=False)
+option: DateTime('validnotbefore_to?', autofill=False)
 option: Str('version?')
 output: Output('count', type=[])
 output: ListOfEntries('result')
@@ -756,37 +758,49 @@ output:

Re: [Freeipa-devel] [PATCH] 0045-47: webui: Sub-CAs

2016-06-14 Thread Pavel Vomacka 



On 06/14/2016 06:42 AM, Fraser Tweedale wrote:

On Mon, Jun 13, 2016 at 07:48:58PM +0200, Pavel Vomacka wrote:


On 06/13/2016 06:55 AM, Fraser Tweedale wrote:

On Fri, Jun 10, 2016 at 04:34:33PM +0200, Pavel Vomacka wrote:

Hello,

please review these new patches which add WebUI for Sub-CAs.

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


Hi Pavel, I have reviewed the functionality of the patches.
Functionality is good - a few minor comments below.

Hello, thank you for review.

Patch 45:

1) In the main `Certificate Authorities' table, `Subject DN' is
showing the DN of the IPA object, instead of the Subject DN.

Fixed.

2) In the `Certificate Authorities' detail table, there is an
unlabelled row showing the DN of the IPA object.  IMO we do not need
to show this value at all.

The field removed.

Patch 46:

3) I see a FIXME in certificate.js.  The behaviour (default to IPA
CA / 'ipa') is OK.  Alternatively, you could allow the user to not
specify a CA (this will allow the default - currently 'ipa' - to be
controlled by server).

FIXME comment removed.

Patch 47:

4) For backwards compatibility, a CA ACL without any specified CAs
(and not cacat=all) implies the 'ipa' CA.  It would be good to
indicate this in the UI somehow, or include a notice to explain.

I added tooltip next to the checkbox on adder dialog and also note above the
table with CAs in CA ACL details view.


The message has a small typo: "specificed" should be "specified"
(the typo occurrs in both places).

Once this is fixed, ACK.

Updated patches attached.

--
Pavel^3 Vomacka
From 3a41b336d83c4864f474b01a2afbd705a5279a4e Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Fri, 10 Jun 2016 16:12:45 +0200
Subject: [PATCH 1/3] Add new webui plugin - ca

Whole new entity for CAs.

https://fedorahosted.org/freeipa/ticket/5939
---
 install/ui/src/freeipa/app.js  |  1 +
 install/ui/src/freeipa/navigation/menu_spec.js |  5 ++
 install/ui/src/freeipa/plugins/ca.js   | 91 ++
 3 files changed, 97 insertions(+)
 create mode 100644 install/ui/src/freeipa/plugins/ca.js

diff --git a/install/ui/src/freeipa/app.js b/install/ui/src/freeipa/app.js
index daf17b7ba021d3db8288f2de89a8ae4814172a70..4eb045d7a989bdfee29cef670770d285701ae875 100644
--- a/install/ui/src/freeipa/app.js
+++ b/install/ui/src/freeipa/app.js
@@ -29,6 +29,7 @@ define([
 './aci',
 './automember',
 './automount',
+'./plugins/ca',
 './plugins/caacl',
 './plugins/certprofile',
 './dns',
diff --git a/install/ui/src/freeipa/navigation/menu_spec.js b/install/ui/src/freeipa/navigation/menu_spec.js
index 0afc7daceb725cee5677982c68c09d1666d42885..97a03527e94ba2ae21d70e0170b91efc81b155a4 100644
--- a/install/ui/src/freeipa/navigation/menu_spec.js
+++ b/install/ui/src/freeipa/navigation/menu_spec.js
@@ -142,6 +142,11 @@ var nav = {};
 entity: 'caacl',
 facet: 'search',
 hidden: true
+},
+{
+entity: 'ca',
+facet: 'search',
+hidden: true
 }
 ]
 },
diff --git a/install/ui/src/freeipa/plugins/ca.js b/install/ui/src/freeipa/plugins/ca.js
new file mode 100644
index ..8e2fb702fe3d220e7cae5d30dbb85e0ae5ad9cf7
--- /dev/null
+++ b/install/ui/src/freeipa/plugins/ca.js
@@ -0,0 +1,91 @@
+//
+// Copyright (C) 2016  FreeIPA Contributors see COPYING for license
+//
+
+define([
+'../ipa',
+'../jquery',
+'../phases',
+'../reg',
+'../certificate'
+],
+function(IPA, $, phases, reg, cert) {
+
+/**
+ * ca module
+ * @class plugins.ca
+ * @singleton
+ */
+ var ca = IPA.ca = {};
+
+ var make_ca_spec = function() {
+ var spec = {
+ name: 'ca',
+ facets: [
+ {
+ $type: 'search',
+ disable_facet_tabs: false,
+ tabs_in_sidebar: true,
+ tab_label: '@mo:ca.label',
+ facet_groups: [cert.facet_group],
+ facet_group: 'certificates',
+ columns: [
+ 'cn',
+ 'ipacasubjectdn',
+ 'description'
+ ]
+ },
+ {
+ $type: 'details',
+ disable_facet_tabs: true,
+ fields: [
+ 'cn',
+ {
+ $type: 'textarea',
+ name: 'description'
+ },
+ 'ipacaid',
+ 'ipacaissuerdn',
+ 'ipacasubjectdn'
+ ]
+ }
+ ],
+ adder_dialog: {
+ fields: [
+ {
+ $type: 'text',
+ name:

Re: [Freeipa-devel] [PATCH 0501] Revert: switch /usr/bin/ipa to python3

2016-06-14 Thread Martin Basti 



On 10.06.2016 10:57, Martin Basti wrote:



On 10.06.2016 06:17, Jan Cholasta wrote:

On 9.6.2016 20:57, Martin Basti wrote:

Py3 support was enabled prematurely, attached patches removes python3
from /usr/bin/ipa


Notes:

* ipa 4.3.x won't have enabled py3

* master (ipa 4.4+) will have disabled py3 temporarily


NACK. you reverted this bit wrong:

-%if 0%{?with_python3}
-Requires: python3-ipaclient = %{version}-%{release}
-%else
 Requires: python2-ipaclient = %{version}-%{release}
-%endif
+Requires: %{name}-client-common = %{version}-%{release}
+Requires: python2-ipalib = %{version}-%{release}



Shame on me,

updated patch attached




self NACK, even with reverted patch, /usr/bin/ipa has still configured 
python3, it's a magic something is still putting python3 into ipa
-- 
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] 0003 batch command can be used to trigger internal errors on server

2016-06-14 Thread Martin Basti 



On 14.06.2016 08:04, Stanislav Laznicka wrote:

On 06/13/2016 10:15 AM, Petr Vobornik wrote:

On 06/10/2016 06:31 PM, Stanislav Laznicka wrote:

On 06/08/2016 02:06 PM, Florence Blanc-Renaud wrote:

On 06/08/2016 10:07 AM, Petr Spacek wrote:

On 7.6.2016 15:11, Stanislav Laznicka wrote:

Hello,

Thank you for your patch. As the thin-client patches were pushed 
in the
meantime, the patch won't apply. Could you please send a rebased 
version?


Also, I have a few comments to the patch:

1) I think that the commit message should be rather a brief 
conclusion to the
changes made in the commit. This could help for faster 
orientation in the
changes that were made to a certain part of code should you be 
searching for a
bug introduced by a commit. Should some more info be required, it 
can be added

to the ticket. Could you therefore shorten the commit message?

(My personal opinion, no golden standard.)

Honestly I disagree with Standa. Yes, the commit message seems to 
be a bit
long but *tickets* are not the best place to put *technical* 
information into.


Tickets are planning tool but keep in mind that Trac may/will 
vanish one day

and all we will have will be (Git?) repo.

I would recommend putting the comment about expected input format 
into code

comments somewhere around batch command definition.

This would reduce commit message (roughly, the text needs to be 
adapted) to
part starting 'The code did not check the format of ' ... which is 
perfectly

reasonable description of the change.

IMHO.
Petr^2 Spacek


2) Please do not add the tickets to comments in the code. You can 
use git
blame -L or git log -L to see in which commits were the changes 
introduced to
a certain part of a file, these commits should include the ticket 
number if

more info is needed.

Standa


On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:

Hi all,

the following patch checks the format of parameters passed to a 
method
called through the batch command. I picked the ConversionError 
for invalid
parameters format but this choice can be discussed if you have 
better

suggestions...

Fixes:https://fedorahosted.org/freeipa/ticket/5810
--
Florence Blanc-Renaud
Identity Management Team, Red Hat





Hi,

please find an updated patch version with a less verbose commit 
msg. I also

removed the reference to ticket # in the code.

Flo.



Hello,

Thank you for your updated patch. I have just one small issue that 
maybe exceeds

the scope of this ticket. If the check for dictionary instance in list:

+if not isinstance(arg, dict):
+raise errors.ConversionError(
+name='methods',
+error=_(u'must contain dict objects'))

fails at a further member of the list, by raising an exception, you 
will lose
information about execution of all the previous commands but these 
were already
executed. This hasn't seem to be an issue until now so I wonder if 
it is a

problem or not.

Right, this is something that we will need to address but not in scope
of this ticket and probably not 4.4 release.

So far, batch commands have only been utilized in the WebUI so I am 
adding Petr
for opinions on how to handle this properly so that WebUI could 
react to it
should it ever happen (although AFAIK this should never happen for 
batch

commands called from the UI).

Web UI doesn't care because it sends it correctly :) The bug is trying
to use batch command while talking directly to API - e.g. because of
performance reasons.


Thank you for the explanation. ACK, then.


Pushed to master: 2c7ec27ad94a5a369c7d8a45dcef66a18479900b

--
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 0043] Stop uninstaller from failing if a service can't be started

2016-06-14 Thread Stanislav Laznicka 

On 06/13/2016 02:51 PM, Martin Babinsky wrote:

On 06/07/2016 10:14 AM, Stanislav Laznicka wrote:

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



Umm, wouldn't it be better to augment the `Service.start()/restart()` 
methods themselves with parameters that will suppress exception 
raising and log an error instead of copy-pasting try: ... except: blocks?


A good point. SystemdService start()/restart() methods will need 
augmenting as well (signerd service) but I suppose that's about it for 
this issue. I'll send a patch updated accordingly.


--
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] 0003 batch command can be used to trigger internal errors on server

2016-06-14 Thread Stanislav Laznicka 

On 06/13/2016 10:15 AM, Petr Vobornik wrote:

On 06/10/2016 06:31 PM, Stanislav Laznicka wrote:

On 06/08/2016 02:06 PM, Florence Blanc-Renaud wrote:

On 06/08/2016 10:07 AM, Petr Spacek wrote:

On 7.6.2016 15:11, Stanislav Laznicka wrote:

Hello,

Thank you for your patch. As the thin-client patches were pushed in the
meantime, the patch won't apply. Could you please send a rebased version?

Also, I have a few comments to the patch:

1) I think that the commit message should be rather a brief conclusion to the
changes made in the commit. This could help for faster orientation in the
changes that were made to a certain part of code should you be searching for a
bug introduced by a commit. Should some more info be required, it can be added
to the ticket. Could you therefore shorten the commit message?

(My personal opinion, no golden standard.)

Honestly I disagree with Standa. Yes, the commit message seems to be a bit
long but *tickets* are not the best place to put *technical* information into.

Tickets are planning tool but keep in mind that Trac may/will vanish one day
and all we will have will be (Git?) repo.

I would recommend putting the comment about expected input format into code
comments somewhere around batch command definition.

This would reduce commit message (roughly, the text needs to be adapted) to
part starting 'The code did not check the format of ' ... which is perfectly
reasonable description of the change.

IMHO.
Petr^2 Spacek



2) Please do not add the tickets to comments in the code. You can use git
blame -L or git log -L to see in which commits were the changes introduced to
a certain part of a file, these commits should include the ticket number if
more info is needed.

Standa


On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:

Hi all,

the following patch checks the format of parameters passed to a method
called through the batch command. I picked the ConversionError for invalid
parameters format but this choice can be discussed if you have better
suggestions...

Fixes:https://fedorahosted.org/freeipa/ticket/5810
--
Florence Blanc-Renaud
Identity Management Team, Red Hat





Hi,

please find an updated patch version with a less verbose commit msg. I also
removed the reference to ticket # in the code.

Flo.



Hello,

Thank you for your updated patch. I have just one small issue that maybe exceeds
the scope of this ticket. If the check for dictionary instance in list:

+if not isinstance(arg, dict):
+raise errors.ConversionError(
+name='methods',
+error=_(u'must contain dict objects'))

fails at a further member of the list, by raising an exception, you will lose
information about execution of all the previous commands but these were already
executed. This hasn't seem to be an issue until now so I wonder if it is a
problem or not.

Right, this is something that we will need to address but not in scope
of this ticket and probably not 4.4 release.


So far, batch commands have only been utilized in the WebUI so I am adding Petr
for opinions on how to handle this properly so that WebUI could react to it
should it ever happen (although AFAIK this should never happen for batch
commands called from the UI).

Web UI doesn't care because it sends it correctly :) The bug is trying
to use batch command while talking directly to API - e.g. because of
performance reasons.


Thank you for the explanation. ACK, 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