[Freeipa-devel] [PATCH] 0009 - Internationalize HBAC rule all category exceptions
This patch wraps exception messages in _() https://fedorahosted.org/freeipa/ticket/2267 -- Petr³ From ef61ff93af13e46400e9bff22586b6f9e0d0a63a Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Fri, 10 Feb 2012 05:27:24 -0500 Subject: [PATCH] Internationalize HBAC rule all category exceptions Wrap exception messages in _() https://fedorahosted.org/freeipa/ticket/2267 --- ipalib/plugins/hbacrule.py |8 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ipalib/plugins/hbacrule.py b/ipalib/plugins/hbacrule.py index c83305738b6b39ae109a3e738241ed7c0ce28a85..4666485569c365c9128f2dd04751a67cf71d47e1 100644 --- a/ipalib/plugins/hbacrule.py +++ b/ipalib/plugins/hbacrule.py @@ -265,13 +265,13 @@ class hbacrule_mod(LDAPUpdate): self.obj.handle_not_found(*keys) if is_all(options, 'usercategory') and 'memberuser' in entry_attrs: -raise errors.MutuallyExclusiveError(reason=user category cannot be set to 'all' while there are allowed users) +raise errors.MutuallyExclusiveError(reason=_(user category cannot be set to 'all' while there are allowed users)) if is_all(options, 'hostcategory') and 'memberhost' in entry_attrs: -raise errors.MutuallyExclusiveError(reason=host category cannot be set to 'all' while there are allowed hosts) +raise errors.MutuallyExclusiveError(reason=_(host category cannot be set to 'all' while there are allowed hosts)) if is_all(options, 'sourcehostcategory') and 'sourcehost' in entry_attrs: -raise errors.MutuallyExclusiveError(reason=sourcehost category cannot be set to 'all' while there are allowed source hosts) +raise errors.MutuallyExclusiveError(reason=_(sourcehost category cannot be set to 'all' while there are allowed sourcehosts)) if is_all(options, 'servicecategory') and 'memberservice' in entry_attrs: -raise errors.MutuallyExclusiveError(reason=service category cannot be set to 'all' while there are allowed services) +raise errors.MutuallyExclusiveError(reason=_(service category cannot be set to 'all' while there are allowed services)) return dn api.register(hbacrule_mod) -- 1.7.7.6 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 - Internationalize HBAC rule all category exceptions
On Tue, 14 Feb 2012, Petr Viktorin wrote: This patch wraps exception messages in _() https://fedorahosted.org/freeipa/ticket/2267 ACK. I was looking at hbactest and there are also some non-internationalized messages returned. Maybe you could combine them together with a slightly updated version of this patch? -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 195-199 New DNS features
On Tue, 2012-02-14 at 12:09 +0100, Martin Kosek wrote: A new version of bind-dyndb-ldap has been released, sending fixed patches with the following major changes: - Since bind-dyndb-ldap supports only idnsForwarders global option at this time, all other global options were removed from the API. They were left in the schema though so that the schema is consistent with bind-dyndb-ldap supported schema and the support of these options in the future can be added more seamlessly - idnsAllowQuery and idnsAllowTransfer format has changed to follow BIND format (ACI elements separated with semicolon). An example of such element: ipa dnszone-mod example.com --allow-query=10.0.0.1;!10.0.0.0/8;any; This ACI would forbid machine from any IP from 10.0.0.0/8 network besides 10.0.0.1 to query the name server. All other machines are allowed to issue queries. Any good reason why this is not a multi-value attribute ? Do these ACIs need to be ordered ? (that would be probably a good reason). Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] Fix build issues in master with krb5 1.10
The following 2 patches are need to have a functioning kdc.
Without them building against krb5 1.10 produces a ipadb.so module that
fails to load due to missing symbols leaving kadmin.local and krb5kdc
without a database.
The reason this happens is that during development of this code MIT had
some necessary API functions marked private and didn't expose headers.
These functions have been made public in 1.10 and renamed. Headers with
functions declaration and defines are also available now.
They are needed only in master as in 2.2 this code is commented out and
builds fine against 1.9
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 0f1b3ba1fbdc9c24ea661430aed527e995e5fad6 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Mon, 13 Feb 2012 16:57:57 -0500
Subject: [PATCH 1/5] Remove compat defines
These definitions were needed during development to be a le to build against
krb5 version 1.10
These function headers and defintions are now available in 1.10 that is a hard
dependency for freeipa 3.0, so we can safely drop them.
---
daemons/ipa-kdb/ipa_kdb_mspac.c | 32
1 files changed, 0 insertions(+), 32 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 654f0cb213bd3d2e5b531d51584179e523c95970..7f2e586667f21a33eea6ba7cdc84b7a69fe2c1c0 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -25,38 +25,6 @@
#include util/time.h
#include gen_ndr/ndr_krb5pac.h
-#define KRB5INT_PAC_SIGN_AVAILABLE 1
-#define KRB5INT_FIND_AUTHDATA_AVAILABLE 1
-
-#if KRB5INT_PAC_SIGN_AVAILABLE
-krb5_error_code
-krb5int_pac_sign(krb5_context context,
- krb5_pac pac,
- krb5_timestamp authtime,
- krb5_const_principal principal,
- const krb5_keyblock *server_key,
- const krb5_keyblock *privsvr_key,
- krb5_data *data);
-#define krb5_pac_sign krb5int_pac_sign
-#define KRB5_PAC_LOGON_INFO 1
-#endif
-
-#if KRB5INT_FIND_AUTHDATA_AVAILABLE
-krb5_error_code
-krb5int_find_authdata(krb5_context context,
- krb5_authdata *const *ticket_authdata,
- krb5_authdata *const *ap_req_authdata,
- krb5_authdatatype ad_type, krb5_authdata ***results);
-#define krb5_find_authdata krb5int_find_authdata
-#endif
-
-#ifndef KRB5_PAC_SERVER_CHECKSUM
-#define KRB5_PAC_SERVER_CHECKSUM 6
-#endif
-#ifndef KRB5_PAC_PRIVSVR_CHECKSUM
-#define KRB5_PAC_PRIVSVR_CHECKSUM 7
-#endif
-
static char *user_pac_attrs[] = {
objectClass,
uid,
--
1.7.7.6
From e3539e7114c778357d14528fd760e8e9bb3d4693 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Mon, 13 Feb 2012 17:00:46 -0500
Subject: [PATCH 2/5] Require krb5 1.10
---
freeipa.spec.in |8 ++--
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index a40368dea92b526336d87174555dae6629f97716..4f0cb03ae785d8ebe6715fac969ffdeb2ffa5265 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -37,7 +37,7 @@ BuildRequires: nspr-devel
BuildRequires: nss-devel
BuildRequires: openssl-devel
BuildRequires: openldap-devel
-BuildRequires: krb5-devel
+BuildRequires: krb5-devel = 1.10
BuildRequires: krb5-workstation
BuildRequires: libuuid-devel
%if 0%{?fedora} = 16
@@ -93,11 +93,7 @@ Requires(pre): 389-ds-base = 1.2.10-0.5.a5
Requires: openldap-clients
Requires: nss
Requires: nss-tools
-%if 0%{?fedora} = 16
-Requires: krb5-server = 1.9.1-15
-%else
-Requires: krb5-server
-%endif
+Requires: krb5-server = 1.10
Requires: krb5-pkinit-openssl
Requires: cyrus-sasl-gssapi%{?_isa}
Requires: ntp
--
1.7.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] Implement audit_as kdb layer function
Without this function the audit counters (krbLastFailedAuth,
krbLastSuccessfulAuth, krbLoginFailedCount) are not updated causing a
regression.
This function updates the counters unconditionally upon
successful/failed authentication (only if pre-auth is used which is the
default in FreeIPA).
A side effect of how this is implemented is that no other attributes are
updated when this happens so that replication is not kicked (because we
filter audit counters from replication to avoid replication storms), in
2.1.x updating these counters also ended up updating krbExtraData and
that caused replication to kick in.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From f78034aaf3249532663962f38be21e9ef0f3c8cd Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Mon, 13 Feb 2012 12:15:07 -0500
Subject: [PATCH 3/5] ipa-kdb: add AS auditing support
Fixes: https://fedorahosted.org/freeipa/ticket/2334
---
daemons/ipa-kdb/Makefile.am |1 +
daemons/ipa-kdb/ipa_kdb.c|2 +-
daemons/ipa-kdb/ipa_kdb.h| 18 +-
daemons/ipa-kdb/ipa_kdb_audit_as.c | 120 ++
daemons/ipa-kdb/ipa_kdb_passwords.c | 79 ++
daemons/ipa-kdb/ipa_kdb_principals.c | 36 ++-
daemons/ipa-kdb/ipa_kdb_pwdpolicy.c | 89 +-
util/ipa_pwd.h |3 +
8 files changed, 257 insertions(+), 91 deletions(-)
create mode 100644 daemons/ipa-kdb/ipa_kdb_audit_as.c
diff --git a/daemons/ipa-kdb/Makefile.am b/daemons/ipa-kdb/Makefile.am
index 77b92e8db865eb4bcc8ced50e0ef0352b6ea334f..17c090418ec5a0e2a39d948dc385d509c5d05321 100644
--- a/daemons/ipa-kdb/Makefile.am
+++ b/daemons/ipa-kdb/Makefile.am
@@ -35,6 +35,7 @@ ipadb_la_SOURCES = \
ipa_kdb_pwdpolicy.c \
ipa_kdb_mspac.c \
ipa_kdb_delegation.c \
+ ipa_kdb_audit_as.c \
$(KRB5_UTIL_SRCS) \
$(NULL)
diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c
index ca266d546b9529778eec1b27bc10e0150c952b1f..1dae4e6c1824c7936b0359d71dad809aed668d04 100644
--- a/daemons/ipa-kdb/ipa_kdb.c
+++ b/daemons/ipa-kdb/ipa_kdb.c
@@ -456,7 +456,7 @@ kdb_vftabl kdb_function_table = {
NULL, /* check_transited_realms */
NULL, /* check_policy_as */
NULL, /* check_policy_tgs */
-NULL, /* audit_as_req */
+ipadb_audit_as_req, /* audit_as_req */
NULL, /* refresh_config */
ipadb_check_allowed_to_delegate /* check_allowed_to_delegate */
};
diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h
index 2531d03281e899152b9418b545dd7f1122cf61bb..22e28223c2c9c178c0041512a9d8b5621f590441 100644
--- a/daemons/ipa-kdb/ipa_kdb.h
+++ b/daemons/ipa-kdb/ipa_kdb.h
@@ -103,7 +103,8 @@ struct ipadb_e_data {
time_t last_pwd_change;
char *pw_policy_dn;
char **pw_history;
-struct ipapwd_policy pol;
+struct ipapwd_policy *pol;
+time_t last_admin_unlock;
};
struct ipadb_context *ipadb_get_context(krb5_context kcontext);
@@ -165,6 +166,11 @@ krb5_error_code ipadb_iterate(krb5_context kcontext,
krb5_pointer func_arg);
/* POLICY FUNCTIONS */
+
+krb5_error_code ipadb_get_ipapwd_policy(struct ipadb_context *ipactx,
+char *pw_policy_dn,
+struct ipapwd_policy **pol);
+
krb5_error_code ipadb_create_pwd_policy(krb5_context kcontext,
osa_policy_ent_t policy);
krb5_error_code ipadb_get_pwd_policy(krb5_context kcontext, char *name,
@@ -230,3 +236,13 @@ krb5_error_code ipadb_check_allowed_to_delegate(krb5_context kcontext,
krb5_const_principal client,
const krb5_db_entry *server,
krb5_const_principal proxy);
+
+/* AS AUDIT */
+
+void ipadb_audit_as_req(krb5_context kcontext,
+krb5_kdc_req *request,
+krb5_db_entry *client,
+krb5_db_entry *server,
+krb5_timestamp authtime,
+krb5_error_code error_code);
+
diff --git a/daemons/ipa-kdb/ipa_kdb_audit_as.c b/daemons/ipa-kdb/ipa_kdb_audit_as.c
new file mode 100644
index ..c71568c385595b3f6b9b595f4807ece8356349e0
--- /dev/null
+++ b/daemons/ipa-kdb/ipa_kdb_audit_as.c
@@ -0,0 +1,120 @@
+/*
+ * MIT Kerberos KDC database backend for FreeIPA
+ *
+ * Authors: Simo Sorce sso...@redhat.com
+ *
+ * Copyright (C) 2012 Simo Sorce, Red Hat
+ * see file 'COPYING' for use and warranty information
+ *
+ * This program is free software you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free
[Freeipa-devel] [PATCH] 479 optimize modify principal operations
We were unconditionally searching the LDAP database to find the
principal on modification.
By using the stored entry_dn when available we avoid a costly round-trip
to the LDAP server just to save back some modified attributes.
This is an important performance improvement for the KDC given now we
save data each time an AS request is performed.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 87a093331f4e7661d110e3d0a548055756043da8 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Mon, 13 Feb 2012 22:21:17 -0500
Subject: [PATCH 4/5] ipa-kdb: Avoid lookup on modify if possible
This avoids one useless search if we already have the entry_dn.
---
daemons/ipa-kdb/ipa_kdb_principals.c | 46 --
1 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index 3540f0eea2ac5f9c36a7312f3676939f8611f6a9..9a3c86fb0c249a3c1c0f66fb4ddf67a1890df73f 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1760,33 +1760,37 @@ static krb5_error_code ipadb_modify_principal(krb5_context kcontext,
LDAPMessage *lentry;
struct ipadb_mods *imods = NULL;
char *dn = NULL;
+struct ipadb_e_data *ied;
ipactx = ipadb_get_context(kcontext);
if (!ipactx) {
return KRB5_KDB_DBNOTINITED;
}
-kerr = krb5_unparse_name(kcontext, entry-princ, principal);
-if (kerr != 0) {
-goto done;
-}
+ied = (struct ipadb_e_data *)entry-e_data;
+if (!ied || !ied-entry_dn) {
+kerr = krb5_unparse_name(kcontext, entry-princ, principal);
+if (kerr != 0) {
+goto done;
+}
-kerr = ipadb_fetch_principals(ipactx, principal, res);
-if (kerr != 0) {
-goto done;
-}
+kerr = ipadb_fetch_principals(ipactx, principal, res);
+if (kerr != 0) {
+goto done;
+}
-/* FIXME: no alias allowed for now, should we allow modifies
- * by alias name ? */
-kerr = ipadb_find_principal(kcontext, 0, res, principal, lentry);
-if (kerr != 0) {
-goto done;
-}
+/* FIXME: no alias allowed for now, should we allow modifies
+ * by alias name ? */
+kerr = ipadb_find_principal(kcontext, 0, res, principal, lentry);
+if (kerr != 0) {
+goto done;
+}
-dn = ldap_get_dn(ipactx-lcontext, lentry);
-if (!dn) {
-kerr = KRB5_KDB_INTERNAL_ERROR;
-goto done;
+dn = ldap_get_dn(ipactx-lcontext, lentry);
+if (!dn) {
+kerr = KRB5_KDB_INTERNAL_ERROR;
+goto done;
+}
}
kerr = new_ipadb_mods(imods);
@@ -1800,7 +1804,11 @@ static krb5_error_code ipadb_modify_principal(krb5_context kcontext,
goto done;
}
-kerr = ipadb_simple_modify(ipactx, dn, imods-mods);
+if (!ied || !ied-entry_dn) {
+kerr = ipadb_simple_modify(ipactx, dn, imods-mods);
+} else {
+kerr = ipadb_simple_modify(ipactx, ied-entry_dn, imods-mods);
+}
done:
ipadb_mods_free(imods);
--
1.7.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 077 Redirection to PTR records from A, AAAA records
On 02/14/2012 08:11 AM, Endi Sukma Dewata wrote: On 2/9/2012 7:57 AM, Petr Vobornik wrote: Address column in A, DNS records was extended by redirection capabilities. Redirection dialog is shown after a click on a value. Dialog does following steps: 1) fetch all dns zones 2) find most accurate reverse zone for IP address 2 -fail) show error message, stop 3) checks if target record exists in the zone 3 -fail) show 'dns record create link', stop 4) redirects Click on 'dns record create link': 1) creates record 1 -fail) show error, stop 2) redirects https://fedorahosted.org/freeipa/ticket/1975 One minor issue: 1. After redirection the breadcrumb doesn't link to the correct zone. Try this: a) Open the details page of an A record. b) Click the IP address (create the PTR record if necessary), it will show the PTR record. c) Click the reverse zone in the breadcrumb, it will go to the forward zone. Hmm. It's because it was the last zone opened and the redirection (clicking on breadcrumb causes redirection to facet's redirection facet - in this case the zone) takes in consideration only a facet not an entry (current pkeys state). From this point of view it behaves correctly but I understand your point. Are you saying, that we should change breadcrumb navigation code to take in consideration the current state (pkeys). If so I would rather do it separately. Possible improvements: 2. We can also create a link from the PTR record to go back to the A record. Yep. Do you think it should be part of this patch? 3. When all of the redirection steps work correctly, the redirection dialog will appear and then disappear immediately. There's no time to read the message. I think the redirection steps should be done in the background after clicking the link and the dialog should only appear when there's an error. So it might be better to move the redirection code into the redirection column. My idea behind the dialog is that if you are on slow vpn you can see some progress and notice that something is happening. Also it prevents doing other, not desired actions. 4. The dialog is an error dialog, so the dialog title could say something like Unable to open PTR record. From users' perspective they are following a link, it's not a redirection. I don't consider it an error dialog. The title wording can be changed to something more appropriate but not to error message. 5. The messages shown in the dialog can be simplified. Users don't need to know the detailed steps such as fetching the DNS zones, etc. If there's no matching reverse zone it could just say something like: Reverse DNS zone for IP address not found. If there's a reverse zone but no PTR record, it could say something like this: PTR record for IP address not found. [Create PTR Record] Yeah, maybe it is too chatty. It may be better to display general checking for PTR record with activity indicator next to it. 6. To be consistent, all user action that changes the data should be presented as a button. So the Create PTR Record above should be converted into a button. Good point. Will do. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 480 Do not store LastPwdChange unless it really changed
Due to an idiosyncrasy of kadmin, the right flag to indicate
krbLastPwdChange is changed is not set. The previous check ended up
always saving the data in all cases because the data was always present.
Restrict it to store a password change when there is actually new key
material.
This prevents also audit operations to cause replications.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 6ce7908595cfbc5a38d929c84d7cbe783ac92f15 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Mon, 13 Feb 2012 22:43:15 -0500
Subject: [PATCH 5/5] ipa-kdb: set krblastpwdchange only when keys have been
effectively changed
---
daemons/ipa-kdb/ipa_kdb_principals.c |6 --
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index 9a3c86fb0c249a3c1c0f66fb4ddf67a1890df73f..a0d4687175d8283020ce9da83b355dd1c94051b0 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1422,7 +1422,8 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
/* KADM5_LAST_PWD_CHANGE */
/* apparently, at least some versions of kadmin fail to set this flag
* when they do include a pwd change timestamp in TL_DATA.
- * So for now always check for it regardless. */
+ * So for now check if KADM5_KEY_DATA has been set, which kadm5
+ * always does on password changes */
#if KADM5_ACTUALLY_SETS_LAST_PWD_CHANGE
if (entry-mask KMASK_LAST_PWD_CHANGE) {
if (!entry-n_tl_data) {
@@ -1431,7 +1432,8 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
}
#else
-if (entry-n_tl_data) {
+if (entry-n_tl_data
+entry-mask KMASK_KEY_DATA) {
#endif
kerr = ipadb_get_tl_data(entry,
KRB5_TL_LAST_PWD_CHANGE,
--
1.7.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 195-199 New DNS features
Simo Sorce wrote: On Tue, 2012-02-14 at 12:09 +0100, Martin Kosek wrote: A new version of bind-dyndb-ldap has been released, sending fixed patches with the following major changes: - Since bind-dyndb-ldap supports only idnsForwarders global option at this time, all other global options were removed from the API. They were left in the schema though so that the schema is consistent with bind-dyndb-ldap supported schema and the support of these options in the future can be added more seamlessly - idnsAllowQuery and idnsAllowTransfer format has changed to follow BIND format (ACI elements separated with semicolon). An example of such element: ipa dnszone-mod example.com --allow-query=10.0.0.1;!10.0.0.0/8;any; This ACI would forbid machine from any IP from 10.0.0.0/8 network besides 10.0.0.1 to query the name server. All other machines are allowed to issue queries. Any good reason why this is not a multi-value attribute ? Do these ACIs need to be ordered ? (that would be probably a good reason). That's exactly it! rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 195-199 New DNS features
On Tue, 2012-02-14 at 09:10 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-02-14 at 12:09 +0100, Martin Kosek wrote: A new version of bind-dyndb-ldap has been released, sending fixed patches with the following major changes: - Since bind-dyndb-ldap supports only idnsForwarders global option at this time, all other global options were removed from the API. They were left in the schema though so that the schema is consistent with bind-dyndb-ldap supported schema and the support of these options in the future can be added more seamlessly - idnsAllowQuery and idnsAllowTransfer format has changed to follow BIND format (ACI elements separated with semicolon). An example of such element: ipa dnszone-mod example.com --allow-query=10.0.0.1;!10.0.0.0/8;any; This ACI would forbid machine from any IP from 10.0.0.0/8 network besides 10.0.0.1 to query the name server. All other machines are allowed to issue queries. Any good reason why this is not a multi-value attribute ? Do these ACIs need to be ordered ? (that would be probably a good reason). That's exactly it! rob Yup. Previous release of bind-dyndb-ldap followed the multi-valued LDAP attribute format, but we found out that we cannot do it this way as the ACI list need to be ordered. When bind evaluates if it should allow/reject query/tranfer request it simply traverses the ACI list, one by one, and accepts the result of the first match, i.e. the order is crucial there. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 - Internationalize HBAC rule all category exceptions
On 02/14/2012 10:49 AM, Alexander Bokovoy wrote:
On Tue, 14 Feb 2012, Petr Viktorin wrote:
This patch wraps exception messages in _()
https://fedorahosted.org/freeipa/ticket/2267
ACK.
I was looking at hbactest and there are also some
non-internationalized messages returned. Maybe you could combine them
together with a slightly updated version of this patch?
Yes; attaching updated patch. I also wrapped some texts in ipalib.output
in _() calls.
--
Petr³
From 57c65171d46ac6753029a91b139c0a3edfc77302 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 10 Feb 2012 05:27:24 -0500
Subject: [PATCH] Internationalization for HBAC and ipalib.output
* hbacrule: Internationalize HBAC rule all category exceptions
https://fedorahosted.org/freeipa/ticket/2267
* hbactest: Use internationalized names (doc) instead of names
for output items
Also don't convert result to bool, `not` does it implicitly
* ipalib.output: Internationalize descriptions of some standard entries
---
ipalib/output.py | 12 ++--
ipalib/plugins/hbacrule.py |8
ipalib/plugins/hbactest.py |6 +++---
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/ipalib/output.py b/ipalib/output.py
index 61713627437596eea1e203c08b08f10d2d636a47..1202ee19923bd6710a4c4e82a108d79ec63251b8 100644
--- a/ipalib/output.py
+++ b/ipalib/output.py
@@ -111,11 +111,11 @@ class ListOfEntries(Output):
result = Output('result', doc=_('All commands should at least have a result'))
summary = Output('summary', (unicode, NoneType),
-'User-friendly description of action performed'
+_('User-friendly description of action performed')
)
value = Output('value', unicode,
-The primary_key value of the entry, e.g. 'jdoe' for a user,
+_(The primary_key value of the entry, e.g. 'jdoe' for a user),
flags=['no_display'],
)
@@ -130,19 +130,19 @@ standard_entry = (
standard_list_of_entries = (
summary,
ListOfEntries('result'),
-Output('count', int, 'Number of entries returned'),
-Output('truncated', bool, 'True if not all results were returned'),
+Output('count', int, _('Number of entries returned')),
+Output('truncated', bool, _('True if not all results were returned')),
)
standard_delete = (
summary,
-Output('result', dict, 'list of deletions that failed'),
+Output('result', dict, _('List of deletions that failed')),
value,
)
standard_boolean = (
summary,
-Output('result', bool, 'True means the operation was successful'),
+Output('result', bool, _('True means the operation was successful')),
value,
)
diff --git a/ipalib/plugins/hbacrule.py b/ipalib/plugins/hbacrule.py
index c83305738b6b39ae109a3e738241ed7c0ce28a85..4666485569c365c9128f2dd04751a67cf71d47e1 100644
--- a/ipalib/plugins/hbacrule.py
+++ b/ipalib/plugins/hbacrule.py
@@ -265,13 +265,13 @@ class hbacrule_mod(LDAPUpdate):
self.obj.handle_not_found(*keys)
if is_all(options, 'usercategory') and 'memberuser' in entry_attrs:
-raise errors.MutuallyExclusiveError(reason=user category cannot be set to 'all' while there are allowed users)
+raise errors.MutuallyExclusiveError(reason=_(user category cannot be set to 'all' while there are allowed users))
if is_all(options, 'hostcategory') and 'memberhost' in entry_attrs:
-raise errors.MutuallyExclusiveError(reason=host category cannot be set to 'all' while there are allowed hosts)
+raise errors.MutuallyExclusiveError(reason=_(host category cannot be set to 'all' while there are allowed hosts))
if is_all(options, 'sourcehostcategory') and 'sourcehost' in entry_attrs:
-raise errors.MutuallyExclusiveError(reason=sourcehost category cannot be set to 'all' while there are allowed source hosts)
+raise errors.MutuallyExclusiveError(reason=_(sourcehost category cannot be set to 'all' while there are allowed sourcehosts))
if is_all(options, 'servicecategory') and 'memberservice' in entry_attrs:
-raise errors.MutuallyExclusiveError(reason=service category cannot be set to 'all' while there are allowed services)
+raise errors.MutuallyExclusiveError(reason=_(service category cannot be set to 'all' while there are allowed services))
return dn
api.register(hbacrule_mod)
diff --git a/ipalib/plugins/hbactest.py b/ipalib/plugins/hbactest.py
index 92b7145a3fca717b4699749c2ec2b88ae3647cd5..b81dca3deb5a45bb092e3ae2adf3119ef256ff9e 100644
--- a/ipalib/plugins/hbactest.py
+++ b/ipalib/plugins/hbactest.py
@@ -359,7 +359,7 @@ class hbactest(Command):
if res == pyhbac.HBAC_EVAL_DENY:
notmatched_rules.append(ipa_rule.name)
if warning_flag:
-warning_rules.append(u'Sourcehost value of rule %s is ignored' % (ipa_rule.name))
+warning_rules.append(_(u'Sourcehost
Re: [Freeipa-devel] [PATCH] 078 Fixed entity link disabling
On 2/9/2012 7:59 AM, Petr Vobornik wrote: Problem: Entity link (eg: to hosts in dns record or to dns record in host) is not changing its state when linked record doesn't exist. The link can remain wrongly enabled from previous state. Fixed: The link is disabled when target doesn't exist. https://fedorahosted.org/freeipa/ticket/2364 ACK. Pushed to master and ipa-2-2. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 079 Removed question marks from field labels
On 2/10/2012 11:35 AM, Petr Vobornik wrote: In user group adder dialog, the Is this a POSIX group? was replaced with POSIX group. In host search facet, the Enrolled? was replaced with Enrolled. https://fedorahosted.org/freeipa/ticket/2353 ACK. Pushed to master and ipa-2-2. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Implement audit_as kdb layer function
Simo Sorce wrote: Without this function the audit counters (krbLastFailedAuth, krbLastSuccessfulAuth, krbLoginFailedCount) are not updated causing a regression. This function updates the counters unconditionally upon successful/failed authentication (only if pre-auth is used which is the default in FreeIPA). A side effect of how this is implemented is that no other attributes are updated when this happens so that replication is not kicked (because we filter audit counters from replication to avoid replication storms), in 2.1.x updating these counters also ended up updating krbExtraData and that caused replication to kick in. Simo. This still isn't working quite right. The user lockout is not working. The failed counter plateaus at the lockout value (in my case 6). Any failures beyond 6 do not increment the counter, I'm assuming there is some other interaction going on. It does set the dates properly. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 077 Redirection to PTR records from A, AAAA records
On 2/14/2012 8:01 AM, Petr Vobornik wrote: 1. After redirection the breadcrumb doesn't link to the correct zone. Try this: a) Open the details page of an A record. b) Click the IP address (create the PTR record if necessary), it will show the PTR record. c) Click the reverse zone in the breadcrumb, it will go to the forward zone. Hmm. It's because it was the last zone opened and the redirection (clicking on breadcrumb causes redirection to facet's redirection facet - in this case the zone) takes in consideration only a facet not an entry (current pkeys state). From this point of view it behaves correctly but I understand your point. Are you saying, that we should change breadcrumb navigation code to take in consideration the current state (pkeys). If so I would rather do it separately. I think so. If the text in the breadcrumb says reverse zone it should go to the reverse zone. This is an existing issue so let's do this separately. So the patch is ACKed, feel free to push. The other improvements can be done separately if you want. 2. We can also create a link from the PTR record to go back to the A record. Yep. Do you think it should be part of this patch? It's not part of the original ticket. It can be done separately. 3. When all of the redirection steps work correctly, the redirection dialog will appear and then disappear immediately. There's no time to read the message. I think the redirection steps should be done in the background after clicking the link and the dialog should only appear when there's an error. So it might be better to move the redirection code into the redirection column. My idea behind the dialog is that if you are on slow vpn you can see some progress and notice that something is happening. Also it prevents doing other, not desired actions. We already have the activity indicator. A real progress dialog would need a progress bar. Right now it looks like a debug/console window. I don't think users expect to see that when they click what looks like a regular link. A link would either work (no dialog) or fail (error dialog). A progress dialog would be more appropriate for relatively long running operations started using a button. I also don't think it's necessary to prevent other operations because it's not changing the data. We also have the same situation with other links in the UI. It allows users to change their mind and click something else. 4. The dialog is an error dialog, so the dialog title could say something like Unable to open PTR record. From users' perspective they are following a link, it's not a redirection. I don't consider it an error dialog. The title wording can be changed to something more appropriate but not to error message. OK, it depends on what you decide on #3. 5. The messages shown in the dialog can be simplified. Users don't need to know the detailed steps such as fetching the DNS zones, etc. If there's no matching reverse zone it could just say something like: Reverse DNS zone for IP address not found. If there's a reverse zone but no PTR record, it could say something like this: PTR record for IP address not found. [Create PTR Record] Yeah, maybe it is too chatty. It may be better to display general checking for PTR record with activity indicator next to it. That would be better, but it also depends on #3. 6. To be consistent, all user action that changes the data should be presented as a button. So the Create PTR Record above should be converted into a button. Good point. Will do. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 - Internationalize HBAC rule all category exceptions
On Tue, 14 Feb 2012, Petr Viktorin wrote: On 02/14/2012 10:49 AM, Alexander Bokovoy wrote: On Tue, 14 Feb 2012, Petr Viktorin wrote: This patch wraps exception messages in _() https://fedorahosted.org/freeipa/ticket/2267 ACK. I was looking at hbactest and there are also some non-internationalized messages returned. Maybe you could combine them together with a slightly updated version of this patch? Yes; attaching updated patch. I also wrapped some texts in ipalib.output in _() calls. Thanks. ACK. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 940 apply some validation to some classes only
On 7.2.2012 20:25, Rob Crittenden wrote: Rob Crittenden wrote: Jan Cholasta wrote: Dne 7.2.2012 09:27, Martin Kosek napsal(a): On Mon, 2012-02-06 at 11:52 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Fri, 2012-02-03 at 16:58 -0500, Rob Crittenden wrote: There is some validation that we only need to apply when an entry is being created, namely the key itself. This is to allow us to manage an otherwise illegal entry that finds its way into the system (i.e. migration). Consider this. We migrate a group with a space in it. This isn't allowed in IPA but we also provide no way to delete it because the cn regex kicks out the group-del command. The trick is adding appropriate context so we can know during validation how we got here. A command object has a bases field which contains the base classes associated with it, which appears to contain only the leaf baseclass. So using this we can tell how we got to validation and can skip based on that baseclass name. I went back and forth a bit on where the right place to put this was, I'm open to more fine tuning. I initially skipped just the pattern validation then expanded it to apply to all validation in the Parameter. rob 1) This patch breaks API.txt, it needs re-generating. 2) This may be a matter of opinion but I think that + if self.onlyvalidateclasses and \ + not any(x in self.onlyvalidateclasses for x in classbases): + return would be more readable than: + if self.onlyvalidateclasses and \ + not [x for x in classbases if x in self.onlyvalidateclasses]: + return 3) I would move the entire self.onlyvalidateclasses up to the validate() method. It would have several benefits: - the validation skip would be done just once, not for every value as the decision does not use the value at all - we would not have to modify _validate_scalar() methods at all since they won't need classbases 4) I think it would be better to keep validation for --rename options. As it is generated from RDN attribute parameter, it inherits onlyvalidateclasses list as well. Otherwise your patch would let user to rename a valid object to an invalid one: # ipa user-mod fbar --rename=bad boy Modified user fbar User login: bad boy First name: Foo Last name: Bar Home directory: /home/fbar Login shell: /bin/sh UID: 480800040 GID: 480800040 Account disabled: False Password: False Member of groups: ipausers Kerberos keys available: False Martin This should address your concerns. rob Its almost OK, there is just one part that's not that OK: @@ -831,6 +836,9 @@ class Param(ReadOnly): else: raise RequirementError(name=self.name) return + if 'rename' not in (self.name, self.cli_name): + if self.onlyvalidateclasses and not [x for x in self.onlyvalidateclasses if x in classbases]: #pylint: disable=E1101 + return if self.multivalue: if type(value) is not tuple: raise TypeError( I don't think that hard-coding this skip for onlyvalidateclasses based just on parameter name is a good thing to do and may cause problems in the future. For example if we create some option called rename and deliberately set onlyvalidateclasses for the option - it would then be skipped as well. I think it would be a better solution to just update _get_rename_option() in baseldap.py to set onlyvalidateclasses to () Martin I must say I'm not a big fan of this approach. You are healing the symptoms (custom validation on some parameters makes it impossible to manipulate existing LDAP entries with invalid attribute values = add a way to mark parameters to be validated only in certain commands to prevent that), but the real issue here is that we should not perform custom validation when referring to existing LDAP attribute values at all (or only partially), no matter what parameter and command. Fixing this would make the problem go away for all commands, present or future, without the need for adding a list of command classes to each parameter that is affected. Honza It is all about context, and parameters have very little of it. The bottom line is we only want to do validation on new values. I think I can bump this up a level by adding a validate boolean to Param and Command both defaulting to False. crud.Create will override this to True and cloning rename could perhaps also set this flag. Then in validation if the parent object has validation set or the parameter does we validate, otherwise we skip it. This will require some other changes for Enums, they may always require validation (I'll need to be sure --delattr can remove a bad one). rob Updated patch. The primary key will be validated only on adds. The values will be validated on adds and mods. It turns out there already is infrastructure which does exactly the same thing: the query kwarg of the Parameter class. We should fix its behavior rather than reinvent the wheel. rob Also, the pattern and pattern_errmsg kwargs should not be copied from the primary key parameter, but from
Re: [Freeipa-devel] [PATCH] 940 apply some validation to some classes only
On 14.2.2012 16:44, Jan Cholasta wrote: On 7.2.2012 20:25, Rob Crittenden wrote: Rob Crittenden wrote: Jan Cholasta wrote: Dne 7.2.2012 09:27, Martin Kosek napsal(a): On Mon, 2012-02-06 at 11:52 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Fri, 2012-02-03 at 16:58 -0500, Rob Crittenden wrote: There is some validation that we only need to apply when an entry is being created, namely the key itself. This is to allow us to manage an otherwise illegal entry that finds its way into the system (i.e. migration). Consider this. We migrate a group with a space in it. This isn't allowed in IPA but we also provide no way to delete it because the cn regex kicks out the group-del command. The trick is adding appropriate context so we can know during validation how we got here. A command object has a bases field which contains the base classes associated with it, which appears to contain only the leaf baseclass. So using this we can tell how we got to validation and can skip based on that baseclass name. I went back and forth a bit on where the right place to put this was, I'm open to more fine tuning. I initially skipped just the pattern validation then expanded it to apply to all validation in the Parameter. rob 1) This patch breaks API.txt, it needs re-generating. 2) This may be a matter of opinion but I think that + if self.onlyvalidateclasses and \ + not any(x in self.onlyvalidateclasses for x in classbases): + return would be more readable than: + if self.onlyvalidateclasses and \ + not [x for x in classbases if x in self.onlyvalidateclasses]: + return 3) I would move the entire self.onlyvalidateclasses up to the validate() method. It would have several benefits: - the validation skip would be done just once, not for every value as the decision does not use the value at all - we would not have to modify _validate_scalar() methods at all since they won't need classbases 4) I think it would be better to keep validation for --rename options. As it is generated from RDN attribute parameter, it inherits onlyvalidateclasses list as well. Otherwise your patch would let user to rename a valid object to an invalid one: # ipa user-mod fbar --rename=bad boy Modified user fbar User login: bad boy First name: Foo Last name: Bar Home directory: /home/fbar Login shell: /bin/sh UID: 480800040 GID: 480800040 Account disabled: False Password: False Member of groups: ipausers Kerberos keys available: False Martin This should address your concerns. rob Its almost OK, there is just one part that's not that OK: @@ -831,6 +836,9 @@ class Param(ReadOnly): else: raise RequirementError(name=self.name) return + if 'rename' not in (self.name, self.cli_name): + if self.onlyvalidateclasses and not [x for x in self.onlyvalidateclasses if x in classbases]: #pylint: disable=E1101 + return if self.multivalue: if type(value) is not tuple: raise TypeError( I don't think that hard-coding this skip for onlyvalidateclasses based just on parameter name is a good thing to do and may cause problems in the future. For example if we create some option called rename and deliberately set onlyvalidateclasses for the option - it would then be skipped as well. I think it would be a better solution to just update _get_rename_option() in baseldap.py to set onlyvalidateclasses to () Martin I must say I'm not a big fan of this approach. You are healing the symptoms (custom validation on some parameters makes it impossible to manipulate existing LDAP entries with invalid attribute values = add a way to mark parameters to be validated only in certain commands to prevent that), but the real issue here is that we should not perform custom validation when referring to existing LDAP attribute values at all (or only partially), no matter what parameter and command. Fixing this would make the problem go away for all commands, present or future, without the need for adding a list of command classes to each parameter that is affected. Honza It is all about context, and parameters have very little of it. The bottom line is we only want to do validation on new values. I think I can bump this up a level by adding a validate boolean to Param and Command both defaulting to False. crud.Create will override this to True and cloning rename could perhaps also set this flag. Then in validation if the parent object has validation set or the parameter does we validate, otherwise we skip it. This will require some other changes for Enums, they may always require validation (I'll need to be sure --delattr can remove a bad one). rob Updated patch. The primary key will be validated only on adds. The values will be validated on adds and mods. It turns out there already is infrastructure which does exactly the same thing: the query kwarg of the Parameter class. We should fix its behavior rather than reinvent the wheel. This might be good enough: diff --git a/ipalib/crud.py b/ipalib/crud.py
Re: [Freeipa-devel] [PATCH] 202 Add reverse DNS record when forward is created
On 10.2.2012 16:42, Martin Kosek wrote: On Tue, 2012-02-07 at 16:26 +0100, Martin Kosek wrote: On Mon, 2012-02-06 at 15:56 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Mon, 2012-01-30 at 11:52 -0500, Rob Crittenden wrote: Martin Kosek wrote: Adding reverse DNS record may be a time consuming task, especially for IPv6 addresses. Having a way to automatically create a reverse record when a forward record is created could speed up the process. host-add command already has this possibility. This patch takes advantage of the new per-type API and adds new options for A/ record types: --a-create-reverse and ---create-reverse. These commands can be used to automatically create reverse records for new A/ addresses (both forward and reverse zones need to be managed by FreeIPA server): ipa dnsrecord-add example.com foo --a-rec=10.0.0.1 --a-create-reverse This command would add a new A record to record foo in zone example.com and a PTR record to appropriate reverse zone for IP address 10.0.0.1 (for example PTR record 1 in zone 0.0.10.in-addr.arpa. pointing to foo.example.com.). Few modification were done to new DNS API to support this feature: - Refactor --ip-address option handling from host-add and place it to dns.py to be used by both modules - Add support for extra per-type options - Hide DNS record part options in dnsrecord_find command as they have no effect for this command https://fedorahosted.org/freeipa/ticket/2009 Can the options -a-create-reverse and --create-reverse be combined? I was able to create an IPv4 addr using --create-reverse: # ipa dnsrecord-add example.com baz --a-rec=192.168.166.115 ---create-reverse Record name: baz A record: 192.168.166.115 Otherwise the patch seems fine. These 2 options can be combined, you can add both A and forward records and create records in their reverse records at the same time: ipa dnsrecord-add example.com bar --a-rec=10.0.0.1 --a-create-reverse ---rec=2001::beef:1 ---create-reverse In your case the option ---create-reverse is ignored as there is no rec added. Thus no record callback which would create this reverse record is called. We may implement some checks which would throw a validation error when --a-create-reverse/---create-reverse is called without a respective A/ record. Martin Yes, I think that is the way to go, otherwise this is confusing. rob Now, an exception is thrown if you try to pass --rrtype-create-reverse without an appropriate --rrtype-rec option filled: # ipa dnsrecord-add example.com baz --a-rec=192.168.166.115 ---create-reverse ipa: ERROR: 'record' is required I also refactored pre_callback of dnsrecord-add command a little, I didn't like parsingrrtype from parameter name using regexes. Now, every DNS part option has a link to parent DNS record stored in hint attribute. Martin Petr Vobornik noticed that reserved IP address passed to --a-rec (---rec) causes an Internal Error when --a-create-reverse is set at the same time: # ipa dnsrecord-add example.com foo ---ip-address=F:F:F:A::12 ---create-reverse ipa: ERROR: an internal error has occurred Attached patch fixes it: # ipa dnsrecord-add example.com foo ---ip-address=F:F:F:A::12 ---create-reverse ipa: ERROR: invalid 'record': cannot use IANA reserved IP address Martin I would prefer if there was a single --create-reverse option for both A and records, as it IMO makes more sense from user's point of view. What do you think? Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Implement audit_as kdb layer function
On Tue, 2012-02-14 at 10:22 -0500, Rob Crittenden wrote: Simo Sorce wrote: Without this function the audit counters (krbLastFailedAuth, krbLastSuccessfulAuth, krbLoginFailedCount) are not updated causing a regression. This function updates the counters unconditionally upon successful/failed authentication (only if pre-auth is used which is the default in FreeIPA). A side effect of how this is implemented is that no other attributes are updated when this happens so that replication is not kicked (because we filter audit counters from replication to avoid replication storms), in 2.1.x updating these counters also ended up updating krbExtraData and that caused replication to kick in. Simo. This still isn't working quite right. Can you tell me how did you test ? The user lockout is not working. The failed counter plateaus at the lockout value (in my case 6). Any failures beyond 6 do not increment the counter, I'm assuming there is some other interaction going on. It does set the dates properly. The audit functions do not lock the user, so please open a separate bug for that. I need to investigate where it is done in the kdc's default database code to see if it is yet another function that has been delegated to the driver. The reason it does not go beyond 6 is probably that max_fail is set to 5 ? Can you confirm ? If that's the case stopping at 6 is correct according to my reading of the equivalent code in MIT's own database (perhaps I should stop at 5 even, we can debate that). Tangential to that I think we may have a bug in the ldap bind preop plugin. I think we reset failures if someone successfully binds, the problem is that those are cleared even for GSSAPI binds, and that is wrong I think. Because it means that if a suer has a valid ticket for LDAP and keeps querying it, we keep resetting the counter but no real authentication has been performed. An attcker could wait such an event to sneak in up to max_fail -1 auth attemepts and then wai tfor a user bind to clear the flag. I think the reset should happen only on password based logins, and defer to the KDC to clear the auth count if the user makes an actual successful AS request. If you agree I will open a ticket on that. 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] 202 Add reverse DNS record when forward is created
On Tue, 2012-02-14 at 16:52 +0100, Jan Cholasta wrote: On 10.2.2012 16:42, Martin Kosek wrote: On Tue, 2012-02-07 at 16:26 +0100, Martin Kosek wrote: On Mon, 2012-02-06 at 15:56 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Mon, 2012-01-30 at 11:52 -0500, Rob Crittenden wrote: Martin Kosek wrote: Adding reverse DNS record may be a time consuming task, especially for IPv6 addresses. Having a way to automatically create a reverse record when a forward record is created could speed up the process. host-add command already has this possibility. This patch takes advantage of the new per-type API and adds new options for A/ record types: --a-create-reverse and ---create-reverse. These commands can be used to automatically create reverse records for new A/ addresses (both forward and reverse zones need to be managed by FreeIPA server): ipa dnsrecord-add example.com foo --a-rec=10.0.0.1 --a-create-reverse This command would add a new A record to record foo in zone example.com and a PTR record to appropriate reverse zone for IP address 10.0.0.1 (for example PTR record 1 in zone 0.0.10.in-addr.arpa. pointing to foo.example.com.). Few modification were done to new DNS API to support this feature: - Refactor --ip-address option handling from host-add and place it to dns.py to be used by both modules - Add support for extra per-type options - Hide DNS record part options in dnsrecord_find command as they have no effect for this command https://fedorahosted.org/freeipa/ticket/2009 Can the options -a-create-reverse and --create-reverse be combined? I was able to create an IPv4 addr using --create-reverse: # ipa dnsrecord-add example.com baz --a-rec=192.168.166.115 ---create-reverse Record name: baz A record: 192.168.166.115 Otherwise the patch seems fine. These 2 options can be combined, you can add both A and forward records and create records in their reverse records at the same time: ipa dnsrecord-add example.com bar --a-rec=10.0.0.1 --a-create-reverse ---rec=2001::beef:1 ---create-reverse In your case the option ---create-reverse is ignored as there is no rec added. Thus no record callback which would create this reverse record is called. We may implement some checks which would throw a validation error when --a-create-reverse/---create-reverse is called without a respective A/ record. Martin Yes, I think that is the way to go, otherwise this is confusing. rob Now, an exception is thrown if you try to pass --rrtype-create-reverse without an appropriate --rrtype-rec option filled: # ipa dnsrecord-add example.com baz --a-rec=192.168.166.115 ---create-reverse ipa: ERROR: 'record' is required I also refactored pre_callback of dnsrecord-add command a little, I didn't like parsingrrtype from parameter name using regexes. Now, every DNS part option has a link to parent DNS record stored in hint attribute. Martin Petr Vobornik noticed that reserved IP address passed to --a-rec (---rec) causes an Internal Error when --a-create-reverse is set at the same time: # ipa dnsrecord-add example.com foo ---ip-address=F:F:F:A::12 ---create-reverse ipa: ERROR: an internal error has occurred Attached patch fixes it: # ipa dnsrecord-add example.com foo ---ip-address=F:F:F:A::12 ---create-reverse ipa: ERROR: invalid 'record': cannot use IANA reserved IP address Martin I would prefer if there was a single --create-reverse option for both A and records, as it IMO makes more sense from user's point of view. What do you think? Honza I am not sure this would be a good idea for several reasons: 1) It would be inconsistent with the rest of new per-type API, i.e. --rr-option options like --mx-preference or --mx-exchanger generated by the new framework. These new options just follow this pattern: --a-create-reverse and ---create-reverse With these per-type options it is then also easy to run special RR callbacks, like this one without any other special code handling this case (--create-reverse is passed, call A and callbacks). It may also make WebUI DNS processing more difficult. 2) With current options you can run commands like this one: ipa dnsrecord-add example.com foo --a-rec=10.0.0.1 --a-create-reverse ---rec=2001:beef::2 and create reverse records just for the A record. (Although it may have a limited use of course). 3) It would create an inconsistency in command help. Now, you have sections for every record and see all relevant options: A Record: --a-rec=ARECORD Comma-separated list of raw A records --a-ip-address=STR IP Address --a-create-reverse Create reverse record for this IP Address Record: ---rec=RECORD
Re: [Freeipa-devel] [PATCH 61] Cache authentication in session
On 2/9/2012 8:32 AM, John Dennis wrote: Currently when the UI is loaded for the first time it will execute an ipa_init operation which consists of: 1. Loading I18 messages. 2. Getting user info (whoami). 3. Loading environment variables. 4. Checking whether DNS is enabled. 5. Loading objects commands metadata for labels and validations. Right now if the UI detects a change in the principal name or server version it will reload itself and so it will execute the ipa_init again. With your patch #61 the UI will automatically renew the session, but if the principal stays the same it won't execute the ipa_init again. I think the only thing that might change after session renewal is user info (#2). The others are pretty much static. So we probably can move whoami into another method that can be called separately. I agree this represents a good modification to the client logic however it don't see how it's specific to sessions. Wouldn't the same hold true for the current case when the principal changes? If someone already has a session then changes to another principal, will he get a new session and then reauthenticate against the login URL? Or will he use the same session but just reauthenticate? What if he then changes back to the previous principal, will he reuse the old session? If so will he be required to authenticate again? I think if the principal change is always preceded by reauthentication, the UI will only need to redo the whoami after successful login. Otherwise, the UI will need to check the principal change after each operation like in the current code. Would it be possible for someone from another machine to randomly guess a session ID and then take over an existing authenticated session? On top of your changes there would be some additional UI changes: 1. As described above, we need to move whoami out of ipa_init and call it after each session renewal. 2. The whoami output contains the user's authz info (group/role membership). We need to redraw the UI components if the user's authz has changed and make sure they are re-initialized correctly. 3. We need to redirect the user to the UI main page if the current page is no longer accessible due to authz changes. O.K. got it, here are my thoughts: 1) The enhanced logic is independent of sessions. 2) We need to test and exercise the new session auth so that code should be there. 3) However, adding the session logic in item 2 will be affected by the code changes in item 1. Therefore both should be done ASAP. We can either add the session code immediately and modify it later when the code changes in item 1 are done or put the session changes in immediately and modify it to use the new logic in item 1 when it's ready. I don't have strong feelings either way. However I would prefer if you or another UI guru made the changes you outline above rather than me. I think that would be more efficient and someone who intimately knows the UI code would be less likely to introduce a problem I might not be aware of. I think the code that is already pushed is fine, except that we loses the backward compatibility, and your next patch will fix that. After that the above UI changes can be done separately by the UI team. The only thing which might be unaccounted for would be if the web UI for some reason wanted to use the old /ipa/json and not use sessions. It would need some extra logic to handle this but I don't see any need for this, after the server is updated to support sessions it sends back an ipa.js javascript file to the client which always elects to use the /ipa/session/json URL. If for some reason the browser is still using an old copy of ipa.js it simply ends up using the old /ipa/json URL without sessions, which should just work. Right, no need to worry about this, the UI will just use one of the methods. On the second thought, after some discussions, we might want to support both cases in case a user cannot/would not use cookies. As long as both URL's are available, we can figure out later how the UI will use it. I do think we need a logout button which will invalidate the session auth. The current patch does not include an RPC command to accomplish that, but it's on my to-do list. Since we have to redo the patch to handle both session and non-session auth I could add that in at the same time (or we could open a new ticket and defer). Here are the tickets, the logout URL can be added separately if you want: https://fedorahosted.org/freeipa/ticket/2362 https://fedorahosted.org/freeipa/ticket/2363 -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 - Internationalize HBAC rule all category exceptions
On Tue, 2012-02-14 at 17:37 +0200, Alexander Bokovoy wrote: On Tue, 14 Feb 2012, Petr Viktorin wrote: On 02/14/2012 10:49 AM, Alexander Bokovoy wrote: On Tue, 14 Feb 2012, Petr Viktorin wrote: This patch wraps exception messages in _() https://fedorahosted.org/freeipa/ticket/2267 ACK. I was looking at hbactest and there are also some non-internationalized messages returned. Maybe you could combine them together with a slightly updated version of this patch? Yes; attaching updated patch. I also wrapped some texts in ipalib.output in _() calls. Thanks. ACK. Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 59-65 SSH public key management
On Thu, 2012-02-09 at 18:18 +0100, Jan Cholasta wrote: On 8.2.2012 16:35, Rob Crittenden wrote: Jan Cholasta wrote: Patch 62: need a failsafe to remove CCACHE_FILE in case something goes wrong. I should note too that this won't work on platforms prior to Python 2.6 (RHEL-5 is one). This is fine, just means host keys won't be automatically updated. What exactly won't work on Python 2.6? Sorry, I wasn't very clear. It isn't something specific to your patch, it is large portions of the framework in general. Just wanted to alert you. rob Updated rebased the patches. There is going to be one additional patch, which will make IPA take advantage of the new SSH support in SSSD. I have decided not to submit it now, because it breaks ipa-client-install if SSSD isn't patched with my Add missing services to sssd.api.conf and Add methods for activating and deactivating services to SSSDConfig patches (see sssd-devel). I'll submit it once the next SSSD beta is released. Honza Ok, I went through the patches and they works and generally looks ok (although some minor rebasing is needed before the push). I just have one concern at the moment. If you update FreeIPA server with DNS support, it won't update the update policy for current zones. Thus, only A and record update is allowed and ipa-client-install always fail to update SSHFP records in such zones. But I don't think its crucial, I would be OK with pushing the patches as they are and create another ticket to either fix or document it. Otherwise ACK. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 947 fix synxtax in 30-s4u2proxy.update
On Mon, 2012-02-13 at 11:43 -0500, Rob Crittenden wrote: Remove quotes around a value in 30-s4u2proxy.update. The update was failing to apply. I also noticed that FQDN wasn't being set properly in all cases in sub_dict. This should fix it. rob This patch did not apply for me. I guess it depends on some other patch that fixes wrong DN in s4u2proxy ipaAllowedTargets: -default: ipaAllowedTarget: 'cn=ipa-ldap-delegation-targets,cn=s4u2proxy,cn=etc,$SUFFIX' +default: ipaAllowedTarget: cn=ipa-ldap-delegation-targets,cn=s4u2proxy,cn=etc,$SUFFIX Current update file says: default: ipaAllowedTarget: 'cn=ipa-ldap-delegation-targets,cn=etc,$SUFFIX' which is a non-existent DN. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Implement audit_as kdb layer function
Simo Sorce wrote: On Tue, 2012-02-14 at 10:22 -0500, Rob Crittenden wrote: Simo Sorce wrote: Without this function the audit counters (krbLastFailedAuth, krbLastSuccessfulAuth, krbLoginFailedCount) are not updated causing a regression. This function updates the counters unconditionally upon successful/failed authentication (only if pre-auth is used which is the default in FreeIPA). A side effect of how this is implemented is that no other attributes are updated when this happens so that replication is not kicked (because we filter audit counters from replication to avoid replication storms), in 2.1.x updating these counters also ended up updating krbExtraData and that caused replication to kick in. Simo. This still isn't working quite right. Can you tell me how did you test ? The user lockout is not working. The failed counter plateaus at the lockout value (in my case 6). Any failures beyond 6 do not increment the counter, I'm assuming there is some other interaction going on. It does set the dates properly. The audit functions do not lock the user, so please open a separate bug for that. I need to investigate where it is done in the kdc's default database code to see if it is yet another function that has been delegated to the driver. The reason it does not go beyond 6 is probably that max_fail is set to 5 ? Can you confirm ? If that's the case stopping at 6 is correct according to my reading of the equivalent code in MIT's own database (perhaps I should stop at 5 even, we can debate that). Opened ticket https://fedorahosted.org/freeipa/ticket/2393 for lockout. Tangential to that I think we may have a bug in the ldap bind preop plugin. I think we reset failures if someone successfully binds, the problem is that those are cleared even for GSSAPI binds, and that is wrong I think. Because it means that if a suer has a valid ticket for LDAP and keeps querying it, we keep resetting the counter but no real authentication has been performed. An attcker could wait such an event to sneak in up to max_fail -1 auth attemepts and then wai tfor a user bind to clear the flag. I think the reset should happen only on password based logins, and defer to the KDC to clear the auth count if the user makes an actual successful AS request. If you agree I will open a ticket on that. That sounds reasonable to me, I think you should match the current behavior of the KDC whatever it may be. I wasn't able to test the lockout changes of this but the code looks ok, so ACK given the new ticket(s). You'll need a slight rebase against ipa-2-2. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 59-65 SSH public key management
Martin Kosek wrote: On Thu, 2012-02-09 at 18:18 +0100, Jan Cholasta wrote: On 8.2.2012 16:35, Rob Crittenden wrote: Jan Cholasta wrote: Patch 62: need a failsafe to remove CCACHE_FILE in case something goes wrong. I should note too that this won't work on platforms prior to Python 2.6 (RHEL-5 is one). This is fine, just means host keys won't be automatically updated. What exactly won't work on Python 2.6? Sorry, I wasn't very clear. It isn't something specific to your patch, it is large portions of the framework in general. Just wanted to alert you. rob Updated rebased the patches. There is going to be one additional patch, which will make IPA take advantage of the new SSH support in SSSD. I have decided not to submit it now, because it breaks ipa-client-install if SSSD isn't patched with my Add missing services to sssd.api.conf and Add methods for activating and deactivating services to SSSDConfig patches (see sssd-devel). I'll submit it once the next SSSD beta is released. Honza Ok, I went through the patches and they works and generally looks ok (although some minor rebasing is needed before the push). I just have one concern at the moment. If you update FreeIPA server with DNS support, it won't update the update policy for current zones. Thus, only A and record update is allowed and ipa-client-install always fail to update SSHFP records in such zones. But I don't think its crucial, I would be OK with pushing the patches as they are and create another ticket to either fix or document it. Otherwise ACK. Martin Can you open a ticket on that? ACK, pushed all 11 to master and ipa-2-2. I updated the commit messages to include a ticket number in each for tracking. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 947 fix synxtax in 30-s4u2proxy.update
Martin Kosek wrote: On Mon, 2012-02-13 at 11:43 -0500, Rob Crittenden wrote: Remove quotes around a value in 30-s4u2proxy.update. The update was failing to apply. I also noticed that FQDN wasn't being set properly in all cases in sub_dict. This should fix it. rob This patch did not apply for me. I guess it depends on some other patch that fixes wrong DN in s4u2proxy ipaAllowedTargets: -default: ipaAllowedTarget: 'cn=ipa-ldap-delegation-targets,cn=s4u2proxy,cn=etc,$SUFFIX' +default: ipaAllowedTarget: cn=ipa-ldap-delegation-targets,cn=s4u2proxy,cn=etc,$SUFFIX Current update file says: default: ipaAllowedTarget: 'cn=ipa-ldap-delegation-targets,cn=etc,$SUFFIX' which is a non-existent DN. Martin It relies on patch 941 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 940 apply some validation to some classes only
Jan Cholasta wrote: On 14.2.2012 16:44, Jan Cholasta wrote: On 7.2.2012 20:25, Rob Crittenden wrote: Rob Crittenden wrote: Jan Cholasta wrote: Dne 7.2.2012 09:27, Martin Kosek napsal(a): On Mon, 2012-02-06 at 11:52 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Fri, 2012-02-03 at 16:58 -0500, Rob Crittenden wrote: There is some validation that we only need to apply when an entry is being created, namely the key itself. This is to allow us to manage an otherwise illegal entry that finds its way into the system (i.e. migration). Consider this. We migrate a group with a space in it. This isn't allowed in IPA but we also provide no way to delete it because the cn regex kicks out the group-del command. The trick is adding appropriate context so we can know during validation how we got here. A command object has a bases field which contains the base classes associated with it, which appears to contain only the leaf baseclass. So using this we can tell how we got to validation and can skip based on that baseclass name. I went back and forth a bit on where the right place to put this was, I'm open to more fine tuning. I initially skipped just the pattern validation then expanded it to apply to all validation in the Parameter. rob 1) This patch breaks API.txt, it needs re-generating. 2) This may be a matter of opinion but I think that + if self.onlyvalidateclasses and \ + not any(x in self.onlyvalidateclasses for x in classbases): + return would be more readable than: + if self.onlyvalidateclasses and \ + not [x for x in classbases if x in self.onlyvalidateclasses]: + return 3) I would move the entire self.onlyvalidateclasses up to the validate() method. It would have several benefits: - the validation skip would be done just once, not for every value as the decision does not use the value at all - we would not have to modify _validate_scalar() methods at all since they won't need classbases 4) I think it would be better to keep validation for --rename options. As it is generated from RDN attribute parameter, it inherits onlyvalidateclasses list as well. Otherwise your patch would let user to rename a valid object to an invalid one: # ipa user-mod fbar --rename=bad boy Modified user fbar User login: bad boy First name: Foo Last name: Bar Home directory: /home/fbar Login shell: /bin/sh UID: 480800040 GID: 480800040 Account disabled: False Password: False Member of groups: ipausers Kerberos keys available: False Martin This should address your concerns. rob Its almost OK, there is just one part that's not that OK: @@ -831,6 +836,9 @@ class Param(ReadOnly): else: raise RequirementError(name=self.name) return + if 'rename' not in (self.name, self.cli_name): + if self.onlyvalidateclasses and not [x for x in self.onlyvalidateclasses if x in classbases]: #pylint: disable=E1101 + return if self.multivalue: if type(value) is not tuple: raise TypeError( I don't think that hard-coding this skip for onlyvalidateclasses based just on parameter name is a good thing to do and may cause problems in the future. For example if we create some option called rename and deliberately set onlyvalidateclasses for the option - it would then be skipped as well. I think it would be a better solution to just update _get_rename_option() in baseldap.py to set onlyvalidateclasses to () Martin I must say I'm not a big fan of this approach. You are healing the symptoms (custom validation on some parameters makes it impossible to manipulate existing LDAP entries with invalid attribute values = add a way to mark parameters to be validated only in certain commands to prevent that), but the real issue here is that we should not perform custom validation when referring to existing LDAP attribute values at all (or only partially), no matter what parameter and command. Fixing this would make the problem go away for all commands, present or future, without the need for adding a list of command classes to each parameter that is affected. Honza It is all about context, and parameters have very little of it. The bottom line is we only want to do validation on new values. I think I can bump this up a level by adding a validate boolean to Param and Command both defaulting to False. crud.Create will override this to True and cloning rename could perhaps also set this flag. Then in validation if the parent object has validation set or the parameter does we validate, otherwise we skip it. This will require some other changes for Enums, they may always require validation (I'll need to be sure --delattr can remove a bad one). rob Updated patch. The primary key will be validated only on adds. The values will be validated on adds and mods. It turns out there already is infrastructure which does exactly the same thing: the query kwarg of the Parameter class. We should fix its behavior rather than reinvent the wheel. This might be good enough: diff --git a/ipalib/crud.py
Re: [Freeipa-devel] [PATCHES] 59-65 SSH public key management
On Tue, 2012-02-14 at 15:33 -0500, Rob Crittenden wrote: Martin Kosek wrote: On Thu, 2012-02-09 at 18:18 +0100, Jan Cholasta wrote: On 8.2.2012 16:35, Rob Crittenden wrote: Jan Cholasta wrote: Patch 62: need a failsafe to remove CCACHE_FILE in case something goes wrong. I should note too that this won't work on platforms prior to Python 2.6 (RHEL-5 is one). This is fine, just means host keys won't be automatically updated. What exactly won't work on Python 2.6? Sorry, I wasn't very clear. It isn't something specific to your patch, it is large portions of the framework in general. Just wanted to alert you. rob Updated rebased the patches. There is going to be one additional patch, which will make IPA take advantage of the new SSH support in SSSD. I have decided not to submit it now, because it breaks ipa-client-install if SSSD isn't patched with my Add missing services to sssd.api.conf and Add methods for activating and deactivating services to SSSDConfig patches (see sssd-devel). I'll submit it once the next SSSD beta is released. Honza Ok, I went through the patches and they works and generally looks ok (although some minor rebasing is needed before the push). I just have one concern at the moment. If you update FreeIPA server with DNS support, it won't update the update policy for current zones. Thus, only A and record update is allowed and ipa-client-install always fail to update SSHFP records in such zones. But I don't think its crucial, I would be OK with pushing the patches as they are and create another ticket to either fix or document it. Otherwise ACK. Martin Can you open a ticket on that? https://fedorahosted.org/freeipa/ticket/2394 I can simply enhance dns.py update plugin added by my patch set 95-99 (when its acked) to update the policies as well as idnsAllowQuery and idnsAllowTransfer. Martin ACK, pushed all 11 to master and ipa-2-2. I updated the commit messages to include a ticket number in each for tracking. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 944 upgrade files for selinuxusermap
Rob Crittenden wrote: The update files were missing for SELinuxUserMap support, this adds them. Rebased patch rob freeipa-rcrit-944-2.selinux.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 944 upgrade files for selinuxusermap
Rob Crittenden wrote: Rob Crittenden wrote: The update files were missing for SELinuxUserMap support, this adds them. Rebased patch rob Sorry, sent the wrong patch. Here is the right one. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Implement audit_as kdb layer function
On Tue, 2012-02-14 at 14:31 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-02-14 at 10:22 -0500, Rob Crittenden wrote: Simo Sorce wrote: Without this function the audit counters (krbLastFailedAuth, krbLastSuccessfulAuth, krbLoginFailedCount) are not updated causing a regression. This function updates the counters unconditionally upon successful/failed authentication (only if pre-auth is used which is the default in FreeIPA). A side effect of how this is implemented is that no other attributes are updated when this happens so that replication is not kicked (because we filter audit counters from replication to avoid replication storms), in 2.1.x updating these counters also ended up updating krbExtraData and that caused replication to kick in. Simo. This still isn't working quite right. Can you tell me how did you test ? The user lockout is not working. The failed counter plateaus at the lockout value (in my case 6). Any failures beyond 6 do not increment the counter, I'm assuming there is some other interaction going on. It does set the dates properly. The audit functions do not lock the user, so please open a separate bug for that. I need to investigate where it is done in the kdc's default database code to see if it is yet another function that has been delegated to the driver. The reason it does not go beyond 6 is probably that max_fail is set to 5 ? Can you confirm ? If that's the case stopping at 6 is correct according to my reading of the equivalent code in MIT's own database (perhaps I should stop at 5 even, we can debate that). Opened ticket https://fedorahosted.org/freeipa/ticket/2393 for lockout. Tangential to that I think we may have a bug in the ldap bind preop plugin. I think we reset failures if someone successfully binds, the problem is that those are cleared even for GSSAPI binds, and that is wrong I think. Because it means that if a suer has a valid ticket for LDAP and keeps querying it, we keep resetting the counter but no real authentication has been performed. An attcker could wait such an event to sneak in up to max_fail -1 auth attemepts and then wai tfor a user bind to clear the flag. I think the reset should happen only on password based logins, and defer to the KDC to clear the auth count if the user makes an actual successful AS request. If you agree I will open a ticket on that. That sounds reasonable to me, I think you should match the current behavior of the KDC whatever it may be. I wasn't able to test the lockout changes of this but the code looks ok, so ACK given the new ticket(s). You'll need a slight rebase against ipa-2-2. Pushed to master and ipa-2-2 Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
