Refactored logic for determining SSL inputs #1607
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rjrudin
commented
Oct 2, 2023
| */ | ||
| private SSLInputs buildSSLInputs(String authType) { | ||
| SSLContext sslContext = null; | ||
| X509TrustManager userTrustManager = getTrustManager(); |
Contributor
Author
There was a problem hiding this comment.
This is where all the changes start. This method should be a lot easier to understand now.
rjrudin
commented
Oct 2, 2023
| return (X509TrustManager) getDefaultTrustManagers()[0]; | ||
| X509TrustManager trustManager = (X509TrustManager) getDefaultTrustManagers()[0]; | ||
| Logger logger = LoggerFactory.getLogger(SSLUtil.class); | ||
| if (logger.isDebugEnabled() && trustManager.getAcceptedIssuers() != null) { |
Contributor
Author
There was a problem hiding this comment.
I moved this logging here as it's useful to see for debugging purposes regardless of when this method is called.
I was looking at this while analyzing support for 2-way SSL and I couldn't really understand what I wrote. So I refactored in the following way: 1. All construction for SSLContext/TrustManager is in buildSSLInputs. 2. There are 4 approaches, each clearly identified and implemented in its own method. 3. I moved the tests I use for verifying SSL support to a "test.ssl" package (no changes to the tests themselves). This may actually fix a bug but I'm not certain. The bug would have been that "newCertificateAuthContext" was called before all the SSL-input logic had occurred, meaning that e.g. "default" as an sslProtocol value would not have impacted certificate authentication.
rjrudin
force-pushed
the
feature/ssl-cleanup
branch
from
4d5206b to
3b73a4a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I was looking at this while analyzing support for 2-way SSL and I couldn't really understand what I wrote. So I refactored in the following way:
This may actually fix a bug but I'm not certain. The bug would have been that "newCertificateAuthContext" was called before all the SSL-input logic had occurred, meaning that e.g. "default" as an sslProtocol value would not have impacted certificate authentication.