This is an automated email from the ASF dual-hosted git repository.
DaanHoogland pushed a commit to branch 4.22
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.22 by this push:
new c4748669bfc UI: submit object storage creation using POST (#13768)
c4748669bfc is described below
commit c4748669bfc8faa1c427a15de457cb65ca14b231
Author: Brad <[email protected]>
AuthorDate: Thu Oct 22 16:31:41 2026 +0100
UI: submit object storage creation using POST (#13768)
---
.../admin/storage/AddObjectStoragePoolCmd.java | 2 +-
.../admin/storage/AddObjectStoragePoolCmdTest.java | 37 +++
server/src/main/java/com/cloud/api/ApiServlet.java | 66 +++-
.../test/java/com/cloud/api/ApiServletTest.java | 362 +++++++++++++++++++++
ui/src/utils/plugins.js | 1 -
ui/src/utils/request.js | 1 -
ui/src/views/infra/AddObjectStorage.vue | 6 +-
ui/tests/unit/views/infra/AddObjectStorage.spec.js | 99 ++++++
.../src/main/java/com/cloud/utils/StringUtils.java | 2 +-
.../test/java/com/cloud/utils/StringUtilsTest.java | 29 ++
10 files changed, 590 insertions(+), 15 deletions(-)
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 460b8d642e9..c24d61b7200 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.Iterator;
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 c7aeb8ba99b..1d0c3e6673e 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 @@ package org.apache.cloudstack.api.command.admin.storage;
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 org.springframework.test.util.ReflectionTestUtils;
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 class AddObjectStoragePoolCmdTest {
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<String, String> 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<Integer, Map<String, String>> indexedDetails = new HashMap<>();
+ int index = 0;
+ for (Map.Entry<String, String> entry : expected.entrySet()) {
+ Map<String, String> 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 6a14a00198a..3ea9e7b5b5b 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.io.UnsupportedEncodingException;
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<String> 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 @@ public class ApiServlet extends HttpServlet {
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 class ApiServlet extends HttpServlet {
private void checkSingleQueryParameterValue(Map<String, String[]> 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 @@ public class ApiServlet extends HttpServlet {
// 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);
@@ -580,7 +582,7 @@ public class ApiServlet extends HttpServlet {
@Nullable
private String saveLogString(String stringToLog) {
- return stringToLog == null ? null :
stringToLog.replace(LOGGER_REPLACEMENTS, REPLACEMENT);
+ return stringToLog == null ? null :
stringToLog.replaceAll(LOGGER_REPLACEMENTS, REPLACEMENT);
}
/**
@@ -779,4 +781,52 @@ public class ApiServlet extends HttpServlet {
return cleanParamsString.toString();
}
+
+ protected boolean shouldLogRequestParameters(String command, Map<String,
String[]> 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<String, String[]> 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<String> 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 79fe4b86f85..367bba9f3c1 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.AccountService;
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.StringWriter;
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 class ApiServletTest {
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<String, String[]> 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<String> 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<String> 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<String> 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<Map> 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<String> 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<String, String[]> 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<String> 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<Map> 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<String, String[]> 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<String> 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<Map> captured = ArgumentCaptor.forClass(Map.class);
+ Mockito.verify(apiServer).handleRequest(captured.capture(),
Mockito.eq("json"), Mockito.any(StringBuilder.class));
+ Map<String, String> commandParameters = new HashMap<>();
+ Map<String, Object[]> 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<String, String[]> 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<String> 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<String> 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<String> 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<Map> 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<String> 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<String> 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<String> 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<String, String[]> 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<String> logs = captureRequestLogs("GET", query,
queryParameters(query), true, false, HttpServletResponse.SC_OK);
+ assertRequestLogs(logs, context, "ACCESS_SENTINEL", "SECRET_SENTINEL",
"SESSION_SENTINEL");
+
+ ArgumentCaptor<Map> captured = ArgumentCaptor.forClass(Map.class);
+ Mockito.verify(apiServer).handleRequest(captured.capture(),
Mockito.eq("json"), Mockito.any(StringBuilder.class));
+ Map<String, String> unpackInput = new HashMap<>();
+ Map<String, Object[]> 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<String, String> expectedDetails = new HashMap<>();
+ expectedDetails.put("accesskey", "ACCESS_SENTINEL");
+ expectedDetails.put("secretkey", "SECRET_SENTINEL");
+ expectedDetails.put("bucket", "archive");
+ Assert.assertEquals(expectedDetails, command.getDetails());
+ }
+
+ private Map<String, String[]> queryParameters(String query) {
+ Map<String, String[]> 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<String> captureRequestLogs(String method, String query,
Map<String, String[]> params,
+ boolean authorized, boolean enforcePost, int expectedStatus) {
+ Map<String, String[]> original = new HashMap<>();
+ params.forEach((key, values) -> original.put(key, values.clone()));
+ List<String> 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<String> 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 729cef84d02..17c53e1c936 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 2317aac0446..8d16dbe8aa5 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 5410a9b9502..8c206ffb9d0 100644
--- a/ui/src/views/infra/AddObjectStorage.vue
+++ b/ui/src/views/infra/AddObjectStorage.vue
@@ -87,7 +87,7 @@
<a-input v-model:value="form.accessKey" />
</a-form-item>
<a-form-item name="secretKey" ref="secretKey"
:label="$t('label.secret.key')">
- <a-input v-model:value="form.secretKey" />
+ <a-input-password v-model:value="form.secretKey"
autocomplete="off" />
</a-form-item>
<a-form-item name="size" ref="size">
<template #label>
@@ -106,7 +106,7 @@
</template>
<script>
import { ref, reactive, toRaw } from 'vue'
-import { getAPI } from '@/api'
+import { postAPI } from '@/api'
import { mixinForm } from '@/utils/mixin'
import ResourceIcon from '@/components/view/ResourceIcon'
import TooltipLabel from '@/components/widgets/TooltipLabel'
@@ -209,7 +209,7 @@ export default {
},
addObjectStore (params) {
return new Promise((resolve, reject) => {
- getAPI('addObjectStoragePool', params).then(json => {
+ postAPI('addObjectStoragePool', params).then(json => {
resolve()
}).catch(error => {
reject(error)
diff --git a/ui/tests/unit/views/infra/AddObjectStorage.spec.js
b/ui/tests/unit/views/infra/AddObjectStorage.spec.js
new file mode 100644
index 00000000000..0d8933ac008
--- /dev/null
+++ b/ui/tests/unit/views/infra/AddObjectStorage.spec.js
@@ -0,0 +1,99 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import mockAxios from '../../../mock/mockAxios'
+import AddObjectStorage from '@/views/infra/AddObjectStorage'
+import { mount } from '@vue/test-utils'
+
+jest.mock('axios', () => mockAxios)
+jest.mock('@/vue-app', () => ({
+ vueProps: {
+ $localStorage: {
+ get: jest.fn(() => null)
+ }
+ }
+}))
+
+describe('Views > infra > AddObjectStorage.vue', () => {
+ beforeEach(() => {
+ mockAxios.mockReset()
+ })
+
+ it('submits addObjectStoragePool using POST with the request in the body',
async () => {
+ mockAxios.mockResolvedValue({})
+
+ const params = {
+ name: 'test-store',
+ provider: 'MinIO',
+ url: 'https://object-storage.example.test',
+ 'details[0].key': 'accesskey',
+ 'details[0].value': 'test-access-key',
+ 'details[1].key': 'secretkey',
+ 'details[1].value': 'test-secret+key&with=symbols'
+ }
+
+ await AddObjectStorage.methods.addObjectStore(params)
+
+ expect(mockAxios).toHaveBeenCalledTimes(1)
+ const request = mockAxios.mock.calls[0][0]
+ expect(request).toMatchObject({
+ url: '/',
+ method: 'POST'
+ })
+ expect(request.params).toBeUndefined()
+ expect(request.data).toBeInstanceOf(URLSearchParams)
+ expect(request.data.get('command')).toBe('addObjectStoragePool')
+ expect(request.data.get('response')).toBe('json')
+ expect(request.data.get('url')).toBe(params.url)
+
expect(request.data.get('details[0].value')).toBe(params['details[0].value'])
+
expect(request.data.get('details[1].value')).toBe(params['details[1].value'])
+ })
+
+ it('propagates an addObjectStoragePool API failure', async () => {
+ const error = new Error('request failed')
+ mockAxios.mockRejectedValue(error)
+
+ await
expect(AddObjectStorage.methods.addObjectStore({})).rejects.toBe(error)
+ })
+
+ it('masks the object storage secret key', () => {
+ const wrapper = mount(AddObjectStorage, {
+ props: {
+ resource: {}
+ },
+ global: {
+ mocks: {
+ $getApiParams: jest.fn(() => ({
+ name: {},
+ provider: {},
+ url: {},
+ size: {}
+ })),
+ $t: key => key
+ },
+ provide: {
+ parentFetchData: jest.fn()
+ }
+ }
+ })
+
+ const secretKeyInput = wrapper.find('input[type="password"]')
+
+ expect(secretKeyInput.exists()).toBe(true)
+ expect(secretKeyInput.attributes('autocomplete')).toBe('off')
+ })
+})
diff --git a/utils/src/main/java/com/cloud/utils/StringUtils.java
b/utils/src/main/java/com/cloud/utils/StringUtils.java
index ef09c29edbc..e581a0e4795 100644
--- a/utils/src/main/java/com/cloud/utils/StringUtils.java
+++ b/utils/src/main/java/com/cloud/utils/StringUtils.java
@@ -147,7 +147,7 @@ public class StringUtils extends
org.apache.commons.lang3.StringUtils {
// removes a password/accesskey/ property from a response json object
private static final Pattern REGEX_PASSWORD_JSON =
Pattern.compile("\"(([pP])assword|privatekey|accesskey|secretkey)\":\\s?\".*?\",?");
- private static final Pattern REGEX_PASSWORD_DETAILS =
Pattern.compile("(&|%26)?details(\\[|%5B)\\d*(\\]|%5D)\\.key(=|%3D)(([pP])assword|accesskey|secretkey)(?=(%26|[&'\"]))");
+ private static final Pattern REGEX_PASSWORD_DETAILS =
Pattern.compile("(&|%26)?details(\\[|%5B)\\d*(\\]|%5D)\\.key(=|%3D)(([pP])assword|accesskey|secretkey)(?=(%26|[&'\"]|$))");
private static final Pattern REGEX_PASSWORD_DETAILS_INDEX =
Pattern.compile("details(\\[|%5B)\\d*(\\]|%5D)");
diff --git a/utils/src/test/java/com/cloud/utils/StringUtilsTest.java
b/utils/src/test/java/com/cloud/utils/StringUtilsTest.java
index 8a41f956043..62beec11535 100644
--- a/utils/src/test/java/com/cloud/utils/StringUtilsTest.java
+++ b/utils/src/test/java/com/cloud/utils/StringUtilsTest.java
@@ -277,4 +277,33 @@ public class StringUtilsTest {
assertEquals(key, output.first());
assertEquals(value, output.second());
}
+
+ @Test
+ public void testCleanSensitiveDetailsAtEndOfQuery() {
+ for (String index : new String[] {"[1]", "%5B1%5D"}) {
+ for (String key : new String[] {"password", "Password",
"accesskey", "secretkey"}) {
+ String query = "command=addImageStore&name=store-1"
+ + "&details[2].key=bucket&details[2].value=archive"
+ + "&details" + index +
".value=SECRET_SENTINEL&details" + index + ".key=" + key;
+ assertEquals("Terminal descriptor " + key + " at " + index,
+
"command=addImageStore&name=store-1&details[2].key=bucket&details[2].value=archive&",
+ StringUtils.cleanString(query));
+ }
+ }
+ }
+
+ @Test
+ public void
testCleanDetailsDoesNotTreatSensitiveKeyPrefixesAsCredentials() {
+ for (String key : new String[] {"secretkeyname", "accesskeyid",
"passwordenabled", "bucket"}) {
+ String query =
"command=addImageStore&details[1].value=retained-value&details[1].key=" + key;
+ assertEquals(query, StringUtils.cleanString(query));
+ }
+ }
+
+ @Test
+ public void testCleanSensitiveDetailsAtEndOfEncodedQuery() {
+ String query = "command%3DaddImageStore%26name%3Dstore-1"
+ +
"%26details%5B1%5D.value%3DSECRET_SENTINEL%26details%5B1%5D.key%3Dsecretkey";
+ assertEquals("command%3DaddImageStore%26name%3Dstore-1&",
StringUtils.cleanString(query));
+ }
}
