Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDDS-11040. Disable REST endpoint for S3 secret manipulation by username #6839

Merged
merged 5 commits into from
Jul 10, 2024

Conversation

ivanzlenko
Copy link
Contributor

@ivanzlenko ivanzlenko commented Jun 21, 2024

What changes were proposed in this pull request?

Methods to generate and revoke secrets for other users via S3 Gateway were disabled.

What is the link to the Apache JIRA

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

How was this patch tested?

Patch tested manually. Since it is a temporary solution no tests added.

@errose28
Copy link
Contributor

Thanks @ivanzlenko I had noticed this issue earlier as well but was too busy to do a fix. I was trying to come up with a more elegant solution since it seemed people might want to use this, but disabling the whole thing works for me. There are a few other issues with the implementation as well that I think need to be fixed:

  • While having a user facing operation (revoke a user's own secret) exposed via S3 gateway makes sense, putting an admin only operation there doesn't really make sense. I think the deployment model being suggested here is that S3 gateway endpoints are published to a wider audience (for users), than core Ozone endpoints (for admins). In that case S3 gateway should not expose admin-only operations.

  • The change is backwards incompatible because it either masks or will be masked by any existing bucket called secret using path style requests. One solution is to put the operations under their own endpoint like admin, which we can add to later if needed. We would probably need a config to enable or disable this. When enabled, a bucket called admin would not be allowed to be created, and enabling would fail if a bucket called admin already exists.

@ivanzlenko
Copy link
Contributor Author

ivanzlenko commented Jun 21, 2024

One solution is to put the operations under their own endpoint like admin

@errose28 secret endpoint is already separate from main S3G and can be disabled.
Or you talking about completely separating S3G Secret from S3G itself?

@errose28
Copy link
Contributor

The issue with compatibility is if there is a bucket called secret, then it is unclear whether https://s3g/secret should access that bucket or the endpoint. We can add handling for this as I described above, but if we want to add another management operation to s3 gateway later, say foo, then we now have to do the same thing again for another bucket name foo in addition to secret. If we make one endpoint called management or something similar, then today we can have https://s3g/management/secret and at any later point we can add https://s3g/management/foo without compatibility concerns.

@kerneltime
Copy link
Contributor

We can merge this in to stop this feature for now. We should move in the direction listed by @errose28 to reserve the /management namespace to allow for new non S3 REST APIs to be exposed for management.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change only removes the ability for admins to change other users' secrets. I'm ok to merge this to get master to a cleaner state, but we probably want another follow up to either disable the /secret endpoint all together or implement /management/secret as described above before the next release to handle compatibility concerns.

We could also delete code while changes are in the works instead of commenting/erroring it out since git will preserve it and we won't have dead code left around indefinitely. That's a minor preference though and could be done in a follow up if the follow up fixes are going to take a while.

@kerneltime
Copy link
Contributor

@ivanzlenko Can you please address the test failure

Error:  Failures: 
Error:    TestSecretRevoke.testSecretRevokeWithUsername:103 
Wanted but not invoked:
objectStore.revokeS3Secret("test2");
-> at org.apache.hadoop.ozone.client.ObjectStore.revokeS3Secret(ObjectStore.java:228)
Actually, there were zero interactions with this mock.

Error:  Errors: 
Error:    TestSecretGenerate.testSecretGenerateWithUsername:119 NullPointer Cannot invok...
[INFO] 
Error:  Tests run: 245, Failures: 1, Errors: 1, Skipped: 0

@adoroszlai adoroszlai changed the title HDDS-11040. Temporary disable revoke/generate secret by name methods in REST HDDS-11040. Temporarily disable revoke/generate secret by name methods in REST Jun 21, 2024
@adoroszlai adoroszlai changed the title HDDS-11040. Temporarily disable revoke/generate secret by name methods in REST HDDS-11040. Disable REST endpoint for secret manipulation by username Jul 10, 2024
@adoroszlai adoroszlai changed the title HDDS-11040. Disable REST endpoint for secret manipulation by username HDDS-11040. Disable REST endpoint for S3 secret manipulation by username Jul 10, 2024
@adoroszlai adoroszlai merged commit 56ce591 into apache:master Jul 10, 2024
41 checks passed
@adoroszlai
Copy link
Contributor

Thanks @ivanzlenko for the patch, @errose28, @kerneltime for the review.

@errose28
Copy link
Contributor

Thanks Attila for skipping the tests and updating the patch.

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Aug 14, 2024
xichen01 pushed a commit to xichen01/ozone that referenced this pull request Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants