Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a New ConfigurableX509TrustManager Class #6805
Add a New ConfigurableX509TrustManager Class #6805
Changes from all commits
0914bcb
39a2363
e7b18a0
624edfd
1bb3e43
89b5c4f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this could be implemented, but using
KeyStore.aliases()
andKeyStore.getCertificate(alias)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing it will cause one test to fail, and I think the cause is probably https://github.com/netty/netty/blob/netty-4.1.48.Final/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java#L153-L164. It looks like if setting this, Netty internally will use it to do some additional checks, which is not the behavior I want. I might want
checkClientTrusted
andcheckServerTrusted
to cover all the trust behaviors we might have during the authentication.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... That's sort of worrisome. Go ahead and put a comment here saying why it is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually... this looks like it is only for certificate chain verification. What does it break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've summarized my findings in this Gist: https://gist.github.com/ZhenLian/50c73f146493ac2fe9746d196082679b.
If I changed
getAcceptedIssuers
to the one namedchange.java
in my Gist, one of the tests I wrote would fail, with the error message namedfailure.log
.That is actually an expected behavior under my current implementation. The failing test is basically to test under the scenario that server chooses to
SkipAllVerification
, and client sends a bad certificate, in mTLS. This should be OK because server chooses to skip all verification(in real situations, we want application to provide additional custom checks, but since this is test, we just test the simple behavior). But this test won't pass if we have additional check ingetAcceptedIssuers
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is surprising this is being set, since these variables are normally set elsewhere are read within the trust manager as configuration. This looks suspicious. It will also permanently change the parameters, which seems concerning. Have you verified that it is safe to disable endpoint identification here and it not impact any other code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks for the reminding! I have tested it out locally and it seems good. I also added some integration tests(
basicClientSideIntegrationTest
andbasicServerSideIntegrationTest
), and it looks like thisEndpointIdentificationAlgorithm
behaves as expected, without impacting other parts of the code. Please let me know if you find anything suspicious, thanks!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I tested it once" isn't what I really meant by verify. But since this on the engine, it is probably okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is expected to be extended by users? It seems it would be best to remove
private VerificationAuthType verificationType;
and just have users implement thegetVerificationAuthType()
method directly.Since this is intended to be extended with custom behavior, "options" doesn't really fit as a name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'd like to discuss a bit more about this here. Making it an interface seems a reasonable common practice, but the implication becomes slightly different. Before, I want users to set a
VerificationAuthType
at the construction time, and this can be expressed by making it a construction function of an abstract class; If using interface, users may need to read the comments to know they need to setVerificationAuthType
. The method might be more likesetVerificationAuthType
instead ofgetVerificationAuthType
if using interface. May I know your opinions?Also, does
TlsConfig
sound like a better name for you?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not suggesting to make it an interface; keep it as an abstract class. We use abstract classes so we can add methods to them later.
No, even if the user is implementing it,
getVerificationAuthType()
is a proper name for it. I don't think that would cause a problem for Java programmers, especially those that would be implementing this.The other option is to guarantee that the verification type does not change over time and make
getVerificationAuthType()
final. The problem now is that it can be overridden but it is also required for the constructor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG. Updated accordingly.
Regarding the name of this class, would
TlsConfig
sound better?