Skip to content

Conversation

@fmorg-git
Copy link
Contributor

@fmorg-git fmorg-git commented Jan 22, 2026

Please describe your PR in detail:

  • Refactor constants and validation methods from endpoint and backend OzoneManager to a shared location to reduce code duplication. For example, validating roleArn, duration, etc.
  • Further, certain validations were using regular expressions which perform poorly, so use more manual approaches instead.
  • Further it groups certain validation errors together, similar to AWS
  • This PR depends on HDDS-14420. [STS] Add audit logging to endpoint and OzoneManager for STS #9687

What is the link to the Apache JIRA

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

How was this patch tested?

unit tests and smoke tests

@fmorg-git fmorg-git marked this pull request as draft January 22, 2026 06:56
@fmorg-git fmorg-git marked this pull request as ready for review January 22, 2026 06:57
@fmorg-git fmorg-git marked this pull request as draft January 27, 2026 04:18
@ChenSammi ChenSammi added the sts Changes for Ozone's S3 Security Token Service label Jan 28, 2026
@fmorg-git fmorg-git force-pushed the HDDS-14472 branch 2 times, most recently from f54a718 to b17e2b9 Compare January 30, 2026 03:05
@fmorg-git fmorg-git marked this pull request as ready for review January 30, 2026 17:04
final int roleSessionNameLength = roleSessionName.length();
if (roleSessionNameLength < ASSUME_ROLE_SESSION_NAME_MIN_LENGTH ||
roleSessionNameLength > ASSUME_ROLE_SESSION_NAME_MAX_LENGTH) {
throw new OMException("Invalid RoleSessionName: must be " + ASSUME_ROLE_SESSION_NAME_MIN_LENGTH + "-" +
Copy link
Contributor

@sodonnel sodonnel Jan 30, 2026

Choose a reason for hiding this comment

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

Here we know the problem is that the length is outside bounds. In the loop below we know the length bounds are fine (as it passed this check), buts its checking characters. Should be adjust the error messages in both cases to make it more clear that the problem is length in this case and bad characters in the other case?

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

for (int i = 0; i < roleSessionNameLength; i++) {
final char c = roleSessionName.charAt(i);
if (!isRoleSessionNameChar(c)) {
throw new OMException("Invalid RoleSessionName: must be " + ASSUME_ROLE_SESSION_NAME_MIN_LENGTH + "-" +
Copy link
Contributor

Choose a reason for hiding this comment

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

In this error, we have the character which is at fault, so we could add it to the error message to make it a little bit more helpful "Invalid character '" + c + "' in RoleSessionName ..... "

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

Copy link
Contributor

@sodonnel sodonnel left a comment

Choose a reason for hiding this comment

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

I left a couple of minor comments, otherwise looks good.

@fmorg-git fmorg-git requested a review from sodonnel January 30, 2026 18:28
@sodonnel sodonnel merged commit 4b450b4 into apache:HDDS-13323-sts Jan 30, 2026
83 of 84 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.

3 participants