Backwards-compatible support for @cert-authority

evanelias commented 4 months ago

This pull request adds support for \@cert-authority lines in known_hosts files in an optional, backwards-compatible manner. Fixes #7.

With this PR's changes, users can switch from knownhosts.New() to the newly-introduced knownhosts.NewDB() in order to opt-in to the \@cert-authority support. This returns a new HostKeyDB struct instead of a HostKeyCallback. It has a very slight performance penalty as it requires re-reading the known_hosts file, and it has a different return type of its HostKeys() method, but otherwise it should be a drop-in replacement for users who require CA support. This makes sense for use-cases such as general-purpose SSH clients.

~Git-specific SSH use-cases can likely stay on knownhosts.New(), since public Git forges (GitHub, Gitlab, etc) don't seem to use CAs for host keys anyway.~ [Edit: not necessarily true, see discussion in comments below.]

All previous HostKeyCallback logic remains backwards-compatible and avoids any functionality changes. [Edit to clarify: When using knownhosts.New or knownhosts.HostKeyCallback directly, there is no CA support. To get the CA support, your calling code must switch to using knownhosts.NewDB instead.]

Implementation

This includes @Javier-varez's commit from #8 as-is, and then adds an additional commit on top to adjust the following:

Move the CA support to new HostKeyDB struct, making it opt-in. This avoids changing the function signature of the HostKeyCallback.HostKeys() method, in order to retain backwards compatibility and avoiding a v2 version bump for the module.
When re-reading the known_hosts files to implement the CA support, it only re-reads each file a single time, at constructor time instead of in the callback. It reads using buffered IO similar to x/crypto/ssh/knownhosts which should ensure its line-counting behavior matches.
Add test coverage for all new behaviors and @cert-authority logic.

Alternatives considered

Conceptually the information on @cert-authority lines needs to be tracked somewhere, but the difficulty with the previous design is that New() returns a HostKeyCallback which is just a function, rather than a struct. So the chosen solution here leaves that type as-is, and instead introduces a separate new struct which supports adding more fields.

Instead of introducing a separate new struct, an alternative approach would have been to use a non-exported package global of the form map[*HostKeyCallback]CertInfo, in order to track additional information on each HostKeyCallback. This would result in simpler user-facing logic, however it would then require a separate function to "de-register" a callback to avoid a memory leak. Overall that seems hackier, and less extensible if additional metadata fields are needed in the future.

Testing and feedback

The PR includes unit test coverage, but it could use some further real-world testing to ensure it properly solves #7. I will keep this PR open a few days, and hugely appreciate any community feedback.

cc @lonnywong @abakum @Javier-varez

coveralls-official[bot] commented 4 months ago

coverage: 92.857% (+0.3%) from 92.593% when pulling 53a26ccd67909a2b2a5cfad598c6ba7c860996d2 on certs-backwards-compat into 5832aa8abbe19d2e27ddc7e63528efe787578b75 on main.

Javier-varez commented 4 months ago

Hi @evanelias,

Thank you so much for taking the initiative to improve my original proposal. I do think this is a better choice going forward considering the number of users that depend on this repository. Making the functionality opt-in seems sensible to me.

Curiously enough, I do need this functionality for a git ssh backend that uses certificate authorities. We have an internal tool that was recently migrated from plain git subcommand calls to https://github.com/go-git/go-git, and that's when we first encountered this issue. If we go this route, I will then propose the go-git maintainers to either support this functionality or provide some sort of toggle so that it can be enabled in third party tools. Unfortunately for us this problem was hidden a couple of dependencies down the tree.

I will test this during the next couple of days and give you some feedback if it all seems to work as intended, but on first sight, it looks good to me.

Instead of introducing a separate new struct, an alternative approach would have been to use a non-exported package global of the form map[*HostKeyCallback]CertInfo, in order to track additional information on each HostKeyCallback. This would result in simpler user-facing logic, however it would then require a separate function to "de-register" a callback to avoid a memory leak. Overall that seems hackier, and less extensible if additional metadata fields are needed in the future.

