HDDS-11041. Add admin request filter for S3 requests and UGI support for GrpcOmTransport

[devabhishekpal]

What changes were proposed in this pull request?

HDDS-11041. Add admin request filter for S3 requests and UGI support for GrpcOmTransport

Please describe your PR in detail:

• Currently the s3secret endpoints do not support checking for the user making the request. This causes issues like HDDS-11040 where any user can modify the secrets
• As a part of this PR we are adding a new filter to check if the user making the request is a part of Ozone Admins, if not we will not trigger the generation/revocation of secret
• We are also adding the passing of UserGroupInformation to the GrpcOmTransport while creating the proxy connection to allow accessing the user ticket similar to the Hadoop3OmTransport

What is the link to the Apache JIRA

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

How was this patch tested?

Patch was tested using unit tests

[adoroszlai]

@ivanzlenko please take a look

[ivanzlenko]

General question: do we have any new tests for this?

[devabhishekpal]

Hi @ivanzlenko, new tests have not yet been added. We will need to modify existing tests to add user as an admin and generate request. New test will be having a random user try to generate the request and get a forbidden response.

Waiting for go on code change before modifying the tests.

[ivanzlenko]

@devabhishekpal new filter could and should be covered with unit tests at least.

[devabhishekpal]

Yes, these new cases will be added to check for the filter working

[ivanzlenko]

Thanks @devabhishekpal for tiding up code!

[devabhishekpal]

Hi @ivanzlenko, for the new filter it seems if we are adding tests to the TestSecret* testset, the filter is not triggered as the methods are being called directly and the filter doesn't take effect. We can go ahead with tests just for the filter like explicitly passing a mocked requestContext with invalid user and asserting that FORBIDDEN is thrown.

But that doesn't seem to be much use. Do you have any inputs regarding this? I tried manually testing it via setting up a ozonesecure docker environment, but we cannot send requests via POSTMAN as the kerberos auth is not present in the local machine and docker is isolated. It keeps giving 401 error as the user itself is unauthorized and doesn't reach the admin check step.

2024-10-21 15:14:07 2024-10-21 09:44:07,266 [qtp1078566479-63] WARN server.AuthenticationFilter: AuthenticationToken ignored: Unauthorized access

Inputs would be great in this regard @ivanzlenko @adoroszlai @myskov

[devabhishekpal]

Hi @ivanzlenko, @adoroszlai, @myskov, @vtutrinov could you take another look at the changes? Thanks.

• Added new robot test to verify the filter
• Refactored the common config related utils to OzoneAdmins class
• Re-enabled the skipped robot tests added by @ivanzlenko initially as a part of HDDS-11040
• Added unit tests for the new utils

[devabhishekpal]

Thanks for the review @adoroszlai , addressed the test failures.

[ivanzlenko]

@devabhishekpal from my point of view should be enough to have unit tests with mocks to verify that the contract for the filter will remain the same with any changes and extensively cover this functionality with integration tests. Unit tests should be simple and I don't think it will be a good to write something very comprehensive to mimic what could be done with integration tests. P.S. sorry for waiting for a response from me.

[devabhishekpal]

Thanks for the input @ivanzlenko. We re-enabled robot tests to verify this as well as new robot tests to verify the admin filter. New unit tests were also added for the utils. It would be great if you could take a look

[devabhishekpal]

Hi @ivanzlenko, completed with the necessary changes. CI is also green in fork. Could you take another look? Thanks a lot

[devabhishekpal]

Thanks for the quick fix @adoroszlai

[devabhishekpal]

Thanks for the reviews @ivanzlenko @adoroszlai @myskov @vtutrinov @sumitagrawl

[adoroszlai]

Thanks @devabhishekpal for the patch, @ivanzlenko, @myskov, @sumitagrawl, @vtutrinov for the reviews.