Version 7.4.3 fails to accept all certificates

[ivenhov]

Version 7.4.3 fails to accept all certificates

[Hakky54]

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.

[Hakky54]

Hi @ivenhov

Thank you for submitting this issue. This behaviour is indeed happening since the bugfix introduced from this issue: #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?

[ivenhov]

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.

[Hakky54]

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! 😄

[ivenhov]

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

[Hakky54]

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?

[ivenhov]

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.

[Hakky54]

I am wondering which tests will fail. Can you create a pull request, so I can also take a look at your changes.

[Hakky54]

@ivenhov Do you need any help with it?

[ivenhov]

Created here #204
Hopefully i've done it right. I'm not sure if there is automatic way to link pull request with Github issue?