Add --password-expiration to allow an admin to force a password change #621

redhatrises · 2017-03-18T15:46:39Z

Allows an admin to easily force a user to expire their password forcing them to change it.

abbra · 2017-03-18T17:08:52Z

I would prefer this to be an option in ipa passwd, e.g. ipa passwd --force-reset which instead of modifying a user password would modify krbPasswordExpiration value.

redhatrises · 2017-03-20T13:41:18Z

@abbra why not have it in both ipa user-mod and ipa passwd?

abbra · 2017-03-20T14:19:41Z

Hm. ipa user-mod has --random and also supports specifying --password, so yes, both interfaces should be provided.

HonzaCholasta · 2017-03-20T14:30:03Z

I don't agree. There should be one and only one obvious way to do it. There is no real benefit in having this in multiple different places, it just adds unnecessary complexity. Let's not repeat mistakes of the past, put this solely into passwd.

HonzaCholasta · 2017-03-20T14:46:01Z

Actually, maybe user-mod is a better place for the option, as it does LDAP modify operation, whereas passwd does LDAP password change extended operation.

redhatrises · 2017-03-20T16:04:58Z

Okay, so since it will reside in one location, should it be user-mod (PR already uses user-mod) or passwd?

abbra · 2017-03-20T16:39:54Z

Ok, let's go with user-mod as original request goes, based on the fact that we are not changing the password, we are changing its properties.

LGTM.

HonzaCholasta · 2017-03-21T06:59:29Z

I have given this some thought over the night - maybe we should make the option more generic and allow the user to specify the expiration time rather than special case it for "now" time, i.e. --password-expiration=2017-03-21T07:58:05Z to expire the password at a specific time, --password-expiration=now to expire the password now, just like --force-password-reset does.

redhatrises · 2017-03-21T12:30:08Z

@HonzaCholasta that's an interesting idea. Most of the time, a password reset is forced immediately, but that does provide more flexibility. I assume that the datetime input should match the 2017-03-21T07:58:05Z format?

HonzaCholasta · 2017-03-21T14:46:11Z

@redhatrises, do not handle the format yourself, use the DateTime param type. Note that you will need to extend it to correctly interpret the "now" value.

HonzaCholasta · 2017-03-29T05:59:16Z

ipalib/parameters.py

-                    pass
+            if value == u'now':
+                time = datetime.datetime.strptime(strftime("%Y%m%d%H%M%SZ", gmtime()), "%Y%m%d%H%M%SZ")
+                return time


Please use datetime.utctime().

redhatrises · 2017-03-29T13:19:01Z

@HonzaCholasta used datetime.utcnow() as I couldn't find a reference for datetime.utctime()

HonzaCholasta · 2017-03-29T13:22:32Z

@redhatrises, datetime.utcnow() is what I meant.

redhatrises · 2017-03-29T13:28:06Z

@redhatrises, datetime.utcnow() is what I meant.

Oh good. Ready for your review.

HonzaCholasta · 2017-03-29T14:48:53Z

The admin user is not allowed to write to the attribute:

$ kinit admin
Password for admin@ABC.IDM.LAB.ENG.BRQ.REDHAT.COM: 
$ ipa user-mod jcholast --password-expiration=now
ipa: ERROR: Insufficient access: Insufficient 'write' privilege to the 'krbPasswordExpiration' attribute of entry 'uid=jcholast,cn=users,cn=accounts,dc=abc,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com'.

Please update the "Admin can manage any entry" ACI in install/updates/20-aci.update.

redhatrises · 2017-03-29T15:14:52Z

@HonzaCholasta updated "Admins can write passwords" ACI to contain 'krbPasswordExpiration' as the "Admin can manage any entry" ACI already had 'krbPasswordExpiration' added.

MartinBasti · 2017-03-29T17:09:34Z

VERSION.m4

@@ -74,7 +74,7 @@ define(IPA_DATA_VERSION, 20100614120000)
 ########################################################
 define(IPA_API_VERSION_MAJOR, 2)
 define(IPA_API_VERSION_MINOR, 224)


Bump minor version please

MartinBasti · 2017-03-29T17:11:35Z

install/updates/20-aci.update

@@ -54,7 +54,7 @@ remove:aci:(targetattr != "userPassword || krbPrincipalKey || sambaLMPassword ||
 add:aci:(targetattr != "userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory || krbMKey || krbPrincipalName || krbCanonicalName || krbPasswordExpiration || krbPwdHistory || krbLastPwdChange || krbExtraData || krbLastSuccessfulAuth || krbLastFailedAuth || ipaUniqueId || memberOf || enrolledBy || ipaNTHash || ipaProtectedOperation")(version 3.0; acl "Admin can manage any entry"; allow (all) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)
 # Write-only
 remove:aci:(targetattr = "userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory")(version 3.0; acl "Admins can write passwords"; allow (add,delete,write) groupdn="ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)
-add:aci:(targetattr = "userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory || ipaNTHash")(version 3.0; acl "Admins can write passwords"; allow (add,delete,write) groupdn="ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)


Please leave the original ACI there, just replace s/add/remove/ to avoid unholy mess with multiple ACIs (and keep the new one there as well :) )

@MartinBasti not sure I fully understand you correctly, but set the original ACI to remove and added a new ACI one.

yes, add rule to remove current one from LDAP

HonzaCholasta · 2017-03-30T08:58:20Z

@redhatrises, the "Admin can manage any entry" ACI in fact contains a blacklist of attributes which admins aren't allowed to write. To actually fix the issue you must also remove krbPasswordExpiration from the "Admin can manage any entry" ACI.

…ation - Allows an admin to easily force a user to expire their password forcing the user to change it immediately or at a specified time in the future

redhatrises · 2017-03-31T00:24:12Z

@HonzaCholasta I also removed krbPasswordExpiration from the "Admin can manage any entry" ACI.

HonzaCholasta · 2017-03-31T07:11:18Z

Works for me. Thanks!

MartinBasti · 2017-03-31T10:20:02Z

master:

274b0bc Add --password-expiration to allow admin to force user password expiration

simo5

I am not sure the expiration should be couple with the change user password permission.
It should probably be allowed with some other higher level role/permission

simo5 · 2017-03-31T15:25:32Z

ACI.txt

@@ -351,7 +351,7 @@ aci: (targetattr = "member")(target = "ldap:///cn=ipausers,cn=groups,cn=accounts
 dn: cn=users,cn=accounts,dc=ipa,dc=example
 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=ipa,dc=example";)
 dn: cn=users,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "krbprincipalkey || passwordhistory || sambalmpassword || sambantpassword || userpassword")(targetfilter = "(&(!(memberOf=cn=admins,cn=groups,cn=accounts,dc=ipa,dc=example))(objectclass=posixaccount))")(version 3.0;acl "permission:System: Change User password";allow (write) groupdn = "ldap:///cn=System: Change User password,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+aci: (targetattr = "krbpasswordexpiration || krbprincipalkey || passwordhistory || sambalmpassword || sambantpassword || userpassword")(targetfilter = "(&(!(memberOf=cn=admins,cn=groups,cn=accounts,dc=ipa,dc=example))(objectclass=posixaccount))")(version 3.0;acl "permission:System: Change User password";allow (write) groupdn = "ldap:///cn=System: Change User password,cn=permissions,cn=pbac,dc=ipa,dc=example";)


Why do you need to add krbpasswordexpiration to the change user password option ?
I think it should be a separate (higher privileged permission) to tamper with password expirations.
Frontline helpdesk may be allowed to change a user password, but they are not allowed to change expiration for example.

simo5 · 2017-03-31T15:29:03Z

ipaserver/plugins/user.py

@@ -261,7 +261,7 @@ class user(baseuser):
            ],
            'ipapermdefaultattr': {
                'krbprincipalkey', 'passwordhistory', 'sambalmpassword',
-                'sambantpassword', 'userpassword'
+                'sambantpassword', 'userpassword', 'krbpasswordexpiration'


redhatrises force-pushed the force_password_reset branch from 5916353 to 78377d8 Compare March 29, 2017 03:54

HonzaCholasta reviewed Mar 29, 2017

View reviewed changes

redhatrises force-pushed the force_password_reset branch from 78377d8 to c773399 Compare March 29, 2017 13:12

redhatrises changed the title ~~Add --force-password-reset to user_mod in user.py~~ Add --password-expiration to allow an admin to force a password change Mar 29, 2017

redhatrises force-pushed the force_password_reset branch from c773399 to 92126da Compare March 29, 2017 15:13

MartinBasti suggested changes Mar 29, 2017

View reviewed changes

redhatrises force-pushed the force_password_reset branch from 92126da to e0f3075 Compare March 30, 2017 02:35

MartinBasti approved these changes Mar 30, 2017

View reviewed changes

Add --password-expiration to allow admin to force user password expir…

c9e5553

…ation - Allows an admin to easily force a user to expire their password forcing the user to change it immediately or at a specified time in the future

redhatrises force-pushed the force_password_reset branch from e0f3075 to c9e5553 Compare March 31, 2017 00:22

HonzaCholasta added the ack Pull Request approved, can be merged label Mar 31, 2017

MartinBasti added the pushed Pull Request has already been pushed label Mar 31, 2017

MartinBasti closed this Mar 31, 2017

redhatrises deleted the force_password_reset branch March 31, 2017 12:36

simo5 reviewed Mar 31, 2017

View reviewed changes

Add --password-expiration to allow an admin to force a password change #621

Add --password-expiration to allow an admin to force a password change #621

redhatrises commented Mar 18, 2017 •

edited by MartinBasti

abbra commented Mar 18, 2017

redhatrises commented Mar 20, 2017

abbra commented Mar 20, 2017

HonzaCholasta commented Mar 20, 2017 •

edited

HonzaCholasta commented Mar 20, 2017

redhatrises commented Mar 20, 2017

abbra commented Mar 20, 2017

HonzaCholasta commented Mar 21, 2017

redhatrises commented Mar 21, 2017

HonzaCholasta commented Mar 21, 2017

HonzaCholasta Mar 29, 2017

redhatrises commented Mar 29, 2017

HonzaCholasta commented Mar 29, 2017

redhatrises commented Mar 29, 2017

HonzaCholasta commented Mar 29, 2017

redhatrises commented Mar 29, 2017

MartinBasti Mar 29, 2017

MartinBasti Mar 29, 2017

redhatrises Mar 30, 2017

MartinBasti Mar 30, 2017

HonzaCholasta commented Mar 30, 2017

redhatrises commented Mar 31, 2017

HonzaCholasta commented Mar 31, 2017

MartinBasti commented Mar 31, 2017

simo5 left a comment

simo5 Mar 31, 2017

simo5 Mar 31, 2017

Add --password-expiration to allow an admin to force a password change #621

Add --password-expiration to allow an admin to force a password change #621

Conversation

redhatrises commented Mar 18, 2017 • edited by MartinBasti

abbra commented Mar 18, 2017

redhatrises commented Mar 20, 2017

abbra commented Mar 20, 2017

HonzaCholasta commented Mar 20, 2017 • edited

HonzaCholasta commented Mar 20, 2017

redhatrises commented Mar 20, 2017

abbra commented Mar 20, 2017

HonzaCholasta commented Mar 21, 2017

redhatrises commented Mar 21, 2017

HonzaCholasta commented Mar 21, 2017

HonzaCholasta Mar 29, 2017

Choose a reason for hiding this comment

redhatrises commented Mar 29, 2017

HonzaCholasta commented Mar 29, 2017

redhatrises commented Mar 29, 2017

HonzaCholasta commented Mar 29, 2017

redhatrises commented Mar 29, 2017

MartinBasti Mar 29, 2017

Choose a reason for hiding this comment

MartinBasti Mar 29, 2017

Choose a reason for hiding this comment

redhatrises Mar 30, 2017

Choose a reason for hiding this comment

MartinBasti Mar 30, 2017

Choose a reason for hiding this comment

HonzaCholasta commented Mar 30, 2017

redhatrises commented Mar 31, 2017

HonzaCholasta commented Mar 31, 2017

MartinBasti commented Mar 31, 2017

simo5 left a comment

Choose a reason for hiding this comment

simo5 Mar 31, 2017

Choose a reason for hiding this comment

simo5 Mar 31, 2017

Choose a reason for hiding this comment

redhatrises commented Mar 18, 2017 •

edited by MartinBasti

HonzaCholasta commented Mar 20, 2017 •

edited