illumos gate

When a user has "NP" set as their password, attempting to lock them results in no change to their password. For example:

# getent shadow foo
foo:NP:::::::

# passwd -l foo
passwd: password information changed for foo

# getent shadow foo
foo:NP:16518::::::

The password at the end of the example is still set to "NP".

Looking into the code, it appears that when a lock is attempted on the shadow file, the password is not touch if 1) it already has "*LK*" at the beginning or 2) is set to NP. Assuming I understand the semantics of NP correctly, this behavior is incorrect. I've attached a patch that fixes this by simply not giving any special treatment to the string "NP" when locking a user.

diff --git a/usr/src/lib/passwdutil/files_attr.c b/usr/src/lib/passwdutil/files_attr.c
index 2a8ac5c..484bcee 100644
--- a/usr/src/lib/passwdutil/files_attr.c
+++ b/usr/src/lib/passwdutil/files_attr.c
@@ -737,9 +737,8 @@ files_update(attrlist *items, pwu_repository_t *rep, void *buf)
         case ATTR_LOCK_ACCOUNT:
             if (spw->sp_pwdp == NULL) {
                 spw->sp_pwdp = LOCKSTRING;
-            } else if ((strncmp(spw->sp_pwdp, LOCKSTRING,
-                sizeof (LOCKSTRING)-1) != 0) &&
-                (strcmp(spw->sp_pwdp, NOLOGINSTRING) != 0)) {
+            } else if (strncmp(spw->sp_pwdp, LOCKSTRING,
+                sizeof (LOCKSTRING)-1) != 0) {
                 len = sizeof (LOCKSTRING)-1 +
                     strlen(spw->sp_pwdp) + 1;
                 pword = malloc(len);

With this patch in place, this will result in the following behavior

# getent shadow foo
foo:NP:::::::

# passwd -l foo
passwd: password information changed for foo

# getent shadow foo
foo:*LK*NP:16518::::::

# passwd -u foo
passwd: password information changed for foo

# getent shadow foo
foo:NP:16518::::::

Here's the relevant chat log from #illumos on freenode

12:03:37       bahamas10  hey all, i asked this in #smartos pretty late last night but I think it may be more of a generic illumos thing
12:03:57       bahamas10  when locking a user using `passwd -l <user>`, no action is taken if the user has "NP" set as their password https://gist.github.com/bahamas10/9ab2115db23f81a4d7ca
12:04:52       bahamas10  digging into the source though, I can see that the code is setup to noop if the users password starts with "*LK*", or is set to "NP" exactly
12:04:53       bahamas10  https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/passwdutil/files_attr.c#L740-L742
12:05:09       bahamas10  so my question becomes, what is the purpose of "NP"?
12:05:26        LeftWing  So that you can have accounts that are not locked, but which will not allow password authentication.
12:05:29       bahamas10  when a user has NP, they are still able to login using SSH..
12:05:44        LeftWing  For SSH, it becomes a public key-only account.
12:05:56       bahamas10  LeftWing: ok, so then i've been using NP right
12:06:06       bahamas10  NP is considered unlocked correct?
12:06:18        LeftWing  That is correct
12:06:22       bahamas10  so, next question
12:06:23       bahamas10  https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/passwdutil/files_attr.c#L740-L742
12:06:35       bahamas10  the code skips locking the user, if the user has "NP" set as their password
12:06:59       bahamas10  (NOLOGINSTRING is defined as "NP")
12:08:10       bahamas10  which results in what i see in the gist i pasted. if a user has NP set, running `passwd -l <user>` doesn't lock them
12:09:44        LeftWing  I think that's probably the expected behaviour.
12:09:56        LeftWing  What are you wanting to happen by engaging the account lock?
12:10:42        LeftWing  I guess it should probably error out if it was unable to do the right thing.
12:10:56        LeftWing  Well, the _requested_ thing.
12:11:26       bahamas10  so, i guess my real question becomes what does locked actually mean?
12:11:34       bahamas10  I assumed that locking a user meant they could no longer SSH in
12:11:49       bahamas10  but perhaps that was an incorrect assumption
12:12:32        LeftWing  I think you're right, actually. The "-l" flag should probably clobber, or interact with, the "-N" flag to passwd (which is what sets NP in the shadow entry)
12:12:39        LeftWing  Rather than just ignore it.
12:13:19       bahamas10  LeftWing: that's what I imagined would happen
12:13:42       bahamas10  if you look at the code in files_attr.c, i would have assumed the "NP" checking logic would have been in the first if statement
12:14:01       bahamas10  ie. if the current password is NULL, or "NP", set it to "*LK*", else just prepend "*LK*" 
12:15:10        LeftWing  bahamas10: I think that actually *LK*NP makes sense.
12:15:28        LeftWing  That the lock flag should be orthogonal to the nopasswd flag.
12:17:55       bahamas10  LeftWing: ah ok
12:19:06        LeftWing  bahamas10: Are you planning to log an illumos bug?
12:19:12       bahamas10  LeftWing: doing it now
12:19:24        LeftWing  Something along the lines of 'passwd -N and -l flags should interact'
12:19:52        LeftWing  I don't think anybody can correctly be using the current behaviour -- i.e. "-l" fails silently if "-N" has been used.
12:20:16       bahamas10  LeftWing: agreed. mind if i quote some of this chat in the issue?
12:20:17        LeftWing  And, actually, it's dangerous to have it work that way -- given, as you point out, that non-password logins still work.
12:20:21        LeftWing  Sure!