Merge ~goneri/cloud-init:freebsd_set_password into cloud-init:master

[Gonéri Le Bouder]

set_passwords: support for FreeBSD

Add FreeBSD support to `cc_set_passwords`.
FreeBSD does not come with chpasswd binary, a chpasswd port[1] exists
but it's not the same tool than the one from Linux[2].

Finally, NetBSD and OpenBSD don't have the chpasswd binary at all, so
an abstraction layer will be needed.

This patch moves the Linux specific in the Distro class, this allow
anyone to override the default implementation. It also provides an
implementation for FreeBSD.

[1]: https://www.freshports.org/www/chpasswd/
[2]: https://github.com/shadow-maint/shadow/blob/master/src/chpasswd.c

[Ryan Harper (raharper)]

Thanks for the patch, some inline comments and questions.

review: Needs Fixing

[Gonéri Le Bouder (goneri)]

Hi Ryan, I pushed a new update to address your comments.

[Ryan Harper (raharper)]

Thanks, some inline comments.

review: Needs Fixing

[Gonéri Le Bouder (goneri)]

thanks, it should be fine now.

[Ryan Harper (raharper)]

Thanks for refactoring. I've one more request inline.

[Gonéri Le Bouder (goneri)]

Update: set_password() was actually working fine. I've updated my patch to keep the original version.

[Ryan Harper (raharper)]

OK, then do we need this branch at all ?

[Ryan Harper (raharper)]

review: Needs Information

[Gonéri Le Bouder (goneri)]

Answered inline.

[Gonéri Le Bouder (goneri)]

Could you take a look at the last version to be sure I correctly understand what you want? I will refresh the test-suite later.

[Ryan Harper (raharper)]

OK. If the core issue is that freebsd doesn't have a chpasswd, which takes a list of users to modify, rather than a single user (like passwd). Then I think we need a different interface.

In cc_set_password, something like:

def chpassword(plist_in, hashed=False):
    if util.which('chpasswd'):
 util.subp(['chpasswd'], plist_in)
    else:
 for u, p in plist_in.splitlines().split(":"):
     distro.set_passwd(u, p, hashed=hashed)

And then the two places in handle() which call util.subp(['chpasswd'])
will use

chpassword(plist_in)
chpassword(plist_in, hashed=True)

And the expire already uses passwd directly.

The result of the changes should pass unittest without any changes, except on the
FreeBSD path, we'd mock out the util.which('chpasswd') to False, taking us down the
path that calls distro.set_passwd() for each user:pass pair since there's no chpasswd
binary on FreeBSD.

Make sense?

review: Needs Fixing

[Gonéri Le Bouder (goneri)]

It sounds like you want to jumpback to versin #1. I initially started with a tiny patch with a limited impacted. Follow-up to the iterations of the review process, we end-up with this slightly more invasive change. I personally think it's a better patch since:

- there is now only one way to set the user password
- it resolves the chpasswd issue
- it removes > 40 lines of code and several variables.

[Ryan Harper (raharper)]

Gonéri ,

Yes. I'm sorry about the back and forth. I didn't understand the difference between chpasswd and passwd , the former operating on a list of passwords vs a single password. Thank for sticking with this work.

With that in mind; I worry about churn here; and I also don't like converting what was a single subprocess call to chpasswd to multiple calls to passwd. Obviously in the case where chpasswd is not available, we must use multiple calls to passwd.

So yes, I think something closer to your version 1, but I think we can abstract it in cc_set_password itself.

Something like this:

http://paste.ubuntu.com/p/xh5DmKvWy5/

[Gonéri Le Bouder (goneri)]

Thank for your feedback. There is another chpasswd binary on FreeBSD (https://www.freshports.org/www/chpasswd/) that is supposed to be compatible with the one Linux. But it's not maintained anymore and so, may not be as reliable as the official version. I've afraid the use of 'which' may create some lack of consitency in the plugins. Since it's pretty hard to troubleshoot Cloud-Init, I prefer to have something absolutely deterministic.

Also, depending on the format of the metadata, two different functions are used to set the password. This lack of consistency increases the complexity of the software (two functions for the same result) and result of subtile bugs. For instance, in the case of FreeBSD, one of the two strategy was working fine, the other was broken.

I understand you feel nervous with the change, but I'm convinced it will improve the quality of the code base. I suggest we try test it more intensivly instead. If you are willing to go in this direction, I will be happy to design some functional tests to validate the result on different operating system image. For instance, I already use Ansible locally to valide cloud-init behaviour.

[Andrey Fesenko (f0andrey)]

Wow
This Fix all mode cc_set_passwords (chpasswd:), hash, plain text, random generated.

I'm submit update in port FreeBSD
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=240372

[Gonéri Le Bouder (goneri)]

done

[Chad Smith (chad.smith)]

This looks good to me Gonéri, the only thing we 'lose' in terms of behavior:
 - visibility to log message distinguishing whether we are hashed or not
 - bailing on first set_passwd error instead of trying to set all and raising errors at the end

The second point I'd like to fix.

[Ryan Harper (raharper)]

Shouldn't we fix both? My biggest objection here was that
this path is rather critical and I hesitate to make changes.

The least we can do is ensure we have the same path on !FreeBSD
w.r.t behavior and message logs.

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:f653072bbbb21aac0330206982e84a171a78ab53
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1122/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1122//rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:9e18f6780cc4999cc31332e13ad2d22a4d27e8da
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1136/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1136//rebuild

review: Approve (continuous-integration)

[Andrey Fesenko (f0andrey)]

Logs in FreeBSD https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=240372#c4

review: Approve (it's now work)

[Ryan Harper (raharper)]

For non-FreeBSD, I don't understand why we would trade a single exec of chpasswd reading stdin list of users for an exec per-user.

IIUC, the previous patch I proposed:

http://paste.ubuntu.com/p/xh5DmKvWy5/

The objection was that util.which could find a chpasswd on freebsd which could be problematic.

If that were changed to use:

if not util.is_FreeBSD():
   # chpasswd path
else:
   distro.set_password()

Which would ensure that on the FreeBSD path it wouldn't use chpasswd even if present in the image. And for non-FreeBSD we keep the same code path we've had.

review: Needs Fixing

[Gonéri Le Bouder (goneri)]

> For non-FreeBSD, I don't understand why we would trade a single exec of chpasswd
> reading stdin list of users for an exec per-user.

cloud-init has two different way to set a password, this without any concrete benefit.
It's a lot of duplicated code and this creates case like this one where only one of the
two is broken. So, instead of adding an extra if/else block to deal with the current situation,
it sounds reasonable to me to just reduce this complexity.

[Chad Smith (chad.smith)]

Gonéri. Thanks for the continued discussion here.

Per your discussion points:

<Gonéri> cloud-init has two different way to set a password, this without any concrete benefit.
<chad> the concrete benefit is a single bulk operation vs single operations which involve the overhead of spinning up subprocess for each call.

<Gonéri> It's a lot of duplicated code and this creates case like this one where only one of the
two is broken. So, instead of adding an extra if/else block to deal with the current situation,
it sounds reasonable to me to just reduce this complexity.

<chad> yes, bulk operations is broken for freeBSD; but not on centos, sles, opensuse, rhel, ubuntu, debian, gentoo or arch. I'm hesitant to move all distribution behavior out of an optimal feature path because one distro doesn't have supported tooling. If we followed that policy in general, we could only have cloudinit perform only as good as the worst case scenario on any distro or cloud for any feature.

cloud-init would like distributions and individual clouds to be able to differentiate each other with various features and perform relatively better based on feature sets that each platform/distro has. So with that comes a bit of complexity. If there is a better way to do things on one cloud or distro, we want cloud-init to have the best experience there (at the cost of developer complexity).

As such, I've taken both Ryan's and your suggestions and tried to merge them so that bulk-chpasswd OSes can perform one operation and bsd can perform unique chpasswd calls per user.

Here is the attached diff, hopefully it tries to meet the intent of your branch:

https://paste.ubuntu.com/p/YP9yW864zT/

If we can agree on something close to this, I think we can merge it and resolve the bug.

review: Needs Information

[Gonéri Le Bouder (goneri)]

Thanks Chad! the patch works great.

[Mina Galić (minagalic)]

could we add ~chad.smith's change and get this merged?

review: Needs Fixing

[Gonéri Le Bouder (goneri)]

> could we add ~chad.smith's change and get this merged?

I would prefer if Chad just merge his patch directly. He owns the patch and it will be easier this way.

[Gonéri Le Bouder (goneri)]

Supersede by: https://github.com/canonical/cloud-init/pull/46

Unmerged commits

9e18f67... by Gonéri Le Bouder on 2019-06-01

set_passwords: support for FreeBSD

Add FreeBSD support to `cc_set_passwords`.
FreeBSD does not come with chpasswd binary, a chpasswd port[1] exists
but it's not the same tool than the one from Linux[2].

Finally, NetBSD and OpenBSD don't have the chpasswd binary at all, so
an abstraction layer will be needed.

This patch moves the Linux specific in the Distro class, this allow
anyone to override the default implementation. It also provides an
implementation for FreeBSD.

[1]: https://www.freshports.org/www/chpasswd/
[2]: https://github.com/shadow-maint/shadow/blob/master/src/chpasswd.c

diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
index cf9b5ab..9e6238a 100755
--- a/cloudinit/config/cc_set_passwords.py
+++ b/cloudinit/config/cc_set_passwords.py
@@ -166,74 +166,34 @@ def handle(_name, cfg, cloud, log, args):
         else:
             log.warn("No default or defined user to change password for.")
 
-    errors = []
+    random_msg = (
+        "Set the following 'random' password:\n"
+        "  '%s' to %s")
+
     if plist:
-        plist_in = []
-        hashed_plist_in = []
-        hashed_users = []
-        randlist = []
         users = []
         # N.B. This regex is included in the documentation (i.e. the module
         # docstring), so any changes to it should be reflected there.
         prog = re.compile(r'\$(1|2a|2y|5|6)(\$.+){2}')
         for line in plist:
-            u, p = line.split(':', 1)
-            if prog.match(p) is not None and ":" not in p:
-                hashed_plist_in.append("%s:%s" % (u, p))
-                hashed_users.append(u)
-            else:
-                if p == "R" or p == "RANDOM":
-                    p = rand_user_password()
-                    randlist.append("%s:%s" % (u, p))
-                plist_in.append("%s:%s" % (u, p))
-                users.append(u)
-
-        ch_in = '\n'.join(plist_in) + '\n'
-        if users:
             try:
-                log.debug("Changing password for %s:", users)
-                util.subp(['chpasswd'], ch_in)
-            except Exception as e:
-                errors.append(e)
-                util.logexc(
-                    log, "Failed to set passwords with chpasswd for %s", users)
-
-        hashed_ch_in = '\n'.join(hashed_plist_in) + '\n'
-        if hashed_users:
-            try:
-                log.debug("Setting hashed password for %s:", hashed_users)
-                util.subp(['chpasswd', '-e'], hashed_ch_in)
-            except Exception as e:
-                errors.append(e)
-                util.logexc(
-                    log, "Failed to set hashed passwords with chpasswd for %s",
-                    hashed_users)
-
-        if len(randlist):
-            blurb = ("Set the following 'random' passwords\n",
-                     '\n'.join(randlist))
-            sys.stderr.write("%s\n%s\n" % blurb)
-
-        if expire:
-            expired_users = []
-            for u in users:
-                try:
-                    util.subp(['passwd', '--expire', u])
-                    expired_users.append(u)
-                except Exception as e:
-                    errors.append(e)
-                    util.logexc(log, "Failed to set 'expire' for %s", u)
-            if expired_users:
-                log.debug("Expired passwords for: %s users", expired_users)
+                u, p = line.split(':', 1)
+                log.debug("Setting password for: %s" % u)
+                if prog.match(p) is not None and ":" not in p:
+                    cloud.distro.set_passwd(u, p, hashed=True, expire=expire)
+                elif p == "R" or p == "RANDOM":
+                        p = rand_user_password()
+                        sys.stderr.write(random_msg % (p, u))
+                        cloud.distro.set_passwd(u, p, expire=expire)
+                else:
+                    cloud.distro.set_passwd(u, p, expire=expire)
+            except Exception:
+                log.exception('failed to set password')
 
     handle_ssh_pwauth(
         cfg.get('ssh_pwauth'), service_cmd=cloud.distro.init_cmd,
         service_name=cloud.distro.get_option('ssh_svcname', 'ssh'))
 
-    if len(errors):
-        log.debug("%s errors occured, re-raising the last one", len(errors))
-        raise errors[-1]
-
 
 def rand_user_password(pwlen=9):
     return util.rand_str(pwlen, select_from=PW_SET)
diff --git a/cloudinit/config/tests/test_set_passwords.py b/cloudinit/config/tests/test_set_passwords.py
index a2ea5ec..274c253 100644
--- a/cloudinit/config/tests/test_set_passwords.py
+++ b/cloudinit/config/tests/test_set_passwords.py
@@ -101,11 +101,39 @@ class TestSetPasswordsHandle(CiTestCase):
             'DEBUG: Handling input for chpasswd as list.',
             self.logs.getvalue())
         self.assertIn(
-            "DEBUG: Setting hashed password for ['root', 'ubuntu']",
+            "DEBUG: Setting password for: root",
             self.logs.getvalue())
-        self.assertEqual(
-            [mock.call(['chpasswd', '-e'],
-             '\n'.join(valid_hashed_pwds) + '\n')],
+        self.assertIn(
+            "DEBUG: Setting password for: ubuntu",
+            self.logs.getvalue())
+        self.assertEqual([
+            mock.call(['chpasswd', '-e'], valid_hashed_pwds[0],
+                      logstring='chpasswd for root'),
+            mock.call(['passwd', '--expire', 'root']),
+            mock.call(['chpasswd', '-e'], valid_hashed_pwds[1],
+                      logstring='chpasswd for ubuntu'),
+            mock.call(['passwd', '--expire', 'ubuntu'])],
             m_subp.call_args_list)
 
+    @mock.patch(MODPATH + "util.subp")
+    def test_handle_on_broken_list(self, m_subp):
+        """Ensure the other password is set even if the first is broken."""
+        cloud = self.tmp_cloud(distro='ubuntu')
+        pwds = ['broken:a', 'continue:c']
+        cfg = {'chpasswd': {'list': pwds, 'expire': False}}
+        with mock.patch(MODPATH + 'util.subp') as m_subp:
+            def side_effect(*args, **kwargs):
+                if len(args) > 1 and args[1] == 'broken:a':
+                    raise Exception('Boom!')
+            m_subp.side_effect = side_effect
+            setpass.handle(
+                'IGNORED', cfg=cfg, cloud=cloud, log=self.logger, args=[])
+        self.assertIn(
+            "WARNING: Failed to set password for broken",
+            self.logs.getvalue())
+        self.assertIn(
+            "DEBUG: Setting password for: continue",
+            self.logs.getvalue())
+
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index 00bdee3..ff740b6 100644
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -591,7 +591,7 @@ class Distro(object):
             util.logexc(LOG, 'Failed to disable password for user %s', name)
             raise e
 
-    def set_passwd(self, user, passwd, hashed=False):
+    def set_passwd(self, user, passwd, hashed=False, expire=False):
         pass_string = '%s:%s' % (user, passwd)
         cmd = ['chpasswd']
 
@@ -607,8 +607,20 @@ class Distro(object):
             util.logexc(LOG, "Failed to set password for %s", user)
             raise e
 
+        if expire:
+            self._expire_password(user)
         return True
 
+    def _expire_password(self, user):
+        """
+        The users will have to enter a new password the first time they log in
+        """
+        try:
+            util.subp(['passwd', '--expire', user])
+        except Exception as e:
+            util.logexc(LOG, "Failed to set pw expiration for %s", user)
+            raise e
+
     def ensure_sudo_dir(self, path, sudo_base='/etc/sudoers'):
         # Ensure the dir is included and that
         # it actually exists as a directory
diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
index f7825fd..67cd42e 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -233,7 +233,7 @@ class Distro(distros.Distro):
         if passwd_val is not None:
             self.set_passwd(name, passwd_val, hashed=True)
 
-    def set_passwd(self, user, passwd, hashed=False):
+    def set_passwd(self, user, passwd, hashed=False, expire=False):
         if hashed:
             hash_opt = "-H"
         else:
@@ -246,6 +246,16 @@ class Distro(distros.Distro):
             util.logexc(LOG, "Failed to set password for %s", user)
             raise e
 
+        if expire:
+            self._expire_password(user)
+
+    def _expire_password(self, user):
+        try:
+            util.subp(['pw', 'usermod', user, '-p', '01-Jan-1970'])
+        except Exception as e:
+            util.logexc(LOG, "Failed to set pw expiration for %s", user)
+            raise e
+
     def lock_passwd(self, name):
         try:
             util.subp(['pw', 'usermod', name, '-h', '-'])
diff --git a/tools/build-on-freebsd b/tools/build-on-freebsd
index 8ae6456..876368a 100755
--- a/tools/build-on-freebsd
+++ b/tools/build-on-freebsd
@@ -18,7 +18,6 @@ py_prefix=$(${PYTHON} -c 'import sys; print("py%d%d" % (sys.version_info.major, 
 depschecked=/tmp/c-i.dependencieschecked
 pkgs="
     bash
-    chpasswd
     dmidecode
     e2fsprogs
     $py_prefix-Jinja2