diff --git a/api/src/main/java/com/cloud/user/AccountService.java b/api/src/main/java/com/cloud/user/AccountService.java index fc450e9179c5..990bf18238ce 100644 --- a/api/src/main/java/com/cloud/user/AccountService.java +++ b/api/src/main/java/com/cloud/user/AccountService.java @@ -185,4 +185,6 @@ User createUser(String userName, String password, String firstName, String lastN String getAccessingApiKey(BaseCmd cmd); List getAllKeypairPermissions(String apiKey); + + List getAllExplicitKeyPairPermissions(Long keyPairId); } diff --git a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java index 286a3598e4fb..cc701029a350 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -20,6 +20,7 @@ import com.cloud.user.Account; import com.cloud.user.User; import com.cloud.utils.component.Adapter; +import org.apache.cloudstack.acl.apikeypair.ApiKeyPair; import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission; import java.util.List; @@ -32,8 +33,8 @@ public interface APIChecker extends Adapter { // If true, apiChecker has checked the operation // If false, apiChecker is unable to handle the operation or not implemented // On exception, checkAccess failed don't allow - boolean checkAccess(User user, String apiCommandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException; - boolean checkAccess(Account account, String apiCommandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException; + boolean checkAccess(User user, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException; + boolean checkAccess(Account account, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException; /** * Verifies if the account has permission for the given list of APIs and returns only the allowed ones. * diff --git a/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java b/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java index f3e2335519a4..0bf0d9373ee5 100644 --- a/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java +++ b/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java @@ -28,6 +28,7 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; +import org.apache.cloudstack.acl.apikeypair.ApiKeyPair; import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission; import org.apache.cloudstack.acl.RolePermissionEntity.Permission; import org.apache.cloudstack.api.APICommand; @@ -141,7 +142,7 @@ protected Account getAccountFromIdUsingCache(long accountId) { } @Override - public boolean checkAccess(User user, String commandName, ApiKeyPairPermission ... apiKeyPairPermissions) throws PermissionDeniedException { + public boolean checkAccess(User user, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException { if (!isEnabled()) { return true; } @@ -150,11 +151,11 @@ public boolean checkAccess(User user, String commandName, ApiKeyPairPermission . throw new PermissionDeniedException(String.format("Account for user with ID [%s] cannot be found", user.getUuid())); } - return checkAccess(account, commandName, apiKeyPairPermissions); + return checkAccess(account, commandName, keyPair, apiKeyPairPermissions); } @Override - public boolean checkAccess(Account account, String commandName, ApiKeyPairPermission ... apiKeyPairPermissions) { + public boolean checkAccess(Account account, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission ... apiKeyPairPermissions) { Pair> roleAndPermissions = getRolePermissionsUsingCache(account.getRoleId()); final Role accountRole = roleAndPermissions.first(); if (accountRole == null) { @@ -166,7 +167,8 @@ public boolean checkAccess(Account account, String commandName, ApiKeyPairPermis return true; } - boolean considerKeyPairPermissions = apiKeyPairPermissions.length > 0; + boolean keyPairHasExplicitPermissions = keyPair != null && !accountService.getAllExplicitKeyPairPermissions(keyPair.getId()).isEmpty(); + boolean considerKeyPairPermissions = apiKeyPairPermissions.length > 0 || keyPairHasExplicitPermissions; List allRules = considerKeyPairPermissions ? Arrays.asList(apiKeyPairPermissions) : new ArrayList<>(roleAndPermissions.second()); if (checkApiPermissionByRole(accountRole, commandName, allRules, considerKeyPairPermissions)) { return true; diff --git a/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java b/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java index 71fbbb4a3650..069f24c965d2 100644 --- a/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java +++ b/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java @@ -85,14 +85,14 @@ public void setUp() throws NoSuchFieldException, IllegalAccessException { // Enabled plugin Mockito.doReturn(true).when(apiAccessCheckerSpy).isEnabled(); - Mockito.doCallRealMethod().when(apiAccessCheckerSpy).checkAccess(Mockito.any(User.class), Mockito.anyString()); + Mockito.doCallRealMethod().when(apiAccessCheckerSpy).checkAccess(Mockito.any(User.class), Mockito.anyString(), Mockito.any()); } @Test public void testInvalidAccountCheckAccess() { Mockito.when(accountService.getAccount(Mockito.anyLong())).thenReturn(null); try { - apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi"); + apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi", null); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -102,7 +102,7 @@ public void testInvalidAccountCheckAccess() { public void testInvalidAccountRoleCheckAccess() { Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(null); try { - apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi"); + apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi", null); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -112,14 +112,14 @@ public void testInvalidAccountRoleCheckAccess() { public void testDefaultRootAdminAccess() { Mockito.when(accountService.getAccount(Mockito.anyLong())).thenReturn(new AccountVO("root admin", 1L, null, Account.Type.ADMIN, "some-uuid")); Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "SomeRole", RoleType.Admin, "default root admin role")); - assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), "anyApi")); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), "anyApi", null)); } @Test public void testInvalidRolePermissionsCheckAccess() { Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.emptyList()); try { - apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi"); + apiAccessCheckerSpy.checkAccess(getTestUser(), "someApi", null); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -130,7 +130,7 @@ public void testValidAllowRolePermissionApiCheckAccess() { final String allowedApiName = "someAllowedApi"; final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null); Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); - assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName, null)); } @Test @@ -138,7 +138,7 @@ public void testValidAllowRolePermissionWildcardCheckAccess() { final String allowedApiName = "someAllowedApi"; final RolePermission permission = new RolePermissionVO(1L, "some*", Permission.ALLOW, null); Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); - assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName, null)); } @Test @@ -147,7 +147,7 @@ public void testValidDenyRolePermissionApiCheckAccess() { final RolePermission permission = new RolePermissionVO(1L, denyApiName, Permission.DENY, null); Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); try { - apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName); + apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName, null); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -159,7 +159,7 @@ public void testValidDenyRolePermissionWildcardCheckAccess() { final RolePermission permission = new RolePermissionVO(1L, "*Deny*", Permission.DENY, null); Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); try { - apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName); + apiAccessCheckerSpy.checkAccess(getTestUser(), denyApiName, null); fail("Exception was expected"); } catch (PermissionDeniedException ignored) { } @@ -169,7 +169,7 @@ public void testValidDenyRolePermissionWildcardCheckAccess() { public void testAnnotationFallbackCheckAccess() { final String allowedApiName = "someApiWithAnnotations"; apiAccessCheckerSpy.addApiToRoleBasedAnnotationsMap(getTestRole().getRoleType(), allowedApiName); - assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), allowedApiName, null)); } @Test @@ -202,21 +202,21 @@ public void getApisAllowedToUserTestPermissionDenyForGivenApiShouldReturnEmptyLi public void checkAccessTestInvalidApiKeyPairPermission() { final String api = "someDeniedApi"; final ApiKeyPairPermission permission = new ApiKeyPairPermissionVO(1L, api, Permission.DENY, null); - assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permission)); + assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permission)); } @Test(expected = UnavailableCommandException.class) public void checkAccessTestUnrelatedApiKeyPairPermission() { final String api = "someDeniedApi"; final ApiKeyPairPermission permission = new ApiKeyPairPermissionVO(1L, "apiName", Permission.ALLOW, null); - assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permission)); + assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permission)); } @Test public void checkAccessTestValidApiKeyPairPermission() { final String api = "someAllowedApi"; final ApiKeyPairPermission permission = new ApiKeyPairPermissionVO(1L, api, Permission.ALLOW, null); - assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permission)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permission)); } @Test @@ -226,7 +226,7 @@ public void checkAccessTestValidMultipleApiKeyPermissions() { new ApiKeyPairPermissionVO(1L, "someDeniedApi", Permission.DENY, null), new ApiKeyPairPermissionVO(1L, api, Permission.ALLOW, null) }; - assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permissions)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permissions)); } @Test(expected = UnavailableCommandException.class) @@ -236,7 +236,7 @@ public void checkAccessTestInvalidMultipleApiKeyPermissions() { new ApiKeyPairPermissionVO(1L, "someAllowedApi", Permission.ALLOW, null), new ApiKeyPairPermissionVO(1L, api, Permission.DENY, null) }; - assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, permissions)); + assertFalse(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, permissions)); } @@ -247,7 +247,7 @@ public void checkAccessTestValidApiKeyPairPermissionWithNullOverride() { final RolePermission permission = new RolePermissionVO(1L, api, Permission.ALLOW, null); Mockito.doReturn(Collections.singletonList(permission)).when(roleServiceMock).findAllPermissionsBy(Mockito.anyLong()); - assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, emptyPermissionArray)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, emptyPermissionArray)); Mockito.verify(roleServiceMock).findAllPermissionsBy(Mockito.anyLong()); } @@ -258,7 +258,7 @@ public void checkAccessTestInvalidApiKeyPairPermissionWithNullOverride() { final RolePermission permission = new RolePermissionVO(1L, api, Permission.DENY, null); Mockito.doReturn(Collections.singletonList(permission)).when(roleServiceMock).findAllPermissionsBy(Mockito.anyLong()); - assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, emptyPermissionArray)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestUser(), api, null, emptyPermissionArray)); Mockito.verify(roleServiceMock, Mockito.times(1)).findAllPermissionsBy(Mockito.anyLong()); } } diff --git a/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java b/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java index 8513f458660c..6d5da8dee1a7 100644 --- a/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java +++ b/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java @@ -23,6 +23,7 @@ import javax.naming.ConfigurationException; import org.apache.cloudstack.acl.RolePermissionEntity.Permission; +import org.apache.cloudstack.acl.apikeypair.ApiKeyPair; import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission; import org.apache.cloudstack.context.CallContext; @@ -106,7 +107,7 @@ public List getApisAllowedToUser(Role role, User user, List apiN } @Override - public boolean checkAccess(User user, String apiCommandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException { + public boolean checkAccess(User user, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException { if (!isEnabled()) { return true; } @@ -151,7 +152,7 @@ public boolean checkAccess(User user, String apiCommandName, ApiKeyPairPermissio } @Override - public boolean checkAccess(Account account, String apiCommandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException { + public boolean checkAccess(Account account, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException { return true; } diff --git a/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java b/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java index 6cf4da88f5c8..6692c948be10 100644 --- a/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java +++ b/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java @@ -27,6 +27,7 @@ import javax.naming.ConfigurationException; import com.cloud.exception.UnavailableCommandException; +import org.apache.cloudstack.acl.apikeypair.ApiKeyPair; import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission; import org.apache.cloudstack.api.APICommand; @@ -92,7 +93,7 @@ public List getApisAllowedToUser(Role role, User user, List apiN } @Override - public boolean checkAccess(User user, String commandName, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException { + public boolean checkAccess(User user, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException { if (!isEnabled()) { return true; } @@ -102,11 +103,11 @@ public boolean checkAccess(User user, String commandName, ApiKeyPairPermission.. throw new PermissionDeniedException(String.format("The account with id [%s] for user with uuid [%s] is null.", user.getAccountId(), user.getUuid())); } - return checkAccess(account, commandName); + return checkAccess(account, commandName, keyPair); } @Override - public boolean checkAccess(Account account, String commandName, ApiKeyPairPermission... apiKeyPairPermissions) { + public boolean checkAccess(Account account, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) { if (!isEnabled()) { return true; } diff --git a/plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java b/plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java index d412f12fce24..17bbc84e9f55 100644 --- a/plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java +++ b/plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java @@ -252,7 +252,7 @@ public List listApiNames(Account account) { boolean isAllowed = true; for (APIChecker apiChecker : _apiAccessCheckers) { try { - apiChecker.checkAccess(account, apiName); + apiChecker.checkAccess(account, apiName, null); } catch (Exception ex) { isAllowed = false; } @@ -287,7 +287,7 @@ public ListResponse listApis(User user, String name, Lis for (APIChecker apiChecker : _apiAccessCheckers) { try { - apiChecker.checkAccess(user, name); + apiChecker.checkAccess(user, name, null); } catch (Exception ex) { logger.error(String.format("API discovery access check failed for [%s] with error [%s].", name, ex.getMessage()), ex); return null; diff --git a/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java b/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java index afa2b6155de6..c1baccfd9f5d 100644 --- a/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java +++ b/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java @@ -29,6 +29,7 @@ import org.apache.cloudstack.acl.Role; import org.apache.cloudstack.acl.RolePermissionEntity; import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.acl.apikeypair.ApiKeyPair; import org.apache.cloudstack.acl.apikeypair.ApiKeyPairPermission; import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import org.springframework.stereotype.Component; @@ -164,17 +165,17 @@ public void throwExceptionDueToApiRateLimitReached(Long accountId) throws Reques } @Override - public boolean checkAccess(User user, String apiCommandName, ApiKeyPairPermission ... apiKeyPairPermissions) throws PermissionDeniedException { + public boolean checkAccess(User user, String apiCommandName, ApiKeyPair keyPair, ApiKeyPairPermission ... apiKeyPairPermissions) throws PermissionDeniedException { if (!isEnabled()) { return true; } Account account = _accountService.getAccount(user.getAccountId()); - return checkAccess(account, apiCommandName, apiKeyPairPermissions); + return checkAccess(account, apiCommandName, keyPair, apiKeyPairPermissions); } @Override - public boolean checkAccess(Account account, String commandName, ApiKeyPairPermission ... apiKeyPairPermissions) { + public boolean checkAccess(Account account, String commandName, ApiKeyPair keyPair, ApiKeyPairPermission ... apiKeyPairPermissions) { Long accountId = account.getAccountId(); if (_accountService.isRootAdmin(accountId)) { logger.info(String.format("Account [%s] is Root Admin, in this case, API limit does not apply.", diff --git a/plugins/api/rate-limit/src/test/java/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java b/plugins/api/rate-limit/src/test/java/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java index 6bfd201253ff..5687de29d40f 100644 --- a/plugins/api/rate-limit/src/test/java/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java +++ b/plugins/api/rate-limit/src/test/java/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java @@ -89,7 +89,7 @@ private User createFakeUser() { private boolean isUnderLimit(User key) { try { - s_limitService.checkAccess(key, null); + s_limitService.checkAccess(key, null, null); return true; } catch (RequestLimitException ex) { return false; diff --git a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java index a0b1973fd8e8..0fb4f2be063d 100644 --- a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java +++ b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java @@ -632,4 +632,9 @@ public User getOneActiveUserForAccount(Account account) { public Account getAccountByUuid(String accountUuid) { return null; } + + @Override + public List getAllExplicitKeyPairPermissions(Long keyPairId) { + return null; + } } diff --git a/server/src/main/java/com/cloud/api/ApiServer.java b/server/src/main/java/com/cloud/api/ApiServer.java index 4f35ccf05736..a8b24a3113fe 100644 --- a/server/src/main/java/com/cloud/api/ApiServer.java +++ b/server/src/main/java/com/cloud/api/ApiServer.java @@ -1007,7 +1007,7 @@ public boolean verifyRequest(final Map requestParameters, fina // if userId not null, that mean that user is logged in if (userId != null) { final User user = ApiDBUtils.findUserById(userId); - return commandAvailable(remoteAddress, commandName, user); + return commandAvailable(remoteAddress, commandName, user, null); } else { if (commandName.equalsIgnoreCase(ListGuiThemesCmd.class.getAnnotation(APICommand.class).name())) { return true; @@ -1120,7 +1120,7 @@ public boolean verifyRequest(final Map requestParameters, fina return false; } - if (!commandAvailable(remoteAddress, commandName, user)) { + if (!commandAvailable(remoteAddress, commandName, user, null)) { return false; } @@ -1156,7 +1156,7 @@ public boolean verifyRequest(final Map requestParameters, fina CallContext.register(user, account); List keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(keyPair.getId(), account.getRoleId()); - if (commandAvailable(remoteAddress, commandName, user, keyPairPermissions.toArray(new ApiKeyPairPermission[0]))) { + if (commandAvailable(remoteAddress, commandName, user, keyPair, keyPairPermissions.toArray(new ApiKeyPairPermission[0]))) { logger.info("API accessed through API Key Pair. API Key: [{}].", keyPair.getApiKey()); return true; } @@ -1170,9 +1170,9 @@ public boolean verifyRequest(final Map requestParameters, fina return false; } - private boolean commandAvailable(final InetAddress remoteAddress, final String commandName, final User user, ApiKeyPairPermission... rolePermissions) { + private boolean commandAvailable(final InetAddress remoteAddress, final String commandName, final User user, ApiKeyPair keyPair, ApiKeyPairPermission... rolePermissions) { try { - checkCommandAvailable(user, commandName, remoteAddress, rolePermissions); + checkCommandAvailable(user, commandName, remoteAddress, keyPair, rolePermissions); } catch (final RequestLimitException ex) { logger.debug(ex.getMessage()); throw new ServerApiException(ApiErrorCode.API_LIMIT_EXCEED, ex.getMessage()); @@ -1465,7 +1465,7 @@ public String getDomainId(Map params) { return domainIdArr[0]; } - private void checkCommandAvailable(final User user, final String commandName, final InetAddress remoteAddress, ApiKeyPairPermission ... apiKeyPairPermissions) throws PermissionDeniedException { + private void checkCommandAvailable(final User user, final String commandName, final InetAddress remoteAddress, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) throws PermissionDeniedException { if (user == null) { throw new PermissionDeniedException("User is null for role based API access check for command" + commandName); } @@ -1483,7 +1483,7 @@ private void checkCommandAvailable(final User user, final String commandName, fi } for (final APIChecker apiChecker : apiAccessCheckers) { - apiChecker.checkAccess(user, commandName, apiKeyPairPermissions); + apiChecker.checkAccess(user, commandName, keyPair, apiKeyPairPermissions); } } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index db9c1d1dafde..b34be6cd5b5d 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -45,10 +45,12 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; +import com.cloud.serializer.GsonHelper; import com.cloud.user.dao.AccountDao; import com.cloud.user.dao.SSHKeyPairDao; import com.cloud.user.dao.UserAccountDao; import com.cloud.user.dao.UserDao; +import com.google.gson.reflect.TypeToken; import org.apache.cloudstack.acl.APIChecker; import org.apache.cloudstack.acl.ApiKeyPairManagerImpl; import org.apache.cloudstack.acl.ApiKeyPairPermissionVO; @@ -101,6 +103,7 @@ import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; +import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO; import org.apache.cloudstack.framework.messagebus.MessageBus; import org.apache.cloudstack.framework.messagebus.PublishScope; import org.apache.cloudstack.kms.KMSManager; @@ -1501,7 +1504,7 @@ protected void checkRoleEscalation(Account caller, Account requested) { List apiCheckers = getEnabledApiCheckers(); for (String command : apiNameList) { try { - checkApiAccess(apiCheckers, requested, command); + checkApiAccess(apiCheckers, requested, command, null); } catch (PermissionDeniedException pde) { if (logger.isTraceEnabled()) { logger.trace(String.format( @@ -1520,7 +1523,7 @@ protected void checkRoleEscalation(Account caller, Account requested) { logger.trace(String.format("permission to \"%s\" is requested", command)); } - checkApiAccess(apiCheckers, caller, command); + checkApiAccess(apiCheckers, caller, command, null); } catch (PermissionDeniedException pde) { String msg = String.format("User of Account %s and domain %s can not create an account with access to more privileges they have themself.", caller, _domainMgr.getDomain(caller.getDomainId())); @@ -1530,9 +1533,9 @@ protected void checkRoleEscalation(Account caller, Account requested) { } } - private void checkApiAccess(List apiCheckers, Account caller, String command, ApiKeyPairPermission... apiKeyPairPermissions) { + private void checkApiAccess(List apiCheckers, Account caller, String command, ApiKeyPair keyPair, ApiKeyPairPermission... apiKeyPairPermissions) { for (final APIChecker apiChecker : apiCheckers) { - apiChecker.checkAccess(caller, command, apiKeyPairPermissions); + apiChecker.checkAccess(caller, command, keyPair, apiKeyPairPermissions); } } @@ -1541,20 +1544,22 @@ public void checkApiAccess(Account caller, String command, String apiKey) { List apiCheckers = getEnabledApiCheckers(); List keyPairPermissions = new ArrayList<>(); + ApiKeyPair keyPair = null; if (apiKey != null) { Ternary keyPairTernary = findUserByApiKey(apiKey); if (keyPairTernary != null) { keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(keyPairTernary.third().getId(), caller.getRoleId()); + keyPair = keyPairTernary.third(); } } - checkApiAccess(apiCheckers, caller, command, keyPairPermissions.toArray(new ApiKeyPairPermission[0])); + checkApiAccess(apiCheckers, caller, command, keyPair, keyPairPermissions.toArray(new ApiKeyPairPermission[0])); } @Override public void checkApiAccess(Account caller, String command) { List apiCheckers = getEnabledApiCheckers(); - checkApiAccess(apiCheckers, caller, command); + checkApiAccess(apiCheckers, caller, command, null); } @NotNull @@ -3367,24 +3372,29 @@ private Boolean isAccessingKeypairSuperset(ApiKeyPair accessedKeyPair, BaseCmd c @Override public String getAccessingApiKey(BaseCmd cmd) { try { - if (cmd instanceof BaseAsyncCmd && ((BaseAsyncCmd) cmd).getJob().toString().contains("\"signature\"")) { - return parseApiKeyFromAsyncJob((BaseAsyncCmd) cmd); + Map requestPayload = cmd.getFullUrlParams(); + + if (cmd instanceof BaseAsyncCmd && ((BaseAsyncCmd) cmd).getJob() instanceof AsyncJobVO) { + String asyncJobPayload = ((AsyncJobVO) ((BaseAsyncCmd) cmd).getJob()).getCmdInfo(); + requestPayload = GsonHelper.getGson().fromJson(asyncJobPayload, new TypeToken>() {}.getType()); } - boolean accessedByApiKey = cmd.getFullUrlParams().containsKey(ApiConstants.SIGNATURE); - String accessingApiKey = cmd.getFullUrlParams().get("apiKey"); + + boolean accessedByApiKey = requestPayload.keySet().stream().anyMatch(ApiConstants.SIGNATURE::equalsIgnoreCase); if (accessedByApiKey) { - return accessingApiKey; + String apiKey = requestPayload.entrySet().stream() + .filter(e -> ApiConstants.API_KEY.equalsIgnoreCase(e.getKey())) + .map(Map.Entry::getValue).findFirst().orElse(null); + if (apiKey != null) { + logger.info("Request's API key is [{}].", apiKey); + return apiKey; + } } } catch (NullPointerException e) { - logger.info("Accessing API through session."); + logger.warn("Unable to identify request API key due to: {}.", e); } - return null; - } - private String parseApiKeyFromAsyncJob(BaseAsyncCmd cmd) { - String jobString = cmd.getJob().toString(); - int indexOfApiKey = jobString.indexOf("apiKey") + 9; - return jobString.substring(indexOfApiKey, jobString.indexOf("\"", indexOfApiKey)); + logger.info("Request's signature or API key were not identified; assuming it has been authenticated via session."); + return null; } private Boolean isApiKeySupersetOfPermission(List baseKeyPairPermissions, List comparedPermissions) { @@ -3434,8 +3444,13 @@ public void deleteApiKey(ApiKeyPair keyPair) { internalDeleteApiKey(keyPair); } + @Override + public List getAllExplicitKeyPairPermissions(Long keyPairId) { + return apiKeyPairPermissionsDao.findAllByApiKeyPairId(keyPairId); + } + private void internalDeleteApiKey(ApiKeyPair keyPair) { - List permissions = apiKeyPairPermissionsDao.findAllByApiKeyPairId(keyPair.getId()); + List permissions = getAllExplicitKeyPairPermissions(keyPair.getId()); for (ApiKeyPairPermission permission : permissions) { apiKeyPairPermissionsDao.remove(permission.getId()); } @@ -3603,6 +3618,14 @@ private ApiKeyPairVO validateAndPersistKeyPairAndPermissions(Account account, Ap permissions.add(new ApiKeyPairPermissionVO(0, rule, rulePermission, ruleDescription)); } + if (permissions.isEmpty() && accessingApiKey != null && doesKeyPairHaveExplicitPermissions(accessingApiKey)) { + logger.debug("No rules were specified for the new API key pair. Since the accessing API key [{}]" + + " has explicit permissions, these permissions will be defined as the rule set for the new pair.", accessingApiKey); + permissions = allPermissions.stream().map(permission -> ( + new ApiKeyPairPermissionVO(0, permission.getRule().getRuleString(), permission.getPermission(), permission.getDescription()) + )).collect(Collectors.toList()); + } + if (!isApiKeySupersetOfPermission(allPermissions, permissions)) { throw new InvalidParameterValueException(String.format("The key pair being created has a bigger set of permissions than the account [%s] " + "that owns it. This is not allowed.", account.getUuid())); @@ -3617,6 +3640,16 @@ private ApiKeyPairVO validateAndPersistKeyPairAndPermissions(Account account, Ap return savedApiKeyPair; } + private boolean doesKeyPairHaveExplicitPermissions(String apiKey) { + ApiKeyPair apiKeyPair = keyPairManager.findByApiKey(apiKey); + if (apiKeyPair == null) { + logger.info("Unable to find API key pair entity with the API key [{}].", apiKey); + return false; + } + + return !getAllExplicitKeyPairPermissions(apiKeyPair.getId()).isEmpty(); + } + @Override public List getAllKeypairPermissions(String apiKey) { if (apiKey == null) { diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index b3bc69835ff5..ab76071732a3 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3683,7 +3683,9 @@ public UserVm destroyVm(DestroyVMCmd cmd, boolean checkExpunge) throws ResourceU if (checkExpunge && expunge) { String jobParamsString = ((AsyncJobVO) cmd.getJob()).getCmdInfo(); HashMap jobParams = GsonHelper.getGson().fromJson(jobParamsString, jobParamsType); - String apiKey = jobParams.get("apiKey"); + String apiKey = jobParams.entrySet().stream() + .filter(e -> ApiConstants.API_KEY.equalsIgnoreCase(e.getKey())) + .map(Map.Entry::getValue).findFirst().orElse(null); checkExpungeVmPermission(ctx.getCallingAccount(), apiKey); }