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

[Fraser Tweedale]

On Wed, Jun 08, 2016 at 01:00:36PM +0200, Jan Cholasta wrote:
> On 8.6.2016 05:15, Fraser Tweedale wrote:
> > On Tue, Jun 07, 2016 at 03:42:22PM +1000, Fraser Tweedale wrote:
> > > On Wed, Jun 01, 2016 at 02:51:04PM +1000, Fraser Tweedale wrote:
> > > > Hi team,
> > > > 
> > > > This patchset implements the 'ca' plugin for creating and managing
> > > > lightweight sub-CAs, and updates the 'caacl' plugin and
> > > > 'cert-request' command to support multiple CAs.
> > > > 
> > > > A brief overview of the patches:
> > > > 
> > > > 0059
> > > >   'ca' plugin, associated schema changes and container objects,
> > > >   Dogtag REST API wrapper
> > > > 0060
> > > >   Add CA entry for the IPA CA on install/upgrade
> > > > 0061
> > > >   Update 'caacl' plugin with CA support (including enforcement)
> > > > 0062
> > > >   Update ra.request_certificate() to support specifying target CA
> > > > 0063
> > > >   Add '--ca' option to 'cert-request' command
> > > > 0064
> > > >   Add '--issuer' option to 'cert-find' command
> > > > 
> > > > These patches depend on other pending patches:
> > > > 
> > > > 0051, 0052, 0053, 0054, 0055, 0056
> > > > 
> > > > Signing key replication depends on unmerged Dogtag patches.  Builds
> > > > of Dogtag with the required patches, and of FreeIPA with all
> > > > completed sub-CAs work, should be available from my COPR soon:
> > > > https://copr.fedorainfracloud.org/coprs/ftweedal/freeipa/
> > > > 
> > > > Some parts of the design are not implemented in the current
> > > > patchset, including:
> > > > 
> > > > - local parent CA (ipaca object) references
> > > > - sub-CA certificate renewal
> > > > - 'cert-show' command '--ca=NAME' option
> > > > - certmonger support for specifying CA
> > > > - revocation of deleted CAs
> > > > 
> > > > I look forward to your reviews!
> > > > 
> > > > Thanks,
> > > > Fraser
> > > > 
> > > Rebased and updated patches attached.
> > > 
> > > Substantive changes:
> > > 
> > > - add required attributes for issuer DN and subject DN
> > > - prevent rename of IPA CA
> > > - when adding IPA CA entry, contact Dogtag to learn authority id,
> > >   issuer DN and subject DN
> > > - add 'read_ca' method to Dogtag interface
> > > - tighten ACIs to prevent modification of ipacaid attribute
> > > 
> > Updated patch 0064-3; adds --issuer option to cert-show and --ca
> > option to cert-show and cert-find.
> 
> Patch 0059:
> 
> 1) On upgrade, why is the lightweight CA container created twice - once in
> 41-subca.update, once using ensure_entry() call? It should be done only
> once.
> 
I'll remove 41-subca.update; the routine in cainstance is the one
that's needed.

> 2) In ca_del, every CA specified in args[0] should be deleted, not just the
> first one.
> 
> 3) Do not use NonFatalError, issue a warning instead:
> 
> self.add_message(MyNewWarningClass(name=...))
> 
> 4) Can it actually happen that ca_show does not return ipacaid? I guess not,
> so you should be able to remove the check altogether and don't bother with
> the warning.
> 
ipacaid is mandatory now, so I'll remove the check.

> 
> Patch 0060-0062: LGTM
> 
Yippee \o/

> 
> Patch 0063:
> 
> Could you please define the CA param as follows:
> 
> Str('cacn?',
> cli_name='ca',
> query=True,
> label=_("CA"),
> doc=_("CA to use"),
> ),
> 
> ?
> 
> This is for consitency with framework-generated parent key params, which
> unfortunately we cannot leverage in cert_request currently.
> 
No problemo.

> 
> Patch 0064:
> 
> 1) See my comment for patch 0063, it applies here as well.
> 
> 2) The --issuer option should not be included in cert_show - show commands
> are supposed to retrieve an object given primary key(s), and the primary key
> of CA objects is just their cn.
> 
The --issuer argument is because primary key for a cert is really
(issuer, serial).  So it show the cert _with_ that issuer (and
serial), not the cert _for_ that issuer.

> 3) In find commands, the options form a filter, so instead of raising
> MutuallyExclusiveError in cert-find, return an empty result, as with any
> other unmatched filter.
> 
Here, --issuer and --ca are two different ways to specify the
issuer.  --issuer lets you give the issuer DN straight up; --ca
takes the name of an IPA CA object and looks up its issuer DN.
(Thus it makes no sense to give both options at once).

Do you think it would be clearer to provide a single option that
takes issuer DN or the name of CA?  I considered this but decided
against it because collisions are possible (one could name an IPA CA
something that passes for a stingified DN).

Thanks for reviewing!
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] git commit message

[Lukas Slebodnik]

On (08/06/16 14:09), Petr Vobornik 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.
>
>+1
>
>The commit message is very good and honestly I'd like to see more of
>such commit messages.
>
Regarding to commit message.

I like recommendation about git commit message from Openstack wiki
https://wiki.openstack.org/wiki/GitCommitMessages#Information_in_commit_messages
We can at least inspire.

IMHO longer commit message is always better.

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] 0042: Fix bad searching of reverse DNS zone

[Petr Vobornik]

On 06/07/2016 10:30 AM, Pavel Vomacka wrote:
> 
> 
> On 06/07/2016 09:08 AM, Petr Spacek wrote:
>> Hi,
>>
>> the commit message does not say what was wrong and why and what works
>> now.
>> Please improve the commit message before pushing this.
> Commit message improved.
>>
>> Petr^2 Spacek
>>
>> On 6.6.2016 19:03, Pavel Vomacka wrote:
>>> Fix bad searching of reverse DNS zone
>>>
>>> https://fedorahosted.org/freeipa/ticket/5796
>>>

It fails with zones:
* 13.10.10.in-addr.arpa.
* 3.10.10.in-addr.arpa.

And A record: 10.10.13.1

reason: you forgot to update target_zone.index when udating
target_zone.target_zone
-- 
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] 0003 batch command can be used to trigger internal errors on server

[Stanislav Laznicka]

On 06/08/2016 02:09 PM, Petr Vobornik 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.

+1

The commit message is very good and honestly I'd like to see more of
such commit messages.


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.

Batch command deserves a doc string, so that it would be visible in API
browser but that is not subject of this ticket. Btw, expected format is
already in the code.

It is, although it may be not be as clear it is what you're looking for.

As for the commit message, I would like see the description of batch 
commands (which I think is really good) in the doc string rather than in 
the commit message to get the information about the change I need. 
However, that would require proper documentation (such as that in the 
commit message) in the code (where it belongs). As it is not there, or 
it is not that clear, I guess that the commit message is the right spot 
to have it and I was wrong here.

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.

+1


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












--
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 0403-0407] Preparation work for per-server config in LDAP

[Petr Spacek]

On 7.6.2016 12:09, Petr Spacek wrote:
> Hello,
> 
> this patch set is preparation work for per-server config in LDAP, which is
> required for DNS location in IPA.
> 
> This patch set should not cause any user-visible changes.
> 
> https://fedorahosted.org/bind-dyndb-ldap/ticket/162

Here is revised patch set. It fixes couple interesting bugs in corner cases I
found during work on next features.

-- 
Petr^2 Spacek
From 5a4e0b7026dc4f7f786d1d59a3a9ad33bfe89e30 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Wed, 4 May 2016 16:20:44 +0200
Subject: [PATCH] Fix in destroy_ldap_instance() caused by uninitialized
 MetaLDAP.

This happened only if an new_ldap_instance() failed with an error before
initializing MetaLDAP.
---
 src/mldap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mldap.c b/src/mldap.c
