Skip to content
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: fix ssh host key verification regression #771

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

aryan9600
Copy link
Member

Earlier, host key verification could potentially fail if there were
multiple entries in the known_hosts file and if the intended encryption
algorithm wasn't the first entry. This happened because we used the same
hasher object to compute the sum of all the public keys present in the
known_hosts file, which led to invalid hashes, resulting in a mismatch
when compared with the hash of the advertised public key. This is fixed,
by not creating the hasher ourselves and instead delegating that to the
function actually doing the matching, ensuring that a new hasher is used
for each comparison.

Regression introduced in v0.25.0 and reported in
fluxcd/image-automation-controller#378

Signed-off-by: Sanskar Jaiswal jaiswalsanskar078@gmail.com

@aryan9600 aryan9600 changed the title fix ssh host key verification regression libgit2: fix ssh host key verification regression Jun 8, 2022
@aryan9600 aryan9600 requested a review from pjbgf June 8, 2022 08:14
Earlier, host key verification could potentially fail if there were
multiple entries in the known_hosts file and if the intended encryption
algorithm wasn't the first entry. This happened because we used the same
hasher object to compute the sum of all the public keys present in the
known_hosts file, which led to invalid hashes, resulting in a mismatch
when compared with the hash of the advertised public key. This is fixed,
by not creating the hasher ourselves and instead delegating that to the
function actually doing the matching, ensuring that a new hasher is used
for each comparison.

Regression introduced in v0.25.0 and reported in
fluxcd/image-automation-controller#378

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

@aryan9600 nice one!

LGTM

Comment on lines +14 to +17
var knownHostsFixture = `github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==
github.com ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBEmKSENjQEezOmxkZMy7opKgwFB9nkt5YRrYMjNuG5N87uRgg6CLrbo5wAdT/y6v0mKV0U2w0WZ2YB/++Tpockg=
github.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl
`
Copy link
Member

@pjbgf pjbgf Jun 8, 2022

Choose a reason for hiding this comment

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

This could fail to simulate the scenario based on external factors that may change in the future. This is OK for now, but we should consider depending on a test server when refactoring/consolidating the Git implementation.

@pjbgf pjbgf added the area/git Git related issues and pull requests label Jun 8, 2022
@pjbgf pjbgf merged commit d48f4b4 into fluxcd:main Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git related issues and pull requests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants