Skip to content

Expose GIT_CERT_SSH_SHA256 - #690

Merged
lhchavez merged 1 commit into
libgit2:masterfrom
cypres:expose-cert-ssh-sha256
Nov 27, 2020
Merged

Expose GIT_CERT_SSH_SHA256#690
lhchavez merged 1 commit into
libgit2:masterfrom
cypres:expose-cert-ssh-sha256

Conversation

@cypres

@cypres cypres commented Nov 26, 2020

Copy link
Copy Markdown
Contributor

Seems libgit2 had SHA256 fingerprint support since libgit2 v0.99.0; libgit2/libgit2#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.

@lhchavez lhchavez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, thanks for adding this!

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.

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 CertificateCheckCallback and would be better to have the full ssh.PublicKey available. That depends on libgit2/libgit2#5704 being merged, so stay tuned!

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?

we can also do the same thing that libgit2 does: https://github.com/libgit2/libgit2/blob/b8cdc9c9c59c61fc699550af5245c57744c1bcbd/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)

Comment thread remote.go Outdated
@cypres

cypres commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

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.

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 CertificateCheckCallback and would be better to have the full ssh.PublicKey available. That depends on libgit2/libgit2#5704 being merged, so stay tuned!

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.

(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)

True, though don't the other tests in remote_test.go already depend on network connectivity for their HTTPS based tests?

@lhchavez

Copy link
Copy Markdown
Contributor

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.

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 CertificateCheckCallback and would be better to have the full ssh.PublicKey available. That depends on libgit2/libgit2#5704 being merged, so stay tuned!

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?

totally! :D

Having the full public key would mean we can likely use golang.org/x/crypto/ssh/knownhosts to validate against known hosts files.

yeah, that's The Dream™️!

(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)

True, though don't the other tests in remote_test.go already depend on network connectivity for their HTTPS based tests?

oh yeah, we should fix that at some point. it's easy enough to spin up an http server within the tests.

Newer versions of libssh2 use SHA256 fingerprints
@cypres
cypres force-pushed the expose-cert-ssh-sha256 branch from f2b1f6c to 8bd8fb4 Compare November 27, 2020 00:43
@cypres
cypres requested a review from lhchavez November 27, 2020 00:46
@cypres

cypres commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

oh yeah, we should fix that at some point. it's easy enough to spin up an http server within the tests.

I can take a stab at that in the next couple of days if you want. Is that blocking this PR from proceeding?

@lhchavez

Copy link
Copy Markdown
Contributor

oh yeah, we should fix that at some point. it's easy enough to spin up an http server within the tests.

I can take a stab at that in the next couple of days if you want.

yay, thanks :D

Is that blocking this PR from proceeding?

not at all! this is still making forward progress, so let's merge this now

@lhchavez
lhchavez merged commit 7497529 into libgit2:master Nov 27, 2020
github-actions Bot pushed a commit that referenced this pull request Nov 27, 2020
Newer versions of libssh2 use SHA256 fingerprints

(cherry picked from commit 7497529)
github-actions Bot pushed a commit that referenced this pull request Nov 27, 2020
Newer versions of libssh2 use SHA256 fingerprints

(cherry picked from commit 7497529)
github-actions Bot pushed a commit that referenced this pull request Nov 27, 2020
Newer versions of libssh2 use SHA256 fingerprints

(cherry picked from commit 7497529)
@cypres
cypres deleted the expose-cert-ssh-sha256 branch November 27, 2020 01:38
lhchavez pushed a commit that referenced this pull request Nov 27, 2020
Newer versions of libssh2 use SHA256 fingerprints

(cherry picked from commit 7497529)

Co-authored-by: Hans Duedal <hans.duedal@gmail.com>
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.

2 participants