Skip to content

Commit c474866

Browse files
Dogface2kDogface2k
authored
UI: submit object storage creation using POST (#13768)
1 parent 8074c3e commit c474866

10 files changed

Lines changed: 590 additions & 15 deletions

File tree

‎api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddObjectStoragePoolCmd.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
import java.util.Map;
3333

3434
@APICommand(name = "addObjectStoragePool", description = "Adds a object storage pool", responseObject = ObjectStoreResponse.class, since = "4.19.0",
35-
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
35+
requestHasSensitiveInfo = true, responseHasSensitiveInfo = false)
3636
public class AddObjectStoragePoolCmd extends BaseCmd {
3737

3838
/////////////////////////////////////////////////////

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import com.cloud.exception.DiscoveryException;
2222
import com.cloud.storage.StorageService;
23+
import org.apache.cloudstack.api.APICommand;
2324
import org.apache.cloudstack.api.ResponseGenerator;
2425
import org.apache.cloudstack.api.response.ObjectStoreResponse;
2526
import org.apache.cloudstack.context.CallContext;
@@ -38,6 +39,8 @@
3839
import java.util.HashMap;
3940
import java.util.Map;
4041

42+
import static org.junit.Assert.assertNotNull;
43+
import static org.junit.Assert.assertTrue;
4144
import static org.mockito.ArgumentMatchers.any;
4245

4346
@RunWith(MockitoJUnitRunner.class)
@@ -98,4 +101,38 @@ public void testAddObjectStore() throws DiscoveryException {
98101
Mockito.verify(storageService, Mockito.times(1))
99102
.discoverObjectStore(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
100103
}
104+
105+
@Test
106+
public void testRequestIsMarkedAsContainingSensitiveInformation() {
107+
APICommand apiCommand = AddObjectStoragePoolCmd.class.getAnnotation(APICommand.class);
108+
109+
assertNotNull(apiCommand);
110+
assertTrue(apiCommand.requestHasSensitiveInfo());
111+
}
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+
}
101138
}

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

Lines changed: 58 additions & 8 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,6 +86,8 @@ 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"));
@@ -100,6 +103,7 @@ public class ApiServlet extends HttpServlet {
100103
"updaterolepermission",
101104
"updateprojectrolepermission",
102105
"createstoragepool",
106+
"addobjectstoragepool",
103107
"addhost",
104108
"updatehostpassword",
105109
"addcluster",
@@ -172,7 +176,7 @@ void utf8Fixup(final HttpServletRequest req, final Map<String, Object[]> params)
172176
String value = decodeUtf8(paramTokens[1]);
173177
params.put(name, new String[] {value});
174178
} else {
175-
LOGGER.debug("Invalid parameter in URL found. param: " + param);
179+
LOGGER.debug("Invalid parameter in URL found (missing equals sign).");
176180
}
177181
}
178182
}
@@ -199,8 +203,8 @@ public void run() {
199203
private void checkSingleQueryParameterValue(Map<String, String[]> params) {
200204
params.forEach((k, v) -> {
201205
if (v.length > 1) {
202-
String message = String.format("Query parameter '%s' has multiple values %s. Only the last value will be respected." +
203-
"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);
204208
LOGGER.warn(message);
205209
}
206210
});
@@ -237,17 +241,15 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
237241

238242
// logging the request start and end in management log for easy debugging
239243
String reqStr = "";
240-
String cleanQueryString = StringUtils.cleanString(req.getQueryString());
244+
String cleanQueryString = getCleanQueryString(command, req.getQueryString(), reqParams);
241245
if (LOGGER.isDebugEnabled()) {
242246
reqStr = auditTrailSb.toString() + " " + cleanQueryString;
243247
if (req.getMethod().equalsIgnoreCase("POST") && org.apache.commons.lang3.StringUtils.isNotBlank(command)) {
244-
if (!POST_REQUESTS_TO_DISABLE_LOGGING.contains(command.toLowerCase()) && !reqParams.containsKey(ApiConstants.USER_DATA)) {
248+
if (shouldLogRequestParameters(command, reqParams)) {
245249
String cleanParamsString = getCleanParamsString(reqParams);
246250
if (org.apache.commons.lang3.StringUtils.isNotBlank(cleanParamsString)) {
247251
reqStr += "\n" + cleanParamsString;
248252
}
249-
} else {
250-
reqStr += " " + command;
251253
}
252254
}
253255
LOGGER.debug("===START=== " + reqStr);
@@ -580,7 +582,7 @@ protected void setClientAddressForConsoleEndpointAccess(String command, Map<Stri
580582

581583
@Nullable
582584
private String saveLogString(String stringToLog) {
583-
return stringToLog == null ? null : stringToLog.replace(LOGGER_REPLACEMENTS, REPLACEMENT);
585+
return stringToLog == null ? null : stringToLog.replaceAll(LOGGER_REPLACEMENTS, REPLACEMENT);
584586
}
585587

586588
/**
@@ -779,4 +781,52 @@ private String getCleanParamsString(Map<String, String[]> reqParams) {
779781

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

0 commit comments

Comments
 (0)