kumaab commented on code in PR #1232:
URL: https://github.com/apache/ranger/pull/1232#discussion_r4211096184
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceMgr.java:
##########
@@ -82,6 +82,10 @@ public class ServiceMgr {
private static final long _DefaultTimeoutValue_Lookp = 1000; // 1
s
private static final long _DefaultTimeoutValue_ValidateConfig = 10000; //
10 s
+ // Avoid returning low-level connect/reachability details from Test
Connection to the UI
+ private static final String SAFE_TEST_CONN_FAILURE_MSG =
Review Comment:
Suggestion: use single line.
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceMgr.java:
##########
@@ -741,6 +752,41 @@ private VXResponse generateResponseForTestConn(Map<String,
Object> responseData,
return vXResponse;
}
+ /**
+ * Returns true when Test Connection failure text would disclose whether a
host/port is reachable
+ * (for example Connection refused vs timeout), which can be used as a
lightweight port probe.
+ */
+ static boolean disclosesHostPortReachability(String... texts) {
+ if (texts == null) {
+ return false;
+ }
+
+ for (String text : texts) {
+ if (StringUtils.isBlank(text)) {
+ continue;
+ }
+
+ String normalized = text.toLowerCase();
+
+ if (normalized.contains("connection refused")
+ || normalized.contains("connectexception")
+ || normalized.contains("unknownhostexception")
+ || normalized.contains("noroutetohostexception")
+ || normalized.contains("no route to host")
+ || normalized.contains("network is unreachable")
+ || normalized.contains("sockettimeoutexception")
+ || normalized.contains("socketexception")
+ || normalized.contains("connection timed out")
+ || normalized.contains("connect timed out")
+ || normalized.contains("connection reset")
+ || normalized.contains("connection reset by peer")) {
Review Comment:
"connection reset by peer" can never match, because "connection reset"
already matches it.
Suggested:
```
private static final List<String> HOST_PORT_REACHABILITY_PATTERNS =
Arrays.asList(
"connection refused",
"connectexception",
"unknownhostexception",
"noroutetohostexception",
"no route to host",
"network is unreachable",
"sockettimeoutexception",
"socketexception",
"connection timed out",
"connect timed out",
"connection reset");
static boolean disclosesHostPortReachability(String... texts) {
boolean ret;
if (texts == null) {
ret = false;
}
ret = Arrays.stream(texts)
.filter(StringUtils::isNotBlank)
.map(text -> text.toLowerCase(Locale.ROOT))
.anyMatch(text ->
HOST_PORT_REACHABILITY_PATTERNS.stream().anyMatch(text::contains));
return ret;
}
```
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceMgr.java:
##########
@@ -726,6 +730,13 @@ private VXResponse generateResponseForTestConn(Map<String,
Object> responseData,
}
}
+ if (!connectivityStatus && disclosesHostPortReachability(message,
description)) {
+ LOG.error("Test Connection failed; sanitizing UI response.
Original message=[{}] description=[{}]", message, description);
Review Comment:
Message and description are same !?
##########
security-admin/src/test/java/org/apache/ranger/biz/TestServiceMgr.java:
##########
@@ -385,4 +385,79 @@ public void
test13_validateConfig_allowsLegitimateExternalUrls() throws Exceptio
Assertions.assertNotNull(response);
Assertions.assertNotEquals(VXResponse.STATUS_ERROR,
response.getStatusCode());
}
+
+ @Test
+ public void test14_validateConfig_sanitizesConnectionRefusedDetails()
throws Exception {
Review Comment:
`test14` and `test15` repeat about 15 lines of identical setup. A helper
that builds the mocked service and response map would remove that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