index 8cffe8a1fbf8eaa20aae79c28ad8d7a305494f19..143abce757f3d65e68356a2c0e660c475ed0ab58 100644
--- a/src/mldap.c
+++ b/src/mldap.c
@@ -79,7 +79,7 @@ void
 mldap_destroy(mldapdb_t **mldapp) {
 	mldapdb_t *mldap;
 
-	REQUIRE(mldapp != NULL && *mldapp != NULL);
+	REQUIRE(mldapp != NULL);
 
 	mldap = *mldapp;
 	if (mldap == NULL)
-- 
2.5.5

From 6c4cabd8be5d90502786a5f4356dbb4742ec7a70 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Thu, 2 Jun 2016 15:30:19 +0200
Subject: [PATCH] Separate BIND config utilities from ACL parsing.

In future we will use the code from multiple modules.
There should be no user-visible changes caused by this split.

https://fedorahosted.org/bind-dyndb-ldap/ticket/162
---
 src/Makefile.am|   2 +
 src/acl.c  | 118 +++--
 src/bindcfg.c  | 116 
 src/bindcfg.h  |  24 +++
 src/zone_manager.c |   3 ++
 5 files changed, 151 insertions(+), 112 deletions(-)
 create mode 100644 src/bindcfg.c
 create mode 100644 src/bindcfg.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 58f73ec9d37471471036fb532ac239523512eb80..595fb4d95577f025e97dbce0770325a82a053ad8 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -3,6 +3,7 @@ bindplugindir=$(libdir)/bind
 
 HDRS =\
 	acl.h			\
+	bindcfg.h		\
 	compat.h		\
 	empty_zones.h		\
 	fs.h			\
@@ -31,6 +32,7 @@ HDRS =\
 ldap_la_SOURCES =		\
 	$(HDRS)			\
 	acl.c			\
+	bindcfg.c		\
 	empty_zones.c		\
 	fwd_register.c		\
 	fs.c			\
diff --git a/src/acl.c b/src/acl.c
index e4b3f524876ac93a177613c7f325883398367af8..d03a34d8c4ba5016ae9b248dc86781952d92c8d4 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -6,13 +6,11 @@
 
 #include 
 #include 
-#include 
 #include 
 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -30,17 +28,12 @@
 #include 
 
 #include "acl.h"
+#include "bindcfg.h"
 #include "str.h"
 #include "util.h"
 #include "log.h"
 #include "types.h"
 
-static isc_once_t once = ISC_ONCE_INIT;
-static cfg_type_t *update_policy;
-static cfg_type_t *allow_query;
-static cfg_type_t *allow_transfer;
-static cfg_type_t *forwarders;
-
 /* Following definitions are necessary for context ("map" configuration object)
  * required during ACL parsing. */
 static cfg_clausedef_t * empty_map_clausesets[] = {
@@ -60,105 +53,6 @@ const enum_txt_assoc_t acl_type_txts[] = {
 	{ -1,			NULL		} /* end marker */
 };
 
-static cfg_type_t * ATTR_NONNULLS ATTR_CHECKRESULT
-get_type_from_tuplefield(const cfg_type_t *cfg_type, const char *name)
-{
-	cfg_type_t *ret = NULL;
-	const cfg_tuplefielddef_t *field;
-
-	REQUIRE(cfg_type != NULL && cfg_type->of != NULL);
-	REQUIRE(name != NULL);
-
-	field = (cfg_tuplefielddef_t *)cfg_type->of;
-	for (int i = 0; field[i].name != NULL; i++) {
-		if (!strcmp(field[i].name, name)) {
-			ret = field[i].type;
-			break;
-		}
-	}
-
-	return ret;
-}
-
-static cfg_type_t * ATTR_NONNULLS ATTR_CHECKRESULT
-get_type_from_clause(const cfg_clausedef_t *clause, const char *name)
-{
-	cfg_type_t *ret = NULL;
-
-	REQUIRE(clause != NULL);
-	REQUIRE(name != NULL);
-
-	for (int i = 0; clause[i].name != NULL; i++) {
-		if (!strcmp(clause[i].name, name)) {
-			ret = clause[i].type;
-			break;
-		}
-	}
-
-	return ret;
-}
-
-static cfg_type_t * ATTR_NONNULLS ATTR_CHECKRESULT
-get_type_from_clause_array(const cfg_type_t *cfg_type, const char *name)
-{
-	cfg_type_t *ret = NULL;
-	const cfg_clausedef_t **clauses;
-
-	REQUIRE(cfg_type != NULL && cfg_type->of != NULL);
-	REQUIRE(name != NULL);
-
-	clauses = (const cfg_clausedef_t **)cfg_type->of;
-	for (int i = 0; clauses[i] != NULL; i++) {
-		ret = get_type_from_clause(clauses[i], name);
-		if (ret != NULL)
-			break;
-	}
-
-	return ret;
-}
-
-static void
-init_cfgtypes(void)
-{
-	cfg_type_t *zoneopts;
-
-	zoneopts = _type_namedconf;
-	zoneopts = get_type_from_clause_array(zoneopts, "zone");
-	zoneopts = get_type_from_tuplefield(zoneopts, "options");
-
-	update_policy = get_type_from_clause_array(zoneopts, "update-policy");
-	allow_query = get_type_from_clause_array(zoneopts, "allow-query");
-	

Re: [Freeipa-devel] thin client regressions: otptoken

[Alexander Bokovoy]

On Wed, 08 Jun 2016, Jan Cholasta wrote:

On 7.6.2016 10:41, Alexander Bokovoy wrote:

On Tue, 07 Jun 2016, Jan Cholasta wrote:

On 7.6.2016 10:17, Alexander Bokovoy wrote:

ipa: ERROR: AttributeError: 'str' object has no attribute 'decode'
Traceback (most recent call last):
File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1345, in run
 sys.exit(api.Backend.cli.run(argv))
File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1110, in run
 rv = cmd.output_for_cli(self.api.Backend.textui, result, *args,
**options)
File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py",
line 103, in output_for_cli
 qr = self._get_qrcode(output, uri, options['version'])
File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py",
line 61, in _get_qrcode
 qr_code = qr_output.getvalue().decode(encoding)
AttributeError: 'str' object has no attribute 'decode' ipa: ERROR: an
internal error has occurred


Looks rather like a py3 regression to me.


Fix attached, made a ticket https://fedorahosted.org/freeipa/ticket/5938


The check is incorrect actually - a proper fix would be to use encode 
instead of decode.

Updated patch attached. It indeed works.
--
/ Alexander Bokovoy
From be6167d050d30a9204a65c10519a23ff2a7f93f7 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy 
Date: Tue, 7 Jun 2016 11:37:41 +0300
Subject: [PATCH 5/7] otptoken: support Python 3 for the qr code

When IPA client is using Python 3, there is no str.decode() method
anymore.

ipa: ERROR: AttributeError: 'str' object has no attribute 'decode'
Traceback (most recent call last):
 File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1345, in run
   sys.exit(api.Backend.cli.run(argv))
 File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1110, in run
   rv = cmd.output_for_cli(self.api.Backend.textui, result, *args, **options)
 File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py", line 
103, in output_for_cli
   qr = self._get_qrcode(output, uri, options['version'])
 File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py", line 
61, in _get_qrcode
   qr_code = qr_output.getvalue().decode(encoding)
AttributeError: 'str' object has no attribute 'decode' ipa: ERROR: an internal 
error has occurred

Fixes https://fedorahosted.org/freeipa/ticket/5938
---
 ipaclient/plugins/otptoken.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipaclient/plugins/otptoken.py b/ipaclient/plugins/otptoken.py
index 5d3f1f8..d7d5356 100644
--- a/ipaclient/plugins/otptoken.py
+++ b/ipaclient/plugins/otptoken.py
@@ -58,7 +58,7 @@ class otptoken_add(MethodOverride):
 encoding = locale.getpreferredencoding(False)
 
 try:
-qr_code = qr_output.getvalue().decode(encoding)
+qr_code = qr_output.getvalue().encode(encoding)
 except UnicodeError:
 add_message(
 version,
-- 
2.7.4

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

[Freeipa-devel] [PATCH 0408] Do not apply forwarding configuration for disabled master zones

[Petr Spacek]

Hello,

Do not apply forwarding configuration for disabled master zones.

We have to respect idnsZoneActive attribute when calling
fwd_configure_zone().

https://fedorahosted.org/bind-dyndb-ldap/ticket/164

-- 
Petr^2 Spacek
From 1c59eeb30b5a3bf5a1b7626b029f400b86821554 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 7 Jun 2016 16:58:43 +0200
Subject: [PATCH] Do not apply forwarding configuration for disabled master
 zones.

We have to respect idnsZoneActive attribute when calling
fwd_configure_zone().

https://fedorahosted.org/bind-dyndb-ldap/ticket/164
---
 src/ldap_helper.c | 37 +
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 59a394d08bee893f12bf05b3bab4e9c8cc4a559c..c7a4c04e37cd2ef872efc0849bec7782fd024730 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -155,6 +155,7 @@ struct ldap_instance {
 	/* Settings. */
 	settings_set_t		*local_settings;
 	settings_set_t		*global_settings;
+	settings_set_t		empty_fwdz_settings;
 
 	sync_ctx_t		*sctx;
 	mldapdb_t		*mldapdb;
@@ -255,6 +256,12 @@ static setting_t settings_global_default[] = {
 	end_of_settings
 };
 
+static setting_t settings_fwdz_defaults[] = {
+	{ "forward_policy",	no_default_string	},
+	{ "forwarders",		no_default_string	},
+	end_of_settings
+};
+
 /*
  * Forward declarations.
  */
@@ -593,6 +600,14 @@ new_ldap_instance(isc_mem_t *mctx, const char *db_name,
 	if (settings_set_isfilled(ldap_inst->global_settings) != ISC_TRUE)
 		CLEANUP_WITH(ISC_R_FAILURE);
 
+	ldap_inst->empty_fwdz_settings = (settings_set_t) {
+			NULL,
+			"dummy LDAP zone forwarding settings",
+			ldap_inst->global_settings,
+			NULL,
+			(setting_t *) _fwdz_defaults[0]
+	};
+
 	CHECK(setting_get_uint("connections", ldap_inst->local_settings, ));
 
 	CHECK(zr_create(mctx, ldap_inst, ldap_inst->global_settings,
@@ -1148,6 +1163,10 @@ activate_zones(isc_task_t *task, ldap_instance_t *inst) {
 			result = activate_zone(task, inst, );
 			if (result == ISC_R_SUCCESS)
 ++published_cnt;
+			result = fwd_configure_zone(settings, inst, );
+			if (result != ISC_R_SUCCESS)
+log_error_r("could not configure forwarding");
+
 		}
 	};
 
@@ -1404,11 +1423,6 @@ ldap_parse_fwd_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst)
 	char name_txt[DNS_NAME_FORMATSIZE];
 	isc_result_t result;
 
-	static const setting_t fwdz_defaults[] = {
-		{ "forward_policy",		no_default_string	},
-		{ "forwarders",			no_default_string	},
-		end_of_settings
-	};
 	settings_set_t *fwdz_settings = NULL;
 
 	REQUIRE(entry != NULL);
@@ -1424,7 +1438,7 @@ ldap_parse_fwd_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst)
 		goto cleanup;
 	}
 
-	CHECK(settings_set_create(inst->mctx, fwdz_defaults, sizeof(fwdz_defaults),
+	CHECK(settings_set_create(inst->mctx, settings_fwdz_defaults, sizeof(settings_fwdz_defaults),
   "fake fwdz settings", inst->global_settings,
   _settings));
 	result = fwd_parse_ldap(entry, fwdz_settings);
@@ -2011,12 +2025,7 @@ ldap_parse_master_zoneentry(ldap_entry_t * const entry, dns_db_t * const olddb,
    _settings));
 	CHECK(zone_master_reconfigure(entry, zone_settings, raw, secure, task));
 	result = fwd_parse_ldap(entry, zone_settings);
-	if (result == ISC_R_SUCCESS) {
-		result = fwd_configure_zone(zone_settings, inst, >fqdn);
-		if (result != ISC_R_SUCCESS)
-			log_error_r("%s: could not configure forwarding",
-ldap_entry_logname(entry));
-	} else if (result != ISC_R_IGNORE)
+	if (result != ISC_R_SUCCESS && result != ISC_R_IGNORE)
 		goto cleanup;
 	/* synchronize zone origin with LDAP */
 	CHECK(zr_get_zone_dbs(inst->zone_register, >fqdn, , ));
@@ -2080,9 +2089,13 @@ ldap_parse_master_zoneentry(ldap_entry_t * const entry, dns_db_t * const olddb,
 		if (new_zone == ISC_TRUE || activity_changed == ISC_TRUE)
 			CHECK(publish_zone(task, inst, toview));
 		CHECK(load_zone(toview, ISC_FALSE));
+		CHECK(fwd_configure_zone(zone_settings, inst, >fqdn));
 	} else if (activity_changed == ISC_TRUE) { /* Zone was deactivated */
 		CHECK(unpublish_zone(inst, >fqdn,
  ldap_entry_logname(entry)));
+		/* emulate "no explicit forwarding config" */
+		CHECK(fwd_configure_zone(>empty_fwdz_settings, inst,
+	 >fqdn));
 		dns_zone_log(toview, ISC_LOG_INFO, "zone deactivated "
 			 "and removed from view");
 	}
-- 
2.5.5

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

Re: [Freeipa-devel] [PATCH] 0066 Load server plugins in certmonger renewal helper

[Jan Cholasta]

On 8.6.2016 07:22, Fraser Tweedale wrote:

Client/server plugin split apparently broke the certmonger renewal
helper (https://fedorahosted.org/freeipa/ticket/5943).  Please
review attached patch - hopefully it is correct way to fix it.


It is the correct way. Thanks for the patch, ACK.

Pushed to master: 6b3db0dc736a74c54b41f0746f1abc2091ba1be8

--
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] 0041: webui: add create/retrieve keytab tables for hosts

[Pavel Vomacka]

On 06/08/2016 02:20 PM, Petr Vobornik wrote:

On 06/06/2016 04:17 PM, Pavel Vomacka wrote:

Hello,

please review attached patch.

Ticket: https://fedorahosted.org/freeipa/ticket/5931


Also tables for host groups are needed.

+ the same UI should be also on host page.

Added, please see attached patch.
From 3b32ca1641fa388cd23bc7437f28e99ecbcba9b2 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Mon, 6 Jun 2016 12:59:24 +0200
Subject: [PATCH] Add lists of hosts allowed to create or retrieve keytabs

Attributes tables are added on host and service pages.

https://fedorahosted.org/freeipa/ticket/5931
---
 install/ui/src/freeipa/host.js| 64 +++
 install/ui/src/freeipa/service.js | 64 +++
 2 files changed, 128 insertions(+)

diff --git a/install/ui/src/freeipa/host.js b/install/ui/src/freeipa/host.js
index 817e62efca7dca9745c71ee4d4d4823f885a8fab..d1795d5bf1fe4b593fd7db87fb1385f8a31ed07a 100644
--- a/install/ui/src/freeipa/host.js
+++ b/install/ui/src/freeipa/host.js
@@ -187,6 +187,38 @@ return {
 link: true
 }
 ]
+},
+{
+$type: 'association_table',
+id: 'service_ipaallowedtoperform_read_keys_host',
+name: 'ipaallowedtoperform_read_keys_host',
+add_method: 'allow_retrieve_keytab',
+remove_method: 'disallow_retrieve_keytab',
+add_title: '@i18n:keytab.add_retrive',
+remove_title: '@i18n:keytab.remove_retrieve',
+columns: [
+{
+name: 'ipaallowedtoperform_read_keys_host',
+label: '@mo:host.label_singular',
+link: true
+}
+]
+},
+{
+$type: 'association_table',
+id: 'service_ipaallowedtoperform_read_keys_hostgroup',
+name: 'ipaallowedtoperform_read_keys_hostgroup',
+add_method: 'allow_retrieve_keytab',
+remove_method: 'disallow_retrieve_keytab',
+add_title: '@i18n:keytab.add_retrive',
+remove_title: '@i18n:keytab.remove_retrieve',
+columns: [
+{
+name: 'ipaallowedtoperform_read_keys_hosts_group',
+label: '@mo:hostgroup.label_singular',
+link: true
+}
+]
 }
 ]
 },
@@ -226,6 +258,38 @@ return {
 link: true
 }
 ]
