-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-14150. [STS] Connect STS Endpoint to Backend Processing #9654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| } | ||
|
|
||
| @Test | ||
| public void testAssumeRoleRejectedWhenStsEnabledAndNativeAuthorizerNotUsed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testAssumeRoleRejectedWhenStsEnabledAndNativeAuthorizerNotUsed ->
testAssumeRoleAllowedWhenStsEnabledAndNativeAuthorizerNotUsed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 21381a9
| if (queryParams == null) { | ||
| return null; | ||
| } | ||
| final String stsQueryParam = queryParams.getFirst("X-Amz-Security-Token"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since "X-Amz-Security-Token" is case insensitive, it's better to loop the queryParams, and use the compareToIgnoreCase to compare the parameter name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 21381a9
| Random random = new Random(); | ||
| for (int i = 0; i < length; i++) { | ||
| sb.append(chars.charAt(random.nextInt(chars.length()))); | ||
| final String requestId = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we return the requestId from OM, and add the requestId in assumeRole audit log?
| .build(); | ||
| } | ||
|
|
||
| final AssumeRoleResponseInfo responseInfo = getClient() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better we catch the exception from assumeRole() and generateAssumeRoleResponse(), wrap it as OS3Exception to return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like audit log is not supported in S3AssumeRoleRequest currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe in case of signature mismatch, we should catch OMException too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated - per internal communication, I identified that the endpoint was returning plain text instead of XML, so I revamped the error handling to use a new OSTSException that conforms to AWS XML structure. It distinguishes between signature mismatch and access denied similar to AWS bdb33a4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| final String accountId = parts[4]; | ||
| final String resource = parts[5]; // role/<name> | ||
|
|
||
| if (accountId == null || accountId.isEmpty() || resource == null || !resource.startsWith("role/") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can leverage Strings.isNullOrEmpty() for string null and empty check. Strings.isNullOrEmpty is wildly used in Ozone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 21381a9
|
I think we can remove all the appearance of |
Per internal communication, I noticed recently the endpoint was returning plain text for errors when it should have been returning XML. The |
|
This PR is getting a little big and unwieldy - I will close this and break it up into multiple PRs.
|
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14150
How was this patch tested?
unit tests and smoke testing