ssh_util: Handle sshd_config.d folder #1618

aciba90 · 2022-07-25T17:37:57Z

Proposed Commit Message

ssh_util: Handle sshd_config.d folder

Write sshd config to /etc/ssh/sshd_config.d/00-cloud-init.conf
if the sshd_config sources sshd_config.d

LP: #1968873

Additional Context

SC-1172
LP: #1968873

In order to ease upgrading openssh-server, write out sshd_config
into a separated cloud-init conf file with high priority under
/etc/ssh/sshd_config.d/00-cloud-init.conf.

This fixes #1968873 in main and will be SRUed to focal, impish
and focal.

Test Steps

cat <<EOF > /tmp/user-data
#cloud-config
ssh_pwauth: true
password: asdf
EOF

lxc launch ubuntu:focal ff --config=user.user-data="$(cat /tmp/user-data)"
lxc exec ff -- cloud-init status --wait

$ lxc exec ff -- grep -rn 'PasswordAuthentication ' /etc/ssh/sshd_config /etc/ssh/sshd_config.d
/etc/ssh/sshd_config:58:PasswordAuthentication no
/etc/ssh/sshd_config.d/00-cloud-init.conf:1:PasswordAuthentication yes

# Previously
# /etc/ssh/sshd_config:58:PasswordAuthentication yes

$ lxc exec ff -- grep -ri 'Writing to /etc/ssh/sshd_config - wb' /var/log/cloud-init.log
# Expect no output

# Previously:
# 2022-07-26 10:04:33,269 - util.py[DEBUG]: Writing to /etc/ssh/sshd_config - wb: [644] 3288 bytes

$ lxc exec ff -- grep -ri 'Writing to /etc/ssh/sshd_config.d' /var/log/cloud-init.log
2022-07-26 09:51:10,076 - util.py[DEBUG]: Writing to /etc/ssh/sshd_config.d/00-cloud-init.conf - ab: [644] 0 bytes
2022-07-26 09:51:10,076 - util.py[DEBUG]: Writing to /etc/ssh/sshd_config.d/00-cloud-init.conf - wb: [644] 27 bytes


lxc exec ff -- apt-get udpate
lxc exec ff -- apt-get upgrade -y
$ lxc exec ff -- do-release-upgrade -d
# A prompt does appear:

# A new version (/tmp/tmp.dAvrCWldyw) of configuration file /etc/ssh/sshd_config is available, but the version installed currently has been locally modified.
#
# What do you want to do about modified configuration file sshd_config?
# ...

# click on show diff
# the diff must not contain a diff with `PasswordAuthentication`

# Note: `/etc/ssh/sshd_config` still has modifications but not made by `cloud-init`.

lxc exec ff -- apt-get purge -y cloud-init
$ lxc exec ff -- ls /etc/ssh/sshd_config.d/00-cloud-init.conf 
ls: cannot access '/etc/ssh/sshd_config.d/00-cloud-init.conf': No such file or directory

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

aciba90 · 2022-07-26T08:36:48Z

FYI, @toabctl, here is the sshd_config fix.

aciba90 · 2022-07-26T08:47:53Z

There is another reference to /etc/ssh/sshd_config in tools/uncloud-init#L126.

I guess that tool is a development tool and in order to minimize the SRU patch, we can defer its fix to a follow-up PR. Is that right? @blackboxsw, @holmanb, @TheRealFalcon.

aciba90 · 2022-07-26T14:47:22Z

There is another reference to /etc/ssh/sshd_config in tools/uncloud-init#L126.

I guess that tool is a development tool and in order to minimize the SRU patch, we can defer its fix to a follow-up PR. Is that right? @blackboxsw, @holmanb, @TheRealFalcon.

After a team discussion, we decided to not extend /tools/uncloud-init as it had no activity since 3 years and might be removed.

What we want to extend is the post-remove deb hook during purge to clean-up 99-cloud-init.conf. I mark this PR as Draft until done.

aciba90 · 2022-07-26T18:20:14Z

packages/debian/cloud-init.postrm added and tested. This PR is ready to be reviewed.

cloudinit/ssh_util.py

toabctl

LGTM. thanks!

blackboxsw

This looks really good @aciba90 thank you much for putting this together.
Minor comments on:

file permissions on the config parts file
using prefix 50-cloud-init.conf
integration tests for the file emitted

otherwise +1.

cloudinit/ssh_util.py

packages/debian/cloud-init.postrm

tests/integration_tests/modules/test_set_password.py

tests/integration_tests/modules/test_ssh_keys_provided.py

tests/unittests/config/test_cc_ssh.py

- Extend integration tests - Use 50-cloud-init.conf as sshd config file - with 0o600 permissions

aciba90 · 2022-08-05T10:17:58Z

This looks really good @aciba90 thank you much for putting this together. Minor comments on:
* file permissions on the config parts file

* using prefix 50-cloud-init.conf

* integration tests for the file emitted
otherwise +1.

Addressed. Thanks, @blackboxsw, for the review and constructive comments. This is ready to be reviewed.

blackboxsw · 2022-08-05T14:42:51Z

FYI: @stevebeattie & @setharnold for general awareness of security-related behavior changes in cloud-init sshd_config.d.

blackboxsw

LGTM. Thanks for the permissions fixes @aciba90. I'm good to land this EOD today.

aciba90 · 2022-08-08T14:45:38Z

Is the idea to allow supporting multiple sshds on a system? eg, one for PAM-based authentication on usual port 22, and one for unprivileged access on another port without PAM, just checking a key? or perhaps different PAM configurations? I'm not entirely sure I see the use-cases, but so long as you're content that only trusted inputs are supplied, so eg fname isn't set to ../../../../etc/nologin or similar, it all looked fine enough.

Thanks

The idea is to not modify /etc/ssh/shhd_config file in systems where /etc/ssh/shhd_config does source /etc/ssh/shhd_config.d/ (focal, jammy, kinetic), because this file is distributed via open-sshd package and want users to have a seamless do-release-upgrade process.

In respect to fname, we define it as /etc/ssh/sshd_config/50-cloud-init.conf and the function is for internal usage only.

setharnold · 2022-08-08T20:13:39Z

Okay, cool, so it's impossible for untrusted code to ever call update_ssh_config(foo, fname="../../../etc/logrotate") for example?

Thanks

aciba90 · 2022-08-10T18:08:30Z

Okay, cool, so it's impossible for untrusted code to ever call update_ssh_config(foo, fname="../../../etc/logrotate") for example?

Thanks

Well, we would have to define what is untrusted code to answer that. What is clear is that this PR does not introduce new attack vectors, as the function already accepted fname as a pointer to a file to write config in. We are just extending that function to verify the case where a .d folder exists. Please, let me know any concern / clarification. Thanks.

holmanb · 2022-08-10T20:30:43Z

@aciba90 @setharnold

Okay, cool, so it's impossible for untrusted code to ever call update_ssh_config(foo, fname="../../../etc/logrotate") for example?

I think the answer to this is technically "no it is not impossible". A (root) user could technically write a 3rd party module (not trusted, by some definitions) that uses this function, insert the module into a root-only writable directory on the fs where config modules live, configure cloud-init to load this module (in their root-only writable cloud.cfg), and cloud-init would would technically run untrusted code that calls update_ssh_config(foo, fname="../../../etc/logrotate").

However this isn't an escalation issue because every step of that has Posix permissions preventing a non-root user from doing it.

So I agree with what @aciba90 said: it depends on how you define untrusted. And I don't think this change poses a problem, because in the default case the code executing this is trusted.

setharnold · 2022-08-11T00:11:21Z

I think I was thinking more along the lines of an ISP-supplied "easy deployment" sort of web interface that might take a "machine name" or "service name" sort of field and use it in surprising ways.

If you're content then I'm content :D Thanks!

ani-sinha · 2023-08-08T11:47:20Z

re: /etc/ssh/sshd_config.d/50-cloud-init.conf generated by cloud-init I see it is removed using packages/debian/cloud-init.postrm . Since the package manager does not install the conf, could we not have cleaned it up as a part of "cloud-init clean" operation?

ani-sinha · 2023-08-08T11:48:08Z

The reason I ask is that for centos/rhel, if we took similar approach, we would have to remove it from the rpm spec file. Instead we could have a common place to remove such conf files, @blackboxsw

ani-sinha · 2023-08-08T11:51:30Z

The reason I ask is that for centos/rhel, if we took similar approach, we would have to remove it from the rpm spec file. Instead we could have a common place to remove such conf files, @blackboxsw

For example, we could we not remove it as a part of cloud-init clean or some such? Today it removes only log files.

aciba90 · 2023-08-22T13:38:18Z

Thanks, @ani-sinha, for making cloud-init better.

I think it is reasonable to expect cloud-init clean to remove the file. Could you please create an issue referencing this PR?

Another instance of this problem with a different file: #3240

ani-sinha · 2023-08-22T14:00:28Z

Thanks, @ani-sinha, for making cloud-init better.

I think it is reasonable to expect cloud-init clean to remove the file. Could you please create an issue referencing this PR?

Another instance of this problem with a different file: #3240

I have some reservations about this after thinking. When cloud-init clean removes log files its harmless. But if config files that are produced by cloud-init are removed, will that create some inconsistency particularly if cloud-init init does not recreate them?

aciba90 · 2023-08-22T14:10:25Z

Per cli.html#clean and #3240 (comment), I understand that files that are dynamically created by cloud-init at runtime are okay to be removed by cloud-init clean.

But if you think this in not required, then do not create the issue, thanks.

ani-sinha · 2023-08-22T14:30:13Z

Per cli.html#clean and #3240 (comment), I understand that files that are dynamically created by cloud-init at runtime are okay to be removed by cloud-init clean.

But if you think this in not required, then do not create the issue, thanks.

Right now the clean command only cleans artefacts from ’/var/lib/cloud’ and the cli documentation also says so. Adding config files to it would be something new and I am not sure if it's safe. If we do this the cli documentation also needs updating. Hence I need inputs from cloud-init maintainers . I would be happy to send a PR if it seems ok to do it .

aciba90 · 2023-08-22T15:06:01Z

By safe, do you mean software security? cloud-init clean would be able to remove the conf file only if the user executing the command has the appropriated permissions to do so, and if that is true, then the user could remove / modify that file by other means.

If we do this the cli documentation also needs updating. Hence I need inputs from cloud-init maintainers.

Yes, the documentation should be updated in that case. Something like:

Remove cloud-init artifacts from /var/lib/cloud and config files (best effort) to simulate a clean instance. On reboot, cloud-init will re-run all stages as it did on first boot.

ani-sinha · 2023-08-22T15:26:15Z

By safe, do you mean software security?

No what I mean is that there might be some inconsistency if ‘cloud-init init‘ doesn't recreate all the config files that were present before cleaning. The inconsistency might break cloud-init functionality.

ani-sinha · 2023-08-23T05:55:38Z

By safe, do you mean software security?

No what I mean is that there might be some inconsistency if ‘cloud-init init‘ doesn't recreate all the config files that were present before cleaning. The inconsistency might break cloud-init functionality.

@blackboxsw what do you think?

cloud-init generates several config files today that are not cleaned as a part of `cloud-init clean` operation. For example, cloud-init uses /etc/ssh/sshd_config.d/50-cloud-init.conf to configure ssh daemon which is then left uncleaned. This change adds a new option `cloud-init clean --config` that would clean up these config files as well. Additionally, the cli documentation has also been updated to document this new option. Reference: canonical#1618 Fixes: canonicalGH-3240 Signed-off-by: Ani Sinha <anisinha@redhat.com>

cloud-init generates several config files today that are not cleaned as a part of `cloud-init clean` operation. For example, cloud-init uses `/etc/ssh/sshd_config.d/50-cloud-init.conf` to configure ssh daemon which is then left uncleaned. This change adds a new option `cloud-init clean --config` that would clean up these config files as well. Additionally, the cli documentation has also been updated to document this new option. Reference: canonical#1618 Fixes: canonicalGH-3240 Signed-off-by: Ani Sinha <anisinha@redhat.com>

cloud-init generates several config files today that are not cleaned as a part of `cloud-init clean` operation. For example, cloud-init uses `/etc/ssh/sshd_config.d/50-cloud-init.conf` to configure ssh daemon which is then left uncleaned. This change adds a new option `cloud-init clean --configs` that would clean up these config files as well. Additionally, the cli documentation has also been updated to document this new option. Reference: canonical#1618 Fixes: canonicalGH-3240 Signed-off-by: Ani Sinha <anisinha@redhat.com>

blackboxsw · 2023-08-25T14:07:01Z

By safe, do you mean software security?

No what I mean is that there might be some inconsistency if ‘cloud-init init‘ doesn't recreate all the config files that were present before cleaning. The inconsistency might break cloud-init functionality.

@blackboxsw what do you think?

@ani-sinha thanks for the ping on this issue. I think the functionality of cloud-init clean is to provide a best effort to "reset" cloud-init to initial state so that cloud-init treats next boot of that image as a first initialization or "greenfield" boot. While the clean subcommand doesn't currently make an effort to remove any configuration artifacts produced by the previous run (such as user creation, ssh keys, ssh config, files written by write_files) I could see us trying to grow that optional functionality.
The reason I don't want cloud-init clean to purge any artifacts produced by various cc_*.py config modules is because I know of a few use-cases where individuals use two stage configuration to create reusable base images. They'll launch an image with cloud-init user-data in stage one to setup a common base image with base users,services package installs. I believe some packer installs also take this approach to creating a base image for reuse. Then that "cleaned" base image is uploaded to cloud/platform of choice and launched with new user-data to finalize configuration on vsphere or your favorite platform. If we grow cloud-init clean to be more 'destructive to previous config artifacts' let's make this optional/supplemental behavior and find a way to make it conspicuous.

ani-sinha · 2023-08-29T06:51:35Z

By safe, do you mean software security?

No what I mean is that there might be some inconsistency if ‘cloud-init init‘ doesn't recreate all the config files that were present before cleaning. The inconsistency might break cloud-init functionality.

@blackboxsw what do you think?

@ani-sinha thanks for the ping on this issue. I think the functionality of cloud-init clean is to provide a best effort to "reset" cloud-init to initial state so that cloud-init treats next boot of that image as a first initialization or "greenfield" boot. While the clean subcommand doesn't currently make an effort to remove any configuration artifacts produced by the previous run (such as user creation, ssh keys, ssh config, files written by write_files) I could see us trying to grow that optional functionality. The reason I don't want cloud-init clean to purge any artifacts produced by various cc_*.py config modules is because I know of a few use-cases where individuals use two stage configuration to create reusable base images. They'll launch an image with cloud-init user-data in stage one to setup a common base image with base users,services package installs. I believe some packer installs also take this approach to creating a base image for reuse. Then that "cleaned" base image is uploaded to cloud/platform of choice and launched with new user-data to finalize configuration on vsphere or your favorite platform. If we grow cloud-init clean to be more 'destructive to previous config artifacts' let's make this optional/supplemental behavior and find a way to make it conspicuous.

OK I have added a new option under cloud-init clean which is optional. Also updated the documentation for the same which should make it conspicuous. See #4384 .

cloud-init generates several config files today that are not cleaned as a part of `cloud-init clean` operation. For example, cloud-init uses `/etc/ssh/sshd_config.d/50-cloud-init.conf` to configure ssh daemon which is then left uncleaned. This change adds a new option `cloud-init clean --configs` that would clean up these config files as well. This new option provides two choices - `ssh_config` and `network` that defines the scope of the config files that would be cleaned - ssh configs or network configs. Additionally, the cli documentation has also been updated to document this new option. Reference: canonical#1618 Fixes: canonicalGH-3240 Signed-off-by: Ani Sinha <anisinha@redhat.com>

cloud-init generates several config files today that are not cleaned as a part of `cloud-init clean` operation. For example, cloud-init uses `/etc/ssh/sshd_config.d/50-cloud-init.conf` to configure ssh daemon which is then left uncleaned. This change adds a new option `cloud-init clean --configs` that would clean up these config files as well. This new option provides two choices - `ssh_config` and `network` that defines the scope of the config files that would be cleaned - ssh configs or network configs. They can be extended further to include other types of config files as well in the future. Additionally, the cli documentation has also been updated to document this new option. Reference: canonical#1618 Fixes: canonicalGH-3240 Signed-off-by: Ani Sinha <anisinha@redhat.com>

cloud-init generates several config files today that are not cleaned as a part of `cloud-init clean` operation. For example, cloud-init uses `/etc/ssh/sshd_config.d/50-cloud-init.conf` to configure ssh daemon which is then left uncleaned. This change adds a new option `cloud-init clean --configs` that would clean up these config files as well. This new option provides three choices - `all`, `ssh_config` and `network` that defines the scope of the config files that would be cleaned - ssh configs or network configs or all configs. They can be extended further to include other types of config files as well in the future. Additionally, the cli documentation has also been updated to document this new option. Reference: canonical#1618 Fixes: canonicalGH-3240 Signed-off-by: Ani Sinha <anisinha@redhat.com> Co-authored-by: Chad Smith <chad.smith@canonical.com>

cloud-init generates several config files today that are not cleaned as a part of `cloud-init clean` operation. For example, cloud-init uses `/etc/ssh/sshd_config.d/50-cloud-init.conf` to configure ssh daemon which is then left uncleaned. This change adds a new option `cloud-init clean --configs` that would clean up these config files as well. This new option provides three choices - `all`, `ssh_config` and `network` that defines the scope of the config files that would be cleaned - ssh configs or network configs or all configs. They can be extended further to include other types of config files as well in the future. The cli documentation has also been updated to document this new option. Additionally, this patch adds a new integration test for the `clean` command. Reference: canonical#1618 Fixes: canonicalGH-3240 Signed-off-by: Ani Sinha <anisinha@redhat.com> Co-authored-by: Chad Smith <chad.smith@canonical.com>

cloud-init generates several config files today that are not cleaned as a part of `cloud-init clean` operation. For example, cloud-init uses `/etc/ssh/sshd_config.d/50-cloud-init.conf` to configure ssh daemon which is then left uncleaned. This change adds a new option `cloud-init clean --configs` that would clean up these config files as well. This new option provides three choices - `all`, `ssh_config` and `network` that defines the scope of the config files that would be cleaned - ssh configs or network configs or all configs. They can be extended further to include other types of config files as well in the future. The cli documentation has also been updated to document this new option. Additionally, this patch adds a new integration test for the `clean` command. Reference: #1618 Fixes GH-3240 Signed-off-by: Ani Sinha <anisinha@redhat.com> Co-authored-by: Chad Smith <chad.smith@canonical.com>

aciba90 changed the title ~~Sshd~~ ssh_util: Handle sshd_config.d folder Jul 25, 2022

aciba90 force-pushed the sshd branch 2 times, most recently from 7d45889 to 31b38c6 Compare July 26, 2022 07:57

aciba90 marked this pull request as ready for review July 26, 2022 07:58

aciba90 marked this pull request as draft July 26, 2022 14:47

aciba90 marked this pull request as ready for review July 26, 2022 18:19

toabctl suggested changes Jul 26, 2022

View reviewed changes

cloudinit/ssh_util.py Outdated Show resolved Hide resolved

toabctl suggested changes Jul 27, 2022

View reviewed changes

cloudinit/ssh_util.py Outdated Show resolved Hide resolved

aciba90 force-pushed the sshd branch from e217069 to c05e8f7 Compare July 27, 2022 09:02

toabctl reviewed Jul 27, 2022

View reviewed changes

blackboxsw self-assigned this Aug 4, 2022

blackboxsw requested changes Aug 4, 2022

View reviewed changes

aciba90 added 7 commits August 5, 2022 12:01

ssh_util: Handle sshd_config.d folder

ed756e8

Improve unit testing

0a9df42

Add postrm purging sshd_config

8d6630c

Fix commented include case

c291c79

Give more priority to cloud-init sshd config

55063e7

Simplify integration tests

51e4add

PR comments

4a8805a

- Extend integration tests - Use 50-cloud-init.conf as sshd config file - with 0o600 permissions

aciba90 force-pushed the sshd branch from 86b2efc to 4a8805a Compare August 5, 2022 10:11

aciba90 requested review from blackboxsw and toabctl August 5, 2022 10:18

blackboxsw added the security label Aug 5, 2022

blackboxsw approved these changes Aug 5, 2022

View reviewed changes

aciba90 deleted the sshd branch August 8, 2022 14:45

ani-sinha mentioned this pull request Aug 24, 2023

clean: add a new option to clean generated config files #4384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ssh_util: Handle sshd_config.d folder #1618

ssh_util: Handle sshd_config.d folder #1618

aciba90 commented Jul 25, 2022 •

edited

aciba90 commented Jul 26, 2022

aciba90 commented Jul 26, 2022

aciba90 commented Jul 26, 2022

aciba90 commented Jul 26, 2022

toabctl left a comment

blackboxsw left a comment

aciba90 commented Aug 5, 2022

blackboxsw commented Aug 5, 2022 •

edited

blackboxsw left a comment

aciba90 commented Aug 8, 2022

setharnold commented Aug 8, 2022

aciba90 commented Aug 10, 2022

holmanb commented Aug 10, 2022

setharnold commented Aug 11, 2022

ani-sinha commented Aug 8, 2023

ani-sinha commented Aug 8, 2023

ani-sinha commented Aug 8, 2023

aciba90 commented Aug 22, 2023

ani-sinha commented Aug 22, 2023

aciba90 commented Aug 22, 2023

ani-sinha commented Aug 22, 2023

aciba90 commented Aug 22, 2023

ani-sinha commented Aug 22, 2023

ani-sinha commented Aug 23, 2023

blackboxsw commented Aug 25, 2023

ani-sinha commented Aug 29, 2023

ssh_util: Handle sshd_config.d folder #1618

ssh_util: Handle sshd_config.d folder #1618

Conversation

aciba90 commented Jul 25, 2022 • edited

Proposed Commit Message

Additional Context

Test Steps

Checklist:

aciba90 commented Jul 26, 2022

aciba90 commented Jul 26, 2022

aciba90 commented Jul 26, 2022

aciba90 commented Jul 26, 2022

toabctl left a comment

Choose a reason for hiding this comment

blackboxsw left a comment

Choose a reason for hiding this comment

aciba90 commented Aug 5, 2022

blackboxsw commented Aug 5, 2022 • edited

blackboxsw left a comment

Choose a reason for hiding this comment

aciba90 commented Aug 8, 2022

setharnold commented Aug 8, 2022

aciba90 commented Aug 10, 2022

holmanb commented Aug 10, 2022

setharnold commented Aug 11, 2022

ani-sinha commented Aug 8, 2023

ani-sinha commented Aug 8, 2023

ani-sinha commented Aug 8, 2023

aciba90 commented Aug 22, 2023

ani-sinha commented Aug 22, 2023

aciba90 commented Aug 22, 2023

ani-sinha commented Aug 22, 2023

aciba90 commented Aug 22, 2023

ani-sinha commented Aug 22, 2023

ani-sinha commented Aug 23, 2023

blackboxsw commented Aug 25, 2023

ani-sinha commented Aug 29, 2023

aciba90 commented Jul 25, 2022 •

edited

blackboxsw commented Aug 5, 2022 •

edited