Fix AdvancedTlsX509TrustManager to handle client side validation of socket

[cfredri4]

There's a missing if-statement to check SSLEngine vs socket before calling currentDelegateManager.checkClientTrusted

[kannanjgithub]

Please add unit tests.

[cfredri4]

Please add unit tests.

There appears to be a test for this already but that test is a bit moot because it doesn't actually test much (it sets INSECURELY_SKIP_ALL_VERIFICATION). And had that test tested more things it would have failed because of the issue this PR fixes...

I just made this PR as a no-brainer fix to a problem I spotted when looking over unrelated code; I unfortunately don't have the time right now to investigate fixing the existing (moot) test. Feel free to either close the PR, or leave it open until I or someone else finds time to do that.

[erm-g]

Please add unit tests.

There appears to be a test for this already but that test is a bit moot because it doesn't actually test much (it sets INSECURELY_SKIP_ALL_VERIFICATION). And had that test tested more things it would have failed because of the issue this PR fixes...

I just made this PR as a no-brainer fix to a problem I spotted when looking over unrelated code; I unfortunately don't have the time right now to investigate fixing the existing (moot) test. Feel free to either close the PR, or leave it open until I or someone else finds time to do that.

The test you mentioned doesn't go into that branch. We need a new one, maybe based on trustManagerBadCustomVerificationTest. I'll try to spend some time next week.

[ejona86]

@erm-g, are you okay with approving this and you'll make the test as a follow-up?

[erm-g]

@erm-g, are you okay with approving this and you'll make the test as a follow-up?

Yeah, sure

[erm-g]

Please add unit tests.

Test added in https://github.com/grpc/grpc-java/pull/11385