I agree with you. I tend to really dislike globals for many reasons, like concurrency issues (to avoid concurrent mutations you'd probably introduce an undesired lock) and just the plain fact that over the runtime of a program, the host key callbacks can also change, so keeping them around doesn't sound like the best idea. I like this alternative best.

abakum commented 4 months ago

It is correct that the returned result from func (hkdb *knownhosts.HostKeyDB) HostKeyCallback() is ssh.HostKeyCallback but from func knownhosts.New(files ...string) is knownhosts.HostKeyCallback ?

evanelias commented 4 months ago

@Javier-varez

Curiously enough, I do need this functionality for a git ssh backend that uses certificate authorities.

Oh interesting, I didn't realize anyone used CAs for Git use-cases. I'll revise the package and method doc comments later today to remove the stuff about Git use-cases being fine to remain on the old New constructor.

I will then propose the go-git maintainers to either support this functionality or provide some sort of toggle so that it can be enabled in third party tools

Sounds good. Probably the best path would be to just have go-git switch to using NewDB always. Making it an optional toggle in go-git could be messy, since NewDB and New return different types.

Just to be safe, I'll add a method to make this easier, allowing conversion from knownhosts.HostKeyCallback to a non-CA-supporting knownhosts.HostKeyDB for this situation. But hopefully callers won't actually need it.

I will test this during the next couple of days and give you some feedback if it all seems to work as intended

Thank you, that sounds great.

evanelias commented 4 months ago

It is correct that the returned result from func (hkdb *knownhosts.HostKeyDB) HostKeyCallback() is ssh.HostKeyCallback but from func knownhosts.New(files ...string) is knownhosts.HostKeyCallback ?

@abakum yes, that is correct and intentional. After switching to knownhosts.NewDB / knownhosts.HostKeyDB, you should no longer need to use knownhosts.New or knownhosts.HostKeyCallback at all for anything. I can try to improve the method doc string comments further if that isn't clear currently?

abakum commented 4 months ago

Thanks, evanelias, it works for me!

abakum commented 4 months ago

@evanelias, if line in known_hosts like

@cert-authority * SHA256:HGzeMguvVfTsMb+WfkqmjZNXaeVcBXCQqXyjKUBy9pA

then kh.HostKeyAlgorithms("127.0.0.1:22") return [ecdsa-sha2-nistp256-cert-v01@openssh.com] but kh.HostKeyAlgorithms("127.0.0.1:2222") return [] and ssh from OpenSSH works well. How to handle it?

evanelias commented 4 months ago

@abakum Interesting catch, thanks. But the core host-matching logic is still handled by x/crypto/ssh/knownhosts, we don't re-implement or change that here.

The match logic in x/crypto/ssh/knownhosts appears to only apply wildcards like * to the hostname portion, not the port, per https://cs.opensource.google/go/x/crypto/+/refs/tags/v0.24.0:ssh/knownhosts/knownhosts.go;l=110

return wildcardMatch([]byte(p.addr.host), []byte(a.host)) && p.addr.port == a.port

So in order to match 127.0.0.1:2222 the known_hosts entry would need to specifically be[*]:2222.

I just searched the issue tracker for https://github.com/golang/go and found https://github.com/golang/go/issues/52056 which seems to describe the root of the problem. I've commented there now too.

abakum commented 4 months ago

https://github.com/golang/go/issues/67295#issuecomment-2105738884

evanelias commented 4 months ago

On second thought, it might be possible to build a work-around here in skeema/knownhosts for that problem with wildcards and non-standard ports. The callback logic I'm envisioning will need to be nested and tricky, so I'm not 100% certain this is feasible, but I can give this a try sometime in the next few days.

abakum commented 4 months ago

err

Javier-varez commented 4 months ago

@evanelias

Just to be safe, I'll add a method to make this easier, allowing conversion from knownhosts.HostKeyCallback to a non-CA-supporting knownhosts.HostKeyDB for this situation. But hopefully callers won't actually need it.

This is a good point, thank you. I drafted here how it would look like to use the knownhosts DB. https://github.com/Javier-varez/go-git/commit/0879ef12d83e42fe0d4cc0d23ca1a21ce5a406c3

I will test this during the next couple of days and give you some feedback if it all seems to work as intended

Thank you, that sounds great.

On this front, I tested the change, and it works great for my usecase. Thank you!

abakum commented 4 months ago

I'm trying to re-check https://github.com/abakum/knownhosts/blob/0280d4dc9533ee3f92de6b57a5efb45087cbf3e0/cmd/main.go 1) First run with empty known_hosts

PS Y:\src\knownhosts\cmd> go run .
main.go:95: []
main.go:98: Failed to dial:  ssh: handshake failed: knownhosts: key is unknown
main.go:145: []
main.go:117: innerCallback(hostname, remote, key) knownhosts: key is unknown
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) true
main.go:129: Added host 10.161.115.189:22 to known_hosts
main.go:95: []
main.go:98: Failed to dial:  ssh: handshake failed: knownhosts: key is unknown
main.go:145: []
main.go:117: innerCallback(hostname, remote, key) knownhosts: key is unknown
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) true
main.go:129: Added host 10.161.115.189:222 to known_hosts

2) known_hosts now

10.161.115.189 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=
[10.161.115.189]:222 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=

3) Edit known_hosts to:

* ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=

4) run ssh -v user_@10.161.115.189

...
debug1: Host '10.161.115.189' is known and matches the ECDSA host key.
debug1: Found key in C:\\Users\\user/.ssh/known_hosts:1
...

5) run ssh -v user_@10.161.115.189 -p 222

