diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmd.java index 460b8d642e90..c24d61b7200b 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmd.java @@ -32,7 +32,7 @@ import java.util.Map; @APICommand(name = "addObjectStoragePool", description = "Adds a object storage pool", responseObject = ObjectStoreResponse.class, since = "4.19.0", - requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) + requestHasSensitiveInfo = true, responseHasSensitiveInfo = false) public class AddObjectStoragePoolCmd extends BaseCmd { ///////////////////////////////////////////////////// diff --git a/api/src/test/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmdTest.java index c7aeb8ba99bf..1d0c3e6673e8 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmdTest.java @@ -20,6 +20,7 @@ import com.cloud.exception.DiscoveryException; import com.cloud.storage.StorageService; +import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ResponseGenerator; import org.apache.cloudstack.api.response.ObjectStoreResponse; import org.apache.cloudstack.context.CallContext; @@ -38,6 +39,8 @@ import java.util.HashMap; import java.util.Map; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; @RunWith(MockitoJUnitRunner.class) @@ -98,4 +101,38 @@ public void testAddObjectStore() throws DiscoveryException { Mockito.verify(storageService, Mockito.times(1)) .discoverObjectStore(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); } + + @Test + public void testRequestIsMarkedAsContainingSensitiveInformation() { + APICommand apiCommand = AddObjectStoragePoolCmd.class.getAnnotation(APICommand.class); + + assertNotNull(apiCommand); + assertTrue(apiCommand.requestHasSensitiveInfo()); + } + + @Test + public void testCredentialsAndProviderOptionsReachDiscoveryUnchanged() throws DiscoveryException { + Map expected = new HashMap<>(); + expected.put("accesskey", "ACCESS_SENTINEL"); + expected.put("secretkey", "SECRET+key&with=symbols"); + expected.put("validateSSL", "true"); + expected.put("s3Url", "https://s3.example.test"); + expected.put("iamUrl", "https://iam.example.test"); + Map> indexedDetails = new HashMap<>(); + int index = 0; + for (Map.Entry entry : expected.entrySet()) { + Map detail = new HashMap<>(); + detail.put("key", entry.getKey()); + detail.put("value", entry.getValue()); + indexedDetails.put(index++, detail); + } + ReflectionTestUtils.setField(addObjectStoragePoolCmdSpy, "details", indexedDetails); + Mockito.doReturn(objectStore).when(storageService).discoverObjectStore(Mockito.anyString(), + Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), any()); + Mockito.doReturn(new ObjectStoreResponse()).when(responseGenerator).createObjectStoreResponse(any()); + + addObjectStoragePoolCmdSpy.execute(); + + Mockito.verify(storageService).discoverObjectStore(name, url, size, provider, expected); + } } diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index 3ac5bbb01a75..99162018c4e1 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -20,6 +20,7 @@ import java.net.InetAddress; import java.net.URLDecoder; import java.net.UnknownHostException; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; @@ -85,6 +86,8 @@ public class ApiServlet extends HttpServlet { private static final Logger ACCESSLOGGER = LogManager.getLogger("apiserver." + ApiServlet.class.getName()); private static final String REPLACEMENT = "_"; private static final String LOGGER_REPLACEMENTS = "[\n\r\t]"; + private static final Pattern SENSITIVE_QUERY_PARAMETER = Pattern.compile( + ".*(?:password|privatekey|accesskey|secretkey)|sessionkey|secretcode|token|userdata", Pattern.DOTALL); private static final Pattern GET_REQUEST_COMMANDS = Pattern.compile("^(get|list|query|find)(\\w+)+$"); private static final HashSet GET_REQUEST_COMMANDS_LIST = new HashSet<>(Set.of("isaccountallowedtocreateofferingswithtags", "readyforshutdown", "cloudianisenabled", "quotabalance", "quotasummary", "quotatarifflist", "quotaisenabled", "quotastatement", "verifyoauthcodeandgetuser")); @@ -100,6 +103,7 @@ public class ApiServlet extends HttpServlet { "updaterolepermission", "updateprojectrolepermission", "createstoragepool", + "addobjectstoragepool", "addhost", "updatehostpassword", "addcluster", @@ -172,7 +176,7 @@ void utf8Fixup(final HttpServletRequest req, final Map params) String value = decodeUtf8(paramTokens[1]); params.put(name, new String[] {value}); } else { - LOGGER.debug("Invalid parameter in URL found. param: " + param); + LOGGER.debug("Invalid parameter in URL found (missing equals sign)."); } } } @@ -199,8 +203,8 @@ public void run() { private void checkSingleQueryParameterValue(Map params) { params.forEach((k, v) -> { if (v.length > 1) { - String message = String.format("Query parameter '%s' has multiple values %s. Only the last value will be respected." + - "It is advised to pass only a single parameter", k, Arrays.toString(v)); + String message = String.format("Query parameter '%s' has %d values. Only the last value will be respected. " + + "It is advised to pass only a single parameter", saveLogString(k), v.length); LOGGER.warn(message); } }); @@ -237,17 +241,15 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp // logging the request start and end in management log for easy debugging String reqStr = ""; - String cleanQueryString = StringUtils.cleanString(req.getQueryString()); + String cleanQueryString = getCleanQueryString(command, req.getQueryString(), reqParams); if (LOGGER.isDebugEnabled()) { reqStr = auditTrailSb.toString() + " " + cleanQueryString; if (req.getMethod().equalsIgnoreCase("POST") && org.apache.commons.lang3.StringUtils.isNotBlank(command)) { - if (!POST_REQUESTS_TO_DISABLE_LOGGING.contains(command.toLowerCase()) && !reqParams.containsKey(ApiConstants.USER_DATA)) { + if (shouldLogRequestParameters(command, reqParams)) { String cleanParamsString = getCleanParamsString(reqParams); if (org.apache.commons.lang3.StringUtils.isNotBlank(cleanParamsString)) { reqStr += "\n" + cleanParamsString; } - } else { - reqStr += " " + command; } } LOGGER.debug("===START=== " + reqStr); @@ -588,7 +590,7 @@ protected void setClientAddressForConsoleEndpointAccess(String command, Map reqParams) { return cleanParamsString.toString(); } + + protected boolean shouldLogRequestParameters(String command, Map reqParams) { + return (org.apache.commons.lang3.StringUtils.isBlank(command) + || !POST_REQUESTS_TO_DISABLE_LOGGING.contains(command.toLowerCase(java.util.Locale.ROOT))) + && !reqParams.containsKey(ApiConstants.USER_DATA); + } + + protected String getCleanQueryString(String command, String queryString, Map reqParams) { + boolean sensitiveRequest = !shouldLogRequestParameters(command, reqParams); + boolean includeCommand = sensitiveRequest && org.apache.commons.lang3.StringUtils.isNotBlank(command); + if (org.apache.commons.lang3.StringUtils.isEmpty(queryString)) { + return includeCommand ? "command=" + saveLogString(command) : ""; + } + + List cleanParameters = new ArrayList<>(); + try { + // Split before decoding so an encoded ampersand remains part of its original value. + // Read the query only: reqParams also contains the POST body. + for (String parameter : queryString.split("&", -1)) { + int separator = parameter.indexOf('='); + if (separator < 0) { + continue; + } + String name = decodeUtf8(parameter.substring(0, separator)); + if (ApiConstants.COMMAND.equals(name)) { + includeCommand = false; + } + String lowerName = name.toLowerCase(java.util.Locale.ROOT); + if (SENSITIVE_QUERY_PARAMETER.matcher(lowerName).matches()) { + continue; + } + // Sensitive requests can carry credentials in indexed maps. Omit those entries + // rather than trust descriptors that may be in the POST body or use different case. + if (sensitiveRequest && (name.indexOf('[') >= 0 || ApiConstants.DETAILS.equals(lowerName))) { + continue; + } + cleanParameters.add(parameter); + } + } catch (IllegalArgumentException e) { + // Formatting must not expose an undecodable query or change request processing. + return includeCommand ? "command=" + saveLogString(command) : ""; + } + if (includeCommand) { + cleanParameters.add(0, "command=" + saveLogString(command)); + } + // Preserve the existing cleaning of nested URLs and JSON in the retained query fields. + return StringUtils.cleanString(String.join("&", cleanParameters)); + } } diff --git a/server/src/test/java/com/cloud/api/ApiServletTest.java b/server/src/test/java/com/cloud/api/ApiServletTest.java index 79fe4b86f859..367bba9f3c15 100644 --- a/server/src/test/java/com/cloud/api/ApiServletTest.java +++ b/server/src/test/java/com/cloud/api/ApiServletTest.java @@ -26,19 +26,32 @@ import com.cloud.user.User; import com.cloud.user.UserAccount; import com.cloud.utils.HttpUtils; +import com.cloud.utils.StringUtils; +import com.cloud.api.dispatch.DispatchTask; +import com.cloud.api.dispatch.ParamUnpackWorker; import com.cloud.vm.UserVmManager; import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.command.admin.storage.AddImageStoreCmd; +import org.apache.cloudstack.api.command.admin.storage.AddObjectStoragePoolCmd; import org.apache.cloudstack.api.auth.APIAuthenticationManager; import org.apache.cloudstack.api.auth.APIAuthenticationType; import org.apache.cloudstack.api.auth.APIAuthenticator; import org.apache.cloudstack.api.command.admin.config.ListCfgsByCmd; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.apache.logging.log4j.core.config.Property; import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; @@ -52,9 +65,14 @@ import java.io.UnsupportedEncodingException; import java.lang.reflect.Field; import java.net.InetAddress; +import java.net.URLDecoder; import java.net.URLEncoder; import java.net.UnknownHostException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; import static org.mockito.ArgumentMatchers.nullable; @@ -461,4 +479,348 @@ public void testVerify2FAWhenExpectedCommandIsNotCalled() throws UnknownHostExce Assert.assertEquals(false, result); } + + @Test + public void preservesPostParameterLoggingPolicy() { + for (String command : new String[] {"addObjectStoragePool", "addHost", "createAccount"}) { + Assert.assertFalse(servlet.shouldLogRequestParameters(command, new HashMap<>())); + } + Assert.assertTrue(servlet.shouldLogRequestParameters("listZones", new HashMap<>())); + Map params = new HashMap<>(); + params.put(ApiConstants.USER_DATA, new String[] {"USER_DATA_SENTINEL"}); + Assert.assertFalse(servlet.shouldLogRequestParameters("deployVirtualMachine", params)); + } + + @Test + public void preservesAddHostQueryContextInAllRequestLogs() { + String query = "command=addHost&zoneid=zone-1&username=test-user&password=PASSWORD_HEAD%26PASSWORD_TAIL&response=json"; + List logs = captureRequestLogs("GET", query, queryParameters(query), true, false, HttpServletResponse.SC_OK); + assertRequestLogs(logs, "command=addHost&zoneid=zone-1&username=test-user&response=json", "PASSWORD_HEAD", "PASSWORD_TAIL"); + Mockito.verify(apiServer).handleRequest(Mockito.anyMap(), Mockito.eq("json"), Mockito.any(StringBuilder.class)); + } + + @Test + public void preservesCreateAccountQueryContextInAllRequestLogs() { + String query = "command=createAccount&username=test-user&account=test-account&password=PASSWORD_SENTINEL&response=json"; + List logs = captureRequestLogs("GET", query, queryParameters(query), true, false, HttpServletResponse.SC_OK); + assertRequestLogs(logs, "command=createAccount&username=test-user&account=test-account&response=json", "PASSWORD_SENTINEL"); + } + + @Test + @SuppressWarnings({"unchecked", "rawtypes"}) + public void redactsUserDataWithoutRemovingItsQueryContextOrChangingTheRequest() { + String query = "command=deployVirtualMachine&id=vm-1&userdata=USER_DATA_HEAD%26USER_DATA_TAIL&response=json"; + List logs = captureRequestLogs("GET", query, queryParameters(query), true, false, HttpServletResponse.SC_OK); + assertRequestLogs(logs, "command=deployVirtualMachine&id=vm-1&response=json", "USER_DATA_HEAD", "USER_DATA_TAIL"); + ArgumentCaptor captured = ArgumentCaptor.forClass(Map.class); + Mockito.verify(apiServer).handleRequest(captured.capture(), Mockito.eq("json"), Mockito.any(StringBuilder.class)); + Assert.assertArrayEquals(new String[] {"USER_DATA_HEAD&USER_DATA_TAIL"}, (Object[]) captured.getValue().get(ApiConstants.USER_DATA)); + } + + @Test + public void preservesPostEnforcementAndRedactsRejectedObjectStoreGet() { + String query = "command=addObjectStoragePool&name=store-1&provider=MinIO&details[0].key=accesskey" + + "&details[0].value=ACCESS_SENTINEL&details[1].key=secretkey" + + "&details[1].value=SECRET_HEAD%2B%26SECRET_TAIL%3D&response=json"; + List logs = captureRequestLogs("GET", query, queryParameters(query), false, true, HttpServletResponse.SC_BAD_REQUEST); + assertRequestLogs(logs, "command=addObjectStoragePool&name=store-1&provider=MinIO&response=json", "ACCESS_SENTINEL", "SECRET_HEAD", "SECRET_TAIL"); + Mockito.verify(apiServer, Mockito.never()).verifyRequest(Mockito.anyMap(), nullable(Long.class), Mockito.any(InetAddress.class)); + Mockito.verify(apiServer, Mockito.never()).handleRequest(Mockito.anyMap(), Mockito.anyString(), Mockito.any(StringBuilder.class)); + } + + @Test + @SuppressWarnings({"unchecked", "rawtypes"}) + public void acceptsObjectStorePostWithoutLoggingOrChangingItsBody() { + Map params = queryParameters("command=addObjectStoragePool&name=store-1&provider=MinIO&response=json"); + params.put("url", new String[] {"https://storage.example.test"}); + params.put("details[0].key", new String[] {"accesskey"}); + params.put("details[0].value", new String[] {"BODY_ACCESS_SENTINEL"}); + params.put("details[1].key", new String[] {"secretkey"}); + params.put("details[1].value", new String[] {"BODY_SECRET+&=SENTINEL"}); + String responseBody = "{\"addobjectstoragepoolresponse\":{\"objectstore\":{\"id\":\"store-id\"}}}"; + Mockito.when(apiServer.handleRequest(Mockito.anyMap(), Mockito.eq("json"), Mockito.any(StringBuilder.class))).thenReturn(responseBody); + List logs = captureRequestLogs("POST", null, params, true, true, HttpServletResponse.SC_OK); + assertRequestLogs(logs, "command=addObjectStoragePool", "BODY_ACCESS_SENTINEL", "BODY_SECRET", "SENTINEL"); + Assert.assertEquals(responseBody, responseWriter.toString()); + ArgumentCaptor captured = ArgumentCaptor.forClass(Map.class); + Mockito.verify(apiServer).handleRequest(captured.capture(), Mockito.eq("json"), Mockito.any(StringBuilder.class)); + Assert.assertArrayEquals(new String[] {"BODY_SECRET+&=SENTINEL"}, (Object[]) captured.getValue().get("details[1].value")); + Assert.assertArrayEquals(new String[] {"POST"}, (Object[]) captured.getValue().get("httpmethod")); + } + + @Test + @SuppressWarnings({"unchecked", "rawtypes"}) + public void protectsMixedQueryAndBodyDetailsWithoutReinterpretingFieldCase() { + String query = "command=addObjectStoragePool&name=store-1&provider=MinIO&response=json" + + "&details[1].KEY=public&details[1].value=MIXED_SECRET_SENTINEL"; + Map params = queryParameters(query); + params.put("details[1].key", new String[] {"secretkey"}); + params.put("details[0].key", new String[] {"accesskey"}); + params.put("details[0].value", new String[] {"BODY_ACCESS_SENTINEL"}); + List logs = captureRequestLogs("POST", query, params, true, true, HttpServletResponse.SC_OK); + assertRequestLogs(logs, "command=addObjectStoragePool&name=store-1&provider=MinIO&response=json", "MIXED_SECRET_SENTINEL", "BODY_ACCESS_SENTINEL"); + + ArgumentCaptor captured = ArgumentCaptor.forClass(Map.class); + Mockito.verify(apiServer).handleRequest(captured.capture(), Mockito.eq("json"), Mockito.any(StringBuilder.class)); + Map commandParameters = new HashMap<>(); + Map forwarded = captured.getValue(); + forwarded.forEach((key, values) -> commandParameters.put(key, (String) values[0])); + AddObjectStoragePoolCmd command = new AddObjectStoragePoolCmd(); + DispatchTask task = new DispatchTask(command, commandParameters); + new ParamUnpackWorker().handle(task); + command.setDetails((Map) task.getParams().get(ApiConstants.DETAILS)); + Assert.assertEquals("MIXED_SECRET_SENTINEL", command.getDetails().get("secretkey")); + Assert.assertEquals("BODY_ACCESS_SENTINEL", command.getDetails().get("accesskey")); + } + + @Test + public void retainsQueryMetadataButNeverCopiesPostOnlyFieldsIntoTheAccessLog() { + String query = "name=query-store&provider=MinIO&response=json"; + Map params = queryParameters(query); + params.put(ApiConstants.COMMAND, new String[] {"addObjectStoragePool"}); + params.put("details[1].key", new String[] {"secretkey"}); + params.put("details[1].value", new String[] {"BODY_SECRET_SENTINEL"}); + params.put("tags", new String[] {"BODY_ONLY_TAG_SENTINEL"}); + List logs = captureRequestLogs("POST", query, params, true, true, HttpServletResponse.SC_OK); + assertRequestLogs(logs, "command=addObjectStoragePool&name=query-store&provider=MinIO&response=json", "BODY_SECRET_SENTINEL", "BODY_ONLY_TAG_SENTINEL"); + } + + @Test + public void preservesAuthorizationRejectionWithoutLoggingProviderDetails() { + String query = "command=addObjectStoragePool&name=store-1&provider=Cloudian%20HyperStore" + + "&details[2].key=s3Url&details[2].value=PROVIDER_DETAIL_SENTINEL&response=json"; + List logs = captureRequestLogs("POST", query, queryParameters(query), false, true, HttpServletResponse.SC_UNAUTHORIZED); + assertRequestLogs(logs, "command=addObjectStoragePool&name=store-1&provider=Cloudian%20HyperStore&response=json", "PROVIDER_DETAIL_SENTINEL"); + Mockito.verify(apiServer, Mockito.never()).handleRequest(Mockito.anyMap(), Mockito.anyString(), Mockito.any(StringBuilder.class)); + } + + @Test + @SuppressWarnings({"unchecked", "rawtypes"}) + public void doesNotPrintDuplicateCredentialValuesAndKeepsExistingLastQueryValue() { + String query = "command=addObjectStoragePool&name=store-1&details[1].key=secretkey" + + "&details[1].value=FIRST_SECRET_SENTINEL&details[1].value=LAST_SECRET_SENTINEL&response=json"; + List logs = captureRequestLogs("POST", query, queryParameters(query), true, true, HttpServletResponse.SC_OK); + assertRequestLogs(logs, "command=addObjectStoragePool&name=store-1&response=json", "FIRST_SECRET_SENTINEL", "LAST_SECRET_SENTINEL"); + Assert.assertTrue(logs.stream().anyMatch(line -> line.contains("has 2 values"))); + ArgumentCaptor captured = ArgumentCaptor.forClass(Map.class); + Mockito.verify(apiServer).handleRequest(captured.capture(), Mockito.eq("json"), Mockito.any(StringBuilder.class)); + Assert.assertArrayEquals(new String[] {"LAST_SECRET_SENTINEL"}, (Object[]) captured.getValue().get("details[1].value")); + } + + @Test + public void doesNotEchoUnstructuredQueryTokensBeforeRedaction() { + String query = "command=addObjectStoragePool&name=store-1&MALFORMED_SECRET_SENTINEL&response=json"; + List logs = captureRequestLogs("POST", query, queryParameters(query), false, false, HttpServletResponse.SC_UNAUTHORIZED); + assertRequestLogs(logs, "command=addObjectStoragePool&name=store-1&response=json", "MALFORMED_SECRET_SENTINEL"); + Assert.assertTrue(logs.stream().anyMatch(line -> line.contains("Invalid parameter in URL found"))); + } + + @Test + public void preservesBackendFailureStatusAndDescription() { + String query = "command=addObjectStoragePool&name=store-1&details[1].key=secretkey" + + "&details[1].value=ERROR_SECRET_SENTINEL&response=json"; + Mockito.when(apiServer.handleRequest(Mockito.anyMap(), Mockito.eq("json"), Mockito.any(StringBuilder.class))) + .thenThrow(new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Provider unavailable")); + List logs = captureRequestLogs("POST", query, queryParameters(query), true, true, ApiErrorCode.INTERNAL_ERROR.getHttpCode()); + assertRequestLogs(logs, "command=addObjectStoragePool&name=store-1&response=json", "ERROR_SECRET_SENTINEL"); + Mockito.verify(response).setHeader("X-Description", "Provider unavailable"); + } + + @Test + public void keepsOrdinaryGetAndPostDiagnostics() { + String query = "command=listZones&name=zone-1&response=json"; + List logs = captureRequestLogs("POST", query, queryParameters(query), true, false, HttpServletResponse.SC_OK); + assertRequestLogs(logs, query); + Assert.assertTrue(logs.stream().anyMatch(line -> line.contains("===START===") && line.contains("\n") && line.contains("name=zone-1"))); + Assert.assertEquals(query, servlet.getCleanQueryString("listZones", query, queryParameters(query))); + } + + @Test + public void excludesObjectStorageDetailsRegardlessOfEncodingOrderOrDescriptor() { + for (String fields : new String[] { + "details%5b001%5d.KEY=public&DETAILS[%2B1].value=SECRET_SENTINEL", + "details[1].value=SECRET_SENTINEL&details[1].key=secretkey", + "details[-1]suffix.VALUE=SECRET_SENTINEL", + "details[invalid].value=SECRET_SENTINEL", + "details[1].value=SECRET_SENTINEL", + "details=SECRET_SENTINEL"}) { + String query = "command=addObjectStoragePool&name=store-1&" + fields + "&response=json"; + Assert.assertEquals("command=addObjectStoragePool&name=store-1&response=json", + servlet.getCleanQueryString("addObjectStoragePool", query, new HashMap<>())); + } + } + + @Test + public void redactsCompleteSensitiveTopLevelValuesButKeepsNonSensitiveFlags() { + String query = "command=listZones&userdataid=id-1&passwordenabled=true&newPassword=PASSWORD_HEAD%26PASSWORD_TAIL" + + "&ACCESSKEY=ACCESS_SENTINEL&privatekey=PRIVATE_SENTINEL&SeSsIoNkEy=SESSION_SENTINEL%2B%3D" + + "&secretcode=CODE_SENTINEL&token=RESET_TOKEN_SENTINEL&UserData=DATA_SENTINEL&response=json"; + Assert.assertEquals("command=listZones&userdataid=id-1&passwordenabled=true&response=json", + servlet.getCleanQueryString("listZones", query, new HashMap<>())); + } + + @Test + public void keepsExistingNestedUrlCleaningAndOrdinaryDetails() { + String query = "command=addHost&zoneid=zone-1&url=cifs%3A%2F%2Fstorage%2Fshare%3Fuser%3Dtest" + + "%26password%3DPASSWORD_SENTINEL%26domain%3Dexample"; + Assert.assertEquals(StringUtils.cleanString(query), servlet.getCleanQueryString("addHost", query, new HashMap<>())); + String ordinary = "command=deployVirtualMachine&details[0].key=keyboard&details[0].value=us&response=json"; + Assert.assertEquals(ordinary, servlet.getCleanQueryString("deployVirtualMachine", ordinary, new HashMap<>())); + } + + @Test + public void cannotEchoAnUndecodableQueryAndSanitizesCommandMetadata() { + String query = "details%ZZ.value=SECRET_SENTINEL"; + Assert.assertEquals("command=addObjectStoragePool", servlet.getCleanQueryString("addObjectStoragePool", query, new HashMap<>())); + Assert.assertEquals("", servlet.getCleanQueryString("listZones", query, new HashMap<>())); + Map params = new HashMap<>(); + params.put(ApiConstants.USER_DATA, new String[] {"DATA_SENTINEL"}); + Assert.assertEquals("command=unknown_command__", servlet.getCleanQueryString("unknown\ncommand\r\t", null, params)); + } + + @Test + public void preservesContextWithoutExposingOtherSensitiveCommandsIndexedMaps() { + String[][] cases = { + {"createStoragePool", "details[0].key=secretkey&details[0].value=STORAGE_SECRET_SENTINEL%26TAIL"}, + {"createAccount", "accountdetails[0].key=secretkey&accountdetails[0].value=ACCOUNT_SECRET_SENTINEL"}, + {"addHost", "externaldetails[0].endpoint.password=HOST_SECRET_SENTINEL"} + }; + for (String[] testCase : cases) { + String query = "command=" + testCase[0] + "&name=resource-1&" + testCase[1] + "&response=json"; + Assert.assertEquals("command=" + testCase[0] + "&name=resource-1&response=json", + servlet.getCleanQueryString(testCase[0], query, new HashMap<>())); + } + String reset = "command=resetPassword&username=test-user&token=RESET_TOKEN_SENTINEL&password=PASSWORD_SENTINEL"; + Assert.assertEquals("command=resetPassword&username=test-user", + servlet.getCleanQueryString("resetPassword", reset, new HashMap<>())); + } + + @Test + public void preservesImageStoreCredentialRedactionAfterRemovingTrailingSessionKey() { + assertImageStoreQueryRemainsRedacted(false); + } + + @Test + public void preservesEncodedImageStoreCredentialRedactionAfterRemovingTrailingSessionKey() { + assertImageStoreQueryRemainsRedacted(true); + } + + @Test + public void preservesIndexedCredentialRedactionAfterFilteringOtherTrailingFields() { + String context = "command=addImageStore&provider=S3&name=store-1"; + String credentials = "&details[1].value=SECRET_SENTINEL&details[1].key=secretkey"; + for (String trailing : new String[] {"", "&sessionkey=SESSION_SENTINEL", "&password=PASSWORD_SENTINEL", + "&token=TOKEN_SENTINEL", "&privatekey=PRIVATE_SENTINEL", "&userdata=USERDATA_SENTINEL"}) { + String query = context + credentials + trailing; + String cleaned = servlet.getCleanQueryString("addImageStore", query, queryParameters(query)); + Assert.assertTrue("Missing metadata: " + cleaned, cleaned.startsWith(context)); + Assert.assertFalse("Credential survived filtering: " + cleaned, cleaned.contains("SENTINEL")); + } + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + private void assertImageStoreQueryRemainsRedacted(boolean encodedBrackets) { + String context = "command=addImageStore&provider=S3&name=store-1&response=json" + + "&details[2].key=bucket&details[2].value=archive"; + String query = context + "&details[0].key=accesskey&details[0].value=ACCESS_SENTINEL" + + "&details[1].value=SECRET_SENTINEL&details[1].key=secretkey&sessionkey=SESSION_SENTINEL"; + if (encodedBrackets) { + query = query.replace("[", "%5B").replace("]", "%5D"); + context = context.replace("[", "%5B").replace("]", "%5D"); + } + List logs = captureRequestLogs("GET", query, queryParameters(query), true, false, HttpServletResponse.SC_OK); + assertRequestLogs(logs, context, "ACCESS_SENTINEL", "SECRET_SENTINEL", "SESSION_SENTINEL"); + + ArgumentCaptor captured = ArgumentCaptor.forClass(Map.class); + Mockito.verify(apiServer).handleRequest(captured.capture(), Mockito.eq("json"), Mockito.any(StringBuilder.class)); + Map unpackInput = new HashMap<>(); + Map forwarded = captured.getValue(); + forwarded.forEach((key, values) -> unpackInput.put(key, (String) values[0])); + AddImageStoreCmd command = new AddImageStoreCmd(); + DispatchTask task = new DispatchTask(command, unpackInput); + new ParamUnpackWorker().handle(task); + command.setDetails((Map) task.getParams().get(ApiConstants.DETAILS)); + Map expectedDetails = new HashMap<>(); + expectedDetails.put("accesskey", "ACCESS_SENTINEL"); + expectedDetails.put("secretkey", "SECRET_SENTINEL"); + expectedDetails.put("bucket", "archive"); + Assert.assertEquals(expectedDetails, command.getDetails()); + } + + private Map queryParameters(String query) { + Map params = new HashMap<>(); + for (String part : query.split("&")) { + String[] pair = part.split("=", 2); + if (pair.length != 2) { + continue; + } + String name = URLDecoder.decode(pair[0], StandardCharsets.UTF_8); + String value = URLDecoder.decode(pair[1], StandardCharsets.UTF_8); + params.merge(name, new String[] {value}, (existing, added) -> { + String[] values = Arrays.copyOf(existing, existing.length + 1); + values[existing.length] = added[0]; + return values; + }); + } + return params; + } + + private List captureRequestLogs(String method, String query, Map params, + boolean authorized, boolean enforcePost, int expectedStatus) { + Map original = new HashMap<>(); + params.forEach((key, values) -> original.put(key, values.clone())); + List logs = new ArrayList<>(); + AbstractAppender appender = new AbstractAppender("ApiServletRequestLoggingTest", null, null, true, Property.EMPTY_ARRAY) { + @Override + public void append(LogEvent event) { + logs.add(event.getLoggerName() + ": " + event.getMessage().getFormattedMessage()); + } + }; + org.apache.logging.log4j.core.Logger managementLogger = (org.apache.logging.log4j.core.Logger) LogManager.getLogger(ApiServlet.class); + org.apache.logging.log4j.core.Logger accessLogger = (org.apache.logging.log4j.core.Logger) LogManager.getLogger("apiserver." + ApiServlet.class.getName()); + Level managementLevel = managementLogger.getLevel(); + Level accessLevel = accessLogger.getLevel(); + appender.start(); + managementLogger.addAppender(appender); + accessLogger.addAppender(appender); + managementLogger.setLevel(Level.DEBUG); + accessLogger.setLevel(Level.INFO); + try { + Mockito.when(request.getMethod()).thenReturn(method); + Mockito.when(request.getQueryString()).thenReturn(query); + Mockito.when(request.getParameterMap()).thenReturn(params); + Mockito.when(authManager.getAPIAuthenticator(Mockito.anyString())).thenReturn(null); + if (authorized) { + Mockito.when(apiServer.verifyRequest(Mockito.anyMap(), nullable(Long.class), Mockito.any(InetAddress.class))).thenReturn(true); + } + if (enforcePost) { + Mockito.when(apiServer.isPostRequestsAndTimestampsEnforced()).thenReturn(true); + } + servlet.processRequestInContext(request, response); + Mockito.verify(response).setStatus(expectedStatus); + Assert.assertEquals(original.keySet(), params.keySet()); + original.forEach((key, values) -> Assert.assertArrayEquals(values, params.get(key))); + return logs; + } finally { + managementLogger.removeAppender(appender); + accessLogger.removeAppender(appender); + managementLogger.setLevel(managementLevel); + accessLogger.setLevel(accessLevel); + appender.stop(); + } + } + + private void assertRequestLogs(List logs, String expectedContext, String... secrets) { + int boundaries = 0; + for (String line : logs) { + if (line.contains("===START===") || line.contains("===END===") || line.startsWith("apiserver.")) { + boundaries++; + Assert.assertTrue("Missing request context: " + line, line.contains(expectedContext)); + } + for (String secret : secrets) { + Assert.assertFalse("Sensitive data in log: " + line, line.contains(secret)); + } + } + Assert.assertEquals("Expected START, END and access records", 3, boundaries); + } } diff --git a/ui/src/utils/plugins.js b/ui/src/utils/plugins.js index 729cef84d021..17c53e1c9369 100644 --- a/ui/src/utils/plugins.js +++ b/ui/src/utils/plugins.js @@ -217,7 +217,6 @@ export const pollJobPlugin = { export const notifierPlugin = { install (app) { app.config.globalProperties.$notifyError = function (error) { - console.log(error) var msg = i18n.global.t('message.request.failed') var desc = '' if (error && error.response) { diff --git a/ui/src/utils/request.js b/ui/src/utils/request.js index 2317aac04465..8d16dbe8aa59 100644 --- a/ui/src/utils/request.js +++ b/ui/src/utils/request.js @@ -34,7 +34,6 @@ const err = (error) => { const response = error.response let countNotify = store.getters.countNotify if (response) { - console.log(response) if (response.status === 403) { const data = response.data countNotify++ diff --git a/ui/src/views/infra/AddObjectStorage.vue b/ui/src/views/infra/AddObjectStorage.vue index 5410a9b9502f..8c206ffb9d07 100644 --- a/ui/src/views/infra/AddObjectStorage.vue +++ b/ui/src/views/infra/AddObjectStorage.vue @@ -87,7 +87,7 @@ - +