[GitHub] [ozone] ChenSammi commented on a diff in pull request #3982: HDDS-7339. Implement Certificate renewal task for services

[GitBox <gi...@apache.org>]

ChenSammi commented on code in PR #3982:
URL: https://github.com/apache/ozone/pull/3982#discussion_r1060525522


##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -1095,4 +1103,314 @@ public synchronized void close() throws IOException {
       clientKeyStoresFactory.destroy();
     }
   }
+
+  /**
+   * Check how much time before certificate will enter expiry grace period.
+   * @return Duration, time before certificate enters the grace
+   *                   period defined by "hdds.x509.renew.grace.duration"
+   */
+  public Duration timeBeforeExpiryGracePeriod(X509Certificate certificate) {
+    Duration gracePeriod = securityConfig.getRenewalGracePeriod();
+    Date expireDate = certificate.getNotAfter();
+    LocalDateTime gracePeriodStart = expireDate.toInstant()
+        .atZone(ZoneId.systemDefault()).toLocalDateTime().minus(gracePeriod);
+    LocalDateTime currentTime = LocalDateTime.now();
+    if (gracePeriodStart.isBefore(currentTime)) {
+      // Cert is already in grace period time.
+      return Duration.ZERO;
+    } else {
+      return Duration.between(currentTime, gracePeriodStart);
+    }
+  }
+
+  /**
+   * Renew keys and certificate. Save the keys are certificate to disk in new
+   * directories, swap the current key directory and certs directory with the
+   * new directories.
+   * @param force, check certificate expiry time again if force is false.
+   * @return String, new certificate ID
+   * */
+  public String renewAndStoreKeyAndCertificate(boolean force)
+      throws CertificateException {
+    if (!force) {
+      synchronized (this) {
+        Preconditions.checkArgument(
+            timeBeforeExpiryGracePeriod(x509Certificate).isZero());
+      }
+    }
+
+    String newKeyPath = securityConfig.getKeyLocation(component)
+        .toString() + HDDS_NEW_KEY_CERT_DIR_NAME_SUFFIX;
+    String newCertPath = securityConfig.getCertificateLocation(component)
+        .toString() + HDDS_NEW_KEY_CERT_DIR_NAME_SUFFIX;
+    File newKeyDir = new File(newKeyPath);
+    File newCertDir = new File(newCertPath);
+    try {
+      FileUtils.deleteDirectory(newKeyDir);
+      FileUtils.deleteDirectory(newCertDir);
+      Files.createDirectories(newKeyDir.toPath());
+      Files.createDirectories(newCertDir.toPath());
+    } catch (IOException e) {
+      throw new CertificateException("Error while deleting/creating " +
+          newKeyPath + " or " + newCertPath + " directories to cleanup " +
+          " certificate storage. ", e, RENEW_ERROR);
+    }
+
+    // Generate key
+    KeyCodec newKeyCodec = new KeyCodec(securityConfig, newKeyDir.toPath());
+    KeyPair newKeyPair;
+    try {
+      newKeyPair = createKeyPair(newKeyCodec);
+    } catch (CertificateException e) {
+      throw new CertificateException("Error while creating new key pair.",
+          e, RENEW_ERROR);
+    }
+
+    // Get certificate signed
+    String newCertSerialId;
+    try {
+      CertificateSignRequest.Builder csrBuilder = getCSRBuilder(newKeyPair);
+      newCertSerialId = signAndStoreCertificate(csrBuilder.build(),
+          Paths.get(newCertPath));
+    } catch (Exception e) {
+      throw new CertificateException("Error while signing and storing new" +
+          " certificates.", e, RENEW_ERROR);
+    }
+
+    // switch Key and Certs directory on disk
+    File currentKeyDir = new File(
+        securityConfig.getKeyLocation(component).toString());
+    File currentCertDir = new File(
+        securityConfig.getCertificateLocation(component).toString());
+    File backupKeyDir = new File(
+        securityConfig.getKeyLocation(component).toString() +
+            HDDS_BACKUP_KEY_CERT_DIR_NAME_SUFFIX);
+    File backupCertDir = new File(
+        securityConfig.getCertificateLocation(component).toString() +
+            HDDS_BACKUP_KEY_CERT_DIR_NAME_SUFFIX);
+
+    if (!currentKeyDir.renameTo(backupKeyDir)) {
+      // Cannot rename current key dir to the backup dir
+      throw new CertificateException("Failed to rename " +
+          currentKeyDir.getAbsolutePath() +
+          " to " + backupKeyDir.getAbsolutePath() + " during " +
+          "certificate renew.", RENEW_ERROR);
+    }
+    if (!currentCertDir.renameTo(backupCertDir)) {
+      // Cannot rename current cert dir to the backup dir
+      rollbackBackupDir(currentKeyDir, currentCertDir, backupKeyDir,
+          backupCertDir);
+      throw new CertificateException("Failed to rename " +
+          currentCertDir.getAbsolutePath() +
+          " to " + backupCertDir.getAbsolutePath() + " during " +
+          "certificate renew.", RENEW_ERROR);
+    }
+
+    if (!newKeyDir.renameTo(currentKeyDir)) {
+      // Cannot rename new dir as the current dir
+      String msg = "Failed to rename " + newKeyDir.getAbsolutePath() +
+          " to " + currentKeyDir.getAbsolutePath() +
+          " during certificate renew.";
+      // rollback
+      rollbackBackupDir(currentKeyDir, currentCertDir, backupKeyDir,
+          backupCertDir);
+      throw new CertificateException(msg, RENEW_ERROR);
+    }
+
+    if (!newCertDir.renameTo(currentCertDir)) {
+      // Cannot rename new dir as the current dir
+      String msg = "Failed to rename " + newCertDir.getAbsolutePath() +
+          " to " + currentCertDir.getAbsolutePath() +
+          " during certificate renew.";
+      // delete new key directory
+      try {
+        Files.createDirectories(currentKeyDir.toPath());
+      } catch (IOException e) {
+        throw new CertificateException(msg, RENEW_ERROR);
+      }
+      // rollback
+      rollbackBackupDir(currentKeyDir, currentCertDir, backupKeyDir,
+          backupCertDir);
+      throw new CertificateException(msg, RENEW_ERROR);
+    }
+
+    getLogger().info("Successful renew key and certificate." +
+        " New certificate {}.", newCertSerialId);
+    return newCertSerialId;
+  }
+
+  private void rollbackBackupDir(File currentKeyDir, File currentCertDir,
+      File backupKeyDir, File backupCertDir) throws CertificateException {
+    // move backup dir back as current dir
+    if (!currentKeyDir.exists() && backupKeyDir.exists()) {

Review Comment:
   > This condition seems to be problematic, in the case when we already moved parts of the new key and cert to the current location. Let's assume we successfully move the newKeyDir to currentKeyDir, and then moving newCertDir to currentCertDir fails, and we call this method. In that case currentKeyDir.exists() will return true, which makes this expression to evaluate to false, and we do not do rollback, however we indeed should, as if we don't, and we report just a RENEW_ERROR, then things go on with the new keys and the old certificates directory as I think.
   > 
   
    currentKeyDir is deleted before it comes to rollbackBackupDir function, as you have suggested that there is no need to move currentKeyDir back t o newKeyDir. so it's deleted before rollbackBackupDir function.  What  rollbackBackupDir does is just as the name indicating, move backupDir back as currentDir.  So the case you mentioned actually will not happen. 



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