Skip to content

HDDS-10014. Fixed internal error on generating S3 secret #5887

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
adoroszlai merged 5 commits into from
Jan 21, 2024
Merged

HDDS-10014. Fixed internal error on generating S3 secret #5887

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9147e74
HDDS-10014. Fixed internal error on generating S3 secret
Dec 26, 2023
3e4dfa9
HDDS-10014. fixed review comments
Jan 2, 2024
0086789
Apply suggestions from code review
myskov Jan 2, 2024
ab962e7
HDDS-10014. fixed review comments
Jan 21, 2024
cd067da
Merge branch 'HDDS-10014_generate_secret_if_exists' of https://github…
Jan 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions hadoop-ozone/dist/src/main/smoketest/commonlib.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@ Get test user principal
[return] ${user}/${instance}@EXAMPLE.COM

Kinit HTTP user
Pass Execution If '${SECURITY_ENABLED}' == 'false' Skip in unsecure cluster
${principal} = Get test user principal HTTP
Wait Until Keyword Succeeds 2min 10sec Execute kinit -k -t /etc/security/keytabs/HTTP.keytab ${principal}

Kinit test user
Pass Execution If '${SECURITY_ENABLED}' == 'false' Skip in unsecure cluster
[arguments] ${user} ${keytab}
${TEST_USER} = Get test user principal ${user}
Set Suite Variable ${TEST_USER}
Expand Down
5 changes: 5 additions & 0 deletions hadoop-ozone/dist/src/main/smoketest/s3/commonawslib.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,8 @@ Verify Multipart Upload
${tmp} = Catenate @{files}
Execute cat ${tmp} > /tmp/original${random}
Compare files /tmp/original${random} /tmp/verify${random}

Revoke S3 secrets
Execute and Ignore Error ozone s3 revokesecret -y
Execute and Ignore Error ozone s3 revokesecret -y -u testuser

33 changes: 17 additions & 16 deletions hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,30 +21,31 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
Suite Setup Setup s3 tests
Default Tags no-bucket-type
Test Setup Run Keywords Kinit test user testuser testuser.keytab
Copy link
Contributor

@ivanzlenko ivanzlenko Jan 3, 2024

Choose a reason for hiding this comment

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

I think we should revoke secret for testuser as well, just in case.

Copy link
Contributor

@adoroszlai adoroszlai Jan 12, 2024

Choose a reason for hiding this comment

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

@ivanzlenko Revoke S3 secrets (called in the next line) does revoke for testuser, too, so this is fine.

... AND Revoke S3 secrets
Test Teardown Run Keyword Revoke S3 secrets

*** Variables ***
${ENDPOINT_URL} http://s3g:9878
${SECURITY_ENABLED} true

*** Test Cases ***