...
debug1: Host '[10.161.115.189]:222' is known and matches the ECDSA host key.
debug1: Found key in C:\\Users\\user/.ssh/known_hosts:1...

6) Second run with * in known_hosts

PS Y:\src\knownhosts\cmd> go run .
main.go:95: [ecdsa-sha2-nistp256]
main.go:145: [ecdsa-sha2-nistp256]
main.go:117: innerCallback(hostname, remote, key) <nil>
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) false
main.go:95: []
main.go:98: Failed to dial:  ssh: handshake failed: knownhosts: key is unknown
main.go:145: []
main.go:117: innerCallback(hostname, remote, key) knownhosts: key is unknown
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) true
main.go:129: Added host 10.161.115.189:222 to known_hosts

evanelias commented 4 months ago

@Javier-varez awesome, thank you! re: proposed go-git changes, makes sense and looks good. Might need to leave the old NewKnownHostsCallback in place too though as deprecated, since it's exported and could be used by third-party code, unless their versioning policy isn't strict about this. Inside go-git itself that function is also called from a unit test -- see plumbing/transport/ssh/auth_method_test.go

@abakum can you please summarize the finding? Is it just confirming that * wildcards are not working with non-standard ports, or something else? Thanks!

abakum commented 4 months ago

Yes golang not working but OpenSSH working

evanelias commented 4 months ago

Thank you both again for the testing assistance! I'm going to merge this momentarily.

Early next week, I'll do a separate branch / pull request with a fix for the wildcards on non-standard port issue. And then once that one looks good and gets merged too, I'll tag a new release.

abakum commented 4 months ago

I'm trying to re-check https://github.com/abakum/knownhosts/blob/0280d4dc9533ee3f92de6b57a5efb45087cbf3e0/cmd/main.go

First run with empty known_hosts

PS Y:\src\knownhosts\cmd> go run .
main.go:95: []
main.go:98: Failed to dial:  ssh: handshake failed: knownhosts: key is unknown
main.go:145: []
main.go:117: innerCallback(hostname, remote, key) knownhosts: key is unknown
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) true
main.go:129: Added host 10.161.115.189:22 to known_hosts
main.go:95: []
main.go:98: Failed to dial:  ssh: handshake failed: knownhosts: key is unknown
main.go:145: []
main.go:117: innerCallback(hostname, remote, key) knownhosts: key is unknown
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) true
main.go:129: Added host 10.161.115.189:222 to known_hosts

known_hosts now

10.161.115.189 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=
[10.161.115.189]:222 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=

Edit known_hosts to:

* ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=

run ssh -v user_@10.161.115.189

...
debug1: Host '10.161.115.189' is known and matches the ECDSA host key.
debug1: Found key in C:\\Users\\user/.ssh/known_hosts:1
...

run ssh -v user_@10.161.115.189 -p 222

...
debug1: Host '[10.161.115.189]:222' is known and matches the ECDSA host key.
debug1: Found key in C:\\Users\\user/.ssh/known_hosts:1...

Second run with * in known_hosts

PS Y:\src\knownhosts\cmd> go run .
main.go:95: [ecdsa-sha2-nistp256]
main.go:145: [ecdsa-sha2-nistp256]
main.go:117: innerCallback(hostname, remote, key) <nil>
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) false
main.go:95: []
main.go:98: Failed to dial:  ssh: handshake failed: knownhosts: key is unknown
main.go:145: []
main.go:117: innerCallback(hostname, remote, key) knownhosts: key is unknown
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) true
main.go:129: Added host 10.161.115.189:222 to known_hosts

known_hosts now

* ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=
[10.161.115.189]:222 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=

Edit known_hosts to:

* ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBARrpBl177hs/ykMnXHfkjmyKTbsax/vtSl+rInZvJoF8LfJaWCZSrai0uD5qRuYhy4QnJs563NBTmCgSBhm/MA=

After fix from https://github.com/skeema/knownhosts/tree/fix-wildcards-port-match run with * in known_hosts works well:

PS Y:\src\knownhosts\cmd> go run .
main.go:95: [ecdsa-sha2-nistp256]
main.go:145: [ecdsa-sha2-nistp256]
main.go:117: innerCallback(hostname, remote, key) <nil>
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) false
main.go:95: [ecdsa-sha2-nistp256]
main.go:145: [ecdsa-sha2-nistp256]
main.go:117: innerCallback(hostname, remote, key) <nil>
main.go:118: knownhosts.IsHostKeyChanged(err) false
main.go:119: knownhosts.IsHostUnknown(err) false

Thanks, @evanelias !

evanelias commented 4 months ago

Just opened #10 for the wildcard host matching fix, along with some additional documentation/README tweaks.

evanelias commented 4 months ago

I've just tagged v1.3.0 which includes this PR, as well as the wildcard matching fix in #10. Thanks again @Javier-varez and @abakum for all your assistance here!

skeema / knownhosts