v0.22.0 appears to disable SSH access to git repositories #234

mrthehud · 2020-10-22T11:40:21Z

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://[email protected]/"]
	insteadOf = [email protected]:
[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.

The text was updated successfully, but these errors were encountered:

mrthehud · 2020-10-22T11:57:07Z

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 · 2020-10-22T20:46:15Z

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 · 2020-10-23T13:42:19Z

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 · 2020-10-29T19:54:36Z

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 · 2020-10-30T12:50:13Z

@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 · 2020-11-02T12:04:00Z

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://[email protected]/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 · 2020-11-02T13:57:00Z

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://[email protected]/[email protected]:

This is consistent with the code in git.go. Your URL should not match [email protected]:, 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 · 2020-11-02T16:17:03Z

Unfortunately I'm not able to share the repository, but our dependency URLs are of the form: "private-dep": "git+ssh://[email protected]/our-org/private-dep#0.1.1", which matches the [email protected] in the insteadOf, which I think causes git to try and check out https://[email protected]/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://[email protected]/.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 [email protected]:our-org/private-dep.git
Directory: /tmp/build/50979f87/resource_git
Output:
fatal: could not read Password for 'https://[email protected]': 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 [email protected]:our-org/private-dep.git
Directory: /tmp/build/50979f87/resource_git
Output:
fatal: could not read Password for 'https://[email protected]': 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 [email protected]:our-org/private-dep.git
11:09:28.536814 git.c:371               trace: built-in: git 'ls-remote' '--tags' '--heads' '[email protected]:our-org/private-dep.git'
11:09:28.536964 run-command.c:350       trace: run_command: 'git-remote-https' '[email protected]:our-org/private-dep.git' 'https://[email protected]/our-org/private-dep.git'
Password for 'https://[email protected]': ^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://[email protected]/"]
	insteadOf = [email protected]:
[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 · 2020-11-02T16:42:43Z

Ah, here is the offender:

Arguments: ls-remote --tags --heads [email protected]:our-org/private-dep.git

The [email protected]: 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 · 2020-11-02T19:10:16Z

@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 · 2020-11-05T16:46:58Z

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

mrthehud · 2020-11-05T17:26:40Z

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

jhosteny mentioned this issue Nov 2, 2020

fix: ensure insteadOf gets picked up for submodules #200

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v0.22.0 appears to disable SSH access to git repositories #234

v0.22.0 appears to disable SSH access to git repositories #234

mrthehud commented Oct 22, 2020

mrthehud commented Oct 22, 2020 •

edited

rickardl commented Oct 22, 2020

mrthehud commented Oct 23, 2020 •

edited

jhosteny commented Oct 29, 2020

jhosteny commented Oct 30, 2020

mrthehud commented Nov 2, 2020

jhosteny commented Nov 2, 2020

mrthehud commented Nov 2, 2020

jhosteny commented Nov 2, 2020

jhosteny commented Nov 2, 2020

mrthehud commented Nov 5, 2020

mrthehud commented Nov 5, 2020

v0.22.0 appears to disable SSH access to git repositories #234

v0.22.0 appears to disable SSH access to git repositories #234

Comments

mrthehud commented Oct 22, 2020

mrthehud commented Oct 22, 2020 • edited

rickardl commented Oct 22, 2020

mrthehud commented Oct 23, 2020 • edited

jhosteny commented Oct 29, 2020

jhosteny commented Oct 30, 2020

mrthehud commented Nov 2, 2020

jhosteny commented Nov 2, 2020

mrthehud commented Nov 2, 2020

jhosteny commented Nov 2, 2020

jhosteny commented Nov 2, 2020

mrthehud commented Nov 5, 2020

mrthehud commented Nov 5, 2020

mrthehud commented Oct 22, 2020 •

edited

mrthehud commented Oct 23, 2020 •

edited