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

Remove ssh credentials from support plugin #862

Merged
merged 3 commits into from
Apr 29, 2019
Merged

Remove ssh credentials from support plugin #862

merged 3 commits into from
Apr 29, 2019

Conversation

jetersen
Copy link
Member

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!

  • Please describe what you did

  • Link to relevant GitHub issues or pull requests

  • Link to relevant Jenkins JIRA issues

  • Did you provide a test-case? That demonstrates feature works or fixes the issue.

Description

Thanks, @jvz for your work on jenkinsci/ssh-credentials-plugin#40

Means we can get rid of another support configurator!

@@ -38,13 +38,6 @@
<optional>true</optional>
</dependency>

<dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you verified / is there a test that shows that the private key isn't exported in plain text?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YES 😆

Arguably this should be updated to check the direct key

List<BasicSSHUserPrivateKey> creds2 = CredentialsProvider.lookupCredentials(BasicSSHUserPrivateKey.class,Jenkins.getInstanceOrNull(), null, Collections.emptyList());
assertThat(creds2.size(), is(1));
assertEquals("agentuser", creds2.get(0).getUsername());
assertEquals("password", creds2.get(0).getPassphrase().getPlainText());
assertEquals("ssh private key used to connect ssh slaves", creds2.get(0).getDescription());

privateKeySource:
directEntry:
privateKey: sp0ds9d+skkfjf

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently not ready, see CI... the test ran locally 😕

Copy link
Member Author

@jetersen jetersen Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvz we need some more magic it seems!
We just needed to fix our tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really needs to be, as it was a security issue, and we don't want it accidentally re-introduced

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works locally, I don't get it 😕

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why its trying to use this:

Caused by: java.lang.IllegalArgumentException: No com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey$PrivateKeySource implementation found for FileOnMasterPrivateKeySource
Possible solution: Try to install 'configuration-as-code-support' plugin

it should be directEntry not the fileOnMaster...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in: 202ab7b

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add an export test please to ensure what is exported isn't plain text

jvz
jvz previously approved these changes Apr 25, 2019
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray!

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the missing test @Casz, mind checking you're happy with it when you get a chance please?

assertThat(ssh_with_passphrase.getPassphrase().getPlainText(), equalTo("ABCD"));

final DirectEntryPrivateKeySource source = (DirectEntryPrivateKeySource) ssh_with_passphrase.getPrivateKeySource();
assertThat(source.getPrivateKey(), equalTo("s3cr3t"));
assertThat(source.getPrivateKey().getPlainText(), equalTo("s3cr3t"));
Copy link
Member

@jvz jvz Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker by any means, but it could be useful to create a matcher for secrets so you'd be able to assert something like assertThat(source.getPrivateKey(), hasPlainText("s3cr3t")); which would read better. Most appropriate place to add it would be here.

Edit: I submitted a PR for that idea.

@timja timja added the removed A PR that removes code - used by Release Drafter label Apr 29, 2019
@timja timja merged commit 8058906 into jenkinsci:master Apr 29, 2019
@jetersen jetersen deleted the ssh-credentials branch April 29, 2019 08:22
@jetersen jetersen changed the title remove ssh credentials from support plugin Remove ssh credentials from support plugin Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removed A PR that removes code - used by Release Drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants