[Freeipa-devel] [PATCH 0045] Enforce host existence only where needed in ipa-replica-manage
Hi, In ipa-replica-manage commands, we enforce that hostnames we work with are resolvable. However, this caused errors while deleting or disconnecting a ipa / winsync replica, if that replica was down and authoritative server for itself. https://fedorahosted.org/freeipa/ticket/3524 Tomas From f4024fa1d4a68a478572580ac3abde09fd1556df Mon Sep 17 00:00:00 2001 From: Tomas Babej tba...@redhat.com Date: Tue, 9 Apr 2013 13:45:34 +0200 Subject: [PATCH] Enforce host existence only where needed in ipa-replica-manage In ipa-replica-manage commands, we enforce that hostnames we work with are resolvable. However, this caused errors while deleting or disconnecting a ipa / winsync replica, if that replica was down and authoritative server for itself. https://fedorahosted.org/freeipa/ticket/3524 --- install/tools/ipa-replica-manage | 16 +--- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage index 636529caaeca222c09d94d3e5539002fcd3139a9..ae66a7b716fb3e23c71ba0ed7b75900d9fdebddd 100755 --- a/install/tools/ipa-replica-manage +++ b/install/tools/ipa-replica-manage @@ -120,6 +120,7 @@ def test_connection(realm, host): returns True if connection successful, False otherwise try: +enforce_host_existence(host) replman = replication.ReplicationManager(realm, host, None) ents = replman.find_replication_agreements() del replman @@ -136,8 +137,7 @@ def test_connection(realm, host): def list_replicas(realm, host, replica, dirman_passwd, verbose): -for check_host in [host, replica]: -enforce_host_existence(check_host) +enforce_host_existence(host) is_replica = False winsync_peer = None @@ -232,9 +232,6 @@ def del_link(realm, replica1, replica2, dirman_passwd, force=False): @force: force deletion even if one server is down -for check_host in [replica1, replica2]: -enforce_host_existence(check_host) - repl2 = None try: @@ -370,8 +367,6 @@ def list_ruv(realm, host, dirman_passwd, verbose): replica IDs. -enforce_host_existence(host) - servers = get_ruv(realm, host, dirman_passwd) for (netloc, rid) in servers: print %s: %s % (netloc, rid) @@ -549,8 +544,6 @@ def enforce_host_existence(host, message=None): def del_master(realm, hostname, options): -enforce_host_existence(hostname) - force_del = False delrepl = None @@ -868,8 +861,8 @@ def show_DNA_ranges(hostname, master, realm, dirman_passwd, nextrange=False): Returns nothing -for check_host in [hostname, master]: -enforce_host_existence(check_host) + +enforce_host_existence(hostname) try: repl = replication.ReplicationManager(realm, hostname, dirman_passwd) @@ -1214,3 +1207,4 @@ except RuntimeError, e: except Exception, e: print unexpected error: %s % str(e) sys.exit(1) + -- 1.8.1.4 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0142] Improve LDAP error logging
Hello,
Improve LDAP error logging.
Diagnostic error message is logged when it is available.
Plugin with this patch produces messages like:
LDAP error: Server is unwilling to perform: Minimum SSF not met.: bind to LDAP
server failed
intead of
bind to LDAP server failed: Server is unwilling to perform
Second example is:
LDAP error: Object class violation: attribute mgrecord not allowed
: while modifying(add) entry 'idnsName=pspacek,
idnsname=example.com,cn=dns,dc=e,dc=test'
instead of
:-D
--
Petr^2 Spacek
From 183a8019c8217b6db79766e0ac93c48344fb2498 Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Tue, 9 Apr 2013 15:19:36 +0200
Subject: [PATCH] Improve LDAP error logging.
Diagnostic error message is logged when it is available.
Signed-off-by: Petr Spacek pspa...@redhat.com
---
src/ldap_entry.c | 2 +-
src/ldap_helper.c | 11 +--
src/log.h | 33 -
3 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/src/ldap_entry.c b/src/ldap_entry.c
index 3e82b39d31c7ed13255de61d0763800b4d01efef..2a2c7b5291d446c248389ca37b4b51405b213aad 100644
--- a/src/ldap_entry.c
+++ b/src/ldap_entry.c
@@ -217,7 +217,7 @@ ldap_entry_create(isc_mem_t *mctx, LDAP *ld, LDAPMessage *ldap_entry,
entry-dn = ldap_get_dn(ld, ldap_entry);
if (entry-dn == NULL) {
- log_ldap_error(ld);
+ log_ldap_error(ld, unable to get entry DN);
CLEANUP_WITH(ISC_R_FAILURE);
}
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 385bc4710e9c431904ab99b2405b34c69ea8775d..e86060b0ca4ee2b5646324ae82770947c150b5ae 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -2412,8 +2412,7 @@ force_reconnect:
}
if (ret != LDAP_SUCCESS) {
- log_error(bind to LDAP server failed: %s,
- ldap_err2string(ret));
+ log_ldap_error(ldap_conn-handle, bind to LDAP server failed);
/*
* Clean the connection handle.
@@ -2475,12 +2474,13 @@ handle_connection_error(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn
break;
case LDAP_INVALID_DN_SYNTAX:
case LDAP_INVALID_SYNTAX:
- log_bug(Invalid syntax in handle_connection_error indicates a bug);
+ log_ldap_error(ldap_conn-handle, invalid syntax in
+ handle_connection_error indicates a bug);
result = ISC_R_UNEXPECTEDTOKEN;
break;
default:
/* Try to reconnect on other errors. */
- log_error(LDAP error: %s, ldap_err2string(err_code));
+ log_ldap_error(ldap_conn-handle, connection error);
reconnect:
if (ldap_conn-tries == 0)
log_error(connection to the LDAP server was lost);
@@ -2579,8 +2579,7 @@ ldap_modify_do(ldap_instance_t *ldap_inst, const char *dn, LDAPMod **mods,
operation_str = adding;
}
- log_debug(2, error(%s) %s entry %s, ldap_err2string(err_code),
- operation_str, dn);
+ log_ldap_error(ldap_conn-handle, while %s entry '%s', operation_str, dn);
/* do not error out if we are trying to delete an
* unexisting attribute */
diff --git a/src/log.h b/src/log.h
index 312f24322fd0c6f9943c6beb810ac0bcd8f3896c..cbf1a3faaaccea7391d65d018e80d8ec688fc111 100644
--- a/src/log.h
+++ b/src/log.h
@@ -55,15 +55,30 @@
log_write(GET_LOG_LEVEL(level), format, ##__VA_ARGS__)
/* LDAP logging functions */
-#define log_ldap_error(ld) \
- do {\
- int err; \
- char *errmsg = UNKNOWN;\
- if (ldap_get_option(ld, LDAP_OPT_RESULT_CODE, err) \
- == LDAP_OPT_SUCCESS)\
- errmsg = ldap_err2string(err); \
- log_error_position(LDAP error: %s, errmsg); \
- } while (0); \
+#define LOG_LDAP_ERR_PREFIX LDAP error:
+#define log_ldap_error(ld, desc, ...) \
+ do { \
+ int err; \
+ char *errmsg = NULL; \
+ char *diagmsg = NULL; \
+ if (ldap_get_option(ld, LDAP_OPT_RESULT_CODE, err) \
+ == LDAP_OPT_SUCCESS) { \
+ errmsg = ldap_err2string(err);\
+ if (ldap_get_option(ld, LDAP_OPT_DIAGNOSTIC_MESSAGE, diagmsg) \
+ == LDAP_OPT_SUCCESS diagmsg != NULL) { \
+errmsg = ldap_err2string(err); \
+log_error(LOG_LDAP_ERR_PREFIX %s: %s: desc, \
+ errmsg, diagmsg, ##__VA_ARGS__); \
+ldap_memfree(diagmsg);\
+ } else \
+log_error(LOG_LDAP_ERR_PREFIX %s: desc, \
+ errmsg, ##__VA_ARGS__); \
+ } else { \
+ log_error(LOG_LDAP_ERR_PREFIX\
+ unable to obtain LDAP error code: \
+ desc, ##__VA_ARGS__);\
+ }\
+ } while (0);
void
log_write(int level, const char *format, ...) ISC_FORMAT_PRINTF(2, 3);
--
1.7.11.7
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0143] Treat syntax errors in LDAP filters as fatal
Hello, Treat syntax errors in LDAP filters as fatal. Filters are hardcoded at the moment, this is preventive action. -- Petr^2 Spacek From 7d903641d343f3e083feed3e935d34c19ede2971 Mon Sep 17 00:00:00 2001 From: Petr Spacek pspa...@redhat.com Date: Tue, 9 Apr 2013 15:28:19 +0200 Subject: [PATCH] Treat syntax errors in LDAP filters as fatal. Filters are hardcoded at the moment, this is preventive action. Signed-off-by: Petr Spacek pspa...@redhat.com --- src/ldap_helper.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ldap_helper.c b/src/ldap_helper.c index e86060b0ca4ee2b5646324ae82770947c150b5ae..d19686f2c2fe48600e4d1a2eccac5da76f959c39 100644 --- a/src/ldap_helper.c +++ b/src/ldap_helper.c @@ -2474,6 +2474,7 @@ handle_connection_error(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn break; case LDAP_INVALID_DN_SYNTAX: case LDAP_INVALID_SYNTAX: + case LDAP_FILTER_ERROR: log_ldap_error(ldap_conn-handle, invalid syntax in handle_connection_error indicates a bug); result = ISC_R_UNEXPECTEDTOKEN; -- 1.7.11.7 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 125 Do actually stop pki_cad in stop_pkicad instead of starting it
Hi, this patch fixes https://fedorahosted.org/freeipa/ticket/3554. Honza -- Jan Cholasta From fca3caa0515e2ca37b9e04c3c960d59477ccd0a9 Mon Sep 17 00:00:00 2001 From: Jan Cholasta jchol...@redhat.com Date: Tue, 9 Apr 2013 15:49:15 +0200 Subject: [PATCH] Do actually stop pki_cad in stop_pkicad instead of starting it. https://fedorahosted.org/freeipa/ticket/3554 --- install/restart_scripts/stop_pkicad | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/install/restart_scripts/stop_pkicad b/install/restart_scripts/stop_pkicad index f023b1b..c8589b2 100644 --- a/install/restart_scripts/stop_pkicad +++ b/install/restart_scripts/stop_pkicad @@ -35,9 +35,9 @@ syslog.syslog(syslog.LOG_NOTICE, certmonger stopping %sd % dogtag_instance) try: if configured_constants.DOGTAG_VERSION == 9: -ipaservices.knownservices.pki_cad.start(dogtag_instance) +ipaservices.knownservices.pki_cad.stop(dogtag_instance) else: -ipaservices.knownservices.pki_tomcatd.start(dogtag_instance) +ipaservices.knownservices.pki_tomcatd.stop(dogtag_instance) except Exception, e: syslog.syslog(syslog.LOG_ERR, Cannot stop %sd: %s % (dogtag_instance, str(e))) -- 1.8.1.4 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 123 Use http instead of https for OCSP and CRL URLs in IPA certificate profile
On 04/08/2013 05:09 PM, Martin Kosek wrote: On 04/08/2013 03:47 PM, Dmitri Pal wrote: On 04/08/2013 08:42 AM, Martin Kosek wrote: On 04/08/2013 10:48 AM, Jan Cholasta wrote: On 8.4.2013 10:47, Jan Cholasta wrote: Hi, this patch fixes https://fedorahosted.org/freeipa/ticket/3552. Honza Re-sending with correct subject. I tested the change both for upgrades and for fresh installs and it worked fine both cases, even when testing with Firefox enforcing mode. So far, as the biggest issue in current process I see NSS not being able to fallback to other defined OCSP responder (I tested with Firefox 20). This way, Firefox will fail validating the FreeIPA site when the first tested OCSP responder is not available (e.g. the original IPA CA signing the http cert, or an `ipa-ca.$domain` host that is currently not up). Have we filed a ticket with FF? AFAIU, this is rather NSS issue, that Firefox issue. There is a bug open for NSS: https://bugzilla.mozilla.org/show_bug.cgi?id=797815 Rob seems to have more context about this bug background. Martin We may want to wait with pushing this patch until we get some response in the NSS Bugzilla above. If our request is rejected, we may be forced to use just a single CRL/OCSP (which would be probably the general one) and thus supersede patch 123. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 123 Use http instead of https for OCSP and CRL URLs in IPA certificate profile
On Tue, 2013-04-09 at 16:02 +0200, Martin Kosek wrote: On 04/08/2013 05:09 PM, Martin Kosek wrote: On 04/08/2013 03:47 PM, Dmitri Pal wrote: On 04/08/2013 08:42 AM, Martin Kosek wrote: On 04/08/2013 10:48 AM, Jan Cholasta wrote: On 8.4.2013 10:47, Jan Cholasta wrote: Hi, this patch fixes https://fedorahosted.org/freeipa/ticket/3552. Honza Re-sending with correct subject. I tested the change both for upgrades and for fresh installs and it worked fine both cases, even when testing with Firefox enforcing mode. So far, as the biggest issue in current process I see NSS not being able to fallback to other defined OCSP responder (I tested with Firefox 20). This way, Firefox will fail validating the FreeIPA site when the first tested OCSP responder is not available (e.g. the original IPA CA signing the http cert, or an `ipa-ca.$domain` host that is currently not up). Have we filed a ticket with FF? AFAIU, this is rather NSS issue, that Firefox issue. There is a bug open for NSS: https://bugzilla.mozilla.org/show_bug.cgi?id=797815 Rob seems to have more context about this bug background. Martin We may want to wait with pushing this patch until we get some response in the NSS Bugzilla above. If our request is rejected, we may be forced to use just a single CRL/OCSP (which would be probably the general one) and thus supersede patch 123. Well it will have to depend on when you create certs. The first IPA server own cert should probably point at the ipa server name. Then we should warn in bold letters that the user should create such and such a DNS name if they did not let IPA handle DNS. If we can handle DNS then any other use can refer to the common name which can be an A name with multiple entries (each IPA CA server should be listed there by default and the record should be changed at ca replicas install/decommission time, however we should allow admins to add/remove names as well manually in case they want to add proxies otr conceal some of the CA servers. We may also want to change the RA client code to use that record to fetch certs. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 125 Do actually stop pki_cad in stop_pkicad instead of starting it
On 04/09/2013 03:52 PM, Jan Cholasta wrote: Hi, this patch fixes https://fedorahosted.org/freeipa/ticket/3554. Honza Yeah, this does much better job at stopping pki-ca... ACK. Pushed to master, ipa-3-1. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 267 Filter groups by type (normal, posix, external)
On 04/04/2013 12:02 PM, Martin Kosek wrote:
On 04/04/2013 11:48 AM, Tomas Babej wrote:
On 03/22/2013 03:03 PM, Martin Kosek wrote:
On 03/21/2013 06:10 PM, Petr Vobornik wrote:
On 03/21/2013 05:10 PM, Martin Kosek wrote:
On 03/16/2013 03:32 AM, Endi Sukma Dewata wrote:
On 3/12/2013 11:28 AM, Petr Vobornik wrote:
Here's a patch for filtering groups by type.
Design page: http://www.freeipa.org/page/V3/Filtering_groups_by_type
The interface is:
StrEnum('type?',
cli_name='type',
label=_('Type'),
doc=_('Group type'),
values=(u'posix', u'normal', u'external'),
),
I have two design questions.
1. Is --type the right option name?
Fine by me, it matches the label and description.
2. Is `normal` the right name for non-posix, non-external group? The
default group type (when adding group) is posix. Should the name be
something else: `simple`, `plain`, `ordinary`?
We also use 'normal' in the group adder dialog, so it's consistent. Other
options are 'basic', 'standard', 'regular'.
I didn't want to create an option for each type. IMO it brings more
complexity.
Maybe the group-add/mod command should use the same --type option?
https://fedorahosted.org/freeipa/ticket/3483
ACK from me, but maybe others might have some comments.
I am just thinking about if the new API is right. For example, when we add an
external group, we use ipa group-add --external. But when we search for
external groups, we suddenly use
# ipa group-find --type=external
and not
# ipa group-find --external
or
# ipa group-find --nonposix
Wouldn't that cause confusion? I am looking for same second opinion on this
one.
I also did not like normal group type very much, maybe we should just
call it
nonposix? As that's the option you use when you are creating such group:
# ipa group-add --nonposix foo
Otherwise, the patch looks good functionally.
Martin
I have to note that external group is also non-posix. Following command is
valid:
# ipa group-add foo --desc=a --external --nonposix
By that logic
# ipa group-find --nonposix
Would also list external groups.
I fine with renaming 'normal' to something better (will also require Web UI
change), but it is not 'nonposix'.
I think this logic is flawed as well. Then you could say that posix group is
also nonposix, because it contains the same objectclasses as nonpoxis group +
posixGroup objectclass.
nonposix is the term we already use (see --nonposix), not something
artificial or new, so I would not be afraid of it.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Let us try to move on with this, here are my 2 cents:
1.) normal is not a suitable name for non-posix, non-external group. As a user,
I would assume that
# ipa group-find --type=normal
would return the groups that I created using simple
# ipa group-add testgroup
command. By that logic, any other suggested synonym implying there's nothing
special about this
group is not suitable.
2.) If not normal (or any other synonym implying there's nothing special about
this group) then what?
We can either:
- use exact but complicated --non-posix-non-external
- use --nonposix and deal with the fact that sets defined by the type are not
disjunct
- make up our own new term and define it
While none of these options are fortunate, let's look for the least resistance:
- exact, but complicated names are ugly and do not keep interface simple
- nonposix groups are superset of external groups
- confuses the user and makes the learning curve steeper
From this I would go for option 2, indeed, if you think about --nonposix /
--external as flags, where
the external takes priority before nonposix, this kind of makes sense. If the
user does not think
about the implementation (that every external group is nonposix), he may indeed
find himself in this mindset.
3.) I'm fine both with --type=external and --external approaches. The latterr
is more consistent with the way we do things,
*-find commands search mainly on selected subset of attributes, so using the
flag analogy I mentioned an paragraph ago,
you would expect --external to behave as an attribute, especially if group-add
command accepts it in this form.
Having 3 options instead of one will clutter things a bit more, but if we keep
them in the same place (in the list of options)
it should not cause much confusion, more so if the descriptions would be nearly
the same, one would quickly see that these
belong together.
Tomas
Thanks Tomas for your opinion, I can agree with that. To make it more in an
actual design, this is API following this discussion that I would propose:
This is API we already have in IPA:
ipa group-add --external
ipa group-add --nonposix
ipa group-find --private
This is API that I would propose to add to be consistent with what we already
have:
ipa group-find --nonposix
ipa group-find --posix
ipa
Re: [Freeipa-devel] [PATCH] 123 Use http instead of https for OCSP and CRL URLs in IPA certificate profile
On Tue, 2013-04-09 at 11:18 -0400, Dmitri Pal wrote: On 04/09/2013 10:19 AM, Simo Sorce wrote: On Tue, 2013-04-09 at 16:02 +0200, Martin Kosek wrote: On 04/08/2013 05:09 PM, Martin Kosek wrote: On 04/08/2013 03:47 PM, Dmitri Pal wrote: On 04/08/2013 08:42 AM, Martin Kosek wrote: On 04/08/2013 10:48 AM, Jan Cholasta wrote: On 8.4.2013 10:47, Jan Cholasta wrote: Hi, this patch fixes https://fedorahosted.org/freeipa/ticket/3552. Honza Re-sending with correct subject. I tested the change both for upgrades and for fresh installs and it worked fine both cases, even when testing with Firefox enforcing mode. So far, as the biggest issue in current process I see NSS not being able to fallback to other defined OCSP responder (I tested with Firefox 20). This way, Firefox will fail validating the FreeIPA site when the first tested OCSP responder is not available (e.g. the original IPA CA signing the http cert, or an `ipa-ca.$domain` host that is currently not up). Have we filed a ticket with FF? AFAIU, this is rather NSS issue, that Firefox issue. There is a bug open for NSS: https://bugzilla.mozilla.org/show_bug.cgi?id=797815 Rob seems to have more context about this bug background. Martin We may want to wait with pushing this patch until we get some response in the NSS Bugzilla above. If our request is rejected, we may be forced to use just a single CRL/OCSP (which would be probably the general one) and thus supersede patch 123. Well it will have to depend on when you create certs. The first IPA server own cert should probably point at the ipa server name. Then we should warn in bold letters that the user should create such and such a DNS name if they did not let IPA handle DNS. If we can handle DNS then any other use can refer to the common name which can be an A name with multiple entries (each IPA CA server should be listed there by default and the record should be changed at ca replicas install/decommission time, however we should allow admins to add/remove names as well manually in case they want to add proxies otr conceal some of the CA servers. We may also want to change the RA client code to use that record to fetch certs. Simo. I see a lot of RFEs in this comment. Are we going to file them? We'll see how NSS is going to respond to the ticket, and then adjust accordingly. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 267 Filter groups by type (normal, posix, external)
On 04/09/2013 05:06 PM, Martin Kosek wrote:
On 04/09/2013 04:38 PM, Petr Vobornik wrote:
On 04/04/2013 12:02 PM, Martin Kosek wrote:
Thanks Tomas for your opinion, I can agree with that. To make it more in an
actual design, this is API following this discussion that I would propose:
This is API we already have in IPA:
ipa group-add --external
ipa group-add --nonposix
ipa group-find --private
This is API that I would propose to add to be consistent with what we already
have:
ipa group-find --nonposix
ipa group-find --posix
ipa group-find --external
--nonposix would only match groups added with --nonposix flag in group-add,
i.e. no --external groups.
As Tomas said, these should also be close together. We can even add a specific
option group for them, like there are with ipa dnsrecord-add, named for example
Group Types. We may also raise OptionError when these option are used
together to make this less confusing - e.g. OptionError(group type options
(--nonposix, --posix and --external) are mutually exclusive).
Martin
New version attached.
1) default=False parameter for Flag is redundant:
+Flag('external',
+ cli_name='external',
+ doc=_('search for groups with support of external non-IPA members
from trusted domains'),
+ default=False,
+),
2) No need to import StrEnum:
+from ipalib import Int, Str, StrEnum
3) This can be simplified:
+if len(filters):
TO:
+if filters:
Besides these minor issues, that patch works fine and I think we can push a
fixed version.
Thanks,
Martin
Additional self-nack.
4) original filter is ignored when some of the new options is used. It
prevents from effective searching and also shows private groups when
--posix is used.
All fixed, new unit test added. New version attached.
The fix doesn't touch usage of --private because it's a special case -
private groups don't have ipausergroup oc and therefore they are
incompatible with original filter.
--
Petr Vobornik
From b5ed783390b14df8482e246cb1a7771be942c102 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Mon, 11 Mar 2013 12:37:29 +0100
Subject: [PATCH] Filter groups by type (POSIX, non-POSIX, external)
Added flag for each groups type: --posix, --nonposix, --external to group-find command.
Group types:
* non-POSIX: not posix, not external
* POSIX: with objectclass posixgroup
* external: with objectclass ipaexternalgroup
https://fedorahosted.org/freeipa/ticket/3483
---
API.txt| 5 +-
ipalib/plugins/group.py| 28
tests/test_xmlrpc/objectclasses.py | 1 +
tests/test_xmlrpc/test_group_plugin.py | 116 -
tests/test_xmlrpc/xmlrpc_test.py | 4 ++
5 files changed, 151 insertions(+), 3 deletions(-)
diff --git a/API.txt b/API.txt
index 81a1f6187583029a4378e44d65bcc7b8d4496508..87db8d678510ebd58d8071ef4b1638085754aa0b 100644
--- a/API.txt
+++ b/API.txt
@@ -1307,11 +1307,12 @@ output: Output('result', type 'bool', None)
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: Output('value', type 'unicode', None)
command: group_find
-args: 1,24,4
+args: 1,27,4
arg: Str('criteria?', noextrawhitespace=False)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Str('cn', attribute=True, autofill=False, cli_name='group_name', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, query=True, required=False)
option: Str('description', attribute=True, autofill=False, cli_name='desc', multivalue=False, query=True, required=False)
+option: Flag('external', autofill=True, cli_name='external', default=False)
option: Int('gidnumber', attribute=True, autofill=False, cli_name='gid', minvalue=1, multivalue=False, query=True, required=False)
option: Str('group*', cli_name='groups', csv=True)
option: Str('in_group*', cli_name='in_groups', csv=True)
@@ -1321,12 +1322,14 @@ option: Str('in_role*', cli_name='in_roles', csv=True)
option: Str('in_sudorule*', cli_name='in_sudorules', csv=True)
option: Str('no_group*', cli_name='no_groups', csv=True)
option: Str('no_user*', cli_name='no_users', csv=True)
+option: Flag('nonposix', autofill=True, cli_name='nonposix', default=False)
option: Str('not_in_group*', cli_name='not_in_groups', csv=True)
option: Str('not_in_hbacrule*', cli_name='not_in_hbacrules', csv=True)
option: Str('not_in_netgroup*', cli_name='not_in_netgroups', csv=True)
option: Str('not_in_role*', cli_name='not_in_roles', csv=True)
option: Str('not_in_sudorule*', cli_name='not_in_sudorules', csv=True)
option: Flag('pkey_only?', autofill=True, default=False)
+option: Flag('posix', autofill=True, cli_name='posix', default=False)
option: Flag('private', autofill=True, cli_name='private', default=False)
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option:
Re: [Freeipa-devel] [PATCH] 267 Filter groups by type (normal, posix, external)
On 04/09/2013 06:45 PM, Petr Vobornik wrote:
On 04/09/2013 05:06 PM, Martin Kosek wrote:
On 04/09/2013 04:38 PM, Petr Vobornik wrote:
On 04/04/2013 12:02 PM, Martin Kosek wrote:
Thanks Tomas for your opinion, I can agree with that. To make it more in an
actual design, this is API following this discussion that I would propose:
This is API we already have in IPA:
ipa group-add --external
ipa group-add --nonposix
ipa group-find --private
This is API that I would propose to add to be consistent with what we already
have:
ipa group-find --nonposix
ipa group-find --posix
ipa group-find --external
--nonposix would only match groups added with --nonposix flag in group-add,
i.e. no --external groups.
As Tomas said, these should also be close together. We can even add a specific
option group for them, like there are with ipa dnsrecord-add, named for
example
Group Types. We may also raise OptionError when these option are used
together to make this less confusing - e.g. OptionError(group type options
(--nonposix, --posix and --external) are mutually exclusive).
Martin
New version attached.
1) default=False parameter for Flag is redundant:
+Flag('external',
+ cli_name='external',
+ doc=_('search for groups with support of external non-IPA members
from trusted domains'),
+ default=False,
+),
2) No need to import StrEnum:
+from ipalib import Int, Str, StrEnum
3) This can be simplified:
+if len(filters):
TO:
+if filters:
Besides these minor issues, that patch works fine and I think we can push a
fixed version.
Thanks,
Martin
Additional self-nack.
4) original filter is ignored when some of the new options is used. It prevents
from effective searching and also shows private groups when --posix is used.
All fixed, new unit test added. New version attached.
The fix doesn't touch usage of --private because it's a special case - private
groups don't have ipausergroup oc and therefore they are incompatible with
original filter.
ACK, thanks for the fix. Pushed to master.
Please also make sure that you update design page
(http://www.freeipa.org/page/V3/Filtering_groups_by_type) with respect to the
API change we have discussed above.
Thanks,
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] WIP backup and restore
Petr Viktorin wrote:
On 04/05/2013 10:54 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 04/04/2013 03:04 PM, Rob Crittenden wrote:
Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/23/2013 05:06 AM, Rob Crittenden wrote:
There are strict limits on what can be restored where. Only exact
matching hostnames and versions are allowed right now. We can
probably
relax the hostname requirement if we're only restoring data, and the
version perhaps for only the the first two values (so you can
restore a
3.0.0 backup on 3.0.1 but not on 3.1.0).
Do we also need to limit the versions of Dogtag, 389, Kerberos...?
Or is what they put in /var/lib guaranteed portable across versions?
An interesting question. I'd imagine that a major db change would also
require a major update to IPA, therefore if we restrict by IPA version
we should be ok. I AM doing an untar of files though so yeah, there
is a
risk here.
That's good to hear!
I think while developing, you should run with -v to get all the
output.
And for production, please have the commands log by default (set
log_file_name).
Yes, I plan on adding that in the future.
ipa-backup does all its binds using ldapi. ipa-restore needs the DM
password because we need to contact remote servers to enable/disable
replication.
The restore assumes that ipa is already installed, right? I can't
just
run it on a clean machine? Id would be good to check/document this.
It depends.
For a full backup you can actually restore to an uninstalled
server. In
fact, you could restore to a machine w/no IPA bits on it at all in all
likelihood (which wouldn't be very nice, but not exactly wrong either
IMHO).
I tested this with:
- ipa-server-install
- ipa-backup
- ipa-server-install --uninstall -U
- ipa-restore
- ran the unit tests
I looked in the backup tarball and saw dirsrv PID file and lock
directories. Are these needed?
Probably not. I gathered some of the files to backup based on watching
what files that changed during an install that are independent of us.
I'll take another pass through it, there may be other oddities too.
The tool runners (install/tools/ipa-backup and
install/tools/ipa-restore) are missing, so IPA doesn't build.
Probably
just a forgotten git add.
Yup.
The patch adds an extra backslash in install/tools/man/Makefile.am;
consider adding $(NULL) at the end.
I'll take a look.
Backup.dirs, Backup.files, Backup.logs:
The code modifies these class-level attributes. This means you can't
run
the command more than once in one Python session, so it can't be
used as
a library (e.g. in a hypothetical test suite).
Please make the class atrributes immutable (tuples), then in
__init__ do
`self.dirs = list(self.dirs)` to to get instance-specific lists.
Ok, fair point.
Code that creates temporary files/directories or does chdir()
should be
next to (or in) a try block whose finally: restores things.
Yes, I mean to add a big try/finally block around everything in run
eventually (or the equivalent anyway).
Instead of star imports (from ipapython.ipa_log_manager import *),
please explicitly import whatever you're using from that package. In
this case, nothing at all!
Yeah, I haven't run this through pylint yet to see all the bad
imports I
cobbled together.
If there's a get_connection() method, it should return the
connection,
and it should always be used to get it. Store the connection in
self._conn, and rather than:
self.get_connection()
self.conn.do_work()
do:
conn = self.get_connection()
conn.do_work()
This makes forgetting to call get_connection() impossible.
My purpose was to avoid creating multiple connections.
If a variable is only used in a single method, like `filename` in
Restore.extract_backup, don't store it in the admintool object.
In general, try to avoid putting data in self if possible. It's
convenient but it allows complex interdependencies.
(Yes, I'm guilty of not following this myself.)
Yes, there is certainly a bit of cleanup to do. I was in a bit of a
rush
to get this WIP out.
In several places, the backup tool logs critical errors and then just
continues on. Is that by design? I think a nonzero exit code would be
appropriate.
I'll take a look at them, all I can say at this point is maybe.
In the ipa-restore man page, --backend is not documented.
In db2ldif, db2bak, etc., a more conscise way to get the time
string is
`time.strftime('export_%Y_%m_%d_%H_%M_%S')`.
When validating --gpg-keyring, it would be good to check both files
(.sec and .pub).
Ok, I can do those.
Here:
if (self.backup_type != 'FULL' and not options.data_only and
not instances):
raise admintool.ScriptError('Cannot restore a data backup
into
an empty system')
did you mean `(self.backup_type != 'FULL' or options.data_only) and
not
instances`? (I'd introduce a `data_only` flag to prevent confusion.)
Yeah, looks like that should be an or.
In the ReplicationManager.check_repl_init
Re: [Freeipa-devel] [PATCH 0044] Update only selected attributes for winsync agreement
Tomas Babej wrote: Hi, Trying to insert nsDS5ReplicatedAttributeListTotal and nsds5ReplicaStripAttrs to winsync agreements caused upgrade errors. With this patch, these attributes are skipped for winsync agreements. Made find_ipa_replication_agreements() in replication.py more corresponding to find_replication_agreements. It returns list of entries instead of unicode strings now. https://fedorahosted.org/freeipa/ticket/3522 Tomas This will still do some work against a winsync agreement. Do we need to do that at all? I'm not sure we do. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1094 fix 2 broken tests
Ana Krivokapic wrote: On 04/05/2013 10:30 PM, Rob Crittenden wrote: Two tests are failing due to missing attributes since the krb ticket flags patch was pushed. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel The patch fixes failing tests, ACK. pushed to master rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 123 Use http instead of https for OCSP and CRL URLs in IPA certificate profile
On 04/09/2013 12:11 PM, Simo Sorce wrote: On Tue, 2013-04-09 at 11:18 -0400, Dmitri Pal wrote: On 04/09/2013 10:19 AM, Simo Sorce wrote: On Tue, 2013-04-09 at 16:02 +0200, Martin Kosek wrote: On 04/08/2013 05:09 PM, Martin Kosek wrote: On 04/08/2013 03:47 PM, Dmitri Pal wrote: On 04/08/2013 08:42 AM, Martin Kosek wrote: On 04/08/2013 10:48 AM, Jan Cholasta wrote: On 8.4.2013 10:47, Jan Cholasta wrote: Hi, this patch fixes https://fedorahosted.org/freeipa/ticket/3552. Honza Re-sending with correct subject. I tested the change both for upgrades and for fresh installs and it worked fine both cases, even when testing with Firefox enforcing mode. So far, as the biggest issue in current process I see NSS not being able to fallback to other defined OCSP responder (I tested with Firefox 20). This way, Firefox will fail validating the FreeIPA site when the first tested OCSP responder is not available (e.g. the original IPA CA signing the http cert, or an `ipa-ca.$domain` host that is currently not up). Have we filed a ticket with FF? AFAIU, this is rather NSS issue, that Firefox issue. There is a bug open for NSS: https://bugzilla.mozilla.org/show_bug.cgi?id=797815 Rob seems to have more context about this bug background. Martin We may want to wait with pushing this patch until we get some response in the NSS Bugzilla above. If our request is rejected, we may be forced to use just a single CRL/OCSP (which would be probably the general one) and thus supersede patch 123. Well it will have to depend on when you create certs. The first IPA server own cert should probably point at the ipa server name. Then we should warn in bold letters that the user should create such and such a DNS name if they did not let IPA handle DNS. If we can handle DNS then any other use can refer to the common name which can be an A name with multiple entries (each IPA CA server should be listed there by default and the record should be changed at ca replicas install/decommission time, however we should allow admins to add/remove names as well manually in case they want to add proxies otr conceal some of the CA servers. We may also want to change the RA client code to use that record to fetch certs. Simo. I see a lot of RFEs in this comment. Are we going to file them? We'll see how NSS is going to respond to the ticket, and then adjust accordingly. Simo. Well... time to adjust... accordingly ;-) -- Thank you, Dmitri Pal Sr. Engineering Manager for IdM portfolio Red Hat Inc. --- Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
