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.

Author: resmo

server/src/main/java/com/cloud/user/AccountManagerImpl.java

@@ -3315,15 +3315,14 @@ public Pair<Boolean, Map<String, String>> 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.
+     * <p>
+     * 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<RolePermissionEntity> baseKeyPairPermissions, List<RolePermissionEntity> comparedPermissions) {
         Map<String, RolePermissionEntity> apiNameToBaseKeyPermissions = roleService.getRoleRulesAndPermissions(baseKeyPairPermissions);
 
@@ -3727,6 +3759,9 @@ public List<RolePermissionEntity> 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<ApiKeyPairPermission> keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(apiKeyPair.getId(), account.getRoleId());
         return new ArrayList<>(keyPairPermissions);

server/src/test/java/com/cloud/user/AccountManagerImplTest.java

@@ -2289,4 +2289,100 @@ public void testCheckRoleEscalationMultipleCheckersAppliedSequentially() throws
 
         accountManagerImpl.checkRoleEscalation(caller, requested);
     }
+
+    private Map<String, String> buildSignedRequestParams(String apiKey) {
+        Map<String, String> 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<String, String> 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<Boolean, Map<String, String>> 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"));
+    }
 }