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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
