Expose GIT_CERT_SSH_SHA256 #690
Loading…
Reference in New Issue
No description provided.
Delete Branch "expose-cert-ssh-sha256"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Seems libgit2 had SHA256 fingerprint support since libgit2 v0.99.0; https://github.com/libgit2/libgit2/pull/5307
This means that if you run libssh2 1.9.0 or newer, most likely the CertificateCheckCallback doesn't actually work for ssh/hostkey based remotes, because the fingerprint is not available.
~In addition I don't think we should be doing memcpy of the structures not set by libgit2.~
I have a test for it as well, but you can't anonymously connect over ssh, so I need a deploy key (which is read only) from https://github.com/libgit2/TestGitRepository. I had intended for the test to be automatically skipped if the ssh feature was not available, and check the hostkey's against the fingerprints from https://api.github.com/meta
If you could provide said deploy key, I'd be happy to add the test to this PR?
Let me know if anything needs to be changed. Thank you.
Neat, thanks for adding this!
FYI I'm going to upstream some changes to avoid depending on libssh2 altogether soon-ish, since it's a bit hard to work with just the hashes in
CertificateCheckCallbackand would be better to have the fullssh.PublicKeyavailable. That depends on https://github.com/libgit2/libgit2/pull/5704 being merged, so stay tuned!we can also do the same thing that libgit2 does:
b8cdc9c9c5/ci/test.sh (L109-L150)(TL;DR spin up an ssh server locally and create the keys on the fly to avoid having to depend on network connectivity, which could be flaky)
since these fields are explicitly zero-initialized in libgit2, it's completely fine if they are
memcpy()-ed unconditionally (as the code was doing before), since they are also zero-initialized here and won't be filled with garbage.if that is still undersirable, we have to fix a documentation bug in L180: that's supposed to be a bit field, not a normal enum! the source of confusion can be traced all the way back to a comment in libgit2, which should be fixed in https://github.com/libgit2/libgit2/pull/5704/files#diff-ae6473f1bca49ada56eedc638fa7c80395ac6a1f5a3e5ee8ae9574d587567715R100
In my testing they don't come back zero-initialized for some reason. I'm not sure why that is?
Running libgit2 1.1.0-1 on arch linux
ah then that means that libgit2 was successfully able to ask libssh2 for that particular certificate hash and it should be the correct hash.
unless, of course, there's a bug lurking somewhere :S
Oh that's awesome! I think it would still be good to expose the hash when we can, especially because it can be back-ported?
Having the full public key would mean we can likely use golang.org/x/crypto/ssh/knownhosts to validate against known hosts files.
True, though don't the other tests in remote_test.go already depend on network connectivity for their HTTPS based tests?
totally! :D
yeah, that's The Dream™️!
oh yeah, we should fix that at some point. it's easy enough to spin up an http server within the tests.
Ah, I get it now. It will actually return all the hashes, which is why it's a bit field. The returned hashes didn't match my expectations from the output of
ssh-keyscan -t rsa whoami.filippo.io | ssh-keygen -l -E md5 -f -but it's likely me that is holding it wrong. I'll do some more testing, and remove this part of the PR.Nevermind, I was authenticating with Ed25519 when I thought I was using RSA 🤦, which is why the hash'es didn't match.
I can take a stab at that in the next couple of days if you want. Is that blocking this PR from proceeding?
yay, thanks :D
not at all! this is still making forward progress, so let's merge this now