webhook: fix delivery hardening regression

plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookDeliveryThread.java

@@ -47,12 +47,19 @@
 import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpPost;
+import org.apache.http.config.Registry;
+import org.apache.http.config.RegistryBuilder;
+import org.apache.http.conn.DnsResolver;
+import org.apache.http.conn.socket.ConnectionSocketFactory;
+import org.apache.http.conn.socket.PlainConnectionSocketFactory;
 import org.apache.http.conn.ssl.NoopHostnameVerifier;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
 import org.apache.http.conn.ssl.TrustAllStrategy;
 import org.apache.http.entity.ContentType;
 import org.apache.http.entity.StringEntity;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClients;
+import org.apache.http.impl.conn.BasicHttpClientConnectionManager;
 import org.apache.http.message.BasicHeader;
 import org.apache.http.ssl.SSLContextBuilder;
 import org.apache.http.util.EntityUtils;
@@ -102,17 +109,30 @@ protected boolean isValidJson(String json) {
         return true;
     }
 
-    protected void setHttpClient() throws NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
+    protected SSLConnectionSocketFactory getSslConnectionSocketFactory() throws NoSuchAlgorithmException,
+            KeyStoreException, KeyManagementException {
         if (webhook.isSslVerification()) {
-            httpClient = HttpClients.custom().disableRedirectHandling().build();
-            return;
+            return SSLConnectionSocketFactory.getSocketFactory();
         }
+        return new SSLConnectionSocketFactory(new SSLContextBuilder().loadTrustMaterial(null,
+                TrustAllStrategy.INSTANCE).build(), NoopHostnameVerifier.INSTANCE);
+    }
+
+    protected DnsResolver getDnsResolver() {
+        return new WebhookUrlValidator.DestinationPolicyDnsResolver(destinationBlocklist, blockLocalAddresses);
+    }
+
+    protected void setHttpClient() throws NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
+        Registry<ConnectionSocketFactory> socketFactoryRegistry = RegistryBuilder.<ConnectionSocketFactory>create()
+                .register("http", PlainConnectionSocketFactory.getSocketFactory())
+                .register("https", getSslConnectionSocketFactory())
+                .build();
+        // Destination addresses are validated by the DNS resolver
         httpClient = HttpClients
                 .custom()
                 .disableRedirectHandling()
-                .setSSLContext(new SSLContextBuilder().loadTrustMaterial(null,
-                        TrustAllStrategy.INSTANCE).build())
-                .setSSLHostnameVerifier(NoopHostnameVerifier.INSTANCE)
+                .setConnectionManager(new BasicHttpClientConnectionManager(socketFactoryRegistry, null, null,
+                        getDnsResolver()))
                 .build();
     }
 
@@ -205,18 +225,34 @@ public void run() {
             callback.complete(new WebhookDeliveryResult(headers, payload, success, response, new Date()));
             return;
         }
-        if (payloadUri == null) {
-            payloadUri = WebhookUrlValidator.validateWebhookDestinationUrl(webhook.getPayloadUrl(), allowHttp,
-                    destinationBlocklist, blockLocalAddresses);
-        }
-        while (attempt < deliveryTries) {
-            attempt++;
-            if (delivery(attempt)) {
-                success = true;
-                break;
+        try {
+            if (payloadUri == null) {
+                payloadUri = URI.create(webhook.getPayloadUrl());
+                WebhookUrlValidator.validateScheme(payloadUri, allowHttp);
             }
+            while (attempt < deliveryTries) {
+                attempt++;
+                if (delivery(attempt)) {
+                    success = true;
+                    break;
+                }
+            }
+        } catch (InvalidParameterValueException | IllegalArgumentException e) {
+            LOGGER.warn("Failed to deliver {} due to invalid payload URL: {}", webhook, e.getMessage());
+            response = String.format("Failed due to : %s", e.getMessage());
+        } finally {
+            closeHttpClient();
+        }
+        callback.complete(new WebhookDeliveryResult(headers, payload, success, response,
+                startTime != null ? startTime : new Date()));
+    }
+
+    protected void closeHttpClient() {
+        try {
+            httpClient.close();
+        } catch (IOException e) {
+            LOGGER.debug("Failed to close HTTP client for {}: {}", webhook, e.getMessage());
         }
-        callback.complete(new WebhookDeliveryResult(headers, payload, success, response, startTime));
     }
 
     protected void updateResponseFromRequest(HttpEntity entity) {
@@ -271,16 +307,19 @@ protected boolean delivery(int attempt) {
                                 MAX_REDIRECT_HOPS);
                         return false;
                     }
-                    currentUri = WebhookUrlValidator.validateWebhookDestinationURI(
-                            currentUri.resolve(locationHeader.getValue()),
-                            allowHttp,destinationBlocklist, blockLocalAddresses);
+                    currentUri = currentUri.resolve(locationHeader.getValue());
+                    WebhookUrlValidator.validateScheme(currentUri, allowHttp);
                 }
             }
+        } catch (WebhookUrlValidator.BlockedDestinationException e) {
+            LOGGER.warn("Failed to deliver {}, in attempt #{} due to security policy: {}",
+                    webhook, attempt, e.getMessage());
+            response = String.format("Failed due to : %s", e.getMessage());
         } catch (IOException | DecoderException | NoSuchAlgorithmException | InvalidKeyException e) {
             LOGGER.warn("Failed to deliver {}, in attempt #{} due to: {}",
                     webhook, attempt, e.getMessage());
             response = String.format("Failed due to : %s", e.getMessage());
-        } catch (InvalidParameterValueException e) {
+        } catch (InvalidParameterValueException | IllegalArgumentException e) {
             LOGGER.warn("Failed to deliver {}, in attempt #{} due to security policy: {}",
                     webhook, attempt, e.getMessage());
             response = String.format("Failed due to : %s", e.getMessage());

plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidator.java

@@ -19,11 +19,12 @@
 
 import java.net.InetAddress;
 import java.net.URI;
-import java.net.URISyntaxException;
 import java.net.UnknownHostException;
 import java.util.Arrays;
 
 import org.apache.commons.lang3.StringUtils;
+import org.apache.http.conn.DnsResolver;
+import org.apache.http.impl.conn.SystemDefaultDnsResolver;
 
 import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.utils.net.NetUtils;
@@ -42,18 +43,9 @@ public static URI validateWebhookDestinationUrl(final String payloadUrl, final b
     public static URI validateWebhookDestinationURI(final URI uri, final boolean allowHttp,
                                                     final String blocklist, final boolean blockLocalAddresses) {
         validateScheme(uri, allowHttp);
-        InetAddress[] resolved = validateResolvedAddresses(uri, blocklist, blockLocalAddresses);
-        return buildResolvedUri(uri, resolved[0]);
-    }
-
-    private static URI buildResolvedUri(final URI uri, final InetAddress address) {
-        try {
-            return new URI(uri.getScheme(), uri.getUserInfo(), address.getHostAddress(), uri.getPort(),
-                    uri.getPath(), uri.getQuery(), uri.getFragment());
-        } catch (URISyntaxException e) {
-            throw new InvalidParameterValueException(
-                    String.format("Failed to build resolved webhook payload URL from [%s]", uri));
-        }
+        validateResolvedAddresses(uri, blocklist, blockLocalAddresses);
+        // Keep hostname for TLS and Host header
+        return uri;
     }
 
     public static void validateScheme(final URI uri, final boolean allowHttp) {
@@ -93,15 +85,22 @@ public static InetAddress[] validateResolvedAddresses(final URI uri, final Strin
                     String.format("Failed to resolve webhook payload URL host [%s]", host));
         }
 
-        final String[] blockedCidrs = getNormalizedBlocklist(blocklist);
-        for (InetAddress address : resolved) {
+        if (isAnyAddressBlocked(resolved, getNormalizedBlocklist(blocklist), blockLocalAddresses)) {
+            throw new InvalidParameterValueException(
+                    String.format("Webhook payload URL [%s] resolves to a blocked IP address",  uri));
+        }
+        return resolved;
+    }
+
+    static boolean isAnyAddressBlocked(final InetAddress[] addresses, final String[] blockedCidrs,
+            final boolean blockLocalAddresses) {
+        for (InetAddress address : addresses) {
             if ((blockLocalAddresses && isLocalManagementServerAddress(address)) ||
                     NetUtils.isIpInCidrList(address, blockedCidrs)) {
-                throw new InvalidParameterValueException(
-                        String.format("Webhook payload URL [%s] resolves to a blocked IP address",  uri));
+                return true;
             }
         }
-        return resolved;
+        return false;
     }
 
     static boolean isLocalManagementServerAddress(final InetAddress address) {
@@ -120,4 +119,42 @@ public static String[] getNormalizedBlocklist(final String blocklist) {
                 .filter(StringUtils::isNotBlank)
                 .toArray(String[]::new);
     }
+
+    // DnsResolver can only throw UnknownHostException
+    public static class BlockedDestinationException extends UnknownHostException {
+        public BlockedDestinationException(final String host) {
+            super(String.format("Webhook payload URL host [%s] resolves to a blocked IP address", host));
+        }
+    }
+
+    // Validates addresses HttpClient connects to (not with a proxy)
+    public static class DestinationPolicyDnsResolver implements DnsResolver {
+        private final DnsResolver delegate;
+        private final String[] blockedCidrs;
+        private final boolean blockLocalAddresses;
+
+        public DestinationPolicyDnsResolver(final String blocklist, final boolean blockLocalAddresses) {
+            this(SystemDefaultDnsResolver.INSTANCE, blocklist, blockLocalAddresses);
+        }
+
+        public DestinationPolicyDnsResolver(final DnsResolver delegate, final String blocklist,
+                final boolean blockLocalAddresses) {
+            this.delegate = delegate;
+            this.blockedCidrs = getNormalizedBlocklist(blocklist);
+            this.blockLocalAddresses = blockLocalAddresses;
+        }
+
+        @Override
+        public InetAddress[] resolve(final String host) throws UnknownHostException {
+            final InetAddress[] resolved = delegate.resolve(host);
+            if (resolved == null || resolved.length == 0) {
+                throw new UnknownHostException(host);
+            }
+            // Fail closed if any address is blocked
+            if (isAnyAddressBlocked(resolved, blockedCidrs, blockLocalAddresses)) {
+                throw new BlockedDestinationException(host);
+            }
+            return resolved;
+        }
+    }
 }