This is an automated email from the ASF dual-hosted git repository.
adoroszlai pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new 9238be33a8 HDDS-10200. OM may terminate due to NPE in `S3SecretValue`
proto conversion (#6089)
9238be33a8 is described below
commit 9238be33a88f98a055ec61d432c3b33f972f4ac5
Author: Doroszlai, Attila <[email protected]>
AuthorDate: Thu Jan 25 19:18:28 2024 +0100
HDDS-10200. OM may terminate due to NPE in `S3SecretValue` proto conversion
(#6089)
---
.../apache/hadoop/ozone/om/S3InMemoryCache.java | 8 +---
.../hadoop/ozone/om/helpers/S3SecretValue.java | 47 ++++++++++------------
.../ozone/client/rpc/TestSecureOzoneRpcClient.java | 4 +-
.../ozone/om/helpers/TestS3SecretValueCodec.java | 2 +-
.../hadoop/ozone/om/S3SecretManagerImpl.java | 3 +-
.../om/request/s3/security/OMSetSecretRequest.java | 18 +++------
.../om/request/s3/security/S3GetSecretRequest.java | 13 ++----
.../tenant/OMTenantAssignUserAccessIdRequest.java | 5 +--
.../TestOzoneDelegationTokenSecretManager.java | 4 +-
.../ozone/s3/remote/vault/VaultS3SecretStore.java | 2 +-
.../s3/remote/vault/TestVaultS3SecretStore.java | 6 +--
.../hadoop/ozone/client/ClientProtocolStub.java | 2 +-
.../hadoop/ozone/s3secret/TestSecretGenerate.java | 40 ++++++++++++------
13 files changed, 72 insertions(+), 82 deletions(-)
diff --git
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java
index 4f1f66facc..122b04b715 100644
---
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java
+++
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java
@@ -44,13 +44,7 @@ public class S3InMemoryCache implements S3SecretCache {
@Override
public void invalidate(String id) {
- S3SecretValue secret = cache.getIfPresent(id);
- if (secret == null) {
- return;
- }
- secret.setDeleted(true);
- secret.setAwsSecret(null);
- cache.put(id, secret);
+ cache.asMap().computeIfPresent(id, (k, secret) -> secret.deleted());
}
/**
diff --git
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java
index e97adc0a50..cb1ed0976a 100644
---
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java
+++
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java
@@ -27,7 +27,7 @@ import java.util.Objects;
/**
* S3Secret to be saved in database.
*/
-public class S3SecretValue {
+public final class S3SecretValue {
private static final Codec<S3SecretValue> CODEC = new DelegatedCodec<>(
Proto2Codec.get(S3Secret.getDefaultInstance()),
S3SecretValue::fromProtobuf,
@@ -38,16 +38,29 @@ public class S3SecretValue {
}
// TODO: This field should be renamed to accessId for generalization.
- private String kerberosID;
- private String awsSecret;
- private boolean isDeleted;
- private long transactionLogIndex;
+ private final String kerberosID;
+ private final String awsSecret;
+ private final boolean isDeleted;
+ private final long transactionLogIndex;
- public S3SecretValue(String kerberosID, String awsSecret) {
- this(kerberosID, awsSecret, false, 0L);
+ public static S3SecretValue of(String kerberosID, String awsSecret) {
+ return of(kerberosID, awsSecret, 0);
}
- public S3SecretValue(String kerberosID, String awsSecret, boolean isDeleted,
+ public static S3SecretValue of(String kerberosID, String awsSecret, long
transactionLogIndex) {
+ return new S3SecretValue(
+ Objects.requireNonNull(kerberosID),
+ Objects.requireNonNull(awsSecret),
+ false,
+ transactionLogIndex
+ );
+ }
+
+ public S3SecretValue deleted() {
+ return new S3SecretValue(kerberosID, "", true, transactionLogIndex);
+ }
+
+ private S3SecretValue(String kerberosID, String awsSecret, boolean isDeleted,
long transactionLogIndex) {
this.kerberosID = kerberosID;
this.awsSecret = awsSecret;
@@ -59,26 +72,14 @@ public class S3SecretValue {
return kerberosID;
}
- public void setKerberosID(String kerberosID) {
- this.kerberosID = kerberosID;
- }
-
public String getAwsSecret() {
return awsSecret;
}
- public void setAwsSecret(String awsSecret) {
- this.awsSecret = awsSecret;
- }
-
public boolean isDeleted() {
return isDeleted;
}
- public void setDeleted(boolean status) {
- this.isDeleted = status;
- }
-
public String getAwsAccessKey() {
return kerberosID;
}
@@ -87,12 +88,8 @@ public class S3SecretValue {
return transactionLogIndex;
}
- public void setTransactionLogIndex(long transactionLogIndex) {
- this.transactionLogIndex = transactionLogIndex;
- }
-
public static S3SecretValue fromProtobuf(S3Secret s3Secret) {
- return new S3SecretValue(s3Secret.getKerberosID(),
s3Secret.getAwsSecret());
+ return S3SecretValue.of(s3Secret.getKerberosID(), s3Secret.getAwsSecret());
}
public S3Secret getProtobuf() {
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java
index 548b1b3035..38044805f3 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java
@@ -264,7 +264,7 @@ public class TestSecureOzoneRpcClient extends
TestOzoneRpcClient {
// Add secret to S3Secret table.
s3SecretManager.storeSecret(accessKey,
- new S3SecretValue(accessKey, secret));
+ S3SecretValue.of(accessKey, secret));
OMRequest writeRequest = OMRequest.newBuilder()
.setCmdType(OzoneManagerProtocolProtos.Type.CreateVolume)
@@ -313,7 +313,7 @@ public class TestSecureOzoneRpcClient extends
TestOzoneRpcClient {
// Override secret to S3Secret store with some dummy value
s3SecretManager
- .storeSecret(accessKey, new S3SecretValue(accessKey, "dummy"));
+ .storeSecret(accessKey, S3SecretValue.of(accessKey, "dummy"));
// Write request with invalid credentials.
omResponse = cluster.getOzoneManager().getOmServerProtocol()
diff --git
a/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java
b/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java
index d1c4372ca9..0f3af5f3a3 100644
---
a/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java
+++
b/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java
@@ -43,7 +43,7 @@ public class TestS3SecretValueCodec
final Codec<S3SecretValue> codec = getCodec();
S3SecretValue s3SecretValue =
- new S3SecretValue(UUID.randomUUID().toString(),
+ S3SecretValue.of(UUID.randomUUID().toString(),
UUID.randomUUID().toString());
byte[] data = codec.toPersistedFormat(s3SecretValue);
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java
index 4e46adc66b..195ff816bc 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java
@@ -62,8 +62,7 @@ public class S3SecretManagerImpl implements S3SecretManager {
// purposely deleted the secret. Hence, we do not have to check the DB.
return null;
}
- return new S3SecretValue(cacheValue.getKerberosID(),
- cacheValue.getAwsSecret());
+ return cacheValue;
}
S3SecretValue result = s3SecretStore.getSecret(kerberosID);
if (result != null) {
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java
index 1edece52da..aa7fe46992 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java
@@ -113,26 +113,20 @@ public class OMSetSecretRequest extends OMClientRequest {
try {
omClientResponse = ozoneManager.getS3SecretManager()
.doUnderLock(accessId, s3SecretManager -> {
- // Intentionally set to final so they can only be set once.
- final S3SecretValue newS3SecretValue;
// Update legacy S3SecretTable, if the accessId entry exists
- if (s3SecretManager.hasS3Secret(accessId)) {
- // accessId found in S3SecretTable. Update S3SecretTable
- LOG.debug("Updating S3SecretTable cache entry");
- // Update S3SecretTable cache entry in this case
- newS3SecretValue = new S3SecretValue(accessId, secretKey);
- // Set the transactionLogIndex to be used for updating.
- newS3SecretValue.setTransactionLogIndex(termIndex.getIndex());
- s3SecretManager
- .updateCache(accessId, newS3SecretValue);
- } else {
+ if (!s3SecretManager.hasS3Secret(accessId)) {
// If S3SecretTable is not updated,
// throw ACCESS_ID_NOT_FOUND exception.
throw new OMException("accessId '" + accessId + "' not found.",
OMException.ResultCodes.ACCESS_ID_NOT_FOUND);
}
+ // Update S3SecretTable cache entry in this case
+ // Set the transactionLogIndex to be used for updating.
+ final S3SecretValue newS3SecretValue = S3SecretValue.of(accessId,
secretKey, termIndex.getIndex());
+ s3SecretManager.updateCache(accessId, newS3SecretValue);
+
// Compose response
final SetS3SecretResponse.Builder setSecretResponse =
SetS3SecretResponse.newBuilder()
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
index dcf5688e39..90c27038eb 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
@@ -150,21 +150,16 @@ public class S3GetSecretRequest extends OMClientRequest {
try {
omClientResponse = ozoneManager.getS3SecretManager()
.doUnderLock(accessId, s3SecretManager -> {
- S3SecretValue assignS3SecretValue;
- S3SecretValue s3SecretValue =
- s3SecretManager.getSecret(accessId);
+ final S3SecretValue assignS3SecretValue;
+ S3SecretValue s3SecretValue = s3SecretManager.getSecret(accessId);
if (s3SecretValue == null) {
// Not found in S3SecretTable.
if (createIfNotExist) {
// Add new entry in this case
- assignS3SecretValue =
- new S3SecretValue(accessId, awsSecret.get());
- // Set the transactionLogIndex to be used for updating.
-
assignS3SecretValue.setTransactionLogIndex(termIndex.getIndex());
+ assignS3SecretValue = S3SecretValue.of(accessId,
awsSecret.get(), termIndex.getIndex());
// Add cache entry first.
- s3SecretManager.updateCache(accessId,
- assignS3SecretValue);
+ s3SecretManager.updateCache(accessId, assignS3SecretValue);
} else {
assignS3SecretValue = null;
}
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
index a69b357419..3179a7d0f3 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
@@ -271,10 +271,7 @@ public class OMTenantAssignUserAccessIdRequest extends
OMClientRequest {
}
}
- final S3SecretValue s3SecretValue =
- new S3SecretValue(accessId, awsSecret);
- // Set the transactionLogIndex to be used for updating.
- s3SecretValue.setTransactionLogIndex(transactionLogIndex);
+ final S3SecretValue s3SecretValue = S3SecretValue.of(accessId,
awsSecret, transactionLogIndex);
// Add to tenantAccessIdTable
final OmDBAccessIdInfo omDBAccessIdInfo = new OmDBAccessIdInfo.Builder()
diff --git
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java
index 49d19d3bc3..ce7c0c848f 100644
---
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java
+++
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java
@@ -106,9 +106,9 @@ public class TestOzoneDelegationTokenSecretManager {
serviceRpcAdd = new Text("localhost");
final Map<String, S3SecretValue> s3Secrets = new HashMap<>();
s3Secrets.put("testuser1",
- new S3SecretValue("testuser1", "dbaksbzljandlkandlsd"));
+ S3SecretValue.of("testuser1", "dbaksbzljandlkandlsd"));
s3Secrets.put("abc",
- new S3SecretValue("abc", "djakjahkd"));
+ S3SecretValue.of("abc", "djakjahkd"));
om = mock(OzoneManager.class);
OMMetadataManager metadataManager = new OmMetadataManagerImpl(conf, om);
when(om.getMetadataManager()).thenReturn(metadataManager);
diff --git
a/hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java
b/hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java
index 892b86eaa7..c9bb4d6435 100644
---
a/hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java
+++
b/hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java
@@ -115,7 +115,7 @@ public class VaultS3SecretStore implements S3SecretStore {
return null;
}
- return new S3SecretValue(kerberosID, s3Secret);
+ return S3SecretValue.of(kerberosID, s3Secret);
} catch (VaultException e) {
LOG.error("Failed to read secret", e);
throw new IOException("Failed to read secret", e);
diff --git
a/hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java
b/hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java
index 4700a283cd..082a7da3f2 100644
---
a/hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java
+++
b/hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java
@@ -89,7 +89,7 @@ public class TestVaultS3SecretStore {
@Test
public void testReadWrite() throws IOException {
SUCCESS_OPERATION_LIMIT.set(2);
- S3SecretValue secret = new S3SecretValue("id", "value");
+ S3SecretValue secret = S3SecretValue.of("id", "value");
s3SecretStore.storeSecret(
"id",
secret);
@@ -101,7 +101,7 @@ public class TestVaultS3SecretStore {
public void testReAuth() throws IOException {
SUCCESS_OPERATION_LIMIT.set(1);
AUTH_OPERATION_PROVIDER.set(1);
- S3SecretValue secret = new S3SecretValue("id", "value");
+ S3SecretValue secret = S3SecretValue.of("id", "value");
s3SecretStore.storeSecret("id", secret);
assertEquals(secret, s3SecretStore.getSecret("id"));
@@ -112,7 +112,7 @@ public class TestVaultS3SecretStore {
@Test
public void testAuthFail() throws IOException {
SUCCESS_OPERATION_LIMIT.set(1);
- S3SecretValue secret = new S3SecretValue("id", "value");
+ S3SecretValue secret = S3SecretValue.of("id", "value");
s3SecretStore.storeSecret("id", secret);
assertThrows(IOException.class,
diff --git
a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java
b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java
index f0528facbb..ebd441567a 100644
---
a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java
+++
b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java
@@ -379,7 +379,7 @@ public class ClientProtocolStub implements ClientProtocol {
@Override
@Nonnull
public S3SecretValue getS3Secret(String kerberosID) throws IOException {
- return new S3SecretValue(STUB_KERBEROS_ID, STUB_SECRET);
+ return S3SecretValue.of(STUB_KERBEROS_ID, STUB_SECRET);
}
@Override
diff --git
a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java
b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java
index e6ff4024d1..007fa9099e 100644
---
a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java
+++
b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java
@@ -42,14 +42,14 @@ import org.mockito.junit.jupiter.MockitoExtension;
import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.notNull;
import static org.mockito.Mockito.when;
/**
* Test for S3 secret generate endpoint.
*/
@ExtendWith(MockitoExtension.class)
-public class TestSecretGenerate {
+class TestSecretGenerate {
private static final String USER_NAME = "test";
private static final String OTHER_USER_NAME = "test2";
private static final String USER_SECRET = "test_secret";
@@ -69,12 +69,11 @@ public class TestSecretGenerate {
private static S3SecretValue getS3SecretValue(InvocationOnMock invocation) {
Object[] args = invocation.getArguments();
- return new S3SecretValue((String) args[0], USER_SECRET);
+ return S3SecretValue.of((String) args[0], USER_SECRET);
}
@BeforeEach
- void setUp() throws IOException {
- when(proxy.getS3Secret(any())).then(TestSecretGenerate::getS3SecretValue);
+ void setUp() {
OzoneConfiguration conf = new OzoneConfiguration();
OzoneClient client = new OzoneClientStub(new ObjectStoreStub(conf, proxy));
@@ -89,23 +88,20 @@ public class TestSecretGenerate {
@Test
void testSecretGenerate() throws IOException {
- when(principal.getName()).thenReturn(USER_NAME);
- when(securityContext.getUserPrincipal()).thenReturn(principal);
- when(context.getSecurityContext()).thenReturn(securityContext);
+ setupSecurityContext();
+ hasNoSecretYet();
S3SecretResponse response =
(S3SecretResponse) endpoint.generate().getEntity();
+
assertEquals(USER_SECRET, response.getAwsSecret());
assertEquals(USER_NAME, response.getAwsAccessKey());
}
@Test
void testIfSecretAlreadyExists() throws IOException {
- when(principal.getName()).thenReturn(USER_NAME);
- when(securityContext.getUserPrincipal()).thenReturn(principal);
- when(context.getSecurityContext()).thenReturn(securityContext);
- when(proxy.getS3Secret(any())).thenThrow(new OMException("Secret already
exists",
- OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS));
+ setupSecurityContext();
+ hasSecretAlready();
Response response = endpoint.generate();
@@ -116,9 +112,27 @@ public class TestSecretGenerate {
@Test
void testSecretGenerateWithUsername() throws IOException {
+ hasNoSecretYet();
+
S3SecretResponse response =
(S3SecretResponse) endpoint.generate(OTHER_USER_NAME).getEntity();
assertEquals(USER_SECRET, response.getAwsSecret());
assertEquals(OTHER_USER_NAME, response.getAwsAccessKey());
}
+
+ private void setupSecurityContext() {
+ when(principal.getName()).thenReturn(USER_NAME);
+ when(securityContext.getUserPrincipal()).thenReturn(principal);
+ when(context.getSecurityContext()).thenReturn(securityContext);
+ }
+
+ private void hasNoSecretYet() throws IOException {
+ when(proxy.getS3Secret(notNull()))
+ .then(TestSecretGenerate::getS3SecretValue);
+ }
+
+ private void hasSecretAlready() throws IOException {
+ when(proxy.getS3Secret(notNull()))
+ .thenThrow(new OMException("Secret already exists",
OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
