HDDS-11167. Use Key/TrustManagers directly for TLS connection instead of factories #6943
Conversation
|
@fapifta, @dombizita please take a look |
fapifta
left a comment
fapifta
left a comment
There was a problem hiding this comment.
Thank you for simplifying this code @Galsza, the change looks good to me, I have one suggestion that helps to avoid some casts, and a really small nuanced nitpick comment. Please take a look, I am +1 overall after these are implemented.
| * Return the store factory for key manager and trust manager for server. | ||
| */ | ||
| KeyStoresFactory getServerKeyStoresFactory() throws CertificateException; | ||
| KeyManager getKeyManager() throws CertificateException; |
There was a problem hiding this comment.
If you define the return value of this method to be ReloadingX509KeyManager, then you can spare a cast in the methods implementing these and in some tests also, while that way the returned object still remains a KeyManager also, so it can be set to TLSConfig, and SSLContext and other places as is. This same is true for the getTrustManager method where you can use ReloadingX509TrustManager instead of TrustManager.
There was a problem hiding this comment.
Thank for that, that was a great observation. Fixed it
| this.type = type; | ||
| keyManagerRef = new AtomicReference<>(); | ||
| keyManagerRef.set(loadKeyManager(caClient)); | ||
| this.alias = componentName + "_key"; |
There was a problem hiding this comment.
nit: can we move this up by one line before the instantiation of keyManagerRef?
Change the return type of getTrust/KeyManagers. remove some linebreaks only needed for 80 characters/line
|
There is a related test failure on my fork, converting this to draft until I can fix it. |
|
@fapifta please take another look when you get the chance |
2 participants
What changes were proposed in this pull request?
It's possible to use KeyManagers and TrustManagers directly for mTLS communication instead of creating Key/TrustStoresFactory objects.
Please describe your PR in detail:
Typical usage of the Key/TrustManager before this pull request was that the PemFilesBasedKeyStoresFactory created factory methods to get instances of the key and trust managers and use those instances to establish mTLS communication. This is more complicated than it needs to be as for the mTLS only the KeyManager and the TrustManager is needed. By refactoring the CertificateClient can return these.
HDDS-11167
How was this patch tested?
This patch has no functional change, ci coverage should be enough:
https://github.com/Galsza/ozone/actions/runs/9913347327