Skip to content

Commit f5a58ef

Browse files
Dogface2kDogface2k
committed
API: centralize credential-safe request diagnostics
1 parent d4712cd commit f5a58ef

6 files changed

Lines changed: 75 additions & 15 deletions

File tree

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

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer
257257
private MessageBus messageBus;
258258

259259
private static final Set<String> sensitiveFields = new HashSet<>(Arrays.asList(
260-
"password", "secretkey", "apikey", "token",
260+
"password", "privatekey", "secretkey", "apikey", "token",
261261
"sessionkey", "accesskey", "signature",
262262
"authorization", "credential", "secret"
263263
));
@@ -645,12 +645,8 @@ public String handleRequest(final Map params, final String responseType, final S
645645
final String keyStr = (String) key;
646646
final String[] value = (String[]) params.get(key);
647647

648-
String lowerKeyStr = keyStr.toLowerCase();
649-
boolean isSensitive = sensitiveFields.stream()
650-
.anyMatch(lowerKeyStr::contains);
651-
652648
String logValue;
653-
if (isSensitive) {
649+
if (isSensitiveParameter(keyStr)) {
654650
logValue = "******"; // mask sensitive values
655651
} else {
656652
logValue = (value == null) ? "'null'" : value[0];
@@ -759,6 +755,14 @@ public String handleRequest(final Map params, final String responseType, final S
759755
return response;
760756
}
761757

758+
static boolean isSensitiveParameter(String parameterName) {
759+
if (parameterName == null) {
760+
return false;
761+
}
762+
String normalizedParameterName = parameterName.toLowerCase(java.util.Locale.ROOT);
763+
return sensitiveFields.stream().anyMatch(normalizedParameterName::contains);
764+
}
765+
762766
@Override
763767
public boolean isPostRequestsAndTimestampsEnforced() {
764768
return isPostRequestsAndTimestampsEnforced;

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -730,7 +730,7 @@ private static String getCorrectIPAddress(String ip) {
730730
return null;
731731
}
732732

733-
private String getCleanParamsString(Map<String, String[]> reqParams) {
733+
String getCleanParamsString(Map<String, String[]> reqParams) {
734734
if (MapUtils.isEmpty(reqParams)) {
735735
return "";
736736
}
@@ -741,13 +741,10 @@ private String getCleanParamsString(Map<String, String[]> reqParams) {
741741
continue;
742742
}
743743

744-
cleanParamsString.append(reqParam.getKey());
744+
cleanParamsString.append(saveLogString(reqParam.getKey()));
745745
cleanParamsString.append("=");
746746

747-
if (reqParam.getKey().toLowerCase().contains("password")
748-
|| reqParam.getKey().toLowerCase().contains("privatekey")
749-
|| reqParam.getKey().toLowerCase().contains("accesskey")
750-
|| reqParam.getKey().toLowerCase().contains("secretkey")) {
747+
if (ApiServer.isSensitiveParameter(reqParam.getKey())) {
751748
cleanParamsString.append("\n");
752749
continue;
753750
}

‎server/src/test/java/com/cloud/api/ApiServletTest.java‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,4 +542,48 @@ public void shouldLogDuplicateParameterNameAndCountWithoutValues() {
542542
ApiServlet.LOGGER = originalLogger;
543543
}
544544
}
545+
546+
@Test
547+
public void shouldRemoveAuthenticationValuesFromLoggedPostParameters() {
548+
Map<String, String[]> params = new HashMap<>();
549+
params.put("command", new String[] {"listZones"});
550+
params.put("password", new String[] {"SYNTHETIC_PASSWORD"});
551+
params.put("privatekey", new String[] {"SYNTHETIC_PRIVATE_KEY"});
552+
params.put("accesskey", new String[] {"SYNTHETIC_ACCESS_KEY"});
553+
params.put("secretkey", new String[] {"SYNTHETIC_SECRET_KEY"});
554+
params.put("sessionkey", new String[] {"SYNTHETIC_SESSION_KEY"});
555+
params.put("signature", new String[] {"SYNTHETIC_SIGNATURE"});
556+
params.put("apikey", new String[] {"SYNTHETIC_API_KEY"});
557+
params.put("authorization", new String[] {"SYNTHETIC_AUTHORIZATION"});
558+
params.put("token", new String[] {"SYNTHETIC_TOKEN"});
559+
params.put("credential", new String[] {"SYNTHETIC_CREDENTIAL"});
560+
params.put("secret", new String[] {"SYNTHETIC_SECRET"});
561+
562+
String result = servlet.getCleanParamsString(params);
563+
564+
Assert.assertTrue(result.contains("command=listZones"));
565+
Assert.assertFalse(result.contains("SYNTHETIC_PASSWORD"));
566+
Assert.assertFalse(result.contains("SYNTHETIC_PRIVATE_KEY"));
567+
Assert.assertFalse(result.contains("SYNTHETIC_ACCESS_KEY"));
568+
Assert.assertFalse(result.contains("SYNTHETIC_SECRET_KEY"));
569+
Assert.assertFalse(result.contains("SYNTHETIC_SESSION_KEY"));
570+
Assert.assertFalse(result.contains("SYNTHETIC_SIGNATURE"));
571+
Assert.assertFalse(result.contains("SYNTHETIC_API_KEY"));
572+
Assert.assertFalse(result.contains("SYNTHETIC_AUTHORIZATION"));
573+
Assert.assertFalse(result.contains("SYNTHETIC_TOKEN"));
574+
Assert.assertFalse(result.contains("SYNTHETIC_CREDENTIAL"));
575+
Assert.assertFalse(result.contains("SYNTHETIC_SECRET"));
576+
}
577+
578+
@Test
579+
public void shouldKeepOrdinaryValuesInLoggedPostParameters() {
580+
Map<String, String[]> params = new HashMap<>();
581+
params.put("command", new String[] {"listZones"});
582+
params.put("response", new String[] {"json"});
583+
584+
String result = servlet.getCleanParamsString(params);
585+
586+
Assert.assertTrue(result.contains("command=listZones"));
587+
Assert.assertTrue(result.contains("response=json"));
588+
}
545589
}

‎ui/src/utils/apiError.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717

1818
function cleanLogValue (value) {
1919
if (typeof value !== 'string') {
20-
return value
20+
return undefined
2121
}
2222
return value.replace(/[\n\r\t]/g, '_').slice(0, 256)
2323
}
@@ -44,7 +44,7 @@ export function getSafeApiErrorDetails (error) {
4444
return {
4545
name: cleanLogValue(error?.name),
4646
code: cleanLogValue(error?.code),
47-
status: response?.status,
47+
status: Number.isInteger(response?.status) ? response.status : undefined,
4848
statusText: cleanLogValue(response?.statusText),
4949
method: typeof method === 'string' ? method.toUpperCase() : method,
5050
command: cleanLogValue(getCommand(config))

‎ui/tests/unit/utils/apiError.spec.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,4 +109,19 @@ describe('API error logging', () => {
109109
expect(serializedDetails).not.toContain(secretKey)
110110
expect(serializedDetails).not.toContain(sessionKey)
111111
})
112+
113+
it('does not serialize unexpected objects from the error or request metadata', () => {
114+
const secret = 'SYNTHETIC_NESTED_SECRET'
115+
const error = createAxiosError()
116+
error.name = { secret }
117+
error.code = { secret }
118+
error.response.status = { secret }
119+
error.response.statusText = { secret }
120+
error.response.config.method = { secret }
121+
error.response.config.params = { command: { secret } }
122+
123+
const serializedDetails = JSON.stringify(getSafeApiErrorDetails(error))
124+
125+
expect(serializedDetails).not.toContain(secret)
126+
})
112127
})

‎ui/tests/unit/views/infra/AddObjectStorage.spec.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ describe('Views > infra > AddObjectStorage.vue', () => {
4343
'details[0].key': 'accesskey',
4444
'details[0].value': 'test-access-key',
4545
'details[1].key': 'secretkey',
46-
'details[1].value': 'test-secret-key'
46+
'details[1].value': 'test-secret+key&with=symbols'
4747
}
4848

4949
await AddObjectStorage.methods.addObjectStore(params)

0 commit comments

Comments
 (0)