chihsuan commented on code in PR #11030:
URL: https://github.com/apache/ozone/pull/11030#discussion_r3805132575
##########
hadoop-hdds/docs/content/design/ozone-sts.md:
##########
@@ -146,9 +146,13 @@ credential will have the permissions and actions
comprising the intersection of
In the rare event temporary credentials need to be revoked (ex. for security
reasons), a table in the OzoneManager RocksDB will be created
to store revoked tokens, and a command-line utility will be created to add
tokens to the table. A background cleaner service
-will be created to run every 3 hours to delete revoked tokens that have been
in the table for more than 12 hours. The
-input parameter for the command-line utility will be the sessionToken - this
value is returned in plain text as a result
-of the AssumeRole call (mentioned above). In this way, specific STS tokens
can be revoked as opposed to all tokens. Furthermore,
+will be created to run every 3 hours to delete revoked tokens that have been
in the table for more than 12 hours. The
+command-line utility accepts `originalAccessKeyId` and `tempAccessKeyId`. The
OM stores revocations by building a
+single key from these two values joined by an unescaped `|` delimiter, with
`tempAccessKeyId` first:
+`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,
Review Comment:
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?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSTSTokenRequest.java:
##########
@@ -63,16 +69,14 @@ public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
final OzoneManagerProtocolProtos.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();
Review Comment:
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?
##########
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());
Review Comment:
`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?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -314,6 +314,10 @@ public final class OzoneConsts {
public static final String S3_SETSECRET_USER = "S3SetSecretUser";
public static final String S3_REVOKESECRET_USER = "S3RevokeSecretUser";
public static final String S3_REVOKESTSTOKEN_USER = "S3RevokeSTSTokenUser";
+ 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;
Review Comment:
Just curious, do we need the two `S3_REVOKESTSTOKEN_*` aliases here?
--
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]
