HDDS-8689. Rotate Root CA and Sub CA in SCM. #4943
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,14 +26,19 @@ | |
| import javax.net.ssl.TrustManager; | ||
| import javax.net.ssl.TrustManagerFactory; | ||
| import javax.net.ssl.X509TrustManager; | ||
| import javax.security.auth.x500.X500Principal; | ||
| import java.io.IOException; | ||
| import java.security.GeneralSecurityException; | ||
| import java.security.KeyStore; | ||
| import java.security.KeyStoreException; | ||
| import java.security.cert.CertificateException; | ||
| import java.security.cert.X509Certificate; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| /** | ||
| * A {@link TrustManager} implementation that exposes a method, | ||
|
|
@@ -55,7 +60,7 @@ public final class ReloadingX509TrustManager implements X509TrustManager { | |
| /** | ||
| * Current Root CA cert in trustManager, to detect if certificate is changed. | ||
| */ | ||
| private List<String> currentRootCACertIds = new ArrayList<>(); | ||
|
|
||
| /** | ||
| * Creates a reloadable trustmanager. The trustmanager reloads itself | ||
|
|
@@ -80,7 +85,16 @@ public void checkClientTrusted(X509Certificate[] chain, String authType) | |
| throws CertificateException { | ||
| X509TrustManager tm = trustManagerRef.get(); | ||
| if (tm != null) { | ||
| try { | ||
| tm.checkClientTrusted(chain, authType); | ||
| } catch (CertificateException e) { | ||
| LOG.info("Client certificate chain {} for authType {} is not trusted", | ||
| chain == null ? "" : Arrays.stream(chain) | ||
| .map(X509Certificate::getSubjectX500Principal) | ||
| .map(X500Principal::toString) | ||
| .collect(Collectors.joining(",")), authType); | ||
| throw e; | ||
| } | ||
| } else { | ||
| throw new CertificateException("Unknown client chain certificate: " + | ||
| chain[0].toString()); | ||
|
|
@@ -92,7 +106,16 @@ public void checkServerTrusted(X509Certificate[] chain, String authType) | |
| throws CertificateException { | ||
| X509TrustManager tm = trustManagerRef.get(); | ||
| if (tm != null) { | ||
| try { | ||
| tm.checkServerTrusted(chain, authType); | ||
| } catch (CertificateException e) { | ||
| LOG.info("Client certificate chain {} for authType {} is not trusted", | ||
| chain == null ? "" : Arrays.stream(chain) | ||
| .map(X509Certificate::getSubjectX500Principal) | ||
| .map(X500Principal::toString) | ||
| .collect(Collectors.joining(",")), authType); | ||
| throw e; | ||
| } | ||
| } else { | ||
| throw new CertificateException("Unknown server chain certificate: " + | ||
| chain[0].toString()); | ||
|
|
@@ -127,23 +150,22 @@ public ReloadingX509TrustManager loadFrom(CertificateClient caClient) { | |
| X509TrustManager loadTrustManager(CertificateClient caClient) | ||
| throws GeneralSecurityException, IOException { | ||
| // SCM certificate client sets root CA as CA cert instead of root CA cert | ||
| Set<X509Certificate> certList = caClient.getAllRootCaCerts(); | ||
| Set<X509Certificate> rootCACerts = certList.isEmpty() ? | ||
| caClient.getAllCaCerts() : certList; | ||
|
|
||
| // Certificate keeps the same. | ||
| if (rootCACerts.size() > 0 && | ||
| currentRootCACertIds.size() == rootCACerts.size() && | ||
| rootCACerts.stream().allMatch(c -> | ||
| currentRootCACertIds.contains(c.getSerialNumber().toString()))) { | ||
| return null; | ||
| } | ||
|
|
||
| X509TrustManager trustManager = null; | ||
| KeyStore ks = KeyStore.getInstance(type); | ||
| ks.load(null, null); | ||
| insertCertsToKeystore(rootCACerts, ks); | ||
|
|
||
| TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance( | ||
| TrustManagerFactory.getDefaultAlgorithm()); | ||
|
|
@@ -155,15 +177,19 @@ X509TrustManager loadTrustManager(CertificateClient caClient) | |
| break; | ||
| } | ||
| } | ||
| currentRootCACertIds.clear(); | ||
| rootCACerts.forEach( | ||
| c -> currentRootCACertIds.add(c.getSerialNumber().toString())); | ||
| return trustManager; | ||
| } | ||
|
|
||
| private void insertCertsToKeystore(Iterable<X509Certificate> certs, | ||
| KeyStore ks) throws KeyStoreException { | ||
| LOG.info("Trust manager is loaded with certificates"); | ||
| for (X509Certificate certToInsert : certs) { | ||
| String certId = certToInsert.getSerialNumber().toString(); | ||
| ks.setCertificateEntry(certId, certToInsert); | ||
| LOG.info(certToInsert.toString()); | ||
| } | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -296,7 +296,8 @@ public synchronized void writeCertificate(Path basePath, String fileName, | |
| try (FileOutputStream file = new FileOutputStream(certificateFile)) { | ||
| file.write(pemEncodedCertificate.getBytes(DEFAULT_CHARSET)); | ||
| } | ||
| LOG.info("Save certificate to {}", certificateFile.getAbsolutePath()); | ||
| LOG.info("Certificate {}", pemEncodedCertificate); | ||
| Files.setPosixFilePermissions(certificateFile.toPath(), permissionSet); | ||
| } | ||
|
|
||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -480,8 +480,11 @@ private OzoneConsts() { | |
| public static final String SCM_ROOT_CA_COMPONENT_NAME = | ||
| Paths.get(SCM_CA_CERT_STORAGE_DIR, SCM_CA_PATH).toString(); | ||
|
|
||
| // %s to distinguish different certificates | ||
| public static final String SCM_SUB_CA = "scm-sub"; | ||
| public static final String SCM_SUB_CA_PREFIX = SCM_SUB_CA + "-%s@"; | ||
| public static final String SCM_ROOT_CA = "scm"; | ||
| public static final String SCM_ROOT_CA_PREFIX = SCM_ROOT_CA + "-%s@"; | ||
|
|
||
| // Layout Version written into Meta Table ONLY during finalization. | ||
| public static final String LAYOUT_VERSION_KEY = "#LAYOUTVERSION"; | ||
|
|
||
Oops, something went wrong.
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.
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.
We had a long discussion on this one today, and on Tuesday as well, let me summarize what we have found and agreed on.
The initial problem with the workaround from my side was that it just seems inappropriate to preserve all the old key manager references, and go over all of them, I wanted to understand why it works, and what else we can do.
The final cause for the problem as we determined is within the Netty tcnative code as you have commented as well. The following happens:
During the setup of the connections there is an
SSLContextto be created, that uses theTrustManager'sgetAcceptedIssuers()method, and within theReferenceCountedOpenSslServerContextthe results are cached to the native layer via BIO. This cached value is not updated ever after initialization, but based on our debugging, that value is used to present to theKeyManagercounterpart for mTLS authentication when thisSSLContextis used for communication via the bidirectional Netty channel under the Ratis implementation.During the communication, the KeyManager's
chooseEngineClientAlias(String, Principal[], SSLEngine)method is being called, and the cached data is used to present the Principals to thechooseEngineClientAlias()method.The original
KeyManagerwe have the reference for has a mechanism to select the certificate and key alias based on the provided principals, and after the first rotation, the newKeyManagerwill not have anything to provide to the original certificates that were present to the native layer via thegetAcceptedIssuers()call, therefore once we have rotated the certificate, thechooseEngineClientAlias()call returned null, and therefore in the Ratis layer the mTLS authentication failed, then was retried without a chance to succeed.The workaround works, as at the end of the day it goes back in time to the first
KeyManager, which still is able to resolve and then return the same alias that was/is used for all the earlier and the currentKeyManagerto store the certificates to be used after the actual rotation.Based on these we agreed that this class should not have the workaround implemented this way, but instead we should stick to the alias we use at the creation of the
KeyManagerin the load methods.The reason is that, we just put one private key and one certificate to every
KeyManagerinstance when we create it in theloadKeyManager()method, and we can be sure that once there is query for a key and certificate, we want to use the only key and certificate that is present.We discussed an alternative, where we use the same
SubjectDNfor all the rootCA certificates, but that causes operational trouble when we want to understand certificate chaining during debugging.Furthermore I believe that as we have just one key and one certificate in every KeyManager, we can safely return the same alias from all the methods that are there to request an alias (
chooseEngineClientAlias,chooseEngineServerAlias,getClientAliases,chooseClientAlias,getServerAliases,chooseServerAlias), and we can safely return the only key and only certificate from thegetPrivateKeyandgetCertificateChainmethods, but that is up for you to consider @ChenSammi I am fine with delegating the rest down to a non-custom engine providedKeyManagerinstance.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.
Yea, it's a wonder discussion. @fapifta ,thanks for the further digging. I will update the comments with more detail info explain the work around the the issue. My current thought is keeping the workaround in a small scope as possbile, try not to bring any unnecessary side effect. So besides chooseEngineClientAlias which needs the workaround now, for other KeyManager functions which doesn't affect by this issue, I tend to keep them untouched, unless later we found they are affected too.
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.
Agree, we can look into that later, agree to keep the scope low, just wanted to note that this might as well be a good approach.