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

Upgrading jetty from 9.2.18 to 9.4.9 #4630

Merged
merged 2 commits into from Apr 26, 2018

Conversation

Projects
None yet
2 participants
@jyotisingh
Copy link
Contributor

commented Apr 9, 2018

One potential breaking change - it has support for TLS1.2 alone. Meaning it disables (by default) all cipher suites that matches ^.*_(MD5|SHA|SHA1)$ which are the only ones which work with TLS 1.0 and TLS1.1. So if any of the client uses TLS1.1 or TLS1.0 then the communication will fail. Need to test if this is an issue for us or not.
This version fixes some known issues with JDK9
fixes a class loader leak issue on the version we were on
bunch of bug fixes and a few feature enhancements

@jyotisingh jyotisingh force-pushed the jyotisingh:jetty_upgrade_9.4.8 branch from 358b2d0 to 4fb60f9 Apr 9, 2018

@ketan

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

If we merge this — we may have to enable the weak ciphers and give a timeline for deprecation. https://githubengineering.com/crypto-deprecation-notice/ is a good example.

@jyotisingh jyotisingh force-pushed the jyotisingh:jetty_upgrade_9.4.8 branch 2 times, most recently from 685fce1 to c94853c Apr 10, 2018

@@ -128,6 +128,9 @@ task verifyJar(type: VerifyJarTask) {
"jdom-2.0.2.jar",
"jetty-io-${versions.jetty}.jar",
"jetty-util-${versions.jetty}.jar",
"jetty-client-${versions.jetty}.jar",
"jetty-http-${versions.jetty}.jar",
"jetty-xml-${versions.jetty}.jar",

This comment has been minimized.

Copy link
@ketan

ketan Apr 11, 2018

Member

Can these be ignored, or do these have to be pulled in?

@jyotisingh jyotisingh force-pushed the jyotisingh:jetty_upgrade_9.4.8 branch 3 times, most recently from 606de23 to 560385b Apr 18, 2018

@jyotisingh jyotisingh changed the title [WIP] Upgrading jetty from 9.2.18 to 9.4.8 Upgrading jetty from 9.2.18 to 9.4.8 Apr 19, 2018

@@ -32,7 +32,11 @@ dependencies {
compile group: 'org.objenesis', name: 'objenesis', version: project.versions.objenesis
compile group: 'org.bouncycastle', name: 'bcprov-jdk15on', version: '1.47'
compile group: 'commons-configuration', name: 'commons-configuration', version: '1.10'
compile group: 'org.eclipse.jetty.websocket', name: 'websocket-client', version: versions.jetty
compile(group: 'org.eclipse.jetty.websocket', name: 'websocket-client', version: versions.jetty){
exclude module: 'jetty-http'

This comment has been minimized.

Copy link
@ketan

ketan Apr 23, 2018

Member

I have a suspicion that jetty-http may be needed as a dependency. Websockets always start with an http connnection and "upgrade" to a websocket connection. Did you test websocket comm between agent/server?

This comment has been minimized.

Copy link
@jyotisingh

jyotisingh Apr 23, 2018

Author Contributor

No, i haven't. I was hoping one of the functional tests would catch that. Theh are running at the moment. Don't merge this yet.

@ketan

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

FYI: I don't think I have anything else more to review. As long as the tests go green, this is good to merge. I don't know if we we have functional tests for agent/server comm over websockets. Can @rajiesh confirm?

@jyotisingh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2018

Well, I didn't look for it. I assumed there was one based on a conversation in past, may be I am mistaken. Will let @rajiesh confirm about the automated test. I will quickly test it out on local box.

@jyotisingh

This comment has been minimized.

@jyotisingh jyotisingh force-pushed the jyotisingh:jetty_upgrade_9.4.8 branch 3 times, most recently from 209c079 to bfb827d Apr 23, 2018

@jyotisingh jyotisingh added this to the Release 18.4 milestone Apr 24, 2018

@ketan ketan force-pushed the jyotisingh:jetty_upgrade_9.4.8 branch 2 times, most recently from 500a9eb to be124a7 Apr 26, 2018

@ketan ketan changed the title Upgrading jetty from 9.2.18 to 9.4.8 Upgrading jetty from 9.2.18 to 9.4.9 Apr 26, 2018

jyotisingh added some commits Apr 6, 2018

Upgrading jetty from 9.2.18 to 9.4.9
Points of highlight with newer jetty:

* One potential breaking change - it has support for TLS1.2 alone.
  Meaning it disables (by default) all cipher suites that matches
  `^.*_(MD5|SHA|SHA1)$` which are the only ones which work with TLS 1.0
  and TLS1.1. So if any of the client uses TLS1.1 or TLS1.0 then the
  communication will fail. Need to test if this is an issue for us or not.
* This version fixes some known issues with JDK9
* Fixes a class loader leak issue on the version we were on
* Funch of bug fixes and a few feature enhancements
Clearing up default exclusion protocols and cipher-suites setup by 9.…
…4.8 version of jetty as they could potentially break some setups

Wrapped this around a system environment flag to switch the behavior to defaults set by jetty in future
Also removed the code to fall back to weak ciphers since no one has ever seen an issue with the default configurable list

@ketan ketan force-pushed the jyotisingh:jetty_upgrade_9.4.8 branch from be124a7 to 1daf12d Apr 26, 2018

@ketan ketan merged commit 1daf12d into gocd:master Apr 26, 2018

1 check passed

license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.