Community
Participate
Working Groups
Build Identifier: 0 See summary. Reproducible: Always
Created attachment 191274 [details] patch
I, Teerawat Chaiyakijpichet, confirm that: 1) I am the only author of the code contribution. 2) I apply the EPL to the code contribution.
Created attachment 191275 [details] Test Keys
Created attachment 191276 [details] Test Trust
Note to Eike: After applying the patch, you'll have to drop the attached 'testKeys' and 'testTrust' files in the following 3 places to be able to run the SSL test suites (AllTestsMEMBranchesSSL and AllSSLTests) and examples (EchoSSLClient/Server): plugins/org.eclipse.net4j.tests/sslKey/testKeys plugins/org.eclipse.net4j.tests/sslKey/testTrust plugins/org.eclipse.emf.cdo.tests/sslKey/testKeys plugins/org.eclipse.emf.cdo.tests/sslKey/testTrust plugins/org.eclipse.net4j.examples/sslKey/testKeys plugins/org.eclipse.net4j.examples/sslKey/testTrust
Guys, this works like a charm and the code looks quite good!!! I've made some changes though, added some TODO markers. Details follow: New packages: I've moved the two ssl packages down one level below tcp and adjusted the manifest.mf so that the restricted access warnings disappear. SSLUtil.createSSLEngine(boolean, String, int): I've moved the final beginHandshake() call to the ctor of SSLEngineManager. SSLEngineManager.read(SocketChannel): I've removed the unused readSize variable. Buffer.readChannel(SocketChannel, ByteBuffer), SSLEngineManager.handleWrite(SocketChannel), SSLEngineManager.handleRead(SocketChannel): I've added catch clauses to prevent redundant exception wrapping: catch (ClosedChannelException ex) { throw ex; } SSLEngineManager.performHandshake(SocketChannel, boolean): This may be okay but to be sure, what do you want to break with the inner break statement? switch (engineResult.getStatus()) { case OK: while (packetSendBuf.position() > 0) { nBytes = handleWrite(socketChannel); if (nBytes == 0) { // Prevent spinning if the channel refused the write break; } } break; SSLEngineManager.executeTasks(), SSLConnector.handleConnect(ITCPSelector, SocketChannel), SSLConnector.handleRegistration(ITCPSelector, SocketChannel): The creation of multiple, unpooled threads should be optimized. I propose to use an injectable ExecutorService. Note that ITransportConfig already carries an executor service instance, but I suggest that we add a separate getter/setter pair to SSLEngineManager. I've added TODO markers. SSLEngineManager.checkRehandShake(SocketChannel): I've removed this superfluous try/catch: try { checkInitialHandshake(socket); } catch (Exception ex) { throw ex; } Exception handling: I removed the try/catch in SSLProperties.load(String). I rethrow the InterruptedException in SSLConnector.waitForHandShakeFinish(). I call OM.LOG.warn(ex) instead of ex.printStackTrace(). SSLEngineManager.ReadLock, SSLEngineManager.WriteLock: Introduced for better monitor debugging. SSLSelector et al: I've removed these guys entirely because they add no functionality but rather prevent sharing of the same selector by SSL connectors and normal TCP connectors. I've adjusted TCPConnectorTest.provideTransport() and Bugzilla_241463_Test.createContainer() accordingly. Further I've added a TCPUtil.prepareContainer() call to SSLUtil.prepareContainer(). plugin.xml: I've added markup for SSLAcceptorFactory and SSLConnectorFactory. SSL keys and build.properties: I've added the sslKey folders where necessary. Are the keys also needed in StandaloneContainerExampleSSL? SSLBuffer.startGetting(SocketChannel), SSLBuffer.write(SocketChannel): There's a total of 17 calls to the inherited, not-inlineable getByteBuffer() method. Assuming that none of the other calls actually changes the result of that method I'm remembering it in local variables now. Please review carefully if my assumption is valid!!! SSLProperties.SSLProperties(): The System.getProperties() call is problematic in OSGi. Can you please rewrite your code so that it rather makes use of OMPlatform.INSTANCE.getProperty(String) and/or OMPlatform.INSTANCE.getProperty(String, String)? I've added a TODO marker. New APIs and configuration: I wonder we should make things like SSLEngineManager.HANDSHAKE_TIMEOUT or SSLConnector.WAIT_TIME configurable. One way could be system properties. Another way could be to provide public interfaces for the main implementation classes so that a user could create/configure a custom SSLEngineManager and pass it into SSLConnectors before their doActivate() method is called. Just an idea... Loosely related is the idea to use URLs rather than file names to load the keys. Then OSGi can load them even from inside of jarred and signed bundles. I've added TODO markers.
Created attachment 191284 [details] Patch v2 I've also removed the extra configuration-ssl folder and added an XML comment about SSL to the original cdo-server.xml
Created attachment 191285 [details] Patch v3 v2 was incomplete!
Created attachment 191286 [details] Patch v4 Once more ;-(
Internal note: I've submitted https://dev.eclipse.org/ipzilla/show_bug.cgi?id=5020 to Eclipse legal.
Created attachment 191498 [details] Patch_V5 I am not sure. I create the patch correctly. Please verify.
Comment on attachment 191498 [details] Patch_V5 change the description and reviewing status.
SSLEngineManager.performHandshake(SocketChannel, boolean): This may be okay but to be sure, what do you want to break with the inner break statement? switch (engineResult.getStatus()) { case OK: while (packetSendBuf.position() > 0) { nBytes = handleWrite(socketChannel); if (nBytes == 0) { // Prevent spinning if the channel refused the write break; } } break; <!--- I need to break in order to prevent the infinite loop, if handleWrite still always return 0 ---> SSLEngineManager.executeTasks(), SSLConnector.handleConnect(ITCPSelector, SocketChannel), SSLConnector.handleRegistration(ITCPSelector, SocketChannel): The creation of multiple, unpooled threads should be optimized. I propose to use an injectable ExecutorService. Note that ITransportConfig already carries an executor service instance, but I suggest that we add a separate getter/setter pair to SSLEngineManager. I've added TODO markers. <!--- Done. actually, I ever try to use ExecuterService before and I found it makes some unit test about thread not stable sometime. ---> SSL keys and build.properties: I've added the sslKey folders where necessary. Are the keys also needed in StandaloneContainerExampleSSL? <--- I think we need SSL keys in order to run this class. so I created StandaloneContainerExampleSSL.launch for helping to launch ---> SSLBuffer.startGetting(SocketChannel), SSLBuffer.write(SocketChannel): There's a total of 17 calls to the inherited, not-inlineable getByteBuffer() method. Assuming that none of the other calls actually changes the result of that method I'm remembering it in local variables now. Please review carefully if my assumption is valid!!! <!--- I do not understand what you mean, Could you provide more example? ---> SSLProperties.SSLProperties(): The System.getProperties() call is problematic in OSGi. Can you please rewrite your code so that it rather makes use of OMPlatform.INSTANCE.getProperty(String) and/or OMPlatform.INSTANCE.getProperty(String, String)? I've added a TODO marker. <!--- Done ---> New APIs and configuration: I wonder we should make things like SSLEngineManager.HANDSHAKE_TIMEOUT or SSLConnector.WAIT_TIME configurable. One way could be system properties. Another way could be to provide public interfaces for the main implementation classes so that a user could create/configure a custom SSLEngineManager and pass it into SSLConnectors before their doActivate() method is called. Just an idea... <!--- Done ---, I add "net4j.ssl.hs_timeout" and "net4j.ssl.hs_waittime" properties> Loosely related is the idea to use URLs rather than file names to load the keys. Then OSGi can load them even from inside of jarred and signed bundles. I've added TODO markers. <!--- Done, So I make try to load by URI first, if there is any exception, which make loading by URI fail. It will try to load with normal path, if fail again, SSL Engine cannot initailize. ---> The other issues, ok for me.
Created attachment 191569 [details] Patch v6 Instead of creating a new thread pool in SSLEngineManager, I inject the one of the SSLConnector into the SSLEngineManager. I've renamed the property keys in SSLProperties. SSLProperties.load() still has two flaws: 1. It does still use a FileInputStream and not a URL connection. 2. In case of IO problems the stream is not closed in a finally block. These issues are still to be addressed...
Created attachment 191597 [details] Patch V7 Thanks, for reviewing. Patch V7, I fixed about loading file issues both org.eclipse.net4j.tcp.ssl.SSLUtil and org.eclipse.net4j.internal.tcp.ssl.SSLProperties.
I, Teerawat Chaiyakijpichet, confirm that: 1) I am the only author of the code contribution. 2) I apply the EPL to the code contribution. 3) I have the rights to distribute this code contribution, based on being authorized by my employer to do so.
*** Bug 333947 has been marked as a duplicate of this bug. ***
Patch amended slightly in conformance with comments from IP review team. Committed revision 7598, 7599.
Correction: removing stray files. Don't know why they were in the patch. Committed revision 7602.
Committed revision 7604: - trunk/plugins/org.eclipse.net4j.tcp
(In reply to comment #14) > 1. It does still use a FileInputStream and not a URL connection. I must have overlooked that this flaw is still present in the latest committed code base. Maybe I did not exactly describe my concern. In OSGi we need to be able to load "document contents" from anywhere, in particular fro inside of JAR files. For example in our automated Hudson/Buckminster tests the test project that contains the key files is a jarred bundle. We can not load contents from there via java.io.File and the following code (taken from SSLUtil.java) is nothing else: new File(new URL(trustPath).toURI()) What I tried to ask for is that we do not imply a file system in any way but use arbitrary URLs as config parameters and use them with something like: new URL(trustPath).openStream() Of course that would include the ability to configure file: URLs, as well. Can you please see if you can make it work like that?
I see. Yes, I will fix it.
Thank you, Teerawat! And Happy Easter ;-)
You're welcome and Happy Easter too :)
Created attachment 194040 [details] Patch V8 Hello Eike, I have fixed about "loading key file by URL connection" and modify the launcher file to access key files via URL. Could you create sskKey folder in org.eclipse.emf.cdo.server plugin and put testKeys and testTrust files into it? I modify CDOServer_SSL.launch to access those files. Thanks. I have changed DEFAULT_TIMEOUT_EXPECTED in AbstractOMTest from 2 seconds to 3 seconds in order to avoid timeout in some case. I have fixed about Negotiator in SSL Connector. Since if there is some delay, isActive of SSL Connector return false then waitForHandShakeFinish method will nt wait until handshake finish.
Created attachment 194334 [details] Patch v9 Just to record my changes to your patch, most of which are related to try/finally blocks. If you open a stream in a try block you certainly want to use it there and not in the finally block. The finally block should only be used to dispose of resources. The tests still pass, so I'm going to commit it in a second. Thank you!
Committed revision 7645: - trunk/plugins/org.eclipse.emf.cdo.examples - trunk/plugins/org.eclipse.emf.cdo.server - trunk/plugins/org.eclipse.emf.cdo.tests - trunk/plugins/org.eclipse.net4j.examples - trunk/plugins/org.eclipse.net4j.tcp - trunk/plugins/org.eclipse.net4j.tests
Issues addressed. Resolving...
Thank you :)
Available in R20110608-1407
