fapifta commented on code in PR #3982:
URL: https://github.com/apache/ozone/pull/3982#discussion_r1046147697
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -1095,4 +1089,274 @@ 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(String certId)
+ throws CertificateException {
+ X509Certificate cert = getCertificate(certId);
+ Duration gracePeriod = securityConfig.getRenewalGracePeriod();
+ Date expireDate = cert.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);
+ }
+ }
+
+ public String renewAndStoreKeyAndCertificate(boolean force)
+ throws CertificateException {
+ if (isRenewing.compareAndSet(false, true)) {
+ try {
+ if (!force) {
+ synchronized (this) {
+ Preconditions.checkArgument(
+ timeBeforeExpiryGracePeriod(certSerialId).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);
+ } catch (IOException e) {
+ throw new CertificateException("Error while deleting " + newKeyPath +
+ " or " + newCertPath + " directories to cleanup certificate " +
+ " storage. ", e, RENEW_ERROR);
+ }
+
+ try {
+ Files.createDirectories(newKeyDir.toPath());
+ Files.createDirectories(newCertDir.toPath());
+ } catch (IOException e) {
+ throw new CertificateException("Error while creating " + newKeyPath +
+ " or " + newCertPath + " directories for certificate storage.",
+ e, RENEW_ERROR);
+ }
+
+ // cleanup backup directory
+ cleanBackupDir();
+
+ // 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 dnCertSerialId;
+ try {
+ CertificateSignRequest.Builder csrBuilder =
getCSRBuilder(newKeyPair);
+ dnCertSerialId = 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
+ rollbackDir(currentKeyDir, currentCertDir, newKeyDir, newCertDir,
+ backupKeyDir, backupCertDir, "step-1");
+ 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
+ rollbackDir(currentKeyDir, currentCertDir, newKeyDir, newCertDir,
+ backupKeyDir, backupCertDir, "step-2");
+ 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.";
+ // rollback
+ rollbackDir(currentKeyDir, currentCertDir, newKeyDir, newCertDir,
+ backupKeyDir, backupCertDir, "step-2");
+ throw new CertificateException(msg, RENEW_ERROR);
+ }
+
+ // Delete backup dir on next DN startup
+ getLogger().info("Successful renew key and certificate." +
+ " New certificate {}.", dnCertSerialId);
+ return dnCertSerialId;
+ } finally {
+ isRenewing.set(false);
+ }
+ }
+
+ throw new CertificateException("A renewAndStoreKeyAndCert process" +
+ " is running already.", RENEW_ERROR);
+ }
+
+ private void rollbackDir(File currentKeyDir, File currentCertDir,
+ File newKeyDir, File newCertDir, File backupKeyDir, File backupCertDir,
+ String step) throws CertificateException {
Review Comment:
The step parameter is a bit interesting for me here, and I would propose to
drop it the following way and for the following reasons:
We fail the whole process and throw an exception, when:
- if cleanup of the new directory fails
- if creating the new new directory fails
- if cleanup the backup directory fails
- if creating the keypair into the new directory fails
- if creating and signing the CSR fails
- if moving the current key dir to backup fails
Other than this there is a rollback called.
I would argue that as the whole process starts with removing the new dir if
exists, we do not need to move the current back to new during the rollback, we
just need to ensure that the backed up data is moved back to the actual
location where current data should be.
If we change to this approach, then there is one more thing we can do, after
the new data moved to the current location and successfully applied, we should
also remove the backup location.
So that the next run, we do not need to clean the backup location, as if it
is there, that would mean that a rollback has failed, or the last step
(removing the backup) failed when applying a new certificate, in this case we
can fail, and require a human check to get out of the situation.
I am not sure it is clear, but after the suggested changed the simplified
flow would be:
- start the process, clear new folder (just in case)
- assert that backup location does not exists, if it does exists than fail
and ask human help
- create the new new folder, key and CSR, and then sign the cert and save it
to the new dir
- move the current folder to backup location (if fails rollback and move
back what has been moved)
- move the new folder to current location (if fails, rollback and move back
the backup to current)
- clear the backup location (the current remains the only directory)
If during this process the rollback fails, then just log an error, next run
will fail on asserting that backup locations do not exists.
--
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]
