Skip to content

Conversation

@len548
Copy link
Contributor

@len548 len548 commented Nov 21, 2025

What changes were proposed in this pull request?

Please describe your PR in detail:

  • Introduced a new HTTP server on 9880 port
  • STS endpoint in the new server with mock return values
  • Modification in signature initialization to make it compatible with STS
    The current signature base creation method expects headers to include x-amz-content-sha256 as payload hash for s3. For STS request, it needs actual payload hash.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13345

How was this patch tested?

Unit test and modified existing tests

@ivandika3 ivandika3 added the sts Changes for Ozone's S3 Security Token Service label Nov 22, 2025
@sodonnel
Copy link
Contributor

sodonnel commented Dec 5, 2025

@fmorg-git Could you give this a review pass and if you are happy I can check it too?

byte[] body = readAllBytes(in);
String payloadHash = Hex.encode(MessageDigest.getInstance("SHA-256").digest(body));
context.setEntityStream(new ByteArrayInputStream(body));
return payloadHash;
Copy link

@fm-cdera fm-cdera Dec 6, 2025

Choose a reason for hiding this comment

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

it doesn't appear the previous getPayloadHash() method was reading all the bytes into memory - curious what is the reason we are doing that now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for STS. While S3 requests require x-amz-content-sha256 header as a payload hash while STS requests require hash of the original request body.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps there should be a reasonable limit set on the request body to protect against huge payloads. For example, would there ever be a valid reason the payload is larger than 8192 bytes? If not, then probably readAllBytes could have a check there, and the limit could likely be lower.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a check with the limit set to 32KB. This ensures sufficient buffer for the maximum theoretical size of the request body with required parameters. In the request body, Action, Version, RoleArn, RoleSessionName, and DurationSeconds are required, among which RoleArn can be ~25k bytes in the worse-case scenario based on AWS document. Other parameters add relatively minor overhead. So reasonable limit should be 32KB. But if we have other optional parameters support (e.g. Policy or Tag) , we should consider increasing this default size if needed.

}

