FIPS support ! : by enhancing the winrm4j library to support TLS using Bo… #102
Conversation
…uncycastle (in addition to the common JKS) as the provider. On winrm4j 0.6.1, new changes were introduced by your team: Due to this issue: cloudsoft#80 Commits: cloudsoft#92 cloudsoft#93 Which allowed us to propagate the: * SSLContext (which mostly contains the protocols and as well as the security provider being defined, either JKS or bouncycastle or whatever, vital for FIPS compliance) and the * SSLSocketFactory (which contains the cipher suites to use, vital for FIPS compliance). So when we tried to do it by just passing over both the SSLContext and the SSLSocketFactory, we encountered the following open issue which we had already reported to your team: cloudsoft#97 So in order to fix it: -Based on the SSLSocketFactory (if present), we are just propagating out of it, the cipher suites to the Apache cxf TLSClientParameters class which later on will use such ciphers along with the provider for the secure communication. - Based on the SSLContext, we are passing over the whole SSLContext to the TLSClientParameters, but we are also passing the protocols in the Apache CXF TLSClientParameters, so that the communication can be established successfully and FIPS compliant. There could be better ways to propagate only those parameters (like allowing to pass the ciphers and protocols as new params besides the SSLContext and the SSLSocketFactory), but the fact that how it got implement in 0.6.1 was not stable enough is true, meaning that by incorporating this changes, the client application should only care to propagate both the SSLContext and the SSLSocketFactory and the winrm4j library will use only what's really needed :). Hope you can absorb this changes that will benefit a lot your API, there is no single API similar like yours (like overthere or so) that actually supports FIPS, so this will be a great WIN for your software.
EXTERNALCOREWORKS
changed the title
|
Hi @aledsage, would you mind to take a look at this Improvement PR ? Im willing to have this integrated in the product out of the box, benefiting a lot of customers that looks for FIPS compliance in their secure communications. |
|
Hi colleagues, Any updates on this one ? |
|
Hi, any updates on this PR ? |
There was a problem hiding this comment.
Thanks for this. Some of the changes look ok but others don't compile and look like they wouldn't work. Can you give us an update how much you have tested these, and with what use-cases?
I'm tempted to take the change that sets the cipher suites, but not the change that sets the secureSocketProtocol. However, I'm very hesitant because I don't know what testing if any has been done with this (particularly given the code didn't even compile, and an important line had been accidentally deleted).
| if(sslSocketFactory != null) { | ||
| if(sslSocketFactory.getDefaultCipherSuites()!=null && Arrays.asList(sslSocketFactory.getDefaultCipherSuites()).size()>0) { | ||
| tlsClientParameters.setCipherSuites(Arrays.asList(sslSocketFactory.getDefaultCipherSuites())); | ||
| } |
There was a problem hiding this comment.
Indents not aligned - is this a mix of tabs and spaces?
| } | ||
| if(sslContext != null) { | ||
| tlsClientParameters.setSslContext(sslContext); | ||
| SSLParameters sslparams =sslContext.getDefaultSSLParameters(); |
There was a problem hiding this comment.
This fails to compile for me because there is no import for SSLParameters.
| @@ -291,8 +291,19 @@ public void checkServerTrusted(X509Certificate[] x509Certificates, String s) thr | |||
| if (hostnameVerifier != null || sslSocketFactory != null || sslContext != null) { | |||
| TLSClientParameters tlsClientParameters = new TLSClientParameters(); | |||
| tlsClientParameters.setHostnameVerifier(hostnameVerifier); | |||
| tlsClientParameters.setSSLSocketFactory(sslSocketFactory); | |||
There was a problem hiding this comment.
I don't think you meant to delete this line, to set the SSLSocketFactory!
| SSLParameters sslparams =sslContext.getDefaultSSLParameters(); | ||
| //To make sure that the protocols are correctly set in the cxf context | ||
| if(sslparams.getProtocols() !=null && sslparams.getProtocols().length>0) { | ||
| tlsClientParameters.setSecureSocketProtocol(String.join(",", sslparams.getProtocols())); |
There was a problem hiding this comment.
Is this tested? The code looks surprising: the setSecureSocketProtocol is singular (talking about a single protocol in use), and you pass in a comma-separated list of protocols.
Is it documented anywhere that it can take a list?
Looking at how the code is subsequently used, it seems unlikely.
| tlsClientParameters.setSslContext(sslContext); | ||
| if(sslSocketFactory != null) { | ||
| if(sslSocketFactory.getDefaultCipherSuites()!=null && Arrays.asList(sslSocketFactory.getDefaultCipherSuites()).size()>0) { | ||
| tlsClientParameters.setCipherSuites(Arrays.asList(sslSocketFactory.getDefaultCipherSuites())); |
There was a problem hiding this comment.
Looking at how the tlsClientParameters.getCipherSuites is used, I'm not sure how this will get used:
HttpConduitConfigApplier.applyTlsClientParametershas no effect (code only used inelseblock where SSLSocketFactory was null)HttpConduitConfigApplier.applyTlsClientParameterslooks worrying: it adds to the list by callingp.getCipherSuites().add(...). That code is private so hopefully never called with the cipher suite you pass in - if it was, it would fail (becauseArrays.asListreturns a fixed-size list.AsyncHTTPConduit.initializeSSLEngine: not sure what calls this.
|
Quoting This suggests we do not need to extract cipher suites etc from the Does that contradict the behaviour you have seen @EXTERNALCOREWORKS ? |
…uncycastle (in addition to the common JKS) as the provider.
On winrm4j 0.6.1, new changes were introduced by your team:
Due to this issue: #80
Commits: #92
#93
Which allowed us to propagate the:
So when we tried to do it by just passing over both the SSLContext and the SSLSocketFactory, we encountered the following open issue which we had already reported to your team: Cannot execute Winrm calls passing the SocketFactory and SSLContext to the WinRMTool.Builder #97
So in order to fix it:
*Based on the SSLSocketFactory (if present), we are just propagating out of it, the cipher suites to the Apache cxf TLSClientParameters class which later on will use such ciphers along with the provider for the secure communication.
*Based on the SSLContext, we are passing over the whole SSLContext to the TLSClientParameters, but we are also passing the protocols in the Apache CXF TLSClientParameters, so that the communication can be established successfully and FIPS compliant.
There could be better ways to propagate only those parameters (like allowing to pass the ciphers and protocols as new params besides the SSLContext and the SSLSocketFactory), but the fact that how it got implement in 0.6.1 was not stable enough is true, meaning that by incorporating this changes, the client application should only care to propagate both the SSLContext and the SSLSocketFactory and the winrm4j library will use only what's really needed :).
Hope you can absorb this changes that will benefit a lot your API, there is no single API similar like yours (like overthere or so) that actually supports FIPS, so this will be a great WIN for your software.