sslcontext-kickstart
sslcontext-kickstart copied to clipboard
Version 7.4.3 fails to accept all certificates
Describe the bug
It seems that changes in version 7.4.3 skips accept-all truststore configured using
builder.withUnsafeTrustMaterial()
or builder.withTrustingAllCertificatesWithoutValidation()
This is because UnsafeX509ExtendedTrustManager has 0 accepted X509Certificate therefore it is skipped during checking in CombinableX509TrustManager
To Reproduce
SSLFactory.Builder builder = SSLFactory.builder().withDefaultTrustMaterial();
builder.withUnsafeTrustMaterial();
SSLFactory factory = builder.build();
SSLContext sslContext = factory.getSslContext();
SSLContext.setDefault(sslContext);
Expected behavior Validation of the TLS certificate should pass and connection should be established
Environmental Data:
- Java Version 11.0
- Gradle
- OS MacOS
Additional context The test passes with 7.3.0 and 7.4.2
Hi @ivenhov
Thank you for submitting this issue. This behaviour is indeed happening since the bugfix introduced from this issue: https://github.com/Hakky54/sslcontext-kickstart/issues/181
The issue you have found is fixable, however in your example you are using withUnsafeTrustMaterial
alongside with the withDefaultTrustMaterial
. At runtime all certificate will be ignored/not validated because you are using the withUnsafeTrustMaterial
, so it does not make sense to also use withDefaultTrustMaterial
as all trusted certificates from this option will be ignored. If you prefer to use the withUnsafeTrustMaterial
you can disregard the withDefaultTrustMaterial
option and it will work. But maybe I am missing some context to understand your usecase. Can you explain why you would need to use the unsafeTrustmaterial with other trust options?
Hi Hakky54
Thanks for prompt reply.
Using just withUnsafeTrustMaterial
indeed fixes the problem since CombinableX509TrustManager
is no longer used.
As for the reasons why I used both withDefaultTrustMaterial
and withUnsafeTrustMaterial
it was simply because in my original code I had a check like below
SSLFactory.Builder builder = SSLFactory.builder().withDefaultTrustMaterial();
if (trustAll)
builder.withUnsafeTrustMaterial();
if (pathGiven)
builder.withTrustMaterial(....);
SSLFactory factory = builder.build();
SSLContext sslContext = factory.getSslContext();
SSLContext.setDefault(sslContext);
In addition I was adding withTrustMaterial
with filepath if specified
So I was simply avoiding if/else there and being lazy at the same time.
Also I was assuming since it is a builder its ok to call those with...() in any order and it's composable.
Thanks again for pointing that to me and providing workaround.
Ah, yes now your issue makes more sense. Thank you for providing additional context. I think the library can be improved to cover this kind of use case. The benefit will be that you don't need to care what the order of trustmaterial should be because it should resolve that for you. And next to that i would prefer it to be compatible with previous releases, so I will fix it and publish a new version of it this week.
I made a release today, the fix is available at version 7.4.5. Please let me know if this resolves your issue and if you have other issues! 😄
Hi
I reverted my code changes and tested with 7.4.5 and it works as expected. Thanks for you quick response. I have one question regarding the changes for this patch, link Specifically for the TrustManagerUtils
if (trustManagers.size() == 1) {
baseTrustManager = trustManagers.get(0);
} else {
baseTrustManager = trustManagers.stream()
.map(TrustManagerUtils::unwrapIfPossible)
.flatMap(Collection::stream)
.filter(trustManager -> trustManager.getAcceptedIssuers().length > 0)
.collect(Collectors.collectingAndThen(Collectors.toList(), CompositeX509ExtendedTrustManager::new));
}
If there is only one trust manager given it seems it will assume it will have acceptedIssuers > 0 which may not always be the case. In other words filtering is not performed. I don't know if tests are covering that case. Apologies if not relevant at all or I'm just talking nonsense.
Cheers
Great to hear that it is now working with your original code.
Your remarks regarding the code snippet you have shared are valid indeed. The code in the body of the if statement and even the else statement can be improved. The if branch can have maybe a single trustmanager containing no accepted issuers and if that's the case it will throw a runtime exception during the SSL handshake. So it will be better to throw a custom exception to warn the user beforehand instead of the exception during the SSL handshake which will occur at a much later stage
The else branch can have multiple trustmanagers but after filtering if there is only one trustmanager it should be assigned to the base trustmanager and should not be wrapped by the composite trustmanager. And the other case is that after filtering if there is no trustmanager at all and if that's the case we should throw an exception.
Very well discovered Daniel! Would you like to contribute by creating a pull request?
HI
Sorry for late reply I gave it a go but unfortunately it breaks several tests which rely on the existing behaviour. For example it is common in the test to accept a trust manager with no accepted issuers. implementing 'else' branch handling as you described also breaks several tests.
I am wondering which tests will fail. Can you create a pull request, so I can also take a look at your changes.
@ivenhov Do you need any help with it?
Created here https://github.com/Hakky54/sslcontext-kickstart/pull/204 Hopefully i've done it right. I'm not sure if there is automatic way to link pull request with Github issue?