TLS socket factory clears client TLS credential too early
Description
Environment
Attachments
- 23 Jan 2019, 11:35 PM
Activity
Brent Putman March 25, 2019 at 8:25 PM
If you want some logging around what it's doing here, you can set the following logging categories to TRACE:
org.opensaml.security.httpclient.impl.SecurityEnhancedTLSSocketFactoryorg.opensaml.security.httpclient.impl.ThreadLocalClientTLSCredentialHandler
You should see log entries that bracket the client calls about setup and teardown on the thread-local things.
The latter operation is literally just one line, and is no different than what it did in the past.
log.trace("Clearing thread-local client TLS X509Credential");
ThreadLocalX509CredentialContext.clearCurrent();
That is effectively invoked in the finally of a try/finally block, so it's pretty much guaranteed to always be called, so I think there's not much chance here of resource leakage:. Unless I screwed up some bit of the error handling, but I've gone over it multiple times and there's lots of unit tests which exercise all the permutations of things throwing during the handler and client calls.
Scott Cantor March 25, 2019 at 6:25 PM
requestContext is a V2 compatibility workaround, and this connector was obviously not available in older configuration files, so there was no reason to support legacy features.
Jim Fox March 25, 2019 at 6:21 PM
I tested this with
opensaml-security-impl-3.4.3-20190325.024452-76.jar and java-support-7.5.0-20190325.002657-3.jar
and it worked well. I did not do any testing to see if it cleaned up after itself, e.g. load testing.
A side note. Previously I always used the requestContext to get the principal name, I think that was the standard way to do it. However, the HTTP connector seems to load only the resolutionContext. Possibly everyone else reads all the documentation, but it caught me by surprise.
Brent Putman March 23, 2019 at 2:12 AM
Via Jenkins errors I realized I flubbed this, I neglected to account for the CloseableHttpClient type that is actually defined to be returned from Apache's builder, not HttpClient. Fixed in java-support:
master: c2539d2d0149723eb4fa8893d1b56ef99a4c7bf7
maint-7: 5ed705b0fa5e22f3a95431453b1b598c993a59be
As a side effect, this actually simplifies the client wrapper impl code quite a bit, since we're now extending an abstract class that implements all the execute(...) methods.
Brent Putman March 23, 2019 at 12:19 AM
@Jim Fox, this fix will be in the next OpenSAML and IdP release, which is planned for the near future, pending another change. However, if you wanted to grab this change and test locally, it shouldn't be too hard. You'd just need the new java-support 7.5.0-SNAPSHOT jar and the new opensaml-security-impl-3.4.3-SNAPSHOT.jar. You could either do a source build or after tonight's Jenkins run completes you could grab the binaries from the Nexus repo SNAPSHOT repo.
I am trying to use the rest data connector with a client certificate. I never see the idp attempting to use the provided cert. This is using javax.net debug and debug log on org.apache.http, etc.
Something I notice in SecurityEnhancedTLSSocketFactory.java (from github source) is that the third SecurityEnhancedTLSSocketFactory constructor does not call its parent constructor. The others do.
I've otherwise done a lot of testing around it. The certs and target work fine with my own data connector, that use many of the same apache.http tools as you, but not in such a byzantine manner.
If you have other ideas where I might investigate please let me know.
Thanks,
Jim