Skip to content

HDDS-16110. [STS] Update key in sts revocation table - #11030

Open
fmorg-git wants to merge 8 commits into
apache:HDDS-13323-stsfrom
fmorg-git:HDDS-16110
Open

HDDS-16110. [STS] Update key in sts revocation table #11030
fmorg-git wants to merge 8 commits into
apache:HDDS-13323-stsfrom
fmorg-git:HDDS-16110

Conversation

@fmorg-git

@fmorg-git fmorg-git commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Please describe your PR in detail:

  • Previously, the key for the STS revocation table was the sessionToken. It was noted that sessionToken is not audited, so it's difficult to determine what it is from logs without turning on trace logs to log all headers. Therefore, the revocation key is being updated to a concatenation of the temporaryAccessKeyId (which already was being audited) and the originalAccessKeyId (which will be added to the audit log as part of the PR).
  • As a side effect of testing this change (via TestAuditUtils), it was noticed certain inputs could cause the token parsing to fail that were not already covered, so this PR addresses that as well.
  • The PR is divided into reviewable commits

What is the link to the Apache JIRA

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

How was this patch tested?

  1. smoke tests and unit tests
  2. manually checking keys in revocation table and then lowering the cleanup threshold to ensure they were still being cleaned up:
bash-5.1$ OZONE_OPTS= ozone debug ldb scan --db=/data/metadata/om.db --column-family=s3RevokedStsTokenTable --with-keys --limit=100
{ "ASIA2QG9LEIN3Z02YY8KB17S|svc-iceberg-rest-catalog/s3g@EXAMPLE.COM": 1786922690084
, "ASIABBSLHGE2UVFQAQWBTKT5|svc-iceberg-rest-catalog/s3g@EXAMPLE.COM": 1786922683068
, "ASIAJDZTUVSR7QXI72CXIWW1|svc-iceberg-rest-catalog/s3g@EXAMPLE.COM": 1786922693689
, "ASIAN92LMK2RK15CIUOUZJQL|svc-iceberg-rest-catalog/s3g@EXAMPLE.COM": 1786922686527
 }
bash-5.1$ OZONE_OPTS= ozone debug ldb scan --db=/data/metadata/om.db --column-family=s3RevokedStsTokenTable --with-keys --limit=100
{  }
  1. manually checking S3g audit log to ensure originalAccessKeyId was added:
2026-08-16 23:30:41,877 | INFO  | S3GAudit | user=ASIAZJ3C5CP5QAQJ85V8C0MZ | ip=<redactedIp> | op=CREATE_BUCKET {"bucket":"[sts-bucket-aqievbhi]","originalAccessKeyId":"svc-iceberg-rest-catalog/s3g@EXAMPLE.COM","x-amz-request-id":"d756b407-431e-482b-baba-133946ee4757","x-amz-id-2":"5wC4dfBH"} | ret=SUCCESS | perf={} |  

@chihsuan chihsuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch! @fmorg-git I left a few comments. Otherwise, this looks good.

return omClientResponse;
}

private static void validateRevokeRequestFields(OzoneManagerProtocolProtos.RevokeSTSTokenRequest revokeReq)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the value comes directly from the request, allowing | could create a revoke-key collision across users. Would it make sense to validate tempAccessKeyId against its generated format (ASIA plus 20 [0-9A-Z] characters) and bound both field lengths??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch - updated

throw new SecretManager.InvalidToken("Failed to decode STS token string: " + e);
} catch (IOException | RuntimeException e) {
throw new SecretManager.InvalidToken(
"Failed to decode STS token string: " + e + " for encodedToken: " + encodedToken);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Including the full client-provided token here could make failed requests unusually noisy. Perhaps the token length, a short prefix, or a hash would provide enough debugging context without writing the entire value to the response and logs. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated (sorry I forgot why I left it out originally)

// created the token) so the audit trail is not limited to the opaque tempAccessKeyId.
final String originalAccessKeyId = AuditUtils.getStsOriginalAccessKeyId(s3Auth.getSessionToken());
if (originalAccessKeyId != null) {
auditMap.put("originalAccessKeyId", originalAccessKeyId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: this uses the literal "originalAccessKeyId" while the OM side goes through OzoneConsts.S3_REVOKESTSTOKEN_ORIGINAL_ACCESS_KEY_ID. Could this share a constant so the two audit logs can't drift?

Copy link
Copy Markdown
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
fmorg-git requested a review from chihsuan August 18, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants