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

Reuse SSLSocketFactory #38

Closed
michaelrice opened this Issue Mar 13, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@michaelrice
Member

michaelrice commented Mar 13, 2015

http://sourceforge.net/p/vijava/feature-requests/24/

This part of the code has been refactored since this was reported but I dont think its really been fixed. I think its just called another way now.

WCClient uses a new instance of SSLSocketFactory every time it connects to VC. As a result, sockets cannot be reused. There is also a small performance issue that trustAllHttpsCertificates() is executed upon each connection when it actually needs to be executed only once per the lifetime of the application.
Suggested code changes:

private static volatile boolean initialized = false; // new code
private static void trustAllHttpsCertificates() throws NoSuchAlgorithmException, KeyManagementException {
    if (initialized) {return; }// new code
    initialized = true; // new code
    TrustManager[] trustAllCerts = new TrustManager[1]; 
    trustAllCerts[0] = new TrustAllManager(); 
    SSLContext sc = SSLContext.getInstance("SSL"); 
    sc.init(null, trustAllCerts, null); 
    HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
}
@michaelrice

This comment has been minimized.

Member

michaelrice commented Jun 18, 2015

An issue was raised on the vijava forums about the SSL being a global disable. This fix may as well go along with this change. Post for reference: http://sourceforge.net/p/vijava/discussion/826592/thread/33f68590/

michaelrice added a commit to michaelrice/yavijava that referenced this issue Jun 18, 2015

michaelrice added a commit to michaelrice/yavijava that referenced this issue Jun 20, 2015

Refactor TrustAllSSL and WSClient to fix security
The way the library has always worked the default ssl
context (when using ignore certs) would change the global
context for anything in the jvm. This means ALL certs were
trusted when using ignoreCert in YAVIJAVA/VIJAVA. This is
a big security issue. A user on the vijava forums pointed
the issue out: http://sourceforge.net/p/vijava/discussion/826592/thread/33f68590/
This fixes that issue and includes tests to verify fix.

Fixes yavijava#38

michaelrice added a commit that referenced this issue Jun 20, 2015

Merge pull request #100 from michaelrice/issue-38
starting to address issue raised in #38
@BigBeaule

This comment has been minimized.

Contributor

BigBeaule commented Jul 8, 2015

Hello,

The current fix is correcting the security issue, but not the reuse issue. Each time a request is sent, a new handshaking occurs which really impact the performance if you do a lot of them (around 4 time slower).

You are reusing the SSLContext but not the SSLSocketFactory which is the important part to use the fast handshaking. Saving the SSLSocketFactory in the constructor and reusing it for each connection seem to solve the issue.

Regards.

@michaelrice michaelrice reopened this Jul 8, 2015

@michaelrice

This comment has been minimized.

Member

michaelrice commented Jul 8, 2015

Cool thanks for pointing this out. I'll take another stab at it unless you would like to send a pull request.

BigBeaule added a commit to BigBeaule/yavijava that referenced this issue Jul 8, 2015

Update WSClient.java
Attempt to fix issue yavijava#38 in order to reuse the SSLSocketFactory

BigBeaule added a commit to BigBeaule/yavijava that referenced this issue Jul 9, 2015

Update WSClient.java
Forgot the import to fix issu yavijava#38. Sorry for that, first time helping on a open source project!

michaelrice added a commit to michaelrice/yavijava that referenced this issue Jul 15, 2015

Update WSClient.java
Attempt to fix issue yavijava#38 in order to reuse the SSLSocketFactory

michaelrice added a commit to michaelrice/yavijava that referenced this issue Jul 15, 2015

Update WSClient.java
Attempt to fix issue yavijava#38 in order to reuse the SSLSocketFactory

Fixes yavijava#38

michaelrice added a commit to michaelrice/yavijava that referenced this issue Aug 19, 2015

Update WSClient for yavijava#38 and yavijava#123
Attempt to fix issue yavijava#38 and yavijava#123 in order to reuse the SSLSocketFactory

Fixes yavijava#38
Fixes yavijava#123

Added test for the SSLSocketFactory initialization

@michaelrice michaelrice added bug and removed enhancement labels Aug 19, 2015

michaelrice added a commit to michaelrice/yavijava that referenced this issue Aug 19, 2015

Update WSClient for yavijava#38 and yavijava#123
Attempt to fix issue yavijava#38 and yavijava#123 in order to reuse the SSLSocketFactory

Fixes yavijava#38
Fixes yavijava#123

Added test for the SSLSocketFactory initialization

michaelrice added a commit to michaelrice/yavijava that referenced this issue Aug 24, 2015

Update WSClient for yavijava#38 and yavijava#123
Attempt to fix issue yavijava#38 and yavijava#123 in order to reuse the SSLSocketFactory

Fixes yavijava#38
Fixes yavijava#123

Added test for the SSLSocketFactory initialization

michaelrice added a commit to michaelrice/yavijava that referenced this issue Aug 24, 2015

Update WSClient for yavijava#38 and yavijava#123
Attempt to fix issue yavijava#38 and yavijava#123 in order to reuse the SSLSocketFactory

Fixes yavijava#38
Fixes yavijava#123

Added test for the SSLSocketFactory initialization

michaelrice added a commit to michaelrice/yavijava that referenced this issue Aug 25, 2015

Update WSClient for yavijava#38 and yavijava#123
Attempt to fix issue yavijava#38 and yavijava#123 in order to reuse the SSLSocketFactory

Fixes yavijava#38
Fixes yavijava#123

Added test for the SSLSocketFactory initialization
Made change to build file so signing only happens for production.

michaelrice added a commit that referenced this issue Aug 26, 2015

scottmlaplante added a commit to scottmlaplante/yavijava that referenced this issue Aug 31, 2015

michaelrice added a commit that referenced this issue Sep 8, 2015

Merge pull request #134 from scottmlaplante/trustmanager-sslsocketfac…
…tory-fix

Fix for trust manager regression introduced by changes for #38 and #123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment