skeema / knownhosts

Go SSH known_hosts wrapper with host key lookup
Apache License 2.0
32 stars 4 forks source link

Certificate in known_hosts #7

Closed abakum closed 4 months ago

abakum commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

For known_hosts with @cert-authority * ecdsa-sha 2-nistp256... this unfortunately does not work: https://github.com/abakum/knownhosts/commit/f35d32f354f2923465e71e95e03c073699bb7fba

evanelias commented 4 months ago

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.