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: improve known_hosts error messages #783

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Jun 14, 2022

Known hosts can be a difficult problem to troubleshoot.
To make it easier for end users, the generic message has
now been changed with a much more user friendly one.

Now if a known_host is not set, an error message will be
returned, instead of it simply being ignored.

@pjbgf pjbgf added the area/git Git related issues and pull requests label Jun 14, 2022
@pjbgf pjbgf added this to the GA milestone Jun 14, 2022
pkg/git/libgit2/managed/transport_test.go Outdated Show resolved Hide resolved
pkg/git/libgit2/managed/transport_test.go Outdated Show resolved Hide resolved
knownHosts: []byte(knownHostsFixtureUnormalized),
hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434")},
expectedHost: "source.developers.google.com:2022",
want: fmt.Errorf("no entries in known_hosts matches host '[source.developers.google.com]:2022' with fingerprint 'AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434='"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Pointing this out just to highlight how this looks like when printing the fingerprint in the error, so that we can discuss and think if there's a better way to express this error.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what it looks like in the GitRepo object status:

status:
  conditions:
  - lastTransitionTime: "2022-06-14T15:42:46Z"
    message: no artifact for resource in storage
    observedGeneration: 1
    reason: NoArtifact
    status: "True"
    type: Reconciling
  - lastTransitionTime: "2022-06-14T15:42:47Z"
    message: 'failed to checkout and determine revision: unable to fetch-connect to
      remote ''ssh://git@example.com:2224/foo/bar.git'':
      ssh: handshake failed: no entries in known_hosts matches host ''[example.com]:2224''
      with fingerprint ''AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434='''
    observedGeneration: 1
    reason: GitOperationFailed
    status: "False"
    type: Ready
  - lastTransitionTime: "2022-06-14T15:42:47Z"
    message: 'failed to checkout and determine revision: unable to fetch-connect to
      remote ''ssh://git@example.com:2224/foo/bar.git'':
      ssh: handshake failed: no entries in known_hosts matches host ''[example.com]:2224''
      with fingerprint ''AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434='''
    observedGeneration: 1
    reason: GitOperationFailed
    status: "True"
    type: FetchFailed

Copy link
Member Author

Choose a reason for hiding this comment

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

Small point here, based on @aryan9600's suggestion, we will no longer add padding to the fingerprints.

@darkowlzz darkowlzz requested review from hiddeco and removed request for darkowlzz June 14, 2022 15:40
pkg/git/libgit2/managed/ssh_test.go Show resolved Hide resolved
pkg/git/libgit2/managed/transport.go Outdated Show resolved Hide resolved
@pjbgf pjbgf force-pushed the known_hosts_error branch 2 times, most recently from 1addcd0 to 494bfc2 Compare June 16, 2022 10:07
Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

LGTM! 💯

Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

The overall change looks good to me. It'll make it much easier to debug the next time I use a non-standard port.
If we are fine with the error message that it'd put on the GitRepo status with fingerprint string, I'm good with merging this.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @pjbgf

Known hosts can be a difficult problem to troubleshoot.
To make it easier for end users, the generic message has
now been changed with a much more user friendly one.

Now if a known_host is not set, an error message will be
returned, instead of it simply being ignored.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@darkowlzz
Copy link
Contributor

Rebased.

@darkowlzz darkowlzz merged commit ea516d8 into fluxcd:main Jun 22, 2022
@pjbgf pjbgf deleted the known_hosts_error branch June 28, 2022 07:45
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.

4 participants