HDDS-8164. Authorize secret key APIs #4597

duongkame · 2023-04-20T06:52:05Z

What changes were proposed in this pull request?

HDDS-7830 implements secret key APIs in SCMSecurityProtocol, which for now doesn't enforce authorization (any authenticated principal can call the API).

For secret key APIs, we want to limit the access to Ozone principals only, aka, Datanode and OM. The APIs should be moved to a separate protocol to enforce authorization separately.

What is the link to the Apache JIRA

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

How was this patch tested?

Integration tests.

kerneltime

Done with 1/4th of the code review, will wrap this up tomorrow.

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

kerneltime · 2023-05-04T06:19:29Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocol/SCMSecretKeyProtocolOm.java

Why this todo?

This config key is used in a lot of places (more than 25 usages), I want to avoid including those changes in this PR. Filed a ticket: HDDS-8540

kerneltime · 2023-05-04T06:21:48Z

...ds/framework/src/main/java/org/apache/hadoop/hdds/protocol/SCMSecretKeyProtocolDatanode.java

No ProtocolInfo annotation for this protocol?

It's a client-side protocol interface, and doesn't need a ProtocolInfo.

umamaheswararao · 2023-05-08T21:38:31Z

@kerneltime do you have further comments?
@ChenSammi and @fapifta do you want to take a look at it?

kerneltime

So far looks good, will re-look post rebase

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecretKeysApi.java

fapifta

Hi @duongkame thank you for your work on this one, so far I found three small things, and in general the code looks good, though there are some parts I would like to give one more review tomorrow.

fapifta · 2023-05-10T22:37:38Z

...dds/common/src/main/java/org/apache/hadoop/hdds/security/exception/SCMSecurityException.java

-    NOT_A_PRIMARY_SCM,
-    SECRET_KEY_NOT_ENABLED,
-    SECRET_KEY_NOT_INITIALIZED
+    NOT_A_PRIMARY_SCM


This error code seems to be only checked but it is not thrown... Do we still need it?

Looks like SCM never throws that error code. It can be deleted I guess, but we should do that on master.

Ahh... I see now...
Sorry, I thought it is newly added here, but it is just a punctuation change in the line... Ok, please ignore this earlier comment for this PR.

...src/main/java/org/apache/hadoop/hdds/protocolPB/SecretKeyProtocolClientSideTranslatorPB.java

jojochuang · 2023-05-12T05:22:34Z

PR makes sense to me. Will this work with old clients?
Also there needs to be a user doc for setting up a secure (properly authorized) system.

duongkame · 2023-05-12T05:38:59Z

Thanks for looking at this, @jojochuang

Will this work with old clients?

Fortunately, secret key APIs don't have old clients to deal with. The APIs are introduced in a feature branch that is not merged to master yet.

Also there needs to be a user doc for setting up a secure (properly authorized) system.

Yes, we need something like this. I suppose a doc of config changes is also a part of the process to merge the feature branch to master.

fapifta · 2023-05-12T13:22:25Z

@jojochuang as this will be in internal protocol, that is not used by the clients, and as we do not support rolling upgrade, there should not be compatibility problems with it.

@duongkame thank you for your patience, I took a second look, I think we are good to go.
+1

umamaheswararao · 2023-05-12T19:46:24Z

could you approve the pr?

jojochuang · 2023-05-12T22:31:27Z

Merged. Thanks all for the review and offering the patch!

* master: (73 commits) HDDS-8587. Test that CertificateClient can store multiple rootCA certificates (apache#4852) HDDS-8801. ReplicationManager: Add metric to count how often replication is throttled (apache#4864) HDDS-8477. Unit test for Snapdiff using tombstone entries (apache#4678) HDDS-7507. [Snapshot] Implement List Snapshot API Pagination (apache#4065) (apache#4861) HDDS-8373. Document that setquota doesn't accept decimals (apache#4856) HDDS-8779. Recon - Expose flag for enable/disable of heatmap. (apache#4845) HDDS-8677. Ozone admin OM CLI command for block tokens (apache#4760) HDDS-8164. Authorize secret key APIs (apache#4597) HDDS-7945. Integrate secret keys to SCM snapshot (apache#4549) HDDS-8003. E2E integration test cases for block tokens (apache#4547) HDDS-7831. Use symmetric secret key to sign and verify token (apache#4417) HDDS-7830. SCM API for OM and Datanode to get secret keys (apache#4345) HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM (apache#4194) HDDS-8679. Add dedicated, configurable thread pool for OM gRPC server (apache#4771) HDDS-8790. Split EC acceptance tests (apache#4855) HDDS-8714. TestScmHAFinalization: mark testFinalizationWithRestart as flaky, enable other test cases HDDS-8787. Reduce ozone sh calls in robot tests (apache#4854) HDDS-8774. Log allocation stack trace for leaked CodecBuffer (apache#4840) HDDS-8729. Add metric for count of blocks pending deletion on datanode (apache#4800) HDDS-8780. Leak of ManagedChannel in HASecurityUtils (apache#4850) ...

* tmp-dir-refactor: (73 commits) HDDS-8587. Test that CertificateClient can store multiple rootCA certificates (apache#4852) HDDS-8801. ReplicationManager: Add metric to count how often replication is throttled (apache#4864) HDDS-8477. Unit test for Snapdiff using tombstone entries (apache#4678) HDDS-7507. [Snapshot] Implement List Snapshot API Pagination (apache#4065) (apache#4861) HDDS-8373. Document that setquota doesn't accept decimals (apache#4856) HDDS-8779. Recon - Expose flag for enable/disable of heatmap. (apache#4845) HDDS-8677. Ozone admin OM CLI command for block tokens (apache#4760) HDDS-8164. Authorize secret key APIs (apache#4597) HDDS-7945. Integrate secret keys to SCM snapshot (apache#4549) HDDS-8003. E2E integration test cases for block tokens (apache#4547) HDDS-7831. Use symmetric secret key to sign and verify token (apache#4417) HDDS-7830. SCM API for OM and Datanode to get secret keys (apache#4345) HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM (apache#4194) HDDS-8679. Add dedicated, configurable thread pool for OM gRPC server (apache#4771) HDDS-8790. Split EC acceptance tests (apache#4855) HDDS-8714. TestScmHAFinalization: mark testFinalizationWithRestart as flaky, enable other test cases HDDS-8787. Reduce ozone sh calls in robot tests (apache#4854) HDDS-8774. Log allocation stack trace for leaked CodecBuffer (apache#4840) HDDS-8729. Add metric for count of blocks pending deletion on datanode (apache#4800) HDDS-8780. Leak of ManagedChannel in HASecurityUtils (apache#4850) ...

duongkame marked this pull request as ready for review April 20, 2023 21:02

kerneltime requested review from ChenSammi, fapifta and kerneltime April 24, 2023 16:13

kerneltime reviewed May 4, 2023

View reviewed changes

duongkame force-pushed the HDDS-8164 branch 2 times, most recently from eb3fdcd to 91d2903 Compare May 4, 2023 17:59

kerneltime reviewed May 9, 2023

View reviewed changes

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java Outdated Show resolved Hide resolved

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecretKeysApi.java Show resolved Hide resolved

duongkame force-pushed the HDDS-7733-Symmetric-Tokens branch from 783c90f to ba51dba Compare May 10, 2023 05:27

duongkame added 2 commits May 9, 2023 22:45

HDDS-8164. SCM: Authorize secret key APIs

057c1cc

Adapt to rebase.

acc6e99

duongkame force-pushed the HDDS-8164 branch from 28e9d8b to acc6e99 Compare May 10, 2023 07:03

duongkame added 2 commits May 10, 2023 00:36

correct proto info.

0665ebb

fix tests

4b62146

fapifta reviewed May 10, 2023

View reviewed changes

duongkame added 3 commits May 10, 2023 23:12

Update javadoc.

ff4a860

Correct new lines.

9ea7d76

Prevent system exit in integ test.

875e8c7

kerneltime requested a review from jojochuang May 11, 2023 21:34

jojochuang approved these changes May 12, 2023

View reviewed changes

jojochuang merged commit 2f0d930 into apache:HDDS-7733-Symmetric-Tokens May 12, 2023

duongkame added a commit to duongkame/ozone that referenced this pull request Jun 8, 2023

HDDS-8164. Authorize secret key APIs (apache#4597)

dec468a

duongkame added a commit that referenced this pull request Jun 8, 2023

HDDS-8164. Authorize secret key APIs (#4597)

5257f74

duongkame deleted the HDDS-8164 branch April 12, 2025 00:10

HDDS-8164. Authorize secret key APIs #4597

HDDS-8164. Authorize secret key APIs #4597

Uh oh!

Conversation

duongkame commented Apr 20, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

kerneltime left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kerneltime May 4, 2023

Choose a reason for hiding this comment

Uh oh!

duongkame May 4, 2023

Choose a reason for hiding this comment

Uh oh!

kerneltime May 4, 2023

Choose a reason for hiding this comment

Uh oh!

duongkame May 4, 2023

Choose a reason for hiding this comment

Uh oh!

umamaheswararao commented May 8, 2023

Uh oh!

kerneltime left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fapifta left a comment

Choose a reason for hiding this comment

Uh oh!

fapifta May 10, 2023

Choose a reason for hiding this comment

Uh oh!

duongkame May 11, 2023

Choose a reason for hiding this comment

Uh oh!

fapifta May 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jojochuang commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duongkame commented May 12, 2023

Uh oh!

fapifta commented May 12, 2023

Uh oh!

umamaheswararao commented May 12, 2023

Uh oh!

jojochuang commented May 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jojochuang commented May 12, 2023 •

edited

Loading