Bug 39817 - Locked login methods ignored by Samba 4
Locked login methods ignored by Samba 4
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC - Users
UCS 4.1
Other Linux
: P5 normal (vote)
: UCS 4.3
Assigned To: Arvid Requate
Stefan Gohmann
: interim-2
: 24185 42947 (view as bug list)
Depends on: 32014
Blocks: 46351 46431 46175 46200
  Show dependency treegraph
 
Reported: 2015-11-06 18:15 CET by Stefan Gohmann
Modified: 2019-02-27 18:04 CET (History)
6 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 3: A User would likely not purchase the product
User Pain: 0.171
Enterprise Customer affected?: Yes
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number: 2017013121000226, 2012051121003316, 201708032100053
Bug group (optional):
Max CVSS v3 score:
best: Patch_Available+


Attachments
Screenshot new layout of diabled/locked options (39.10 KB, image/png)
2018-01-31 12:50 CET, Florian Best
Details
Screenshot "password change next login" now on the first tab (44.04 KB, image/png)
2018-01-31 12:51 CET, Florian Best
Details
Screenshot "Mail options are now in an own tab" (40.58 KB, image/png)
2018-01-31 12:52 CET, Florian Best
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Gohmann univentionstaff 2015-11-06 18:15:18 CET
In UMC a user account can be locked by selecting one of the following values  for the attribute login methods:
 - Lock all login methods
 - Lock Windows/Kerberos only
 - Lock POSIX/LDAP only

But it is still possible to make a smbclient or a kinit in a Samba 4 environment if all login methods or Windows/Kerberos only was selected.

I think it worked with previous versions but I was able to reproduce it with 4.0-3errata281. So it is not a regression of UCS 4.1.

I think the S4 connector must synchronize the lock state.
Comment 1 Arvid Requate univentionstaff 2016-12-07 18:54:51 CET
> In UMC a user account can be locked

To have clear terms, we need to distinguish between "account disabling" and password "lockout". Disabling the account should be simple and work. If not, then that's a different issue. So let's focus on "lockout":

In Samba/AD the lockout is managed by the Domain Controller according to lockout policies (see Bug 32974) in case of repeated logon failure. An Administrator only has the possibility to unlock. So UMC has a different concept of this.


Technically, as Lukas just discovered, the UF_LOCKOUT bit of the userAccountControl attribute in Samba/AD has a special behavior which doesn't allow it to be set by a modify operation (Details: https://msdn.microsoft.com/en-us/library/cc245673.aspx ).

So, technically I'm inclined to close this Bug as duplicate of Bug 32014, which contains a detailed analysis of the intricacies of synchronizing the (temporal) lockout state.

Instead, I'm re-purposing this bug into a bug against the UDM/UMC users/user module to discuss, how we can improve the current "lockout" concept in UCS

A) to be compatible with Samba/AD

and

B) to be clearly distinguishable from account the "disable" settings


I don't know if customers can understand the difference between "lockout" and "disable". At least I usually have my difficulties. So I suggest to simplify this by removing the current "lockout" handling in UMC/UDM and replacing it by proper policy based lockout plus an UMC widget allowing administrative unlock. When doing this, we need to consider the research results of Bug 32014 to make sync work.
Comment 2 Florian Best univentionstaff 2017-01-29 18:20:41 CET
@Stefan
please set an asignee
Comment 3 Stefan Gohmann univentionstaff 2017-01-31 19:28:01 CET
@Jens, FYI this is the issue you asked about, Bug #32014 is about another topic.
Comment 4 Florian Best univentionstaff 2017-02-10 14:16:52 CET
@Arvid:
I am assigning you. If the changes need to be done in UDM or in UMC you can assign me. Please define a concept how the behavior should be and what needs to be implemented then.
Comment 5 Arvid Requate univentionstaff 2017-02-13 17:08:51 CET
I think Comment 1 says it all:

> So I suggest to simplify this by removing the current "lockout" handling in
> UMC/UDM and replacing it by proper policy based lockout plus an UMC widget
> allowing administrative unlock.

The rationale goes like this:

Active Directory doesn't implement the concept of manually "locking" an account, only "disabling" and "unlocking". "locking" is done automatically, internally via a policy module, similar to the ppolicy OpenLDAP overlay (Bug 31907). And since Active Directory does not have this concept, we have two options:

A) Implement a custom interface in Samba to map the UMC/UDM account locking concept to Samba (but not native AD) and adjust the S4-Connector (but not the AD-Connector).

or

B) Get rid of the UMC/UDM concept of "locking" accounts.


Bug 18825 is of historical interest here. I have gathered an overview about the central UCS bugs that make up the history of the topic of locking vs deactivation:
==========================================================================
We have several different concepts and techniques here and need to clearly define what we want, can and need to do (see Bug 39817 Comment 1):


A) Automatic temporary account lockout on repeated failed logon attempts
A.1) For Samba/AD: (Bug 32974)
A.2) For PAM login: auth/faillog (Bug 18750)
A.3) For LDAP login: ldap/ppolicy/.* (Bug 31907)

B) Locked login methods (see Bug 39817)
B.1) UCS specific concept (Bug 18825)

C) Account deactivation
C.1) Samba/AD: Manually setting userAccountControl "D" flag via ADUC
C.2) UDM (Bug 18825)
==========================================================================
Comment 8 Florian Best univentionstaff 2017-04-26 17:40:11 CEST
Since Bug #31907 we support also LDAP lockouts via the attribute "pwdAccountLockedTime".
I think we need to display the fields also in UMC:

Samba Login locked: <timestamp>    Unlock Samba Login [x]
LDAP Bind locked: <timestamp>    Unlock LDAP Bind [x]

The second should probably be hidden if not ucr.is_true('ldap/ppolicy/enabled') or not ucr.is_true('ldap/ppolicy').
Comment 9 Florian Best univentionstaff 2018-01-24 14:59:44 CET
I just found out that there is a "locked" state also in kerberos, which is evaluated.
This is the flag (1 << 17) in krb5KDCFlags.

# udm users/user create --set username=myusername --set password=univention --set lastname=foo
Object created: uid=myusername,dc=dev,dc=local

# kinit myusername
myusername@DEV.LOCAL's Password: 
→ succeds

# python -c 'import univention.admin.uldap; lo,po=univention.admin.uldap.getMachineConnection(); lo.modify("uid=myusername,dc=dev,dc=local", [("krb5KDCFlags", ["126"], ["131198"])])'                                                                           
# univention-ldapsearch -LLLb uid=myusername,dc=dev,dc=local krb5KDCFlags
dn: uid=myusername,dc=dev,dc=local
krb5KDCFlags: 131198

# kinit myusername
myusername@DEV.LOCAL's Password: 
kinit: krb5_get_init_creds: Clients credentials have been revoked
Comment 10 Florian Best univentionstaff 2018-01-26 18:23:38 CET
We currently have a script which locks a user in a PAM stack.
As locking users is not anymore supported via UDM should it instead disable the user account?

base/univention-pam/conffiles/etc/pam.d/common-auth-nowrite:
»   »   print 'auth»   [default=die]»   pam_runasroot.so program=/usr/lib/univention-pam/lock-user'

base/univention-pam/conffiles/etc/pam.d/common-auth.d/30univention-pam_local:
»   »   print 'auth»   [default=die]»   pam_runasroot.so program=/usr/lib/univention-pam/lock-user'
Comment 11 Florian Best univentionstaff 2018-01-26 18:27:11 CET
There is also univention.lib.account.lock(userdn) which is used by the ppolicy overlay module to lock an account.
We need to update it as well.
Comment 12 Florian Best univentionstaff 2018-01-29 16:22:30 CET
The following scripts need to be adjusted:
/usr/share/univention-directory-manager-tools/lock_expired_accounts
/usr/share/univention-directory-manager-tools/lock_expired_passwords

The directory reports might be adjusted.
Comment 13 Arvid Requate univentionstaff 2018-01-29 21:25:06 CET
> As locking users is not anymore supported via UDM should it instead disable the user account?

As discussed, we can lock it in Samba (not in MS AD) via S4-Connector, see https://git.knut.univention.de/univention/ucs/commit/f49b7aca864cb4df8e90f718d29105438d5d4068

setting sambaBadPasswordTime in addition to sambaAcctFlags is required for this.
Comment 14 Florian Best univentionstaff 2018-01-30 18:37:50 CET
*** Bug 42947 has been marked as a duplicate of this bug. ***
Comment 15 Florian Best univentionstaff 2018-01-30 20:54:42 CET
Changes have been merged:

univention-pam (11.0.0-1)
ae734b95f70c | Bug #39817: use univention.lib.account lock for locking user accounts

ucs-test (8.0.23-1)
893797232ef1 | Bug #39817: Bug #32014: Merge branch 'fbest/39817-simplify-unify-locked-disabled' into 4.3-0
73bd550a5779 | Bug #39817: unify disabled/locked

univention-directory-manager-modules (13.0.10-1)
893797232ef1 | Bug #39817: Bug #32014: Merge branch 'fbest/39817-simplify-unify-locked-disabled' into 4.3-0
73bd550a5779 | Bug #39817: unify disabled/locked

univention-s4-connector (12.0.2-1)
893797232ef1 | Bug #39817: Bug #32014: Merge branch 'fbest/39817-simplify-unify-locked-disabled' into 4.3-0
73bd550a5779 | Bug #39817: unify disabled/locked

univention-directory-manager-modules (13.0.9-1)
d3142a25328c | Bug #39817: adjust scripts to lock user accounts
55201b36d03c | Bug #39817: unify locked, disabled and add unlock state

univention-ad-connector (12.0.0-3)
893797232ef1 | Bug #39817: Bug #32014: Merge branch 'fbest/39817-simplify-unify-locked-disabled' into 4.3-0
73bd550a5779 | Bug #39817: unify disabled/locked

univention-ad-connector (12.0.0-2)
3abbfbc9918f | Bug #39817: disabled now uses 1/0 instead of all/none

univention-pam (11.0.1-1)
893797232ef1 | Bug #39817: Bug #32014: Merge branch 'fbest/39817-simplify-unify-locked-disabled' into 4.3-0
73bd550a5779 | Bug #39817: unify disabled/locked

univention-directory-manager-modules (13.0.12-1)
eecba8a74a38 | Bug #39817: fix fuzzy

ucs-test (8.0.22-3)
3f4bdc3d1239 | Bug #39817: replace locking/unlocking calls with the corresponding actions
87e2308c844f | Bug #39817: disabled and locked now use 1/0 instead of all/none/posix/windows/kerberos/etc.

univention-directory-manager-modules (13.0.11-1)
602430cbaffe | Bug #39817: #45877: update translations

univention-s4-connector (12.0.1-1)
3abbfbc9918f | Bug #39817: disabled now uses 1/0 instead of all/none

univention-management-console-module-udm (8.0.1-0)
025284e0abf5 | Bug #39817: disabled now uses 1/0 instead of all/none

univention-management-console-module-udm (8.0.2-1)
893797232ef1 | Bug #39817: Bug #32014: Merge branch 'fbest/39817-simplify-unify-locked-disabled' into 4.3-0
73bd550a5779 | Bug #39817: unify disabled/locked
Comment 16 Florian Best univentionstaff 2018-01-31 12:38:40 CET
I discarded the idea of having one "unlock" property. Instead "locked" will be a checkbox with the old behavior. If the checkbox is unset ("0") then the javascript widget will prevent that it can be checked.
Comment 17 Florian Best univentionstaff 2018-01-31 12:50:47 CET
Created attachment 9372 [details]
Screenshot new layout of diabled/locked options
Comment 18 Florian Best univentionstaff 2018-01-31 12:51:54 CET
Created attachment 9373 [details]
Screenshot "password change next login" now on the first tab
Comment 19 Florian Best univentionstaff 2018-01-31 12:52:53 CET
Created attachment 9374 [details]
Screenshot "Mail options are now in an own tab"
Comment 20 Florian Best univentionstaff 2018-01-31 13:13:03 CET
I set this bug to resolved now.
Summary of Changes:

* locked is now a checkbox, which can be unchecked to unlock a user.
* While the UMC frontend prevents it, the CLI still allows to set locked=1. This is at the moment okay. Technically this would be wrong and it does not work with the AD connector.
* There is a script which can be used for testing purposes to lock a user:

python -m univention.lib.account lock --dn uid=foo,cn=users,dc=dev2,dc=local  --lock-time "$(date --utc '+%Y%m%d%H%M%SZ')"

* locked does not set the "!" in the userPassword anymore. Instead setting "disabled" will cause this.

* a property "lockedTime" which maps to the attribute "sambaBadPasswordTime" has been added. It's not displayed anywhere. It is the value when the password was locked.

* a property "unlockTime" has been added, which is the date / time when the user password gets automatically unlocked. It is calculated from the "lockedTime" plus "sambaLockoutDuration" of the "objectClass=sambaDomain" object.

* "disabled" is now a checkbox as well, which disables POSIX, Samba and Kerberos.

* More descriptions/tooltips of some fields have been added.

* Layout changes:
"mailPrimaryAdress" has been moved into a new tab "Mail", which contains also all other mail related options.

"pwdChangeNextLogin" has been moved to the password input fields, where it technically and user-experience belongs to.

Disabled and locked are in two different groups.
Comment 21 Stefan Gohmann univentionstaff 2018-01-31 13:26:49 CET
I don't think we should changes these two layout changes:

(In reply to Florian Best from comment #20)
> * Layout changes:
> "mailPrimaryAdress" has been moved into a new tab "Mail", which contains
> also all other mail related options.

The mail address is not part of this bug and for me it makes sense to have the most used values on the first tab.
 
> "pwdChangeNextLogin" has been moved to the password input fields, where it
> technically and user-experience belongs to.

Can you move it back to the account field?
Comment 22 Florian Best univentionstaff 2018-01-31 13:35:07 CET
(In reply to Stefan Gohmann from comment #21)
> I don't think we should changes these two layout changes:
> 
> (In reply to Florian Best from comment #20)
> > * Layout changes:
> > "mailPrimaryAdress" has been moved into a new tab "Mail", which contains
> > also all other mail related options.
> 
> The mail address is not part of this bug and for me it makes sense to have
> the most used values on the first tab.
>  
> > "pwdChangeNextLogin" has been moved to the password input fields, where it
> > technically and user-experience belongs to.
> 
> Can you move it back to the account field?

Okay, layout changes have been reverted.
Comment 23 Florian Best univentionstaff 2018-01-31 15:16:06 CET
*** Bug 24185 has been marked as a duplicate of this bug. ***
Comment 24 Florian Best univentionstaff 2018-01-31 21:51:18 CET
Currently the test cases fail:
01_base/47faillog
10_ldap/50ppolicy_account_lockout

I tried to analyze both. I can't explain what's wrong.
In the latter one, all ldap attributes are regular and correct for the user object. I checked even the crypt password. It seems the ppolicy overlay still blocks the user from doing an ldap bind.
Comment 25 Arvid Requate univentionstaff 2018-02-06 12:52:40 CET
I have adjusted 61_udm-users/26_password_expire_date to expect the new behavior of --set disable=1.
Comment 26 Arvid Requate univentionstaff 2018-02-06 13:45:06 CET
I fixed a shell quoting error in tests/61_udm-users/25_script_lock_expired_*
Comment 27 Arvid Requate univentionstaff 2018-02-06 17:39:01 CET
I've also fixed the same quoting bug in
* tests/61_udm-users/37_user_modification_set_deactivation_and_locked
* tests/61_udm-users/100_test_users.py

Also, to make 01_base/47faillog work, I had to update the faillog.py Listener module code to the new "locked" definition. It's using a function from users/user now (unmapLocked) to detect the status.
Comment 28 Arvid Requate univentionstaff 2018-02-06 19:22:42 CET
Ok I found why 10_ldap/50ppolicy_account_lockout failed and adjusted the resetting code for pwdAccountLockedTime in users/user accordingly. The problem was, that pwdAccountLockedTime is an operational attribute and as such not found in oldattr by default. I've adjusted the code in fbest/39817-simplify-unify-locked-disabled and merged it into 4.3-0. The package has been rebuilt.
Comment 29 univention 2018-02-06 21:57:44 CET
Interesting, thank you very much Arvid!
Comment 30 Stefan Gohmann univentionstaff 2018-02-07 11:28:35 CET
I think we should change the layout.

Currently:

User has to change password on next login
User password is locked
Password expiry date
Password unlock date

Suggestion:

User has to change password on next login
Password expiry date
User password is locked
Password unlock date

Do we have any options to make the status of "User password is locked" more understandable? Currently, I can't change anything and it is not clear why. If we can't make it visable, we should adjust the help text.
Comment 31 Arvid Requate univentionstaff 2018-02-07 19:06:36 CET
Layout and wording adjusted to match Active Directory and UDM Samba Domain object.

Documentation and changelog updated.

The locked checkbox looks a bit ugly in the default (unlocked) state: Bug 37620.
Comment 32 Stefan Gohmann univentionstaff 2018-02-21 08:13:08 CET
It works much better as in UCS 4.2.

So far, I found the following two issues:

1. If I configure faillog including the global lock and a user reaches the faillog limit, a ldap bind is not possible for this user in UCS 4.2. In UCS 4.3, it is possible.

2. If a user is locked in UCS 4.3, the "Reset lockup" button is activated. So, I have to disable the checkbox "Reset lockup" to reset it. For me, it is confusing.
Comment 33 Arvid Requate univentionstaff 2018-02-22 23:54:13 CET
Ok, I've adjusted univention-pam/lock-user to disable the user after locking.

Then I discovered that Samba/AD automatically resets the lockedTime to 0 when an account gets disabled. I've not checked the MS-specification on this but it makes kind of sense. To save unnecessary roundtrips in the S4-Connector sync and make things compatible to non-Samba domains I've adjusted udm users/user to do the same, triggering reset of "locked" to 0 when "disabled" is 1.

And it is actually very convenient for us, because it connects the change of "disabled" state to the automatic counter reset of pam_tally. That way I didn't need to adjust the faillog.py listener to explicitly monitor for changes in the "disabled" state.

To simplify tests I've split the auth/faillog/lock_global=true half of the larger 01_base/47faillog test case off into a separate test 01_base/47faillog-global, which now also tests that LDAP authentication disabled in this situation too.

I've also replaced the checkbox indicating the "locked" property of the account by a more useful checkbox "unlock", as requested.
Comment 34 Stefan Gohmann univentionstaff 2018-02-23 15:33:46 CET
(In reply to Stefan Gohmann from comment #32)
> It works much better as in UCS 4.2.
> 
> So far, I found the following two issues:
> 
> 1. If I configure faillog including the global lock and a user reaches the
> faillog limit, a ldap bind is not possible for this user in UCS 4.2. In UCS
> 4.3, it is possible.
> 
> 2. If a user is locked in UCS 4.3, the "Reset lockup" button is activated.
> So, I have to disable the checkbox "Reset lockup" to reset it. For me, it is
> confusing.

Both issues work now.
Comment 35 Stefan Gohmann univentionstaff 2018-03-14 14:38:03 CET
UCS 4.3 has been released:
 https://docs.software-univention.de/release-notes-4.3-0-en.html
 https://docs.software-univention.de/release-notes-4.3-0-de.html

If this error occurs again, please use "Clone This Bug".