-
-
Notifications
You must be signed in to change notification settings - Fork 580
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
refactor: use StrictHostKeyChecking=accept-new #5104
refactor: use StrictHostKeyChecking=accept-new #5104
Conversation
Download the artifacts for this pull request:
See Testing a PR |
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 thought this would require a change to image; the only image it affects is the ddev-webserver. I guess I should push a new version for that though.
Pushed new web image and updated versionconstants.go |
TestSSHAuth failed with spoof complaint in https://buildkite.com/ddev/wsl2-docker-inside/builds/1871#018932b4-a699-4849-bc1d-b75ef6959db0 |
4df4a57
to
e9bf8ca
Compare
Rebased with new pushed/updated image. |
This does seem to consistently fail the wsl2-docker-inside tests, I haven't studied it carefully enough to understand why. |
7615163
to
22fa611
Compare
I manually ran the TestSSHAuth on wsl2-docker-inside and it worked fine. |
Hoping you're able to take a look @jonaseberle - happy to help with recreation or environments. I'll close this about 15 Aug 2023 if it's still stale, but it can also be reopened after that. |
I'm going to close this now, but happy to reopen it and help you chase it if it helps. Thanks for the initiative to work on this. |
I'm so sorry, I haven't seen the comments earlier. The host key of I am rebasing this and will prepend all |
@rfay could we reopen? |
Shoot, it seems that I can't reopen because you pushed the new commits, https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c?permalink_comment_id=4323332 Per that gist maybe you can force-push it back to the commit it was on previously, 22fa611 Then I'll reopen, then you can add your commits back? Or if that doesn't work, just reopen a new PR, not too hard. Thanks for working on this! |
Oh, now they'll let me! |
No, I guess they won't. The button was there for a minute. |
Thanks for your patience :) If not I'll just open a new one ;) |
Yay, reopened! |
Also, if you want to replace or update that test image, I'm all for it. It hasn't been touched for some years. |
I guess https://hub.docker.com/r/ddev/test-ssh-server was pushed during the move to ddev org, so it's not that old. However, I just pushed https://hub.docker.com/layers/ddev/test-ssh-server/v1.22.2/images/sha256-4b3aced3bd6e657bf85722e3a1c9a524683d3b078995e8115c4d7893b97ff0de?context=explore if you care to use it, shouldn't be any difference. Thanks for working on this! |
2a8aca7
to
d648ba1
Compare
I've now rebased to current master. I solved this single conflicts in favor of the latter:
|
The failing Linux test ran fine for me 3x locally.
@rfay do you have an idea? |
An important bug in the ddev-webserver was fixed since yours was pushed. And a test was added for it. I'll push an updated ddev-webserver that has this fix, and push a dummy commit to run the tests again. Locally, you'll want to make sure you |
Now it's back to TestAuthSSH and TestCmdAuthSSH so that at least makes sense. |
I have retriggered the failed buildkite runs and opened a topic on Discord regarding the failing MacOS Colima install. |
Looks like you solved the test problem! |
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.
It seems like you've got this now, and it should work out just fine.
Let me know when you're ready for me to pull it and I'll pull and update the new image(s)
Yes, the failed tests were from the wrong strategy in cleaning up an old host key in From my side this is ready. |
This option has been introduced in OpenSSH 7.6. It provides the protection against IP-spoofing without the previously necessary user interaction to accept unknown keys on first connect. In case a host key has actually changed, OpenSSH gives a helpful warning including the exact command that is necessary to remove the old host key from known_hosts. The tests have been adapted to actually use the web container's efault SSH config. Relates: https://github.com/orgs/ddev/discussions/5029
This just deletes /home/.ssh-agent/known_hosts instead of trying to just delete the host line. That might help if the host was added under its IP, too.
e1f95e6
to
863e613
Compare
Pushed fresh image and rebased |
The Issue
SSH power users and security folks expect
ssh
to check host keys.How This PR Solves The Issue
The option
StrictHostKeyChecking=accept-new
has been introduced in OpenSSH 7.6. It provides the protection against IP-spoofing without the previously necessary user interaction (in case of=yes/true
) to accept unknown keys on first connect.In case a host key has actually changed, OpenSSH gives a helpful warning including the exact command that is necessary to remove the old host key from known_hosts.
Manual Testing Instructions
Using
ssh
from within containers works as before. If a host key actually changed, the connection is rejected and OpenSSH shows an error message.Automated Testing Overview
Tests have been adapted to use the web container's default SSH config.
Related Issue Link(s)
(Discussion topic) https://github.com/orgs/ddev/discussions/5029
Release/Deployment Notes
Would be great to see this for 1.22.