HDDS-1796. Add admin access check for write opertaions in SCMClientProtocolServer and SCMBlockProtocolServer. #5203

devmadhuu · 2023-08-21T13:48:09Z

What changes were proposed in this pull request?

Most of the calls to SCM are only done by the namespace layer (OzoneManager), it makes sense to make all the client-facing APIs of SCM to do Admin check. SCM should only get calls from namespace service or from admin, it cannot get direct calls from a client.
allocateBlock, deleteBlock and sortDatanodes doesn't have Admin check now. If we do Admin check for getContainerWithPipeline, we should also do the same in allocateBlock, deleteBlock and sortDatanodes.

What is the link to the Apache JIRA

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

How was this patch tested?

This patch is tested with existing Junit test cases of all the updated client facing APIs of SCM.

…eck for admin access.

ChenSammi · 2023-08-23T09:29:06Z

...-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMBlockProtocolServer.java

  @Override
  public List<DatanodeDetails> sortDatanodes(List<String> nodes,
      String clientMachine) throws IOException {
+    scm.checkAdminAccess(getRemoteUser(), false);


sortDatanodes is kind of a read operation, it doesn't change the in memory or persist state of pipeline.

Thanks @ChenSammi for review. I understood and handled the comment. Pls re-review.

ChenSammi · 2023-08-23T09:31:30Z

@devmadhuu , the overall patch looks good.

…eck for admin access.

errose28 · 2023-08-23T17:33:57Z

Secure acceptance tests are failing, likely because OM user is not an SCM admin. What is the best way to resolve this?

ChenSammi · 2023-08-24T03:18:56Z

Good question. We have property "ozone.administrators" and "ozone.administrators.groups" for Ozone administrator configuration. There properties cover both OM and SCM. I didn't check how the unit is failed. In a real Ozone cluster, if OM and SCM are lauched by same user, then it will be fine. If different users are used to lauch OM and SCM, we requires end user to explictily include the user who launch OM in "ozone.administrators", or the user group in "ozone.administrators.groups", otherwise the request from OM will be rejected by SCM.

devmadhuu · 2023-08-24T14:33:56Z

Secure acceptance tests are failing, likely because OM user is not an SCM admin. What is the best way to resolve this?

@errose28 Thanks for pointing out, As per @ChenSammi suggested to have specified admin user under "ozone.administrators" config based on test case, it can be handled, but I have noticed that few robot acceptance test cases are expecting for permission denied and if I specify admin user , those failed with message "'[ ]' does not contain 'PERMISSION_DENIED'", but since we have only docker-config where "ozone.administrators" config can be added, so positive test cases passes, but negative test cases fails. Any suggestion how to handle this ?

ChenSammi · 2023-08-24T14:37:25Z

Secure acceptance tests are failing, likely because OM user is not an SCM admin. What is the best way to resolve this?

@errose28 Thanks for pointing out, As per @ChenSammi suggested to have specified admin user under "ozone.administrators" config based on test case, it can be handled, but I have noticed that few robot acceptance test cases are expecting for permission denied and if I specify admin user , those failed with message "'[ ]' does not contain 'PERMISSION_DENIED'", but since we have only docker-config where "ozone.administrators" config can be added, so positive test cases passes, but negative test cases fails. Any suggestion how to handle this ?

Is it available to use other users which are not included in "ozone.administrators" to run the test which expects a PERMISSION_DENIED?

devmadhuu · 2023-08-26T11:38:13Z

Secure acceptance tests are failing, likely because OM user is not an SCM admin. What is the best way to resolve this?

@errose28 Thanks for pointing out, As per @ChenSammi suggested to have specified admin user under "ozone.administrators" config based on test case, it can be handled, but I have noticed that few robot acceptance test cases are expecting for permission denied and if I specify admin user , those failed with message "'[ ]' does not contain 'PERMISSION_DENIED'", but since we have only docker-config where "ozone.administrators" config can be added, so positive test cases passes, but negative test cases fails. Any suggestion how to handle this ?

Is it available to use other users which are not included in "ozone.administrators" to run the test which expects a PERMISSION_DENIED?

I have resolved and able to successfully run all secure acceptance test cases now, however below 2 test cases related to aws s3g API is still failing.

errose28 · 2023-08-28T20:08:39Z

As per @ChenSammi suggested to have specified admin user under "ozone.administrators" config based on test case, it can be handled

Would OM admin be considered an SCM admin by default? I'm not sure we want this, but if we do not do it this way it would be a breaking change because all users would need to update their admin configs.

Also it looks like acceptance tests are still failing. @devmadhuu its probably better to convert this to a draft and debug the failures on your fork until CI is green.

ChenSammi · 2023-08-29T10:18:30Z

As per @ChenSammi suggested to have specified admin user under "ozone.administrators" config based on test case, it can be handled

Would OM admin be considered an SCM admin by default? I'm not sure we want this, but if we do not do it this way it would be a breaking change because all users would need to update their admin configs.

@errose28 , currently the "ozone.administrators" applies to both OM and SCM. There is no individual property to control OM or SCM admins. And both OM and SCM will add the user who launches the services as admin. Usually, the user who launches OM, will launches SCM too, so it will not be a general problem.
Before this PR, there is already admin check on getContainerWithPipeline. This PR add the check to other APIs, so the behavior is consistent.

devmadhuu · 2023-08-29T14:03:20Z

Should we handle other file integration tests failing in this PR as part of other JIRA/PR ?

For intermittent failures in integration tests of this PR, it will be handled as part of other PR, as many other integration tests fails intermittently.

If all is Ok in this PR, pls have a re-look.

@errose28 @ChenSammi

ChenSammi · 2023-08-30T04:59:51Z

Should we handle other file integration tests failing in this PR as part of other JIRA/PR ?

file integration test failing can be handled with a new JIRA.

devmadhuu · 2023-08-31T11:09:56Z

@ChenSammi Thank you for detailed review, As discussed, have removed checkAdminAccess for following SCM APIs:

queryNode
getReplicationManagerReport
getContainerWithPipelineBatch
getExistContainerWithPipelinesInBatch
sortDatanodes

Pls have a re-look

ChenSammi · 2023-09-06T06:45:26Z

...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java


  public List<DeletedBlocksTransactionInfo> getFailedDeletedBlockTxn(int count,
      long startTxId) throws IOException {
+    getScm().checkAdminAccess(getRemoteUser(), true);


This check can be removed too.

This check can be removed too.

Done.

ChenSammi · 2023-09-06T06:46:54Z

hadoop-ozone/dist/src/main/smoketest/basic/links.robot

 Can follow link with read access
    Execute             kdestroy
-    Run Keyword         Kinit test user             testuser2         testuser2.keytab
+    Run Keyword         Kinit test user             testuser          testuser.keytab


Is this still needed now?

Yes removed.

ChenSammi · 2023-09-06T06:47:22Z

hadoop-ozone/dist/src/main/smoketest/basic/ozone-shell-lib.robot

    ${result} =     Execute And Ignore Error         ozone sh key list ${protocol}${server}/${volume}/bb1
                    Should contain      ${result}    PERMISSION_DENIED
+    Execute         kdestroy
+    Run Keyword     Kinit test user     testuser2    testuser2.keytab


Is this still needed now?

devmadhuu · 2023-09-06T14:10:18Z

@ChenSammi Handled comments, pls review.

ChenSammi

Thanks @devmadhuu .

ashishkumar50 · 2023-09-28T08:15:05Z

@ChenSammi @devmadhuu, In real ozone secure cluster, If remoteUser is "om" and scmAdmins is having "scm", because of this allocateBlock fails. Do we need to change any configuration?
Whether allocateBlock, deleteBlock need this check?

HDDS-1796. SCMClientProtocolServer#getContainerWithPipeline should ch…

2cad7af

…eck for admin access.

umamaheswararao requested review from duongkame and kerneltime August 21, 2023 16:25

devmadhuu marked this pull request as ready for review August 22, 2023 05:09

ChenSammi reviewed Aug 23, 2023

View reviewed changes

HDDS-1796. SCMClientProtocolServer#getContainerWithPipeline should ch…

08ee455

…eck for admin access.

deveshsingh added 3 commits August 24, 2023 12:22

HDDS-1796. Fix for s3g secure acceptance tests failing.

ff4b93f

HDDS-1796. Fix for s3g secure acceptance tests failing.

24be0da

HDDS-1796. Fix for s3g secure acceptance tests failing.

b4f8d7e

deveshsingh added 5 commits August 24, 2023 21:43

HDDS-1796. Fix for s3g secure acceptance tests failing.

5a9a9e4

HDDS-1796. Fix for s3g secure acceptance tests failing.

b4ebf91

HDDS-1796. Fix for s3g secure acceptance tests failing.

a2af114

HDDS-1796. Fix for s3g secure acceptance tests failing.

adfa985

HDDS-1796. Fix for s3g secure acceptance tests failing.

e44f89f

deveshsingh added 2 commits August 28, 2023 14:19

HDDS-1796. Fix for s3g secure acceptance tests failing.

e656507

HDDS-1796. Fix for s3g secure acceptance tests failing.

8118063

ChenSammi marked this pull request as draft August 29, 2023 03:23

reset user in container.robot

9d55be2

ChenSammi marked this pull request as ready for review August 29, 2023 07:02

devmadhuu marked this pull request as draft August 29, 2023 12:59

devmadhuu marked this pull request as ready for review August 29, 2023 14:02

deveshsingh added 3 commits August 31, 2023 16:31

HDDS-1796. Removed checkAdminAccess for some SCM APIs.

78cb77b

Merge remote-tracking branch 'origin/master' into HDDS-1796

6a9ef65

Merge branch 'HDDS-1796' of github.com:devmadhuu/ozone into HDDS-1796

3c7ba63

HDDS-1796. Removed checkAdminAccess for some SCM APIs.

ee5fc9e

ChenSammi reviewed Sep 6, 2023

View reviewed changes

HDDS-1796. Removed checkAdminAccess for some SCM APIs.

6875544

ChenSammi approved these changes Sep 7, 2023

View reviewed changes

ChenSammi changed the title ~~HDDS-1796. SCMClientProtocolServer#getContainerWithPipeline should check for admin access~~ HDDS-1796. Add admin access check for write opertaions in SCMClientProtocolServer and SCMBlockProtocolServer. Sep 7, 2023

ChenSammi merged commit a31e438 into apache:master Sep 7, 2023

HDDS-1796. Add admin access check for write opertaions in SCMClientProtocolServer and SCMBlockProtocolServer. #5203

HDDS-1796. Add admin access check for write opertaions in SCMClientProtocolServer and SCMBlockProtocolServer. #5203

Uh oh!

Conversation

devmadhuu commented Aug 21, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

ChenSammi Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

devmadhuu Aug 23, 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!

ChenSammi commented Aug 23, 2023

Uh oh!

errose28 commented Aug 23, 2023

Uh oh!

ChenSammi commented Aug 24, 2023

Uh oh!

devmadhuu commented Aug 24, 2023

Uh oh!

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

Uh oh!

Uh oh!

devmadhuu commented Aug 26, 2023

Uh oh!

errose28 commented Aug 28, 2023

Uh oh!

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

Uh oh!

Uh oh!

devmadhuu commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChenSammi commented Aug 30, 2023

Uh oh!

devmadhuu commented Aug 31, 2023

Uh oh!

ChenSammi Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

devmadhuu Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

ChenSammi Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

devmadhuu Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

ChenSammi Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

devmadhuu Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

devmadhuu commented Sep 6, 2023

Uh oh!

ChenSammi left a comment

Choose a reason for hiding this comment

Uh oh!

ashishkumar50 commented Sep 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

devmadhuu Aug 23, 2023 •

edited

Loading

ChenSammi commented Aug 24, 2023 •

edited

Loading

ChenSammi commented Aug 29, 2023 •

edited

Loading

devmadhuu commented Aug 29, 2023 •

edited

Loading