v0.22.0 appears to disable SSH access to git repositories

[mrthehud]

Probably relates to #233

The git config in the resource has changed to something like:

resource_git# cat .git/config 
[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[user]
	name = concourse-ci
	email = concourse@local
[url "https://x-oauth-basic@github.com/"]
	insteadOf = git@github.com:
[url "https://"]
	insteadOf = git://
[remote "origin"]
	url = https://x-oauth-basic:<token>@github.com/<org>/<repo>
	fetch = +refs/heads/*:refs/remotes/origin/*

This prevents us then running a yarn install to install our private dependencies using a private key.

[mrthehud]

I do get the idea behind this, and perhaps this isn't strictly a bug with v0.22.0, but I'm struggling to work out how to get yarn to use the askpass script, or otherwise use a token for our private repositories, without re-writing all our package.json's.
https://github.com/telia-oss/github-pr-resource/compare/v0.21.0..v0.22.0#diff-8e18fb19c78cb69939e3f2e91531323a5b0e214e2f22a6ec28f9e6b11d376ecbR2

I wonder if the new functionality should be opt-in?
https://github.com/telia-oss/github-pr-resource/compare/v0.21.0..v0.22.0#diff-a5b3c7179f46260946322aeabb36e11915f518ad02939b3923a39dc6a85cb698R77-R82

For now we're pinning at v0.21.0

[rickardl]

Hi,

Thanks for the reporting, if you like to contribute I guess we would consider adding a feature flag to change this behaviour for your use case.

[mrthehud]

Thanks. It certainly would help us out, so if it's something that would be considered then it would make sense for us to spend some time on it. Before we do, would something like the following be ideal?

| `skip_repository_uri_config` | No | `true` | Disable repository uri replacement. Useful if you use private repositories. Use with care! |

This would toggle out the two git config calls here:
https://github.com/telia-oss/github-pr-resource/compare/v0.21.0..v0.22.0#diff-a5b3c7179f46260946322aeabb36e11915f518ad02939b3923a39dc6a85cb698R77-R82

I have no idea what else that will break yet, will obviously test as best as we can if provisionally happy with this approach.

[jhosteny]

Hi @mrthehud, can you elaborate a bit on how this is breaking for you? How is this interacting with yarn install?

I added this, so I apologize for any trouble. I had a further change in #200 to make it fully work for private submodules, so I'd like to make sure I can fix that too if need be.

[jhosteny]

@rickardl I would add that the insteadOf is only necessary for private submodule support, so a good proxy for feature flagging it may be the submodules field on the get config.

[mrthehud]

Hi @jhosteny, it's no trouble, thank you for the features :)

Our situation:
We use this resource to pull our app code, then run yarn to install dependencies, before running tests and packaging for deployment. We also use yarn to install private dependencies, using something like the following in a package.json:

    "private-dep": "git+ssh://git@github.com/our-org/private-dep#0.1.1",

With this config, yarn would use the ssh key that we add to the container in our build/test/package job to checkout those from github using SSH.

With the new feature, what appears to happen (and I'm a bit unsure here) is that because of the change to .git/config, yarn tries to pull by ssh, but the underlying git process then tries to use https, with the x-oauth-basic user.

Another possible solution that's come to mind is to revert those changes after the pull to fetch submodules - although that would be more involved. We could possibly handle that in our jobs by removing the two config lines from git config, but to me it seems that trying to achieve that in the resource would be more appropriate.

How feasible do you think it might be to return the git config to the state it was in before finishing the get?

Alternatively, using the submodules flag to toggle this sounds like a good idea too.

[jhosteny]

Hi @mrthehud,

So, one thing I am confused by is why git is performing a substitution here. If I look at a config in one of my active PR builds, I see (in the .git/config):

url.https://x-oauth-basic@github.com/.insteadof=git@github.com:

This is consistent with the code in git.go. Your URL should not match git@github.com:, and thus not get the substitution. Are you able to share your repo, or perhaps one with any proprietary code stripped out?

Two other things to note: the original change used --global flag with git config. This was flagged in review for polluting the config for testing, but I think that would confine the changes to the get container step, since the mapped repository config would not be changed.

Second, I think that #200 may be able to remove the generic insteadOf config, since the substitution is now passed explicitly for all the submodule commands. I will test that out, since I need #200 to be merged anyway.

The only issue I have with your suggestion is that we would likely have to make a copy of the original config file and restore it when it is done, which feels incorrect to me. If it comes to that, it might be better to work with the maintainers to see if there is a way to restore the --global flag without affecting the tests.

Regardless, I'd like to understand why the substitution is even occurring for you.

[mrthehud]

Unfortunately I'm not able to share the repository, but our dependency URLs are of the form: "private-dep": "git+ssh://git@github.com/our-org/private-dep#0.1.1", which matches the git@github.com in the insteadOf, which I think causes git to try and check out https://x-oauth-basic@github.com/our-org/private-dep#0.1.1 in the build step, which then attempts to ask for a password / token, but fails as it's not interactive.

--global would help here as you suggest, and I agree that trying to backup and reapply the config is messy, and a bit of a non-starter. If using --global is untenable, perhaps calling a cleanup function after the submodule update that performs an unset would help?

git config --unset url.https://x-oauth-basic@github.com/.insteadOf
git config --unset url.https://.insteadOf

Conditionally setting those in the Init would also probably solve our case, but might leave this problem open to people who are using submodules AND private yarn dependencies. I imagine that isn't very likely, but not impossible either.

When this happened, I hijacked the container to have a peek. The below contains some of what I found at the time:

root@4a456fc9-a9d1-4f24-7910-20c72f8eb311:/tmp/build/50979f87# cd resource_git/

root@4a456fc9-a9d1-4f24-7910-20c72f8eb311:/tmp/build/50979f87/resource_git# yarn install --verbose
...
verbose 1.459789031 Error: Command failed.
Exit code: 128
Command: git
Arguments: ls-remote --tags --heads git@github.com:our-org/private-dep.git
Directory: /tmp/build/50979f87/resource_git
Output:
fatal: could not read Password for 'https://x-oauth-basic@github.com': terminal prompts disabled
    at ProcessTermError.ExtendableBuiltin (/opt/yarn-v1.22.4/lib/cli.js:721:66)
    at ProcessTermError.MessageError (/opt/yarn-v1.22.4/lib/cli.js:750:123)
    at new ProcessTermError (/opt/yarn-v1.22.4/lib/cli.js:790:113)
    at ChildProcess.<anonymous> (/opt/yarn-v1.22.4/lib/cli.js:25884:17)
    at ChildProcess.emit (events.js:315:20)
    at maybeClose (internal/child_process.js:1021:16)
    at Socket.<anonymous> (internal/child_process.js:443:11)
    at Socket.emit (events.js:315:20)
    at Pipe.<anonymous> (net.js:674:12)
error Command failed.
Exit code: 128
Command: git
Arguments: ls-remote --tags --heads git@github.com:our-org/private-dep.git
Directory: /tmp/build/50979f87/resource_git
Output:
fatal: could not read Password for 'https://x-oauth-basic@github.com': terminal prompts disabled
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

root@4a456fc9-a9d1-4f24-7910-20c72f8eb311:/tmp/build/50979f87/resource_git# GIT_TRACE=2 git ls-remote --tags --heads git@github.com:our-org/private-dep.git
11:09:28.536814 git.c:371               trace: built-in: git 'ls-remote' '--tags' '--heads' 'git@github.com:our-org/private-dep.git'
11:09:28.536964 run-command.c:350       trace: run_command: 'git-remote-https' 'git@github.com:our-org/private-dep.git' 'https://x-oauth-basic@github.com/our-org/private-dep.git'
Password for 'https://x-oauth-basic@github.com': ^C

root@4a456fc9-a9d1-4f24-7910-20c72f8eb311:/tmp/build/50979f87/resource_git# cat .git/config 
[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[user]
	name = concourse-ci
	email = concourse@local
[url "https://x-oauth-basic@github.com/"]
	insteadOf = git@github.com:
[url "https://"]
	insteadOf = git://
[remote "origin"]
	url = https://x-oauth-basic:<TOKEN>@github.com/our-org/main-project
	fetch = +refs/heads/*:refs/remotes/origin/*

[jhosteny]

Ah, here is the offender:

Arguments: ls-remote --tags --heads git@github.com:our-org/private-dep.git

The git@github.com: should not have matched because of the trailing :, but this obviously will.

I will have a version of the resource today that will remove the git config step from my pending PR, as I believe it is unnecessary with the other changes that are in there. I can push that to Docker Hub if you would like to try after I verify that it works for us.

[jhosteny]

@mrthehud this is fixed with the latest commit for #200. The top level config was no longer necessary given that the insteadOf is passed on the command line to each submodule command.

[mrthehud]

Thanks @jhosteny! I'm not sure when I can get a chance to fix this, but I will try to soon.

[mrthehud]

Update on that - I've tested, and your fix works for me!