gnodet-bot commented on code in PR #29056:
URL: https://github.com/apache/camel/pull/29056#discussion_r4458423830
##########
components/camel-as2/camel-as2-api/src/main/java/org/apache/camel/component/as2/api/AS2AsynchronousMDNManager.java:
##########
@@ -223,6 +276,26 @@ public HttpCoreContext send(
}
}
+
+ private Socket createSocket(boolean https, String host, int port) throws
IOException {
+ if (!https) {
+ return new Socket(host, port);
+ }
+ SSLSocket sslSocket = (SSLSocket)
sslContext.getSocketFactory().createSocket(host, port);
+ try {
+ // verify the delivery host against the certificate the peer
presents during the handshake
+ SSLParameters sslParameters = sslSocket.getSSLParameters();
+ sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
+ sslSocket.setSSLParameters(sslParameters);
+ // handshake now so a certificate or hostname mismatch fails
before the MDN and any credentials are written
+ sslSocket.startHandshake();
+ } catch (IOException e) {
+ sslSocket.close();
+ throw e;
+ }
+ return sslSocket;
+ }
Review Comment:
⚠️ **Socket leak on unchecked exception.** The `catch (IOException e)` block
correctly closes the socket on `IOException`, but unchecked exceptions thrown
by `setSSLParameters` or `startHandshake` (e.g. an `IllegalArgumentException`,
or any `Error`) bypass the catch and leave the socket open. The string
`"HTTPS"` is valid so this is unlikely in practice, but the pattern is fragile
for security-critical socket setup. Widen the catch:
```suggestion
private Socket createSocket(boolean https, String host, int port) throws
IOException {
if (!https) {
return new Socket(host, port);
}
SSLSocket sslSocket = (SSLSocket)
sslContext.getSocketFactory().createSocket(host, port);
try {
// verify the delivery host against the certificate the peer
presents during the handshake
SSLParameters sslParameters = sslSocket.getSSLParameters();
sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
sslSocket.setSSLParameters(sslParameters);
// handshake now so a certificate or hostname mismatch fails
before the MDN and any credentials are written
sslSocket.startHandshake();
} catch (Exception e) {
sslSocket.close();
if (e instanceof IOException ioe) {
throw ioe;
}
throw new IOException("TLS handshake setup failed", e);
}
return sslSocket;
}
```
##########
components/camel-as2/camel-as2-api/src/main/java/org/apache/camel/component/as2/api/AS2AsynchronousMDNManager.java:
##########
@@ -234,18 +307,82 @@ private static String normalizeHost(String host) {
return host;
}
- private boolean isAllowedHost(String host) {
+ /**
+ * Writes a delivery authority the way an {@code asyncMdnAllowedHosts}
entry is written, so a log message names an
+ * entry the operator can copy. The host has been through normalizeHost,
so an IPv6 literal (the only host that
+ * contains a colon) is put back in square brackets ({@code [::1]:8443}):
unbracketed, {@code ::1:8443} would be
+ * read as another IPv6 address on the default port. Any other host is
written as {@code host:port}.
+ */
+ static String formatAuthority(String host, int port) {
+ return host.indexOf(':') >= 0 ? "[" + host + "]:" + port : host + ":"
+ port;
+ }
+
+ /**
+ * Matches the resolved delivery authority - host and port - against
{@code asyncMdnAllowedHosts}. Each entry is a
+ * host with an optional port ({@code host} or {@code host:port}); a
bare-host entry matches only the default port
+ * for the scheme ({@code defaultPort}), so a listed host does not
authorise every port on that host.
+ */
+ boolean isAllowedAuthority(String host, int port, int defaultPort) {
if (allowedHosts == null || allowedHosts.isBlank()) {
return false;
}
for (String allowed : allowedHosts.split(",")) {
- if (allowed.trim().equalsIgnoreCase(host)) {
+ String entry = allowed.trim();
+ if (!entry.isEmpty() && authorityMatches(entry, host, port,
defaultPort)) {
return true;
}
}
return false;
}
+ private static boolean authorityMatches(String entry, String host, int
port, int defaultPort) {
+ String entryHost;
+ int entryPort;
+ if (entry.charAt(0) == '[') {
+ // bracketed IPv6 literal, optionally followed by :port
+ int close = entry.indexOf(']');
+ if (close < 0) {
+ return false;
+ }
+ entryHost = entry.substring(1, close);
+ entryPort = defaultPort;
+ if (close + 1 < entry.length()) {
+ if (entry.charAt(close + 1) != ':') {
+ return false;
+ }
+ Integer parsed = parsePort(entry.substring(close + 2));
+ if (parsed == null) {
+ return false;
+ }
+ entryPort = parsed;
+ }
+ } else {
+ int colon = entry.indexOf(':');
+ if (colon >= 0 && entry.indexOf(':', colon + 1) < 0) {
+ // a single colon separates host and port; an unbracketed IPv6
literal has several and keeps the default
+ entryHost = entry.substring(0, colon);
+ Integer parsed = parsePort(entry.substring(colon + 1));
+ if (parsed == null) {
+ return false;
+ }
+ entryPort = parsed;
+ } else {
+ entryHost = entry;
+ entryPort = defaultPort;
+ }
+ }
+ return entryPort == port && entryHost.equalsIgnoreCase(host);
+ }
+
+ private static Integer parsePort(String value) {
+ try {
+ int parsed = Integer.parseInt(value.trim());
Review Comment:
🔍 **Port 0 is accepted by the allow-list.** `parsed >= 0` allows port 0,
which is the OS wildcard and not a valid outbound TCP destination. An operator
who writes `partner.example:0` in the allow-list would silently pass
validation, then get a connect-time failure with no clear error message
pointing back to the allow-list. Change the lower bound to 1:
```suggestion
return parsed >= 1 && parsed <= 65535 ? parsed : null;
```
--
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]
