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

crypto/ssh/knownhosts: check more than one key #254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

caarlos0
Copy link

I believe this fixes golang/go#36126 .

The problem was that it was keeping only the first known key of each type found. If you have a server advertising multiple keys of the same type, you might get a missmatch key error.

Per sshd(8) man page, it should allow reapeatable hosts with different host keys, although it don't specify anything about hosts being from different types:

It is permissible (but not recommended) to have several lines or
different host keys for the same names.  This will inevitably happen when
short forms of host names from different domains are put in the file.  It
is possible that the files contain conflicting information;
authentication is accepted if valid information can be found from either
file.

So, this changes knownhosts behavior to accept any of the keys for a given host, regardless of type.

closes golang/go#36126

@gopherbot
Copy link
Contributor

This PR (HEAD: adcf924) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/478535 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/478535.
After addressing review feedback, remember to publish your drafts!

@lonnywong
Copy link

Any progress? Love to see this merged. Thanks!

@evanelias
Copy link

One potential downside to this PR is it changes the documented behavior of knownhosts.KeyError.Want, re: "For each key algorithm there can be one hostkey"

type KeyError struct {
	// Want holds the accepted host keys. For each key algorithm,
	// there can be one hostkey.  If Want is empty, the host is
	// unknown. If Want is non-empty, there was a mismatch, which
	// can signify a MITM attack.
	Want []KnownKey
}

@lonnywong out of curiosity, do you really have 2 keys of the same type for the same hostname (same situation as golang/go#36126, which would be solved by this PR)? Or do you have a server presenting multiple hostkeys of different types, some of which aren't in the client knownhosts file (see golang/go#29286)?

Both issues result in the same user-facing error on the client side, but they have different root causes. In my experience, the latter situation is much more common.

I have a widely-used package, github.com/skeema/knownhosts, which uses some trickery to provide a workaround for golang/go#29286, although it doesn't help with golang/go#36126.

Just mentioning this here because github.com/skeema/knownhosts currently assumes the "For each key algorithm there can be one hostkey" logic remains the case, although I don't think it would break anything if that changes -- pretty sure it would just lead to potential duplicate entries in ssh.ClientConfig.HostKeyAlgorithms which hopefully is harmless? Anyway that should be easy to solve if it is a problem. Not sure if other packages out there depend on that logic more strictly though.

@lonnywong
Copy link

@evanelias Thanks very much for your help. My case is multiple hostkeys of different types. I didn’t find golang/go#29286 before. I’ll try your awesome package https://github.com/skeema/knownhosts this weekend. Thanks again.

evanelias added a commit to skeema/knownhosts that referenced this pull request Sep 18, 2023
Currently the behavior of HostKeyAlgorithms never contains duplicates, only by
virtue of golang.org/x/crypto/ssh/knownhosts exposing a maximum of one key per
algorithm in its KeyError.Want slice.

However, that upstream behavior could theoretically change in the future,
especially since golang.org/x/crypto is versioned as a pre-v1 module, and the
one-key-per-type behavior is only documented as a comment (e.g. not part of
any type or function signature).

This commit makes our HostKeyAlgorithms function more robust / future-proof
by ensuring that its result does not contain duplicates, regardless of
upstream behavior.

This means if golang/go#28870 is solved (for example
by golang/crypto#254), there should not be any harm to
our behavior here in github.com/skeema/knownhosts.
@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/478535.
After addressing review feedback, remember to publish your drafts!

@caarlos0
Copy link
Author

anything I can do to help move this forward?

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.

x/crypto/ssh/knownhosts: cannot have multiple keys for same host
4 participants