private String getExpirationTime(int durationSeconds) {
Instant expiration = Instant.now().plusSeconds(durationSeconds);
Copy link

@fm-cdera fm-cdera Dec 6, 2025

Choose a reason for hiding this comment

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

I think this can use a Clock parameter instead of Instant.now() and we may need to have a unit test that uses TestClock to ensure the expiration on the return XML is set correctly. For example, you can have a private static final field with the value Clock.system(ZoneOffset.UTC); and pass the Clock into getExpirationTime method.

Copy link
Contributor

@fmorg-git fmorg-git Dec 11, 2025

Choose a reason for hiding this comment

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

actually, maybe this unit test isn't required here - looks like I have a similar one in TestS3AssumeRoleRequest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agreed with that. This method can be removed once getting AssumeRole response from OM to this endpoint is implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left TODO comments in the class for which methods can be removed once OM response is implemented in the class.

@Inject
private SignatureInfo signatureInfo;

protected static final AuditLogger AUDIT =
Copy link

Choose a reason for hiding this comment

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

nit: this can probably be all on one line. Also, is this AUDIT variable being used?

Copy link
Contributor Author

@len548 len548 Jan 9, 2026

Choose a reason for hiding this comment

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

Actually S3STSEndpoint class that inherits this class should use this to log to write success/failure message, similarly to s3 endpoint classes (e.g. BucketEndpoint). But since it does not affect this PR, a new Jira for this can be open after this is merged. For now, I will remove this variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will use this variable in a different PR to log in the audit.

* @param roleArn The ARN of the role to assume (for AssumeRole)
* @param roleSessionName Session name for the role (for AssumeRole)
* @param durationSeconds Duration of the token validity in seconds
* @param version AWS STS API version (should be "2011-06-15")
Copy link

Choose a reason for hiding this comment

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

does the spec require this to be 2011-06-15? if so, it doesn't appear it is being validated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@fmorg-git
Copy link
Contributor

hi @sodonnel I did a first pass review - I was logged in as @fm-cdera at the time.

Copy link
Contributor

@Tejaskriya Tejaskriya left a comment

Choose a reason for hiding this comment

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

Thanks @len548 mostly looks good to me, just a few minor suggestions below.
@sodonnel would you like to take a look?

<tag>OZONE, S3GATEWAY</tag>
<description>
The bind host for the S3 Gateway STS HTTP server.
If not set, the value of ozone.s3g.http-bind-host is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also mention the override aspect (other similar configs have this uniform description):

Suggested change
If not set, the value of ozone.s3g.http-bind-host is used.
If this optional address is set, it overrides only the hostname portion of
ozone.s3g.sts.http-address.
If not set, the value of ozone.s3g.http-bind-host is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed and updated.

<tag>OZONE, S3GATEWAY</tag>
<description>
The bind host for the S3 Gateway STS HTTPS server.
If not set, the value of ozone.s3g.https-bind-host is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly:

Suggested change
If not set, the value of ozone.s3g.https-bind-host is used.
If this optional address is set, it overrides only the hostname portion of
ozone.s3g.sts.https-address.
If not set, the value of ozone.s3g.https-bind-host is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Comment on lines 52 to 53
"<Code>" + errorCode + "</Code>" +
"<Message>" + errorMessage + "</Message>" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason for creating the strings for errorCode and message instead of using Response.Status.NOT_IMPLEMENTED itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it.

Comment on lines +68 to +70
private static final int DEFAULT_DURATION_SECONDS = 3600;
private static final int MAX_DURATION_SECONDS = 43200; // 12 hours
private static final int MIN_DURATION_SECONDS = 900; // 15 minutes
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we consider making these into configurations instead? Or universal constants (like in OzoneConst). The default can be configurable. The min and max can be fixed to AWS specifics if we want to strictly adhere to their design. (IMO having all of them as configs would be better)

cc: @fm-cdera @fmorg-git (not sure which account you are using)
I think in one of your PRs, I had seen similar constants getting created. To avoid having multiple constants mentioned like this can we find a better place to specify this?

Copy link
Contributor

@fmorg-git fmorg-git Dec 16, 2025

Choose a reason for hiding this comment

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

I originally thought the backend would only be doing the validations, so I have constants in S3AssumeRoleRequest. Since the endpoint will be doing some validations as well (per earlier discussion above #9343 (comment)), perhaps those constants in S3AssumeRoleRequest can be made public instead of private and be reused in the endpoint.

@fapifta
Copy link
Contributor

fapifta commented Jan 13, 2026

Based on what we have discussed with @len548 and @fmorg-git I think we are good to go and we can commit this PR. I reviewed the code and I am +1 to commit it.
Two things are remaining here, one is to add audit logging, for which I will create a new JIRA and we already agreed with @len548 that he will take it on and finish that part.
The minimum and maximum values that are addressed by @Tejaskriya we think they should not be configurable, as the AWS spec is pretty concrete and it defines these as non-configurable boundaries with a non-configurable default value, so we are safe to mimic that and we don't see the use of configurability for those values. @Tejaskriya let me know if you disagree but as this is something you mention as a possibility I assume we are good to go this way.
The unification of these constants is to be done by @fmorg-git as part of the integration taks that integrates this endpoint with the OM itself.

Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

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

I have created HDDS-14420 to track the addition of audit logging.
With that I think this PR is ready to merge once CI is green, as this is agains a feature branch, I am going to merge it to help unblock the integration for @fmorg-git.

@Tejaskriya if you think we should add configuration options for the duration constants, let's discuss this in a separate JIRA before we merge back to master, I hope my assumption is correct that you are ok without configurability if @fmorg-git pull these together while following up with the integration.

@fapifta fapifta merged commit bf1453d into apache:HDDS-13323-sts Jan 19, 2026
123 of 125 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sts Changes for Ozone's S3 Security Token Service

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants