Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP
On 06/09/2014 05:54 PM, Dmitri Pal wrote:
On 06/09/2014 10:03 AM, Nathaniel McCallum wrote:
On Mon, 2014-06-09 at 09:01 -0400, Simo Sorce wrote:
From: Martin Kosek mko...@redhat.com
Given all sort of issues we get, I am thinking we should just revert it
unless
there is a quick fix available.
Instead of reverting I am thinking we may want to make this optional by
adding a configuration parameter that defaults to False for now. Once we can
manage better the password change we can turn it on by deault, in the
meanwhile admins can choose by themselves the lesser evil.
Thoughts?
I'm not a fan of introducing a new configuration parameter for a
temporary workaround.
My preference is to revert it and have a small project for the next
release which handles all the non-authenticated corner cases. This
would include:
* Expired passwords
* Password changes
* Token syncing
* Unauthenticated RPCs (rpcserver.py rework)
* others?
I think there is some value to be gained by thinking about these
problems as a whole and devising a set of consistent mechanisms for
them.
Nathaniel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
+1
This is my preference as well, as written in other part of this thread.
I reverted the patch, added an appropriate description (attached) and pushed to
master.
I updated the ticket, added Nathaniel's suggestions and moved it to needs
triage.
Thanks,
Martin
From c41b782bc59cd0cb70cbd62d543f9c538109d410 Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Tue, 10 Jun 2014 08:42:03 +0200
Subject: [PATCH] Revert Check for password expiration in pre-bind
This reverts commit bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6.
Forceful validation of password expiration date in a BIND pre-callback
breaks LDAP password change extended operation as the password change
is only allowed via authenticated (bound) channel. Passwords could be
only changed via kadmin protocol. This change would thus break
LDAP-only clients and Web UI password change hook.
This patch will need to be revisited so that unauthenicated corner
cases are also revisited.
https://fedorahosted.org/freeipa/ticket/1539
---
daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 33 +++
1 file changed, 4 insertions(+), 29 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
index 6786c6ddb4de4158ec680e271cae29318bc150ce..23c7cb18c9a1cb5256254f20080c5d9aaec25579 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
@@ -1217,35 +1217,13 @@ static bool ipapwd_pre_bind_otp(const char *bind_dn, Slapi_Entry *entry,
}
static int ipapwd_authenticate(const char *dn, Slapi_Entry *entry,
- const struct berval *credentials,
- const char **errmsg)
+ const struct berval *credentials)
{
Slapi_Value **pwd_values = NULL; /* values of userPassword attribute */
Slapi_Value *value = NULL;
Slapi_Attr *attr = NULL;
-struct tm expire_tm;
-char *expire;
-char *p;
int ret;
-/* check the if the krbPrincipalKey attribute is present */
-ret = slapi_entry_attr_find(entry, krbprincipalkey, attr);
-if (!ret) {
-/* check that the password is not expired */
-expire = slapi_entry_attr_get_charptr(entry, krbpasswordexpiration);
-if (expire) {
-memset(expire_tm, 0, sizeof (expire_tm));
-p = strptime(expire, %Y%m%d%H%M%SZ, expire_tm);
-if (*p) {
-LOG(Invalid expiration date string format);
-return 1;
-} else if (time(NULL) mktime(expire_tm)) {
-*errmsg = The user password is expired;
-return 1;
-}
-}
-}
-
/* retrieve userPassword attribute */
ret = slapi_entry_attr_find(entry, SLAPI_USERPWD_ATTR, attr);
if (ret) {
@@ -1403,7 +1381,7 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
static const char *attrs_list[] = {
SLAPI_USERPWD_ATTR, ipaUserAuthType, krbprincipalkey, uid,
krbprincipalname, objectclass, passwordexpirationtime,
-passwordhistory, krbprincipalexpiration, krbpasswordexpiration,
+passwordhistory, krbprincipalexpiration,
NULL
};
struct berval *credentials = NULL;
@@ -1416,7 +1394,6 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
time_t expire_time;
char *principal_expire = NULL;
struct tm expire_tm;
-const char *errmsg = NULL;
/* get BIND parameters */
ret |= slapi_pblock_get(pb, SLAPI_BIND_TARGET, dn);
@@ -1477,12 +1454,10 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
}
/* Authenticate the user. */
-ret = ipapwd_authenticate(dn, entry,
Re: [Freeipa-devel] [PATCH] 592-628 Update to PatternFly
On 10.6.2014 01:52, Endi Sukma Dewata wrote: On 6/9/2014 8:46 AM, Petr Vobornik wrote: I've fixed issues #4, #2, #20 and #18. Commits in the branch, no rebase. With these 4 changes we are ready for the push. I'll squash them, if necessary. You mean #11 instead of #2? The fixes are confirmed. Yes #11, sorry for the confusion. Fixed for issue #20 was squashed into patch #640. Pushed to master: * ff17af16e7ba953a70c434fea1dbe128810f6028 webui: remove logout.html * b577b3d3656f51edf0c33bdb00863c03b11ae512 webui: remove login.html * 6a8eeff22d98c8c32c770869427883198278d077 webui: add PaternFly css * 78f026bc9013347d4bc2b4c02e72b19495a1b8ac webui: apply PatternFly login theme on reset_password.html * 1829fa2c1571428cb4318443387dde1707fc9641 webui: apply PatternFly theme on config pages * 5a2aed99baa059e9ccdfd9f0d2f2b4cb68ba8930 webui: styles for alert icons * f0cf2e10d5ca6ce261c256288e2b6d15b23b1418 webui: apply PatternFly theme on migration pages * b5ebdb604bd2c6edbbd99bbd7f21306cbf367412 webui: remove remnants of jquery-ui * 6b5b9a118522a53bca40532aa2df326d0f7bfe38 webui: remove unused icons * 563dcdc3ebbc11cd979454e75e664767f9ea43f8 webui: remove unused collapsible feature from section * 7e94ee11eb377c8d4ec72e04f618c76f0a30e7a4 webui: remove unused images * 4333161ac36c9487f225d61c79d8ff904f51d629 webui: change absolutely positioned layout to fluid * 0e15a282e85b0e3eb71fd3fce1965646aeb47a27 webui: remove column sizing in tables, use PF styles * 3eaa69a68681a2478a6feeff7fb9e4cf2a27deee webui: change navigation from RCUE to PatternFly * 2e9e5792bc7c5fd1ed65b4d72e3915442db060f5 webui: adjust styles to PatternFly * bcb2ce7f2464281923dd54396fa18d62e48ffebe webui: display undo and multivalued delete buttons in input-group * 216e710188279d15c262da907efbc09be92fb50a webui: allow multiple base section layouts * ad338b9d74fd3aa22bce5680b39fdaae3b90d5ca webui: change breadcrumb to PatternFly * 3dd34d6e55ae5d671158e2fa8f0213072d897036 webui: use h1 in facet title instead of h3 * fc0926ba91f57cb5cd182f2edf50f24d4cfd7432 webui: remove action list widget * c7af2458091ba95eccc26f4468234413e8b016b9 webui: add action dropdown * ec9539d0fde6c368e15c0a07ff75f82adba3f36e webui: add space between action buttons's icon and text * be3aadd06ad2cdc434827e78e5227f34ecf63aa0 webui: remove select action * a98df325b6b0d7bf41509e8c8247a9422f179429 webui: add confirmation to action dropdown actions * 4e1c0ad423b0b78f620b72a07d2c174ec76d4a82 webui: move certificate actions to action dropdown * 2f3dc7908d1c62b729ab38b6d684dc0e942c4528 webui: move user reset password action to action dropdown * faf4fea30fd01ad5f5c372877d0e8fe20963dc91 webui: patternFly dialog * dff5f6319fd76d6b67b30be4eadbcdb414784802 webui: adjust association adder dialog to PatternFly * f631b07507d12d2ab5c1b535a987f09cb07a5565 webui: activity indicators * 21651d9d3f463f5c07355d0f4e52a8d3867d88cc webui: improve pagination * 9c1da611ea94046090dd88d320253d2cded5c76e webui: do not show empty table footer * 54990227824957fb1be4a45ee0f0879812d71d94 webui: restyle automember default group * 4f45e3ea924cd64f4c1f698dfd762d561226199e webui: preload automember default group select list * ea93590ef1051c17fa55115edc14d646581188e4 webui: adjust login page to PatternFly * 74fc85d003af9376462b0610593f6ab8e8a34bf1 webui: use BS alerts in validation_summary_widget * bf9eeb823b5df3e581061b23ee89f3b94b0c87b3 webui-ci: select search table item - chrome issue * 99ed015c0a3ad5f4c1d16190cfa513299ed1cf6c webui: remove old css for standalone pages * 5c3fd4bb832859172fa6e7c739724f6562ccfb7f webui: adjust header controls alignment * 40a25ecf371ebfcbb0801e1f99e3a2853439f44b webui: add search box placeholder text * 408457ce53c553c27f25a682f3f5118ae2e1ab30 webui: change control buttons to normal buttons * 05a917eb17d7c99338dcb972470f15d9d83b37b4 webui: certificate search - select search attribute only when defined * 29f60931e2bee750e5acdf7bead5ed3b21d7e4d5 webui: association adder dialog - change find label to filter * 2df5e0b132be032dc5d12ec65314a44af87b6deb webui: use dark color for facet titles without pkey * 841e0cd3ae904cb8021c4387472b4e66487a5c6a webui-ci: assert_action_list_action * 2af21743df2e09cabcf4d2209cd35786ed3cfdd6 webui: move host action panel actions to action dropdown * 254b41e485e065b870205bb1bc50701451800bff webui: move service action panel actions to action dropdown * dd69557f4e51aa0ab35d82a5c1f67cea20303e70 webui: use normal buttons instead of link buttons in multivalued widget * 0fadb14ec7fe84596058e106a5e8068afed81f51 webui: move radius proxy action panel commands to header actions * bedd128de07f502cdc68d8c7a9f6b8ef48d1727b webui: proper alerts in dialogs * bc6105b270ad10349d12d144b5f67660e7f734e3 webui: use propert alerts in header notification area * dea2da4455b3a4bd189828e2d22f19ab90bf75bd webui: fix search box overlap in mobile mode * 31df435e4194927649fbdacaa3a62fc28b8f5029 webui: fix layout of QR code on wide screens
Re: [Freeipa-devel] [PATCHES] 0568-0570 Convert User default permissions to managed
On 06/10/2014 10:13 AM, Martin Kosek wrote:
On 06/09/2014 02:20 PM, Petr Viktorin wrote:
On 06/06/2014 11:38 AM, Martin Kosek wrote:
[...]
4) When running user unit tests, I found couple issues:
a) Some attributes we may still miss in the permissions:
- krbPrincipalExpiration
- userclass
- ipaUserAuthType
- preferredLanguage
I am thinking we could base Modify Users permission on the read one and add
regular attributes there
I put in userclass and preferredLanguage.
I'm not sure about the other two; should regular user admins be able to change
these?
Good question. I think User Administrators should be able to set
krbPrincipalExpiration or ipaUserAuthType, though it is true it may not be part
of regular Modify Users permission and we may want to create more permissions.
Simo, does that sound OK?
I can't think of a good name. Manage User authentication?
Note that this can go in a later patch.
b) Read membership ACIs for users and groups miss member attribute and thus
indirect/direct processing goes wrong.
Added.
1) Hm, I think was not clear enough. memberOf should not be added to users, as
no object should be member of user. Please revert this change. I originally
thought it is missing in Read Group Membership permissions, but it isn't.
We are again hitting the problem of a search operation when we are not allowed
to search on all attributes (the CVE fix). This is the search produced by ipa
user-show fbar:
[10/Jun/2014:09:59:28 +0200] conn=155 op=5 SRCH base=dc=example,dc=com
scope=2
filter=(|(member=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberUser=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberHost=uid=fbar,cn=users,cn=accounts,dc=example,dc=com))
attrs=
[10/Jun/2014:09:59:28 +0200] conn=155 op=5 RESULT err=0 tag=101 nentries=0
etime=0
It returns zero results, until I also allow memberUser and memberHost:
# ipa permission-mod 'System: Read Group Membership'
--attrs={member,memberof,memberuid,memberuser,memberhost}
Now I get the results:
[10/Jun/2014:10:04:25 +0200] conn=160 op=5 SRCH base=dc=example,dc=com
scope=2
filter=(|(member=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberUser=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberHost=uid=fbar,cn=users,cn=accounts,dc=example,dc=com))
attrs=
[10/Jun/2014:10:04:25 +0200] conn=160 op=5 RESULT err=0 tag=101 nentries=1
etime=0
ipa user-show fbar
...
Member of groups: ipausers
Indirect Member of role: test
...
User still cannot see if he is direct or indirect member of role, but this may
not be a problem.
The easiest approach solution may be to update all Read * Membership
permissions/ACIs to always contain membermemberusermemberhost unless we want
to again produce multiple LDAP searches for checking direct/indirect membership.
Ah, now I see what you mean.
So: a read permission that includes any of these 3 should include all of
them.
It would make sense for makeaci to enforce this, so I'll include it in
the other patchset.
2) Installation produces update errors:
ipa.ipaserver.install.ldapupdate.LDAPUpdate: ERRORAdd failure missing
required attribute objectclass
ipa.ipaserver.install.ldapupdate.LDAPUpdate: ERRORAdd failure missing
required attribute objectclass
Problem is in install/updates/45-roles.update, permissions you converted still
have member update here.
dn: cn=Change a user password,cn=permissions,cn=pbac,$SUFFIX
add:member: 'cn=Modify Users and Reset passwords,cn=privileges,cn=pbac,$SUFFIX'
dn: cn=Modify Users,cn=permissions,cn=pbac,$SUFFIX
add:member: 'cn=Modify Users and Reset passwords,cn=privileges,cn=pbac,$SUFFIX'
Speaking if which, this privilege also needs to be added to default privileges
of the managed permissions.
Thanks for the catch.
--
Petr³
From 8b44bca76cb73aa0b0bc14e165c4462885b9b2a7 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Wed, 4 Jun 2014 16:11:30 +0200
Subject: [PATCH] managed perm updater: Handle case where we changed default
ACIs in the past
This handles the case where IPA's default ACIs changed in something else
than just attribute lists.
In this case we can narrow the set of ACIs we think the user might be
upgrading from.
Part of the work for: https://fedorahosted.org/freeipa/ticket/4346
---
.../install/plugins/update_managed_permissions.py| 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/ipaserver/install/plugins/update_managed_permissions.py b/ipaserver/install/plugins/update_managed_permissions.py
index 13433d353cd09de77029fd76f7070bf79a432774..e6f852c09d1a732109da5d56320192d4e617ab38 100644
--- a/ipaserver/install/plugins/update_managed_permissions.py
+++ b/ipaserver/install/plugins/update_managed_permissions.py
@@ -408,11 +408,20 @@ def get_upgrade_attr_lists(self, current_acistring, default_acistrings):
An attribute will be included if the user has it in LDAP but it does
not appear in *any* historic ACI.
It will be excluded
Re: [Freeipa-devel] [PATCHES] 0572-0575 Add ACI.txt + default bind rule type
On 06/10/2014 03:22 PM, Petr Viktorin wrote:
On 06/10/2014 01:30 PM, Martin Kosek wrote:
On 06/10/2014 10:05 AM, Petr Viktorin wrote:
On 06/09/2014 08:08 PM, Petr Viktorin wrote:
Having another verification tool should help reviewing the permission
patches.
To avoid conflicts, apply on top of my patches 0568-0570 (Write User
permissions).
0572: I tried to make the ACIs generated by the permission plugin as
predictable as possible, but I missed one place it's affected by
dict/set iteration order (which is undefined). Here's a fix.
0573: Minor refactoring to make the next patch easier.
0574: Add ACI.txt makeaci. Due to the predictable ACIs, all this needs
to do is generate the file; comparing can be done bit-by-bit.
I do run the validation results through difflib, but frankly it's easier
just to use Git.
On my way home yesterday I remembered I left out an important piece of
information - the DN where the ACI is. Attaching updated patch 0574.
0575: Make 'permission' the default bind rule type for managed
permissions. Rationale in the commit message.
Run makeaci to verify this doesn't change the result.
This will create some additional burden for you when converting ACIs, but the
idea is good.
Any ACI.txt conflicts are easily resolved by running makeaci, and I think the
additional verification is worth it.
Yup, I am not complaining :-)
The script worked for me, can we just create some more friendly error message
than an assertion traceback?
# ./makeaci --validate
...
Traceback (most recent call last):
File ./makeaci, line 116, in module
main(options)
File ./makeaci, line 108, in main
raise AssertionError('validation failed')
AssertionError: validation failed
The tool is meant for developers and packagers; tracebacks are designed to be
friendly for this target group.
But, OK, I've made it exit() instead, and added some instructions.
Ok, thanks.
From the thread on user permissions:
On 06/10/2014 10:13 AM, Martin Kosek wrote:
1) Hm, I think was not clear enough. memberOf should not be added to users,
as
no object should be member of user. Please revert this change. I originally
thought it is missing in Read Group Membership permissions, but it isn't.
We are again hitting the problem of a search operation when we are not
allowed
to search on all attributes (the CVE fix). This is the search produced by
ipa
user-show fbar:
[10/Jun/2014:09:59:28 +0200] conn=155 op=5 SRCH base=dc=example,dc=com
scope=2
filter=(|(member=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberUser=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberHost=uid=fbar,cn=users,cn=accounts,dc=example,dc=com))
attrs=
[10/Jun/2014:09:59:28 +0200] conn=155 op=5 RESULT err=0 tag=101 nentries=0
etime=0
It returns zero results, until I also allow memberUser and memberHost:
# ipa permission-mod 'System: Read Group Membership'
--attrs={member,memberof,memberuid,memberuser,memberhost}
Now I get the results:
[10/Jun/2014:10:04:25 +0200] conn=160 op=5 SRCH base=dc=example,dc=com
scope=2
filter=(|(member=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberUser=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberHost=uid=fbar,cn=users,cn=accounts,dc=example,dc=com))
attrs=
[10/Jun/2014:10:04:25 +0200] conn=160 op=5 RESULT err=0 tag=101 nentries=1
etime=0
ipa user-show fbar
...
Member of groups: ipausers
Indirect Member of role: test
...
User still cannot see if he is direct or indirect member of role, but this
may
not be a problem.
The easiest approach solution may be to update all Read * Membership
permissions/ACIs to always contain membermemberusermemberhost unless we
want
to again produce multiple LDAP searches for checking direct/indirect
membership.
I've amended the read permissions for containerish objects, and added a check
to makeaci to enforce this in the future.
I insist on printing a traceback on error here. This check is meant for
developers, tracebacks are perfect for them.
If you insist, I can live with that. As you said, it is for packagers and
developers after all.
ACK to the whole patch set.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0065] Regression fix in host.py
DNS requires absolute zone name, host must provide it.
IDNA patch caused this.
Patch attached.
--
Martin^2 Basti
From bac9f62a7062d6fb25e9135d8fd62767411e46e0 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Tue, 10 Jun 2014 15:57:30 +0200
Subject: [PATCH] Make zonenames absolute in host plugin
This is fix for regression caused by IDNA patch, zone names must be
absolute.
---
ipalib/plugins/host.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index bd7a3a2ba375a65b587bf0b8b6d47c1797dbad6a..133d55356bbb100fc3c1f1451a68571129945861 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -453,7 +453,7 @@ class host_add(LDAPCreate):
check_reverse = not options.get('no_reverse', False)
add_records_for_host_validation('ip_address',
DNSName(host),
-DNSName(domain),
+DNSName(domain).make_absolute(),
options['ip_address'],
check_forward=True,
check_reverse=check_reverse)
@@ -505,7 +505,8 @@ class host_add(LDAPCreate):
if options.get('ip_address'):
add_reverse = not options.get('no_reverse', False)
-add_records_for_host(DNSName(host), DNSName(domain),
+add_records_for_host(DNSName(host),
+ DNSName(domain).make_absolute(),
options['ip_address'],
add_forward=True,
add_reverse=add_reverse)
--
1.8.3.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] Update plugins to use Registry API
Hi, On 6.6.2014 20:33, Nathaniel McCallum wrote: I kept seeing the old plugin registration style when writing/reviewing code and I decided to fix it. Attached are patches to update the remaining 31 plugins from the old plugin registration style to the new style. This would be a great starting point for someone new to doing reviews. Nathaniel I can't imagine a situation in which having these in separate commits would be beneficial, so I don't think this really deserves to be split among multiple patches. Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] Update plugins to use Registry API
On 10.6.2014 17:29, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 16:45 +0200, Jan Cholasta wrote: Hi, On 6.6.2014 20:33, Nathaniel McCallum wrote: I kept seeing the old plugin registration style when writing/reviewing code and I decided to fix it. Attached are patches to update the remaining 31 plugins from the old plugin registration style to the new style. This would be a great starting point for someone new to doing reviews. Nathaniel I can't imagine a situation in which having these in separate commits would be beneficial, so I don't think this really deserves to be split among multiple patches. My thought was to make it easier to review in small chunks for new reviewers. But if we want to do it as a single patch, one is attached. Nathaniel ACK btw it was easier to review the batch at once. should we also update the tutorial in ipalib/__init__.py and other comments? Note that there is quite a lot of `api.register(cls)` calls outside of ipalib/plugins dir. IMHO it's OK since it's not a subject of this patch. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote: Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() Instead of adding complex explanations I immediately return with an error if get_realm_backend() returns NULL. The logic here is correct, it just reads awkwardly. It is probably fine as is. There appear to be indiscriminate tab indents throughout the patch. Please changes these to spaces. There are because the coed is old, I do not change the style of a piece of code if we are just changing a few lines. You need to read the patch in the context of the code to seen it. If it were just the problem you alluded to, I wouldn't have called it out. I'm referring to tabs in the middle of new code that uses spaces. This is most likely the result of copy/paste. Either write all the new code to use tabs or match the copy/pasted lines to the surrounding new code (my preference). Nearly all the new code in ipapwd_setkeytab() uses space indents where the surrounding code uses tab indents. I'm not sure the block comment in is_allowed_to_access_attr() belongs there. Also, a utility function like is_allowed_to_access_attr() would probably be handy in upstream 389ds. I might use this in an upcoming token sync patch. This is not a requirement of ACK'ing the patch. 0002 ACK One small nitpick, then I too say ACK. In the commit message, the second sentence doesn't need a line break. I try to keep comments within 72 chars (though sometimes I forget and go past till 80), which is why there is a line break there. Yeah, it just looks bad when sent over email, which is why I noticed it. This didn't get fixed. Add should follow patch. on the same line. 0003 Same as patch 002: unnecessary line breaks in the commit message. See above. Also not fixed. The new set of keys should follow existing ones. on the same line. I think ipapwd_getkeytab() is unnecessarily long. Several sections of it could be broken out into functions and would make it much more readable. That has already been done :-) You should see the original ipa_setkeytab() ... I'm not talking about ipapwd_setkeytab(). I'm talking about ipapwd_getkeytab(). This is entirely new code. There are very clear spots where it could be broken up. I consider this the biggest issue in this set of patches for two important reasons: 1. It makes the function really hard to review. 2. It is one of the most security/policy sensitive part of the code. These two are a bad combo. Much better! I was a bit disappointed that decode_getkeytab_request()/encode_getkeytab_reply() don't output/input a single request/response struct, but
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-10 at 14:27 -0400, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote: Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() Instead of adding complex explanations I immediately return with an error if get_realm_backend() returns NULL. The logic here is correct, it just reads awkwardly. It is probably fine as is. There appear to be indiscriminate tab indents throughout the patch. Please changes these to spaces. There are because the coed is old, I do not change the style of a piece of code if we are just changing a few lines. You need to read the patch in the context of the code to seen it. If it were just the problem you alluded to, I wouldn't have called it out. I'm referring to tabs in the middle of new code that uses spaces. This is most likely the result of copy/paste. Either write all the new code to use tabs or match the copy/pasted lines to the surrounding new code (my preference). Nearly all the new code in ipapwd_setkeytab() uses space indents where the surrounding code uses tab indents. I'm not sure the block comment in is_allowed_to_access_attr() belongs there. Also, a utility function like is_allowed_to_access_attr() would probably be handy in upstream 389ds. I might use this in an upcoming token sync patch. This is not a requirement of ACK'ing the patch. 0002 ACK One small nitpick, then I too say ACK. In the commit message, the second sentence doesn't need a line break. I try to keep comments within 72 chars (though sometimes I forget and go past till 80), which is why there is a line break there. Yeah, it just looks bad when sent over email, which is why I noticed it. This didn't get fixed. Add should follow patch. on the same line. 0003 Same as patch 002: unnecessary line breaks in the commit message. See above. Also not fixed. The new set of keys should follow existing ones. on the same line. I think ipapwd_getkeytab() is unnecessarily long. Several sections of it could be broken out into functions and would make it much more readable. That has already been done :-) You should see the original ipa_setkeytab() ... I'm not talking about ipapwd_setkeytab(). I'm talking about ipapwd_getkeytab(). This is entirely new code. There are very clear spots where it could be broken up. I consider this the biggest issue in this set of patches for two important reasons: 1. It makes the function really hard to review.
Re: [Freeipa-devel] [PATCH] 591 webui: add idnsSecInlineSigning option to DNS zone details facet
On 4/30/2014 5:28 AM, Petr Vobornik wrote: Web UI part of pviktori-543 https://fedorahosted.org/freeipa/ticket/3801 ACK. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 631 webui: fix regression: enabled gid field on group add
On 5/14/2014 9:41 AM, Petr Vobornik wrote: GID field should be enabled by default since the default group is posix. Was caused by option_widget_base not properly reporting value change while selecting the default value. It has to be notified with delay otherwise the event is consumed by FieldBinder. https://fedorahosted.org/freeipa/ticket/4325 ACK. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 632 webui: simplify self-service menu
On 5/15/2014 8:58 AM, Petr Vobornik wrote: Just an idea: there is only one top level item in self service menu - no point of having this level. This patch replaces top level with second menu level original: * http://pvoborni.fedorapeople.org/images/self-service-menu-2levels.png new: * http://pvoborni.fedorapeople.org/images/self-service-menu-1level.png ACK. Some unrelated issues: 1. If I recall correctly, a new user is required to change the password upon the initial login. This can be done with kinit, but can this be done via UI too? Right now a new user will get a login error without any message or link to reset the password. 2. When I logged in to the self-service mode, I got the following error: http://edewata.fedorapeople.org/ipa/images/snapshot7.png 3. In the login screen if the page is shorter than a certain height, there will be a blank area appearing at the bottom. The background doesn't extend all the way to the bottom: http://edewata.fedorapeople.org/ipa/images/snapshot8.png -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 645 webui: display only dialogs which belong to current facet
On 5/29/2014 10:15 AM, Petr Vobornik wrote: On 27.5.2014 12:49, Petr Vobornik wrote: Dialog instances no longer directly call IPA.opened_dialog methods. It's handled through events (decoupled from dialog's POV). IPA.open_dialogs with assistance of ApplicationController makes sure that there is only one dialog opened at the same time. It also makes sure to hide all dialogs, which are not global dialogs and did not originate from current facet, when switching facets. https://fedorahosted.org/freeipa/ticket/4348 Attaching rebased version (on top of latest PatternFly patch set). ACK, but see comments below: 1. Let's say I had a dialog open and I clicked the button to execute the operation but the session expired so I'd get a login screen. Once I relogged in, the operation was executed immediately without a chance to review the operation. I think it's safer to require the user to reclick the button (i.e. the UI ignores the user action that happens while the session expired). 2. Can this expression !facet || (facet dialog.facet === facet) be simplified into this? !facet || dialog.facet === facet 3. Unrelated. In self-service user page the action menus for Disable, Delete, and Rebuild auto membership are enabled although the user doesn't have the rights to execute those actions. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 646 webui: handle back button when unauthenticated
On 5/27/2014 5:50 AM, Petr Vobornik wrote: using browser history when unauthenticated causes transition to the original and/or preceding facets. But nothing works since all commands fail due to expired credentials in session. These changes make sure that user stays on login screen if he misses valid session credentials while he wants to switch to facet which requires authentication. https://fedorahosted.org/freeipa/ticket/4353 ACK. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
On Tue, 2014-06-10 at 16:24 -0400, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 14:27 -0400, Nathaniel McCallum wrote: On Tue, 2014-06-10 at 12:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 21:49 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 20:58 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 17:53 -0400, Nathaniel McCallum wrote: On Mon, 2014-06-09 at 15:02 -0400, Simo Sorce wrote: On Mon, 2014-06-09 at 13:39 -0400, Rob Crittenden wrote: Simo Sorce wrote: This patch set is an initial implementation of ticket #3859 It seem to be working fine in my initial testing but I have not yet tested all cases. However I wonted to throw it on the list to get some initial feedback about the choices I made wrt access control and ipa-getkeytab flags and default behavior. In particular, the current patch set would require us to make host/service keytabs readable by the requesting party (whoever that is, admin or host itself) in order to allow it to get back the actual keytab. I am not sure this is ideal. Also write access to the keytab is still all is needed to allow someone to change it. Neither is ideal, but it was simpler as a first implementation. In particular I think we should allow either permission indipendently, and it should be something an admin can change. However I do not like allowing normal writes or reads to these attributes, mostly because w/o access to the master key nobody can really make sense of actually reading out the contents of KrbPrincipalKey or could write a blob that can be successfully decrypted. So I was wondering if we might want to prevent both reading and writing via LDAP (except via extended operations) and instead use another method to determine access patterns. As for ipa-getkeytab is everyone ok with tryin the new method first and always falling back to the old one (if a password has been provided) ? 0001 get_realm_backend() should probably have a comment on why returning NULL is ok (either because there is no sdn or because there is no be). It appears that things will eventually fail in get_entry_by_principal() Instead of adding complex explanations I immediately return with an error if get_realm_backend() returns NULL. The logic here is correct, it just reads awkwardly. It is probably fine as is. There appear to be indiscriminate tab indents throughout the patch. Please changes these to spaces. There are because the coed is old, I do not change the style of a piece of code if we are just changing a few lines. You need to read the patch in the context of the code to seen it. If it were just the problem you alluded to, I wouldn't have called it out. I'm referring to tabs in the middle of new code that uses spaces. This is most likely the result of copy/paste. Either write all the new code to use tabs or match the copy/pasted lines to the surrounding new code (my preference). Nearly all the new code in ipapwd_setkeytab() uses space indents where the surrounding code uses tab indents. I'm not sure the block comment in is_allowed_to_access_attr() belongs there. Also, a utility function like is_allowed_to_access_attr() would probably be handy in upstream 389ds. I might use this in an upcoming token sync patch. This is not a requirement of ACK'ing the patch. 0002 ACK One small nitpick, then I too say ACK. In the commit message, the second sentence doesn't need a line break. I try to keep comments within 72 chars (though sometimes I forget and go past till 80), which is why there is a line break there. Yeah, it just looks bad when sent over email, which is why I noticed it. This didn't get fixed. Add should follow patch. on the same line. 0003 Same as patch 002: unnecessary line breaks in the commit message. See above. Also not fixed. The new set of keys should follow existing ones. on the same line. I think ipapwd_getkeytab() is unnecessarily long. Several sections of it could be broken out into functions and would make it much more readable. That has already been done :-) You should see the original ipa_setkeytab() ... I'm not talking about ipapwd_setkeytab(). I'm talking about ipapwd_getkeytab(). This is entirely new
[Freeipa-devel] Woes updating and oldish devel server to latest master
I ma getting a failure to login in the UI The error is somewhere in ldap/schema/subentry.py KeyError: 'ipattokenhotp' A schema update may have failed I guess ? but running ipa-ldap-updater doesn't help ... Ideas ? 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] faster ways to build/test dogtag?
On Tue, May 27, 2014 at 12:20:46PM +0200, Martin Kosek wrote: On 05/27/2014 09:00 AM, Fraser Tweedale wrote: Hi all, I've been working on a fix for a profile issue (https://fedorahosted.org/freeipa/ticket/2915). Unfortunately I find the scripts/compose_pki_core_packages - yum install - test cycle frustratingly slow on idm.lab.bos. Is there a quicker way to build and test the software - particularly as part of the larger IPA/certmonger/dogtag ecosystem? Cheers, Fraser I am not sure about dogtag - you would need to ask on pki-devel mailing list. As for FreeIPA - I use a home-grown synchronization script that puts modified python file in my local git directly into my test VM system so I do not need to compile and install FreeIPA rpms every time. Martin I'm compiling an etherpad of all the dev scripts/tools people are using in their workflow (for both IPA and Dogtag). Could you please add link to and summary for your synchronisation script? http://idm.etherpad.corp.redhat.com/dev-tools Cheers, Fraser ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
