ozone icon indicating copy to clipboard operation
ozone copied to clipboard

HDDS-11040. Temporarily disable revoke/generate secret by name methods in REST

Open ivanzlenko opened this issue 1 year ago • 5 comments

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.

ivanzlenko avatar Jun 21 '24 05:06 ivanzlenko

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.

errose28 avatar Jun 21 '24 15:06 errose28

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?

ivanzlenko avatar Jun 21 '24 15:06 ivanzlenko

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.

errose28 avatar Jun 21 '24 15:06 errose28

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.

kerneltime avatar Jun 21 '24 17:06 kerneltime

@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

kerneltime avatar Jun 21 '24 19:06 kerneltime

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

adoroszlai avatar Jul 10 '24 11:07 adoroszlai

Thanks Attila for skipping the tests and updating the patch.

errose28 avatar Jul 10 '24 14:07 errose28