From 787f261024c5f923e8164d96e8ec013affe912c3 Mon Sep 17 00:00:00 2001 From: Rene Moser Date: Wed, 9 Sep 2026 23:28:19 +0200 Subject: [PATCH] Fix NPE on getUserKeys when accessed via the integration API port Requests sent to the integration API port (integration.api.port, 8096 by default) are not signature-checked and run as the system user. Clients such as the cs CLI still send an apikey and signature parameter, so getAccessingApiKey() picked up an API key that was never used to authenticate the request. Looking that key up returned null and getUserKeys, listUserKeys, registerUserKeys and listApis failed with a NullPointerException. Only treat the request's API key as the accessing key pair when it maps to a key pair owned by the calling user. For the system user (integration API port) a mismatching key is ignored and the caller's role permissions apply, as for session-authenticated requests. For any other caller a mismatching key is rejected with a PermissionDeniedException, since the request cannot have been authenticated through it. Also guard the key pair lookups against null so an unknown key can no longer cause a NullPointerException. --- .../com/cloud/user/AccountManagerImpl.java | 49 ++++++++-- .../cloud/user/AccountManagerImplTest.java | 96 +++++++++++++++++++ 2 files changed, 138 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index c06228e44f9d..82eff78bffa2 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -3315,15 +3315,14 @@ public Pair> getKeys(GetUserKeysCmd cmd) { verifyCallerPrivilegeForUserOrAccountOperations(user); String accessingApiKey = getAccessingApiKey(cmd); - ApiKeyPair keyPair; + ApiKeyPair keyPair = null; if (accessingApiKey != null) { ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(accessingApiKey); - if (userId == accessingKeyPair.getUserId()) { - keyPair = apiKeyPairService.findByApiKey(accessingApiKey); - } else { - keyPair = _accountService.getLatestUserKeyPair(userId); + if (accessingKeyPair != null && userId == accessingKeyPair.getUserId()) { + keyPair = accessingKeyPair; } - } else { + } + if (keyPair == null) { keyPair = _accountService.getLatestUserKeyPair(userId); } @@ -3436,6 +3435,10 @@ private Boolean isAccessingKeypairSuperset(ApiKeyPair accessedKeyPair, BaseCmd c return Boolean.TRUE; } ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(apiKey); + if (accessingKeyPair == null) { + logger.info("Unable to find the API key pair used to access the API; therefore, its permissions cannot be verified."); + return Boolean.FALSE; + } return isApiKeySupersetOfPermission(new ArrayList<>(getAllKeypairPermissions(accessingKeyPair.getApiKey())), new ArrayList<>(getAllKeypairPermissions(accessedKeyPair.getApiKey()))); } @@ -3454,7 +3457,7 @@ public String getAccessingApiKey(BaseCmd cmd) { String apiKey = requestPayload.entrySet().stream() .filter(e -> ApiConstants.API_KEY.equalsIgnoreCase(e.getKey())) .map(Map.Entry::getValue).findFirst().orElse(null); - if (apiKey != null) { + if (apiKey != null && isApiKeyOwnedByCallingUser(apiKey)) { logger.info("Request's API key is [{}].", apiKey); return apiKey; } @@ -3467,6 +3470,35 @@ public String getAccessingApiKey(BaseCmd cmd) { return null; } + /** + * Checks whether the API key present in the request belongs to the calling user. When a request is authenticated through + * an API key pair, the calling user is always the owner of that key pair, so a request whose API key does not map to a + * key pair of the calling user was not authenticated through it. + *

+ * Requests sent to the integration API port (see {@code integration.api.port}) are not signature-checked and run under the + * system user, so any API key and signature they carry are ignored and the caller's role permissions apply instead. + * For any other caller such a request is rejected: on the regular API port the signature is only verified when there is no + * authenticated session, so a mismatching API key can only be the result of a tampered request, and honoring it would + * derive permissions from a key pair that the caller did not prove ownership of. + * + * @throws PermissionDeniedException if the calling user is not the system user and the API key does not belong to them. + */ + protected boolean isApiKeyOwnedByCallingUser(String apiKey) { + long callingUserId = CallContext.current().getCallingUserId(); + ApiKeyPair keyPair = apiKeyPairService.findByApiKey(apiKey); + if (keyPair != null && Long.valueOf(callingUserId).equals(keyPair.getUserId())) { + return true; + } + String keyPairDescription = keyPair == null ? "an API key that does not map to any API key pair" : + String.format("API key pair [%s] which belongs to user with ID [%s]", keyPair.getUuid(), keyPair.getUserId()); + if (callingUserId == User.UID_SYSTEM) { + logger.debug("Request made by the system user (e.g. through the integration API port) contains {}; ignoring it.", keyPairDescription); + return false; + } + logger.warn("Request made by user with ID [{}] contains {}; rejecting the request.", callingUserId, keyPairDescription); + throw new PermissionDeniedException("The API key present in the request does not belong to the calling user."); + } + private Boolean isApiKeySupersetOfPermission(List baseKeyPairPermissions, List comparedPermissions) { Map apiNameToBaseKeyPermissions = roleService.getRoleRulesAndPermissions(baseKeyPairPermissions); @@ -3727,6 +3759,9 @@ public List getAllKeypairPermissions(String apiKey) { throw new InvalidParameterValueException("API key not present in the request's URL and, thus, unable to fetch API key rules."); } ApiKeyPair apiKeyPair = keyPairManager.findByApiKey(apiKey); + if (apiKeyPair == null) { + throw new InvalidParameterValueException("Unable to find an API key pair matching the API key present in the request's URL and, thus, unable to fetch API key rules."); + } Account account = _accountDao.findById(apiKeyPair.getAccountId()); List keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(apiKeyPair.getId(), account.getRoleId()); return new ArrayList<>(keyPairPermissions); diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 955c939f4fa3..05fc5ed875fa 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -2289,4 +2289,100 @@ public void testCheckRoleEscalationMultipleCheckersAppliedSequentially() throws accountManagerImpl.checkRoleEscalation(caller, requested); } + + private Map buildSignedRequestParams(String apiKey) { + Map params = new HashMap<>(); + params.put(ApiConstants.API_KEY, apiKey); + params.put(ApiConstants.SIGNATURE, "signature"); + return params; + } + + @Test + public void getAccessingApiKeyTestReturnsApiKeyWhenKeyPairBelongsToCallingUser() { + Mockito.when(callingUser.getId()).thenReturn(111L); + CallContext.register(callingUser, callingAccount); + Mockito.when(_getkeyscmd.getFullUrlParams()).thenReturn(buildSignedRequestParams("api-key")); + Mockito.when(apiKeyPairVOMock.getUserId()).thenReturn(111L); + Mockito.when(apiKeyPairService.findByApiKey("api-key")).thenReturn(apiKeyPairVOMock); + + Assert.assertEquals("api-key", accountManagerImpl.getAccessingApiKey(_getkeyscmd)); + } + + @Test(expected = PermissionDeniedException.class) + public void getAccessingApiKeyTestThrowsWhenKeyPairBelongsToAnotherUser() { + Mockito.when(callingUser.getId()).thenReturn(111L); + CallContext.register(callingUser, callingAccount); + Mockito.when(_getkeyscmd.getFullUrlParams()).thenReturn(buildSignedRequestParams("api-key")); + Mockito.when(apiKeyPairVOMock.getUserId()).thenReturn(222L); + Mockito.when(apiKeyPairService.findByApiKey("api-key")).thenReturn(apiKeyPairVOMock); + + accountManagerImpl.getAccessingApiKey(_getkeyscmd); + } + + @Test(expected = PermissionDeniedException.class) + public void getAccessingApiKeyTestThrowsWhenKeyPairDoesNotExist() { + Mockito.when(callingUser.getId()).thenReturn(111L); + CallContext.register(callingUser, callingAccount); + Mockito.when(_getkeyscmd.getFullUrlParams()).thenReturn(buildSignedRequestParams("dummy")); + Mockito.when(apiKeyPairService.findByApiKey("dummy")).thenReturn(null); + + accountManagerImpl.getAccessingApiKey(_getkeyscmd); + } + + @Test + public void getAccessingApiKeyTestReturnsNullWhenKeyPairBelongsToAnotherUserAndCallerIsSystemUser() { + Mockito.when(callingUser.getId()).thenReturn(User.UID_SYSTEM); + CallContext.register(callingUser, callingAccount); + Mockito.when(_getkeyscmd.getFullUrlParams()).thenReturn(buildSignedRequestParams("api-key")); + Mockito.when(apiKeyPairVOMock.getUserId()).thenReturn(222L); + Mockito.when(apiKeyPairService.findByApiKey("api-key")).thenReturn(apiKeyPairVOMock); + + Assert.assertNull(accountManagerImpl.getAccessingApiKey(_getkeyscmd)); + } + + @Test + public void getAccessingApiKeyTestReturnsNullWhenKeyPairDoesNotExistAndCallerIsSystemUser() { + Mockito.when(callingUser.getId()).thenReturn(User.UID_SYSTEM); + CallContext.register(callingUser, callingAccount); + Mockito.when(_getkeyscmd.getFullUrlParams()).thenReturn(buildSignedRequestParams("dummy")); + Mockito.when(apiKeyPairService.findByApiKey("dummy")).thenReturn(null); + + Assert.assertNull(accountManagerImpl.getAccessingApiKey(_getkeyscmd)); + } + + @Test + public void getAccessingApiKeyTestReturnsNullWhenRequestIsNotSigned() { + Map params = new HashMap<>(); + params.put(ApiConstants.API_KEY, "api-key"); + Mockito.when(_getkeyscmd.getFullUrlParams()).thenReturn(params); + + Assert.assertNull(accountManagerImpl.getAccessingApiKey(_getkeyscmd)); + Mockito.verify(apiKeyPairService, Mockito.never()).findByApiKey(Mockito.anyString()); + } + + @Test + public void getKeysTestReturnsLatestKeyPairWhenRequestApiKeyIsNotVerified() { + // Requests through the integration API port run as the system user and are not signature-checked, so the + // API key and signature they may carry must not be used to derive permissions. + Mockito.when(callingUser.getId()).thenReturn(User.UID_SYSTEM); + CallContext.register(callingUser, callingAccount); + long userId = 2L; + Mockito.when(_getkeyscmd.getId()).thenReturn(userId); + Mockito.when(_getkeyscmd.getFullUrlParams()).thenReturn(buildSignedRequestParams("dummy")); + Mockito.doReturn(userVoMock).when(accountManagerImpl).getActiveUser(userId); + Mockito.when(userVoMock.getApiKeyAccess()).thenReturn(Boolean.TRUE); + Mockito.when(_accountDao.findByIdIncludingRemoved(accountMockId)).thenReturn(callingAccount); + Mockito.doNothing().when(accountManagerImpl).checkAccess(Mockito.any(User.class), Mockito.any(ControlledEntity.class)); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations(Mockito.any(User.class)); + Mockito.when(apiKeyPairService.findByApiKey("dummy")).thenReturn(null); + Mockito.when(apiKeyPairVOMock.getApiKey()).thenReturn("latest-api-key"); + Mockito.when(apiKeyPairVOMock.getSecretKey()).thenReturn("latest-secret-key"); + Mockito.when(_accountService.getLatestUserKeyPair(userId)).thenReturn(apiKeyPairVOMock); + + Pair> result = accountManagerImpl.getKeys(_getkeyscmd); + + Assert.assertTrue(result.first()); + Assert.assertEquals("latest-api-key", result.second().get("apikey")); + Assert.assertEquals("latest-secret-key", result.second().get("secretkey")); + } }