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

[JENKINS-56167] Replace Trilead-ssh2 PEMParser class #3902

Merged
merged 7 commits into from Feb 25, 2019

Conversation

kuisathaverat
Copy link
Contributor

See JENKINS-56167.

Replace Trilead-ssh2 PEMParser class by Mina SSHD SecurityUtils.loadKeyPairIdentity implementation, this allows to remove the trilead-ssh2 library from the core and support the new key OpenSSH armory format.

Proposed changelog entries

  • Change CLI load SSH keys implementation.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@oleg-nenashev @Wadeck @daniel-beck @alecharp @jeffret-b

…urityUtils.loadKeyPairIdentity implementation
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.

Looks good overall.

cli/src/test/java/hudson/cli/PrivateKeyProviderTest.java Outdated Show resolved Hide resolved
@@ -0,0 +1,12 @@
-----BEGIN DSA PRIVATE KEY-----
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if there's an easy pure-Java way to generate these keys, but using randomly generated keys each time the test is run would at least prevent security scanners from complaining about versioned secrets.

Copy link
Contributor

Choose a reason for hiding this comment

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

There probably isn't a way to easily generate these for tests. Do security scanners look in test code for such things?

Copy link
Member

Choose a reason for hiding this comment

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

Some do, yes. I updated a test in jsch-plugin to avoid the warning: jenkinsci/jsch-plugin#6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those keys are generated with ssh-keygen that it is probably the most common tool to make it. If I make it using Java, it would add a bunch of code not related to tests.

Copy link
Contributor

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

The replacement code is much simpler and there's a nice set of tests.

cli/src/main/java/hudson/cli/PrivateKeyProvider.java Outdated Show resolved Hide resolved
CEuJqU5VRW5WlayYRUsJnQTSaeUuJJvQWAeo9TI/DtYzvp8AAAADAQABAAAAgBRXdq7kj/
iR+WIEs7uifSMwuPGDjtxksg2Uj09kSGRLFmZdu4EWtvUh0uV0J37vbfBSkubU3fAvrP99
bRxUHhD5Z444BIyht8jlBetfoJOBSE/TQJ/69xguSmHB8XH8/WUqEaNZ2F+q0AAkRt5CTs
y0qIAH7i6ZHFqHlr1OnCPzqtzIt4McoyIDEf+9eH3ktKAAAAQQD8uQ/jcs27tMPwb2GYyU
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR: the line lkML/YJI1mPzy+0ny6tS8hAAAAQQD6N3CByknj5WrDIJQCce+zbhbftnN4RM6OBuaHv4mm from openssh was removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is made on purpose to break the key and cause an error

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I put the comment for the next reviewers, to make the things clearer :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you didn't see the comment in the test :P

}

@Test(expected = NoSuchAlgorithmException.class)
public void loadKeyUnsupportedCipher() throws IOException, GeneralSecurityException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment explaining how the key was generated, what cypher is not supported, etc. Without more information the 3 keys are equivalent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

Much better, thank you very much!

Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

LGTM

@Wadeck Wadeck added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 22, 2019
@oleg-nenashev oleg-nenashev merged commit 46cbaf5 into jenkinsci:master Feb 25, 2019
@oleg-nenashev
Copy link
Member

Thanks @kuisathaverat !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
6 participants