Community
Participate
Working Groups
Implementation of pluggable module for JSSE ssl provider. Pluggable layer should be supported by the bug #195644.
Samson, please add FDD
Approved by the AG for TPTP 4.4.1 with the following comments: -See meetings from review call (http://dev.eclipse.org/mhonarc/lists/tptp-platform-dev/msg02106.html).
Igor. How can the location of the JVM library be passed to the pluggable provider? Can we add a "setNVPair(char* name, char* value)" function to the interface? This way we can pass anything before "init()" is called.
Samson, I thought all AC modules should use single embedded JVM. A mechanism to submit an java job should be provided by AC.
Igor, I don't think we can do that if we want to use a common interface for both JSSE and OpenSSL provider. Furthermore the JVM is currently created in the CCTL. By providing the "setNVPair()" option we can isolate this provider with the CCTL. If later the CCTL/ACTL is being removed, the JSSE provider will still be functional. Is it true that you are concerning about memory usage? If that's true, I would rather trade off memory for ease of maintainance.
Samson, sure we can add any functions you need to the interface. You solution is reasonable. But this way implies that second embedded jvm will be launched by AC which will rise up total memory consumption. Another option could be extracting jvm from the CCTL in a separate module. And provide all other modules with ability to submit an java job.
Created attachment 87588 [details] SetConfig JSSE Changes
Created attachment 87589 [details] Java Side JSSE Patch
Created attachment 87594 [details] Native Side AC Patch
Created attachment 87595 [details] Native AC JSSE Patch Bugzilla patch flag applied.
Created attachment 87692 [details] Patch - Race condition fix to execution framework code Patch for a race condition in the execution framework's ConnectionImpl class in authenticateUser(...) and processControlMessage(...) methods. The problem is that the AuthenticateCommand is sent to the agent controller and the response is received and processed by processControlMessage(..) BEFORE authenticateUser calls wait on the authLock. This means that the wait(...) call in authenticateUser is waiting for a notification which has already occurred. This causes a ten second delay, which causes the createConnection() method of AgentControllerFactory to time out and throw the connection exception to the user. Fix is just a patch to properly synchronize the wait and notify statements. I discovered this bug during testing of my JSSE pluggable layer, so I've posted the patch here rather than opening a new bug against the execution framework code.
Created attachment 87695 [details] Java Side JSSE Patch - 2 Updated Java Side JSSE Patch: - Minor tweaks to thread synchronization and SSL context initialization.
Created attachment 87718 [details] Native AC JSSE Patch - 2 Minor Updates: - Updated debug statements. - Updated classpath calls to Java provider
Igor, can you review the patches?
(In reply to comment #11) > Created an attachment (id=87692) [details] > Patch - Race condition fix to execution framework code > Patch for a race condition in the execution framework's ConnectionImpl class in > authenticateUser(...) and processControlMessage(...) methods. > The problem is that the AuthenticateCommand is sent to the agent controller and > the response is received and processed by processControlMessage(..) BEFORE > authenticateUser calls wait on the authLock. This means that the wait(...) call > in authenticateUser is waiting for a notification which has already occurred. > This causes a ten second delay, which causes the createConnection() method of > AgentControllerFactory to time out and throw the connection exception to the > user. > Fix is just a patch to properly synchronize the wait and notify statements. > I discovered this bug during testing of my JSSE pluggable layer, so I've posted > the patch here rather than opening a new bug against the execution framework > code. Hi Jonathan, the patch is good. Please just make the same changes in synchronization for authentication failure.
(In reply to comment #13) > Native AC JSSE Patch - 2 Hi Jonathan, the patch is good, please see my comments below. - You are resolving java methods each time to call. This is especially expensive in the sslRead() and sslWrite(). Please consider to use global references and its single resolving during module initialization. - I'd suggest to free arrays, allocated with NewByteArray(), using DeleteLocalRef() call. According to JNI spec., these references "will be freed once the native method returns". You are using Invocation API and your methods never "return" control to the JVM and it seems you are responsible to free the memory. - I'd suggest to use one call AttachCurrentThread() instead of pair tptpCreateJVM(NULL), tptpGetJNIEnv() - jsse/tptpCommon.h - I'd suggest to remove this file and use definitions for LOAD_LIBRARY from tptp/tptpCommon.h instead of. - The file Resource.h seems to be unused - remove/hide <windows.h> to prevent build problems on Linux
(In reply to comment #12) > Java Side JSSE Patch - 2 Hi Jonathan, the patch is good. I'll send you my minor comments by email.
(In reply to comment #17) > (In reply to comment #12) > > Java Side JSSE Patch - 2 > Hi Jonathan, the patch is good. > I'll send you my minor comments by email. Sorry, I have one note: You are opening a server socket to listen. IMHO user should be informed about failures: port might be busy...
Created attachment 87791 [details] Java Side JSSE Patch - 3 - Post Review Java side JSSE updated based on review comments: - Joel and I updated build settings for this plugin. - Java code now prints an error to the user when ServerSocket cannot listen on the port.
Created attachment 87793 [details] Race condition fix to exec framework code - Patch 2 Minor update based on review comments.
Created attachment 87796 [details] Native AC JSSE Patch - 3 - Post Review Changes based on Igor's review: - Cached methodIDs for all native methods - DeleteLocalRef is called in nativeRead(..) and nativeWrite(..) (I made the removal of the local reference explicit, even though detaching from the JVM using the detach JNI call has the same effect) - Call AttachCurrentThread() instead of pair tptpCreateJVM(NULL), tptpGetJNIEnv() - Removed windows.h reference, and extraneous VS source files.
Created attachment 87797 [details] Native AC JSSE Patch 3 - Post Review
My patches are good to go as soon as the JVM provider (bug 215346) is approved by PMC and checked in.
(In reply to comment #21) > Created an attachment (id=87796) [details] > Native AC JSSE Patch - 3 - Post Review > Changes based on Igor's review: > - Cached methodIDs for all native methods > - DeleteLocalRef is called in nativeRead(..) and nativeWrite(..) (I made the > removal of the local reference explicit, even though detaching from the JVM > using the detach JNI call has the same effect) > - Call AttachCurrentThread() instead of pair tptpCreateJVM(NULL), > tptpGetJNIEnv() > - Removed windows.h reference, and extraneous VS source files. OK, you are caching methodIDs. Let's go ahead and don't call FindClass() each time sslRead() and sslWrite() used, since this is unnecessary :)
(In reply to comment #19) > Created an attachment (id=87791) [details] > Jave Side JSSE Patch - 3 - Post Review > Java side JSSE updated based on review comments: > - Joel and I updated build settings for this plugin. > - Java code now prints an error to the user when ServerSocket cannot listen on > the port. I agree to check the patch in, since it is working. But users (and we) can have problems to support it. AC might be launched as daemon (or Windows service) and error messages to console might be invisible. Problems with keystore file and password are not reported at all. From other hand, AC implies stopping itself if any Transport Layer has initialization problem. To follow this it is enough to return error code (-1 e.g.). void nativeInit() Java method could return error code to block AC launching in the case of busy port or keys problems. Later, logging to AC log file could be added using SSL layer built-in log service.
> OK, you are caching methodIDs. Let's go ahead and don't call FindClass() each > time sslRead() and sslWrite() used, since this is unnecessary :) > AFAIK you can't directly cache class IDs, as they're local references which disappear when detachThread is called. This was the way Samson had originally written the code, and that code was unstable. I'll see if I can find a workaround ASAP. > But users (and we) can have problems to support it. > AC might be launched as daemon (or Windows service) and error messages to > console might be invisible. Problems with keystore file and password are not > reported at all. Although keystore in particular is auto-generated with a password that doesn't change. Can't imagine we'd have a problem with this unless the user directly tampers with the keystore. > From other hand, AC implies stopping itself if any Transport Layer has > initialization problem. To follow this it is enough to return error code (-1 > e.g.). void nativeInit() Java method could return error code to block AC > launching in the case of busy port or keys problems. > Later, logging to AC log file could be added using SSL layer built-in log > service. > Agreed.... with more time there is always more we can do ;). And as you note, the error codes are a reasonable indicator. We'll open a new bug to add better logging to JSSE if support for it will extend into and past 4.5.
Created attachment 87877 [details] Native AC JSSE Patch - 4 - Post Review Minor patch update based on comment #24 and #26: - Fortunately there is no need to cache class IDs, as we only need them when we grab the method ID, which IS cached. Code updated to reflect this.
Hi Kiryl/Igor, in order to support JSSE in the agent controller we will need to add the jsse.jar and it's pluginconfig.xml file, into the appropriate folders of the agent controller build. New files/folders are: /(AC_DIR)/plugins/org.eclipse.tptp.jsse /(AC_DIR)/plugins/org.eclipse.tptp.jsse/jsse.jar /(AC_DIR)/plugins/org.eclipse.tptp.jsse/config/ /(AC_DIR)/plugins/org.eclipse.tptp.jsse/config/pluginconfig.xml Jsse.jar is produced by the build changes in Java Side JSSE patch, and the JAR can be found under \org.eclipse.tptp.platform.agentcontroller(*)\jsse.jar when compiled. pluginconfig.xml can be found under \org.eclipse.tptp.platform.agentcontroller(*)\jsse-config\pluginconfig.xml. Thanks!
Added text from comment #28 into a separate bug w/ dependency on that bug.
Patches SetConfig JSSE Changes, Java Side JSSE Patch - 3 - Post Review, Race condition fix to exec framework code - Patch 2, and Native AC JSSE Patch - 4 - Post Review checked into HEAD and 4.4.1 branch.
Resolving as fixed. Test pass will be used to verify.
(In reply to comment #26) > AFAIK you can't directly cache class IDs, as they're local references which > disappear when detachThread is called. This was the way Samson had originally > written the code, and that code was unstable. JNI provides "Global References" to resove this. You can get them with NewGlobalRef() call. Please see JNI specification for details. Actually, it seems you don't need it here;
(In reply to comment #28) > Hi Kiryl/Igor, in order to support JSSE in the agent controller we will need to > add the jsse.jar and it's pluginconfig.xml file, into the appropriate folders > of the agent controller build. Hi Jonathan, Kiryl is resolving this, but it seems Win-64 make file should provided as well.
As of TPTP 4.6.0, TPTP is in maintenance mode and focusing on improving quality by resolving relevant enhancements/defects and increasing test coverage through test creation, automation, Build Verification Tests (BVTs), and expanded run-time execution. As part of the TPTP Bugzilla housecleaning process (see http://wiki.eclipse.org/Bugzilla_Housecleaning_Processes), this enhancement/defect is verified/closed by the Project Lead since the originator of this enhancement/defect has an inactive Bugzilla account and considered to be fixed. If this enhancement/defect is still unresolved and reproducible in the latest TPTP release (http://www.eclipse.org/tptp/home/downloads/), please re-open.
