-
Notifications
You must be signed in to change notification settings - Fork 59
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
spread(lxd): work around SSH password authentication disabled by livecd-rootfs #184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
I'll extend the lxd backend spread test to cover more recent images. |
LXD backend spread tests have been updated with Ubuntu 18.04, 20.04, and 22.04. |
Looks like a similar fix but done slightly differently was proposed in #179 |
550ff0d
to
b803505
Compare
Also related, a similar fix for the Google backend: #155 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for catching this, Maybe let's wait for it to be merged in the google backend #155? and then backport it to all other providers so that it is consistent across all of them.
b803505
to
a9fac0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Seems reasonable, but a bit obscure. More details below.
spread/lxd.go
Outdated
@@ -482,6 +482,8 @@ func (p *lxdProvider) serverJSON(name string) (*lxdServerJSON, error) { | |||
func (p *lxdProvider) tuneSSH(name string) error { | |||
cmds := [][]string{ | |||
{"sed", "-i", `s/^\s*#\?\s*\(PermitRootLogin\|PasswordAuthentication\)\>.*/\1 yes/`, "/etc/ssh/sshd_config"}, | |||
// provide a higher priority drop in with our overrides | |||
{"/bin/bash", "-c", `[ -d /etc/ssh/sshd_config.d ] && echo -e "PermitRootLogin yes\nPasswordAuthentication yes" > /etc/ssh/sshd_config.d/00-spread.conf`}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is not making clear what is going on. "Higher priority" than what? Why do we need the same operation in two different ways? Also, if there are duplicates, what happens?
Also, as an aside our comments are typically "// Proper English sentences.", and in the case here it should maybe be above the two lines since they both aim at the same outcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tweaked the wording around the snippets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. The comment explains it better, but I think the wording is still a bit confusing. Let me try an alternative, see what you think.
a9fac0b
to
13f861a
Compare
…otfs In recent images, livecd-rootfs may be disabling SSH password authentication by default, with the intention that cloud-init would enabled that by placing a higher priority file under /etc/sshd/sshd_config.d/. See https://git.launchpad.net/livecd-rootfs/commit/live-build/ubuntu-cpc/hooks.d/chroot/052-ssh_authentication.chroot?id=480d5b26ea97e0bfebac8aedfa1f2bf7286f027a for reference. Do the same as cloud-init would a drop a higher priority config overriding relevant SSHD settings. Signed-off-by: Maciej Borzecki <[email protected]>
13f861a
to
eecf017
Compare
Extend the list of systems. Signed-off-by: Maciej Borzecki <[email protected]>
eecf017
to
842df26
Compare
Signed-off-by: Claudio Matsuoka <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Oops, tests are failing. |
spread/lxd.go
Outdated
// If the sshd configuration is split in snippets in /etc/ssh/sshd_config.d, | ||
// place the configuration in a 00-* file because the first obtained value | ||
// will be used. See sshd_config(5) for details. | ||
{"/bin/bash", "-c", `[ -d /etc/ssh/sshd_config.d ] && echo -e "PermitRootLogin yes\nPasswordAuthentication yes" > /etc/ssh/sshd_config.d/00-spread.conf`}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'shortened' version is actually wrong, exit code [ -d /etc/ssh/sshd_config.d ]
becomes the return status of the whole command. Let me push a fix.
…fixup Use a proper `if ..; then .. fi` statement to avoid the shortened shell form returning with an incorrect status when /etc/ssh/sshd_config.d does not exist. Signed-off-by: Maciej Borzecki <[email protected]>
hmm the test google system ran out of space?
|
Signed-off-by: Maciej Borzecki <[email protected]>
I don't think this is getting anywhere given the flakiness in lxd:
|
Thanks! |
In recent images, livecd-rootfs may be disabling SSH password authentication by default, with the intention that cloud-init would enabled that by placing a higher priority file under /etc/sshd/sshd_config.d/. See https://git.launchpad.net/livecd-rootfs/commit/live-build/ubuntu-cpc/hooks.d/chroot/052-ssh_authentication.chroot?id=480d5b26ea97e0bfebac8aedfa1f2bf7286f027a for reference.
Do the same as cloud-init would a drop a higher priority config overriding relevant SSHD settings.