HDDS-13888. [STS] Introduce S3AssumeRoleRequest and S3AssumeRoleResponse #9276
Conversation
|
Can you rebase this one now that #9254 is committed please? |
fmorg-git
force-pushed
the
HDDS-13888
branch
from
9826a1a to
7b39729
Compare
|
|
||
| // Expected format: arn:aws:iam::123456789012:role/[optional path segments/]RoleName | ||
| if (!roleArn.startsWith("arn:aws:iam::")) { | ||
| throw new OMException("Invalid role ARN: " + roleArn, OMException.ResultCodes.INVALID_REQUEST); |
There was a problem hiding this comment.
Is it worth adding "does not start with ..." to the error message?
| // Split ARN into parts: arn:aws:iam::accountId:role/path/name | ||
| final String[] parts = roleArn.split(":", 6); | ||
| if (parts.length < 6 || !parts[5].startsWith("role/")) { | ||
| throw new OMException("Invalid role ARN: " + roleArn, OMException.ResultCodes.INVALID_REQUEST); |
There was a problem hiding this comment.
Similar to above, is it worth adding "unexpected field count" or something similar to this error?
There was a problem hiding this comment.
updated - I added more descriptive messages in other cases as well
| * arn:aws:iam::123456789012:role/path/RoleB -> RoleB | ||
| */ | ||
| @VisibleForTesting | ||
| static String validateAndExtractRoleNameFromArn( |
There was a problem hiding this comment.
It feels like the logic to validate and extract parts of the Arn could be a class of its own. However I am happy if you leave it as it is, but if we have other areas of the code that need to decode this in the future, or extra different fields, perhaps keep in mind that this can be extracted into a standalone class.
| ); | ||
|
|
||
| // TODO sts - generate a real STS token in a future PR that incorporates the components above | ||
| return originalAccessKeyId + ":" + roleArn + ":" + assumeRoleRequest.getDurationSeconds() + |
There was a problem hiding this comment.
Could use a String builder to avoid all the concatenations, but this is temporary code so leave it as it is for now.
|
|
||
| @Override | ||
| public void addToDBBatch(OMMetadataManager omMetadataManager, | ||
| BatchOperation batchOperation) throws IOException { |
There was a problem hiding this comment.
You missed reformatting this parameter to have 4 space indend.
|
Looks largely good. I left a few minor comments I thought of as I went through it. |
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
LGTM - thanks for making the changes. But there are javadoc failures in the CI:
Warning: Javadoc Warnings
Warning: /home/runner/work/ozone/ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/AwsRoleArnValidator.java:38: warning - invalid usage of tag >
Warning: /home/runner/work/ozone/ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/AwsRoleArnValidator.java:39: warning - invalid usage of tag >
If you could fix those then we can commit it. Thanks!
|
hi @sodonnel - thanks - I believe I fixed the Javadoc issue and the CI issue. Can you please merge if everything looks ok? And also would you be able to merge upstream master into the feature branch HDDS-13323-sts after this merge so it doesn't get too old? |
3 participants
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13888
How was this patch tested?
Unit tests