Make ssh commands used in the git smart transport compatible with libgit2 #852

2021-11-07T12:24:23-06:00

darkowlzz commented

2021-11-07 12:24:23 -06:00

(Migrated from github.com)

Before this change, the commands sent to the git server were of the form:

git-upload-pack "/bar/test-reponame"

This resulted in the git server (gitkit) returning error:
error parsing command: invalid git command

This change replaces the double quotes with single quotes:

git-upload-pack '/bar/test-reponame'

Using git smart transport via managed ssh transport against gitkit, fails with the following errors:

2021/11/07 21:57:24 ssh: incoming exec request: git-upload-pack "/bar/test-reponame"
2021/11/07 21:57:24 ssh: payload 'git-upload-pack "/bar/test-reponame"'
2021/11/07 21:57:24 ssh: error parsing command: invalid git command

Resulting in a clone failure: unable to clone: EOF.

Using ssh transport based on the libgit2 libraries work with gitkit.
Upon further investigation, I found that gitkit has a regular expression for parsing the commands and it fails to parse commands with " (double quote). Replacing the double quotes with single quote fixes the issue.
It felt like a bug in gitkit itself. So I wasn't sure if we should fix it in git2go. But after going through the libgit2 code, I found that the libgit2 implementation also uses single quotes, refer: 358a60e1b4/src/transports/ssh.c (L96-L99).

I guess it'd be better if we are consistent with libgit2 to maintain compatibility.

Before this change, the commands sent to the git server were of the form: ``` git-upload-pack "/bar/test-reponame" ``` This resulted in the git server ([gitkit](https://github.com/sosedoff/gitkit)) returning error: `error parsing command: invalid git command` This change replaces the double quotes with single quotes: ``` git-upload-pack '/bar/test-reponame' ``` Using git smart transport via managed ssh transport against gitkit, fails with the following errors: ```console 2021/11/07 21:57:24 ssh: incoming exec request: git-upload-pack "/bar/test-reponame" 2021/11/07 21:57:24 ssh: payload 'git-upload-pack "/bar/test-reponame"' 2021/11/07 21:57:24 ssh: error parsing command: invalid git command ``` Resulting in a clone failure: `unable to clone: EOF`. Using ssh transport based on the libgit2 libraries work with gitkit. Upon further investigation, I found that gitkit has a [regular expression](https://github.com/sosedoff/gitkit/blob/99577b562191e1a27b81dfb2d569ea54452d77e9/git_command.go#L9) for parsing the commands and it fails to parse commands with `"` (double quote). Replacing the double quotes with single quote fixes the issue. It felt like a bug in gitkit itself. So I wasn't sure if we should fix it in git2go. But after going through the libgit2 code, I found that the libgit2 implementation also uses single quotes, refer: https://github.com/libgit2/libgit2/blob/358a60e1b46000ea99ef10b4dd709e92f75ff74b/src/transports/ssh.c#L96-L99. I guess it'd be better if we are consistent with libgit2 to maintain compatibility.

❤️ 1

lhchavez (Migrated from github.com) reviewed 2021-11-07 12:38:06 -06:00

lhchavez (Migrated from github.com) left a comment

thanks for this change! there's a small vulnerability being introduced, but once that's addressed, 👍!

thanks for this change! there's a small vulnerability being introduced, but once that's addressed, :+1:!

ssh.go

													
				@ -77,0 +79,4 @@

					uPath := strings.Replace(u.Path, `\`, `\\`, -1)

					uPath = strings.Replace(uPath, `'`, `\'`, -1)

					// TODO: Add percentage decode similar to libgit2.

lhchavez (Migrated from github.com) commented

2021-11-07 12:37:32 -06:00

can \ and ' be escaped? otherwise one of these characters can be added to the path and cause some unwanted shenanigans on the remote host.

also, the link that was shared made me realize that we are currently missing the %-decoding that's done in libgit2! 358a60e1b4/src/str.c (L455-L481) since this does not regress on that regard, it's fine if that is not fixed in this same commit (to keep its scope small), but would it be possible to add a small TODO for us to fix it later?

can `\` and `'` be escaped? otherwise one of these characters can be added to the path and cause some unwanted shenanigans on the remote host. also, the link that was shared made me realize that we are currently missing the %-decoding that's done in libgit2! https://github.com/libgit2/libgit2/blob/358a60e1b46000ea99ef10b4dd709e92f75ff74b/src/str.c#L455-L481 since this does not regress on that regard, it's fine if that is not fixed in this same commit (to keep its scope small), _but_ would it be possible to add a small `TODO` for us to fix it later?

darkowlzz (Migrated from github.com) reviewed 2021-11-07 13:51:36 -06:00

ssh.go

													
				@ -77,0 +79,4 @@

					uPath := strings.Replace(u.Path, `\`, `\\`, -1)

					uPath = strings.Replace(uPath, `'`, `\'`, -1)

					// TODO: Add percentage decode similar to libgit2.

darkowlzz (Migrated from github.com) commented

2021-11-07 13:51:35 -06:00

Thanks for pointing that out.
I've tried escaping them. Please correct me if it's not right.
And also, if the TODO message is reasonable.

Thanks for pointing that out. I've tried escaping them. Please correct me if it's not right. And also, if the TODO message is reasonable.

lhchavez (Migrated from github.com) reviewed 2021-11-07 13:55:56 -06:00

ssh.go

lhchavez (Migrated from github.com) commented

2021-11-07 13:55:56 -06:00

	uPath := strings.ReplaceAll(u.Path, `\`, `\\`)
	uPath = strings.ReplaceAll(u.Path, `'`, `\'`)

maybe it's easier to only use backticks to make it easier to understand: currently the first line is a no-op, because `\` == "\\" ! source: https://play.golang.org/p/gVcwOh-VM-f

```suggestion uPath := strings.ReplaceAll(u.Path, `\`, `\\`) uPath = strings.ReplaceAll(u.Path, `'`, `\'`) ``` maybe it's easier to only use backticks to make it easier to understand: currently the first line is a no-op, because ``` `\` == "\\"``` ! source: https://play.golang.org/p/gVcwOh-VM-f

darkowlzz (Migrated from github.com) reviewed 2021-11-07 14:03:19 -06:00

ssh.go

darkowlzz (Migrated from github.com) commented

2021-11-07 14:03:19 -06:00

Thanks for providing that example. Very helpful.
Updated the commit.

Thanks for providing that example. Very helpful. Updated the commit.

lhchavez (Migrated from github.com) reviewed 2021-11-07 14:17:30 -06:00

ssh.go

lhchavez (Migrated from github.com) commented

2021-11-07 14:17:24 -06:00

	uPath := strings.Replace(u.Path, `\`, `\\`, -1)
	uPath = strings.Replace(uPath, `'`, `\'`, -1)

ugh, i just realized this is not there yet in Go 1.11. Since this is easy to support in older versions of Go (thanks to strings.ReplaceAll being a very thin wrapper around strings.Replace), let's make this change.

```suggestion uPath := strings.Replace(u.Path, `\`, `\\`, -1) uPath = strings.Replace(uPath, `'`, `\'`, -1) ``` ugh, i just realized this is not there yet in Go 1.11. Since this is easy to support in older versions of Go (thanks to [`strings.ReplaceAll`](https://cs.opensource.google/go/go/+/refs/tags/go1.17.3:src/strings/strings.go;l=963) being a very thin wrapper around `strings.Replace`), let's make this change.

Sign in to join this conversation.

No reviewers