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-36871] JNLPProtocol4 #92

Merged
merged 77 commits into from
Aug 11, 2016
Merged

[JENKINS-36871] JNLPProtocol4 #92

merged 77 commits into from
Aug 11, 2016

Conversation

stephenc
Copy link
Member

@stephenc stephenc commented Jul 27, 2016

This is a JNLP protocol that uses TLS upgrade over the plaintext socket before exchanging any connection secrets and allows for end-to-end security leveraging the TLS certificates on the Jenkins master HTTPS end-point where the master is using TLS on the web UI.

Initial measurements show that the TLS encryption using SSLEngine coupled with the ByteBuffer backed NIO implementation does not seem to have any negative performance impact. Additionally the new NIO implementation shows significantly better scalability and performance due to the offloading of I/O processing to the worker threads (this was necessary for to use SSLEngine, but a nice side-effect when compared with the previous NIO implementation (i.e. JNLPProtocol2) which does the actual I/O processing on the selector thread and thus can cause excessive transport delays when the selector thread becomes overloaded)

See JENKINS-36871 for the tracking issue.

This code is being contributed to the Jenkins Community by CloudBees, Inc. (though some of this code was work developed by me in personal time, but I am contributing that to the Jenkins Community anyway)

Work in progress while I complete our internal process for all the files required to be released.


This change is Reviewable

- Some of the tests require JUnit 4.12
- Resurected from history as we will be forced to diverge to map against the JNLPProtocol APIs rather than follow the nicer implementation path from CJOC
…c within remoting

- rather than having one half of the protocol in jenkins and the other half in remoting.
- I need to write tests for this and then remote the old classes
@stephenc stephenc closed this Jul 29, 2016
@stephenc stephenc reopened this Jul 29, 2016
@stephenc
Copy link
Member Author

stephenc commented Jul 29, 2016

TODO

  • Remove the JnlpProtocol / JnlpProtocol1 / JnlpProtocol2 / JnlpProtocol3 classes
  • Remove dependencies of the JnlpProtocol classes
  • Remove the JnlpServerHandshake and related classes
  • Refactor tests of the JnlpProtocol classes to be tests of JnlpProtocolHandler classes
  • Add tests of JnlpProtocol4Handler
  • Refactor Jenkins core to use the JnlpProtocol_Handler classes
  • Some magic in Jenkins core to make JnlpAgentReceiver work / replace with JnlpConnectionStateListener

- Also causes commands that span multiple frames to be corrupted
…onnects

- Don't think this affected any real protocols, but better to fix after finding
- Avoids intermediate String representation. Could probably get faster still with BigInteger and zero-left-pad but it becomes harder to understand
…et concurrent modification exceptions

- They'll recover, but better is not to have to worry about it at all
@stephenc
Copy link
Member Author

Let's see if we can get lucky

@stephenc stephenc closed this Aug 10, 2016
@stephenc stephenc reopened this Aug 10, 2016
@stephenc
Copy link
Member Author

- Of course we are now weakening the equality test, but since this has come through the cipher
  this shouldn't be too bad.
- Found another set of cases where the cookie gets mangled, as the EngineUtil.readLine does a `.trim()`
  If the encrypted cookie happens to being or end with a whitespace character... boom
  A first order analysis looks only at the single bytes that could be whitespace.
  There are 9 such bytecodes, but that includes `\n` which we have already tested for
  So there is an 8 in 256 or 1 in 32 chance that the first character is whitespace
  There is a 31 in 32 chance that it is not and a 1 in 32 chance that the last is whitespace
  Thus (1/32 + 32/32*1/32) = 6%... or in total 28% of the time the generated cookies will
  be unusable with the current handshaking ciphers

- Also stop creating throw-away instances of `SecureRandom` because that is a waste
…s introduced bug

- We don't want to synchronize as that will cause issues during the close. We can rely on the stack for ensuring that reads are serialized and writes are serialized and the `channel` field is write once with the write guarded by the synchronized setup method
 - On machines with fewer cores than my machine, the selector thread may not have processed the interest removal and hence the test can fail randomly for lower spec machines
