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

HostKeyAlgorithms: add rsa-sha2-256 and rsa-sha2-512 for ssh-rsa #6

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

lonnywong
Copy link
Contributor

  • Related to new conn [] failed: ssh: handshake failed trzsz/trzsz-ssh#93

  • Steps to reproduce:

    • Change /etc/ssh/sshd_config on the ssh server, add a configuration:
      HostKeyAlgorithms rsa-sha2-512,rsa-sha2-256
      
    • Restart the ssh server service ssh restart.
    • Delete the known host items of the server in ~/.ssh/known_hosts on the client.
    • Log in to the server using go ssh client, which should add a ssh-rsa item to ~/.ssh/known_hosts.
    • Log in to the server using go ssh client again, an error will occur:
      client offered: [ssh-rsa], server offered: [rsa-sha2-512 rsa-sha2-256]
      

@evanelias
Copy link
Contributor

Great debugging and fix, thank you @lonnywong, this is excellent!

One minor nit: should the 3 resulting rsa entries get returned in the opposite order, so that the most-secure algorithm gets listed first? I'm pondering this since ssh.ClientConfig.HostKeyAlgorithms is a sorted list by preference, first match between client and server wins. In other words, instead of expanding the rsa key's algorithms to "ssh-rsa", "rsa-sha2-256", "rsa-sha2-512", should it be ordered like "rsa-sha2-512", "rsa-sha2-256", "ssh-rsa" ?

To be clear, aside from how we expand ssh-rsa knownhosts entries, we can keep all other ordering as-is: that is, the returned order just matches the order of the entries in the knownhosts file.

Separately, CI is failing because of something with golint, probably because the Go version I have in the GitHub Actions config is old. I'll try bumping that version on my end momentarily; or if that fails I'll just finally remove golint since it is deprecated/unmaintained.

@evanelias
Copy link
Contributor

update: CI is now fixed on main branch, if you rebase your PR against latest main commit it should hopefully work now 👍

@lonnywong
Copy link
Contributor Author

@evanelias Now, the order is rsa-sha2-512, rsa-sha2-256, ssh-rsa.

@evanelias
Copy link
Contributor

Thanks, looks perfect! Merging momentarily and tagging a new version.

@evanelias evanelias merged commit e73fcfc into skeema:main Mar 12, 2024
1 check passed
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.

None yet

2 participants