You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ChenSammi (via GitHub)" <gi...@apache.org> on 2023/06/20 15:53:33 UTC
[GitHub] [ozone] ChenSammi opened a new pull request, #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
ChenSammi opened a new pull request, #4943:
URL: https://github.com/apache/ozone/pull/4943
https://issues.apache.org/jira/browse/HDDS-8689
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1251522001
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
@@ -85,15 +89,65 @@ public ReloadingX509KeyManager(String type, CertificateClient caClient)
@Override
public String chooseEngineClientAlias(String[] strings,
Principal[] principals, SSLEngine sslEngine) {
- return keyManagerRef.get()
+ String ret = keyManagerRef.get()
.chooseEngineClientAlias(strings, principals, sslEngine);
+
+ if (ret == null) {
+ /*
+ Workaround to address that netty tc-native cannot handle the dynamic
+ key and certificate refresh well. What happens is during the setup of
+ the grpc channel, an SSLContext is created, which is
+ ReferenceCountedOpenSslServerContext in the native tc-native case.
+ This class uses the TrustManager's getAcceptedIssuers() as the trusted
+ CA certificate list. The list is not updated after channel is built.
+ With the list being used to present the Principals during the mTLS
+ authentication via the Netty channel under Ratis implementation,
+ the counterpart(client) KeyManager's
+ chooseEngineClientAlias(String, Principal[], SSLEngine) method is
+ called with this old root certificate subject principal, which is now
+ not available in the new Key Manager after refreshed, so the method
+ will return null, which cause the mutual TLS connection establish
+ failure.
+
+ Example error message:
+ Engine client aliases for RSA, DH_RSA, EC, EC_RSA, EC_EC,
+ O=CID-f9f2b2cf-a784-49d7-8577-5d3b13bf0b46,
+ OU=9f52487c-f8f9-45ee-bb56-aca60b56327f,
+ CN=scm-1@scm1.org,
+ org.apache.ratis.thirdparty.io.netty.handler.ssl.OpenSslEngine@5eec0d10
+ is null
+
+ Example success message:
+ Engine client aliases for RSA, DH_RSA, EC, EC_RSA, EC_EC,
+ O=CID-f9f2b2cf-a784-49d7-8577-5d3b13bf0b46,
+ OU=9f52487c-f8f9-45ee-bb56-aca60b56327f,
+ CN=scm-1@scm1.org,
+ org.apache.ratis.thirdparty.io.netty.handler.ssl.OpenSslEngine@5eec0d10
+ is scm/sub-ca_key
+ */
+ ret = alias;
+ LOG.info("Engine client aliases for {}, {}, {} is returned as {}",
+ strings == null ? "" : Arrays.stream(strings).map(Object::toString)
+ .collect(Collectors.joining(", ")),
+ principals == null ? "" : Arrays.stream(principals)
+ .map(Object::toString).collect(Collectors.joining(", ")),
+ sslEngine == null ? "" : sslEngine.toString(), ret);
Review Comment:
Just a personal habit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1618386085
Thank you Sammi for working on this, it looks good to me. We can merge it once you fix Attila's comments
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1621099122
@ChenSammi @adoroszlai @nandakumar131 I'm good with merging this patch, could you please take a look?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1621694804
Thanks @ChenSammi for addressing my comments.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1602815250
> Please don't duplicate `ozonesecure-ha` environment as `ozonesecure-carotation`. Add a new test script `ozonesecure-ha/test-certificate-rotation.sh` instead.
>
> See https://github.com/apache/ozone/blob/master/hadoop-ozone/dist/src/main/compose/ozonesecure/test-certificate-rotation.sh as an example.
>
@adoroszlai , thanks, I refactored the robot test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247448394
##########
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:
Nice suggestion.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247432993
##########
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:
I written one test case to test the effect of System.exit(), it will not continue execute the statement after it. So don't worry that the daemon will continue the execution.
public class TestExecutorService {
static final ScheduledExecutorService executor = Executors.newScheduledThreadPool(1,
new ThreadFactoryBuilder().setNameFormat("Test")
.setDaemon(true).build());
static void test() throws InterruptedException {
executor.submit(new Runnable() {
@Override
public void run() {
while(true) {
if (Thread.currentThread().isInterrupted()) {
System.out.println("interrupted " + new Date());
//break;
} else {
System.out.println("not interrupted " + new Date());
}
executor.shutdownNow();
System.out.println("shutdownNow is called ");
System.exit(1);
System.out.println("exit is called ");
}
}
});
Thread.sleep(1000);
System.out.println("Exiting normally...");
}
public static void main(String[] args) {
try {
test();
} catch (InterruptedException e) {
e.printStackTrace();
}
}
And shutdownNow is a good advice.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1619202856
Thank you for the changes and continued work on this one @ChenSammi I think from my end we are good, so Attila's open comments remains to be addressed than I also think we are good to go.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247466488
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
Review Comment:
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247361178
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
Review Comment:
The only case I can think of to do some cleanup of certificateMap is when some certificates are expired, especially root certificates, or certificate revocation. Besides those, I don't see other cases we need to cleanup certificateMap. For the expired root certificate case, we can do it in a follower up JIRA. For revocation case, I think we can deal with it when revocation is implemented in future.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247514332
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
Review Comment:
agree, let's defer this later, revocation will surely help, as in that case we can revoke the old certificate after the rotation and revocation can handle the removal from the in-memory structures if needed. I forget yesterday, but now I have added a JIRA to track this. (https://issues.apache.org/jira/browse/HDDS-8963)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247509051
##########
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:
It might happen together when the grace period is 0 seconds, so the two monitors are kicking in almost at the same time, besides that an other case, when there is a failure, and the wait ack is equal to the grace period, as then the first failure will push the second try to the time when the certificate monitor also kicks in.
These are rare cases, and should not cause any trouble due to the proper locking now, it might result in unnecessary renewals. I am not worried about those, I think it is just more explicit to have the certificate lifetime monitor explicitly disabled showing that it is not used in SCM. But let's leave ti for now as it is, I fine with it this way as well.
##########
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:
Right, let's use scm.shutdown in combination with just shutdownNow in the scm.stop() method for the rotation manager. Thank you for looking into it!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1615500529
@ChenSammi Both scripts are executed in _acceptance (HA-secure)_ because they both are marked for this test split:
https://github.com/apache/ozone/blob/ecb32652052f6173188ce5dfbf9dbf9d6d94cbc7/hadoop-ozone/dist/src/main/compose/ozonesecure/test-root-ca-rotation.sh#L18
The non-HA one should be assigned to the `secure` split:
https://github.com/apache/ozone/blob/ecb32652052f6173188ce5dfbf9dbf9d6d94cbc7/hadoop-ozone/dist/src/main/compose/ozonesecure/test-vault.sh#L18
But I don't think that's causing any failure. The cluster simply times out waiting for exit from safe mode.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1621718044
Thanks @adoroszlai @fapifta @nandakumar131 @Galsza and @JacksonYao287 for the code review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #4943:
URL: https://github.com/apache/ozone/pull/4943
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247374826
##########
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:
A SCM along doing the certificate rotation is not a common case. I can just think one. Here is the flow,
a. SCM A bootstrapped with its sub CA root signed by root certficate 1. Later some how it doesn't start and join the SCM HA immediately.
b. Root certificate was rotated from 1 to 2.
c. SCM A started and joined the SCM HA.
d. Some time later, SCM A found it need to rotate its certificate.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247588593
##########
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");
Review Comment:
I guess we want to stop scheduling this task again...
I think we can take the reference of the Future once the task is submitted, and cancel via that one instead.
Nice catch @nandakumar131 we have talked about this with Sammi, I just missed this one from my notes as it seems. Similarly as we cancel the waitAckTimeoutTask.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1250278996
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
@@ -85,15 +89,65 @@ public ReloadingX509KeyManager(String type, CertificateClient caClient)
@Override
public String chooseEngineClientAlias(String[] strings,
Principal[] principals, SSLEngine sslEngine) {
- return keyManagerRef.get()
+ String ret = keyManagerRef.get()
.chooseEngineClientAlias(strings, principals, sslEngine);
+
+ if (ret == null) {
+ /*
+ Workaround to address that netty tc-native cannot handle the dynamic
+ key and certificate refresh well. What happens is during the setup of
+ the grpc channel, an SSLContext is created, which is
+ ReferenceCountedOpenSslServerContext in the native tc-native case.
+ This class uses the TrustManager's getAcceptedIssuers() as the trusted
+ CA certificate list. The list is not updated after channel is built.
+ With the list being used to present the Principals during the mTLS
+ authentication via the Netty channel under Ratis implementation,
+ the counterpart(client) KeyManager's
+ chooseEngineClientAlias(String, Principal[], SSLEngine) method is
+ called with this old root certificate subject principal, which is now
+ not available in the new Key Manager after refreshed, so the method
+ will return null, which cause the mutual TLS connection establish
+ failure.
+
+ Example error message:
+ Engine client aliases for RSA, DH_RSA, EC, EC_RSA, EC_EC,
+ O=CID-f9f2b2cf-a784-49d7-8577-5d3b13bf0b46,
+ OU=9f52487c-f8f9-45ee-bb56-aca60b56327f,
+ CN=scm-1@scm1.org,
+ org.apache.ratis.thirdparty.io.netty.handler.ssl.OpenSslEngine@5eec0d10
+ is null
+
+ Example success message:
+ Engine client aliases for RSA, DH_RSA, EC, EC_RSA, EC_EC,
+ O=CID-f9f2b2cf-a784-49d7-8577-5d3b13bf0b46,
+ OU=9f52487c-f8f9-45ee-bb56-aca60b56327f,
+ CN=scm-1@scm1.org,
+ org.apache.ratis.thirdparty.io.netty.handler.ssl.OpenSslEngine@5eec0d10
+ is scm/sub-ca_key
+ */
+ ret = alias;
+ LOG.info("Engine client aliases for {}, {}, {} is returned as {}",
+ strings == null ? "" : Arrays.stream(strings).map(Object::toString)
+ .collect(Collectors.joining(", ")),
+ principals == null ? "" : Arrays.stream(principals)
+ .map(Object::toString).collect(Collectors.joining(", ")),
+ sslEngine == null ? "" : sslEngine.toString(), ret);
Review Comment:
```suggestion
strings == null ? "" : Arrays.toString(strings),
principals == null ? "" : Arrays.toString(principals),
sslEngine == null ? "" : sslEngine, ret);
```
Also, why do you prefer `""` instead of `null` in the log?
##########
hadoop-ozone/dist/src/main/compose/testlib.sh:
##########
@@ -323,15 +338,20 @@ wait_for_port(){
wait_for_execute_command(){
local container=$1
local timeout=$2
- local command=$3
+ shift 2
+ local command=$@
#Reset the timer
SECONDS=0
while [[ $SECONDS -lt $timeout ]]; do
- if docker-compose exec -T $container bash -c '$command'; then
- echo "$command succeed"
- return
+ set +e
+ docker-compose exec -T $container /bin/bash -c "$command"
+ status=$?
+ set -e
+ if [ $status -eq 0 ] ; then
+ echo "$command succeed"
+ return;
Review Comment:
This was recently changed, please avoid reverting to previous implementation.
```suggestion
if docker-compose exec -T $container /bin/bash -c "$command"; then
echo "$command succeed"
```
##########
hadoop-ozone/dist/src/main/compose/testlib.sh:
##########
@@ -262,6 +263,20 @@ execute_command_in_container(){
docker-compose exec -T "$@"
}
+## @description Execute specific commands in docker container
+## @param container name
+## @param specific commands to execute
+execute_commands_in_container(){
+ local container=$1
+ shift 1
+ local command=$@
+
+ set -e
+ # shellcheck disable=SC2068
+ docker-compose exec -T $container /bin/bash -c "$command"
+ set +e
Review Comment:
```suggestion
# shellcheck disable=SC2068
docker-compose exec -T $container /bin/bash -c "$command"
```
`execute_command_in_container` was recently changed to avoid changing `exit-on-error` setting, please apply the same here.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
@@ -85,15 +89,65 @@ public ReloadingX509KeyManager(String type, CertificateClient caClient)
@Override
public String chooseEngineClientAlias(String[] strings,
Principal[] principals, SSLEngine sslEngine) {
- return keyManagerRef.get()
+ String ret = keyManagerRef.get()
.chooseEngineClientAlias(strings, principals, sslEngine);
+
+ if (ret == null) {
+ /*
+ Workaround to address that netty tc-native cannot handle the dynamic
+ key and certificate refresh well. What happens is during the setup of
+ the grpc channel, an SSLContext is created, which is
+ ReferenceCountedOpenSslServerContext in the native tc-native case.
+ This class uses the TrustManager's getAcceptedIssuers() as the trusted
+ CA certificate list. The list is not updated after channel is built.
+ With the list being used to present the Principals during the mTLS
+ authentication via the Netty channel under Ratis implementation,
+ the counterpart(client) KeyManager's
+ chooseEngineClientAlias(String, Principal[], SSLEngine) method is
+ called with this old root certificate subject principal, which is now
+ not available in the new Key Manager after refreshed, so the method
+ will return null, which cause the mutual TLS connection establish
+ failure.
+
+ Example error message:
+ Engine client aliases for RSA, DH_RSA, EC, EC_RSA, EC_EC,
+ O=CID-f9f2b2cf-a784-49d7-8577-5d3b13bf0b46,
+ OU=9f52487c-f8f9-45ee-bb56-aca60b56327f,
+ CN=scm-1@scm1.org,
+ org.apache.ratis.thirdparty.io.netty.handler.ssl.OpenSslEngine@5eec0d10
+ is null
+
+ Example success message:
+ Engine client aliases for RSA, DH_RSA, EC, EC_RSA, EC_EC,
+ O=CID-f9f2b2cf-a784-49d7-8577-5d3b13bf0b46,
+ OU=9f52487c-f8f9-45ee-bb56-aca60b56327f,
+ CN=scm-1@scm1.org,
+ org.apache.ratis.thirdparty.io.netty.handler.ssl.OpenSslEngine@5eec0d10
+ is scm/sub-ca_key
+ */
+ ret = alias;
+ LOG.info("Engine client aliases for {}, {}, {} is returned as {}",
+ strings == null ? "" : Arrays.stream(strings).map(Object::toString)
+ .collect(Collectors.joining(", ")),
+ principals == null ? "" : Arrays.stream(principals)
+ .map(Object::toString).collect(Collectors.joining(", ")),
+ sslEngine == null ? "" : sslEngine.toString(), ret);
+ }
+ return ret;
}
@Override
public String chooseEngineServerAlias(String s, Principal[] principals,
SSLEngine sslEngine) {
- return keyManagerRef.get()
+ String ret = keyManagerRef.get()
.chooseEngineServerAlias(s, principals, sslEngine);
+ if (ret == null && LOG.isDebugEnabled()) {
+ LOG.debug("Engine server aliases for {}, {}, {} is null", s,
+ principals == null ? "" : Arrays.stream(principals)
+ .map(Object::toString).collect(Collectors.joining(", ")),
+ sslEngine == null ? "" : sslEngine.toString());
Review Comment:
```suggestion
principals == null ? "" : Arrays.toString(principals),
sslEngine == null ? "" : sslEngine);
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/SCMSecurityProtocolServerSideTranslatorPB.java:
##########
@@ -253,7 +253,7 @@ public SCMGetCertResponseProto getSCMCertificate(
throw createNotHAException();
}
String certificate = impl.getSCMCertificate(request.getScmDetails(),
- request.getCSR());
+ request.getCSR(), request.hasRenew() ? request.getRenew() : false);
Review Comment:
Nit:
```suggestion
request.getCSR(), request.hasRenew() && request.getRenew());
```
##########
hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test-root-ca-rotation.sh:
##########
@@ -0,0 +1,93 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#suite:HA-secure
+
+COMPOSE_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+export COMPOSE_DIR
+
+export SECURITY_ENABLED=true
+export OM_SERVICE_ID="omservice"
+export SCM=scm1.org
+export COMPOSE_FILE=docker-compose.yaml:root-ca-rotation.yaml
+
+: ${OZONE_BUCKET_KEY_NAME:=key1}
+
+# shellcheck source=/dev/null
+source "$COMPOSE_DIR/../testlib.sh"
+
+start_docker_env
+
+execute_command_in_container kms hadoop key create ${OZONE_BUCKET_KEY_NAME}
+
+execute_robot_test scm1.org kinit.robot
+
+# verify root CA rotation monitor task is active on leader
+wait_for_execute_command scm1.org 30 "jps | grep StorageContainerManagerStarter | sed 's/StorageContainerManagerStarter//' | xargs | xargs -I {} jstack {} | grep 'RootCARotationManager-Active'"
+
+# wait and verify root CA is rotated
+wait_for_execute_command scm1.org 180 "ozone admin cert info 2"
+
+# transfer leader to scm2.org
+execute_robot_test scm1.org scmha/scm-leader-transfer.robot
+wait_for_execute_command scm1.org 30 "jps | grep StorageContainerManagerStarter | sed 's/StorageContainerManagerStarter//' | xargs | xargs -I {} jstack {} | grep 'RootCARotationManager-Inactive'"
+
+# verify om operations
+execute_commands_in_container scm1.org "ozone sh volume create /r-v1 && ozone sh bucket create /r-v1/r-b1"
+
+# verify scm operations
+execute_robot_test scm1.org admincli/pipeline.robot
+
+# wait for next root CA rotation
+wait_for_execute_command scm1.org 180 "ozone admin cert info 3"
+
+# bootstrap new SCM4 and verify certificate
+docker-compose up -d scm4.org
+wait_for_port scm4.org 9894 120
+execute_robot_test scm4.org kinit.robot
+wait_for_execute_command scm4.org 120 "ozone admin scm roles | grep scm4.org"
+wait_for_execute_command scm4.org 30 "ozone admin cert list --role=scm | grep scm4.org"
+
+# wait for next root CA rotation
+wait_for_execute_command scm4.org 180 "ozone admin cert info 4"
+
+#transfer leader to scm4.org
+export TARGET_SCM=scm4.org
+execute_robot_test scm4.org scmha/scm-leader-transfer.robot
Review Comment:
```suggestion
execute_robot_test scm4.org -v "TARGET_SCM:scm4.org" scmha/scm-leader-transfer.robot
```
##########
hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test-root-ca-rotation.sh:
##########
@@ -0,0 +1,93 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#suite:HA-secure
+
+COMPOSE_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+export COMPOSE_DIR
+
+export SECURITY_ENABLED=true
+export OM_SERVICE_ID="omservice"
+export SCM=scm1.org
+export COMPOSE_FILE=docker-compose.yaml:root-ca-rotation.yaml
+
+: ${OZONE_BUCKET_KEY_NAME:=key1}
+
+# shellcheck source=/dev/null
+source "$COMPOSE_DIR/../testlib.sh"
+
+start_docker_env
+
+execute_command_in_container kms hadoop key create ${OZONE_BUCKET_KEY_NAME}
+
+execute_robot_test scm1.org kinit.robot
+
+# verify root CA rotation monitor task is active on leader
+wait_for_execute_command scm1.org 30 "jps | grep StorageContainerManagerStarter | sed 's/StorageContainerManagerStarter//' | xargs | xargs -I {} jstack {} | grep 'RootCARotationManager-Active'"
+
+# wait and verify root CA is rotated
+wait_for_execute_command scm1.org 180 "ozone admin cert info 2"
+
+# transfer leader to scm2.org
+execute_robot_test scm1.org scmha/scm-leader-transfer.robot
+wait_for_execute_command scm1.org 30 "jps | grep StorageContainerManagerStarter | sed 's/StorageContainerManagerStarter//' | xargs | xargs -I {} jstack {} | grep 'RootCARotationManager-Inactive'"
+
+# verify om operations
+execute_commands_in_container scm1.org "ozone sh volume create /r-v1 && ozone sh bucket create /r-v1/r-b1"
+
+# verify scm operations
+execute_robot_test scm1.org admincli/pipeline.robot
+
+# wait for next root CA rotation
+wait_for_execute_command scm1.org 180 "ozone admin cert info 3"
+
+# bootstrap new SCM4 and verify certificate
+docker-compose up -d scm4.org
+wait_for_port scm4.org 9894 120
+execute_robot_test scm4.org kinit.robot
+wait_for_execute_command scm4.org 120 "ozone admin scm roles | grep scm4.org"
+wait_for_execute_command scm4.org 30 "ozone admin cert list --role=scm | grep scm4.org"
+
+# wait for next root CA rotation
+wait_for_execute_command scm4.org 180 "ozone admin cert info 4"
+
+#transfer leader to scm4.org
+export TARGET_SCM=scm4.org
+execute_robot_test scm4.org scmha/scm-leader-transfer.robot
+
+# add new datanode4 and verify certificate
+docker-compose up -d datanode4
+wait_for_port datanode4 9856 60
+wait_for_execute_command scm4.org 60 "ozone admin datanode list | grep datanode4"
+
+#transfer leader to scm3.org
+execute_robot_test scm3.org kinit.robot
+export TARGET_SCM=scm3.org
+execute_robot_test scm4.org scmha/scm-leader-transfer.robot
Review Comment:
```suggestion
execute_robot_test scm4.org -v "TARGET_SCM:scm3.org" scmha/scm-leader-transfer.robot
```
##########
hadoop-ozone/dist/src/main/compose/testlib.sh:
##########
@@ -203,6 +203,7 @@ execute_robot_test(){
-v OZONE_DIR:"${OZONE_DIR}" \
-v SECURITY_ENABLED:"${SECURITY_ENABLED}" \
-v SCM:"${SCM}" \
+ -v TARGET_SCM:"${TARGET_SCM:-scm2.org}" \
Review Comment:
`TARGET_SCM` is specific to few tests, it should be passed in `ozonesecure-ha/test-root-ca-rotation.sh`.
```suggestion
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/RootCARotationHandler.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.security;
+
+import org.apache.hadoop.hdds.scm.metadata.Replicate;
+
+import java.io.IOException;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * This interface defines APIs for sub-ca rotation instructions.
+ */
+public interface RootCARotationHandler {
+
+ /**
+ * Notify SCM peers to do sub-ca rotation preparation and replicate
+ * this operation through RATIS.
+ * @param rootCertId the new root certificate serial ID
+ * @throws IOException on failure to persist configuration
+ */
+ @Replicate
+ void rotationPrepare(String rootCertId)
+ throws IOException, TimeoutException;
+
+ @Replicate
+ void rotationPrepareAck(String rootCertId, String scmCertId, String scmId)
+ throws IOException, TimeoutException;
+
+ @Replicate
+ void rotationCommit(String rootCertId)
+ throws IOException, TimeoutException;
+
+ @Replicate
+ void rotationCommitted(String rootCertId)
+ throws IOException, TimeoutException;
Review Comment:
`@Replicate` no longer requires `TimeoutException` to be declared.
##########
hadoop-ozone/dist/src/main/smoketest/scmha/scm-leader-transfer.robot:
##########
@@ -21,6 +21,7 @@ Resource ../commonlib.robot
Test Timeout 5 minutes
*** Variables ***
+${TARGET_SCM}= %{TARGET_SCM=scm2.org}
Review Comment:
Robot variable is passed via `-v` option. This can be simplified to avoid using an environment variable, too.
```suggestion
${TARGET_SCM} scm2.org
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "JacksonYao287 (via GitHub)" <gi...@apache.org>.
JacksonYao287 commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1621619470
LGTM +1， thanks @ChenSammi for working on this!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1615557337
> @ChenSammi Both scripts are executed in _acceptance (HA-secure)_ because they both are marked for this test split:
>
> https://github.com/apache/ozone/blob/ecb32652052f6173188ce5dfbf9dbf9d6d94cbc7/hadoop-ozone/dist/src/main/compose/ozonesecure/test-root-ca-rotation.sh#L18
>
> The non-HA one should be assigned to the `secure` split:
>
> https://github.com/apache/ozone/blob/ecb32652052f6173188ce5dfbf9dbf9d6d94cbc7/hadoop-ozone/dist/src/main/compose/ozonesecure/test-vault.sh#L18
>
> But I don't think that's causing any failure. The cluster simply times out waiting for exit from safe mode. (Logs are available only until the failed check is re-run...)
Good catch! Thanks @adoroszlai. Let me fix that.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247374826
##########
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:
A SCM along doing the certificate rotation is not a common case. I can just think one. Here is the flow,
a. SCM A bootstrapped with its sub CA root signed by root certficate 1. Later some how it doesn't start and join the SCM HA immediately.
b. Root certificate was rotated from 1 to 2.
c. SCM A started and joined the SCM HA.
d. Some time later, SCM A found it need to rotate its certificate.
So let's keep the CertificateLifetimeMonitor in SCMCertificateClient.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247588593
##########
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");
Review Comment:
If I remember well, we want to stop scheduling this task again...
I think we can take the reference of the Future once the task is submitted, and cancel via that one instead.
Nice catch @nandakumar131 we have talked about this with Sammi, I just missed this one from my notes as it seems. Similarly as we cancel the waitAckTimeoutTask.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1248610081
##########
hadoop-ozone/dist/src/main/compose/ozonesecure/test-root-ca-rotation.sh:
##########
@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#suite:secure
+
+COMPOSE_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+export COMPOSE_DIR
+
+export SECURITY_ENABLED=true
+export OM_SERVICE_ID="omservice"
+export SCM=scm1.org
Review Comment:
```suggestion
export SCM=scm
```
This is the cause for the safe mode timeout.
SCM actually exits safe mode, but we're checking in the wrong container (`scm1.org` does not exist):
```
scm_1 | 2023-07-01 04:10:26,398 [EventQueue-OpenPipelineForHealthyPipelineSafeModeRule] INFO safemode.SCMSafeModeManager: SCM exiting safe mode.
```
##########
hadoop-ozone/dist/src/main/compose/ozonesecure/test-root-ca-rotation.sh:
##########
@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#suite:secure
+
+COMPOSE_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+export COMPOSE_DIR
+
+export SECURITY_ENABLED=true
+export OM_SERVICE_ID="omservice"
+export SCM=scm1.org
+export COMPOSE_FILE=docker-compose.yaml:root-ca-rotation.yaml
+
+: ${OZONE_BUCKET_KEY_NAME:=key1}
+
+# shellcheck source=/dev/null
+source "$COMPOSE_DIR/../testlib.sh"
+
+start_docker_env
+
+execute_command_in_container kms hadoop key create ${OZONE_BUCKET_KEY_NAME}
+
+execute_robot_test s3g kinit.robot
+
+# verify root CA rotation monitor task is active on leader
+wait_for_execute_command scm 30 "jps | grep StorageContainerManagerStarter | sed 's/StorageContainerManagerStarter//' | xargs | xargs -I {} jstack {} | grep 'RootCARotationManager-Active'"
+
+# wait and verify root CA is rotated
+wait_for_execute_command scm 90 "ozone admin cert info 2"
+
+# verify om operations and data operations
+execute_commands_in_container scm1.org "ozone sh volume create /r-v1 && ozone sh bucket create /r-v1/r-b1"
Review Comment:
```suggestion
execute_commands_in_container scm "ozone sh volume create /r-v1 && ozone sh bucket create /r-v1/r-b1"
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4943:
URL: https://github.com/apache/ozone/pull/4943#issuecomment-1615493566
I checked run https://github.com/apache/ozone/actions/runs/5428637052,
actually the ozonesecure-ha/test-root-ca-rotation.sh is passed. The acceptance (HA-secure) is failed when it try to run ozonesecure/test-root-ca-rotation.sh under this HA-secure acceptance.
Hi @adoroszlai , is there any limitation on the bash file naming? In this patch, there are two bash file with the same name test-root-ca-rotation.sh, one under ozonesecure, another under ozonesecure-ha.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247505936
##########
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:
Right, let's use scm.shutdown in combination with just shutdownNow in the scm.stop() method for the rotation manager. Thank you for looking into it!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247515568
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
Review Comment:
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] nandakumar131 commented on a diff in pull request #4943: HDDS-8689. Rotate Root CA and Sub CA in SCM.
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #4943:
URL: https://github.com/apache/ozone/pull/4943#discussion_r1247527846
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/HASecurityUtils.java:
##########
@@ -391,5 +405,20 @@ public static SCMRatisResponse submitScmCertsToRatis(RaftGroup raftGroup,
return new SCMSecurityProtocolClientSideTranslatorPB(
new SCMSecurityProtocolFailoverProxyProvider(conf,
UserGroupInformation.getCurrentUser()));
+
+ }
+
+ public static boolean isSelfSignedCertificate(X509Certificate cert) {
+ if (cert.getIssuerX500Principal().equals(cert.getSubjectX500Principal())) {
+ return true;
+ }
+ return false;
Review Comment:
```suggestion
return cert.getIssuerX500Principal().equals(cert.getSubjectX500Principal());
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
@@ -170,6 +252,11 @@ private X509ExtendedKeyManager loadKeyManager(CertificateClient caClient)
privateKey, EMPTY_PASSWORD,
newCertList.toArray(new X509Certificate[0]));
+ LOG.info("New key manager is loaded with certificate chain");
+ for (int i = 0; i < newCertList.size(); i++) {
+ LOG.info(newCertList.get(i).toString());
+ }
Review Comment:
```suggestion
for (X509Certificate x509Certificate : newCertList) {
LOG.info(x509Certificate.toString());
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/RootCARotationHandlerImpl.java:
##########
@@ -0,0 +1,276 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.security;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler;
+import org.apache.hadoop.hdds.scm.ha.SCMRatisServer;
+import org.apache.hadoop.hdds.scm.server.StorageContainerManager;
+import org.apache.hadoop.hdds.security.SecurityConfig;
+import org.apache.hadoop.hdds.security.x509.certificate.client.SCMCertificateClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.reflect.Proxy;
+import java.math.BigInteger;
+import java.nio.file.Files;
+import java.nio.file.StandardCopyOption;
+import java.security.cert.X509Certificate;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicReference;
+
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_BACKUP_KEY_CERT_DIR_NAME_SUFFIX;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_NEW_KEY_CERT_DIR_NAME_PROGRESS_SUFFIX;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_NEW_KEY_CERT_DIR_NAME_SUFFIX;
+import static org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol.RequestType.CERT_ROTATE;
+import static org.apache.hadoop.ozone.OzoneConsts.SCM_ROOT_CA_COMPONENT_NAME;
+
+/**
+ * Root CA Rotation Handler for ratis SCM statemachine.
+ */
+public class RootCARotationHandlerImpl implements RootCARotationHandler {
+
+ public static final Logger LOG =
+ LoggerFactory.getLogger(RootCARotationHandlerImpl.class);
+
+ private final StorageContainerManager scm;
+ private final SCMCertificateClient scmCertClient;
+ private final SecurityConfig secConfig;
+ private Set<String> newScmCertIdSet = new HashSet<>();
+ private String newCAComponent = SCM_ROOT_CA_COMPONENT_NAME +
+ HDDS_NEW_KEY_CERT_DIR_NAME_SUFFIX +
+ HDDS_NEW_KEY_CERT_DIR_NAME_PROGRESS_SUFFIX;
Review Comment:
This field is never used, we can remove it.
##########
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");
Review Comment:
Why are we throwing exception from `finally` block here?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/HASecurityUtils.java:
##########
@@ -391,5 +405,20 @@ public static SCMRatisResponse submitScmCertsToRatis(RaftGroup raftGroup,
return new SCMSecurityProtocolClientSideTranslatorPB(
new SCMSecurityProtocolFailoverProxyProvider(conf,
UserGroupInformation.getCurrentUser()));
+
+ }
+
+ public static boolean isSelfSignedCertificate(X509Certificate cert) {
+ if (cert.getIssuerX500Principal().equals(cert.getSubjectX500Principal())) {
+ return true;
+ }
+ return false;
+ }
+
+ public static boolean isCACertificate(X509Certificate cert) {
+ if (cert.getBasicConstraints() != -1) {
+ return true;
+ }
+ return false;
Review Comment:
```suggestion
return cert.getBasicConstraints() != -1;
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509TrustManager.java:
##########
@@ -155,15 +156,19 @@ X509TrustManager loadTrustManager(CertificateClient caClient)
break;
}
}
- currentRootCACertId = rootCACertId;
+ currentRootCACertIds.clear();
+ rootCACerts.stream().forEach(
+ c -> currentRootCACertIds.add(c.getSerialNumber().toString()));
Review Comment:
```suggestion
rootCACerts.forEach(
c -> currentRootCACertIds.add(c.getSerialNumber().toString()));
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org