You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Tejaskriya (via GitHub)" <gi...@apache.org> on 2023/04/05 09:54:24 UTC
[GitHub] [ozone] Tejaskriya opened a new pull request, #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Tejaskriya opened a new pull request, #4538:
URL: https://github.com/apache/ozone/pull/4538
## What changes were proposed in this pull request?
It's not a good security practice to get/query plain password of an existing user.
Getsecret request creates and returns the secret in plaintext only the first time. Any following requests are handled by throwing an exception and not displaying the secret.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7752
## How was this patch tested?
--in progress
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203198948
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java:
##########
@@ -705,18 +705,9 @@ public void testGetSetRevokeS3Secret() throws Exception {
OmTransportFactory.create(conf, ugi, null),
RandomStringUtils.randomAscii(5));
- //Creates a secret since it does not exist
+ // Creates a secret since it does not exist
S3SecretValue attempt1 = omClient.getS3Secret(username);
- //Fetches the secret from db since it was created in previous step
- S3SecretValue attempt2 = omClient.getS3Secret(username);
-
- //secret fetched on both attempts must be same
- assertEquals(attempt1.getAwsSecret(), attempt2.getAwsSecret());
-
- //access key fetched on both attempts must be same
- assertEquals(attempt1.getAwsAccessKey(), attempt2.getAwsAccessKey());
-
Review Comment:
Rather than simply removing the second `getS3Secret()`, we should confirm it now throws expected exception:
```java
// A second getS3Secret on the same username should throw exception
LambdaTestUtils.intercept(OMException.class,
"already exists",
() -> omClient.getS3Secret(username));
```
--
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] Tejaskriya commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1504667914
> If we do not return the secrets the second time, we need a way to reset the secret and re generate it as well?
Wouldn't revoking the secret and then getting a secret regenerate a new secret?
--
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] Tejaskriya commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1497265864
> @Tejaskriya please enable the `build-branch` workflow in [your fork](https://github.com/Tejaskriya/ozone/actions) to get CI feedback for your change.
I have enabled it now
--
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 a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1326489635
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -167,7 +168,19 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ final OMMultiTenantManager multiTenantManager =
+ ozoneManager.getMultiTenantManager();
+ if (multiTenantManager == null ||
+ multiTenantManager.getTenantForAccessID(accessId) == null ||
+ !multiTenantManager.getTenantForAccessID(accessId)
+ .isPresent()) {
+ // Access Id is not assigned to any tenant and
+ // Secret is found in S3SecretTable. No secret is returned.
+ throw new OMException("Secret for '" + accessId +
Review Comment:
It is not necessarily a flaw in the secret management. The retrieval of secrets is still protected by kerberos principal?
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203198948
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java:
##########
@@ -705,18 +705,9 @@ public void testGetSetRevokeS3Secret() throws Exception {
OmTransportFactory.create(conf, ugi, null),
RandomStringUtils.randomAscii(5));
- //Creates a secret since it does not exist
+ // Creates a secret since it does not exist
S3SecretValue attempt1 = omClient.getS3Secret(username);
- //Fetches the secret from db since it was created in previous step
- S3SecretValue attempt2 = omClient.getS3Secret(username);
-
- //secret fetched on both attempts must be same
- assertEquals(attempt1.getAwsSecret(), attempt2.getAwsSecret());
-
- //access key fetched on both attempts must be same
- assertEquals(attempt1.getAwsAccessKey(), attempt2.getAwsAccessKey());
-
Review Comment:
Rather than simply removing the second `getS3Secret()`, we should confirm it now throws expected exception:
```
// A second getS3Secret on the same username should throw exception
LambdaTestUtils.intercept(OMException.class,
"expected error message",
() -> omClient.getS3Secret(username));
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java:
##########
@@ -705,18 +705,9 @@ public void testGetSetRevokeS3Secret() throws Exception {
OmTransportFactory.create(conf, ugi, null),
RandomStringUtils.randomAscii(5));
- //Creates a secret since it does not exist
+ // Creates a secret since it does not exist
S3SecretValue attempt1 = omClient.getS3Secret(username);
- //Fetches the secret from db since it was created in previous step
- S3SecretValue attempt2 = omClient.getS3Secret(username);
-
- //secret fetched on both attempts must be same
- assertEquals(attempt1.getAwsSecret(), attempt2.getAwsSecret());
-
- //access key fetched on both attempts must be same
- assertEquals(attempt1.getAwsAccessKey(), attempt2.getAwsAccessKey());
-
Review Comment:
Rather than simply removing the second `getS3Secret()`, we should confirm it now throws expected exception:
```java
// A second getS3Secret on the same username should throw exception
LambdaTestUtils.intercept(OMException.class,
"expected error message",
() -> omClient.getS3Secret(username));
```
--
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] Tejaskriya commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1708535811
I have fixed all the test failures and the "Illegal import statement error" coming up in the checkstyle due the the lambda test by replacing it with try-catch blocks.
@ChenSammi @ashishkumar50 Could you please take another look at this and give approvals if the PR is alright?
Regarding the tenant get-secret command, I will handle any changes needed in a follow-up PR. This PR retains the behaviour of it as it is currently i.e., returning the secret any number of times.
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203219169
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -264,6 +265,51 @@ public void testGetSecretOfAnotherUserAsOzoneAdmin() throws IOException {
processSuccessSecretRequest(USER_ALICE, 2, false);
}
+ @Test
+ public void testGetOwnSecretTwice() throws IOException {
+
+ // This effectively makes alice an S3 Admin.
+ when(ozoneManager.isS3Admin(ugiAlice)).thenReturn(true);
+ String userPrincipalId = USER_ALICE;
+
+ S3GetSecretRequest s3GetSecretRequest =
+ new S3GetSecretRequest(
+ new S3GetSecretRequest(
+ s3GetSecretRequest(userPrincipalId)
+ ).preExecute(ozoneManager)
+ );
+ // Run validateAndUpdateCache for the first time
+ OMClientResponse omClientResponse1 =
+ s3GetSecretRequest.validateAndUpdateCache(ozoneManager,
+ 1, ozoneManagerDoubleBufferHelper);
+ // Check response type and cast
+ Assert.assertTrue(omClientResponse1 instanceof S3GetSecretResponse);
+ final S3GetSecretResponse s3GetSecretResponse1 =
+ (S3GetSecretResponse) omClientResponse1;
+ // Secret is returned the first time
+ final S3SecretValue s3SecretValue1 =
+ s3GetSecretResponse1.getS3SecretValue();
+ Assert.assertEquals(userPrincipalId, s3SecretValue1.getKerberosID());
+ final String awsSecret1 = s3SecretValue1.getAwsSecret();
+ Assert.assertNotNull(awsSecret1);
+
+ final GetS3SecretResponse getS3SecretResponse1 =
+ s3GetSecretResponse1.getOMResponse().getGetS3SecretResponse();
+ // The secret inside should be the same.
+ final S3Secret s3Secret2 = getS3SecretResponse1.getS3Secret();
+ Assert.assertEquals(userPrincipalId, s3Secret2.getKerberosID());
+
+ // Run validateAndUpdateCache for the second time
+ OMClientResponse omClientResponse2 =
+ s3GetSecretRequest.validateAndUpdateCache(ozoneManager,
+ 2, ozoneManagerDoubleBufferHelper);
+ // Check response type and cast
+ Assert.assertTrue(omClientResponse2 instanceof S3GetSecretResponse);
+ final S3GetSecretResponse s3GetSecretResponse2 =
+ (S3GetSecretResponse) omClientResponse2;
+ // no secret is returned as it is the second time
+ Assert.assertNull(s3GetSecretResponse2.getS3SecretValue());
+ }
Review Comment:
```suggestion
}
```
--
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 pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1689853668
> > The impact on tenant getsecret could be a deal-breaker. See:
> > [#4538 (comment)](https://github.com/apache/ozone/pull/4538#discussion_r1161453347)
> > The best course of action in this PR is to allow `tenant user getsecret` to behave as-is by checking whether the `accessId` belongs to a tenant. (And maybe tackle that later on in another PR if necessary @prashantpogde )
>
> IMO, it's preferred to keep the behaviour consistent across both `getsecret` requests. As tenant user getsecret always returns a pre-existing secret, can we remove TenantGetSecretHandler instead?
Problem is, removing `ozone tenant user getsecret` will break _part of_ the existing Ozone tenant user assign flow.
Here's how tenant user assignment is supposed to work right now:
1. Ozone (tenant) admin `alice` assign a new user `bob` to a tenant. **Secret is generated** for user `bob` in this tenant **already** at this point. Also see: https://ozone.apache.org/docs/1.3.0/feature/s3-tenant-commands.html#assign-a-user-to-a-tenant
```bash
bash-4.2$ ozone tenant user assign testuser --tenant=tenantone
export AWS_ACCESS_KEY_ID='tenantone$testuser'
export AWS_SECRET_ACCESS_KEY='<GENERATED_SECRET>'
```
2. If user `bob` is able to authenticate via Kerberos, he would do `kinit`, then use `ozone tenant user getsecret` command to get his tenant secret to be used in S3 Gateway.
As you can see, removing `ozone tenant user getsecret` prevents `bob` or any Kerberos user from getting his own S3 secret after an admin assigns him to the tenant.
If we remove it, the only option left would be to ask admin `alice` to send the secret to `bob` over some third-party channels (email / Slack / whatever).
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1165013512
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ // Found in S3SecretTable. No secret returned.
awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ return new S3GetSecretResponse(null,
+ ozoneManager.getS3SecretManager(),
+ createErrorOMResponse(omResponse, new
+ OMException(OMException.ResultCodes.INVALID_REQUEST)));
Review Comment:
Please add a new ResultCodes，say “S3_SECRET_ALREADY_EXIST”.
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1172137337
##########
hadoop-ozone/dist/src/main/smoketest/security/get-S3-secret.robot:
##########
@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+*** Settings ***
+Documentation Smoke test to start cluster with docker-compose environments.
+Library OperatingSystem
+Library String
+Library BuiltIn
+Resource ../commonlib.robot
+Test Timeout 2 minutes
+
+*** Keywords ***
+GetSecret success
+ Run Keyword Kinit test user testuser testuser.keytab
+ ${output}= Execute ozone s3 getsecret
+ Should contain ${output} awsAccessKey
+ Should contain ${output} awsSecret
+ Execute kdestroy
+
+GetSecret failure
+ Run Keyword Kinit test user testuser testuser.keytab
+ ${output2}= Execute and Ignore Error ozone s3 getsecret
+ Should not contain ${output2} awsAccessKey
+ Should not contain ${output2} awsSecret
+ Should contain ${output2} S3_SECRET_ALREADY_EXISTS
Review Comment:
bash-4.2$ ozone s3 getsecret --om-service-id=omservice
S3_SECRET_ALREADY_EXISTS Secret for 'testuser/scm@EXAMPLE.COM' already exists
Please don't show the S3_SECRET_ALREADY_EXISTS in the output. "Secret for 'testuser/scm@EXAMPLE.COM' already exists" is good enough.
--
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] ChenSammi commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1718747611
Thanks @Tejaskriya for the contribution, and others for the code 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] Tejaskriya commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1711459173
@ashishkumar50 removing the check causes a NullPointer issue which can be seen in the unit tests in TestS3GetSecretRequest. I think it's necessary to add the check `multiTenantManager.getTenantForAccessID(accessId) == null`
--
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 pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1694062230
> @smengcl , thanks for the explanation. I understand that "tenant user getsecret" brings convenience to users. And as we all know, security naturally brings in inconvenience. So if we want strong security, we have to pay the cost to lose some convenience.
>
> Back to the example flow you mentioned, besides the "admin alice to send the secret to bob over some third-party channels", bob can also call the "tenant user setsecret" to reset the secret to something he remember. For a new user, reset the password on the first login is a practice used in many systems. What do you think about this solution?
Yes that should do it for this use case.
cc @prashantpogde for a second opinion.
--
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] ashishkumar50 commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1161453347
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
- awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ // Found in S3SecretTable. No secret returned.
Review Comment:
There is an impact on TenantGetSecret after this change.
TenantGetSecret only meant for retrieving secret of existing user, which will become obsolete as it will always return error. So please consider removing this for tenant.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -258,6 +259,51 @@ public void testGetSecretOfAnotherUserAsOzoneAdmin() throws IOException {
processSuccessSecretRequest(USER_ALICE, 2, false);
}
+ @Test
+ public void testGetOwnSecretTwice() throws IOException {
+
+ // This effectively makes alice an S3 Admin.
+ when(ozoneManager.isS3Admin(ugiAlice)).thenReturn(true);
+ String userPrincipalId=USER_ALICE;
+
+ S3GetSecretRequest s3GetSecretRequest =
+ new S3GetSecretRequest(
+ new S3GetSecretRequest(
+ s3GetSecretRequest(userPrincipalId)
+ ).preExecute(ozoneManager)
+ );
+ // Run validateAndUpdateCache for the first time
+ OMClientResponse omClientResponse1 =
+ s3GetSecretRequest.validateAndUpdateCache(ozoneManager,
+ 1, ozoneManagerDoubleBufferHelper);
+ // Check response type and cast
+ Assert.assertTrue(omClientResponse1 instanceof S3GetSecretResponse);
+ final S3GetSecretResponse s3GetSecretResponse1 =
+ (S3GetSecretResponse) omClientResponse1;
+ //Secret is returned the first time
Review Comment:
Give space after "//" and correct in other places too.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -225,7 +225,8 @@ public void testGetOwnSecretAsNonAdmin() throws IOException {
final S3Secret s3Secret2 = processSuccessSecretRequest(
USER_ALICE, 2, false);
- Assert.assertEquals(s3Secret1.getAwsSecret(), s3Secret2.getAwsSecret());
+ //no secret is returned as secret already exists in the DB
Review Comment:
Give space after "//", and do run checkstyle script.
--
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] ChenSammi commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1515792586
Change the CLI getsecret command description from "Returns S3 secret for a user" to "Create S3 secret for a user", GetS3SecretHandler.java.
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1502006350
If we do not return the secrets the second time, we need a way to reset the secret and re generate it as well?
--
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] Tejaskriya commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1557184425
@ChenSammi @smengcl @adoroszlai
All the comments have been fixed and the private fork build has passed too.
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1165013512
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ // Found in S3SecretTable. No secret returned.
awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ return new S3GetSecretResponse(null,
+ ozoneManager.getS3SecretManager(),
+ createErrorOMResponse(omResponse, new
+ OMException(OMException.ResultCodes.INVALID_REQUEST)));
Review Comment:
1. Please add a new ResultCodes，say “S3_SECRET_ALREADY_EXIST”.
2. You can throw the OMException here instead of construct an S3GetSecretResponse directly.
3. the continuous indent is 4, instead of 8 in Ozone. You can set the indent in intellij and reformat all newly added code in this patch.
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1497254423
@Tejaskriya please enable the `build-branch` workflow in [your fork](https://github.com/Tejaskriya/ozone/actions) to get CI feedback for your change.
--
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] Tejaskriya commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1172252179
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -85,10 +85,6 @@ SetSecret Failure For Invalid Secret 2
${rc} ${output} = Run And Return Rc And Output ozone tenant user setsecret 'tenantone$testuser' --secret=short
Should contain ${output} Secret key length should be at least 8 characters
-GetSecret Success
Review Comment:
The test intends to test getsecret for tenant. Refer to this comment: https://github.com/apache/ozone/pull/4538#discussion_r1161453347. Theeffect of this change on Tenant getsecret- It makes it obsolete. Hence the test was removed.
--
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] Tejaskriya commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1209787755
##########
hadoop-ozone/dist/src/main/smoketest/s3/commonawslib.robot:
##########
@@ -72,7 +72,9 @@ Setup v4 headers
Run Keyword if '${SECURITY_ENABLED}' == 'false' Setup dummy credentials for S3
Setup secure v4 headers
- ${result} = Execute ozone s3 getsecret ${OM_HA_PARAM}
+ ${result} = Execute and Ignore error ozone s3 getsecret ${OM_HA_PARAM}
Review Comment:
Without `and ignore error` , the test fails immediately at the Execute line if the following command returns an error response. In order to be able to check the output returned, I have added `Execute and Ignore Error`. In the next line the output is verified. If it's an unexpected output then we are alarmed about the problem in the next line using the `should contain`.
--
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] Tejaskriya commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1211099091
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -85,10 +85,6 @@ SetSecret Failure For Invalid Secret 2
${rc} ${output} = Run And Return Rc And Output ozone tenant user setsecret 'tenantone$testuser' --secret=short
Should contain ${output} Secret key length should be at least 8 characters
-GetSecret Success
Review Comment:
I have added the corrected test again
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203213600
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java:
##########
@@ -496,12 +496,10 @@ public void testOzoneTenantBasicOperations() throws IOException {
checkOutput(err, "Assigned 'bob' to 'finance' with accessId"
+ " 'finance$bob'.\n", true);
- // Try user getsecret again after assignment, should succeed
+ // Try user getsecret again after assignment, should fail
executeHA(tenantShell, new String[] {
"user", "getsecret", "finance$bob"});
- checkOutput(out, "export AWS_ACCESS_KEY_ID='finance$bob'\n",
- false);
- checkOutput(err, "", true);
+ checkOutput(err, "Secret for 'finance$bob' already exists\n", true);
Review Comment:
```suggestion
checkOutput(out, "", true);
checkOutput(err, "Secret for 'finance$bob' already exists\n", true);
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java:
##########
@@ -496,12 +496,10 @@ public void testOzoneTenantBasicOperations() throws IOException {
checkOutput(err, "Assigned 'bob' to 'finance' with accessId"
+ " 'finance$bob'.\n", true);
- // Try user getsecret again after assignment, should succeed
+ // Try user getsecret again after assignment, should fail
executeHA(tenantShell, new String[] {
"user", "getsecret", "finance$bob"});
- checkOutput(out, "export AWS_ACCESS_KEY_ID='finance$bob'\n",
- false);
- checkOutput(err, "", true);
+ checkOutput(err, "Secret for 'finance$bob' already exists\n", true);
Review Comment:
Check stdout emptiness.
```suggestion
checkOutput(out, "", true);
checkOutput(err, "Secret for 'finance$bob' already exists\n", true);
```
--
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] ChenSammi commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1534273304
@Tejaskriya , could you rebase your patch to resolve the conflict?
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1164999528
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java:
##########
@@ -357,6 +357,18 @@ private void checkOutput(ByteArrayOutputStream stream, String stringToMatch,
stream.reset();
}
+ private void checkNotOutput(ByteArrayOutputStream stream, String stringToMatch,
Review Comment:
This function is not used.
--
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] ChenSammi commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1504851707
> If we do not return the secrets the second time, we need a way to reset the secret and re generate it as well?
Reset secret is supported in Ozone. Here is CLI,
"ozone s3 setsecret -u=<username> --secretKey=<secretKey>"
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203216316
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -223,15 +223,16 @@ public void testGetOwnSecretAsNonAdmin() throws IOException {
// This effectively makes alice a regular user.
when(ozoneManager.isS3Admin(ugiAlice)).thenReturn(false);
- final S3Secret s3Secret1 = processSuccessSecretRequest(
+ S3Secret s3Secret1 = processSuccessSecretRequest(
USER_ALICE, 1, true);
Review Comment:
```suggestion
Assert.assertNotNull(s3Secret1);
```
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1165024403
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ // Found in S3SecretTable. No secret returned.
awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ return new S3GetSecretResponse(null,
+ ozoneManager.getS3SecretManager(),
+ createErrorOMResponse(omResponse, new
+ OMException(OMException.ResultCodes.INVALID_REQUEST)));
Review Comment:
@Tejaskriya , thanks for working on this. Would you add a robot test for s3 secret under hadoop-ozone/dist/src/main/smoketest/security?
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1172122939
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -474,6 +474,8 @@ enum Status {
CONTAINS_SNAPSHOT = 88;
SSL_CONNECTION_FAILURE = 89;
+
+ S3_SECRET_ALREADY_EXISTS=90;
Review Comment:
one space around "=".
--
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] Tejaskriya commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1318211717
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
- awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ // Found in S3SecretTable. No secret returned.
Review Comment:
Currently, I have followed the suggestion in this [comment](https://github.com/apache/ozone/pull/4538#pullrequestreview-1440775126).
--
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] Tejaskriya commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1318211717
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
- awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ // Found in S3SecretTable. No secret returned.
Review Comment:
I have taken up the suggestion in this [comment](https://github.com/apache/ozone/pull/4538#pullrequestreview-1440775126).
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1696219256
We should put this behind a config option. We should not break existing CLI behavior to avoid app breakage.
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1308209911
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -167,7 +168,19 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ final OMMultiTenantManager multiTenantManager =
+ ozoneManager.getMultiTenantManager();
+ if (multiTenantManager == null ||
+ multiTenantManager.getTenantForAccessID(accessId) == null ||
+ !multiTenantManager.getTenantForAccessID(accessId)
+ .isPresent()) {
+ // Access Id is not assigned to any tenant and
+ // Secret is found in S3SecretTable. No secret is returned.
+ throw new OMException("Secret for '" + accessId +
Review Comment:
Hi @kerneltime , we all know it's important to keep the backward compatibility for functions. But this one, is a security flaw/issue in the secret management, which requires a fix/mitigation. So, I don't think we should provide such an config option.
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203180451
##########
hadoop-ozone/dist/src/main/smoketest/s3/commonawslib.robot:
##########
@@ -72,7 +72,9 @@ Setup v4 headers
Run Keyword if '${SECURITY_ENABLED}' == 'false' Setup dummy credentials for S3
Setup secure v4 headers
- ${result} = Execute ozone s3 getsecret ${OM_HA_PARAM}
+ ${result} = Execute and Ignore error ozone s3 getsecret ${OM_HA_PARAM}
Review Comment:
Is the `and Ignore error` part necessary? Shouldn't we allow it to fail to alarm us to any problems?
--
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] Tejaskriya commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1172252179
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -85,10 +85,6 @@ SetSecret Failure For Invalid Secret 2
${rc} ${output} = Run And Return Rc And Output ozone tenant user setsecret 'tenantone$testuser' --secret=short
Should contain ${output} Secret key length should be at least 8 characters
-GetSecret Success
Review Comment:
The test intends to test getsecret for tenant. Refer to this comment: https://github.com/apache/ozone/pull/4538#discussion_r1161453347. The effect of this change on Tenant getsecret- It makes it obsolete. Hence the test was removed.
--
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] Tejaskriya commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1170981099
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ // Found in S3SecretTable. No secret returned.
awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ return new S3GetSecretResponse(null,
+ ozoneManager.getS3SecretManager(),
+ createErrorOMResponse(omResponse, new
+ OMException(OMException.ResultCodes.INVALID_REQUEST)));
Review Comment:
Yes, I have made the changes and added a robot test too
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1172125897
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -85,10 +85,6 @@ SetSecret Failure For Invalid Secret 2
${rc} ${output} = Run And Return Rc And Output ozone tenant user setsecret 'tenantone$testuser' --secret=short
Should contain ${output} Secret key length should be at least 8 characters
-GetSecret Success
Review Comment:
Why this one is removed?
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-tenant.robot:
##########
@@ -85,10 +85,6 @@ SetSecret Failure For Invalid Secret 2
${rc} ${output} = Run And Return Rc And Output ozone tenant user setsecret 'tenantone$testuser' --secret=short
Should contain ${output} Secret key length should be at least 8 characters
-GetSecret Success
Review Comment:
Why this test is removed?
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1554595533
Thanks @Tejaskriya for continuing work on this. Secure acceptance tests are failing:
```
ozonefs-ofs-bucket :: Ozone FS tests
==============================================================================
List root | FAIL |
Parent suite setup failed:
255 != 0
...
```
https://github.com/apache/ozone/actions/runs/5022299438/jobs/9005968979?pr=4538#step:5:629
You can find the cause of the failure by downloading the _acceptance-secure_ [artifact](https://github.com/apache/ozone/suites/13007064980/artifacts/704412496) from the [Artifacts](https://github.com/apache/ozone/actions/runs/5022299438?pr=4538#artifacts) section of the CI run, then unzipping and opening `log.html`. Suite setup failed at:
```
Running command 'ozone sh bucket create fstest1/46033-bucket1-ofs --space-quota 1TB --layout FILE_SYSTEM_OPTIMIZED
${rc} = 255
${output} = org.apache.hadoop.security.AccessControlException: Client cannot authenticate via:[TOKEN, KERBEROS]
```
_acceptance (HA-secure)_ ([artifact](https://github.com/apache/ozone/suites/13007064980/artifacts/704412490)) is failing due to:
```
Running command 'ozone s3 getsecret --om-service-id=omservice 2>&1'.
${rc} = 255
${output} = S3_SECRET_ALREADY_EXISTS Secret for 'testuser/s3g@EXAMPLE.COM' already exists
```
--
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] Tejaskriya commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1689318441
> The impact on tenant getsecret could be a deal-breaker. See:
>
> [#4538 (comment)](https://github.com/apache/ozone/pull/4538#discussion_r1161453347)
>
> The best course of action in this PR is to allow `tenant user getsecret` to behave as-is by checking whether the `accessId` belongs to a tenant. (And maybe tackle that later on in another PR if necessary @prashantpogde )
IMO, it's preferred to keep the behaviour consistent across both `getsecret` requests. As tenant user getsecret always returns a pre-existing secret, can we remove TenantGetSecretHandler instead?
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1501493140
> > @Tejaskriya please enable the `build-branch` workflow in [your fork](https://github.com/Tejaskriya/ozone/actions) to get CI feedback for your change.
>
> I have enabled it now
Thanks. You can find results by clicking on green check or red x in the list of [commits in your branch](https://github.com/Tejaskriya/ozone/commits/HDDS-7752). It looks like some secure tests related to this API are failing:
```
GetSecret Success | FAIL |
255 != 0
```
https://github.com/Tejaskriya/ozone/actions/runs/4626535637/jobs/8184390229#step:5:284
```
s3-encrypted.Freon :: S3 gateway test with aws cli
==============================================================================
Run Freon S3BG | FAIL |
Parent suite setup failed:
255 != 0
```
https://github.com/Tejaskriya/ozone/actions/runs/4626535637/jobs/8184390729#step:5:353
```
[ERROR] Failures:
[ERROR] TestOzoneTenantShell.testAssignAdmin:424->checkOutput:343->checkOutput:363 expected:<[]> but was:<[dev
finance
research
]>
[ERROR] TestOzoneTenantShell.testListTenantUsers:766->checkOutput:343->checkOutput:363 expected:<[]> but was:<[dev
finance
research
]>
[ERROR] TestOzoneTenantShell.testOzoneTenantBasicOperations:502->checkOutput:343->checkOutput:365
[ERROR] TestOzoneTenantShell.testTenantSetSecret:813->checkOutput:343->checkOutput:365
[ERROR] Errors:
[ERROR] TestSecureOzoneCluster.testGetSetRevokeS3Secret:711 » OM secret value already ...
[INFO]
[ERROR] Tests run: 181, Failures: 4, Errors: 1, Skipped: 3
```
https://github.com/Tejaskriya/ozone/actions/runs/4626535637/jobs/8183912886#step:5:3873
Also checkstyle and findbugs report minor problems:
https://github.com/Tejaskriya/ozone/actions/runs/4626535637/jobs/8183911270#step:6:512
https://github.com/Tejaskriya/ozone/actions/runs/4626535637/jobs/8183911653#step:6:2283
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203180451
##########
hadoop-ozone/dist/src/main/smoketest/s3/commonawslib.robot:
##########
@@ -72,7 +72,9 @@ Setup v4 headers
Run Keyword if '${SECURITY_ENABLED}' == 'false' Setup dummy credentials for S3
Setup secure v4 headers
- ${result} = Execute ozone s3 getsecret ${OM_HA_PARAM}
+ ${result} = Execute and Ignore error ozone s3 getsecret ${OM_HA_PARAM}
Review Comment:
Is the `and Ignore error` part necessary? Shouldn't we allow it to fail to alarm us about any problems?
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203180451
##########
hadoop-ozone/dist/src/main/smoketest/s3/commonawslib.robot:
##########
@@ -72,7 +72,9 @@ Setup v4 headers
Run Keyword if '${SECURITY_ENABLED}' == 'false' Setup dummy credentials for S3
Setup secure v4 headers
- ${result} = Execute ozone s3 getsecret ${OM_HA_PARAM}
+ ${result} = Execute and Ignore error ozone s3 getsecret ${OM_HA_PARAM}
Review Comment:
Is the `and Ignore error` part necessary? Shouldn't we allow it to fail to alarm us about any problems?
Also I see `Execute and Ignore error` is being used elsewhere in this PR as well.
--
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] Tejaskriya commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1319346315
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -167,7 +168,19 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ final OMMultiTenantManager multiTenantManager =
+ ozoneManager.getMultiTenantManager();
+ if (multiTenantManager == null ||
+ multiTenantManager.getTenantForAccessID(accessId) == null ||
Review Comment:
I have removed it now, Thank you 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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1165013512
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ // Found in S3SecretTable. No secret returned.
awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ return new S3GetSecretResponse(null,
+ ozoneManager.getS3SecretManager(),
+ createErrorOMResponse(omResponse, new
+ OMException(OMException.ResultCodes.INVALID_REQUEST)));
Review Comment:
Please add a new ResultCodes，say “S3_SECRET_ALREADY_EXIST”. You can throw the OMException here instead of construct an S3GetSecretResponse directly.
--
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] ChenSammi commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1693002136
@smengcl , thanks for the explanation. I understand that "tenant user getsecret" can bring convenience to users. And as we all know, security naturally brings in inconvenience. So if we want strong security, we have to pay the cost to lose some convenience.
Back to the example flow you mentioned, besides the "admin alice to send the secret to bob over some third-party channels", bob can also call the "tenant user setsecret" to reset the secret to something he remember. For a new user, reset the password on the first login is a practice used in many systems. What do you think about this solution?
--
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 a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1307798052
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -167,7 +168,19 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ final OMMultiTenantManager multiTenantManager =
+ ozoneManager.getMultiTenantManager();
+ if (multiTenantManager == null ||
+ multiTenantManager.getTenantForAccessID(accessId) == null ||
+ !multiTenantManager.getTenantForAccessID(accessId)
+ .isPresent()) {
+ // Access Id is not assigned to any tenant and
+ // Secret is found in S3SecretTable. No secret is returned.
+ throw new OMException("Secret for '" + accessId +
Review Comment:
We should put this behind a config option. We should not break existing CLI behavior to avoid app breakage.
--
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] ashishkumar50 commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1318248736
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -167,7 +168,19 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ final OMMultiTenantManager multiTenantManager =
+ ozoneManager.getMultiTenantManager();
+ if (multiTenantManager == null ||
+ multiTenantManager.getTenantForAccessID(accessId) == null ||
Review Comment:
No need to check for `multiTenantManager.getTenantForAccessID(accessId) == null`.
--
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] ChenSammi merged pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #4538:
URL: https://github.com/apache/ozone/pull/4538
--
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 pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1507076201
Thanks @Tejaskriya for starting this jira. IMO it is reasonable to do this.
Can we amend the doc as well so users are informed of this change? i.e. add a note [here](https://ci-hadoop.apache.org/view/Hadoop%20Ozone/job/ozone-doc-master/lastSuccessfulBuild/artifact/hadoop-hdds/docs/public/interface/s3.html) stating that we don't allow CLI `getsecret` twice, unless users or admins revoke the existing secret.
Also some acceptance (robot) tests might need to be fixed as a result of this change.
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1172163570
##########
hadoop-ozone/dist/src/main/smoketest/security/get-S3-secret.robot:
##########
@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+*** Settings ***
+Documentation Smoke test to start cluster with docker-compose environments.
+Library OperatingSystem
+Library String
+Library BuiltIn
+Resource ../commonlib.robot
+Test Timeout 2 minutes
+
+*** Keywords ***
+GetSecret success
+ Run Keyword Kinit test user testuser testuser.keytab
+ ${output}= Execute ozone s3 getsecret
+ Should contain ${output} awsAccessKey
+ Should contain ${output} awsSecret
+ Execute kdestroy
+
+GetSecret failure
+ Run Keyword Kinit test user testuser testuser.keytab
+ ${output2}= Execute and Ignore Error ozone s3 getsecret
+ Should not contain ${output2} awsAccessKey
+ Should not contain ${output2} awsSecret
+ Should contain ${output2} S3_SECRET_ALREADY_EXISTS
Review Comment:
Suggest change the file name to S3-secret.robot. New s3 secret tests can be added in future in this robot test suites.
--
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] ChenSammi commented on a diff in pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1165013512
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
+ // Found in S3SecretTable. No secret returned.
awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ return new S3GetSecretResponse(null,
+ ozoneManager.getS3SecretManager(),
+ createErrorOMResponse(omResponse, new
+ OMException(OMException.ResultCodes.INVALID_REQUEST)));
Review Comment:
1. Please add a new ResultCodes，say “S3_SECRET_ALREADY_EXIST”.
2. You can throw the OMException here instead of construct an S3GetSecretResponse directly.
3. the continuous line indent is 4, instead of 8 in Ozone. You can set the indent in intellij and reformat all newly added code in this patch.
--
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] Tejaskriya commented on pull request #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #4538:
URL: https://github.com/apache/ozone/pull/4538#issuecomment-1520150868
> Thanks @Tejaskriya for starting this jira. IMO it is reasonable to do this.
>
> Can we amend the doc as well so users are informed of this change? i.e. add a note [here](https://ci-hadoop.apache.org/view/Hadoop%20Ozone/job/ozone-doc-master/lastSuccessfulBuild/artifact/hadoop-hdds/docs/public/interface/s3.html) stating that we don't allow CLI `getsecret` twice, unless users or admins revoke the existing secret. CLI handler exit code ideally would also need to be non-zero (if it is not already the case) for scriptability.
>
> Also some acceptance (robot) tests might need to be fixed as a result of this change.
I have amended the acceptance tests and added one too. I will raise a separate PR for the documentation change
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203211048
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java:
##########
@@ -733,14 +724,14 @@ public void testGetSetRevokeS3Secret() throws Exception {
S3SecretValue attempt3 = omClient.getS3Secret(username);
// secret should differ because it has been revoked previously
- assertNotEquals(attempt3.getAwsSecret(), attempt2.getAwsSecret());
+ assertNotEquals(attempt3.getAwsSecret(), attempt1.getAwsSecret());
// accessKey is still the same because it is derived from username
- assertEquals(attempt3.getAwsAccessKey(), attempt2.getAwsAccessKey());
+ assertEquals(attempt3.getAwsAccessKey(), attempt1.getAwsAccessKey());
// Admin can set secret for any user
- omClient.setS3Secret(username, secretKeySet);
- assertEquals(secretKeySet, omClient.getS3Secret(username).getAwsSecret());
+ S3SecretValue attempt4 = omClient.setS3Secret(username, secretKeySet);
+ assertEquals(secretKeySet, attempt4.getAwsSecret());
Review Comment:
Add `LambdaTestUtils.intercept()` check after this as well.
--
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 #4538: HDDS-7752. GetS3SecretRequest API should not return secret if secret of user already exists
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4538:
URL: https://github.com/apache/ozone/pull/4538#discussion_r1203208070
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java:
##########
@@ -168,9 +168,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
assignS3SecretValue = null;
}
} else {
- // Found in S3SecretTable.
- awsSecret.set(s3SecretValue.getAwsSecret());
- assignS3SecretValue = null;
+ // Found in S3SecretTable. No secret returned.
Review Comment:
@ashishkumar50 You are right. We need to check whether `accessId` belongs to a tenant user. If so, don't throw.
--
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