-
Notifications
You must be signed in to change notification settings - Fork 187
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
libgit2: check hostkey type when validating hostkey #290
Conversation
240a24a
to
e92a852
Compare
f03dcac
to
72d2ef5
Compare
72d2ef5
to
23b8e99
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks OK to me, but I am linking in @squaremo as a second pair of eyes given I have not been on my best on this topic the past days...
Thank you @phillebaba 🙏
@hiddeco is there a test that verifies libigit2 cloning over ssh? I can manually verify this if it needs to get merged tomorrow, but I will try to get a test in tonight. |
@phillebaba the image controllers cover this indirectly, and confirmed my changes in #288 worked. I do however think that the |
Signed-off-by: Philip Laine <philip.laine@gmail.com>
23b8e99
to
6b3d96e
Compare
pkg/git/libgit2/transport.go
Outdated
if bytes.Compare(hash[:], key) != 0 { | ||
var fingerprint []byte | ||
var hasher hash.Hash | ||
switch hostkey.Kind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hostkey.Kind is a bitset, so you want something like
switch {
case hostkey.Kind & git2go.HostkeySHA256 > 0:
...
}
i.e., check the bitset against each enum value, starting with the most preferred (SHA256) down to the lowly MD5.
(if you print hostkey.Kind
out, it'll usually have the value 7
, which is HostkeyMD5 | HostkeySHA1 | HostkeySHA256
(the enum values are in https://github.com/libgit2/libgit2/blob/27e34f9b9843f7bcc33a4ccfe3e395fe303cba63/include/git2/cert.h#L76)
Signed-off-by: Philip Laine <philip.laine@gmail.com>
My image-auto tests pass with this change, and running an automation against github.com with SSH/libgit2 also works. |
I am just going to add a test to verify priority and then we can merge this. |
Signed-off-by: Philip Laine <philip.laine@gmail.com>
Signed-off-by: Philip Laine <philip.laine@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given it has now test coverage on top of the tests performed by Michael, and the unit tests for the commit that added them passed, I am confident this works.
Thank you for following up @phillebaba 🥇
This change adds checking for fingerprint types other than SHA1 when checking the hostkey. This is a bug which has not been an issue yet as very few git servers use MD5 fingerprints and Azure DevOps still uses SHA1 fingerprints. It could potentially be an issue when using the libgit2 implementation with GitHub as it uses SHA256 fingerprints.