[Freeipa-devel] Expired passwords cannot be changed via LDAP
On 06/09/2014 08:21 AM, Martin Kosek wrote: On 06/06/2014 05:47 PM, Nathaniel McCallum wrote: On Fri, 2014-06-06 at 11:43 -0400, Simo Sorce wrote: On Fri, 2014-06-06 at 11:06 -0400, Nathaniel McCallum wrote: On Fri, 2014-06-06 at 08:00 -0400, Simo Sorce wrote: On Fri, 2014-06-06 at 10:30 +0200, Martin Kosek wrote: On 05/31/2014 03:27 AM, Simo Sorce wrote: I have rebased theold patch attached to the ticket, unfortunately I haven't had time to test it yet, but didn't want to lose it in some branch. Simo. I tested the patch and it worked fine, code also reads OK. Thus, I am willing to ACK it. I am just wondering if there is any scenario we could have missed, but I did not find any. In there is no push back against the patch I may just push it. The only thing I would draw attention to is the fact that now I am sending back the error directly once we have a negative return from the function in which expiration is checked (ipapwd_authenticate). I could not see why we did, in fact, not do that before and I meant asking Nathaniel if he had an explicit reason why we do not, as he is the last one that did some significant refactoring in the bind preop plugin. In the current design, if ipapwd_authenticate() fails, we defer to 389ds for the actual response to the client. The purpose for this is so that verification of the first factor always behaves the same, regardless of what is in pre-bind. So ipapwd_authenticate() is not actually the final authentication. It is a preliminary authentication to determine if the user should be able to write kerberos keys and perform OTP sync. So checking expiration in pre-bind only protects these two operations. I presume that 389ds also checks password expiration. If this assumption is true, ipapwd_authenticate() SHOULD NOT return any response to the client. It should simply fail key-write/otp-sync silently and let dirsrv return the error to the client. 389ds would check it if we were using 389ds native password policies but we are not. So we need to check on our own, which is what this patch implements. The important point here is that so long as 389ds is verifying password expiration, using a correct-but-expired password will should not result in a bind in the current code. It will result in kerberos key writing and OTP sync. Do we actually care about protecting kerberos key writing and OTP sync from correct-but-expired passwords? No, but that's not the point. If 389ds does not check password expiration, then we probably need to patch upstream 389ds or, at the very least, return an error to the client. It is not a 389ds bug, it is just an integration issue due to the fact IPA uses a different schema for password policies (for compatibility with the Kerberos schema). Otherwise, if we don't care about protecting kerberos key writing and OTP sync from correct-but-expired passwords, this patch is entirely unnecessary. Otherwise, the patch is necessary, but should skip kerberos key writing and OTP sync and fall through silently to 389ds. If we fall through to 389ds then authentication will succeed. So from this discussion it seem to me we achieve the goal of the patch and returning an error directly is ok. Unless Nathaniel has something *against* returning an error in this place I think we are good to go. Looks good to me. ACK. Nathaniel Ok, thanks for discussion and double checking! Pushed to master: bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6 Martin I just read a question in the ticket from alonbl about how password change should work on the LDAP level: https://fedorahosted.org/freeipa/ticket/1539#comment:29 Are we ready to say that from now on, expired passwords cannot be changed via LDAP? I.e. this workflow will no longer work: $ ipa user-add --first=Foo --last Bar expired --random Added user expired User login: expired First name: Foo Last name: Bar Full name: Foo Bar Display name: Foo Bar Initials: FB Home directory: /home/expired GECOS: Foo Bar Login shell: /bin/sh Kerberos principal: expi...@mkosek-fedora20.test Email address: expi...@mkosek-fedora20.test Random password: qiCjofo.2pfT UID: 1170600026 GID: 1170600026 Password: True Member of groups: ipausers Kerberos keys available: True $ ldappasswd -h `hostname` -D uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -x -w qiCjofo.2pfT uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -a qiCjofo.2pfT -s Secret123 ldap_bind: Invalid credentials (49) additional info: The user password is expired Thanks, Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Patch for #1539
On 06/09/2014 08:21 AM, Martin Kosek wrote:
On 06/06/2014 05:47 PM, Nathaniel McCallum wrote:
On Fri, 2014-06-06 at 11:43 -0400, Simo Sorce wrote:
On Fri, 2014-06-06 at 11:06 -0400, Nathaniel McCallum wrote:
On Fri, 2014-06-06 at 08:00 -0400, Simo Sorce wrote:
On Fri, 2014-06-06 at 10:30 +0200, Martin Kosek wrote:
On 05/31/2014 03:27 AM, Simo Sorce wrote:
I have rebased theold patch attached to the ticket, unfortunately I
haven't had time to test it yet, but didn't want to lose it in some
branch.
Simo.
I tested the patch and it worked fine, code also reads OK. Thus, I am willing
to ACK it.
I am just wondering if there is any scenario we could have missed, but I did
not find any. In there is no push back against the patch I may just push it.
The only thing I would draw attention to is the fact that now I am
sending back the error directly once we have a negative return from the
function in which expiration is checked (ipapwd_authenticate).
I could not see why we did, in fact, not do that before and I meant
asking Nathaniel if he had an explicit reason why we do not, as he is
the last one that did some significant refactoring in the bind preop
plugin.
In the current design, if ipapwd_authenticate() fails, we defer to 389ds
for the actual response to the client. The purpose for this is so that
verification of the first factor always behaves the same, regardless of
what is in pre-bind.
So ipapwd_authenticate() is not actually the final authentication. It
is a preliminary authentication to determine if the user should be able
to write kerberos keys and perform OTP sync. So checking expiration in
pre-bind only protects these two operations.
I presume that 389ds also checks password expiration. If this assumption
is true, ipapwd_authenticate() SHOULD NOT return any response to the
client. It should simply fail key-write/otp-sync silently and let dirsrv
return the error to the client.
389ds would check it if we were using 389ds native password policies but
we are not. So we need to check on our own, which is what this patch
implements.
The important point here is that so long as 389ds is verifying password
expiration, using a correct-but-expired password will should not result
in a bind in the current code. It will result in kerberos key writing
and OTP sync. Do we actually care about protecting kerberos key writing
and OTP sync from correct-but-expired passwords?
No, but that's not the point.
If 389ds does not check password expiration, then we probably need to
patch upstream 389ds or, at the very least, return an error to the
client.
It is not a 389ds bug, it is just an integration issue due to the fact
IPA uses a different schema for password policies (for compatibility
with the Kerberos schema).
Otherwise, if we don't care about protecting kerberos key writing and
OTP sync from correct-but-expired passwords, this patch is entirely
unnecessary.
Otherwise, the patch is necessary, but should skip kerberos key writing
and OTP sync and fall through silently to 389ds.
If we fall through to 389ds then authentication will succeed.
So from this discussion it seem to me we achieve the goal of the patch
and returning an error directly is ok.
Unless Nathaniel has something *against* returning an error in this
place I think we are good to go.
Looks good to me. ACK.
Nathaniel
Ok, thanks for discussion and double checking!
Pushed to master: bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6
Hello,
This patch broke some of our tests.
ipatests.test_ipaserver.test_changepw:test_changepw.test_invalid_auth
ipatests.test_xmlrpc.test_user_plugin:test_denied_bind_with_expired_principal.test_1_bind_as_test_user
ipatests.test_xmlrpc.test_user_plugin:test_denied_bind_with_expired_principal.test_3_bind_as_renewed_test_user
fail with: INVALID_CREDENTIALS: {'info': 'The user password is expired',
'desc': 'Invalid credentials'}
ipatests.test_ipaserver.test_changepw:test_changepw.test_pwpolicy_error
fails with: AssertionError: 'invalid-password' != 'policy-error'
ipatests.test_ipaserver.test_changepw:test_changepw.test_pwpolicy_success
fails with: AssertionError: 'invalid-password' != 'ok'
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP
On 06/09/2014 10:59 AM, Martin Kosek wrote: On 06/09/2014 08:21 AM, Martin Kosek wrote: On 06/06/2014 05:47 PM, Nathaniel McCallum wrote: On Fri, 2014-06-06 at 11:43 -0400, Simo Sorce wrote: On Fri, 2014-06-06 at 11:06 -0400, Nathaniel McCallum wrote: On Fri, 2014-06-06 at 08:00 -0400, Simo Sorce wrote: On Fri, 2014-06-06 at 10:30 +0200, Martin Kosek wrote: On 05/31/2014 03:27 AM, Simo Sorce wrote: I have rebased theold patch attached to the ticket, unfortunately I haven't had time to test it yet, but didn't want to lose it in some branch. Simo. I tested the patch and it worked fine, code also reads OK. Thus, I am willing to ACK it. I am just wondering if there is any scenario we could have missed, but I did not find any. In there is no push back against the patch I may just push it. The only thing I would draw attention to is the fact that now I am sending back the error directly once we have a negative return from the function in which expiration is checked (ipapwd_authenticate). I could not see why we did, in fact, not do that before and I meant asking Nathaniel if he had an explicit reason why we do not, as he is the last one that did some significant refactoring in the bind preop plugin. In the current design, if ipapwd_authenticate() fails, we defer to 389ds for the actual response to the client. The purpose for this is so that verification of the first factor always behaves the same, regardless of what is in pre-bind. So ipapwd_authenticate() is not actually the final authentication. It is a preliminary authentication to determine if the user should be able to write kerberos keys and perform OTP sync. So checking expiration in pre-bind only protects these two operations. I presume that 389ds also checks password expiration. If this assumption is true, ipapwd_authenticate() SHOULD NOT return any response to the client. It should simply fail key-write/otp-sync silently and let dirsrv return the error to the client. 389ds would check it if we were using 389ds native password policies but we are not. So we need to check on our own, which is what this patch implements. The important point here is that so long as 389ds is verifying password expiration, using a correct-but-expired password will should not result in a bind in the current code. It will result in kerberos key writing and OTP sync. Do we actually care about protecting kerberos key writing and OTP sync from correct-but-expired passwords? No, but that's not the point. If 389ds does not check password expiration, then we probably need to patch upstream 389ds or, at the very least, return an error to the client. It is not a 389ds bug, it is just an integration issue due to the fact IPA uses a different schema for password policies (for compatibility with the Kerberos schema). Otherwise, if we don't care about protecting kerberos key writing and OTP sync from correct-but-expired passwords, this patch is entirely unnecessary. Otherwise, the patch is necessary, but should skip kerberos key writing and OTP sync and fall through silently to 389ds. If we fall through to 389ds then authentication will succeed. So from this discussion it seem to me we achieve the goal of the patch and returning an error directly is ok. Unless Nathaniel has something *against* returning an error in this place I think we are good to go. Looks good to me. ACK. Nathaniel Ok, thanks for discussion and double checking! Pushed to master: bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6 Martin I just read a question in the ticket from alonbl about how password change should work on the LDAP level: https://fedorahosted.org/freeipa/ticket/1539#comment:29 Are we ready to say that from now on, expired passwords cannot be changed via LDAP? I.e. this workflow will no longer work: $ ipa user-add --first=Foo --last Bar expired --random Added user expired User login: expired First name: Foo Last name: Bar Full name: Foo Bar Display name: Foo Bar Initials: FB Home directory: /home/expired GECOS: Foo Bar Login shell: /bin/sh Kerberos principal: expi...@mkosek-fedora20.test Email address: expi...@mkosek-fedora20.test Random password: qiCjofo.2pfT UID: 1170600026 GID: 1170600026 Password: True Member of groups: ipausers Kerberos keys available: True $ ldappasswd -h `hostname` -D uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -x -w qiCjofo.2pfT uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -a qiCjofo.2pfT -s Secret123 ldap_bind: Invalid credentials (49) additional info: The user password is expired Thanks, Martin As noted in other part of this thread, this also breaks password changes via Web UI as the hooks use LDAP (and not kadmin) to update passwords. Given all sort of issues we get, I am thinking we should just revert it unless there is a quick fix
Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP
- Original Message - From: Martin Kosek mko...@redhat.com To: Nathaniel McCallum npmccal...@redhat.com, Simo Sorce s...@redhat.com Cc: freeipa-devel freeipa-devel@redhat.com Sent: Monday, June 9, 2014 1:11:17 PM Subject: Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP On 06/09/2014 10:59 AM, Martin Kosek wrote: On 06/09/2014 08:21 AM, Martin Kosek wrote: On 06/06/2014 05:47 PM, Nathaniel McCallum wrote: On Fri, 2014-06-06 at 11:43 -0400, Simo Sorce wrote: On Fri, 2014-06-06 at 11:06 -0400, Nathaniel McCallum wrote: On Fri, 2014-06-06 at 08:00 -0400, Simo Sorce wrote: On Fri, 2014-06-06 at 10:30 +0200, Martin Kosek wrote: On 05/31/2014 03:27 AM, Simo Sorce wrote: I have rebased theold patch attached to the ticket, unfortunately I haven't had time to test it yet, but didn't want to lose it in some branch. Simo. I tested the patch and it worked fine, code also reads OK. Thus, I am willing to ACK it. I am just wondering if there is any scenario we could have missed, but I did not find any. In there is no push back against the patch I may just push it. The only thing I would draw attention to is the fact that now I am sending back the error directly once we have a negative return from the function in which expiration is checked (ipapwd_authenticate). I could not see why we did, in fact, not do that before and I meant asking Nathaniel if he had an explicit reason why we do not, as he is the last one that did some significant refactoring in the bind preop plugin. In the current design, if ipapwd_authenticate() fails, we defer to 389ds for the actual response to the client. The purpose for this is so that verification of the first factor always behaves the same, regardless of what is in pre-bind. So ipapwd_authenticate() is not actually the final authentication. It is a preliminary authentication to determine if the user should be able to write kerberos keys and perform OTP sync. So checking expiration in pre-bind only protects these two operations. I presume that 389ds also checks password expiration. If this assumption is true, ipapwd_authenticate() SHOULD NOT return any response to the client. It should simply fail key-write/otp-sync silently and let dirsrv return the error to the client. 389ds would check it if we were using 389ds native password policies but we are not. So we need to check on our own, which is what this patch implements. The important point here is that so long as 389ds is verifying password expiration, using a correct-but-expired password will should not result in a bind in the current code. It will result in kerberos key writing and OTP sync. Do we actually care about protecting kerberos key writing and OTP sync from correct-but-expired passwords? No, but that's not the point. If 389ds does not check password expiration, then we probably need to patch upstream 389ds or, at the very least, return an error to the client. It is not a 389ds bug, it is just an integration issue due to the fact IPA uses a different schema for password policies (for compatibility with the Kerberos schema). Otherwise, if we don't care about protecting kerberos key writing and OTP sync from correct-but-expired passwords, this patch is entirely unnecessary. Otherwise, the patch is necessary, but should skip kerberos key writing and OTP sync and fall through silently to 389ds. If we fall through to 389ds then authentication will succeed. So from this discussion it seem to me we achieve the goal of the patch and returning an error directly is ok. Unless Nathaniel has something *against* returning an error in this place I think we are good to go. Looks good to me. ACK. Nathaniel Ok, thanks for discussion and double checking! Pushed to master: bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6 Martin I just read a question in the ticket from alonbl about how password change should work on the LDAP level: https://fedorahosted.org/freeipa/ticket/1539#comment:29 Are we ready to say that from now on, expired passwords cannot be changed via LDAP? I.e. this workflow will no longer work: $ ipa user-add --first=Foo --last Bar expired --random Added user expired User login: expired First name: Foo Last name: Bar Full name: Foo Bar Display name: Foo Bar Initials: FB Home directory: /home/expired GECOS: Foo Bar Login shell: /bin/sh Kerberos principal: expi...@mkosek-fedora20.test Email address: expi...@mkosek-fedora20.test Random password: qiCjofo.2pfT UID: 1170600026 GID: 1170600026 Password: True Member of groups: ipausers Kerberos keys available: True $ ldappasswd -h `hostname` -D uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -x -w qiCjofo.2pfT
Re: [Freeipa-devel] [PATCHES] 0568-0570 Convert User default permissions to managed
On 06/06/2014 11:38 AM, Martin Kosek wrote: On 06/04/2014 06:43 PM, Petr Viktorin wrote: Hello, I try to think about any kind of data the user might have in LDAP, but in the spirit of YAGNI, I'll deal with the various corner cases in IPA's historic default permissions as I go along. Patch 0568 adds support for the case where the default permissions changed in something else than attribute lists. Needed for the 'Change User password' permission. Patch 0569 converts user permissions to managed. Patch 0570 fixes https://fedorahosted.org/freeipa/ticket/3697 1) Add aci has targetfilter part - is that intentional? Yes. From the permission plugin''s point of view, it's part of the definition of --type user (i.e. this applies to users). Regardless I think it should be there. # ipa permission-show 'System: Add Users' --all --raw ... aci: (targetfilter = (objectclass=posixaccount))(version 3.0;acl permission:System: Add Users;allow (add) groupdn = ldap:///cn=System: Add Users,cn=permissions,cn=pbac,dc=mkosek-fedora20,dc=test;) This part IS effective though, so it may not be a bad thing at all, to keep it in the ACI: # ldapadd -Y GSSAPI SASL/GSSAPI authentication started SASL username: f...@mkosek-fedora20.test SASL SSF: 56 SASL data security layer installed. dn: cn=foo,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test objectclass: top objectclass: nscontainer cn: foo adding new entry cn=foo,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test ldap_add: Insufficient access (50) additional info: Insufficient 'add' privilege to add the entry 'cn=foo,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test'. # ipa user-add --first=Foo --last Bar fbar2 -- Added user fbar2 -- User login: fbar2 First name: Foo ... 2) System: Add User to default group I was wondering whether we should keep the AluCI in cn=groups container or directly with the group, but I think the group itself is a good idea. (Unless someone deletes and recreates it). Hm, this is a good point. If the ipausers group is deleted, there'll be an permission with a missing ACI that can't be created. That could be quite annoying. I put the ACI it in the container. 3) System: Change User password I hit some nasty DS error which prevented authorized user to update password. ACI log attached. Ludwig, does that ring any bell? The ACI itself looks OK though as after I restarted DS, it started to work. Maybe DS did not cache the ACIs properly after upgrade? Which DS version are you using? 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? b) Read membership ACIs for users and groups miss member attribute and thus indirect/direct processing goes wrong. Added. -- Petr³ From cbf9d15a6c0a3bb02d3c84594e8f299dd40f831e 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 if it is in *all* historic ACIs but not in LDAP. +Rationale: When we don't know which version of an ACI the user is +upgrading from, we only consider attributes where all the versions +agree. For other attrs we'll use the default from the new managed perm. If the ACIs differ in something else than the list of attributes, raise IncompatibleACIModification. This means manual action is needed (either delete the old permission or change it to resemble the default -again, then re-run ipa-ldap-updater) +again, then re-run ipa-ldap-updater). + +In case there are multiple historic default ACIs, and
Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP
On 06/09/2014 12:15 PM, Alon Bar-Lev 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. The fix should be for the password modify to work within anonymous bind if old password is specified. I am not sure why IPA enforces non anonymous bind for this extended request. Applications should also be modified to perform anonymous bind, exactly per this reason. Searching why IPA requires non anonymous bind is what led me to this bug... :) Simo, do you know the historical reason why this is enforced in ipapwd_chpwop? When we started we wanted to allow password changes using GSSAPI for bind instead of password based authentication, and we ended up not implementing the old-password based one at all... By quickly looking at the code it should not be difficult to fix, but devil is in details and we need to be very cautious in this function. We just need to be careful about what operations are done, but indeed it shouldn't be difficult, I am just not sure it is quick enough for you. I can take a look in a few. Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Patch for #1539
Pushed to master: bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6
Hello,
This patch broke some of our tests.
ipatests.test_ipaserver.test_changepw:test_changepw.test_invalid_auth
ipatests.test_xmlrpc.test_user_plugin:test_denied_bind_with_expired_principal.test_1_bind_as_test_user
ipatests.test_xmlrpc.test_user_plugin:test_denied_bind_with_expired_principal.test_3_bind_as_renewed_test_user
fail with: INVALID_CREDENTIALS: {'info': 'The user password is expired',
'desc': 'Invalid credentials'}
ipatests.test_ipaserver.test_changepw:test_changepw.test_pwpolicy_error
fails with: AssertionError: 'invalid-password' != 'policy-error'
ipatests.test_ipaserver.test_changepw:test_changepw.test_pwpolicy_success
fails with: AssertionError: 'invalid-password' != 'ok'
I guess we need to fix some of these tests, but first we need to fix password
change with expired credentials to work when the old password is provided.
This will not work with non-password based mechanisms though ...
Simo.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP
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? Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP
On 06/09/2014 09:01 AM, 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? Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel I am also concerned about the OTP flows with this change. IMO we might not be ready for this change one way or another. Backing out or adding a default switch turning the feature off works for me. -- Thank you, Dmitri Pal Sr. Engineering Manager IdM portfolio Red Hat, Inc. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP
On 06/09/2014 03:08 PM, Dmitri Pal wrote: On 06/09/2014 09:01 AM, 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? Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel I am also concerned about the OTP flows with this change. IMO we might not be ready for this change one way or another. Backing out or adding a default switch turning the feature off works for me. I do not like the proposal very much. It sounds like oops, this breaks FreeIPA, let's hide it with configuration option and fix later. This would not be a simple fix, we know that Web UI and possibly other workflows are broken unless we introduce password changes via anonymous binds (and thus utilize oldPassword piece). We would also need to make sure the setting is not read with every LDAP bind, otherwise it would also have some performance impact, our BINDs are already slow (see https://fedorahosted.org/freeipa/ticket/3892). If this can be indeed fixed, let us do it before 4.0 Beta (we are talking about 2 weeks of time in ideal scenario) or revert until we are ready. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 592-628 Update to PatternFly
On 6.6.2014 20:35, Endi Sukma Dewata wrote:
On 6/6/2014 10:43 AM, Petr Vobornik wrote:
On 6.6.2014 15:45, Endi Sukma Dewata wrote:
On 6/5/2014 9:25 AM, Endi Sukma Dewata wrote:
ACK for patches #592-#628. I'll continue reviewing the rest.
ACK for patches #633-639, #642, #644, #652, and #653. Patches #640
#641 have an issue (see #19 below) that should be fixed before pushing.
Other issues are minor/unrelated/suggestions that can be addressed
separately.
Thank you for the review.
I've fixed issues:
- #19, #20 in patch #640.
And some low hanging fruit:
- #16, in patch #637
- #17, in patch #612
- #13, in patch #612
The branch has been rebased to current master.
ACK.
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.
I am not able to reproduce issues #4 and #11.
4. In the list page (e.g. Users) in mobile mode the Refresh button
may overlap the search box.
Here's what I saw as I was adjusting the page width:
http://edewata.fedorapeople.org/ipa/images/snapshot1.png
http://edewata.fedorapeople.org/ipa/images/snapshot2.png
http://edewata.fedorapeople.org/ipa/images/snapshot3.png
Notice that in snapshot #2 the search box is partially covered by the
Refresh button.
11. In desktop mode the QR code for new OTP token is displayed
outside the dialog box.
Here's what I saw:
http://edewata.fedorapeople.org/ipa/images/snapshot4.png
Fixed, both were Firefox-only issues.
18. In the New Certificate dialog for Host, the instruction to create a
CSR exceeds the dialog boundary.
caused by BS' CSS:
code {
white-space: nowrap;
}
I wonder if the best solution is to reset it to initial value in all
dialogs.
Alternatively, the sample command could be broken into two lines.
IMHO the style is better, because it's more general and easier for
copypaste. I've also added word-wrap to break very long subjects.
20. The capitalization of Certificate is inconsistent in Host's and
Service's Actions.
Fixed
The View certificate is still inconsistent though.
Shame on me :)
--
Petr Vobornik
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 21-22] ipautil log messages and API version to env
On 06/06/2014 05:02 PM, Gabe Alford wrote: Patch 21: Update per recommendation Patch 22: Added version option as well as updated the manpage. Thanks, Gabe Great! Thanks, ACK, pushed to master: 2a8c509567754877ed0188784d7c38250484be48 In the future, you can put typo fixes in a separate patch, so it's easier to backport/revert individual changes. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Reorganization of Web UI navigation items
On Mon, 2014-06-09 at 16:08 +0200, Petr Vobornik wrote: Accounts/Identity (7): - Users - Groups - Hosts - Host Groups - Netgroups - Services - Automember ^ These are all identity or identity-grouping related objects/actions +1 What are the chances that we will add some other identity to manage in a future? I am not foreseeing anything in the core, but we can move Automember under configuration is we want to. Directory (6): - Permissions - Privileges - Roles - Delegation NOTE: the 4 above can be merged into a single 'Authorization' entry perhaps May be it should be and Administration tab, I do not like the title. I understand where the directory comes from but this is IMo not intuitive for someone who does not know what is under the hood. - Replication Topology +1 that they should be together. They configure the tool and not data. Current IPA Server item name may be more suitable. Well this is not related to just the one server, but the whole set of servers. Maybe the plural IPA Servers ? - Views (future) ^ Everything that deals with direct LDAP access/view I think views do not belong here. They belong in the same place where the trusts are. Just a FYI: I do not think views and trust should be in the same place. Views will also be available for regular IPA server with no trusts, the 2 are not strictly related. Views IMO really belong here with other directory configuration items. Network Services (4): - Automount - DNS - CA - Vault (future) - Radius Server Proxies Isn't this strictly related to OTP ? I would put it in the same place. ^ All the additional network services or configuration of network related services +1 Configuration (3): - Trusts - ID Ranges - Realm Domains - Global - OTP Tokens ? ^ Anything that does not fit the above categories. +1 Docs: - whatever :) (*) The only doubt I have is about OTP Tokens, it may be worth taking them off Policies and putting them into a new tab which in future may also sport a pointer to user certificates management: Yeah, may be for now we put OTP as a top level for now and have tokens and create a RADIUS page to manage radius proxies? We already have RADIUS Servers menu item for Radius s. proxies. Martin forgot it in his proposal. In future when we add other credentials we can rename it and add smart card related options. Authentication: - OTP Tokens - User Certificates (future) With Documentation, Authentication would be the 7th top level item. Ideal number of top level items is about 5-6. Because we have to fit into 768px (minimum screen size before it's switched to compact menu). Why the minimum is 768 ? Maybe we can drop Documentation from the top level ? Or make it really small by using a ? as the menu symbol ? :) Maybe we should stop using full names but instead get a set of icons that represent each item and have the name only as a tooltip ? This way the first level menu bar sizing would be consistent regardless of the language. This functionality is provided by PatternFly. Also take into considerations that languages such as Spanish have much longer expressions. Yeah maybe we should just avoid names here and use icons+tooltips/hover instead. -- 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] Expired passwords cannot be changed via LDAP
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 -- Thank you, Dmitri Pal Sr. Engineering Manager IdM portfolio Red Hat, Inc. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Reorganization of Web UI navigation items
On 9.6.2014 16:42, Simo Sorce wrote: On Mon, 2014-06-09 at 16:08 +0200, Petr Vobornik wrote: Accounts/Identity (7): - Users - Groups - Hosts - Host Groups - Netgroups - Services - Automember ^ These are all identity or identity-grouping related objects/actions +1 What are the chances that we will add some other identity to manage in a future? I am not foreseeing anything in the core, but we can move Automember under configuration is we want to. Directory (6): - Permissions - Privileges - Roles - Delegation NOTE: the 4 above can be merged into a single 'Authorization' entry perhaps May be it should be and Administration tab, I do not like the title. I understand where the directory comes from but this is IMo not intuitive for someone who does not know what is under the hood. - Replication Topology +1 that they should be together. They configure the tool and not data. Current IPA Server item name may be more suitable. Well this is not related to just the one server, but the whole set of servers. Maybe the plural IPA Servers ? - Views (future) ^ Everything that deals with direct LDAP access/view I think views do not belong here. They belong in the same place where the trusts are. Just a FYI: I do not think views and trust should be in the same place. Views will also be available for regular IPA server with no trusts, the 2 are not strictly related. Views IMO really belong here with other directory configuration items. Network Services (4): - Automount - DNS - CA - Vault (future) - Radius Server Proxies Isn't this strictly related to OTP ? I would put it in the same place. ^ All the additional network services or configuration of network related services +1 Configuration (3): - Trusts - ID Ranges - Realm Domains - Global - OTP Tokens ? ^ Anything that does not fit the above categories. +1 Docs: - whatever :) (*) The only doubt I have is about OTP Tokens, it may be worth taking them off Policies and putting them into a new tab which in future may also sport a pointer to user certificates management: Yeah, may be for now we put OTP as a top level for now and have tokens and create a RADIUS page to manage radius proxies? We already have RADIUS Servers menu item for Radius s. proxies. Martin forgot it in his proposal. In future when we add other credentials we can rename it and add smart card related options. Authentication: - OTP Tokens - User Certificates (future) With Documentation, Authentication would be the 7th top level item. Ideal number of top level items is about 5-6. Because we have to fit into 768px (minimum screen size before it's switched to compact menu). Why the minimum is 768 ? It's Bootstrap's minimum width of a small device(tablet). Navbar's collapse threshold (@grid-float-breakpoint) is set to this value by default. It's possible to increase it but I don't think it's the best approach - collapsed menu is harder to use. It can be solved in different manner but it requires additional work. Maybe we can drop Documentation from the top level ? Or make it really small by using a ? as the menu symbol ? :) I like this. Maybe we should stop using full names but instead get a set of icons that represent each item and have the name only as a tooltip ? This way the first level menu bar sizing would be consistent regardless of the language. It would solve the issue, but we should be consistent with other projects as well. Also, it would require very good icons. I'm afraid that it would be harder to use for newcomers. But probably better for experienced users. Kyle what's your take? This functionality is provided by PatternFly. Also take into considerations that languages such as Spanish have much longer expressions. Yeah maybe we should just avoid names here and use icons+tooltips/hover instead. -- 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
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 Needs a rebase. I did my best for testing. In check_service_name() why not just initialize name to NULL rather than assigning it twice? Or is the value of name undefined if krb5_* fails? 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() I think the Access Strategy comment needs to be expanded (or dropped) in is_allowed_to_access_keys() In the fatal error in is_allowed_to_access_keys() should the error include the bound user and the resource they attempted to modify? 0002 ACK 0003 Nit: typo in commit, extedned I'd prefer unique error messages for each ber decode failure. Why is write access always required in ipapwd_getkeytab()? Would there be a case where a principal can only re-fetch existing keys? Since getnew is defined as a boolean in the ASN.1, is the conditional if (getnew == 0) preferred over just if (getnew)? Some errors read Out of Memory and others allocation failed. I think with a fatal error the line number is included in the error. If I'm wrong then perhaps something specific to the allocation should be included. The error message to the call ber_decode_krb5_key_data() doesn't seem to match. 0004 More duplicated error messages, e.g. Missing reply control 0005 More duplicated error messages, Failed to parse key data Typo, Incopatible Functionality-wise. Retrieving a keytab from a service without one fails in a strange way: # ipa-getkeytab -r -s `hostname` -p test1/`hostname` -k /tmp/test.kt Operation failed! Insufficient access rights Failed to get keytab I suppose a new permission is needed to add the ability to re-fetch keytabs. Should the admins group be able to do this out-of-the box? Maybe this is ok, I don't know, but it looks wierd. I fetched with -r the same keytab a number of times and this is what it contains: # klist -kt /tmp/test.kt Keytab name: FILE:/tmp/test.kt KVNO Timestamp Principal --- -- 1 06/09/2014 13:35:50 test1/grindle.example@example.com 1 06/09/2014 13:35:50 test1/grindle.example@example.com 1 06/09/2014 13:35:50 test1/grindle.example@example.com 1 06/09/2014 13:35:50 test1/grindle.example@example.com 1 06/09/2014 13:35:53 test1/grindle.example@example.com 1 06/09/2014 13:35:53 test1/grindle.example@example.com 1 06/09/2014 13:35:53 test1/grindle.example@example.com 1 06/09/2014 13:35:53 test1/grindle.example@example.com 1 06/09/2014 13:36:00 test1/grindle.example@example.com 1 06/09/2014 13:36:00 test1/grindle.example@example.com 1 06/09/2014 13:36:00 test1/grindle.example@example.com 1 06/09/2014 13:36:00 test1/grindle.example@example.com 1 06/09/2014 13:36:45 test1/grindle.example@example.com 1 06/09/2014 13:36:45 test1/grindle.example@example.com 1 06/09/2014 13:36:45 test1/grindle.example@example.com 1 06/09/2014 13:36:45 test1/grindle.example@example.com 1 06/09/2014 13:36:51 test1/grindle.example@example.com 1 06/09/2014 13:36:51 test1/grindle.example@example.com 1 06/09/2014 13:36:51 test1/grindle.example@example.com The krbPrincipalKey value remained constant. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Reorganization of Web UI navigation items
- Original Message - On 9.6.2014 16:42, Simo Sorce wrote: On Mon, 2014-06-09 at 16:08 +0200, Petr Vobornik wrote: Accounts/Identity (7): - Users - Groups - Hosts - Host Groups - Netgroups - Services - Automember ^ These are all identity or identity-grouping related objects/actions +1 What are the chances that we will add some other identity to manage in a future? I am not foreseeing anything in the core, but we can move Automember under configuration is we want to. Directory (6): - Permissions - Privileges - Roles - Delegation NOTE: the 4 above can be merged into a single 'Authorization' entry perhaps May be it should be and Administration tab, I do not like the title. I understand where the directory comes from but this is IMo not intuitive for someone who does not know what is under the hood. - Replication Topology +1 that they should be together. They configure the tool and not data. Current IPA Server item name may be more suitable. Well this is not related to just the one server, but the whole set of servers. Maybe the plural IPA Servers ? - Views (future) ^ Everything that deals with direct LDAP access/view I think views do not belong here. They belong in the same place where the trusts are. Just a FYI: I do not think views and trust should be in the same place. Views will also be available for regular IPA server with no trusts, the 2 are not strictly related. Views IMO really belong here with other directory configuration items. Network Services (4): - Automount - DNS - CA - Vault (future) - Radius Server Proxies Isn't this strictly related to OTP ? I would put it in the same place. ^ All the additional network services or configuration of network related services +1 Configuration (3): - Trusts - ID Ranges - Realm Domains - Global - OTP Tokens ? ^ Anything that does not fit the above categories. +1 Docs: - whatever :) (*) The only doubt I have is about OTP Tokens, it may be worth taking them off Policies and putting them into a new tab which in future may also sport a pointer to user certificates management: Yeah, may be for now we put OTP as a top level for now and have tokens and create a RADIUS page to manage radius proxies? We already have RADIUS Servers menu item for Radius s. proxies. Martin forgot it in his proposal. In future when we add other credentials we can rename it and add smart card related options. Authentication: - OTP Tokens - User Certificates (future) With Documentation, Authentication would be the 7th top level item. Ideal number of top level items is about 5-6. Because we have to fit into 768px (minimum screen size before it's switched to compact menu). Why the minimum is 768 ? It's Bootstrap's minimum width of a small device(tablet). Navbar's collapse threshold (@grid-float-breakpoint) is set to this value by default. It's possible to increase it but I don't think it's the best approach - collapsed menu is harder to use. It can be solved in different manner but it requires additional work. Maybe we can drop Documentation from the top level ? Or make it really small by using a ? as the menu symbol ? :) I like this. Maybe we should stop using full names but instead get a set of icons that represent each item and have the name only as a tooltip ? This way the first level menu bar sizing would be consistent regardless of the language. It would solve the issue, but we should be consistent with other projects as well. Also, it would require very good icons. I'm afraid that it would be harder to use for newcomers. But probably better for experienced users. Kyle what's your take? Icons which represent anything outside of common actions prove to be difficult to recognize for new or experienced users - depending on the amount. I think this concern would better be served by collapsing the top level to less options. Things related to administration of the tool like documentation could live on the top right near the login. This should be treated differently as it is not a currency a tool manages, but an app utility. It is okay if the top level collapses at the 768 width. This is the desired functionality for tablet size. Generally if we have no more than 7 words at the top level we should be fine. This functionality is provided by PatternFly. Also take into considerations that languages such as Spanish have much longer expressions. Yeah maybe we should just avoid names here and use icons+tooltips/hover instead. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0576 Split long docstrings that were recently modified
Hello, Some big plugin module docstrings were modified. This means translators will need to re-translate the whole string. To avoid the extra work in the future, I've split the strings we changed since the last time ipa.pot was regenerated. See: http://www.freeipa.org/page/Coding_Best_Practices#Split_long_translatable_strings Once this is accepted, I'll update Transifex to let people contribute translations for 4.0. -- Petr³ From 1c71f5c3b21e25344a621f165c613ab930137015 Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Mon, 9 Jun 2014 20:10:19 +0200 Subject: [PATCH] Split long docstrings that were recently modified When the strings are changed again, translators will only need to re-translate the modified parts. See: https://fedorahosted.org/freeipa/ticket/3587 --- ipalib/plugins/automember.py | 50 +-- ipalib/plugins/otptoken.py| 14 ++-- ipalib/plugins/radiusproxy.py | 16 +++--- ipalib/plugins/sudorule.py| 24 ++--- 4 files changed, 52 insertions(+), 52 deletions(-) diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py index 143c6a80c4544ba19652db91e24a5be97d7c8714..ddcd5210c078e8146e7bb0f78ef11c144fa7665c 100644 --- a/ipalib/plugins/automember.py +++ b/ipalib/plugins/automember.py @@ -29,48 +29,48 @@ __doc__ = _( Auto Membership Rule. - +) + _( Bring clarity to the membership of hosts and users by configuring inclusive or exclusive regex patterns, you can automatically assign a new entries into a group or hostgroup based upon attribute information. - +) + _( A rule is directly associated with a group by name, so you cannot create a rule without an accompanying group or hostgroup. - +) + _( A condition is a regular expression used by 389-ds to match a new incoming entry with an automember rule. If it matches an inclusive rule then the entry is added to the appropriate group or hostgroup. - +) + _( A default group or hostgroup could be specified for entries that do not match any rule. In case of user entries this group will be a fallback group because all users are by default members of group specified in IPA config. - +) + _( The automember-rebuild command can be used to retroactively run automember rules against existing entries, thus rebuilding their membership. - +) + _( EXAMPLES: - +) + _( Add the initial group or hostgroup: ipa hostgroup-add --desc=Web Servers webservers ipa group-add --desc=Developers devel - +) + _( Add the initial rule: ipa automember-add --type=hostgroup webservers ipa automember-add --type=group devel - +) + _( Add a condition to the rule: ipa automember-add-condition --key=fqdn --type=hostgroup --inclusive-regex=^web[1-9]+\.example\.com webservers ipa automember-add-condition --key=manager --type=group --inclusive-regex=^uid=mscott devel - +) + _( Add an exclusive condition to the rule to prevent auto assignment: ipa automember-add-condition --key=fqdn --type=hostgroup --exclusive-regex=^web5\.example\.com webservers - +) + _( Add a host: ipa host-add web1.example.com - +) + _( Add a user: ipa user-add --first=Tim --last=User --password tuser1 --manager=mscott - +) + _( Verify automembership: ipa hostgroup-show webservers Host-group: webservers @@ -82,45 +82,45 @@ Description: Developers GID: 100420 Member users: tuser - +) + _( Remove a condition from the rule: ipa automember-remove-condition --key=fqdn --type=hostgroup --inclusive-regex=^web[1-9]+\.example\.com webservers - +) + _( Modify the automember rule: ipa automember-mod - +) + _( Set the default (fallback) target group: ipa automember-default-group-set --default-group=webservers --type=hostgroup ipa automember-default-group-set --default-group=ipausers --type=group - +) + _( Remove the default (fallback) target group: ipa automember-default-group-remove --type=hostgroup ipa automember-default-group-remove --type=group - +) + _( Show the default (fallback) target group: ipa automember-default-group-show --type=hostgroup ipa automember-default-group-show --type=group - +) + _( Find all of the automember rules: ipa automember-find - +) + _( Display a automember rule: ipa automember-show --type=hostgroup webservers ipa automember-show --type=group devel - +) + _( Delete an automember rule: ipa automember-del --type=hostgroup webservers ipa automember-del --type=group devel - +) + _( Rebuild membership for all users: ipa automember-rebuild --type=group - +) + _( Rebuild membership for all hosts: ipa automember-rebuild --type=hostgroup - +) + _( Rebuild membership for specified users: ipa automember-rebuild --users=tuser1 --users=tuser2 - +) + _( Rebuild membership for specified hosts: ipa automember-rebuild --hosts=web1.example.com --hosts=web2.example.com ) diff --git
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
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.
I'm a bit confused by the memory management in get_realm_backend() and
its callers. Who owns 'be'?
0002
ACK
One small nitpick, then I too say ACK. In the commit message, the second
sentence doesn't need a line break.
0003
Same as patch 002: unnecessary line breaks in the commit message.
I think ipapwd_getkeytab() is unnecessarily long. Several sections of it
could be broken out into functions and would make it much more readable.
Nearly forty lines of variable declarations is a bit much. :) You could
break apart BER encoding/decoding, key writing, etc.
The ipapwd_getkeytab() function variable declarations contain a mix of
camel case and underscore styles.
The comment containing the ASN.1 code contains invalid syntax.
I find code like this very hard to read:
if (rtag == (ctag | 2)) {
Some named constants would be helpful here, or maybe a descriptively
named macro function.
We have C99 now, so I'd prefer local scope iterators in for loops:
for (int i = 0; ...) -- rather than -- for (i = 0; ...)
This has inconsistent indents:
+svals = ipapwd_encrypt_encode_key(krbcfg, data,
+ kenctypes ? num_kenctypes :
+krbcfg-num_pref_encsalts,
+ kenctypes ? kenctypes :
+krbcfg-pref_encsalts,
+ errMesg);
Has the new OID been registered?
Since getnew is defined as a boolean in the ASN.1, is the conditional
if (getnew == 0)
preferred over just
if (getnew)?
Future proof if we want to change it to a non-boolean value for whatever
reason in the future ? :)
I agree with rcrit. Fix this. :)
0004
More occasional indentation issues (tab vs space).
Local loop iterators preferred (for example: get_control_data()).
I'm not a fan of the output variable name for ipa_ldap_bind().
Other than that, pretty close to ACK on this one.
0005
Unnecessary line breaks in git commit message.
ASN.1 syntax errors. Also, this comment has some rogue tab indents.
In ldap_get_keytab(), can't the big while loop be a for loop with a
local scope iterator? Same with the for loop in
ipa_string_to_enctypes().
Line 308 (int retrieve = 0;) has an 8 space indent. This was likely to
match the tab indents of the surrounding code.
0006
ACK
Nathaniel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 592-628 Update to PatternFly
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. 2. If there's a login error, the logo and the message shifts up. I think it would be nicer to display the error without changing the layout of the original page. Snapshots for #2, just in case: http://edewata.fedorapeople.org/ipa/images/snapshot5.png http://edewata.fedorapeople.org/ipa/images/snapshot6.png -- 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 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.
I'm a bit confused by the memory management in get_realm_backend() and
its callers. Who owns 'be'?
The main DS code afaik.
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.
0003
Same as patch 002: unnecessary line breaks in the commit message.
See above.
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() ...
Nearly forty lines of variable declarations is a bit much. :) You could
break apart BER encoding/decoding, key writing, etc.
Perhaps, but wouldn't change the amount of code, unless you say it is
necessary in order to do a better review I will skip doing that for now.
The ipapwd_getkeytab() function variable declarations contain a mix of
camel case and underscore styles.
Inherited from old code, see ipa_setkeytab()
The comment containing the ASN.1 code contains invalid syntax.
Please be more specific ?
That pseudo code is not meant to be compiled, so it is a bit liberal.
I find code like this very hard to read:
if (rtag == (ctag | 2)) {
Some named constants would be helpful here, or maybe a descriptively
named macro function.
We have C99 now, so I'd prefer local scope iterators in for loops:
for (int i = 0; ...) -- rather than -- for (i = 0; ...)
We still declare everything upfront in freeipa code, not going to change
style with these patches.
This has inconsistent indents:
+svals = ipapwd_encrypt_encode_key(krbcfg, data,
+ kenctypes ? num_kenctypes :
+krbcfg-num_pref_encsalts,
+ kenctypes ? kenctypes :
+krbcfg-pref_encsalts,
+ errMesg);
Yes these indents are not the best but allow to keep the code within 80
chars.
Has the new OID been registered?
Yup.
Since getnew is defined as a boolean in the ASN.1, is the conditional
if (getnew == 0)
preferred over just
if (getnew)?
Future proof if we want to change it to a non-boolean value for whatever
reason in the future ? :)
I agree with rcrit. Fix this. :)
Fixing how ? There is nothing
Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs
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).
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.
0003
Same as patch 002: unnecessary line breaks in the commit message.
See above.
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.
Nearly forty lines of variable declarations is a bit much. :) You could
break apart BER encoding/decoding, key writing, etc.
Perhaps, but wouldn't change the amount of code, unless you say it is
necessary in order to do a better review I will skip doing that for now.
See above.
The ipapwd_getkeytab() function variable declarations contain a mix of
camel case and underscore styles.
Inherited from old code, see ipa_setkeytab()
Sure, but it is also a brand new section. Doesn't your editor have a
variable renamer? Mine does. ;)
The comment containing the ASN.1 code contains invalid syntax.
Please be more specific ?
That pseudo code is not meant to be compiled, so it is a bit liberal.
::= is, I think, the only issue.
I find code like this very hard to read:
if (rtag == (ctag | 2)) {
Some named constants would be helpful here, or maybe a descriptively
named macro function.
We have C99 now, so I'd prefer local scope iterators in for loops:
for (int i = 0; ...) -- rather than -- for (i =
