-
Notifications
You must be signed in to change notification settings - Fork 105
[JENKINS-28399] Add binding for SSH private keys #18
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
Conversation
|
ping. Any chance this might get merged in? |
|
Hello? Is this plugin still active? |
|
Not sure how to vote this one up, but we'd love to have this as well. |
|
I will take a look when I have a moment. |
|
JENKINS-28399 I suppose. Note that the usual alternative is to use the SSH Agent plugin so that the key is not actually used directly. |
|
Maybe this could/should be implemented in the jenkinsci/ssh-credentials-plugin instead? From discussion in #22, my understanding was that it would be better to implement new bindings directly in the plugins which provide the credentials types, inverting the dependency (that's what I ended up proposing in jenkinsci/docker-commons-plugin#54 for Docker client certificates). |
|
fwiw, I did not want to use the ssh agent plugin because of its extra installation burden (dependency on tomcat native libraries and needing to install BouncyCastle as a security provider). Having direct access to the key turned out to be easier. Not sure about where this should live. Seems like putting the ssh key binding into the "binding" plugin seemed like the best place to me. I'll leave that decision up to the maintainer as to where it should live. |
No strong preference I guess. Generally “domain-specific” bindings should live in the plugin defining that JENKINS-19508 is really the right solution IMO, but that is not available now. @stephenc any particular opinion? |
| return new FilePath(workspace.getChannel(), workspace.act(new FilePath.FileCallable<String>() { | ||
| @Override | ||
| public String invoke(File dir, VirtualChannel channel) throws IOException, InterruptedException { | ||
| File f = File.createTempFile(prefix, suffix); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work with Image.inside from docker-workflow. It is necessary to use the standard “temp workspace”. See FileBinding and #25.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
|
||
| @Override | ||
| public void checkRoles(RoleChecker checker) throws SecurityException { | ||
| // no op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Extend MasterToSlaveFileCallable instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| } | ||
|
|
||
| @Override public Set<String> variables() { | ||
| return new HashSet<String>(Arrays.asList(keyFileVariable, usernameVariable, passphraseVariable)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tip: ImmutableSet.of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| } | ||
|
|
||
| @Override public void unbind(Run<?, ?> build, FilePath workspace, Launcher launcher, TaskListener listener) throws IOException, InterruptedException { | ||
| new FilePath(workspace.getChannel(), this.filePath).delete(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workspace.child(filePath)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| @Rule public RestartableJenkinsRule story = new RestartableJenkinsRule(); | ||
| @Rule public TemporaryFolder tmp = new TemporaryFolder(); | ||
|
|
||
| private static class DummyPrivateKey extends BaseCredentials implements SSHUserPrivateKey, Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be unnecessary; use a normal BasicSSHUserPrivateKey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was going to do that originally but got hung on all the other stuff necessary to construct one (like PrivateKeySource), and seemed easier to just create a simple implementation of the interface here. Are you okay maybe with leaving this as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. BasicSSHUserPrivateKey would probably be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would prefer to keep as-is if it's okay with you.
|
|
||
| @Test public void configRoundTrip() throws Exception { | ||
| story.addStep(new Statement() { | ||
| @SuppressWarnings("rawtypes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collections.<TheType>singletonList
| WorkflowRun b = p.getBuildByNumber(1); | ||
| assertNotNull(b); | ||
| SemaphoreStep.success("basics/1", null); | ||
| while (b.isBuilding()) { // TODO 1.607+ use waitForCompletion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use story.j.waitForCompletion. Jenkins dependency irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
Is this dead in the water? I'm trying to figure out how to inject SSH creds into a kubernetes jnlp slave. I think I'm stuck with using a custom image with a key baked in. |
|
Sorry, by the time the review came in I had ditched this workflow for something else and have been distracted since. I can get this back up to date and address the review feedback. |
|
No problem! I've got a workaround going. |
|
just rebased from master. will update per review feedback. |
|
@jglick I think this plugin is the correct place given that JENKINS-19508 has not been implemented (ssh-credentials being slightly more generic than this) |
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>ssh-credentials</artifactId> | ||
| <version>1.11</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just make it a mandatory dep, as we do for plain-credentials: ssh-credentials does no harm if unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay. was trying to be less aggressive here. can change this if you like.
| /* | ||
| * The MIT License | ||
| * | ||
| * Copyright 2013 jglick. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? :-)
|
|
||
| @Override public MultiEnvironment bind(Run<?,?> build, FilePath workspace, Launcher launcher, TaskListener listener) throws IOException, InterruptedException { | ||
| SSHUserPrivateKey sshKey = getCredentials(build); | ||
| FilePath keyFile = tempDir(workspace).child("ssh-key-" + keyFileVariable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use the new API in #25 when ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see you reverted that PR. Do you want to wait until that api is ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Please ping me then when it is ready and I can update this PR.
| public final String usernameVariable; | ||
| public final String passphraseVariable; | ||
|
|
||
| @DataBoundConstructor public SSHUserPrivateKeyBinding(String keyFileVariable, String passphraseVariable, String usernameVariable, String credentialsId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since passphraseVariable and usernameVariable are not necessarily required, better to make these @CheckForNull properties with @DataBoundSetter.
| /* | ||
| * The MIT License | ||
| * | ||
| * Copyright 2015 Jesse Glick. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hah. was copy-pasting from other tests probably. i'll take that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I just leave out this copyright?
| WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p"); | ||
| p.setDefinition(new CpsFlowDefinition("" | ||
| + "node {\n" | ||
| + " withCredentials([[$class: 'SSHUserPrivateKeyBinding', keyFileVariable: 'THEKEY', passphraseVariable: 'THEPASS', usernameVariable: 'THEUSER', credentialsId: '" + credentialsId + "']]) {\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a @Symbol.
| @Rule public RestartableJenkinsRule story = new RestartableJenkinsRule(); | ||
| @Rule public TemporaryFolder tmp = new TemporaryFolder(); | ||
|
|
||
| private static class DummyPrivateKey extends BaseCredentials implements SSHUserPrivateKey, Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. BasicSSHUserPrivateKey would probably be easier.
|
Hey, any chance to merge this soon? Would be much appreciated :) |
|
It's already been another month since #25 was opened and closed. Not seeing any open PRs on that either. It seems strange to me to hold up this PR (which other people than myself clearly have interest in) for a new API that isn't there yet, and it is a little frustrating how long this PR has dragged on. If you guys think this is a worthwhile feature to add, and if the current implementation here is correct, then why not just merge in this PR now and change to use the new API when it is done? |
|
#36 now. |
| } | ||
|
|
||
| @DataBoundSetter | ||
| @CheckForNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This annotation is meaningless on a method returning void! Did you mean to place it on the getter?
| } | ||
|
|
||
| @DataBoundSetter | ||
| @CheckForNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| UnbindableDir keyDir = UnbindableDir.create(workspace); | ||
| FilePath keyFile = keyDir.getDirPath().child("ssh-key-" + keyFileVariable); | ||
|
|
||
| StringWriter stringWriter = new StringWriter(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler to use FilePath.write().
| @@ -0,0 +1,9 @@ | |||
| <div> | |||
| Copies the SSH key file given in the credentials to a temporary location, then sets a variable to that location. | |||
| (The file is deleted when the build completes.) Also sets variables for the SSH key's username and passphrase. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to create help-passphraseVariable.html giving specifics, such as that two parameters are optional. Can also add doCheck* methods to the descriptor guiding the user to fill in keyFileVariable but allowing the others to remain blank.
|
After I get #42 to pass and merge it, you will need to |
|
Done, please merge. |
|
Thank you for pushing this through!! |
jglick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some test failures caught by CI.
| + "node {\n" | ||
| + " withCredentials([sshUserPrivateKey(keyFileVariable: 'THEKEY', passphraseVariable: 'THEPASS', usernameVariable: 'THEUSER', credentialsId: '" + credentialsId + "')]) {\n" | ||
| + " semaphore 'basics'\n" | ||
| + " sh '''\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| + "node {\n" | ||
| + " withCredentials([sshUserPrivateKey(keyFileVariable: 'THEKEY', credentialsId: '" + credentialsId + "')]) {\n" | ||
| + " semaphore 'basics'\n" | ||
| + " sh '''\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| p.setDefinition(new CpsFlowDefinition("" | ||
| + "node {\n" | ||
| + " withCredentials([sshUserPrivateKey(keyFileVariable: 'THEKEY', credentialsId: '" + credentialsId + "')]) {\n" | ||
| + " semaphore 'basics'\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading semaphore name BTW; could use a more generic name (they are not shared between tests).
| + " withCredentials([sshUserPrivateKey(keyFileVariable: 'THEKEY', credentialsId: '" + credentialsId + "')]) {\n" | ||
| + " semaphore 'basics'\n" | ||
| + " sh '''\n" | ||
| + " set +x\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not actually necessary
| + " withCredentials([sshUserPrivateKey(keyFileVariable: 'THEKEY', passphraseVariable: 'THEPASS', usernameVariable: 'THEUSER', credentialsId: '" + credentialsId + "')]) {\n" | ||
| + " semaphore 'basics'\n" | ||
| + " sh '''\n" | ||
| + " set +x\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not actually necessary
|
yeah, the test were written originally w/ only linux in mind.... |
|
Need to either update them to run on Windows, or at a minimum assumeFalse(Functions.isWindows()); // TODO provide a Windows equivalent |
|
sorry. been really busy at work. getting around to this now. |
|
woot! build passed! 🎉 |
|
How would you access this using e.g: |
|
How can I use this inside a Wrapper, like I would do for usernamePassword. Is it possible with latest version? |
|
Hello, I try the usage example (https://github.com/jenkinsci/credentials-binding-plugin/blob/master/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/SSHUserPrivateKeyTest.java#L154) as follow: node {
key = 'mykey'
stage('Obtención de credenciales') {
withCredentials([sshUserPrivateKey(keyFileVariable: 'THEKEY',
passphraseVariable: 'THEPASS',
usernameVariable: 'THEUSER', credentialsId: key)])
{
sh 'echo THEUSER=[$THEUSER] THEKEY=[$THEKEY] THEPASS=[$THEPASS]'
}
}
}But didn't work because: java.lang.NoSuchMethodError: No such DSL method 'sshUserPrivateKey' found among stepsAny advice is appreciated |
|
I am using Jenkins 2.80 with a username / private key in my credentials. I am attempting to use the Credentials Binding Plugin v. 1.13 to retrieve the credentials with the |
|
I should note that I finally discovered what I was doing wrong. To retrieve my key, I did the following in my Hope this helps someone else. |
|
@mbarzare - I am trying to provide the "SSH User Private Key" that I have created globally to a promoted job. While it lets me create a "Credentials Parameter" and select "SSH Username with Private Key", and set its default value to my Global credential, when I echo it, I just see the UUID for the credentials. How can I extract the username, and the private key from it? I have used the "SSH User Private Key" binding in the main Build section, but I need to access the same credentials to use in Promotions. I am not sure how to use the UUID in a shell script to extract the credentials from the UUID. Any help would be appreciated? Thanks. |
|
i think this github issue is becoming a support forum -- can we lock this issue maybe? |
|
Sorry but is this documentation still correct according to this feature? |
|
@kormotodor |
|
@jglick So you mean, that it is impossible to call method "sshUserPrivateKey" from "credentialsBinding" plugin in dsl script on groovy for freestyle job? I expected exactly that behavior like when i use just like in documentation is described. |
|
@kormotodor I do not know, I do not maintain the |
JENKINS-28399