HDDS-16110. [STS] Update key in sts revocation table by fmorg-git · Pull Request #11030 · apache/ozone

fmorg-git · 2026-08-17T00:06:04Z

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?

smoke tests and unit tests
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
{  }

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

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

chihsuan · 2026-08-17T08:57:48Z

    return omClientResponse;
  }
+
+  private static void validateRevokeRequestFields(OzoneManagerProtocolProtos.RevokeSTSTokenRequest revokeReq)


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??

good catch - updated

chihsuan · 2026-08-17T09:00:13Z

-      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);


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?

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

chihsuan · 2026-08-17T09:01:28Z

+      // 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);


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?

chihsuan

Thanks for the update! @fmorg-git While taking another look, I noticed a couple of additional issues. Please see new inline comments for the details. Thanks.

chihsuan · 2026-08-18T14:39:30Z

+  public static final String S3_STS_ORIGINAL_ACCESS_KEY_ID = "originalAccessKeyId";
+  public static final String S3_STS_TEMP_ACCESS_KEY_ID = "tempAccessKeyId";
+  public static final String S3_REVOKESTSTOKEN_ORIGINAL_ACCESS_KEY_ID = S3_STS_ORIGINAL_ACCESS_KEY_ID;
+  public static final String S3_REVOKESTSTOKEN_TEMP_ACCESS_KEY_ID = S3_STS_TEMP_ACCESS_KEY_ID;


Just curious, do we need the two S3_REVOKESTSTOKEN_* aliases here?

these are no longer here after the redesign

chihsuan · 2026-08-18T14:43:22Z

+`tempAccessKeyId|originalAccessKeyId`. Because `tempAccessKeyId` is always `ASIA` followed by characters drawn only
+from `[0-9A-Z]`, it can never contain the `|` delimiter, so no escaping is needed even though `originalAccessKeyId`
+is unconstrained. In this way, specific STS tokens can be revoked as opposed
+to all tokens.  Furthermore,


Note: Furthermore, is left on its own short line after the rewrap, and there are two spaces before it. Could we improve this so it's easier to read?

this is no longer here after the redesign

chihsuan · 2026-08-18T15:04:11Z

-    final STSTokenIdentifier stsTokenIdentifier = STSSecurityUtil.constructValidateAndDecryptSTSToken(
-        sessionToken, ozoneManager.getSecretKeyClient(), CLOCK);
-    final String originalAccessKeyId = stsTokenIdentifier.getOriginalAccessKeyId();
+    final String originalAccessKeyId = revokeReq.getOriginalAccessKeyId();


I noticed that both tempAccessKeyId and originalAccessKeyId now come directly from the request. If originalAccessKeyId is mistyped, OM will store a different revocation key and still return success, while the token remains usable because authentication derives its key from the token itself.

Should we validate that the two IDs belong to the same issued token, or return a distinguishable result when we can't confirm the pairing?

this is no longer applicable after redesign. Only originalAccessKeyId is there and there is a check in S3RevokeSTSTokenRequest that the originalAccessKeyId is valid.

chihsuan · 2026-08-18T15:23:59Z

+    if (s3Auth != null) {
+      // For STS temporary credentials, record the originalAccessKeyId (the permanent principal that
+      // created the token) so the audit trail is not limited to the opaque tempAccessKeyId.
+      final String originalAccessKeyId = AuditUtils.getStsOriginalAccessKeyId(s3Auth.getSessionToken());


originalAccessKeyId is extracted from the client-provided session token before OM validates the token signature. Since the same path is also used for failure audit messages, a malformed or unauthenticated request could record an arbitrary identity in this field. Would it make sense to mark it as client-provided/unverified, or only include it after successful validation?

chihsuan reviewed Aug 17, 2026

View reviewed changes

fmorg-git requested a review from chihsuan August 18, 2026 01:29

chihsuan reviewed Aug 18, 2026

View reviewed changes

Fabian Morgan added 14 commits August 21, 2026 19:34

update design doc

53105d0

update proto and related classes

6b23a57

update rocksDb related clsses

44a44c3

update ozone cli

35ef9c8

update main revocation logic

200ca77

ensure originalAccessKeyId is added to audit log

12d9d99

update smoke tests

0ba0894

pr review updates for chihsuan

de474ad

redesign: update design doc

a16283d

redesign: update proto and related classes

e363be4

redesign: update rocksDb related classes

5b57ffe

redesign: update ozone cli

80144f9

redesign: update main revocation logic

0d3f36a

redesign: update smoke tests

df7fef7

fmorg-git force-pushed the HDDS-16110 branch from 5c6238f to df7fef7 Compare August 22, 2026 02:34

Uh oh!

Conversation

fmorg-git commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

chihsuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chihsuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fmorg-git commented Aug 17, 2026 •

edited

Loading