+},
+{
+$type: 'association_table',
+id: 'service_ipaallowedtoperform_write_keys_host',
+name: 'ipaallowedtoperform_write_keys_host',
+add_method: 'allow_create_keytab',
+remove_method: 'disallow_create_keytab',
+add_title: '@i18n:keytab.add_create',
+remove_title: '@i18n:keytab.remove_create',
+columns: [
+{
+name: 'ipaallowedtoperform_write_keys_host',
+label: '@mo:host.label_singular',
+link: true
+}
+]
+},
+{
+$type: 'association_table',
+id: 'service_ipaallowedtoperform_write_keys_hostgroup',
+name: 'ipaallowedtoperform_write_keys_hostgroup',
+add_method: 'allow_create_keytab',
+remove_method: 'disallow_create_keytab',
+add_title: '@i18n:keytab.add_create',
+remove_title: '@i18n:keytab.remove_create',
+columns: [
+{
+name: 'ipaallowedtoperform_write_keys_hostgroup',
+label: '@mo:hostgroup.label_singular',
+link: true
+}
+   

Re: [Freeipa-devel] thin client regressions: otptoken

[Jan Cholasta]

On 7.6.2016 10:41, Alexander Bokovoy wrote:

On Tue, 07 Jun 2016, Jan Cholasta wrote:

On 7.6.2016 10:17, Alexander Bokovoy wrote:

ipa: ERROR: AttributeError: 'str' object has no attribute 'decode'
Traceback (most recent call last):
File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1345, in run
  sys.exit(api.Backend.cli.run(argv))
File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1110, in run
  rv = cmd.output_for_cli(self.api.Backend.textui, result, *args,
**options)
File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py",
line 103, in output_for_cli
  qr = self._get_qrcode(output, uri, options['version'])
File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py",
line 61, in _get_qrcode
  qr_code = qr_output.getvalue().decode(encoding)
AttributeError: 'str' object has no attribute 'decode' ipa: ERROR: an
internal error has occurred


Looks rather like a py3 regression to me.


Fix attached, made a ticket https://fedorahosted.org/freeipa/ticket/5938


The check is incorrect actually - a proper fix would be to use encode 
instead of decode.


--
Jan Cholasta

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

[Freeipa-devel] [PATCH 0409-0411] Parse idnsServerConfigObject and use its values for forwarder configuration Add LDAP schema for per-server config in LDAP Add server_ldap_settings layer to tree of se

[Petr Spacek]

Hello,

this patch set implements forwarder configuration in idnsServerConfigObject.

https://fedorahosted.org/bind-dyndb-ldap/ticket/162

-- 
Petr^2 Spacek
From 5fc2de3c7e43acc0cb776006b62d8d88a22f2d44 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 7 Jun 2016 12:40:03 +0200
Subject: [PATCH] Add server_ldap_settings layer to tree of setting objects.

Data are not loaded from LDAP yet so this commit has no user-visible
impact.

https://fedorahosted.org/bind-dyndb-ldap/ticket/162
---
 src/ldap_helper.c | 56 +++
 src/settings.c|  1 +
 src/settings.h|  1 +
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index c7a4c04e37cd2ef872efc0849bec7782fd024730..471f8a3d5191b47bbde99d4f55bc4462b8e3 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -156,6 +156,7 @@ struct ldap_instance {
 	settings_set_t		*local_settings;
 	settings_set_t		*global_settings;
 	settings_set_t		empty_fwdz_settings;
+	settings_set_t		*server_ldap_settings;
 
 	sync_ctx_t		*sctx;
 	mldapdb_t		*mldapdb;
@@ -244,6 +245,7 @@ static const setting_t settings_local_default[] = {
 	 * during start up to allow settings_set_isfilled() to pass.*/
 	{ "forward_policy",		no_default_string	},
 	{ "forwarders",			no_default_string	},
+	{ "server_id",			no_default_string	},
 	end_of_settings
 };
 
@@ -256,6 +258,14 @@ static setting_t settings_global_default[] = {
 	end_of_settings
 };
 
+/** Server-specific config from idnsServerConfig object. */
+static setting_t settings_server_ldap_default[] = {
+	{ "fake_mname",		no_default_string	},
+	{ "forwarders",		no_default_string	},
+	{ "forward_policy",	no_default_string	},
+	end_of_settings
+};
+
 static setting_t settings_fwdz_defaults[] = {
 	{ "forward_policy",	no_default_string	},
 	{ "forwarders",		no_default_string	},
@@ -516,6 +526,7 @@ new_ldap_instance(isc_mem_t *mctx, const char *db_name,
 	isc_uint32_t connections;
 	char settings_name[PRINT_BUFF_SIZE];
 	ldap_globalfwd_handleez_t *gfwdevent = NULL;
+	const char *server_id = NULL;
 
 	REQUIRE(ldap_instp != NULL && *ldap_instp == NULL);
 
@@ -600,17 +611,35 @@ new_ldap_instance(isc_mem_t *mctx, const char *db_name,
 	if (settings_set_isfilled(ldap_inst->global_settings) != ISC_TRUE)
 		CLEANUP_WITH(ISC_R_FAILURE);
 
+	/* zero-length server_id means undefined value */
+	CHECK(setting_get_str("server_id", ldap_inst->local_settings,
+			  _id));
+	if (strlen(server_id) == 0)
+		isc_string_printf_truncate(settings_name, PRINT_BUFF_SIZE,
+	   SETTING_SET_NAME_SERVER
+	   " for undefined server_id");
+	else
+		isc_string_printf_truncate(settings_name, PRINT_BUFF_SIZE,
+	   SETTING_SET_NAME_SERVER
+	   " for server id %s", server_id);
+
+	CHECK(settings_set_create(mctx, settings_server_ldap_default,
+	  sizeof(settings_server_ldap_default), settings_name,
+	  ldap_inst->global_settings, _inst->server_ldap_settings));
+	if (settings_set_isfilled(ldap_inst->server_ldap_settings) != ISC_TRUE)
+		CLEANUP_WITH(ISC_R_FAILURE);
+
 	ldap_inst->empty_fwdz_settings = (settings_set_t) {
 			NULL,
 			"dummy LDAP zone forwarding settings",
-			ldap_inst->global_settings,
+			ldap_inst->server_ldap_settings,
 			NULL,
 			(setting_t *) _fwdz_defaults[0]
 	};
 
 	CHECK(setting_get_uint("connections", ldap_inst->local_settings, ));
 
-	CHECK(zr_create(mctx, ldap_inst, ldap_inst->global_settings,
+	CHECK(zr_create(mctx, ldap_inst, ldap_inst->server_ldap_settings,
 			_inst->zone_register));
 	CHECK(fwdr_create(ldap_inst->mctx, _inst->fwd_register));
 	CHECK(mldap_new(mctx, _inst->mldapdb));
@@ -682,6 +711,7 @@ destroy_ldap_instance(ldap_instance_t **ldap_instp)
 
 	settings_set_free(_inst->global_settings);
 	settings_set_free(_inst->local_settings);
+	settings_set_free(_inst->server_ldap_settings);
 
 	sync_ctx_free(_inst->sctx);
 	/* zero out error counter (and do nothing other than that) */
@@ -1439,7 +1469,7 @@ ldap_parse_fwd_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst)
 	}
 
 	CHECK(settings_set_create(inst->mctx, settings_fwdz_defaults, sizeof(settings_fwdz_defaults),
-  "fake fwdz settings", inst->global_settings,
+  "fake fwdz settings", inst->server_ldap_settings,
   _settings));
 	result = fwd_parse_ldap(entry, fwdz_settings);
 	if (result == ISC_R_IGNORE) {
@@ -2392,31 +2422,31 @@ ldap_sasl_interact(LDAP *ld, unsigned flags, void *defaults, void *sin)
 		case SASL_CB_USER:
 			log_debug(4, "got request for SASL_CB_USER");
 			CHECK(setting_get_str("sasl_user",
-	  ldap_inst->global_settings,
+	  ldap_inst->server_ldap_settings,
 	  (const char **)>result));
 			in->len = strlen(in->result);
 			ret = LDAP_SUCCESS;
 			break;
 		case SASL_CB_GETREALM:
 			log_debug(4, "got request for SASL_CB_GETREALM");
 			CHECK(setting_get_str("sasl_realm",
-	  ldap_inst->global_settings,
+	  ldap_inst->server_ldap_settings,
 	  

[Freeipa-devel] [PATCH 0412] Fix interaction between root zone and global forwarders

[Petr Spacek]

Hello,

Fix interaction between root zone and global forwarders.

Finally the following priority order should be respected in all
circumstances:
- root zone (highest priority)
- server config in LDAP
- global config in LDAP
- named.conf

https://fedorahosted.org/bind-dyndb-ldap/ticket/165


This patch and all previous patches can be found in my Github repo in branch
server_config_in_ldap3.

-- 
Petr^2 Spacek
From 65809c64bc21994d663780607f6a0bfe11c44e26 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Wed, 8 Jun 2016 15:18:07 +0200
Subject: [PATCH] Fix interaction between root zone and global forwarders.

Finally the following priority order should be respected in all
circumstances:
- root zone (highest priority)
- server config in LDAP
- global config in LDAP
- named.conf

https://fedorahosted.org/bind-dyndb-ldap/ticket/165
---
 src/fwd.c | 36 
 src/fwd.h |  4 
 src/ldap_helper.c | 16 
 src/ldap_helper.h |  2 ++
 4 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/src/fwd.c b/src/fwd.c
index ba3e59ab107afeabc66cd8ae833bafbd0e13c89e..1f6a9e5d922d6a14dec88e04d41ad911f2dfd389 100644
--- a/src/fwd.c
+++ b/src/fwd.c
@@ -18,6 +18,7 @@
 #include "ldap_helper.h"
 #include "lock.h"
 #include "settings.h"
+#include "zone_register.h"
 
 const enum_txt_assoc_t forwarder_policy_txts[] = {
 	{ dns_fwdpolicy_none,	"none"	},
@@ -675,3 +676,38 @@ fwd_delete_table(dns_view_t *view, dns_name_t *name,
 		return ISC_R_SUCCESS; /* ISC_R_NOTFOUND = nothing to delete */
 	}
 }
+
+/**
+ * Reconfigure global forwarder using latest configuration in priority order:
+ * - root zone (if it is active)
+ * - server LDAP config
+ * - global LDAP config (inheritance is handled by settings tree)
+ * - named.conf (inheritance is handled by settings tree)
+ */
+isc_result_t
+fwd_reconfig_global(ldap_instance_t *inst) {
+	isc_result_t result;
+	settings_set_t *toplevel_settings = NULL;
+	isc_boolean_t root_zone_is_active = ISC_FALSE;
+
+	/* we have to respect forwarding configuration for root zone */
+	result = zr_get_zone_settings(ldap_instance_getzr(inst), dns_rootname,
+  _settings);
+	if (result == ISC_R_SUCCESS)
+		/* is root zone active? */
+		CHECK(setting_get_bool("active", toplevel_settings,
+   _zone_is_active));
+	else if (result != ISC_R_NOTFOUND)
+		goto cleanup;
+
+	if (root_zone_is_active == ISC_FALSE)
+		toplevel_settings = ldap_instance_getsettings_server(inst);
+
+	CHECK(fwd_configure_zone(toplevel_settings, inst, dns_rootname));
+	if (result != ISC_R_SUCCESS)
+		log_error_r("global forwarder could not be set up using %s",
+			toplevel_settings->name);
+
+cleanup:
+	return result;
+}
diff --git a/src/fwd.h b/src/fwd.h
index d1d0f5c491f61890a6cdf80f85b5277577acce47..8416d9578215e4fab562240026f1a532c6ac8e81 100644
--- a/src/fwd.h
+++ b/src/fwd.h
@@ -34,4 +34,8 @@ fwd_delete_table(dns_view_t *view, dns_name_t *name,
 		 const char *msg_obj_type, const char *logname)
 		 ATTR_NONNULLS ATTR_CHECKRESULT;
 
+isc_result_t
+fwd_reconfig_global(ldap_instance_t *inst)
+		ATTR_NONNULLS ATTR_CHECKRESULT;
+
 #endif /* _LD_FWD_H_ */
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 7f8774079cc11f13b31d77fe3e6e262f97443603..081fa37ee4c5b0c6a52339114c8892071c261a40 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1418,10 +1418,7 @@ ldap_parse_configentry(ldap_entry_t *entry, ldap_instance_t *inst)
 
 	result = fwd_parse_ldap(entry, inst->global_settings);
 	if (result == ISC_R_SUCCESS) {
-		result = fwd_configure_zone(inst->global_settings, inst,
-	dns_rootname);
-		if (result != ISC_R_SUCCESS)
-			log_error_r("global forwarder could not be set up");
+		CHECK(fwd_reconfig_global(inst));
 	} else if (result != ISC_R_IGNORE)
 		goto cleanup;
 
@@ -1458,10 +1455,7 @@ ldap_parse_serverconfigentry(ldap_entry_t *entry, ldap_instance_t *inst)
 
 	result = fwd_parse_ldap(entry, inst->server_ldap_settings);
 	if (result == ISC_R_SUCCESS) {
-		result = fwd_configure_zone(inst->server_ldap_settings, inst,
-	dns_rootname);
-		if (result != ISC_R_SUCCESS)
-			log_error_r("global forwarder could not be set up");
+		CHECK(fwd_reconfig_global(inst));
 	} else if (result != ISC_R_IGNORE)
 		goto cleanup;
 
@@ -4453,6 +4447,12 @@ ldap_instance_getsettings_local(ldap_instance_t *ldap_inst)
 	return ldap_inst->local_settings;
 }
 
+settings_set_t *
+ldap_instance_getsettings_server(ldap_instance_t *ldap_inst)
+{
+	return ldap_inst->server_ldap_settings;
+}
+
 const char *
 ldap_instance_getdbname(ldap_instance_t *ldap_inst)
 {
diff --git a/src/ldap_helper.h b/src/ldap_helper.h
index 1d691a29a06db645acb3979a1425df9ecb8577d7..0368ec7343ef7b16e7afb25b17f3067bf7c09f76 100644
--- a/src/ldap_helper.h
+++ b/src/ldap_helper.h
@@ -78,6 +78,8 @@ ldap_mod_free(isc_mem_t *mctx, LDAPMod **changep);
 
 settings_set_t * ldap_instance_getsettings_local(ldap_instance_t *ldap_inst) ATTR_NONNULLS;
 
+settings_set_t * 

Re: [Freeipa-devel] [PATCH] 0003 batch command can be used to trigger internal errors on server

[Petr Spacek]

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


-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Jan Cholasta]

On 6.6.2016 15:25, Fraser Tweedale wrote:

On Wed, Jun 01, 2016 at 02:49:06PM +1000, Fraser Tweedale wrote:

Updated patch attached; comments inline below.

On Mon, Apr 25, 2016 at 07:55:46AM +0200, Jan Cholasta wrote:

I think it would be better to merge the `client` and `client_servicename`
into a single `client_principal` argument, as both of the arguments are used
only to specify the principal name of the client.


Done.


Also I would prefer if the keyfile and keytab arguments were required,
because it's better if you can explicitly see what values are used at the
call site.


Done.


Why is init_creds() now called from __init__()? Why is it still called from
_auth_header()?


I invoke it from __init__ for more eager failure if there's a
problem (e.g. service is not in keytab), giving better error
locality.

It remains necessary to invoke it from _auth_header in case of
short-lived credentials.  I added some comments to the source.


Why is ldap_uri now passed to IPAKEMKeys()?


It tries to use LDAPI by default, so ldap_uri needs to be passed
through if process owner cannot access LDAPI socket.  I added
commentary to source.

Regarding your suggestion to make a base class and override class
variables, I prefer to pass values around.  There are very few
instantiations of CustodiaClient so it is easy enough to follow.


It may be easy to follow, but what you are currently doing is basically 
emulating subclassing with functools.partial, which is both unidiomatic 
and less readable than actual subclassing.


But we are past the nitpicking phase, so I'm going to let that slide :-)




Self-NACK on 0051-4; rebased and updated patch attached.

Only one substantive change, in custodiainstance.py:

-client_service='host@' + self.fqdn,
+client_service='host@%s' % self.fqdn,

First version broke uninstall because CustodiaInstance gets
initialised with 'host_name = None' and '+' doesn't like None.


This will give "host@None" on uninstall, but I guess that's OK, since 
it's currently not used during uninstall.


ACK.

Pushed to master: f94ccca6761f7dbe3f99855d181fe2cec380d476

--
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] 0052..0054 Configure lightweight CA key replication

[Jan Cholasta]

On 6.6.2016 15:32, Fraser Tweedale wrote:

On Wed, Jun 01, 2016 at 02:49:29PM +1000, Fraser Tweedale wrote:

Updated patches attached; comments inline.

On Thu, May 05, 2016 at 04:52:29PM +1000, Fraser Tweedale wrote:

I would rather add a new ACI than have one super-ACI for everything. That
way you don't have to invent any complicated naming schemes *and* it will be
more apparent what the ACI does.


OK, I'll simplify the scheme and create corresponding ACIs.


I added new ACIs for hosts to manage Dogtag keys; they keys live in
a container with RDN cn=dogtag, nested under the main custodia keys
container.


However, calling `CAInstance.setup_lightweight_ca_key_retrieval()'
*directly* from `ca.install_step_1' would probably work.  Are you
happy with putting it there, instead of `configure_instance()'?


Works for me.


Cool, thanks.


This is implemented in the latest patch.


Rebased and updated patches attached.  The only substantive change
is a simplification of the ipa-pki-retrieve-key script (patch 0054)
following a change in Dogtag's ExternalProcessKeyRetriever (it now
handles JSON).


Patch 0052:

The target of the "Dogtag service principals can search Custodia keys" 
ACI matches keys in the top-level Custodia container, but not in the 
Dogtag container. Is this intentional?



Patch 0053:

It seems the `servicename`+`host` and `principal` arguments of set_key() 
are carrying the same information, could you remove one of them?



Patch 0054:

1) Please use ipalib.config to read IPA configuration in 
ipa-pki-retrieve-key:


from ipalib.config import Env

env = Env(in_server=True)
hostname = env.host
realm = env.realm

2) I'm curious why you changed the key name from "ca/$NAME" to 
"ca_wrapped/$NAME". Aren't *all* keys in Custodia wrapped?


3) Given that Dogtag ExternalProcessKeyRetriever handles JSON *now*, I 
would expect a minimum required version bump in the spec file.


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

[Jan Cholasta]

On 8.6.2016 05:15, Fraser Tweedale wrote:

On Tue, Jun 07, 2016 at 03:42:22PM +1000, Fraser Tweedale wrote:

On Wed, Jun 01, 2016 at 02:51:04PM +1000, Fraser Tweedale wrote:

Hi team,

This patchset implements the 'ca' plugin for creating and managing
lightweight sub-CAs, and updates the 'caacl' plugin and
'cert-request' command to support multiple CAs.

A brief overview of the patches:

0059
  'ca' plugin, associated schema changes and container objects,
  Dogtag REST API wrapper
0060
  Add CA entry for the IPA CA on install/upgrade
0061
  Update 'caacl' plugin with CA support (including enforcement)
0062
  Update ra.request_certificate() to support specifying target CA
0063
  Add '--ca' option to 'cert-request' command
0064
  Add '--issuer' option to 'cert-find' command

These patches depend on other pending patches:

0051, 0052, 0053, 0054, 0055, 0056

Signing key replication depends on unmerged Dogtag patches.  Builds
of Dogtag with the required patches, and of FreeIPA with all
completed sub-CAs work, should be available from my COPR soon:
https://copr.fedorainfracloud.org/coprs/ftweedal/freeipa/

Some parts of the design are not implemented in the current
patchset, including:

- local parent CA (ipaca object) references
- sub-CA certificate renewal
- 'cert-show' command '--ca=NAME' option
- certmonger support for specifying CA
- revocation of deleted CAs

I look forward to your reviews!

Thanks,
Fraser


Rebased and updated patches attached.

Substantive changes:

- add required attributes for issuer DN and subject DN
- prevent rename of IPA CA
- when adding IPA CA entry, contact Dogtag to learn authority id,
  issuer DN and subject DN
- add 'read_ca' method to Dogtag interface
- tighten ACIs to prevent modification of ipacaid attribute


Updated patch 0064-3; adds --issuer option to cert-show and --ca
option to cert-show and cert-find.


Patch 0059:

1) On upgrade, why is the lightweight CA container created twice - once 
in 41-subca.update, once using ensure_entry() call? It should be done 
only once.


2) In ca_del, every CA specified in args[0] should be deleted, not just 
the first one.


3) Do not use NonFatalError, issue a warning instead:

self.add_message(MyNewWarningClass(name=...))

4) Can it actually happen that ca_show does not return ipacaid? I guess 
not, so you should be able to remove the check altogether and don't 
bother with the warning.



Patch 0060-0062: LGTM


Patch 0063:

Could you please define the CA param as follows:

Str('cacn?',
cli_name='ca',
query=True,
label=_("CA"),
doc=_("CA to use"),
),

?

This is for consitency with framework-generated parent key params, which 
unfortunately we cannot leverage in cert_request currently.



Patch 0064:

1) See my comment for patch 0063, it applies here as well.

2) The --issuer option should not be included in cert_show - show 
commands are supposed to retrieve an object given primary key(s), and 
the primary key of CA objects is just their cn.


3) In find commands, the options form a filter, so instead of raising 
MutuallyExclusiveError in cert-find, return an empty result, as with any 
other unmatched filter.



--
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] 0006 add context to exception on LdapEntry decode error

[Stanislav Laznicka]

On 06/08/2016 01:13 PM, Stanislav Laznicka wrote:




On 06/07/2016 05:11 PM, Florence Blanc-Renaud wrote:

On 06/07/2016 04:08 PM, Stanislav Laznicka wrote:

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


Hi,

please find attached the patch for Ticket 5434 add context to 
exception on LdapEntry decode error


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




Hello,

Similarly to you patch 0003, could you please shorten the commit 
message? I appreciate that you added the way to reproduce the bug 
but I would rather see it in the comments of the ticket.


I haven't tested the patch yet but I think that the try-except block 
should also wrap the self._conn.decode in the raw_dels loop:


 310 for value in raw_dels:
 311 value = self._conn.decode(value, name)
 312 if value in nice_adds:
 313 continue
 314 nice.remove(value)

Standa


Hi Standa,

thanks for your review. I will shorten the commit message as you 
suggested.


Regarding the other decode() called for raw_dels, I was not sure in 
which case this part of the code was called. Do you happen to know if 
it possible for an invalid value to be in the raw_sync list but not 
in the raw list?


Flo.

I'm not sure if there's an easy way to do that. I would just put it 
there should it ever occur to have more information about what happened.

Forgot to include devel-list, sorry.
-- 
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

[Petr Vobornik]

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.

+1

The commit message is very good and honestly I'd like to see more of
such commit messages.

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

Batch command deserves a doc string, so that it would be visible in API
browser but that is not subject of this ticket. Btw, expected format is
already in the code.

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

+1

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


-- 
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] [WIP] Thin client

[David Kupka]

On 28/04/16 14:45, Jan Cholasta wrote:

Hi,

I have pushed my thin client WIP branch to GitHub:
.

All commits up to "ipalib: use relative imports for cross-plugin
imports" should be good for review. The rest is subject to change
(WARNING: I will force push into this branch).

Honza



Hello!

Patch set:

spec file: require correct packages to get API plugins
schema: fix typo
schema: fix topic command output
replica install: use remote server API to create service entries
schema: do not validate unrequested params in command_defaults

fixes some regressions introduced by previous patches. Works for me and 
I haven't met any new regression or bug, ACK.


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

[Jan Cholasta]

On 8.6.2016 13:37, Pavel Vomacka wrote:



On 06/08/2016 01:21 PM, Pavel Vomacka wrote:




On 06/08/2016 05:15 AM, Fraser Tweedale wrote:

On Tue, Jun 07, 2016 at 03:42:22PM +1000, Fraser Tweedale wrote:

On Wed, Jun 01, 2016 at 02:51:04PM +1000, Fraser Tweedale wrote:

Hi team,

This patchset implements the 'ca' plugin for creating and managing
lightweight sub-CAs, and updates the 'caacl' plugin and
'cert-request' command to support multiple CAs.

A brief overview of the patches:

0059
  'ca' plugin, associated schema changes and container objects,
  Dogtag REST API wrapper
0060
  Add CA entry for the IPA CA on install/upgrade
0061
  Update 'caacl' plugin with CA support (including enforcement)
0062
  Update ra.request_certificate() to support specifying target CA
0063
  Add '--ca' option to 'cert-request' command
0064
  Add '--issuer' option to 'cert-find' command

These patches depend on other pending patches:

0051, 0052, 0053, 0054, 0055, 0056

Signing key replication depends on unmerged Dogtag patches.  Builds
of Dogtag with the required patches, and of FreeIPA with all
completed sub-CAs work, should be available from my COPR soon:
https://copr.fedorainfracloud.org/coprs/ftweedal/freeipa/

Some parts of the design are not implemented in the current
patchset, including:

- local parent CA (ipaca object) references
- sub-CA certificate renewal
- 'cert-show' command '--ca=NAME' option
- certmonger support for specifying CA
- revocation of deleted CAs

I look forward to your reviews!

Thanks,
Fraser


Rebased and updated patches attached.

Substantive changes:

- add required attributes for issuer DN and subject DN
- prevent rename of IPA CA
- when adding IPA CA entry, contact Dogtag to learn authority id,
  issuer DN and subject DN
- add 'read_ca' method to Dogtag interface
- tighten ACIs to prevent modification of ipacaid attribute


Updated patch 0064-3; adds --issuer option to cert-show and --ca
option to cert-show and cert-find.



Hello,

why is there --rename option in ca-mod command? Shouldn't it be rather
--cn to be consistent with ca-show?

Actually, I meant to be consistent with attribute name in result of API
call of ca-show command.

Is there any reason why to have there rename? Just a note: I look at
it mainly from point of view of WebUI.


It is consistent with other mod commands, they all have --rename if the 
object is renameable. You can't use the primary key name (cn) because 
that's already taken by the positional argument of the show command.


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

[Stanislav Laznicka]

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)?
From d554d96135160c8adcd3c798876de1e5ae83b4a0 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 7 Jun 2016 12:11:39 +0200
Subject: [PATCH 1/2] Revert "Removed dead code from
 LDAP{Remove,Add}ReverseMember"

While the code was really dead, it should serve a purpose elsewhere.
This reverts commit c56d65b064e1e0410c03cf1206816cad4d8d86cc.
---
 ipaserver/plugins/baseldap.py | 16 
 1 file changed, 16 insertions(+)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 7367c87988bfa6f1db5c38c6dd633e541f59d27e..62b726da1a0baef9fc9fa4bb386a2101ca1f10b2 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2131,6 +2131,14 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
+if options.get('all', False):
+attrs_list = ['*'] + self.obj.default_attributes
+else:
+attrs_list = set(self.obj.default_attributes)
+if options.get('no_members', False):
+attrs_list.difference_update(self.obj.attribute_members)
+attrs_list = list(attrs_list)
+
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
@@ -,6 +2230,14 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
+if options.get('all', False):
+attrs_list = ['*'] + self.obj.default_attributes
+else:
+attrs_list = set(self.obj.default_attributes)
+if options.get('no_members', False):
+attrs_list.difference_update(self.obj.attribute_members)
+attrs_list = list(attrs_list)
+
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
-- 
2.5.5

From 5a3c58f27efe73bcf478051d502df246ed10d0d2 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 7 Jun 2016 13:51:46 +0200
Subject: [PATCH 2/2] The LDAP*ReverseMember shouldn't imply --all is always
 specified

The LDAP*ReverseMember methods would always return the whole LDAP
object even though --all is not specified.

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 62b726da1a0baef9fc9fa4bb386a2101ca1f10b2..22eea887c4425bf890e0dc0da9e8779812fe5769 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2159,7 +2159,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry_attrs = ldap.get_entry(dn, ['*'])
+entry_attrs = ldap.get_entry(dn, attrs_list + ['objectclass'])
 self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
 for callback in self.get_callbacks('post'):
@@ -2258,7 +2258,7 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
   

Re: [Freeipa-devel] [PATCH] 0041: webui: add create/retrieve keytab tables for hosts

[Petr Vobornik]

On 06/06/2016 04:17 PM, Pavel Vomacka wrote:
> Hello,
> 
> please review attached patch.
> 
> Ticket: https://fedorahosted.org/freeipa/ticket/5931
> 

Also tables for host groups are needed.

+ the same UI should be also on host page.
-- 
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] [WIP] Thin client

[Jan Cholasta]

On 8.6.2016 14:40, Martin Babinsky wrote:

On 06/08/2016 02:11 PM, David Kupka wrote:

On 28/04/16 14:45, Jan Cholasta wrote:

Hi,

I have pushed my thin client WIP branch to GitHub:
.

All commits up to "ipalib: use relative imports for cross-plugin
imports" should be good for review. The rest is subject to change
(WARNING: I will force push into this branch).

Honza



Hello!

Patch set:

spec file: require correct packages to get API plugins
schema: fix typo
schema: fix topic command output
replica install: use remote server API to create service entries

This one is not complete since it breaks replica install w/ --setup-dns.
Removing this line seems to fix this case:

"""
diff --git a/ipaserver/install/server/replicainstall.py
b/ipaserver/install/server/replicainstall.py
index 41eee96..d695a15 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -1478,7 +1478,6 @@ def promote(installer):
 server_api.finalize()

 if options.setup_dns:
-server_api.Backend.rpcclient.connect()
 server_api.Backend.ldap2.connect(autobind=True)
 dns.install(False, True, options, server_api
"""


Fixed.





schema: do not validate unrequested params in command_defaults

fixes some regressions introduced by previous patches. Works for me and
I haven't met any new regression or bug, ACK.







--
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] [WIP] Thin client

[Martin Babinsky]

On 06/08/2016 02:11 PM, David Kupka wrote:

On 28/04/16 14:45, Jan Cholasta wrote:

Hi,

I have pushed my thin client WIP branch to GitHub:
.

All commits up to "ipalib: use relative imports for cross-plugin
imports" should be good for review. The rest is subject to change
(WARNING: I will force push into this branch).

Honza



Hello!

Patch set:

spec file: require correct packages to get API plugins
schema: fix typo
schema: fix topic command output
replica install: use remote server API to create service entries
This one is not complete since it breaks replica install w/ --setup-dns. 
Removing this line seems to fix this case:


"""
diff --git a/ipaserver/install/server/replicainstall.py 
b/ipaserver/install/server/replicainstall.py

index 41eee96..d695a15 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -1478,7 +1478,6 @@ def promote(installer):
 server_api.finalize()

 if options.setup_dns:
-server_api.Backend.rpcclient.connect()
 server_api.Backend.ldap2.connect(autobind=True)
 dns.install(False, True, options, server_api
"""



schema: do not validate unrequested params in command_defaults

fixes some regressions introduced by previous patches. Works for me and
I haven't met any new regression or bug, 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] 0059..0064 Lightweight sub-CAs

[Pavel Vomacka]

On 06/08/2016 05:15 AM, Fraser Tweedale wrote:

On Tue, Jun 07, 2016 at 03:42:22PM +1000, Fraser Tweedale wrote:

On Wed, Jun 01, 2016 at 02:51:04PM +1000, Fraser Tweedale wrote:

Hi team,

This patchset implements the 'ca' plugin for creating and managing
lightweight sub-CAs, and updates the 'caacl' plugin and
'cert-request' command to support multiple CAs.

A brief overview of the patches:

0059
   'ca' plugin, associated schema changes and container objects,
   Dogtag REST API wrapper
0060
   Add CA entry for the IPA CA on install/upgrade
0061
   Update 'caacl' plugin with CA support (including enforcement)
0062
   Update ra.request_certificate() to support specifying target CA
0063
   Add '--ca' option to 'cert-request' command
0064
   Add '--issuer' option to 'cert-find' command

These patches depend on other pending patches:

 0051, 0052, 0053, 0054, 0055, 0056

Signing key replication depends on unmerged Dogtag patches.  Builds
of Dogtag with the required patches, and of FreeIPA with all
completed sub-CAs work, should be available from my COPR soon:
https://copr.fedorainfracloud.org/coprs/ftweedal/freeipa/

Some parts of the design are not implemented in the current
patchset, including:

- local parent CA (ipaca object) references
- sub-CA certificate renewal
- 'cert-show' command '--ca=NAME' option
- certmonger support for specifying CA
- revocation of deleted CAs

I look forward to your reviews!

Thanks,
Fraser


Rebased and updated patches attached.

Substantive changes:

- add required attributes for issuer DN and subject DN
- prevent rename of IPA CA
- when adding IPA CA entry, contact Dogtag to learn authority id,
   issuer DN and subject DN
- add 'read_ca' method to Dogtag interface
- tighten ACIs to prevent modification of ipacaid attribute


Updated patch 0064-3; adds --issuer option to cert-show and --ca
option to cert-show and cert-find.



Hello,

why is there --rename option in ca-mod command? Shouldn't it be rather 
--cn to be consistent with ca-show? Is there any reason why to have 
there rename? Just a note: I look at it mainly from point of view of WebUI.


--
Pavel^3 Vomacka
-- 
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

[Pavel Vomacka]

On 06/08/2016 01:21 PM, Pavel Vomacka wrote:




On 06/08/2016 05:15 AM, Fraser Tweedale wrote:

On Tue, Jun 07, 2016 at 03:42:22PM +1000, Fraser Tweedale wrote:

On Wed, Jun 01, 2016 at 02:51:04PM +1000, Fraser Tweedale wrote:

Hi team,

This patchset implements the 'ca' plugin for creating and managing
lightweight sub-CAs, and updates the 'caacl' plugin and
'cert-request' command to support multiple CAs.

A brief overview of the patches:

0059
   'ca' plugin, associated schema changes and container objects,
   Dogtag REST API wrapper
0060
   Add CA entry for the IPA CA on install/upgrade
0061
   Update 'caacl' plugin with CA support (including enforcement)
0062
   Update ra.request_certificate() to support specifying target CA
0063
   Add '--ca' option to 'cert-request' command
0064
   Add '--issuer' option to 'cert-find' command

These patches depend on other pending patches:

 0051, 0052, 0053, 0054, 0055, 0056

Signing key replication depends on unmerged Dogtag patches.  Builds
of Dogtag with the required patches, and of FreeIPA with all
completed sub-CAs work, should be available from my COPR soon:
https://copr.fedorainfracloud.org/coprs/ftweedal/freeipa/

Some parts of the design are not implemented in the current
patchset, including:

- local parent CA (ipaca object) references
- sub-CA certificate renewal
- 'cert-show' command '--ca=NAME' option
- certmonger support for specifying CA
- revocation of deleted CAs

I look forward to your reviews!

Thanks,
Fraser


Rebased and updated patches attached.

Substantive changes:

- add required attributes for issuer DN and subject DN
- prevent rename of IPA CA
- when adding IPA CA entry, contact Dogtag to learn authority id,
   issuer DN and subject DN
- add 'read_ca' method to Dogtag interface
- tighten ACIs to prevent modification of ipacaid attribute


Updated patch 0064-3; adds --issuer option to cert-show and --ca
option to cert-show and cert-find.



Hello,

why is there --rename option in ca-mod command? Shouldn't it be rather 
--cn to be consistent with ca-show? 
Actually, I meant to be consistent with attribute name in result of API 
call of ca-show command.
Is there any reason why to have there rename? Just a note: I look at 
it mainly from point of view of WebUI.


--
Pavel^3 Vomacka




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

[Florence Blanc-Renaud]

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.

From 927a918ee1a51add5f45786919fe75920ffe5f78 Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud 
Date: Fri, 27 May 2016 08:19:39 +0200
Subject: [PATCH] batch command can be used to trigger internal errors on
 server

In ipalib, the batch command expects a specific format for arguments.
The code did not check the format of the parameters, which could trigger
internal errors on the server.
With this fix:
- a ConversionError is raised if the arg passed to batch() is not a list of
dict
- the result appended to the batch results is a ConversionError if the
'params' does not contain a tuple(list,dict)

https://fedorahosted.org/freeipa/ticket/5810
---
 ipaserver/plugins/batch.py | 24 +---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/ipaserver/plugins/batch.py b/ipaserver/plugins/batch.py
index 84a65057588e01e18a937c6e79f0974b25c529ed..aebdc2f72615eeafeec614fe8b33df4ada7688c7 100644
--- a/ipaserver/plugins/batch.py
+++ b/ipaserver/plugins/batch.py
@@ -90,6 +90,12 @@ class batch(Command):
 def execute(self, methods=None, **options):
 results = []
 for arg in (methods or []):
+# As take_args = Any, no check is done before
+# Need to make sure that methods contain dict objects
+if not isinstance(arg, dict):
+raise errors.ConversionError(
+name='methods',
+error=_(u'must contain dict objects'))
 params = dict()
 name = None
 try:
@@ -100,9 +106,21 @@ class batch(Command):
 name = arg['method']
 if name not in self.Command:
 raise errors.CommandError(name=name)
-a, kw = arg['params']
-newkw = dict((str(k), v) for k, v in kw.items())
-params = api.Command[name].args_options_2_params(*a, **newkw)
+
+# If params are not formated as a tuple(list, dict)
+# the following lines will raise an exception
+# that triggers an internal server error
+# Raise a ConversionError instead to report the issue
+# to the client
+try:
+a, kw = arg['params']
+newkw = dict((str(k), v) for k, v in kw.items())
+params = api.Command[name].args_options_2_params(
+*a, **newkw)
+except (AttributeError, ValueError, TypeError):
+raise errors.ConversionError(
+name='params',
+error=_(u'must contain a tuple (list, dict)'))
 newkw.setdefault('version', options['version'])
 
 result = api.Command[name](*a, **newkw)
-- 
2.5.5

-- 
Manage your subscription for the 

Re: [Freeipa-devel] [PATCH] 0052..0054 Configure lightweight CA key replication

[Jan Cholasta]

On 9.6.2016 06:07, Fraser Tweedale wrote:

Updated patches 0053-6 and 0054-6 attached.  Comments inline.

Thanks,
Fraser

On Wed, Jun 08, 2016 at 10:31:07AM +0200, Jan Cholasta wrote:

Patch 0052:

The target of the "Dogtag service principals can search Custodia keys" ACI
matches keys in the top-level Custodia container, but not in the Dogtag
container. Is this intentional?


ACI applies to all entries under the 'cn=custodia' tree, not only
the entries directly under it.


You are right:



Mea culpa, I thought the target matching behavior was simpler than this.

Anyway, could the ACI be tightened to allow access only to the Dogtag 
subtree?






Patch 0053:

It seems the `servicename`+`host` and `principal` arguments of set_key() are
carrying the same information, could you remove one of them?


OK, just `principal` now.



Patch 0054:

1) Please use ipalib.config to read IPA configuration in
ipa-pki-retrieve-key:

from ipalib.config import Env

env = Env(in_server=True)
hostname = env.host
realm = env.realm


I have implemented this; it was necessary to also call
`env._finalize()`.


Right. I wrote the above snippet off the top of my head, hence the omission.




2) I'm curious why you changed the key name from "ca/$NAME" to
"ca_wrapped/$NAME". Aren't *all* keys in Custodia wrapped?


The payload of the `ca_wrapped` Custodia store is a
PKIArchiveOptions object wrapped by the main CA's private key.
(This payload is then encrypted by Custodia, as all Custodia
payloads are).

See my patch 0056 [1] for implementation details.

[1] https://www.redhat.com/archives/freeipa-devel/2016-May/msg00079.html


Wow, I totally missed patch 0055 and 0056!




3) Given that Dogtag ExternalProcessKeyRetriever handles JSON *now*, I would
expect a minimum required version bump in the spec file.


Indeed; I added this in latest patches.


Thanks. ACK if the ACI in patch 52 does not need tightening.

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

[Petr Vobornik]

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

-- 
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] 0052..0054 Configure lightweight CA key replication

[Fraser Tweedale]

Updated patches 0053-6 and 0054-6 attached.  Comments inline.

Thanks,
Fraser

On Wed, Jun 08, 2016 at 10:31:07AM +0200, Jan Cholasta wrote:
> Patch 0052:
> 
> The target of the "Dogtag service principals can search Custodia keys" ACI
> matches keys in the top-level Custodia container, but not in the Dogtag
> container. Is this intentional?
> 
ACI applies to all entries under the 'cn=custodia' tree, not only
the entries directly under it.

> 
> Patch 0053:
> 
> It seems the `servicename`+`host` and `principal` arguments of set_key() are
> carrying the same information, could you remove one of them?
> 
OK, just `principal` now.

> 
> Patch 0054:
> 
> 1) Please use ipalib.config to read IPA configuration in
> ipa-pki-retrieve-key:
> 
> from ipalib.config import Env
> 
> env = Env(in_server=True)
> hostname = env.host
> realm = env.realm
> 
I have implemented this; it was necessary to also call
`env._finalize()`.

> 2) I'm curious why you changed the key name from "ca/$NAME" to
> "ca_wrapped/$NAME". Aren't *all* keys in Custodia wrapped?
> 
The payload of the `ca_wrapped` Custodia store is a
PKIArchiveOptions object wrapped by the main CA's private key.
(This payload is then encrypted by Custodia, as all Custodia
payloads are).

See my patch 0056 [1] for implementation details.

[1] https://www.redhat.com/archives/freeipa-devel/2016-May/msg00079.html

> 3) Given that Dogtag ExternalProcessKeyRetriever handles JSON *now*, I would
> expect a minimum required version bump in the spec file.
> 
Indeed; I added this in latest patches.
From 5116bc9cd4a2f3f1dde4fbc5f941b091036fa78c Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Mon, 11 Apr 2016 12:42:35 +1000
Subject: [PATCH 53/54] Optionally add service name to Custodia key DNs

Lightweight CAs support introduces new service principals for
Dogtag, with Custodia keys.  The current Custodia key creation uses
a DN that contains only they key type and the hostname, so keys for
multiple services on the same host cannot be created.

Add the 'generate_keys' method to generate keys for a host or an
arbitrary service.  When a service name is given, add the key
entries in a nested container with RDN 'cn='.  (The
container is assumed to exist).

This change does not affect searching because subtree search is
used, filtering on the ipaKeyUsage and memberPrincipal attributes.

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 ipapython/secrets/kem.py | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/ipapython/secrets/kem.py b/ipapython/secrets/kem.py
index 
0abf28ae4403a7b6225404df361d12cb07ccc70b..d45efe8cc4fb63ae9d8c0b2c920fd1f9e5331a9d
 100644
--- a/ipapython/secrets/kem.py
+++ b/ipapython/secrets/kem.py
@@ -3,6 +3,7 @@
 from __future__ import print_function
 from ipaplatform.paths import paths
 from six.moves.configparser import ConfigParser
+from ipapython.dn import DN
 from cryptography.hazmat.backends import default_backend
 from cryptography.hazmat.primitives import serialization
 from cryptography.hazmat.primitives.asymmetric import rsa, ec
@@ -105,11 +106,24 @@ class KEMLdap(iSecLdap):
 encoding=serialization.Encoding.DER,
 format=serialization.PublicFormat.SubjectPublicKeyInfo)
 
-def set_key(self, usage, host, principal, key):
+def set_key(self, usage, principal, key):
+"""
+Write key for the host or service.
+
+Service keys are nested one level beneath the 'cn=custodia'
+container, in the 'cn=' container; this allows
+fine-grained control over key management permissions for
+specific services.
+
+The container is assumed to exist.
+
+"""
 public_key = self._format_public_key(key)
 conn = self.connect()
+servicename, host = principal.split('@')[0].split('/')
 name = '%s/%s' % (KEY_USAGE_MAP[usage], host)
-dn = 'cn=%s,%s' % (name, self.keysbase)
+service_rdn = ('cn', servicename) if servicename != 'host' else DN()
+dn = str(DN(('cn', name), service_rdn, self.keysbase))
 try:
 mods = [('objectClass', ['nsContainer',
  'ipaKeyPolicy',
@@ -170,15 +184,18 @@ class IPAKEMKeys(KEMKeysStore):
 return conn.get_key(usage, kid)
 
 def generate_server_keys(self):
-principal = 'host/%s@%s' % (self.host, self.realm)
+self.generate_keys('host')
+
+def generate_keys(self, servicename):
+principal = '%s/%s@%s' % (servicename, self.host, self.realm)
 # Neutralize the key with read if any
 self._server_keys = None
 # Generate private key and store it
 pubkeys = newServerKeys(self.config['server_keys'], principal)
 # Store public key in LDAP
 ldapconn = KEMLdap(self.ldap_uri)
-ldapconn.set_key(KEY_USAGE_SIG, self.host, principal, pubkeys[0])
-ldapconn.set_key(KEY_USAGE_ENC, 

Re: [Freeipa-devel] [PATCH] man: Decribe ipa-client-install workaround for broken D-Bus enviroment.

[David Kupka]

On 02/03/16 11:18, David Kupka wrote:

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



Sending updated version crafted with Flo's help, thanks.

--
David Kupka
From a7d878e3922720b03b36a2a3b697f8c6c66cc383 Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Wed, 2 Mar 2016 11:08:19 +0100
Subject: [PATCH] man: Decribe ipa-client-install workaround for broken D-Bus
 enviroment.

https://fedorahosted.org/freeipa/ticket/5694
---
 client/man/ipa-client-install.1 | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/client/man/ipa-client-install.1 b/client/man/ipa-client-install.1
index 92ea77a4bda539f8614f3d47cac7b53faf57482c..7f490d153b12d714e7bda7a6abe72fb0756d520c 100644
--- a/client/man/ipa-client-install.1
+++ b/client/man/ipa-client-install.1
@@ -176,6 +176,17 @@ valid for the IPA domain.
 .TP
 \fB\-\-request\-cert\fR
 Request certificate for the machine. The certificate will be stored in /etc/ipa/nssdb under the nickname "Local IPA host".
+
+Using this option requires that D-Bus is properly configured or not configured
+at all. In environment where this condition is not met (e.g. anaconda kickstart
+chroot environment) set the system bus address to /dev/null to enable
+workaround in ipa-client-install.
+
+# env DBUS_SYSTEM_BUS_ADDRESS=unix:path=/dev/null ipa-client-install --request-cert
+
+Note that requesting the certificate when certmonger is not running only
+creates tracking request and the certmonger service must be started to be able
+to track certificates.
 .TP
 \fB\-\-automount\-location\fR=\fILOCATION\fR
 Configure automount by running ipa\-client\-automount(1) with \fILOCATION\fR as
-- 
2.5.5

-- 
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] 0039-40: DNS Location: WebUI

[Petr Vobornik]

On 06/07/2016 10:07 AM, Pavel Vomacka wrote:
> 
> 
> On 06/06/2016 07:51 PM, Martin Basti wrote:
>>
>>
>>
>> On 05.06.2016 18:34, Pavel Vomacka wrote:
>>> Hello,
>>>
>>> please review attached patches which add WebUI part of DNS Locations 
>>> feature.
>>>
>>> -- 
>>> Pavel^3 Vomacka
>>>
>>>
>>
>> NACK
>>
>> 1)
>> When I edit location description and click on revert button, then that nice 
>> location table just disappear :)
> It's the same situation as with using 'Save' button - reported here: 
> https://fedorahosted.org/freeipa/ticket/5776 . I'll write there a comment 
> that 
> revert button hides values in association tables.
>>
>> 2)
>> Can we put a placeholder "100" (gray font or something) to Location weight 
>> in 
>> server detail view? Because when weight is not specified then default is 100
> Placeholder added.
> 


1. please add "disable_facet_tabs: true," to location details page.
There is no point to display single facet tab.

2. can we extend location_association_table the same witth as rule
tables are extended in sudo and hbac rules? IIRC it needs to use
specific section type of 'servers' section.

3. the placeholder 100 should be also added to adder dialog in
location_association_table_widget

4. Description could be textarea - to be consistent with hbac, sudo rules.

5. "Information" section is called "General" or "$Entity Settings" on
other parts of Web UI. It should be consistent.


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