Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,6 @@ public class RevokeSTSTokenHandler extends S3Handler {
description = "STS temporary access key id (for example, ASIA...)")
private String accessKeyId;

@Option(names = "-t",
required = true,
description = "STS session token")
private String sessionToken;

@Option(names = "-y",
description = "Continue without interactive user confirmation")
private boolean yes;
Expand All @@ -73,7 +68,7 @@ protected void execute(OzoneClient client, OzoneAddress address)
}
}

client.getObjectStore().revokeSTSToken(accessKeyId, sessionToken);
client.getObjectStore().revokeSTSToken(accessKeyId);
out().println("STS token revoked for accessKeyId '" + accessKeyId + "'.");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -769,11 +769,10 @@ public AssumeRoleResponseInfo assumeRole(String roleArn, String roleSessionName,
/**
* Revokes an STS token.
* @param accessKeyId The STS accessKeyId (starting with ASIA...)
* @param sessionToken The STS session token
* @throws IOException if an error occurs while revoking the STS token
*/
public void revokeSTSToken(String accessKeyId, String sessionToken) throws IOException {
proxy.revokeSTSToken(accessKeyId, sessionToken);
public void revokeSTSToken(String accessKeyId) throws IOException {
proxy.revokeSTSToken(accessKeyId);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1376,8 +1376,7 @@ AssumeRoleResponseInfo assumeRole(String roleArn, String roleSessionName, int du
/**
* Revokes an STS token.
* @param accessKeyId The STS accessKeyId (starting with ASIA...)
* @param sessionToken The STS session token
* @throws IOException if an error occurs while revoking the STS token
*/
void revokeSTSToken(String accessKeyId, String sessionToken) throws IOException;
void revokeSTSToken(String accessKeyId) throws IOException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -2798,8 +2798,8 @@ public AssumeRoleResponseInfo assumeRole(String roleArn, String roleSessionName,
}

@Override
public void revokeSTSToken(String accessKeyId, String sessionToken) throws IOException {
ozoneManagerClient.revokeSTSToken(accessKeyId, sessionToken);
public void revokeSTSToken(String accessKeyId) throws IOException {
ozoneManagerClient.revokeSTSToken(accessKeyId);
}

private static ExecutorService createThreadPoolExecutor(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1195,10 +1195,9 @@ default AssumeRoleResponseInfo assumeRole(String roleArn, String roleSessionName
/**
* Revokes an STS token.
* @param accessKeyId The STS accessKeyId (starting with ASIA...)
* @param sessionToken The STS session token
* @throws IOException if an error occurs while revoking the STS token
*/
default void revokeSTSToken(String accessKeyId, String sessionToken) throws IOException {
default void revokeSTSToken(String accessKeyId) throws IOException {
throw new UnsupportedOperationException("OzoneManager does not require this to be implemented");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2676,11 +2676,10 @@ public AssumeRoleResponseInfo assumeRole(String roleArn, String roleSessionName,
}

@Override
public void revokeSTSToken(String accessKeyId, String sessionToken) throws IOException {
public void revokeSTSToken(String accessKeyId) throws IOException {
final OzoneManagerProtocolProtos.RevokeSTSTokenRequest request =
OzoneManagerProtocolProtos.RevokeSTSTokenRequest.newBuilder()
.setAccessKeyId(accessKeyId)
.setSessionToken(sessionToken)
.build();

final OMRequest omRequest = createOMRequest(Type.RevokeSTSToken)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2388,7 +2388,6 @@ message AssumeRoleResponse {

message RevokeSTSTokenRequest {
required string accessKeyId = 1;
required string sessionToken = 2;
}

message RevokeSTSTokenResponse {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ String getMultipartKeyFSO(String volume, String bucket, String key, String
*
* @return Table.
*/
Table<String, String> getS3RevokedStsTokenTable();
Table<String, Long> getS3RevokedStsTokenTable();


/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public class OmMetadataManagerImpl implements OMMetadataManager,
private TypedTable<String, String> snapshotRenamedTable;
private TypedTable<String, CompactionLogEntry> compactionLogTable;

private TypedTable<String, String> s3RevokedStsTokenTable;
private TypedTable<String, Long> s3RevokedStsTokenTable;

private OzoneManager ozoneManager;

Expand Down Expand Up @@ -489,8 +489,8 @@ protected void initializeOmTables(CacheType cacheType,

compactionLogTable = initializer.get(OMDBDefinition.COMPACTION_LOG_TABLE_DEF);

// temporaryAccessKeyId -> sessionToken
// FULL_CACHE keeps revocations in memory as there are not expected to be many revoked tokens
// temporaryAccessKeyId -> insertionTimeMillis
// FULL_CACHE keeps revocations in memory as there are not expected to be many
s3RevokedStsTokenTable = initializer.get(
OMDBDefinition.S3_REVOKED_STS_TOKEN_TABLE_DEF, CacheType.FULL_CACHE);
}
Expand Down Expand Up @@ -1691,7 +1691,7 @@ public Table<String, CompactionLogEntry> getCompactionLogTable() {
}

@Override
public Table<String, String> getS3RevokedStsTokenTable() {
public Table<String, Long> getS3RevokedStsTokenTable() {
return s3RevokedStsTokenTable;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
* | userTable | /user :- UserVolumeInfo |
* | dTokenTable | OzoneTokenID :- renew_time |
* | s3SecretTable | s3g_access_key_id :- s3Secret |
* | s3RevokedStsTokenTable | sts_access_key_id :- sessionToken |
* | s3RevokedStsTokenTable | sts_access_key_id :- insertionTimeMillis |
* |------------------------------------------------------------------------|
* }
* </pre>
Expand Down Expand Up @@ -163,11 +163,11 @@ public final class OMDBDefinition extends DBDefinition.WithMap {
S3SecretValue.getCodec());

public static final String S3_REVOKED_STS_TOKEN_TABLE = "s3RevokedStsTokenTable";
/** s3RevokedStsTokenTable: sts_access_key_id :- sessionToken.*/
public static final DBColumnFamilyDefinition<String, String> S3_REVOKED_STS_TOKEN_TABLE_DEF
/** s3RevokedStsTokenTable: sts_access_key_id :- insertionTimeMillis.*/
public static final DBColumnFamilyDefinition<String, Long> S3_REVOKED_STS_TOKEN_TABLE_DEF
= new DBColumnFamilyDefinition<>(S3_REVOKED_STS_TOKEN_TABLE,
StringCodec.get(),
StringCodec.get());
LongCodec.get());

//---------------------------------------------------------------------------
// Volume, Bucket, Prefix and Transaction Tables:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
package org.apache.hadoop.ozone.om.request.s3.security;

import java.io.IOException;
import java.time.Clock;
import java.time.ZoneOffset;
import java.util.HashMap;
import java.util.Map;
import org.apache.hadoop.ozone.OzoneConsts;
Expand All @@ -34,8 +32,6 @@
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
import org.apache.hadoop.ozone.security.STSSecurityUtil;
import org.apache.hadoop.ozone.security.STSTokenIdentifier;
import org.apache.hadoop.security.UserGroupInformation;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -51,9 +47,6 @@
public class S3RevokeSTSTokenRequest extends OMClientRequest {

private static final Logger LOG = LoggerFactory.getLogger(S3RevokeSTSTokenRequest.class);
private static final Clock CLOCK = Clock.system(ZoneOffset.UTC);

private String originalAccessKeyId;

public S3RevokeSTSTokenRequest(OMRequest omRequest) {
super(omRequest);
Expand All @@ -64,32 +57,18 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OzoneManagerProtocolProtos.RevokeSTSTokenRequest revokeReq =
getOmRequest().getRevokeSTSTokenRequest();

// Get the original (long-lived) access key id from the session token
// and enforce the same permission model that is used for S3 secret
// operations (get/set/revoke). Only the owner of the original access
// key (or an S3 / tenant admin) is allowed to revoke its temporary
// STS credentials.
final String sessionToken = revokeReq.getSessionToken();
final String tempAccessKeyId = revokeReq.getAccessKeyId();
final STSTokenIdentifier stsTokenIdentifier = STSSecurityUtil.constructValidateAndDecryptSTSToken(
sessionToken, ozoneManager.getSecretKeyClient(), CLOCK);
originalAccessKeyId = stsTokenIdentifier.getOriginalAccessKeyId();

// Validate that the Access Key ID in the request matches the one in the token
// to prevent users from revoking arbitrary keys using a valid token.
if (!stsTokenIdentifier.getTempAccessKeyId().equals(tempAccessKeyId)) {
throw new OMException("Access Key ID in request does not match the session token",
OMException.ResultCodes.INVALID_REQUEST);
// Only S3/Ozone admins can revoke STS tokens by temporary access key ID.
final OzoneManagerProtocolProtos.UserInfo userInfo = getUserInfo();
final UserGroupInformation ugi = S3SecretRequestHelper.getOrCreateUgi(userInfo.getUserName());
if (!ozoneManager.isS3Admin(ugi)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we allow the user who creates the temporal access ID and session tokens, to revoke the tokens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have the access key ID as the only input parameter to the CLI utility that revokes STS tokens, we wouldn't know who the user was that created the token. (The previous implementation that took both the access key ID and the session token as parameters to the CLI utility had the capability to allow the user who created the temporal access ID to revoke the token, but this was a change to the design that was not agreed upon in Slack discussions, so this PR modified that implementation). If we use the session token instead of access key ID as the one input to the CLI utility, then we would know who created the token, but this would again be a change to the design. Please let me know if you prefer the design to be changed in this way to allow the user who created the token to revoke the token.

throw new OMException("Only S3/Ozone admins can revoke STS tokens.", OMException.ResultCodes.PERMISSION_DENIED);
}

final UserGroupInformation ugi = S3SecretRequestHelper.getOrCreateUgi(originalAccessKeyId);
S3SecretRequestHelper.checkAccessIdSecretOpPermission(ozoneManager, ugi, originalAccessKeyId);

final OMRequest.Builder omRequest = OMRequest.newBuilder()
.setRevokeSTSTokenRequest(revokeReq)
.setCmdType(getOmRequest().getCmdType())
.setClientId(getOmRequest().getClientId())
.setUserInfo(getUserInfo());
.setUserInfo(userInfo);

if (getOmRequest().hasTraceID()) {
omRequest.setTraceID(getOmRequest().getTraceID());
Expand All @@ -104,15 +83,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut

final OzoneManagerProtocolProtos.RevokeSTSTokenRequest revokeReq = getOmRequest().getRevokeSTSTokenRequest();
final String accessKeyId = revokeReq.getAccessKeyId();
final String sessionToken = revokeReq.getSessionToken();

// All actual DB mutations are done in the response's addToDBBatch().
final OMClientResponse omClientResponse = new S3RevokeSTSTokenResponse(
accessKeyId, sessionToken, omResponse.build());
accessKeyId, omResponse.build());

// Audit log
final Map<String, String> auditMap = new HashMap<>();
auditMap.put(OzoneConsts.S3_REVOKESTSTOKEN_USER, originalAccessKeyId);
auditMap.put(OzoneConsts.S3_REVOKESTSTOKEN_USER, getOmRequest().getUserInfo().getUserName());
markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage(
OMAction.REVOKE_STS_TOKEN, auditMap, null, getOmRequest().getUserInfo()));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

import jakarta.annotation.Nonnull;
import java.io.IOException;
import java.time.Clock;
import java.time.ZoneOffset;
import org.apache.hadoop.hdds.utils.db.BatchOperation;
import org.apache.hadoop.hdds.utils.db.Table;
import org.apache.hadoop.ozone.om.OMMetadataManager;
Expand All @@ -35,22 +37,22 @@
@CleanupTableInfo(cleanupTables = {S3_REVOKED_STS_TOKEN_TABLE})
public class S3RevokeSTSTokenResponse extends OMClientResponse {

private static final Clock CLOCK = Clock.system(ZoneOffset.UTC);

private final String accessKeyId;
private final String sessionToken;

public S3RevokeSTSTokenResponse(String accessKeyId, String sessionToken, @Nonnull OMResponse omResponse) {
public S3RevokeSTSTokenResponse(String accessKeyId, @Nonnull OMResponse omResponse) {
super(omResponse);
this.accessKeyId = accessKeyId;
this.sessionToken = sessionToken;
}

@Override
public void addToDBBatch(OMMetadataManager omMetadataManager, BatchOperation batchOperation) throws IOException {
if (accessKeyId != null && getOMResponse().hasStatus() && getOMResponse().getStatus() == OK) {
final Table<String, String> table = omMetadataManager.getS3RevokedStsTokenTable();
final Table<String, Long> table = omMetadataManager.getS3RevokedStsTokenTable();
if (table != null) {
// Store sessionToken as value
table.putWithBatch(batchOperation, accessKeyId, sessionToken);
// Store insertionTimeMillis as value
table.putWithBatch(batchOperation, accessKeyId, CLOCK.millis());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ private static boolean isRevokedStsTempAccessKeyId(STSTokenIdentifier stsTokenId
throw new OMException(msg, INTERNAL_ERROR);
}

final Table<String, String> revokedStsTokenTable = metadataManager.getS3RevokedStsTokenTable();
final Table<String, Long> revokedStsTokenTable = metadataManager.getS3RevokedStsTokenTable();
if (revokedStsTokenTable == null) {
final String msg = "Could not determine STS revocation: revokedStsTokenTable is null";
LOG.warn(msg);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
import org.apache.hadoop.hdds.protocol.StorageType;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.hdds.utils.TransactionInfo;
import org.apache.hadoop.hdds.utils.db.Table;
import org.apache.hadoop.hdds.utils.db.TypedTable;
import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
import org.apache.hadoop.ozone.om.codec.OMDBDefinition;
Expand All @@ -105,6 +105,7 @@
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.PartKeyInfo;
import org.apache.hadoop.ozone.snapshot.ListSnapshotResponse;
import org.apache.hadoop.util.Time;
import org.apache.ozone.test.TestClock;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
Expand Down Expand Up @@ -1299,34 +1300,36 @@ public void testS3RevokedStsTokenTablePutAndGet() throws Exception {
// Ensure the table is initialized
assertNotNull(omMetadataManager.getS3RevokedStsTokenTable(), "s3RevokedStsTokenTable should be initialized");

final TestClock clock = TestClock.newInstance();
final String tempAccessKeyId1 = "ASIA7VUS1EOBCW8RRJVR";
final String sessionToken1 = "test-session-token-1";
final long insertionTime1 = clock.millis();
final String tempAccessKeyId2 = "ASIA904E65QIGL9ON305";
final String sessionToken2 = "test-session-token-2";

final Table<String, String> table = omMetadataManager.getS3RevokedStsTokenTable();
final long insertionTime2 = insertionTime1 + 1234L;

// This table is configured as FULL_CACHE in OmMetadataManagerImpl.
// A put() writes to RocksDB but does not update the table cache, so get() and getIfExist() will return null unless
// the cache is updated with addCacheEntry(). getSkipCache() will read the DB instead of the cache.
table.put(tempAccessKeyId1, sessionToken1);
table.put(tempAccessKeyId2, sessionToken2);
final TypedTable<String, Long> revokedTable =
(TypedTable<String, Long>) omMetadataManager.getS3RevokedStsTokenTable();

revokedTable.put(tempAccessKeyId1, insertionTime1);
revokedTable.put(tempAccessKeyId2, insertionTime2);

// Verify the values are persisted in RocksDB.
assertEquals(sessionToken1, table.getSkipCache(tempAccessKeyId1));
assertEquals(sessionToken2, table.getSkipCache(tempAccessKeyId2));
assertEquals(insertionTime1, revokedTable.getSkipCache(tempAccessKeyId1));
assertEquals(insertionTime2, revokedTable.getSkipCache(tempAccessKeyId2));

// Update cache to make get/getIfExist reflect the write for FULL_CACHE tables.
table.addCacheEntry(tempAccessKeyId1, sessionToken1, 1L);
table.addCacheEntry(tempAccessKeyId2, sessionToken2, 1L);
revokedTable.addCacheEntry(tempAccessKeyId1, insertionTime1, 1L);
revokedTable.addCacheEntry(tempAccessKeyId2, insertionTime2, 1L);

// Verify get and getIfExist return the stored value
assertEquals(sessionToken1, table.get(tempAccessKeyId1));
assertEquals(sessionToken1, table.getIfExist(tempAccessKeyId1));
assertEquals(sessionToken2, table.get(tempAccessKeyId2));
assertEquals(sessionToken2, table.getIfExist(tempAccessKeyId2));
assertEquals(insertionTime1, revokedTable.get(tempAccessKeyId1));
assertEquals(insertionTime1, revokedTable.getIfExist(tempAccessKeyId1));
assertEquals(insertionTime2, revokedTable.get(tempAccessKeyId2));
assertEquals(insertionTime2, revokedTable.getIfExist(tempAccessKeyId2));

// Unknown key should return null for getIfExist
assertNull(table.getIfExist("ASIA_UNKNOWN_ACCESS_KEY"));
assertNull(revokedTable.getIfExist("ASIA_UNKNOWN_ACCESS_KEY"));
}
}
Loading