S3 Gateway Generate Secret
Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit HTTP user
Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled
${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret
IF '${SECURITY_ENABLED}' == 'true'
Should contain ${result} HTTP/1.1 200 OK ignore_case=True
Should Match Regexp ${result} <awsAccessKey>.*</awsAccessKey><awsSecret>.*</awsSecret>
ELSE
Should contain ${result} S3 Secret endpoint is disabled.
END
Should contain ${result} HTTP/1.1 200 OK ignore_case=True
Should Match Regexp ${result} <awsAccessKey>.*</awsAccessKey><awsSecret>.*</awsSecret>

S3 Gateway Secret Already Exists
Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled
[Setup] Setup v4 headers
${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret
Should contain ${result} HTTP/1.1 400 S3_SECRET_ALREADY_EXISTS ignore_case=True

S3 Gateway Generate Secret By Username
Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit test user testuser testuser.keytab
${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser2
IF '${SECURITY_ENABLED}' == 'true'
Should contain ${result} HTTP/1.1 200 OK ignore_case=True
Should Match Regexp ${result} <awsAccessKey>.*</awsAccessKey><awsSecret>.*</awsSecret>
ELSE
Should contain ${result} S3 Secret endpoint is disabled.
END
Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled
${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser
Copy link
Contributor

@adoroszlai adoroszlai Jan 12, 2024

Choose a reason for hiding this comment

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

Also here, can we keep testuser2?

Should contain ${result} HTTP/1.1 200 OK ignore_case=True
Should Match Regexp ${result} <awsAccessKey>.*</awsAccessKey><awsSecret>.*</awsSecret>
22 changes: 8 additions & 14 deletions hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
Suite Setup Setup s3 tests
Default Tags no-bucket-type
Test Setup Run Keyword Setup v4 headers
Copy link
Contributor

@ivanzlenko ivanzlenko Jan 3, 2024

Choose a reason for hiding this comment

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

Shouldn't we generate secret beforehand?



*** Variables ***
${ENDPOINT_URL} http://s3g:9878
Expand All @@ -31,19 +32,12 @@ ${SECURITY_ENABLED} true
*** Test Cases ***

S3 Gateway Revoke Secret
Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit HTTP user
Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled
${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret
Copy link
Contributor

@adoroszlai adoroszlai Jan 12, 2024

Choose a reason for hiding this comment

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

I think @ivanzlenko is right, [setup] to generate secret is missing here.

IF '${SECURITY_ENABLED}' == 'true'
Should contain ${result} HTTP/1.1 200 OK ignore_case=True
ELSE
Should contain ${result} S3 Secret endpoint is disabled.
END
Should contain ${result} HTTP/1.1 200 OK ignore_case=True

S3 Gateway Revoke Secret By Username
Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit test user testuser testuser.keytab
${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser2
IF '${SECURITY_ENABLED}' == 'true'
Should contain ${result} HTTP/1.1 200 OK ignore_case=True
ELSE
Should contain ${result} S3 Secret endpoint is disabled.
END
Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled
[Setup] Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser
Copy link
Contributor

@adoroszlai adoroszlai Jan 12, 2024

Choose a reason for hiding this comment

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

Also, I think it would be better if setup used CLI to generate the secret, not HTTP. (Similarly to how secretgenerate.robot uses CLI to revoke in setup/teardown.)

${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser
Copy link
Contributor

@adoroszlai adoroszlai Jan 12, 2024

Choose a reason for hiding this comment

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

Can we keep "by username" test case generate/revoke for testuser2? Since testuser is executing the test case, this verifies it works for other user.

Should contain ${result} HTTP/1.1 200 OK ignore_case=True
30 changes: 21 additions & 9 deletions .../s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import javax.ws.rs.core.Response;
import java.io.IOException;

import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
import static javax.ws.rs.core.Response.Status.NOT_FOUND;

/**
Expand All @@ -55,15 +56,26 @@ public Response generate(@PathParam("username") String username)
return generateInternal(username);
}

private Response generateInternal(@Nullable String username)
throws IOException {
S3SecretResponse s3SecretResponse = new S3SecretResponse();
S3SecretValue s3SecretValue = generateS3Secret(username);
s3SecretResponse.setAwsSecret(s3SecretValue.getAwsSecret());
s3SecretResponse.setAwsAccessKey(s3SecretValue.getAwsAccessKey());
AUDIT.logReadSuccess(buildAuditMessageForSuccess(
S3GAction.GENERATE_SECRET, getAuditParameters()));
return Response.ok(s3SecretResponse).build();
private Response generateInternal(@Nullable String username) throws IOException {
try {
S3SecretValue s3SecretValue = generateS3Secret(username);

S3SecretResponse s3SecretResponse = new S3SecretResponse();
s3SecretResponse.setAwsSecret(s3SecretValue.getAwsSecret());
s3SecretResponse.setAwsAccessKey(s3SecretValue.getAwsAccessKey());
AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
S3GAction.GENERATE_SECRET, getAuditParameters()));
return Response.ok(s3SecretResponse).build();
} catch (OMException e) {
AUDIT.logWriteFailure(buildAuditMessageForFailure(
S3GAction.GENERATE_SECRET, getAuditParameters(), e));
if (e.getResult() == OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS) {
return Response.status(BAD_REQUEST.getStatusCode(), e.getResult().toString()).build();
} else {
LOG.error("Can't execute get secret request: ", e);
return Response.serverError().build();
}
}
}

private S3SecretValue generateS3Secret(@Nullable String username)
Expand Down
18 changes: 18 additions & 0 deletions ...op-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.security.Principal;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.core.MultivaluedHashMap;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.SecurityContext;
import javax.ws.rs.core.UriInfo;

Expand All @@ -30,6 +31,7 @@
import org.apache.hadoop.ozone.client.OzoneClient;
import org.apache.hadoop.ozone.client.OzoneClientStub;
import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
import org.apache.hadoop.ozone.om.exceptions.OMException;
import org.apache.hadoop.ozone.om.helpers.S3SecretValue;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -38,6 +40,7 @@
import org.mockito.invocation.InvocationOnMock;
import org.mockito.junit.jupiter.MockitoExtension;

import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -96,6 +99,21 @@ void testSecretGenerate() throws IOException {
assertEquals(USER_NAME, response.getAwsAccessKey());
}

Copy link
Contributor

@ivanzlenko ivanzlenko Jan 3, 2024

Choose a reason for hiding this comment

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

Can we add a test to check 500 will be thrown for other exceptions?

Copy link
Contributor

@adoroszlai adoroszlai Jan 12, 2024

Choose a reason for hiding this comment

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

Can be done in follow-up if needed.

@Test
void testIfSecretAlreadyExists() throws IOException {
when(principal.getName()).thenReturn(USER_NAME);
when(securityContext.getUserPrincipal()).thenReturn(principal);
when(context.getSecurityContext()).thenReturn(securityContext);
when(proxy.getS3Secret(any())).thenThrow(new OMException("Secret already exists",
OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS));

Response response = endpoint.generate();

assertEquals(BAD_REQUEST.getStatusCode(), response.getStatus());
assertEquals(OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS.toString(),
response.getStatusInfo().getReasonPhrase());
}

@Test
void testSecretGenerateWithUsername() throws IOException {
S3SecretResponse response =
Expand Down