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

Regression: versions later than 1.8.1 no longer derive credentials from ssh-agent #226

Closed
oliverlockwood opened this issue Mar 7, 2018 · 25 comments
Milestone

Comments

@oliverlockwood
Copy link

oliverlockwood commented Mar 7, 2018

Scenario:

  • Jenkins pipeline build
  • Configuration of axion-release as follows:
    scmVersion {
        checks.aheadOfRemote = false
        repository.pushTagsOnly = true
        tag.prefix = project.name
        versionIncrementer 'incrementMinor'
    }
  • Credentials are loaded into the ssh-agent (via Jenkins sshagent pipeline step). They're not in, for example, the ~/.ssh/id-rsa file.

Previous behaviour

With axion-release version 1.8.1, the pushRelease task works just fine and tags are pushed to the repo.

Regression

With version 1.8.2 onwards, the following error occurs:

Pushing all to remote: origin
Exception occurred during push: org.eclipse.jgit.api.errors.TransportException: git@github.com:sky-uk/dap-core-dashboards.git: Auth fail

Next steps

I can see there's been a number of changes since 1.8.1 in the area of how Git is used by the plugin. So unpicking it may prove to be a little gnarly.

@oliverlockwood
Copy link
Author

oliverlockwood commented Mar 7, 2018

Doing a little bit of digging here, I suspect the first culprit is this commit where for a push (without explicit credentials configured through axion-release) we no longer use GrGit, and instead always directly call JGit.

GrGit was clearly doing some niceties, e.g. in PushOp, the JGit PushCommand gets configured further:

TransportOpUtil.configure(cmd, repo.credentials)

and within TransportOpUtil.configure() we see:

logger.info('The following authentication options are allowed (though they may not be available): {}', config.allowed)

I've seen this log mention HARDCODED, INTERACTIVE, SSHAGENT, PAGEANT before. So I suspect we need to either:

  1. reintroduce GrGit (which I'm guessing you don't want to!), or
  2. find a way to allow the configuring via sshagent, which GrGit was previously doing by magic (which I imagine you'd be happy with).

@adamdubiel
Copy link
Member

Thanks for reporting this! I suspected, that removing GrGit might lead to some problems in scenarios that we don't use at Allegro. I will try to fix it using pure JGit during the weekend. I will reach out if i have more questions (or need additional testing?)

@adamdubiel adamdubiel added this to the 1.9.1 milestone Mar 7, 2018
@oliverlockwood
Copy link
Author

No worries.

Note that when I dug further under the hood, the GrGit JschAgentProxySessionFactory class gets used, which is non-trivial.

I think this probably warrants some thought at a high level from the maintainers of the repo as to how you want to proceed. For example, do you want to:

  • allow identity to be derived from keys in the SSH agent implicitly (as before)?
  • add new configuration to Axion so that people must explicitly specify a property, in order to use SSH agent for identity?

@adamdubiel I'm happy to leave it in your hands, let me know if I can be of further help, in testing or otherwise.

@adamdubiel
Copy link
Member

I know all those classes :) Unfortunately i did not write all necessary SSH tests back in the days, so i have to pay the debt now.

@oliverlockwood
Copy link
Author

@adamdubiel any update, and can I be any of any help?

@adamdubiel adamdubiel modified the milestones: 1.9.1, 1.9.2 May 20, 2018
@oliverlockwood
Copy link
Author

@adamdubiel 3 months on, is there any chance of an update on this, please?

@adamdubiel
Copy link
Member

Update: no progress so far, although i would love to get some time to fix it. Is staying on 1.8.x a huge pain for you?

@adamdubiel adamdubiel removed this from the 1.9.2 milestone Jul 31, 2018
@oliverlockwood
Copy link
Author

@adamdubiel it's just that a number of teams keep bumping into this bug on our CI servers (which use sshagent keys to authenticate with Git) when they proactively bump their plugin versions up. So it's wasting a little bit of time for different people at various points, which is annoying 😄

@adamdubiel
Copy link
Member

Okay, i get it :) Maybe you find it annoying enough to create a PR? I know sshagent tests can be difficult, so i would welcome PR with manual tests only (as it was pre 1.9.x). I think the implementation in GrGit looks like a good point to start. I would really prefer not to reimport GrGit just for this (or maybe as an optional dep)?

@damnhandy
Copy link

Out of curiosity, what was the driver for dropping GrGit as a dependency? I ask as folks have been recommending the Rekon plugin, which does some of what Axion does but, but not as cleanly. Rekon also used GrGit so I'm just curious as what the driver was?

@adamdubiel
Copy link
Member

I had multiple cases of dependencies clash with different versions of GrGit on classpath. I figured out that all the basic operations in JGit are actually easy to perform. I was using JGit in a lot of places anyway due to performance concerns and GrGit not exposing all the APIs. Sadly the agent integration suffered, but this is the only downside of not using GrGit.

@k3mist
Copy link

k3mist commented Nov 7, 2018

I've been watching this ticket for awhile now and figured I would chime in.

We've been having this issue with Jenkins since 1.8.2 as well. I would like to keep our internal gradle plugin (written in scala) on par with your latest version since we do depend on this for all our internal project versioning (seriously Adam, thanks a ton for this plugin!)

Would having GrGit as an optional dependency via scmVersion closure (not quite sure the most elegant approach) be a potential stop-gap before a real solution can be implemented natively?

I'd personally work on a PR myself but my groovy is shit. (hence the reason I wrote our internal plugin in scala)

Anyway, I'm just wondering what options are available to alleviate the issue those us using sshagent are having without GrGit so we can stay on the latest release of this plugin.

Regardless 1.8.1 is still working fine for us but we obviously miss out on any bug fixes/new features in the meantime.

@adamdubiel
Copy link
Member

So i had a couple of hours on the train and decided to tackle this issue. I basically took a look at how grgit detects which ssh agent to use and imitated the code. I tested that basic scenario works, but since i'm not a heavy user of ssh-agent and writing proper tests would require a lot of work (create docker with ssh-agent and run axion-release inside), i would really appreciate if someone tried out the code. Branch: ssh-agent-support

@k3mist would you be willing to try it out?

Funny thing, newest rc versions of grgit 3.0 actually dropped support for jsch altogether and moved to native ssh. However i think that for use cases of this plugin, jsch is good enough.

Just like before 1.9.x, agent support is enabled by default. It can be disabled using -Prelease.disableSshAgent flag.

@k3mist
Copy link

k3mist commented Nov 12, 2018

@adamdubiel I will give it a run as soon as I get some time this week and I'll report back. Work is crazy right now so I'll try my best to get to it before Friday. Thank you for taking a shot at this!

@oliverlockwood
Copy link
Author

@adamdubiel I'll try and give some feedback this week as well if I have the chance. Thanks for taking the time to put together a candidate fix; apologies I never managed to make the time myself.

@adamdubiel
Copy link
Member

Great! Waiting for the news then :)

@k3mist
Copy link

k3mist commented Nov 17, 2018

@adamdubiel Nice job man!

Published the branch build to our nexus snapshot repo and then pulled it into our pipeline plugin

Everything looks good!

dep on our plugin
screenshot_20181117_093644

jenkins stages

screenshot_20181117_094724

create tag
screenshot_20181117_093557

push tag
screenshot_20181117_093614

remote repo
screenshot_20181117_094656

@adamdubiel
Copy link
Member

Yeah :) How about the amount of logging - not too much? Looks like you run it with ./gradlew -i? If everything works, i will merge and release it as 1.10.0.

@k3mist
Copy link

k3mist commented Nov 18, 2018

Logging looked fine/normal. Yeah we run all tasks with -i on the build server

@adamdubiel
Copy link
Member

I just released 1.10.0 version.

@adamdubiel adamdubiel added this to the 1.10.0 milestone Nov 22, 2018
@k3mist
Copy link

k3mist commented Dec 3, 2018

I was on vaca and now just getting a chance to respond.

Thanks so much for this @adamdubiel !!

@edgarvonk
Copy link

Hi @adamdubiel, thanks for the work! But I'm afraid it seems I am still getting the very same error also when using version 1.10.0.

@adamdubiel
Copy link
Member

@edgarvonk do you mind creating a new issue for this? I don't know what is the error :)

@gaglanihetal
Copy link

My team was able to figure out the issue is actually with the keys that are generated using OpenSSH versions after 7.8. If the key is generated using openssh7.4(like mine was) or openssl, the issue goes away. So whatever jgit is doing to load the key seems to be not supported.

@edgarvonk
Copy link

Aha, yes, @hgaglani that seems to be the issue.. We downgraded back to 1.8.1 and with that also the new SSH keys work. I believe this is now being tackled in #270 right @adamdubiel ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants