Skip to content

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

Open
sergiocazzolato wants to merge 3 commits intocanonical:masterfrom
sergiocazzolato:support-sshd_config.d-on-google
Open

support ssh configuration logic through sshd_config.d on google backend#155
sergiocazzolato wants to merge 3 commits intocanonical:masterfrom
sergiocazzolato:support-sshd_config.d-on-google

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
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
Copy link
Contributor

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
Contributor

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

@ZeyadYasser ZeyadYasser mentioned this pull request Mar 20, 2024
@cmatsuoka cmatsuoka closed this Feb 7, 2025
@cmatsuoka cmatsuoka reopened this Feb 7, 2025
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.

7 participants