go-git icon indicating copy to clipboard operation
go-git copied to clipboard

Strict Host Checking

Open vermapratyush opened this issue 8 years ago • 6 comments

Hi, I am trying to clone a git repo (SSH auth)

repo, err = git.Clone(memory.NewStorage(), nil, &git.CloneOptions{
	URL:      <URL>,
	Auth:     getAuth(),
})
func getAuth() ssh.AuthMethod {
	auth, _ := ssh.NewPublicKeysFromFile("git", path, "")
	return auth
}

The above code works when the ~/.ssh/known_host file contains the remote host is verified. However when the server does not contain the host in the file. The git.Clone() command fails with the error knownhosts: key is unknown. I added StrictHostKeyChecking no in ~/.ssh/known_hosts file, however I still get the error.

Basically, I am not sure how to use ssh.InsecureIgnoreHostKey() along with the helper method ssh.NewPublicKeys

Any suggested way to solve this?

vermapratyush avatar Jun 28 '17 15:06 vermapratyush

I was able to solve this by adding the following to the ssh.AuthMethod

import "ssh2 "golang.org/x/crypto/ssh""
auth.(*ssh.PublicKeys).HostKeyCallback = ssh2.InsecureIgnoreHostKey()

However, it there a better more elegant way of solving this?

I see that many functions/methods in go-git return interface instead of struct, which is the preferred method in go. Returning struct instead of interfaces adds to better readability and the above hack would look much cleaner as well :) Any strong reason to not return struct?

vermapratyush avatar Jun 28 '17 16:06 vermapratyush

@vermapratyush Thank you for reporting this, and thank you for providing a workaround!

On the struct vs interface stuff, we really have many places where we can have different implementations (and pluggable ones). There are certainly places were we could change from interface to struct (and viceversa) to improve the API, but it's not something we are up to discussing in general, but with specific cases where we can make the right trade-offs.

@mcuadros What's the appropriate way of doing this with the current API? Should we improve public API here?

smola avatar Jun 30 '17 13:06 smola

By the way, returning structs on ssh.New* functions sounds like a good change, indeed.

smola avatar Jun 30 '17 13:06 smola

I have a similar - ish - requirement, in that I want a fetch operation to proceed over SSH using an in-memory SSH key and known_hosts file.

The approach I'm taking is to set the value of plumbing/transport/ssh.DefaultClient; if FetchOptions (and others) allowed the whole client to be injected as well as (or instead of) the AuthOptions, I wouldn't need to.

Nothing needs converting from interface to struct for this.

lupine avatar Jul 18 '17 11:07 lupine

The much better way is to rely on the strict host checking and use a defined known_hosts file. The only thing to do is to export it before with export SSH_KNOWN_HOSTS=/app/ssh/known_hosts in this case.

dirkaholic avatar Jul 28 '17 07:07 dirkaholic

Just a note for anyone else finding this, you might not just need the code:

auth.HostKeyCallback = ssh.InsecureIgnoreHostKey()

without the type assertion, and you can't add it to the auth struct

// THIS CODE DOESN'T WORK
auth := &ssh2.PublicKeys{
	User:   "git",
	Signer: signer,
        HostKeyCallback: ssh.InsecureIgnoreHostKey(),
}

because it is a promoted field

zwhitchcox avatar Aug 30 '19 21:08 zwhitchcox