chihsuan commented on code in PR #11030:
URL: https://github.com/apache/ozone/pull/11030#discussion_r3795046165
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java:
##########
@@ -603,6 +603,14 @@ protected AuditMessage.Builder auditMessageFor(AuditAction
op) {
Map<String, String> auditMap = getAuditParameters();
auditMap.put("x-amz-request-id", requestIdentifier.getRequestId());
auditMap.put("x-amz-id-2", requestIdentifier.getAmzId());
+ 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());
+ if (originalAccessKeyId != null) {
+ auditMap.put("originalAccessKeyId", originalAccessKeyId);
Review Comment:
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?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/STSSecurityUtil.java:
##########
@@ -157,8 +159,9 @@ private static Token<STSTokenIdentifier>
decodeTokenFromString(String encodedTok
try {
token.decodeFromUrlString(encodedToken);
return token;
- } catch (IOException e) {
- 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);
Review Comment:
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?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSTSTokenRequest.java:
##########
@@ -85,24 +85,39 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, Execut
final OMResponse.Builder omResponse =
OmResponseUtil.getOMResponseBuilder(getOmRequest());
final OzoneManagerProtocolProtos.RevokeSTSTokenRequest revokeReq =
getOmRequest().getRevokeSTSTokenRequest();
- final String sessionToken = revokeReq.getSessionToken();
+ final String originalAccessKeyId = revokeReq.getOriginalAccessKeyId();
+ final String tempAccessKeyId = revokeReq.getTempAccessKeyId();
+ final String revokedStsTokenKey =
STSSecurityUtil.buildRevokedStsTokenKey(tempAccessKeyId, originalAccessKeyId);
// All actual DB mutations are done in the response's addToDBBatch().
- final OMClientResponse omClientResponse = new S3RevokeSTSTokenResponse(
- sessionToken, omResponse.build());
+ final OMClientResponse omClientResponse = new
S3RevokeSTSTokenResponse(revokedStsTokenKey, omResponse.build());
// Audit log
final Map<String, String> auditMap = new HashMap<>();
final OzoneManagerProtocolProtos.UserInfo userInfo =
getOmRequest().getUserInfo();
auditMap.put(OzoneConsts.S3_REVOKESTSTOKEN_USER, userInfo.getUserName());
+ auditMap.put(OzoneConsts.S3_REVOKESTSTOKEN_ORIGINAL_ACCESS_KEY_ID,
originalAccessKeyId);
+ auditMap.put(OzoneConsts.S3_REVOKESTSTOKEN_TEMP_ACCESS_KEY_ID,
tempAccessKeyId);
markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage(
OMAction.REVOKE_STS_TOKEN, auditMap, null, userInfo));
// Update the cache immediately so subsequent validation checks see the
revocation
ozoneManager.getMetadataManager().getS3RevokedStsTokenTable().addCacheEntry(
- new CacheKey<>(sessionToken), CacheValue.get(context.getIndex(),
CLOCK.millis()));
+ new CacheKey<>(revokedStsTokenKey), CacheValue.get(context.getIndex(),
CLOCK.millis()));
- LOG.info("Marked STS session token '{}' as revoked.", sessionToken);
+ LOG.info(
+ "Marked STS token as revoked for originalAccessKeyId={},
tempAccessKeyId={}.", originalAccessKeyId,
+ tempAccessKeyId);
return omClientResponse;
}
+
+ private static void
validateRevokeRequestFields(OzoneManagerProtocolProtos.RevokeSTSTokenRequest
revokeReq)
Review Comment:
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??
--
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]
