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

Proposed by Gonéri Le Bouder on 2019-06-07
Status: Needs review
Proposed branch: ~goneri/cloud-init:freebsd_set_password
Merge into: cloud-init:master
Diff against target: 219 lines (+72/-63)
5 files modified
cloudinit/config/cc_set_passwords.py (+16/-56)
cloudinit/config/tests/test_set_passwords.py (+32/-4)
cloudinit/distros/__init__.py (+13/-1)
cloudinit/distros/freebsd.py (+11/-1)
tools/build-on-freebsd (+0/-1)
Reviewer Review Type Date Requested Status
Igor Galić (community) Needs Fixing on 2019-10-31
Chad Smith Needs Information on 2019-10-18
Ryan Harper 2019-06-07 Needs Fixing on 2019-10-17
Andrey Fesenko (community) it's now work Approve on 2019-10-11
Server Team CI bot continuous-integration Approve on 2019-09-12
Review via email: mp+368507@code.launchpad.net

Commit message

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

To post a comment you must log in.
Ryan Harper (raharper) wrote :

Thanks for the patch, some inline comments and questions.

review: Needs Fixing
Gonéri Le Bouder (goneri) wrote :

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

Ryan Harper (raharper) wrote :

Thanks, some inline comments.

review: Needs Fixing
Gonéri Le Bouder (goneri) wrote :

thanks, it should be fine now.

Ryan Harper (raharper) wrote :

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

Gonéri Le Bouder (goneri) wrote :

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

Ryan Harper (raharper) wrote :

OK, then do we need this branch at all ?

Ryan Harper (raharper) :
review: Needs Information
Gonéri Le Bouder (goneri) wrote :

Answered inline.

Gonéri Le Bouder (goneri) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

done

Chad Smith (chad.smith) wrote :

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) wrote :

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

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)

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) wrote :
review: Approve (it's now work)
Ryan Harper (raharper) wrote :

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) wrote :

> 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) wrote :

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) wrote :

Thanks Chad! the patch works great.

Igor Galić (i.galic) wrote :

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

review: Needs Fixing
Gonéri Le Bouder (goneri) wrote :

> 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.

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
2index cf9b5ab..9e6238a 100755
3--- a/cloudinit/config/cc_set_passwords.py
4+++ b/cloudinit/config/cc_set_passwords.py
5@@ -166,74 +166,34 @@ def handle(_name, cfg, cloud, log, args):
6 else:
7 log.warn("No default or defined user to change password for.")
8
9- errors = []
10+ random_msg = (
11+ "Set the following 'random' password:\n"
12+ " '%s' to %s")
13+
14 if plist:
15- plist_in = []
16- hashed_plist_in = []
17- hashed_users = []
18- randlist = []
19 users = []
20 # N.B. This regex is included in the documentation (i.e. the module
21 # docstring), so any changes to it should be reflected there.
22 prog = re.compile(r'\$(1|2a|2y|5|6)(\$.+){2}')
23 for line in plist:
24- u, p = line.split(':', 1)
25- if prog.match(p) is not None and ":" not in p:
26- hashed_plist_in.append("%s:%s" % (u, p))
27- hashed_users.append(u)
28- else:
29- if p == "R" or p == "RANDOM":
30- p = rand_user_password()
31- randlist.append("%s:%s" % (u, p))
32- plist_in.append("%s:%s" % (u, p))
33- users.append(u)
34-
35- ch_in = '\n'.join(plist_in) + '\n'
36- if users:
37 try:
38- log.debug("Changing password for %s:", users)
39- util.subp(['chpasswd'], ch_in)
40- except Exception as e:
41- errors.append(e)
42- util.logexc(
43- log, "Failed to set passwords with chpasswd for %s", users)
44-
45- hashed_ch_in = '\n'.join(hashed_plist_in) + '\n'
46- if hashed_users:
47- try:
48- log.debug("Setting hashed password for %s:", hashed_users)
49- util.subp(['chpasswd', '-e'], hashed_ch_in)
50- except Exception as e:
51- errors.append(e)
52- util.logexc(
53- log, "Failed to set hashed passwords with chpasswd for %s",
54- hashed_users)
55-
56- if len(randlist):
57- blurb = ("Set the following 'random' passwords\n",
58- '\n'.join(randlist))
59- sys.stderr.write("%s\n%s\n" % blurb)
60-
61- if expire:
62- expired_users = []
63- for u in users:
64- try:
65- util.subp(['passwd', '--expire', u])
66- expired_users.append(u)
67- except Exception as e:
68- errors.append(e)
69- util.logexc(log, "Failed to set 'expire' for %s", u)
70- if expired_users:
71- log.debug("Expired passwords for: %s users", expired_users)
72+ u, p = line.split(':', 1)
73+ log.debug("Setting password for: %s" % u)
74+ if prog.match(p) is not None and ":" not in p:
75+ cloud.distro.set_passwd(u, p, hashed=True, expire=expire)
76+ elif p == "R" or p == "RANDOM":
77+ p = rand_user_password()
78+ sys.stderr.write(random_msg % (p, u))
79+ cloud.distro.set_passwd(u, p, expire=expire)
80+ else:
81+ cloud.distro.set_passwd(u, p, expire=expire)
82+ except Exception:
83+ log.exception('failed to set password')
84
85 handle_ssh_pwauth(
86 cfg.get('ssh_pwauth'), service_cmd=cloud.distro.init_cmd,
87 service_name=cloud.distro.get_option('ssh_svcname', 'ssh'))
88
89- if len(errors):
90- log.debug("%s errors occured, re-raising the last one", len(errors))
91- raise errors[-1]
92-
93
94 def rand_user_password(pwlen=9):
95 return util.rand_str(pwlen, select_from=PW_SET)
96diff --git a/cloudinit/config/tests/test_set_passwords.py b/cloudinit/config/tests/test_set_passwords.py
97index a2ea5ec..274c253 100644
98--- a/cloudinit/config/tests/test_set_passwords.py
99+++ b/cloudinit/config/tests/test_set_passwords.py
100@@ -101,11 +101,39 @@ class TestSetPasswordsHandle(CiTestCase):
101 'DEBUG: Handling input for chpasswd as list.',
102 self.logs.getvalue())
103 self.assertIn(
104- "DEBUG: Setting hashed password for ['root', 'ubuntu']",
105+ "DEBUG: Setting password for: root",
106 self.logs.getvalue())
107- self.assertEqual(
108- [mock.call(['chpasswd', '-e'],
109- '\n'.join(valid_hashed_pwds) + '\n')],
110+ self.assertIn(
111+ "DEBUG: Setting password for: ubuntu",
112+ self.logs.getvalue())
113+ self.assertEqual([
114+ mock.call(['chpasswd', '-e'], valid_hashed_pwds[0],
115+ logstring='chpasswd for root'),
116+ mock.call(['passwd', '--expire', 'root']),
117+ mock.call(['chpasswd', '-e'], valid_hashed_pwds[1],
118+ logstring='chpasswd for ubuntu'),
119+ mock.call(['passwd', '--expire', 'ubuntu'])],
120 m_subp.call_args_list)
121
122+ @mock.patch(MODPATH + "util.subp")
123+ def test_handle_on_broken_list(self, m_subp):
124+ """Ensure the other password is set even if the first is broken."""
125+ cloud = self.tmp_cloud(distro='ubuntu')
126+ pwds = ['broken:a', 'continue:c']
127+ cfg = {'chpasswd': {'list': pwds, 'expire': False}}
128+ with mock.patch(MODPATH + 'util.subp') as m_subp:
129+ def side_effect(*args, **kwargs):
130+ if len(args) > 1 and args[1] == 'broken:a':
131+ raise Exception('Boom!')
132+ m_subp.side_effect = side_effect
133+ setpass.handle(
134+ 'IGNORED', cfg=cfg, cloud=cloud, log=self.logger, args=[])
135+ self.assertIn(
136+ "WARNING: Failed to set password for broken",
137+ self.logs.getvalue())
138+ self.assertIn(
139+ "DEBUG: Setting password for: continue",
140+ self.logs.getvalue())
141+
142+
143 # vi: ts=4 expandtab
144diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
145index 00bdee3..ff740b6 100644
146--- a/cloudinit/distros/__init__.py
147+++ b/cloudinit/distros/__init__.py
148@@ -591,7 +591,7 @@ class Distro(object):
149 util.logexc(LOG, 'Failed to disable password for user %s', name)
150 raise e
151
152- def set_passwd(self, user, passwd, hashed=False):
153+ def set_passwd(self, user, passwd, hashed=False, expire=False):
154 pass_string = '%s:%s' % (user, passwd)
155 cmd = ['chpasswd']
156
157@@ -607,8 +607,20 @@ class Distro(object):
158 util.logexc(LOG, "Failed to set password for %s", user)
159 raise e
160
161+ if expire:
162+ self._expire_password(user)
163 return True
164
165+ def _expire_password(self, user):
166+ """
167+ The users will have to enter a new password the first time they log in
168+ """
169+ try:
170+ util.subp(['passwd', '--expire', user])
171+ except Exception as e:
172+ util.logexc(LOG, "Failed to set pw expiration for %s", user)
173+ raise e
174+
175 def ensure_sudo_dir(self, path, sudo_base='/etc/sudoers'):
176 # Ensure the dir is included and that
177 # it actually exists as a directory
178diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
179index f7825fd..67cd42e 100644
180--- a/cloudinit/distros/freebsd.py
181+++ b/cloudinit/distros/freebsd.py
182@@ -233,7 +233,7 @@ class Distro(distros.Distro):
183 if passwd_val is not None:
184 self.set_passwd(name, passwd_val, hashed=True)
185
186- def set_passwd(self, user, passwd, hashed=False):
187+ def set_passwd(self, user, passwd, hashed=False, expire=False):
188 if hashed:
189 hash_opt = "-H"
190 else:
191@@ -246,6 +246,16 @@ class Distro(distros.Distro):
192 util.logexc(LOG, "Failed to set password for %s", user)
193 raise e
194
195+ if expire:
196+ self._expire_password(user)
197+
198+ def _expire_password(self, user):
199+ try:
200+ util.subp(['pw', 'usermod', user, '-p', '01-Jan-1970'])
201+ except Exception as e:
202+ util.logexc(LOG, "Failed to set pw expiration for %s", user)
203+ raise e
204+
205 def lock_passwd(self, name):
206 try:
207 util.subp(['pw', 'usermod', name, '-h', '-'])
208diff --git a/tools/build-on-freebsd b/tools/build-on-freebsd
209index 8ae6456..876368a 100755
210--- a/tools/build-on-freebsd
211+++ b/tools/build-on-freebsd
212@@ -18,7 +18,6 @@ py_prefix=$(${PYTHON} -c 'import sys; print("py%d%d" % (sys.version_info.major,
213 depschecked=/tmp/c-i.dependencieschecked
214 pkgs="
215 bash
216- chpasswd
217 dmidecode
218 e2fsprogs
219 $py_prefix-Jinja2

Subscribers

People subscribed via source and target branches