Skip to content

Commit 70545ab

Browse files
Dogface2kDogface2k
committed
API: preserve credential-safe query context
Retain cleaned query context instead of applying POST-payload suppression to the entire query. Exclude known credential fields and indexed parameters for requests already covered by the sensitive-logging policy, without interpreting descriptors from mixed query and POST bodies. Keep duplicate and malformed request diagnostics without raw values, and verify request data, response handling and storage discovery remain unchanged.
1 parent 4bac5d4 commit 70545ab

3 files changed

Lines changed: 352 additions & 37 deletions

File tree

‎api/src/test/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmdTest.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,4 +109,30 @@ public void testRequestIsMarkedAsContainingSensitiveInformation() {
109109
assertNotNull(apiCommand);
110110
assertTrue(apiCommand.requestHasSensitiveInfo());
111111
}
112+
113+
@Test
114+
public void testCredentialsAndProviderOptionsReachDiscoveryUnchanged() throws DiscoveryException {
115+
Map<String, String> expected = new HashMap<>();
116+
expected.put("accesskey", "ACCESS_SENTINEL");
117+
expected.put("secretkey", "SECRET+key&with=symbols");
118+
expected.put("validateSSL", "true");
119+
expected.put("s3Url", "https://s3.example.test");
120+
expected.put("iamUrl", "https://iam.example.test");
121+
Map<Integer, Map<String, String>> indexedDetails = new HashMap<>();
122+
int index = 0;
123+
for (Map.Entry<String, String> entry : expected.entrySet()) {
124+
Map<String, String> detail = new HashMap<>();
125+
detail.put("key", entry.getKey());
126+
detail.put("value", entry.getValue());
127+
indexedDetails.put(index++, detail);
128+
}
129+
ReflectionTestUtils.setField(addObjectStoragePoolCmdSpy, "details", indexedDetails);
130+
Mockito.doReturn(objectStore).when(storageService).discoverObjectStore(Mockito.anyString(),
131+
Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), any());
132+
Mockito.doReturn(new ObjectStoreResponse()).when(responseGenerator).createObjectStoreResponse(any());
133+
134+
addObjectStoragePoolCmdSpy.execute();
135+
136+
Mockito.verify(storageService).discoverObjectStore(name, url, size, provider, expected);
137+
}
112138
}

‎server/src/main/java/com/cloud/api/ApiServlet.java‎

Lines changed: 47 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.net.InetAddress;
2121
import java.net.URLDecoder;
2222
import java.net.UnknownHostException;
23+
import java.util.ArrayList;
2324
import java.util.Arrays;
2425
import java.util.HashMap;
2526
import java.util.HashSet;
@@ -85,10 +86,12 @@ public class ApiServlet extends HttpServlet {
8586
private static final Logger ACCESSLOGGER = LogManager.getLogger("apiserver." + ApiServlet.class.getName());
8687
private static final String REPLACEMENT = "_";
8788
private static final String LOGGER_REPLACEMENTS = "[\n\r\t]";
89+
private static final Pattern SENSITIVE_QUERY_PARAMETER = Pattern.compile(
90+
".*(?:password|privatekey|accesskey|secretkey)|sessionkey|secretcode|token|userdata", Pattern.DOTALL);
8891
private static final Pattern GET_REQUEST_COMMANDS = Pattern.compile("^(get|list|query|find)(\\w+)+$");
8992
private static final HashSet<String> GET_REQUEST_COMMANDS_LIST = new HashSet<>(Set.of("isaccountallowedtocreateofferingswithtags",
9093
"readyforshutdown", "cloudianisenabled", "quotabalance", "quotasummary", "quotatarifflist", "quotaisenabled", "quotastatement", "verifyoauthcodeandgetuser"));
91-
private static final HashSet<String> REQUESTS_TO_DISABLE_PARAMETER_LOGGING = new HashSet<>(Set.of(
94+
private static final HashSet<String> POST_REQUESTS_TO_DISABLE_LOGGING = new HashSet<>(Set.of(
9295
"login",
9396
"oauthlogin",
9497
"createaccount",
@@ -173,7 +176,7 @@ void utf8Fixup(final HttpServletRequest req, final Map<String, Object[]> params)
173176
String value = decodeUtf8(paramTokens[1]);
174177
params.put(name, new String[] {value});
175178
} else {
176-
LOGGER.debug("Invalid parameter in URL found. param: " + param);
179+
LOGGER.debug("Invalid parameter in URL found (missing equals sign).");
177180
}
178181
}
179182
}
@@ -200,8 +203,8 @@ public void run() {
200203
private void checkSingleQueryParameterValue(Map<String, String[]> params) {
201204
params.forEach((k, v) -> {
202205
if (v.length > 1) {
203-
String message = String.format("Query parameter '%s' has multiple values %s. Only the last value will be respected." +
204-
"It is advised to pass only a single parameter", k, Arrays.toString(v));
206+
String message = String.format("Query parameter '%s' has %d values. Only the last value will be respected. " +
207+
"It is advised to pass only a single parameter", saveLogString(k), v.length);
205208
LOGGER.warn(message);
206209
}
207210
});
@@ -587,7 +590,7 @@ protected void setClientAddressForConsoleEndpointAccess(String command, Map<Stri
587590

588591
@Nullable
589592
private String saveLogString(String stringToLog) {
590-
return stringToLog == null ? null : stringToLog.replace(LOGGER_REPLACEMENTS, REPLACEMENT);
593+
return stringToLog == null ? null : stringToLog.replaceAll(LOGGER_REPLACEMENTS, REPLACEMENT);
591594
}
592595

593596
/**
@@ -773,14 +776,49 @@ private String getCleanParamsString(Map<String, String[]> reqParams) {
773776

774777
protected boolean shouldLogRequestParameters(String command, Map<String, String[]> reqParams) {
775778
return (org.apache.commons.lang3.StringUtils.isBlank(command)
776-
|| !REQUESTS_TO_DISABLE_PARAMETER_LOGGING.contains(command.toLowerCase(java.util.Locale.ROOT)))
779+
|| !POST_REQUESTS_TO_DISABLE_LOGGING.contains(command.toLowerCase(java.util.Locale.ROOT)))
777780
&& !reqParams.containsKey(ApiConstants.USER_DATA);
778781
}
779782

780783
protected String getCleanQueryString(String command, String queryString, Map<String, String[]> reqParams) {
781-
if (!shouldLogRequestParameters(command, reqParams)) {
782-
return org.apache.commons.lang3.StringUtils.isBlank(command) ? "" : "command=" + saveLogString(command);
784+
boolean sensitiveRequest = !shouldLogRequestParameters(command, reqParams);
785+
boolean includeCommand = sensitiveRequest && org.apache.commons.lang3.StringUtils.isNotBlank(command);
786+
if (org.apache.commons.lang3.StringUtils.isEmpty(queryString)) {
787+
return includeCommand ? "command=" + saveLogString(command) : "";
783788
}
784-
return StringUtils.cleanString(queryString);
789+
790+
List<String> cleanParameters = new ArrayList<>();
791+
try {
792+
// Split before decoding so an encoded ampersand remains part of its original value.
793+
// Read the query only: reqParams also contains the POST body.
794+
for (String parameter : queryString.split("&", -1)) {
795+
int separator = parameter.indexOf('=');
796+
if (separator < 0) {
797+
continue;
798+
}
799+
String name = decodeUtf8(parameter.substring(0, separator));
800+
if (ApiConstants.COMMAND.equals(name)) {
801+
includeCommand = false;
802+
}
803+
String lowerName = name.toLowerCase(java.util.Locale.ROOT);
804+
if (SENSITIVE_QUERY_PARAMETER.matcher(lowerName).matches()) {
805+
continue;
806+
}
807+
// Sensitive requests can carry credentials in indexed maps. Omit those entries
808+
// rather than trust descriptors that may be in the POST body or use different case.
809+
if (sensitiveRequest && (name.indexOf('[') >= 0 || ApiConstants.DETAILS.equals(lowerName))) {
810+
continue;
811+
}
812+
cleanParameters.add(parameter);
813+
}
814+
} catch (IllegalArgumentException e) {
815+
// Formatting must not expose an undecodable query or change request processing.
816+
return includeCommand ? "command=" + saveLogString(command) : "";
817+
}
818+
if (includeCommand) {
819+
cleanParameters.add(0, "command=" + saveLogString(command));
820+
}
821+
// Preserve the existing cleaning of nested URLs and JSON in the retained query fields.
822+
return StringUtils.cleanString(String.join("&", cleanParameters));
785823
}
786824
}

0 commit comments

Comments
 (0)