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

Certificate in known_hosts #7

Closed
abakum opened this issue Jun 6, 2024 · 6 comments · Fixed by #9
Closed

Certificate in known_hosts #7

abakum opened this issue Jun 6, 2024 · 6 comments · Fixed by #9

Comments

@abakum
Copy link

abakum commented Jun 6, 2024

Please append to HostKeyAlgorithms algorithms like:

ssh.CertAlgoSKED25519v01
ssh.CertAlgoECDSA256v01
ssh.CertAlgoECDSA384v01
ssh.CertAlgoECDSA521v01
ssh.CertAlgoSKECDSA256v01
ssh.CertAlgoDSAv01
ssh.CertAlgoRSAv01
ssh.CertAlgoRSASHA256v01
ssh.CertAlgoRSASHA512v01

if known_hosts has lines started with @cert-authority

@evanelias
Copy link
Contributor

Thank you for the feature request, but I'm not sure if this is actually possible today. github.com/skeema/knownhosts doesn't re-implement knownhosts file parsing. It fully relies on golang.org/x/crypto/ssh/knownhosts for that. This package only has access to whatever key types are exposed in KeyError.Want in golang.org/x/crypto/ssh/knownhosts.

Internally, golang.org/x/crypto/ssh/knownhosts knows which lines correspond to certs, but it doesn't export that field. And so far I don't see any obvious hack to get it to reveal that information. But if you can figure out a way that avoids duplicating core logic from golang.org/x/crypto/ssh/knownhosts, I'd happily review a pull request for this.

@evanelias
Copy link
Contributor

I should have clarified this earlier, my apologies, but just wanted to state this more clearly before anyone else submits a pull request for this issue:

This package intentionally does not re-read/re-parse the knownhosts file, as one of its core design concepts. Any PR which re-reads the knownhosts file won't be accepted.

I strongly suspect this means this issue is not possible to solve today. We need x/crypto/ssh/knownhosts to export more fields for this to be possible, unless someone can figure out a clever way to get that cert info from that package as-is, but so far I don't see one.

If you need cert support here, please consider filing an issue with x/crypto/ssh/knownhosts to expose more fields, or maybe there's an existing issue to comment on already, I'm not sure. Unfortunately I cannot devote any more time to this issue in the near future, as none of my product's customers are requesting this support. But if/when x/crypto/ssh/knownhosts exposes cert info, then this issue can be solved cleanly here.

@evanelias
Copy link
Contributor

Forgot to mention, this problem could also be solved in a hacky way at the caller (completely outside of this package). For example, the caller could manipulate the algorithm slice returned by HostKeyAlgorithms() to insert the cert version of each algorithm immediately prior to each non-cert algorithm.

I'm not positive but I think that should work properly in most cases, maybe as long as the host doesn't have multiple CAs of different types and some are missing from known_hosts, or some mix of CAs and regular keys?

If that approach does actually work in most cases, we could add an exported helper function here which performs the algorithm slice manipulation, just to make this a little easier. To be clear, this behavior wouldn't happen automatically or by default. It would be the caller's responsibility to use that helper function if/when desired.

@abakum
Copy link
Author

abakum commented Jun 17, 2024

Just add the certificates IdKeyAlgorithms if they are specified in known_hosts

	config := &ssh.ClientConfig{
		User:              param.user,
		Auth:              authMethods,
		Timeout:           10 * time.Second,
		HostKeyCallback:   cb,
		HostKeyAlgorithms: kh.HostKeyAlgorithms(param.addr),
		BannerCallback: func(banner string) error {
			_, err := fmt.Fprint(os.Stderr, strings.ReplaceAll(banner, "\n", "\r\n"))
			return err
		},
	}
	// Перед вызовом setupHostKeyAlgorithmsConfig должен быть установлен HostKeyAlgorithms.
	// >If hostkeys are known for the destination host then this default is modified to prefer their algorithms.
	debug("HostKeyAlgorithms %v", config.HostKeyAlgorithms)
	debug("IdKeyAlgorithms %v", idKeyAlgorithms)
	// kh Не понимает пока @cert-authority поэтому добавим idKeyAlgorithms
	config.HostKeyAlgorithms = NewStringSet(config.HostKeyAlgorithms...).Add(idKeyAlgorithms...).List()

@abakum
Copy link
Author

abakum commented Jun 18, 2024

For known_hosts with @cert-authority * ecdsa-sha 2-nistp256... this unfortunately does not work:
abakum@f35d32f

@evanelias
Copy link
Contributor

Created a new PR #9. @abakum, if you have a chance, could you please check out that code (it's the certs-backwards-compat branch) and see if it solves this issue with real-world testing? Your calling code will need to switch from knownhosts.New to knownhosts.NewDB in order to test the new CA support logic.

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 a pull request may close this issue.

2 participants