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

Bump Trilead version to receive a number of security enhancements #2848

Merged
merged 1 commit into from
Apr 29, 2017
Merged

Bump Trilead version to receive a number of security enhancements #2848

merged 1 commit into from
Apr 29, 2017

Conversation

mc1arke
Copy link
Member

@mc1arke mc1arke commented Apr 18, 2017

Description

Move to a new version of Trilead to receive a number of improvements in SSH security handling
See:

This is a stop-gap measure until JENKINS-43610 is completed.

This version of Trilead is backwards compatible with the previous version hence the lack of new tests. Jenkins CLI has not been updated since it seems to be out of sync with the core version, and has a hard-coded list of supported keys which doesn't include the new keys introduced in the latest version of Trilead.

Note: This plugin does introduce dependencies on two new JARs. It may be worth setting them to 'optional' in the Jenkins POM to prevent them being exposed to core dependents.

Changelog entries

Proposed changelog entries:

Submitter checklist

  • JIRA issue is well described
  • Link to JIRA ticket in description, if appropriate
  • Appropriate autotests or explanation to why this change has no tests
  • For new API and extension points: Link to the reference implementation in open-source (or example in Javadoc)

Desired reviewers

@jenkinsci/code-reviewers I'm particularly interested in views on whether we hide the new dependencies of Trilead.

@paladox
Copy link

paladox commented Apr 18, 2017

+1. I've tested this locally and fixes ssh slaves with using hmac 256 and the newer sshd standards too.

Could this also be back ported to the jenkins its 2.41 release as that is the last lts release to support java 7 please?

@oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Apr 18, 2017
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

After a quick look the only binary compat issue is removal of public SHA1#main(), which is likely fine.

Since it has been tested by @paladox, I think we could go forward and merge it. Maybe it worth running ATH/PCT just in case.

Regarding the backporting, this change is, ehm, slightly bigger than we commonly backport. But I would start from getting it into weekly in order to collect the community feedback.

@oleg-nenashev
Copy link
Member

@jenkinsci/code-reviewers We need more votes in this case

@paladox
Copy link

paladox commented Apr 21, 2017

bump :)

@oleg-nenashev
Copy link
Member

@paladox @mc1arke Could you please confirm https://issues.jenkins-ci.org/browse/JENKINS-41606 is going to be resolved? There is no updates in the ticket, but segfault risk seems to be hot enough independently from all other fixes.

For the future, maybe it makes sense to switch trilead SSH versioning to something standard. IIRC Jenkins fork is the only more or less alive fork of the original lib, so maybe it could get a standard version (maybe with the major bump)

@mc1arke
Copy link
Member Author

mc1arke commented Apr 22, 2017

@oleg-nenashev JENKINS-41606 isn't going to be fully resolved with this: the logs on that issue show failures during the MD5, SHA1 and AES operation. This update changed Trilead to use the JVMs MD5 and SHA1 hashing implementations - rather than the Trilead hand-crafted versions - which should hopefully be more resilient, but AES stays as the Trilead hand-spun version so would still be susceptible to the same failures. My hope is that the move to the JVM implementations will either resolve the issue, or provide some more information that will help track down the issue (a proper stack trace rather than a core log). Unfortunately the core logs don't give me enough information to accurately identify the cause; I'd need the actual core dump to analyze the true failure, and I've had no joy so far with trying to replicate the issue.

The only other active fork or Trilead I've come across is an implementation called 'sshlib' run under the 'connectbot' project. Given the divergence between the two projects, I'd be happy starting to follow a standard release numbering scheme for our releases.

@oleg-nenashev
Copy link
Member

Yep, if connectbot people want to share the burden, it could even make sense to move the current fork outside jenkinsci org to a separate organization (or just integrate two forks there). Though it is not the top priority of course :)

@paladox
Copy link

paladox commented Apr 22, 2017

@oleg-nenashev hi, that problem may be fixed by going with hmac 256. Though wikimedia haven't experienced a problem like that but they use java 7 and jenkins 1.x on the slaves and prod machine. (well not that i see anyways, i see no tasks for any problems with ssh slaves in wikimedia's phabrciator)

So this could be java 8. But i also haven't experienced the problem on the test server and slave i have which uses jenkins 2.x and java 8.

@oleg-nenashev
Copy link
Member

@jenkinsci/code-reviewers My plan is to merge it towards next weekly if nobody votes against

@oleg-nenashev oleg-nenashev self-assigned this Apr 25, 2017
@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Apr 25, 2017
@daniel-beck daniel-beck self-requested a review April 25, 2017 19:41
@mc1arke
Copy link
Member Author

mc1arke commented Apr 25, 2017

My only change I'd consider in this is to hide the (new) dependencies of Trilead so we don't expose BCrypt and ED25519 encryption algorithms to Jenkins core or plugins. This is purely to prevent them being picked up as dependencies by other plugins, so makes removing Trilead from core a simpler concept, but isn't essential (and may be an unlikely scenario anyway). I'm happy to hear thoughts on this.

@paladox
Copy link

paladox commented Apr 27, 2017

We should keep the dep on ED25519 but I do think something will conflict as I'm aware that sshd 1.4.0 uses ED25519 but it uses the older version then the one in trilead so there could be conflicting classes.

@oleg-nenashev
Copy link
Member

Tested the PR manually together with #2853 . The stuff didn't fall apart, but I have not tried to invoke ED25519 code (EdDSA lib).

IMHO we cannot integrate SSHD Core till it gets upgraded to EdDSA 0.2.0, unless Trilead SSH somehow shades this library. CC @jglick

@oleg-nenashev
Copy link
Member

Full diff between EdDSA 0.1.0 and 0.2.0: str4d/ed25519-java@v0.1.0...a297e51

@jglick
Copy link
Member

jglick commented Apr 27, 2017

@oleg-nenashev I have no idea what the discussion is about. If there is something I need to fix let me know what that is.

@paladox
Copy link

paladox commented Apr 28, 2017

@oleg-nenashev and @mc1arke hi, I've notice that in #2853 it updated the cli/pom file where as this pull here updates core. Would that mean that if we used the eddsa 0.2.0 in core/pom would it conflict in cli/pom eddsa 0.1.0? Or would it even load eddsa from the core Pom in cli?

@oleg-nenashev
Copy link
Member

@paladox CLI is being packaged independently from the Core, and it is not being exposed to the core classpath. So We do not need to care about CLI until we merge SSHD Core in war/pom.xml

@paladox
Copy link

paladox commented Apr 28, 2017

Whoops just saw the war pom.

@oleg-nenashev
Copy link
Member

So my proposal would be to land this change with a higher version into the Weekly. Then let's see what do we do with SSHD Core. I'll raise the question about 1.5.0 release in their channel

@mc1arke
Copy link
Member Author

mc1arke commented Apr 29, 2017

👍 from me

@paladox
Copy link

paladox commented Apr 29, 2017

+1 too as then ssh slaves can gain hmac 256 which is already in sshd 1.2.0

@oleg-nenashev oleg-nenashev merged commit 6ff5086 into jenkinsci:master Apr 29, 2017
@oleg-nenashev
Copy link
Member

OK, I'll see if we can land SSHD next week

@paladox
Copy link

paladox commented Apr 29, 2017

Thanks :)

@daniel-beck
Copy link
Member

@paladox
Copy link

paladox commented Jun 9, 2017

@daniel-beck hi, I've been testing with Debian Jessie and stretch and cannot reproduce ( though I am using the latest Jenkins release)

Maybe we should backport 7dbcd02 ? Includes three fixes https://github.com/jenkinsci/trilead-ssh2/commits/master

(Which was merged on the 2.60 branch.)

@paladox
Copy link

paladox commented Jun 9, 2017

@paladox
Copy link

paladox commented Jun 9, 2017

@oleg-nenashev ^^

@paladox
Copy link

paladox commented Jun 9, 2017

And the manual verification issue may be fixed with jenkinsci/ssh-agents-plugin#54 ?

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Jun 9, 2017

I don't see the null pointer with 2.60.1-rc1, only with 2.58. I believe the null pointer fix is valid and necessary. It isn't the fix for the problem I'm seeing on 2.60-1-rc1.

I've submitted JENKINS-44803 with the steps I took to show the problem in a freshly installed environment.

I'll do some further experimenting with that environment to see if I can find alternate work arounds. Not verifying the host key of the server is a valid technique (since it worked for me for a very long time), but it feels like a step away from the greater security that is sought with larger and larger Jenkins installations.

If the credentials are needed for that environment, I'm willing to share them with anyone who needs to perform diagnosis on the machine(s).

@mc1arke
Copy link
Member Author

mc1arke commented Jun 10, 2017

As @paladox has mentioned, I believe JENKINS-44803 is caused by a bug in ssh-slaves (rather than Trilead) and should be fixed by the outstanding PR. @MarkEWaite are you able to try your tests again using the LTS RC and a snapshot of ssh-slaves from the referenced PR?

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Jun 10, 2017

Testing completed with Jenkins 2.60.1-rc1 and 2.64 and was successful.

This seems like a plugin release that needs to precede the release of Jenkins 2.60.1 LTS.

Thanks very much @paladox , @mc1arke , @oleg-nenashev , and @jglick !

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
Development

Successfully merging this pull request may close these issues.

6 participants