fapifta commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1246593483
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
Review Comment:
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 `SSLContext` to be created,
that uses the `TrustManager`'s `getAcceptedIssuers()` method, and within the
`ReferenceCountedOpenSslServerContext` the 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 the `KeyManager`
counterpart for mTLS authentication when this `SSLContext` is 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 the `chooseEngineClientAlias()` method.
The original `KeyManager` we 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 new `KeyManager` will not have anything to
provide to the original certificates that were present to the native layer via
the `getAcceptedIssuers()` call, therefore once we have rotated the
certificate, the `chooseEngineClientAlias()` 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 current `KeyManager` to
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 `KeyManager` in the load methods.
The reason is that, we just put one private key and one certificate to every
`KeyManager` instance when we create it in the `loadKeyManager()` 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 `SubjectDN` for 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 the `getPrivateKey` and `getCertificateChain`
methods, but that is up for you to consider @ChenSammi I am fine with
delegating the rest down to a non-custom engine provided `KeyManager` instance.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -1220,6 +1245,14 @@ public SCMSecurityProtocolClientSideTranslatorPB
getScmSecureClient()
return scmSecurityClient;
}
+ public static void acquirePermit() throws InterruptedException {
+ semaphore.acquire();
Review Comment:
As we discussed, the SCMCertificateClient is the one that requires this
semaphore for externally syncronizing the renewal, as that happens from within
the SCM state machine in raft as part of the root CA rotation.
Some questions came to my mind regarding this:
Do we need the CertificateLifetimeMonitor within the SCMCertificateClient,
if the whole thing is managed centrally via Ratis? Will a standalone SCM go
through the procedure correctly, or for that we need some further adjustments
in the logic?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509TrustManager.java:
##########
@@ -127,23 +129,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
- X509Certificate rootCACert = caClient.getRootCACertificate() == null ?
- caClient.getCACertificate() : caClient.getRootCACertificate();
+ Set<X509Certificate> rootCACerts = caClient.getAllRootCaCerts().isEmpty() ?
+ caClient.getAllCaCerts() : caClient.getAllRootCaCerts();
- String rootCACertId = rootCACert.getSerialNumber().toString();
// Certificate keeps the same.
- if (currentRootCACertId != null &&
- currentRootCACertId.equals(rootCACertId)) {
+ if (rootCACerts.size() > 0 &&
+ currentRootCACertIds.size() == rootCACerts.size() &&
+ !rootCACerts.stream().filter(
+ c ->
!currentRootCACertIds.contains(c.getSerialNumber().toString()))
+ .findAny().isPresent()) {
Review Comment:
```suggestion
rootCACerts.stream().allMatch(c ->
currentRootCACertIds.contains(c.getSerialNumber().toString()))) {
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
Review Comment:
During testing with small rootCA lifetime, I have noticed that we never do
any cleanup on the certificateMap internally in the DefaultCertificateClient.
It is not critical, and I will file a JIRA separately for that as it is not
critical with the default values, and in our fairly short tests.
If you have an idea where to add it, I am happy to accept that, if not, then
leave this for the future... I don't see any good place at the moment, so I am
on the side to defer this question at least in the context of this PR.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509TrustManager.java:
##########
@@ -127,23 +129,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
- X509Certificate rootCACert = caClient.getRootCACertificate() == null ?
- caClient.getCACertificate() : caClient.getRootCACertificate();
+ Set<X509Certificate> rootCACerts = caClient.getAllRootCaCerts().isEmpty() ?
+ caClient.getAllCaCerts() : caClient.getAllRootCaCerts();
- String rootCACertId = rootCACert.getSerialNumber().toString();
// Certificate keeps the same.
- if (currentRootCACertId != null &&
- currentRootCACertId.equals(rootCACertId)) {
+ if (rootCACerts.size() > 0 &&
+ currentRootCACertIds.size() == rootCACerts.size() &&
+ !rootCACerts.stream().filter(
+ c ->
!currentRootCACertIds.contains(c.getSerialNumber().toString()))
+ .findAny().isPresent()) {
Review Comment:
Also this we might change in the ReloadingX509KeyManager at L240-242
(loadKeyManager method).
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/RootCARotationManager.java:
##########
@@ -255,11 +489,210 @@ public Duration
timeBefore2ExpiryGracePeriod(X509Certificate certificate) {
}
}
+ /**
+ * Task to generate sub-ca key and certificate.
+ */
+ public class SubCARotationPrepareTask implements Runnable {
+ private String rootCACertId;
+
+ public SubCARotationPrepareTask(String newRootCertId) {
+ this.rootCACertId = newRootCertId;
+ }
+
+ @Override
+ public void run() {
+ // Lock to protect the sub CA certificate rotation preparation process,
+ // to make sure there is only one task is ongoing at one time.
+ // Sub CA rotation preparation steps:
+ // 1. generate new sub CA keys
+ // 2. send CSR to leader SCM
+ // 3. wait CSR response and persist the certificate to disk
+ try {
+ acquireLock();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ return;
+ }
+ try {
+ LOG.info("SubCARotationPrepareTask[rootCertId = {}] - started.",
+ rootCACertId);
+
+ SecurityConfig securityConfig =
+ scmCertClient.getSecurityConfig();
+ String progressComponent = SCMCertificateClient.COMPONENT_NAME +
+ HDDS_NEW_KEY_CERT_DIR_NAME_SUFFIX +
+ HDDS_NEW_KEY_CERT_DIR_NAME_PROGRESS_SUFFIX;
+ final String newSubCAProgressPath =
+ securityConfig.getLocation(progressComponent).toString();
+ final String newSubCAPath = securityConfig.getLocation(
+ SCMCertificateClient.COMPONENT_NAME +
+ HDDS_NEW_KEY_CERT_DIR_NAME_SUFFIX).toString();
+
+ File newProgressDir = new File(newSubCAProgressPath);
+ File newDir = new File(newSubCAPath);
+ try {
+ FileUtils.deleteDirectory(newProgressDir);
+ FileUtils.deleteDirectory(newDir);
+ Files.createDirectories(newProgressDir.toPath());
+ } catch (IOException e) {
+ LOG.error("Failed to delete and create {}, or delete {}",
+ newProgressDir, newDir, e);
+ String message = "Terminate SCM, encounter IO exception(" +
+ e.getMessage() + ") when deleting and create directory";
+ scm.shutDown(message);
+ }
+
+ // Generate key
+ Path keyDir = securityConfig.getKeyLocation(progressComponent);
+ KeyCodec keyCodec = new KeyCodec(securityConfig, keyDir);
+ KeyPair newKeyPair = null;
+ try {
+ HDDSKeyGenerator keyGenerator = new HDDSKeyGenerator(securityConfig);
+ newKeyPair = keyGenerator.generateKey();
+ keyCodec.writePublicKey(newKeyPair.getPublic());
+ keyCodec.writePrivateKey(newKeyPair.getPrivate());
+ LOG.info("SubCARotationPrepareTask[rootCertId = {}] - " +
+ "scm key generated.", rootCACertId);
+ } catch (Exception e) {
+ LOG.error("Failed to generate key under {}", newProgressDir, e);
+ String message = "Terminate SCM, encounter exception(" +
+ e.getMessage() + ") when generating new key under " +
+ newProgressDir;
+ scm.shutDown(message);
+ }
+
+ // Get certificate signed
+ String newCertSerialId = "";
+ try {
+ CertificateSignRequest.Builder csrBuilder =
+ scmCertClient.getCSRBuilder();
+ csrBuilder.setKey(newKeyPair);
+ newCertSerialId = scmCertClient.signAndStoreCertificate(
+ csrBuilder.build(),
+ Paths.get(newSubCAProgressPath, HDDS_X509_DIR_NAME_DEFAULT),
+ true);
+ LOG.info("SubCARotationPrepareTask[rootCertId = {}] - " +
+ "scm certificate {} signed.", rootCACertId, newCertSerialId);
+ } catch (Exception e) {
+ LOG.error("Failed to generate certificate under {}",
+ newProgressDir, e);
+ String message = "Terminate SCM, encounter exception(" +
+ e.getMessage() + ") when generating new certificate " +
+ newProgressDir;
+ scm.shutDown(message);
+ }
+
+ // move dir from *-next-progress to *-next
+ try {
+ Files.move(newProgressDir.toPath(), newDir.toPath(),
+ StandardCopyOption.ATOMIC_MOVE,
+ StandardCopyOption.REPLACE_EXISTING);
+ } catch (IOException e) {
+ LOG.error("Failed to move {} to {}",
+ newSubCAProgressPath, newSubCAPath, e);
+ String message = "Terminate SCM, encounter exception(" +
+ e.getMessage() + ") when moving " + newSubCAProgressPath +
+ " to " + newSubCAPath;
+ scm.shutDown(message);
+ }
+
+ // Send ack to rotationPrepare request
+ try {
+ handler.rotationPrepareAck(rootCACertId, newCertSerialId,
+ scm.getScmId());
+ LOG.info("SubCARotationPrepareTask[rootCertId = {}] - " +
+ "rotation prepare ack sent out, new scm certificate {}",
+ rootCACertId, newCertSerialId);
+ } catch (Exception e) {
+ LOG.error("Failed to send ack to rotationPrepare request", e);
+ String message = "Terminate SCM, encounter exception(" +
+ e.getMessage() + ") when sending out rotationPrepare ack";
+ scm.shutDown(message);
+ }
+
+ handler.setSubCACertId(newCertSerialId);
+
+ releaseLockOnTimeoutTask = executorService.schedule(() -> {
+ // If no rotation commit request received after rotation prepare
+ LOG.warn("Failed to have enough rotation acks from SCM. This " +
+ " time root rotation {} is failed. Release the lock.",
+ rootCACertId);
+ releaseLock();
+ }, ackTimeout.toMillis(), TimeUnit.MILLISECONDS);
+
+ } catch (Throwable e) {
+ LOG.error("Unexpected error happen", e);
+ scm.shutDown("Unexpected error happen, " + e.getMessage());
+ }
+ }
+ }
+
+ /**
+ * Task to wait the all acks of prepare request.
+ */
+ public class WaitSubCARotationPrepareAckTask implements Runnable {
+ private String rootCACertId;
+
+ public WaitSubCARotationPrepareAckTask(
+ X509CertificateHolder rootCertHolder) {
+ this.rootCACertId = rootCertHolder.getSerialNumber().toString();
+ }
+
+ @Override
+ public void run() {
+ if (!isRunning()) {
+ LOG.info("SCM is not leader anymore. Delete the in-progress " +
+ "root CA directory");
+ cleanupAndStop("SCM is not leader anymore");
+ return;
+ }
+ synchronized (RootCARotationManager.class) {
+ int numFromHADetails =
+ scm.getSCMHANodeDetails().getPeerNodeDetails().size() + 1;
+ int numFromRatisServer = scm.getScmHAManager().getRatisServer()
+ .getDivision().getRaftConf().getCurrentPeers().size();
+ LOG.info("numFromHADetails {}, numFromRatisServer {}",
+ numFromHADetails, numFromRatisServer);
+ if (handler.rotationPrepareAcks() == numFromRatisServer) {
+ // all acks are received.
+ try {
+ waitAckTimeoutTask.cancel(false);
+ handler.rotationCommit(rootCACertId);
+ handler.rotationCommitted(rootCACertId);
+
+ metrics.incrSuccessRotationNum();
+ long timeTaken = System.nanoTime() - processStartTime.get();
+ metrics.setSuccessTimeInNs(timeTaken);
+ processStartTime.set(null);
+
+ // reset state
+ handler.resetRotationPrepareAcks();
+ String msg = "Root certificate " + rootCACertId +
+ " rotation is finished successfully after " + timeTaken + "
ns";
+ cleanupAndStop(msg);
+ } catch (Throwable e) {
+ LOG.error("Execution error", e);
+ handler.resetRotationPrepareAcks();
+ cleanupAndStop("Execution error, " + e.getMessage());
+ } finally {
+ // stop this task to re-execute again in next cycle
+ throw new RuntimeException("Exit the this " +
+ "WaitSubCARotationPrepareAckTask for root certificate " +
+ rootCACertId + " since the rotation is finished execution");
+ }
+ }
+ }
+ }
+ }
+
/**
* Stops scheduled monitor task.
*/
@Override
public void stop() {
+ if (metrics != null) {
+ metrics.unRegister();
+ }
try {
executorService.shutdown();
if (!executorService.awaitTermination(3, TimeUnit.SECONDS)) {
Review Comment:
As we discussed, due to the various calls to scm.shutDown in case of
unrecoverable failures during the rootCA rotation process, this wait here is
not fortunate, as there is a circular dependency here.
Once we call scm.shutDown, it will call into the stop method of the manager,
then wait 3 seconds to see if the manager stops, but that fundamentally blocks
the manager, and shutdown is graceful, so there won't be interruptions either.
I think we should call the shutdownNow() method only on the executorService,
and whenever we call scm.shutDown also bail out from further execution either
by throwing an exception, or by returning to the caller after scm.shutDown is
called. This way we can ensure that scm shuts down properly, and none of the
rest of the execution that might happen in the executor puts any unexpected
garbage into the on-disk structure, while the executor also tears down
gracefully.
The rollbacks and cleanups seem to happen properly, but I am worried what
happens if we leave the executor to run the thing further, especially because
the executorService uses daemon threads. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
