[GitHub] [ozone] fapifta commented on a diff in pull request #5064: HDDS-9015. Block CSR request in SCM for "hdds.x509.rootca.certificate.polling.interval" time period

["fapifta (via GitHub)" <gi...@apache.org>]

fapifta commented on code in PR #5064:
URL: https://github.com/apache/ozone/pull/5064#discussion_r1281539388


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -208,16 +206,31 @@ public String getCertificate(
         nodeDetails.getNodeType(), nodeDetails.getHostName(),
         nodeDetails.getUuid());
     Objects.requireNonNull(nodeDetails);
-    if (storageContainerManager.getRootCARotationManager() != null &&
-        storageContainerManager.getRootCARotationManager()
-            .isRotationInProgress()) {
-      throw new SCMException(("Root CA and Sub CA rotation is in-progress." +
-          " Please try the operation later again."),
-          SCMException.ResultCodes.CA_ROTATION_IN_PROGRESS);
-    }
+    checkIsCertSignRequestAllowed(false);
     return getEncodedCertToString(certSignReq, nodeDetails.getNodeType());
   }
 
+  private void checkIsCertSignRequestAllowed(boolean isScmCertRenew)
+      throws SCMException {
+    RootCARotationManager rotationManager =
+        storageContainerManager.getRootCARotationManager();
+    if (rotationManager != null) {
+      if (rotationManager.isRotationInProgress() && !isScmCertRenew) {
+        throw new SCMException("Root CA and Sub CA rotation is in-progress." +
+            " Please try the operation later again.",
+            ResultCodes.CA_ROTATION_IN_PROGRESS);
+      }
+      if (rotationManager.isPostRotationInProgress()) {
+        SecurityConfig securityConfig = new SecurityConfig(config);
+        throw new SCMException("This action is prohibited for " +
+            securityConfig.getRootCaCertificatePollingInterval() +

Review Comment:
   In this message don't we want to return the remaining duration instead of the polling interval? Alternatively, I would re-word the exception as, in this current form as I understand it it says that there is pollinterval time from the exception during which time the CSRs are rejected, (as the CA rotation **just** finished), but as I see this exception can be thrwon even 1 seconds before the period actually ends.



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/RootCARotationManager.java:
##########
@@ -710,4 +761,52 @@ public boolean shouldSkipRootCert(String newRootCertId) throws IOException {
     }
     return false;
   }
+
+  private void checkAndHandlePostProcessing() throws IOException,
+      CertificateException {
+    CertInfoProto proto = readConfiguration(CertInfoProto.class);
+    if (proto == null) {
+      LOG.info("No {} configuration found in stateful storage",
+          getServiceName());
+      return;
+    }
+
+    X509Certificate cert =
+        CertificateCodec.getX509Certificate(proto.getX509Certificate());
+
+    List<X509Certificate> scmCertChain = scmCertClient.getTrustChain();
+    Preconditions.checkArgument(scmCertChain.size() > 1);
+    X509Certificate rootCert = scmCertChain.get(scmCertChain.size() - 1);
+
+    int result = rootCert.getSerialNumber().compareTo(cert.getSerialNumber());
+    if (result > 0) {
+      // this could happen if the previous stateful configuration is not deleted
+      LOG.warn("Root CA certificate ID {} in stateful storage is smaller than" +
+              " current scm's root certificate ID {}", cert.getSerialNumber(),
+          rootCert.getSerialNumber());
+
+      deleteConfiguration();
+      LOG.warn("Stateful configuration is deleted");
+      return;
+    } else if (result < 0) {
+      // this should not happen
+      throw new RuntimeException("Root CA certificate ID " +
+          cert.getSerialNumber() + " in stateful storage is bigger than " +
+          "current scm's root CA certificate ID " + rootCert.getSerialNumber());
+    }
+
+    Date issueTime = rootCert.getNotBefore();
+    Date now = Calendar.getInstance().getTime();
+    Duration gap = Duration.between(issueTime.toInstant(), now.toInstant());
+    gap = gap.minus(rootCertPollInterval);

Review Comment:
   Nit: If we do the inverse, and decrease rootCertPollInterval with the gap, then we do not need the multiplication by -1 two lines down to calculate the delay.



##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -1436,8 +1439,9 @@ public void run() {
   public synchronized void setCACertificate(X509Certificate cert)
       throws Exception {
     caCertId = cert.getSerialNumber().toString();
+    String pemCert = CertificateCodec.getPEMEncodedString(cert);
     certificateMap.put(caCertId,
-        CertificateCodec.getCertPathFromPemEncodedString(
-            CertificateCodec.getPEMEncodedString(cert)));
+        CertificateCodec.getCertPathFromPemEncodedString(pemCert));
+    pemEncodedCACerts = Arrays.asList(pemCert);

Review Comment:
   Re running findbugs on this one, I believe we added a separate lock for pemEncodedCACerts in #5122, and with that synchronization related warnings would appear, so I would like to be sure.



-- 
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