HDDS-8591. Create scheduler to check for new root ca certificates #4961

Galsza · 2023-06-22T16:31:31Z

What changes were proposed in this pull request?

After the SCMs finished their part of root ca rotation the clients also need to get the new root CA certificate. This is done through a simple polling mechanism that asks the SCMs for the root CAs and once it observes a change it invokes a consumer from the clients to notify them. Integrating this change is going to be done in later items.

What is the link to the Apache JIRA

HDDS-8591

How was this patch tested?

Added unit tests. Functional change is not yet observed in this patch as the polling is not actually integrated into anything yet.

Galsza · 2023-06-22T16:32:28Z

@ChenSammi, @fapifta please take a look at this patch

ashishkumar50

@Galsza, Thanks for working on this. Please find few comments.

ashishkumar50 · 2023-06-23T11:36:50Z

hadoop-hdds/common/src/main/resources/ozone-default.xml

    </description>
  </property>
+  <property>
+    <name>ozone.scm.security.service.address</name>


ozone.scm.security.service.address This is duplicate config, already there is one. Don't see used in the new code, may be by mistake added. Please remove it.

ashishkumar50 · 2023-06-23T11:37:35Z

.../main/java/org/apache/hadoop/hdds/security/x509/certificate/client/RootCaRotationPoller.java

+        executor.shutdownNow();
+      }
+      if (!executor.awaitTermination(5, TimeUnit.SECONDS)) {
+        LOG.error("Unable to shutdown state machine properly.");


Please correct log wrt cert and not state machine.

ashishkumar50 · 2023-06-23T11:38:20Z

.../main/java/org/apache/hadoop/hdds/security/x509/certificate/client/RootCaRotationPoller.java

+      if (!knownRootCerts.containsAll(scmRootCaCerts)) {
+        rootCaListConsumers.forEach(c -> c.accept(scmRootCaCerts));
+        knownRootCerts = new HashSet<>();
+        knownRootCerts.addAll(scmRootCaCerts);


Would be nice to have a log here, when there is a new cert?

ashishkumar50 · 2023-06-23T11:43:20Z

.../main/java/org/apache/hadoop/hdds/security/x509/certificate/client/RootCaRotationPoller.java

+          scmSecureClient.getAllRootCaCertificates();
+      List<X509Certificate> scmRootCaCerts =
+          OzoneSecurityUtil.convertToX509(pemEncodedRootCaList);
+      if (!knownRootCerts.containsAll(scmRootCaCerts)) {


Objects(X509Certificate) inside knownRootCerts and scmRootCaCerts are same when both have same content? Can you verify if it is fine. If not it will always enter into this condition.

Two certs are the same when their encoded content is the same. I think this should be good enough. I'm going to discuss this with @fapifta just to be sure and make a local test for it.

Galsza · 2023-06-26T12:17:28Z

Converted to draft for now because some additional retry functionality needs to be created for each consumer in the poller.

Galsza · 2023-06-27T16:21:29Z

@ashishkumar50 Please take another look!

ashishkumar50 · 2023-06-28T12:48:05Z

...st/java/org/apache/hadoop/hdds/security/x509/certificate/utils/TestRootCaRotationPoller.java

+    knownCerts.add(knownCert);
+    List<String> certsFromScm = new ArrayList<>();
+    certsFromScm.add(CertificateCodec.getPEMEncodedString(knownCert));
+    certsFromScm.add(CertificateCodec.getPEMEncodedString(newRootCa));


Can you have small test by just adding knowCert to ensure both cases are fine.

I'm not sure what you meant by this, but I have refined the first test case to use the knownCert instead of not using certs at all. I hope this fixes it.

Yes I meant the same. Thanks for updating it.

fapifta

Thank you @Galsza for the work on this one, as we discussed today, let's wait with merging this one until we can get #4943 in, and then adjust this and the subsequent ones.

+1 from my end.

ashishkumar50 · 2023-06-30T07:05:38Z

Thanks @Galsza, LGTM +1.

Galsza · 2023-06-30T13:07:24Z

Moving to draft until Sammi's patch is merged: #4943

Galsza · 2023-07-05T13:32:11Z

Rebased to Sammi's change, since #4943 is merged now

Galsza · 2023-07-05T13:35:52Z

@fapifta @adoroszlai @ChenSammi this pr is ready now, please take another look!

ChenSammi · 2023-07-05T14:23:53Z

.../main/java/org/apache/hadoop/hdds/security/x509/certificate/client/RootCaRotationPoller.java

+    return StringUtils.join(certs.stream()
+        .map(X509Certificate::getSerialNumber)
+        .map(BigInteger::toString)
+        .collect(Collectors.toList()), ", ");


We can use stream function to achieve the join, like this
certs.stream().map(X509Certificate::getSerialNumber).map(BigInteger::toString).collect(Collectors.joining(", "));

ChenSammi · 2023-07-05T14:27:18Z

.../main/java/org/apache/hadoop/hdds/security/x509/certificate/client/RootCaRotationPoller.java

+        LOG.error("Unable to shutdown root ca certificate rotation poller.");
+      }
+    } catch (InterruptedException e) {
+      LOG.error("Error attempting to shutdown.", e);


WARN instead of ERROR is more appropriate here, Line 129 too. Here is an example,

LOG.warn("{} couldn't be stopped gracefully", getClass().getSimpleName());

ChenSammi · 2023-07-05T14:33:31Z

.../main/java/org/apache/hadoop/hdds/security/x509/certificate/client/RootCaRotationPoller.java

+      }
+    } catch (InterruptedException e) {
+      LOG.error("Error attempting to shutdown.", e);
+      executor.shutdownNow();


We can avoid call the executor.shutdownNow() here, for shutdown is already called, and without the explicitly check isInterrupted() or any wait() like function, shutdownNow will have the same effect as shutdown.

ChenSammi · 2023-07-05T14:34:58Z

.../main/java/org/apache/hadoop/hdds/security/x509/certificate/client/RootCaRotationPoller.java

+        }
+      });
+    } catch (IOException e) {
+      LOG.error("Error while trying to rotate root ca certificate", e);


Error while trying to rotate root ca certificate
->
Error while trying to poll root ca certificates

ChenSammi · 2023-07-05T14:36:16Z

.../main/java/org/apache/hadoop/hdds/security/x509/certificate/client/RootCaRotationPoller.java

+    this.knownRootCerts = initiallyKnownRootCaCerts;
+    poller = Executors.newScheduledThreadPool(1,
+        new ThreadFactoryBuilder().setNameFormat(
+                "RootCaRotationPoller")


Better use "this.getClass().getSimpleName()" to avoid hard code class name in case it's changed later.

ChenSammi · 2023-07-05T14:37:12Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java

  public static final String HDDS_X509_CA_ROTATION_ACK_TIMEOUT_DEFAULT =
      "PT15M";
+  public static final String HDDS_X509_ROOTCA_CLIENT_POLLING_INTERVAL =
+      "hdds.x509.rootca.client.polling.interval";


I would suggest change the name to "hdds.x509.rootca.certificate.polling.interval".

ChenSammi · 2023-07-05T14:51:48Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/SecurityConfig.java

    return caAckTimeout;
  }

+  public Duration getRootCaClientPollingInterval() {


@Galsza , the client -> certificate change is missed in this SecurityConfig.java .

@ChenSammi thanks I've addressed it now

Okay, let's wait for the CI to pass.

ChenSammi

The last patch LGTM +1.

* master: (36 commits) HDDS-8990. Intermittent timeout waiting on datanode4 9856 to become available (apache#5039) Revert "HDDS-7750. Incorrect WRITE ACL check. (apache#4992)" HDDS-7750. Incorrect WRITE ACL check. (apache#4992) HDDS-8985. Intermittent timeout exiting safe mode in HA secure tests (apache#5033) HDDS-8593. Add RootCARotationPoller to CertClient (apache#5030) HDDS-7645. Kubernetes check should fail fast if cluster cannot start (apache#5028) HDDS-8981. TestRootedOzoneFileSystem runs out of disk space (apache#5029) HDDS-8592. Fetch and save all root certificates during service's certificate rotation. (apache#5025) HDDS-8981. Disable TestRootedOzoneFileSystem#testSafeMode HDDS-8591. Create scheduler to check for new root ca certificates (apache#4961) HDDS-8979. error validating kustomization.yaml (apache#5024) HDDS-8973. Ozone SCM HA should not allocates duplicate IDs when transferring leadership (apache#5018) HDDS-8970. Snapshot Diff should return path relative to bucket root (apache#5015) HDDS-8975. Clarify SCM HA auto-bootstrap doc (apache#5021) HDDS-8689. Rotate Root CA and Sub CA in SCM. (apache#4943) HDDS-8436. Support setSafeMode(), isFileClosed() FileSystem API (apache#4825) HDDS-8880. Intermittent fork timeout in TestOMRatisSnapshots (apache#5022) HDDS-8962. Ensure docker env is stopped (apache#5011) HDDS-7794. [snapshot] SnapshotDiff should throw better error messages for exception handling (apache#5007) HDDS-7922. [FSO] S3G folder support fso layout filestatus s3A compatibility (apache#4448) ...

ashishkumar50 reviewed Jun 23, 2023

View reviewed changes

Galsza marked this pull request as draft June 26, 2023 12:15

Galsza marked this pull request as ready for review June 27, 2023 16:17

ashishkumar50 reviewed Jun 28, 2023

View reviewed changes

This was referenced Jun 28, 2023

HDDS-8592. Prepare DefaultCertificateClient for Root CA Rotation #5000

Closed

HDDS-8593. Add RootCARotationPoller to CertClient #5001

Closed

fapifta reviewed Jun 29, 2023

View reviewed changes

Galsza marked this pull request as draft June 30, 2023 13:07

fapifta mentioned this pull request Jul 4, 2023

HDDS-8605. Implement the ability to update the ServiceInfo object with the new rootCA #5009

Merged

Galsza added 2 commits July 5, 2023 15:22

HDDS-8591 Create root ca rotation poller

ce518c3

HDDS-8591 Replace the phrase interval instead of frequency

a89ce73

Galsza force-pushed the HDDS-8591_root_ca_rotation_polling branch from d719380 to a89ce73 Compare July 5, 2023 13:27

HDDS-8591 Replace the phrase interval instead of frequency

a9296fc

Galsza marked this pull request as ready for review July 5, 2023 13:32

ChenSammi reviewed Jul 5, 2023

View reviewed changes

HDDS-8591 Address renew comments

546757a

ChenSammi reviewed Jul 5, 2023

View reviewed changes

HDDS-8591 Fix missing client -> certificate rename

859c2f9

trigger CI

4197544

ChenSammi approved these changes Jul 6, 2023

View reviewed changes

ChenSammi merged commit 343df6a into apache:master Jul 6, 2023

HDDS-8591. Create scheduler to check for new root ca certificates #4961

HDDS-8591. Create scheduler to check for new root ca certificates #4961

Uh oh!

Conversation

Galsza commented Jun 22, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Galsza commented Jun 22, 2023

Uh oh!

ashishkumar50 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Galsza commented Jun 26, 2023

Uh oh!

Galsza commented Jun 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fapifta left a comment

Choose a reason for hiding this comment

Uh oh!

ashishkumar50 commented Jun 30, 2023

Uh oh!

Galsza commented Jun 30, 2023

Uh oh!

Galsza commented Jul 5, 2023

Uh oh!

Galsza commented Jul 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ChenSammi Jul 5, 2023 •

edited

Loading