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

fapifta · 2023-06-30T11:54:18Z

What changes were proposed in this pull request?

During the rotation of the rootCA certificate, we need to update the cached PEM format representation of the CA certificates in Ozone Manager, so that via the ServiceInfoEx object we are able to provide those to the clients.
In this PR, I have separated a class called ServiceInfoProvider, into which I have pushed out all the relevant code.
It attaches a listener to the OM's certificate client that manages the polling for the rootCA certificate, and then notifies this provider to update its cached data.
OM every time creates a ServiceInfoEx object once it is requested via the OzoneManagerProtocol, so does the provider.

The provider is needed to separate away the logic and to be able to test the handling of the renew separately and effectively.

In order to utilize some pre-written utilities in the test for the Provider, I have moved the relevant methods from TestInterSCMProtocolService to a test utility class under hdds-common. With that I had to move the HDDSKeyGenerator utility class in the production code as well, so that it is available for the test utilities. Both of these are general utilities, so they are rightfully placed in hdds-common I believe.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-8605

How was this patch tested?

Added UT for the new functionality.

ChenSammi · 2023-06-30T12:10:55Z

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

I would suggest use "interval" instead of "frequency" here and all other places, to be consistent with existing other properties.

@Galsza I believe this comment is for your patch at #4961 ;)

Yes, that's a better word, I'm going to fix it

adoroszlai

Thanks @fapifta for the patch.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

adoroszlai · 2023-07-03T10:06:01Z

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

Since this is only used by ServiceInfoProvider, which already handles PEM encoding for the CA list, I think this method should be omitted.

Yes, this makes testing a bit more complex though I think you are right, a simpler API for the CertificateClient interface is more important, I haven't realized this one before, thank you for catching it.

There are a few more tests that need to be updated:

https://github.com/fapifta/hadoop-ozone/actions/runs/5459036651/jobs/9934647451#step:5:3792
https://github.com/fapifta/hadoop-ozone/actions/runs/5459036651/jobs/9934647503#step:5:3764
https://github.com/fapifta/hadoop-ozone/actions/runs/5459036651/jobs/9934647503#step:5:3810

Sorry for the trouble. :)

Yes, I have adjusted the code to overcome these.

...op-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestServiceInfoProvider.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ServiceInfoProvider.java

hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/security/x509/package-info.java

Galsza · 2023-07-05T10:52:30Z

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

This method could possibly used in other places, (ReloadingTrustManager is what I know by heart, but it's going to change with Sammi's change), could you check once we get there, if there are other places we could use this?

Actually after the new changes, I realized that this method is not needed anymore, and I should use the new methods that we have in the certificate client to get the list of rootCA certificates. I have reworked the relevant code, and related tests.

Galsza

Thanks @fapifta for working on this. I have left 2 comments, one of them is minor, but I'm curious about your opinion on the suggestion of avoiding the race condition between the client's cert renewal and the ServiceInfoProvider's. I hope I didn't misunderstand anything.

Galsza · 2023-07-05T12:36:30Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ServiceInfoProvider.java

I think there is a race condition here with the certificateClient saving its own new rootCaCertificate. On root ca rotation the client will schedule a task to send a CSR and only after receiving the signed certificate will it save and update the new RootCA/SubCA. However when the ServiceInfoProvider asks for it it still might not have reached that point and therefore would receive the old certificate data. (And this notification won't repeat because we already think that the handlers successfully rotated the root ca)

I have a suggestion which could resolve this issue: instead of registering this to the rootCaRotationListeners we could register it to just the CertificateNotation receivers. They will get notified each cert rotation including the client renewing their known root ca.

The thing is that, it is a race condition, but the problem is not the race...

The problem is more of a question instead, around what we want to do with 1.1 or older clients, as all 1.2 or newer client versions work based off of the caCertPemList instead of the caCertPem value.
So newer clients rightfully get the list of the old and the new rootCA certificate, and will work with the cluster fine during the rotation, whichever root of trust the certificates of cluster services have old or new.

But with older clients we have a similar tradeoff as we had once we have introduce SCM HA. With SCM HA old clients got the sub-CA certificate that OM has read from its filesystem as the signer of its certificate, and if the post 1.2 DN that the pre 1.2 DN trying to connect to has its certificate signed by the same cert, then the client was able to read from the DN. Now in our case, we again need a decision, either we give back the old root CA certificate, which is returned by the certClient at this point, or we return the new certificate.

Based on your question I am happy that I thought this through more, and I think we should return the new rootCA certificate here. With that, we will have a glitch in old clients, as we don't know how many DNs have already renewed their certificates with the new rootCA and its subCAs, but for sure the number will be all DNs eventually, while less and less DNs will use their old certificates. So I need to change this code accordingly. Thank you for pointing this out!

fapifta · 2023-07-09T22:11:21Z

I think all the review comments were addressed, @Galsza @ChenSammi @adoroszlai please take a look once you have the time.

I have rebased the patch onto the tip of the master branch, as it has all the related commits. During the process I squashed my commits that were added to address the review comments, but in the meantime, I think I have improved the readability of the code. Let me know if you see any further things to address.
Thank you!

Galsza · 2023-07-10T06:51:20Z

@fapifta Thank you Pifta, there are a few elegant solutions here that I could learn of. LGTM +1

ChenSammi · 2023-07-10T09:25:13Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ServiceInfoProvider.java

Please avoid call certClient.getAllRootCaCerts() twice.

It does not have any significant impact, the getter itself provides an unmodifiable set, and that is basically just a wrapper around the internal data structure inside the CertificateClient.
I am fine with both approach so I have updated the code.

ChenSammi · 2023-07-10T09:28:24Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ServiceInfoProvider.java

"certClient != null" will have the same effect as "new SecurityConfig(config).isSecurityEnabled()" and avoid create a new SecurityConfig object.

I realized that the configuration itself was needed just for this one. So instead I pass on a SecurityConfig object from OzoneManager directly. I know the null check is essentially the same effect, but this way the code is better expressing what it does, and why.

ChenSammi · 2023-07-10T09:33:49Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ServiceInfoProvider.java

@fapifta , I think we should not hide the exception.

Hiding this is not really a problem. The following logic is applied on the client side: if there are a list of CA certs, then the list is being used, if there is no element in the list or the list is null, then the sole caCertPEM is being used. This logic is there to ensure compatibility between the new (1.2+) client and an older server side, so we need to keep that semantic.

If any/all certificates in the list fails to translate to PEM format, then we have a second chance.

I have added an error level log message to sign that there is a problem, but chances are that this will never happen, as we get the in memory X509Certificate already from a PEM formatted string, so we should fail earlier if there is a problem with this conversion.

@fapifta , thanks for the explanation.
It cannot purely sure a checked type exception will not happen. Let's not make that assumption.
Besides, an originally empty caCertPEMList, and an empty caCertPEMList because of certificate convert failure are two different cases. We need to distinguish them. Not sure if the backward comparability handling on client side can cover them both. Even if it can, it's not a recommended behavior.

Looking at it once more I relaized, that this local optimization does not really mean a thing, as other methods might throw an exception and still fail early. So instead of returning null, the exception is re-thrown as runtimeexception.

ChenSammi · 2023-07-10T09:34:59Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ServiceInfoProvider.java

Please use the inline statement directly, instead of calling a one statement function, to save one time function call overhead.

JIT would inline the method call anyways, and probably it would do more. Though this note was useful because it made me check what I was doing, and realize that I have a way better solution to solve the readability problem I wanted to solve with the expiry method.

ChenSammi · 2023-07-10T09:38:00Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ServiceInfoProvider.java

info -> error

ChenSammi · 2023-07-10T09:43:13Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ServiceInfoProvider.java

Need a way to synchronize the fetch and update of caCertPem and caCertPermList.

The easiest seems to synchronize on this when the values are updated in onRootCAChange, and then assign these to variables in a synchronized block so that we can provide a copy to the ServiceInfoEx constructor, and we do not need to care about further synchronization if I think it correclty. Adjusted the code.

ChenSammi · 2023-07-10T10:52:33Z

Hi @fapifta , thanks for working on this. Could you add some file put/get operation in robot test, after the root certificate is rotated and first root certificate is expired? You can leverage test-root-ca-rotation.sh

Galsza

I've left one more comment for the createSelfSignedCert helper method.
(It came from copying it to my own fork temporarily and trying to use it. I've encountered the problem of trying to store multiple generated certs, but the id was the same)

Galsza · 2023-07-11T14:51:29Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/security/x509/CertificateTestUtils.java

This actually generates the same id for the certificate over and over again and hides it away from the invoker. I think for a utility method like this it would be better to generate a new id or explicitly specify that the same keypair will generate the same id.

As this is a utility method, I could design it only for the use case I have at hand. I am adjusting the documentation, but I think we should not overengineer it here, once someone needs some other functionality, we can add it at that point in time either via an overload or via a new parameter and adjusted code where it is used.

fapifta · 2023-07-13T01:44:16Z

Thank you for the additional reviews @ChenSammi @Galsza. I tried to address all your comments properly, if you can take an other look, I would be glad.

This PR in the current stage relies on #5050 and #5052 to be added, until that the ozonesecure and ozonesecure-ha tests are expected to fail.

fapifta · 2023-07-13T01:58:56Z

After I was able to merge #5050 and #5052 I have rebased this branch on master. Pending CI and secondary reviews.

ChenSammi · 2023-07-13T05:16:08Z

hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test-root-ca-rotation.sh


 #transfer leader to scm4.org
 execute_robot_test scm4.org -v "TARGET_SCM:scm4.org" scmha/scm-leader-transfer.robot
+execute_robot_test scm4.org -v PREFIX:"rootca" certrotation/root-ca-rotation-client-checks.robot


rootca -> rootca2

I checked the failed robot test. After leader is transferred to scm4.org, leader OM will fail to allocate block for key upload since the three OM doesn't know scm4.org. We can move this root-ca-rotation-client-checks.robot test ahead of leader transfer.

updates, thank you for checking into this.

ChenSammi · 2023-07-13T05:54:32Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/security/x509/CertificateTestUtils.java

+   */
+  public static X509Certificate createSelfSignedCert(
+      KeyPair keys, String commonName
+  ) throws Exception {


I would like to discuss the function declare code style here. It's better to keep the consistent style with other majority codes, putting as many as possible in one line,

public static X509Certificate createSelfSignedCert( KeyPair keys, String commonName) throws Exception {

I also updated the constructor of ServiceInfoProvider to conform this style.

ChenSammi · 2023-07-13T05:55:01Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/security/x509/CertificateTestUtils.java

+   */
+  public static X509Certificate createSelfSignedCert(
+      KeyPair keys, String commonName, Duration expiresIn
+  ) throws Exception {


same as above comment.

ChenSammi · 2023-07-13T06:18:25Z

@fapifta , thanks for update the patch. I have left four new comments, including the one under toPEMEncodedStringUnsafe.

fapifta · 2023-07-13T08:50:54Z

Thank you for the review, I have pushed changes to address your comments @ChenSammi, thank you for the continous reviews!

fapifta · 2023-07-13T11:26:20Z

Looking at it further after the change in ServiceInfoProvider.toPEMEncodedStringUnsafe, the method felt unnecessary, so I did some more code changes around that, and removed the method.
@ChenSammi please take a look once you have time.

Galsza · 2023-07-13T14:01:10Z

@fapifta thank you for the changes. They are looking good overall. There are a couple of CI failures, but they were successful on your branch as far as I can tell.

ChenSammi · 2023-07-13T15:22:12Z

@fapifta , I checked the failed UT, looks like it's because "hdds.x509.ca.rotation.enabled" default is false now, so RootCaRotationPoller is not instantiated, so does the failed acceptance test.



java.lang.NullPointerException
om_1        | 	at org.apache.hadoop.hdds.security.x509.certificate.client.DefaultCertificateClient.registerRootCARotationListener(DefaultCertificateClient.java:210)
om_1        | 	at org.apache.hadoop.ozone.om.ServiceInfoProvider.<init>(ServiceInfoProvider.java:103)
om_1        | 	at org.apache.hadoop.ozone.om.OzoneManager.<init>(OzoneManager.java:644)
om_1        | 	at org.apache.hadoop.ozone.om.OzoneManager.createOm(OzoneManager.java:751)
om_1        | 	at org.apache.hadoop.ozone.om.OzoneManagerStarter$OMStarterHelper.start(OzoneManagerStarter.java:189)
om_1        | 	at org.apache.hadoop.ozone.om.OzoneManagerStarter.startOm(OzoneManagerStarter.java:86)
om_1        | 	at org.apache.hadoop.ozone.om.OzoneManagerStarter.call(OzoneManagerStarter.java:74)
om_1        | 	at org.apache.hadoop.hdds.cli.GenericCli.call(GenericCli.java:38)
om_1        | 	at picocli.CommandLine.executeUserObject(CommandLine.java:1953)
om_1        | 	at picocli.CommandLine.access$1300(CommandLine.java:145)

…h the new rootCA

…A cert.

… the package.

…s the 4th root ca before checking client operations.

…view discussion, and then removed the method as well.

fapifta · 2023-07-13T16:29:05Z

Oh well... thank you @ChenSammi to take a look at, I am verifying the fix for this quickly on the failed tests, and push the fix.

…ion is not enabled.

ChenSammi · 2023-07-14T02:06:01Z

Thanks @fapifta , LGTM +1.

fapifta marked this pull request as draft June 30, 2023 11:54

fapifta requested review from ChenSammi and adoroszlai June 30, 2023 11:54

ChenSammi reviewed Jun 30, 2023

View reviewed changes

adoroszlai reviewed Jul 3, 2023

View reviewed changes

fapifta force-pushed the HDDS-8605 branch from f0be57f to 9fe3415 Compare July 3, 2023 23:15

Galsza reviewed Jul 5, 2023

View reviewed changes

fapifta force-pushed the HDDS-8605 branch 2 times, most recently from 5f7fa25 to aed245b Compare July 9, 2023 22:01

fapifta marked this pull request as ready for review July 9, 2023 22:01

fapifta added the security label Jul 9, 2023

ChenSammi reviewed Jul 10, 2023

View reviewed changes

Galsza suggested changes Jul 11, 2023

View reviewed changes

fapifta mentioned this pull request Jul 12, 2023

HDDS-8998. Shared mutable state causes rootca poller to miss updates. #5050

Merged

fapifta force-pushed the HDDS-8605 branch from eeb217e to 4feec8c Compare July 13, 2023 01:57

ChenSammi reviewed Jul 13, 2023

View reviewed changes

fapifta added 9 commits July 13, 2023 18:25

HDDS-8605. Implement the ability to update the ServiceInfo object wit…

5ec0d6f

…h the new rootCA

Address simple review comments.

527435a

Add client tests, to check if the client properly gets the new root C…

311582a

…A cert.

Adding/extending API doc to newly added classes.

df0d4de

Reduced the visibility of the class, as we should not need it outside…

ec9e526

… the package.

Address further review comments.

8a6e152

Fix container reference in ozonesecure-ha test. Check for all OMs see…

bff7281

…s the 4th root ca before checking client operations.

Clean some code after changes in toPEMEncodedStringUnsafe based on re…

5807781

…view discussion, and then removed the method as well.

Remove unused import.

4808327

Avoid registering any listener to the poller when the automatic rotat…

bbde3d8

…ion is not enabled.

fapifta force-pushed the HDDS-8605 branch from 9131a9e to bbde3d8 Compare July 13, 2023 16:39

ChenSammi merged commit 37d1335 into apache:master Jul 14, 2023

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

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

Uh oh!

Conversation

fapifta commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

ChenSammi Jun 30, 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!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

fapifta commented Jul 9, 2023

Uh oh!

Galsza commented Jul 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi Jul 10, 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!

ChenSammi Jul 10, 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!

Choose a reason for hiding this comment

Uh oh!

fapifta commented Jun 30, 2023 •

edited

Loading

ChenSammi Jun 30, 2023 •

edited

Loading

ChenSammi Jul 10, 2023 •

edited

Loading

ChenSammi Jul 10, 2023 •

edited

Loading

ChenSammi commented Jul 13, 2023 •

edited

Loading