From d6ac91f2df0903737028f882f121edc8fe19b215 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 9 Jan 2024 17:44:11 +0530 Subject: [PATCH] minio: fix store user creation (#8425) To prevent errors during multi-user access, use account UUID to create/access user on the provider side. Also, update the existing secret key for a user that already exists. --- .../driver/MinIOObjectStoreDriverImpl.java | 100 ++++++++++++------ .../MinIOObjectStoreDriverImplTest.java | 76 +++++++++---- 2 files changed, 123 insertions(+), 53 deletions(-) diff --git a/plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java b/plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java index 15df5df56dc..b85383a65e8 100644 --- a/plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java +++ b/plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java @@ -18,16 +18,38 @@ */ package org.apache.cloudstack.storage.datastore.driver; +import java.io.IOException; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import javax.crypto.KeyGenerator; +import javax.crypto.SecretKey; +import javax.inject.Inject; + +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao; +import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao; +import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO; +import org.apache.cloudstack.storage.object.BaseObjectStoreDriverImpl; +import org.apache.cloudstack.storage.object.Bucket; +import org.apache.cloudstack.storage.object.BucketObject; +import org.apache.commons.codec.binary.Base64; +import org.apache.commons.lang3.StringUtils; +import org.apache.log4j.Logger; + import com.amazonaws.services.s3.model.AccessControlList; import com.amazonaws.services.s3.model.BucketPolicy; import com.cloud.agent.api.to.DataStoreTO; -import org.apache.cloudstack.storage.object.Bucket; import com.cloud.storage.BucketVO; import com.cloud.storage.dao.BucketDao; import com.cloud.user.Account; import com.cloud.user.AccountDetailsDao; import com.cloud.user.dao.AccountDao; import com.cloud.utils.exception.CloudRuntimeException; + import io.minio.BucketExistsArgs; import io.minio.DeleteBucketEncryptionArgs; import io.minio.MakeBucketArgs; @@ -42,26 +64,10 @@ import io.minio.admin.UserInfo; import io.minio.admin.messages.DataUsageInfo; import io.minio.messages.SseConfiguration; import io.minio.messages.VersioningConfiguration; -import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; -import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao; -import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao; -import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO; -import org.apache.cloudstack.storage.object.BaseObjectStoreDriverImpl; -import org.apache.cloudstack.storage.object.BucketObject; -import org.apache.commons.codec.binary.Base64; -import org.apache.log4j.Logger; - -import javax.crypto.KeyGenerator; -import javax.crypto.SecretKey; -import javax.inject.Inject; -import java.security.NoSuchAlgorithmException; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { private static final Logger s_logger = Logger.getLogger(MinIOObjectStoreDriverImpl.class); + protected static final String ACS_PREFIX = "acs"; @Inject AccountDao _accountDao; @@ -81,14 +87,18 @@ public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { private static final String ACCESS_KEY = "accesskey"; private static final String SECRET_KEY = "secretkey"; - private static final String MINIO_ACCESS_KEY = "minio-accesskey"; - private static final String MINIO_SECRET_KEY = "minio-secretkey"; + protected static final String MINIO_ACCESS_KEY = "minio-accesskey"; + protected static final String MINIO_SECRET_KEY = "minio-secretkey"; @Override public DataStoreTO getStoreTO(DataStore store) { return null; } + protected String getUserOrAccessKeyForAccount(Account account) { + return String.format("%s-%s", ACS_PREFIX, account.getUuid()); + } + @Override public Bucket createBucket(Bucket bucket, boolean objectLock) { //ToDo Client pool mgmt @@ -135,8 +145,8 @@ public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { " \"Version\": \"2012-10-17\"\n" + " }"; MinioAdminClient minioAdminClient = getMinIOAdminClient(storeId); - String policyName = "acs-"+account.getAccountName()+"-policy"; - String userName = "acs-"+account.getAccountName(); + String policyName = getUserOrAccessKeyForAccount(account) + "-policy"; + String userName = getUserOrAccessKeyForAccount(account); try { minioAdminClient.addCannedPolicy(policyName, policy); minioAdminClient.setPolicy(userName, false, policyName); @@ -250,22 +260,53 @@ public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { } + protected void updateAccountCredentials(final long accountId, final String accessKey, final String secretKey, final boolean checkIfNotPresent) { + Map details = _accountDetailsDao.findDetails(accountId); + boolean updateNeeded = false; + if (!checkIfNotPresent || StringUtils.isBlank(details.get(MINIO_ACCESS_KEY))) { + details.put(MINIO_ACCESS_KEY, accessKey); + updateNeeded = true; + } + if (StringUtils.isAllBlank(secretKey, details.get(MINIO_SECRET_KEY))) { + s_logger.error(String.format("Failed to retrieve secret key for MinIO user: %s from store and account details", accessKey)); + } + if (StringUtils.isNotBlank(secretKey) && (!checkIfNotPresent || StringUtils.isBlank(details.get(MINIO_SECRET_KEY)))) { + details.put(MINIO_SECRET_KEY, secretKey); + updateNeeded = true; + } + if (!updateNeeded) { + return; + } + _accountDetailsDao.persist(accountId, details); + } + @Override public boolean createUser(long accountId, long storeId) { Account account = _accountDao.findById(accountId); MinioAdminClient minioAdminClient = getMinIOAdminClient(storeId); - String accessKey = "acs-"+account.getAccountName(); + String accessKey = getUserOrAccessKeyForAccount(account); // Check user exists try { UserInfo userInfo = minioAdminClient.getUserInfo(accessKey); if(userInfo != null) { - s_logger.debug("User already exists in MinIO store: "+accessKey); + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Skipping user creation as the user already exists in MinIO store: %s", accessKey)); + } + updateAccountCredentials(accountId, accessKey, userInfo.secretKey(), true); return true; } - } catch (Exception e) { - s_logger.debug("User does not exist. Creating user: "+accessKey); + } catch (NoSuchAlgorithmException | IOException | InvalidKeyException e) { + s_logger.error(String.format("Error encountered while retrieving user: %s for existing MinIO store user check", accessKey), e); + return false; + } catch (RuntimeException e) { // MinIO lib may throw RuntimeException with code: XMinioAdminNoSuchUser + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Ignoring error encountered while retrieving user: %s for existing MinIO store user check", accessKey)); + } + s_logger.trace("Exception during MinIO user check", e); + } + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("MinIO store user does not exist. Creating user: %s", accessKey)); } - KeyGenerator generator = null; try { generator = KeyGenerator.getInstance("HmacSHA1"); @@ -280,10 +321,7 @@ public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { throw new CloudRuntimeException(e); } // Store user credentials - Map details = new HashMap<>(); - details.put(MINIO_ACCESS_KEY, accessKey); - details.put(MINIO_SECRET_KEY, secretKey); - _accountDetailsDao.persist(accountId, details); + updateAccountCredentials(accountId, accessKey, secretKey, false); return true; } diff --git a/plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java b/plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java index 3041a5b232b..ac88a0ded39 100644 --- a/plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java +++ b/plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java @@ -16,16 +16,23 @@ // under the License. package org.apache.cloudstack.storage.datastore.driver; -import com.cloud.storage.BucketVO; -import com.cloud.storage.dao.BucketDao; -import com.cloud.user.AccountDetailVO; -import com.cloud.user.AccountDetailsDao; -import com.cloud.user.AccountVO; -import com.cloud.user.dao.AccountDao; -import io.minio.BucketExistsArgs; -import io.minio.MinioClient; -import io.minio.RemoveBucketArgs; -import io.minio.admin.MinioAdminClient; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyLong; +import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; + import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao; import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao; import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO; @@ -34,22 +41,24 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; -import java.util.ArrayList; +import com.cloud.storage.BucketVO; +import com.cloud.storage.dao.BucketDao; +import com.cloud.user.AccountDetailVO; +import com.cloud.user.AccountDetailsDao; +import com.cloud.user.AccountVO; +import com.cloud.user.dao.AccountDao; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.anyLong; -import static org.mockito.Mockito.anyString; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import io.minio.BucketExistsArgs; +import io.minio.MinioClient; +import io.minio.RemoveBucketArgs; +import io.minio.admin.MinioAdminClient; +import io.minio.admin.UserInfo; @RunWith(MockitoJUnitRunner.class) public class MinIOObjectStoreDriverImplTest { @@ -97,7 +106,7 @@ public class MinIOObjectStoreDriverImplTest { doReturn(minioClient).when(minioObjectStoreDriverImpl).getMinIOClient(anyLong()); doReturn(minioAdminClient).when(minioObjectStoreDriverImpl).getMinIOAdminClient(anyLong()); when(bucketDao.listByObjectStoreIdAndAccountId(anyLong(), anyLong())).thenReturn(new ArrayList()); - when(account.getAccountName()).thenReturn("admin"); + when(account.getUuid()).thenReturn(UUID.randomUUID().toString()); when(accountDao.findById(anyLong())).thenReturn(account); when(accountDetailsDao.findDetail(anyLong(),anyString())). thenReturn(new AccountDetailVO(1L, "abc","def")); @@ -119,4 +128,27 @@ public class MinIOObjectStoreDriverImplTest { verify(minioClient, times(1)).bucketExists(any()); verify(minioClient, times(1)).removeBucket(any()); } + + @Test + public void testCreateUserExisting() throws Exception { + String uuid = "uuid"; + String accessKey = MinIOObjectStoreDriverImpl.ACS_PREFIX + "-" + uuid; + String secretKey = "secret"; + + doReturn(minioAdminClient).when(minioObjectStoreDriverImpl).getMinIOAdminClient(anyLong()); + when(accountDao.findById(anyLong())).thenReturn(account); + when(account.getUuid()).thenReturn(uuid); + UserInfo info = mock(UserInfo.class); + when(info.secretKey()).thenReturn(secretKey); + when(minioAdminClient.getUserInfo(accessKey)).thenReturn(info); + final Map persistedMap = new HashMap<>(); + Mockito.doAnswer((Answer) invocation -> { + persistedMap.putAll((Map)invocation.getArguments()[1]); + return null; + }).when(accountDetailsDao).persist(Mockito.anyLong(), Mockito.anyMap()); + boolean result = minioObjectStoreDriverImpl.createUser(1L, 1L); + assertTrue(result); + assertEquals(accessKey, persistedMap.get(MinIOObjectStoreDriverImpl.MINIO_ACCESS_KEY)); + assertEquals(secretKey, persistedMap.get(MinIOObjectStoreDriverImpl.MINIO_SECRET_KEY)); + } }