@stephenc
Copy link
Member Author

Retriggering to confirm that flaky tests have been fixed

@stephenc stephenc closed this Aug 11, 2016
@stephenc
Copy link
Member Author

And let's see

@stephenc stephenc reopened this Aug 11, 2016
@stephenc
Copy link
Member Author

Timeout... Ffs

@stephenc stephenc closed this Aug 11, 2016
@stephenc stephenc reopened this Aug 11, 2016
@oleg-nenashev
Copy link
Member

The last changes look good.
So 🐝 from me for merging it to a remoting-3.0 branch and then perform some soak testing.

@stephenc
Copy link
Member Author

@oleg-nenashev I'd rather cut a 2.0 branch pre-merge and merge this on master

@oleg-nenashev
Copy link
Member

We agreed that we will follow master and stable branch approach

@stephenc stephenc merged commit a4f5af3 into master Aug 11, 2016
@stephenc stephenc deleted the jenkins-36871 branch August 11, 2016 14:44
oleg-nenashev added a commit to oleg-nenashev/jenkins that referenced this pull request Aug 14, 2016
Changes are listed below:

Fixed issues:
* [JENKINS-22853](https://issues.jenkins-ci.org/browse/JENKINS-22853) -
Be robust against the delayed EOF command when unexporting input and output streams.
(jenkinsci/remoting#97)
* Fixed ~20 minor issues reported by FindBugs.
More fixes to be delivered in future versions.
(jenkinsci/remoting#96)

Enhancements:
* [JENKINS-37218](https://issues.jenkins-ci.org/browse/JENKINS-37218) -
Performance: <code>ClassFilter</code> does not use Regular Expressions anymore to match <code>String.startsWith</code> patterns.
(jenkinsci/remoting#92)
* [JENKINS-37031](https://issues.jenkins-ci.org/browse/JENKINS-37031)
<code>TcpSlaveAgentListener</code> now publishes a list of supported agent protocols to speed up connection setup.
(jenkinsci/remoting#93)
DanielWeber added a commit to DanielWeber/jenkins that referenced this pull request Aug 26, 2016
(Cherry Pick of 06be933)

Changes are listed below:

Fixed issues:
* [JENKINS-22853](https://issues.jenkins-ci.org/browse/JENKINS-22853) -
Be robust against the delayed EOF command when unexporting input and output streams.
(jenkinsci/remoting#97)
* Fixed ~20 minor issues reported by FindBugs.
More fixes to be delivered in future versions.
(jenkinsci/remoting#96)

Enhancements:
* [JENKINS-37218](https://issues.jenkins-ci.org/browse/JENKINS-37218) -
Performance: <code>ClassFilter</code> does not use Regular Expressions anymore to match <code>String.startsWith</code> patterns.
(jenkinsci/remoting#92)
* [JENKINS-37031](https://issues.jenkins-ci.org/browse/JENKINS-37031)
<code>TcpSlaveAgentListener</code> now publishes a list of supported agent protocols to speed up connection setup.
(jenkinsci/remoting#93)
oleg-nenashev added a commit to oleg-nenashev/remoting that referenced this pull request Oct 22, 2016
oleg-nenashev added a commit that referenced this pull request Oct 23, 2016
…121)

* [JENKINS-39161] - Add brief description of remoting versions, provide subpage link stubs

* [JENKINS-39161] - Add Changelog for the 3.0 release

* [JENKINS-39161] - Add the Contributiing page with PR creation guidelines

* [JENKINS-39161] - Add protocol descriptions page

* [JENKINS-39161] - Add placeholders for Jenkins specifics and Troubleshooting

* [JENKINS-39161] - Update JNLP4-plaintext documentation according to comments from @stephenc

* [JENKINS-39161] - Add compatibility notes as a follow-up to #92

* [JENKINS-39161] - Update Readme

* [JENKINS-39161] - Modify the System Property documentation

* [JENKINS-39161] - Fix relative links in the compatibility page

* [JENKINS-39161] - Fix the spelling mistake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants