NMS-9350: Add support for Kerberos message encryption - #32
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
marshallmassengill
left a comment
marshallmassengill
left a comment
There was a problem hiding this comment.
Couple things that seem like blockers (via claude though I've also looked as much as I can and I think these are all valid).
KerberosDecryptInInterceptor.handleMessage returns early when the body doesn't start with --Encrypted Boundary, so a plaintext SOAP response is accepted.
Also, neither wrap nor unwrap inspects the MessageProp afterwards: getPrivacy() is never checked, so an integrity-only token is accepted as confidential in both directions, and isDuplicateToken/isOldToken/isUnseqToken/isGapToken are discarded, throwing away GSS replay detection. With withKerberosEncryption() on, a non-multipart or non-private response has to be a hard failure.
One GSSContextManager per operation, with correctness resting on a JVM-global socket pool. getIdentifier/getEnumerator/getTransferer each call createProxyFor, which constructs a fresh manager, so every identify/enumerate/pull/get runs its own full AP-REQ/AP-REP pre-flight. The encrypted POST then has to land on the exact TCP connection that that manager's handshake bound, but the HttpURLConnection keep-alive pool is keyed only by (host, port, SSLSocketFactory) and hands out any idle socket. Sequential use mostly works because the latest handshake rebinds the single idle socket; two threads sharing a CXFWSManClient can get each other's session. None of these contexts is ever disposed either.
…d enforce GSS confidentiality and replay checks
|
I re-jiggered this because relying on the keepalive pool to maintain the same connection across multiple threads probably wasn't going to work with any real concurrency. Fable helped. |
marshallmassengill
left a comment
marshallmassengill
left a comment
There was a problem hiding this comment.
Minor but I think this is worth doing.
|
Gonna wait for Chandra or Christian to critique it before I merge (no offense) |
christianpape
left a comment
christianpape
left a comment
There was a problem hiding this comment.
The PR adds tlsParams.setSSLSocketFactory(buildPermissiveSslSocketFactory()) next to the existing setTrustManagers(trustAll).
- HttpsURLConnectionFactory.decorateWithTLS:130 — when an SSLSocketFactory is set, CXF uses it and ignores the trust managers, cipher-suite filter and secureSocketProtocol. The trustAll array two lines above is now dead config.
- HttpClientHTTPConduit.setupConnection:352 — an explicit SSLSocketFactory forces CXF off the Java 11 HttpClient conduit back onto the legacy HttpURLConnection conduit, for every strictSSL=false HTTPS endpoint.
The Kerberos path never reads TLSClientParameters at all - KerberosHttpConduit.setupConnection() is a no-op and the session gets its own factory at CXFWSManClient.java:401. So these two lines buy the feature nothing and silently change transport behaviour for the mainstream iDRAC/WinRM-over-HTTPS users. Also builds a fresh SSLContext per operation, losing TLS session resumption. Recommend simply deleting both calls.
| TrustManager[] trustAll = new TrustManager[] { new X509TrustManager() { | ||
| public void checkClientTrusted(java.security.cert.X509Certificate[] certs, String authType) {} | ||
| public void checkServerTrusted(java.security.cert.X509Certificate[] certs, String authType) {} | ||
| public java.security.cert.X509Certificate[] getAcceptedIssuers() { return null; } | ||
| }}; |
There was a problem hiding this comment.
Three identical TrustManager are created inside this class. Check whether they are really used when a SSLSocketFactory is set. If they are really used, consider to create it once as static field and reuse it.
| standalone API jars to make those classes available in the shaded executable jar. | ||
| jaxws-api transitively brings in javax.xml.soap (SAAJ) and javax.annotation as well. | ||
| --> | ||
| <dependency> |
There was a problem hiding this comment.
javax.xml.bind:jaxb-api:2.3.1 and javax.xml.ws:jaxws-api:2.3.1 are now declared twice
| }}; | ||
| TLSClientParameters tlsParams = new TLSClientParameters(); | ||
| tlsParams.setTrustManagers(simpleTrustManager); | ||
| tlsParams.setSSLSocketFactory(buildPermissiveSslSocketFactory()); |
There was a problem hiding this comment.
Same here. I think the TrustManager isn't used at all.
| } else if (operation == WSManOperation.SHELL) { | ||
| if (arguments.isEmpty()) { | ||
| LOG.error("SHELL operation requires a command argument, e.g.: -o SHELL -- ipconfig /all"); | ||
| System.exit(2); |
There was a problem hiding this comment.
This PR wraps the runOperation(client) method in a try-finally block. If System.exit() is called, the finally block is no longer executed. Is this intended?
| LOG.info("Command exited with code {}", result.exitCode()); | ||
| // Close before exiting so the transport session is torn down cleanly | ||
| client.close(); | ||
| System.exit(result.exitCode()); |
4 participants
Adds support for Kerberos message encryption, as detailed in https://msdn.microsoft.com/en-us/library/ee896923.aspx and https://msdn.microsoft.com/en-us/library/cc251574.aspx
This was completed a couple months ago and has been sat on since, so it is not fresh in my head how all this works.
This library is also in a weird half-Jakarta state and could stand some attention once this is merged, before a new release is cut.