You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/08/31 16:16:36 UTC
[GitHub] [ozone] Pochatkin opened a new pull request, #3732: HDDS-7191: Separate prop for s3 admin
Pochatkin opened a new pull request, #3732:
URL: https://github.com/apache/ozone/pull/3732
## What changes were proposed in this pull request?
1. Created new props `ozone.s3.administrators` and `ozone.s3.administrators.groups`
2. In case when at least one of these props is defined all s3 shell operation can be executed only by one of defined user as admin. Each user still should have permission to generate keys for itself.
3. In case when these properties are empty admins should be taken from `ozone.administrators` or `ozone.administrators.groups`
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7191
## How was this patch tested?
Manual testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1238097496
@Pochatkin Can you please enable the build-branch workflow in your fork? Also, checkstyle check is quick, can be run locally simply by `hadoop-ozone/dev-support/checks/checkstyle.sh`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Pochatkin commented on pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
Pochatkin commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1238134566
@adoroszlai Sorry, missed checkstyle warnings in output because build was SUCCESS (this is unpredictable that checkstyle fails doesn't fail whole build). Workflow enabled. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Pochatkin commented on pull request #3732: HDDS-7191: Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
Pochatkin commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1233159855
@adoroszlai Could you please give approve for running workflows?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3732:
URL: https://github.com/apache/ozone/pull/3732#discussion_r963156019
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java:
##########
@@ -30,6 +40,54 @@ public final class OzoneConfigUtil {
private OzoneConfigUtil() {
}
+ /**
+ * Return list of OzoneAdministrators from config.
+ */
+ static Collection<String> getOzoneAdminsFromConfig(OzoneConfiguration conf)
+ throws IOException {
+ Collection<String> ozAdmins =
+ conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS);
+ String omSPN = UserGroupInformation.getCurrentUser().getShortUserName();
+ if (!ozAdmins.contains(omSPN)) {
+ ozAdmins.add(omSPN);
+ }
+ return ozAdmins;
+ }
+
+ /**
+ * Return list of s3 administrators prop from config.
Review Comment:
Let's put this in the javadoc as well:
```suggestion
* Return list of s3 administrators prop from config.
*
* If ozone.s3.administrators value is empty string or unset,
* defaults to ozone.administrators value.
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -329,11 +334,48 @@ public void testGetSecretOfAnotherUserAsNonAdmin() throws IOException {
+ "the permission to get bob's secret in this test case!");
}
+ @Test
+ public void testGetSecretOfAnotherUserAsOzoneAdmin() throws IOException {
+ // This effectively makes alice an admin.
Review Comment:
```suggestion
// This effectively makes alice an S3 admin.
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -329,11 +334,48 @@ public void testGetSecretOfAnotherUserAsNonAdmin() throws IOException {
+ "the permission to get bob's secret in this test case!");
}
+ @Test
+ public void testGetSecretOfAnotherUserAsOzoneAdmin() throws IOException {
+ // This effectively makes alice an admin.
+ when(ozoneManager.isS3Admin(ugiAlice)).thenReturn(true);
+
+
+ int txLogIndex = 1;
+
+ S3GetSecretRequest s3GetSecretRequest = new S3GetSecretRequest(
+ new S3GetSecretRequest(
+ s3GetSecretRequest(USER_CAROL)
+ ).preExecute(ozoneManager));
+
+ // Run validateAndUpdateCache
+ OMClientResponse omClientResponse =
+ s3GetSecretRequest.validateAndUpdateCache(ozoneManager,
+ txLogIndex, ozoneManagerDoubleBufferHelper);
+
+ // Check response type and cast
+ Assert.assertTrue(omClientResponse instanceof S3GetSecretResponse);
+ final S3GetSecretResponse s3GetSecretResponse =
+ (S3GetSecretResponse) omClientResponse;
+
+ // Check response
+ final S3SecretValue s3SecretValue = s3GetSecretResponse.getS3SecretValue();
+ Assert.assertEquals(USER_CAROL, s3SecretValue.getKerberosID());
+ final String awsSecret1 = s3SecretValue.getAwsSecret();
+ Assert.assertNotNull(awsSecret1);
+
+ final GetS3SecretResponse getS3SecretResponse =
+ s3GetSecretResponse.getOMResponse().getGetS3SecretResponse();
+ // The secret inside should be the same.
+ final S3Secret s3Secret1 = getS3SecretResponse.getS3Secret();
+ Assert.assertEquals(USER_CAROL, s3Secret1.getKerberosID());
+ Assert.assertEquals(awsSecret1, s3Secret1.getAwsSecret());
+ }
+
Review Comment:
Can we add a negative test case where e.g. `alice` is an Ozone admin, but **not** an S3 admin, thus can't do S3 secret operations (rejected) for others?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -196,7 +201,7 @@ private OMRequest s3GetSecretRequest(String userPrincipalStr) {
public void testGetSecretOfAnotherUserAsAdmin() throws IOException {
// This effectively makes alice an admin.
Review Comment:
```suggestion
// This effectively makes alice an S3 admin.
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -329,11 +334,48 @@ public void testGetSecretOfAnotherUserAsNonAdmin() throws IOException {
+ "the permission to get bob's secret in this test case!");
}
+ @Test
+ public void testGetSecretOfAnotherUserAsOzoneAdmin() throws IOException {
+ // This effectively makes alice an admin.
+ when(ozoneManager.isS3Admin(ugiAlice)).thenReturn(true);
+
+
+ int txLogIndex = 1;
+
+ S3GetSecretRequest s3GetSecretRequest = new S3GetSecretRequest(
+ new S3GetSecretRequest(
+ s3GetSecretRequest(USER_CAROL)
+ ).preExecute(ozoneManager));
+
+ // Run validateAndUpdateCache
+ OMClientResponse omClientResponse =
+ s3GetSecretRequest.validateAndUpdateCache(ozoneManager,
+ txLogIndex, ozoneManagerDoubleBufferHelper);
+
+ // Check response type and cast
+ Assert.assertTrue(omClientResponse instanceof S3GetSecretResponse);
+ final S3GetSecretResponse s3GetSecretResponse =
+ (S3GetSecretResponse) omClientResponse;
+
+ // Check response
+ final S3SecretValue s3SecretValue = s3GetSecretResponse.getS3SecretValue();
+ Assert.assertEquals(USER_CAROL, s3SecretValue.getKerberosID());
+ final String awsSecret1 = s3SecretValue.getAwsSecret();
+ Assert.assertNotNull(awsSecret1);
+
+ final GetS3SecretResponse getS3SecretResponse =
+ s3GetSecretResponse.getOMResponse().getGetS3SecretResponse();
+ // The secret inside should be the same.
+ final S3Secret s3Secret1 = getS3SecretResponse.getS3Secret();
+ Assert.assertEquals(USER_CAROL, s3Secret1.getKerberosID());
+ Assert.assertEquals(awsSecret1, s3Secret1.getAwsSecret());
+ }
+
@Test
public void testGetSecretWithTenant() throws IOException {
// This effectively makes alice an admin.
Review Comment:
```suggestion
// This effectively makes alice an S3 admin.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3732: HDDS-7191: Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1233829296
We would need a robot test as well to make sure s3 admins and ozone admins can continue to retrieve the secret keys.
The change makes sense overall.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Pochatkin commented on pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
Pochatkin commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1236629533
@kerneltime Done. Could you please review this changes or tag anyone who can?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
adoroszlai merged PR #3732:
URL: https://github.com/apache/ozone/pull/3732
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1238147715
> this is unpredictable that checkstyle fails doesn't fail whole build
I think most developers run checkstyle via the script `hadoop-ozone/dev-support/checks/checkstyle.sh`, which fails on violation, and prints violations in a more useful way.
We can change the default config, though, to cover the case where `mvn checkstyle:check` is run manually:
https://github.com/apache/ozone/blob/3d3e9eaf00385f2d2b5050792e96092e07cc09d7/pom.xml#L2039
That should be fine, as `hadoop-ozone/dev-support/checks/checkstyle.sh` explicitly sets it:
https://github.com/apache/ozone/blob/3d3e9eaf00385f2d2b5050792e96092e07cc09d7/hadoop-ozone/dev-support/checks/checkstyle.sh#L24
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1239263967
Thanks @Pochatkin for iterating on the patch, @kerneltime, @smengcl for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] myskov commented on pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
myskov commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1236912769
@adoroszlai could you please take a look at the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Pochatkin commented on pull request #3732: HDDS-7191. Separate prop for s3 admin
Posted by GitBox <gi...@apache.org>.
Pochatkin commented on PR #3732:
URL: https://github.com/apache/ozone/pull/3732#issuecomment-1237968425
@smengcl Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org