Copilot commented on code in PR #11095:
URL: https://github.com/apache/ozone/pull/11095#discussion_r3842309769
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/STSTokenIdentifier.java:
##########
@@ -146,6 +260,9 @@ public void fromProtoBuf(OMTokenProto token) throws
IOException {
setOwnerId(token.getOwner());
setExpiry(Instant.ofEpochMilli(token.getMaxDate()));
+ if (token.hasIssueDate()) {
+ this.creationTime = Instant.ofEpochMilli(token.getIssueDate());
+ }
Review Comment:
`toProtoBuf()` unconditionally dereferences `creationTime`, but
`fromProtoBuf()` only sets it when `issueDate` is present. This can cause NPEs
and/or make older STS tokens (minted before this change) impossible to
parse/round-trip safely. Consider always initializing `creationTime` in
`fromProtoBuf()` (eg, a sensible fallback when `issueDate` is absent) and/or
guarding `toProtoBuf()` so it never dereferences a null `creationTime`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/STSTokenIdentifier.java:
##########
@@ -123,6 +236,7 @@ public OMTokenProto toProtoBuf() {
builder
.setType(OMTokenProto.Type.S3_STS_TOKEN)
+ .setIssueDate(creationTime.toEpochMilli())
Review Comment:
`toProtoBuf()` unconditionally dereferences `creationTime`, but
`fromProtoBuf()` only sets it when `issueDate` is present. This can cause NPEs
and/or make older STS tokens (minted before this change) impossible to
parse/round-trip safely. Consider always initializing `creationTime` in
`fromProtoBuf()` (eg, a sensible fallback when `issueDate` is absent) and/or
guarding `toProtoBuf()` so it never dereferences a null `creationTime`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/STSSecurityUtil.java:
##########
@@ -180,6 +180,9 @@ static void
ensureEssentialFieldsArePresentInToken(STSTokenIdentifier stsTokenId
if (StringUtils.isEmpty(stsTokenIdentifier.getSecretAccessKey())) {
throw new SecretManager.InvalidToken("Invalid STS token -
secretAccessKey is null/empty");
}
+ if (stsTokenIdentifier.getCreationTime() == null) {
+ throw new SecretManager.InvalidToken("Invalid STS token - creationTime
is null");
+ }
Review Comment:
Requiring `creationTime` during validation will reject any previously issued
STS tokens that do not contain `issueDate` in their serialized form (eg, tokens
minted before this PR). If preserving in-flight token validity across rolling
upgrades is required, you likely need a backwards-compatible defaulting
strategy (eg, set `creationTime` during decode when `issueDate` is missing)
rather than hard-failing here.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSTSTokenRequest.java:
##########
@@ -61,48 +70,94 @@ public S3RevokeSTSTokenRequest(OMRequest omRequest) {
@Override
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
- final OzoneManagerProtocolProtos.RevokeSTSTokenRequest revokeReq =
- omRequest.getRevokeSTSTokenRequest();
+ final RevokeSTSTokenRequest revokeReq =
omRequest.getRevokeSTSTokenRequest();
+ validateRevokeRequestFields(revokeReq);
- // Get the original (long-lived) access key id from the session token
- // and enforce the same permission model that is used for S3 secret
+ // Use the original (long-lived) access key ID from the request and enforce
+ // the same permission model that is used for S3 secret
// operations (get/set/revoke). Only the owner of the original access
// key (i.e. the creator of the STS token) or an S3 / tenant admin is
allowed
// to revoke its temporary STS credentials.
- final String sessionToken = revokeReq.getSessionToken();
- final STSTokenIdentifier stsTokenIdentifier =
STSSecurityUtil.constructValidateAndDecryptSTSToken(
- sessionToken, ozoneManager.getSecretKeyClient(), CLOCK);
- final String originalAccessKeyId =
stsTokenIdentifier.getOriginalAccessKeyId();
+ final String originalAccessKeyId = revokeReq.getOriginalAccessKeyId();
final UserGroupInformation ugi =
S3SecretRequestHelper.getOrCreateUgi(originalAccessKeyId);
S3SecretRequestHelper.checkAccessIdSecretOpPermission(ozoneManager, ugi,
originalAccessKeyId);
- return omRequest;
+ if (!ozoneManager.getS3SecretManager().hasS3Secret(originalAccessKeyId)) {
+ throw new OMException("originalAccessKeyId does not exist: " +
originalAccessKeyId, INVALID_REQUEST);
+ }
+
+ final long revocationTimeMillis = CLOCK.millis();
+ final UpdateRevokeSTSTokenRequest updateRevokeSTSTokenRequest =
UpdateRevokeSTSTokenRequest.newBuilder()
+ .setOriginalAccessKeyId(originalAccessKeyId)
+ .setRevocationTimeMillis(revocationTimeMillis)
+ .build();
+
+ return omRequest.toBuilder()
+ .setUpdateRevokeSTSTokenRequest(updateRevokeSTSTokenRequest)
+ .build();
}
@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
ExecutionContext context) {
final OMResponse.Builder omResponse =
OmResponseUtil.getOMResponseBuilder(getOmRequest());
+ IOException exception = null;
+ OMClientResponse omClientResponse;
+ String originalAccessKeyId = null;
+
+ try {
+ validateReplicatedRevokeRequestFields(getOmRequest());
+ final UpdateRevokeSTSTokenRequest updateRevokeSTSTokenRequest =
getOmRequest().getUpdateRevokeSTSTokenRequest();
+ originalAccessKeyId =
updateRevokeSTSTokenRequest.getOriginalAccessKeyId();
+ final long revocationTimeMillis =
updateRevokeSTSTokenRequest.getRevocationTimeMillis();
- final OzoneManagerProtocolProtos.RevokeSTSTokenRequest revokeReq =
getOmRequest().getRevokeSTSTokenRequest();
- final String sessionToken = revokeReq.getSessionToken();
+ // All actual DB mutations are done in the response's addToDBBatch().
+ omClientResponse = new S3RevokeSTSTokenResponse(originalAccessKeyId,
revocationTimeMillis, omResponse.build());
- // All actual DB mutations are done in the response's addToDBBatch().
- final OMClientResponse omClientResponse = new S3RevokeSTSTokenResponse(
- sessionToken, omResponse.build());
+ // Update the cache immediately so subsequent validation checks see the
revocation
+
ozoneManager.getMetadataManager().getS3RevokedStsTokenTable().addCacheEntry(
+ new CacheKey<>(originalAccessKeyId),
CacheValue.get(context.getIndex(), revocationTimeMillis));
+
+ LOG.info(
+ "Marked STS tokens as revoked for originalAccessKeyId={} with cutoff
time {}.",
+ originalAccessKeyId, revocationTimeMillis);
+ } catch (IOException ex) {
+ exception = ex;
+ omClientResponse = new S3RevokeSTSTokenResponse(null, 0L,
createErrorOMResponse(omResponse, ex));
+ }
// Audit log
final Map<String, String> auditMap = new HashMap<>();
final OzoneManagerProtocolProtos.UserInfo userInfo =
getOmRequest().getUserInfo();
auditMap.put(OzoneConsts.S3_REVOKESTSTOKEN_USER, userInfo.getUserName());
- markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage(
- OMAction.REVOKE_STS_TOKEN, auditMap, null, userInfo));
+ if (originalAccessKeyId != null) {
+ auditMap.put(OzoneConsts.S3_STS_ORIGINAL_ACCESS_KEY_ID,
originalAccessKeyId);
+ }
+ markForAudit(
+ ozoneManager.getAuditLogger(),
buildAuditMessage(OMAction.REVOKE_STS_TOKEN, auditMap, exception, userInfo));
+ return omClientResponse;
+ }
- // Update the cache immediately so subsequent validation checks see the
revocation
-
ozoneManager.getMetadataManager().getS3RevokedStsTokenTable().addCacheEntry(
- new CacheKey<>(sessionToken), CacheValue.get(context.getIndex(),
CLOCK.millis()));
+ private static void validateRevokeRequestFields(RevokeSTSTokenRequest
revokeReq) throws OMException {
+ final String originalAccessKeyId = revokeReq.getOriginalAccessKeyId();
+ if (StringUtils.isEmpty(originalAccessKeyId)) {
+ throw new OMException("originalAccessKeyId is required for STS token
revocation", INVALID_REQUEST);
+ }
+ if (originalAccessKeyId.length() >=
OzoneConsts.OZONE_MAXIMUM_ACCESS_ID_LENGTH) {
+ throw new OMException("originalAccessKeyId length is invalid: " +
originalAccessKeyId.length(), INVALID_REQUEST);
Review Comment:
The length validation error message is missing the permitted maximum, which
makes it hard to diagnose. Consider including the max allowed length (eg,
`...invalid: X (max Y)`), and ideally the constraint definition
(`OZONE_MAXIMUM_ACCESS_ID_LENGTH`).
##########
hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeSTSTokenHandler.java:
##########
@@ -29,18 +29,18 @@
/**
* Executes revocation of STS tokens.
*
- * <p>This command marks the specified STS token as revoked by adding it to
the OM's revoked STS token table.
- * Subsequent S3 requests using the same session token will be rejected once
the revocation
- * state has propagated.</p>
+ * <p>This command records a revocation cutoff for the given original access
key ID in the OM's
+ * revoked STS token table. Subsequent S3 requests using STS tokens created
before that cutoff
+ * will be rejected once the revocation state has propagated.</p>
*/
@Command(name = "revokeststoken",
- description = "Revoke S3 STS token for the given session token")
+ description = "Revoke S3 STS tokens for the given original access key ID")
public class RevokeSTSTokenHandler extends S3Handler {
- @Option(names = "-t",
+ @Option(names = {"-o", "--original-access-key-id"},
required = true,
- description = "STS session token")
- private String sessionToken;
+ description = "Original long-lived access key ID whose STS tokens should
be revoked")
+ private String originalAccessKeyId;
Review Comment:
This changes the CLI option from `-t` (session token) to `-o` (original
access key ID), which is a breaking CLI/API change for existing scripts. If
backward compatibility is important, consider keeping `-t` as a deprecated
alias and translating it client-side (eg, best-effort extraction of
`originalAccessKeyId` from the provided token) or at least emitting a targeted
error that guides users to the new flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
