Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support ssh configuration logic through sshd_config.d on google backend #155

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sergiocazzolato
Copy link
Contributor

@sergiocazzolato sergiocazzolato commented Sep 29, 2022

Some images (like ubuntu kinetic) override the ssh configuration by using the new sshd_config.d directory and no longer ship a /etc/sshd_config file.

This change makes sure these systems will have the needed shell configuration to run spread.

As the first value wins, we need to use 00- for the configuration file to make sure this is the first file with that config

Some images (like ubuntu kinetic) override the ssh configuration by
using the .d logic

This change makes sure these systems will have the needed sh
configuration to run spread.

As the first value wins, we need to use 00 for the confiugartion file to
make sure this is the first file with that config
@sergiocazzolato sergiocazzolato changed the title support ssh configuration logic through sshd_config.d support ssh configuration logic through sshd_config.d on google backend Sep 29, 2022
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing spread with Ubuntu 22.10!

spread/google.go Outdated
@@ -154,6 +154,7 @@ const googleStartupScript = `
echo root:%s | chpasswd

sed -i 's/^\s*#\?\s*\(PermitRootLogin\|PasswordAuthentication\)\>.*/\1 yes/' /etc/ssh/sshd_config
test -d /etc/ssh/sshd_config.d && echo -e 'PermitRootLogin=yes\nPasswordAuthentication=yes' > /etc/ssh/sshd_config.d/00-spread-settings.conf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Juerg just pointed that we probably need the above tweak in client.go:SetupRootAccess() too and probably also in lxd.go ?

@mardy
Copy link

mardy commented Oct 10, 2022

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look reasonable. Only two comments:

  1. What happens if there are other entries in that directory which list these options? Is it last match wins, or first match wins? Should we comment them out first? How about this: if the directory exists, comment out the options in all files that exist there, and then add a new file with our options.

  2. The -settings is redundant with the .conf, and given (1) above, I think we can name this nicely as just "spread.conf", assuming that's the pattern there.

@sergiocazzolato
Copy link
Contributor Author

Updated the change.

Comemnt the lines in case there is a file with the PermitRootLogin or
PasswordAuthentication. As the command could fail when there is not
files in sshd_config.d dir it is added a || true to make sure the script
is not going to fail because of that.

Also it is being updated the name of the config file
Comment on lines +157 to +159
sed -i 's/^PermitRootLogin=/#PermitRootLogin=/g' /etc/ssh/sshd_config.d/* || true
sed -i 's/^PasswordAuthentication=/#PasswordAuthentication=/g' /etc/ssh/sshd_config.d/* || true
test -d /etc/ssh/sshd_config.d && echo -e 'PermitRootLogin=yes\nPasswordAuthentication=yes' > /etc/ssh/sshd_config.d/00-spread.conf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the sshd_config(5) manpage, the config file usually doesn't have a = between the key/value pairs. For example:

% grep = /etc/ssh/sshd_config
# This sshd was compiled with PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games

Comment on lines +157 to +158
sed -i 's/^PermitRootLogin=/#PermitRootLogin=/g' /etc/ssh/sshd_config.d/* || true
sed -i 's/^PasswordAuthentication=/#PasswordAuthentication=/g' /etc/ssh/sshd_config.d/* || true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config files might have whitespace before the the entry. also as @thp-canonical mentioned, the entry usually doesn't have a =.

Suggested change
sed -i 's/^PermitRootLogin=/#PermitRootLogin=/g' /etc/ssh/sshd_config.d/* || true
sed -i 's/^PasswordAuthentication=/#PasswordAuthentication=/g' /etc/ssh/sshd_config.d/* || true
sed -i 's/^\s*\(PermitRootLogin\|PasswordAuthentication\)\>.*/# COMMENTED OUT BY SPREAD: \0/' /etc/ssh/sshd_config.d/* || true

@ZeyadYasser
Copy link

ZeyadYasser commented Mar 20, 2024

@sergiocazzolato could you please also extend the fix to the LXD backend?

It might be a good idea to extend the LXD spread test to run with other system variants such as ubuntu-22.04.
EDIT: This is already being addressed in #182
https://github.com/snapcore/spread/blob/ded9133cdbceaf01f8a1c9decf6ff9ea56e194d6/tests/lxd/task.yaml#L12-L13
We could also extend the base spread.yaml in a follow up PR to add multiple systems most importantly 22.04 to test the google backend. https://github.com/snapcore/spread/blob/ded9133cdbceaf01f8a1c9decf6ff9ea56e194d6/spread.yaml#L17-L31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants