diff --git a/plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookDeliveryThread.java b/plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookDeliveryThread.java index e68fcc2f30c0..9ea18b0f847c 100644 --- a/plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookDeliveryThread.java +++ b/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 socketFactoryRegistry = RegistryBuilder.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()); diff --git a/plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidator.java b/plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidator.java index c0fea96b6653..8cd30d34be5f 100644 --- a/plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidator.java +++ b/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; + } + } } diff --git a/plugins/event-bus/webhook/src/test/java/org/apache/cloudstack/mom/webhook/WebhookDeliveryThreadTlsTest.java b/plugins/event-bus/webhook/src/test/java/org/apache/cloudstack/mom/webhook/WebhookDeliveryThreadTlsTest.java new file mode 100644 index 000000000000..ad70301d7cdb --- /dev/null +++ b/plugins/event-bus/webhook/src/test/java/org/apache/cloudstack/mom/webhook/WebhookDeliveryThreadTlsTest.java @@ -0,0 +1,213 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.mom.webhook; + +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.UnknownHostException; +import java.nio.charset.StandardCharsets; +import java.security.KeyPair; +import java.security.KeyStore; +import java.security.cert.Certificate; +import java.security.cert.X509Certificate; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; + +import javax.net.ssl.ExtendedSSLSession; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SNIHostName; +import javax.net.ssl.SNIServerName; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLSession; +import javax.net.ssl.TrustManagerFactory; + +import org.apache.cloudstack.framework.events.Event; +import org.apache.cloudstack.utils.security.CertUtils; +import org.apache.http.conn.DnsResolver; +import org.apache.http.conn.ssl.SSLConnectionSocketFactory; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.Mockito; + +import com.sun.net.httpserver.HttpsConfigurator; +import com.sun.net.httpserver.HttpsExchange; +import com.sun.net.httpserver.HttpsServer; + +public class WebhookDeliveryThreadTlsTest { + private static final String HOSTNAME = "webhook.test"; + private static final String BLOCKED_HOSTNAME = "internal.test"; + private static final char[] KEYSTORE_PASSWORD = "password".toCharArray(); + + private static SSLContext serverSslContext; + private static SSLContext clientSslContext; + + private HttpsServer server; + private int port; + private final List receivedHostHeaders = new CopyOnWriteArrayList<>(); + private final List receivedSniHostNames = new CopyOnWriteArrayList<>(); + + @BeforeClass + public static void setUpTls() throws Exception { + KeyPair keyPair = CertUtils.generateRandomKeyPair(2048); + X509Certificate certificate = CertUtils.generateV3Certificate(null, keyPair, keyPair.getPublic(), + "CN=" + HOSTNAME, "SHA256WithRSAEncryption", 1, Collections.singletonList(HOSTNAME), null); + + KeyStore keyStore = KeyStore.getInstance("PKCS12"); + keyStore.load(null, null); + keyStore.setKeyEntry("server", keyPair.getPrivate(), KEYSTORE_PASSWORD, new Certificate[] {certificate}); + KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); + kmf.init(keyStore, KEYSTORE_PASSWORD); + serverSslContext = SSLContext.getInstance("TLS"); + serverSslContext.init(kmf.getKeyManagers(), null, null); + + KeyStore trustStore = KeyStore.getInstance("PKCS12"); + trustStore.load(null, null); + trustStore.setCertificateEntry("server", certificate); + TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + tmf.init(trustStore); + clientSslContext = SSLContext.getInstance("TLS"); + clientSslContext.init(null, tmf.getTrustManagers(), null); + } + + @Before + public void startServer() throws Exception { + server = HttpsServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0); + server.setHttpsConfigurator(new HttpsConfigurator(serverSslContext)); + server.createContext("/hook", exchange -> { + recordRequest((HttpsExchange) exchange); + byte[] body = "ok".getBytes(StandardCharsets.UTF_8); + exchange.sendResponseHeaders(200, body.length); + try (OutputStream os = exchange.getResponseBody()) { + os.write(body); + } + }); + server.createContext("/redirect-blocked", exchange -> { + recordRequest((HttpsExchange) exchange); + exchange.getResponseHeaders().add("Location", + String.format("https://%s:%d/hook", BLOCKED_HOSTNAME, port)); + exchange.sendResponseHeaders(302, -1); + exchange.close(); + }); + server.start(); + port = server.getAddress().getPort(); + } + + @After + public void stopServer() { + server.stop(0); + } + + private void recordRequest(HttpsExchange exchange) { + receivedHostHeaders.add(exchange.getRequestHeaders().getFirst("Host")); + SSLSession session = exchange.getSSLSession(); + if (session instanceof ExtendedSSLSession) { + for (SNIServerName name : ((ExtendedSSLSession) session).getRequestedServerNames()) { + if (name instanceof SNIHostName) { + receivedSniHostNames.add(((SNIHostName) name).getAsciiName()); + } + } + } + } + + private WebhookDeliveryThread.WebhookDeliveryResult deliver(String payloadUrl, String blocklist, + boolean allowRedirects) { + Webhook webhook = Mockito.mock(Webhook.class); + Mockito.when(webhook.isSslVerification()).thenReturn(true); + Mockito.when(webhook.getPayloadUrl()).thenReturn(payloadUrl); + Event event = new Event("test", "TEST", "TEST.WEBHOOK", null, null); + event.setEventUuid("event-uuid"); + event.setDescription("{\"test\":\"hello\"}"); + + Map hosts = Map.of(HOSTNAME, "127.0.0.1", BLOCKED_HOSTNAME, "10.0.0.1"); + DnsResolver testDnsResolver = host -> { + String ip = hosts.get(host); + if (ip == null) { + throw new UnknownHostException(host); + } + return new InetAddress[] {InetAddress.getByName(ip)}; + }; + + AtomicReference result = new AtomicReference<>(); + WebhookDeliveryThread thread = new WebhookDeliveryThread(webhook, event, result::set) { + @Override + protected SSLConnectionSocketFactory getSslConnectionSocketFactory() { + return new SSLConnectionSocketFactory(clientSslContext); + } + + @Override + protected DnsResolver getDnsResolver() { + return new WebhookUrlValidator.DestinationPolicyDnsResolver(testDnsResolver, blocklist, false); + } + }; + thread.setDeliveryTries(1); + thread.setDeliveryTimeout(5); + thread.setAllowRedirects(allowRedirects); + thread.run(); + Assert.assertNotNull(result.get()); + return result.get(); + } + + @Test + public void testDeliveryUsesHostnameForTlsAndHostHeader() { + WebhookDeliveryThread.WebhookDeliveryResult result = + deliver(String.format("https://%s:%d/hook", HOSTNAME, port), "10.0.0.0/8", false); + + Assert.assertTrue(result.getResult(), result.isSuccess()); + Assert.assertEquals("ok", result.getResult()); + Assert.assertEquals(List.of(HOSTNAME + ":" + port), receivedHostHeaders); + Assert.assertEquals(List.of(HOSTNAME), receivedSniHostNames); + } + + @Test + public void testDeliveryBlockedWhenHostResolvesToBlockedAddress() { + WebhookDeliveryThread.WebhookDeliveryResult result = + deliver(String.format("https://%s:%d/hook", HOSTNAME, port), "127.0.0.0/8", false); + + Assert.assertFalse(result.isSuccess()); + Assert.assertTrue(result.getResult(), result.getResult().contains("blocked IP address")); + Assert.assertTrue(receivedHostHeaders.isEmpty()); + } + + @Test + public void testDeliveryBlockedWhenRedirectTargetResolvesToBlockedAddress() { + WebhookDeliveryThread.WebhookDeliveryResult result = + deliver(String.format("https://%s:%d/redirect-blocked", HOSTNAME, port), "10.0.0.0/8", true); + + Assert.assertFalse(result.isSuccess()); + Assert.assertTrue(result.getResult(), result.getResult().contains(BLOCKED_HOSTNAME)); + Assert.assertTrue(result.getResult(), result.getResult().contains("blocked IP address")); + Assert.assertEquals(1, receivedHostHeaders.size()); + } + + @Test + public void testDeliveryCompletesCallbackForInvalidScheme() { + WebhookDeliveryThread.WebhookDeliveryResult result = + deliver(String.format("http://%s:%d/hook", HOSTNAME, port), "", false); + + Assert.assertFalse(result.isSuccess()); + Assert.assertTrue(result.getResult(), result.getResult().contains("Only HTTPS")); + Assert.assertTrue(receivedHostHeaders.isEmpty()); + } +} diff --git a/plugins/event-bus/webhook/src/test/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidatorTest.java b/plugins/event-bus/webhook/src/test/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidatorTest.java index 8ea84733d036..3b1103763f63 100644 --- a/plugins/event-bus/webhook/src/test/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidatorTest.java +++ b/plugins/event-bus/webhook/src/test/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidatorTest.java @@ -19,6 +19,7 @@ import java.net.InetAddress; import java.net.URI; +import java.net.UnknownHostException; import org.junit.Assert; import org.junit.Test; @@ -92,6 +93,47 @@ public void testValidateWebhookDestinationUrlAllowsLoopbackWhenLocalAddressBlock Assert.assertEquals(URI.create("https://127.0.0.1/hook"), resolvedUri); } + @Test + public void testValidateWebhookDestinationUrlKeepsHostname() { + URI uri = WebhookUrlValidator.validateWebhookDestinationUrl("https://localhost:8443/hook?a=b", false, "", + false); + Assert.assertEquals(URI.create("https://localhost:8443/hook?a=b"), uri); + } + + @Test + public void testDestinationPolicyDnsResolverReturnsAllowedAddresses() throws Exception { + InetAddress[] addresses = {InetAddress.getByName("8.8.8.8"), InetAddress.getByName("8.8.4.4")}; + WebhookUrlValidator.DestinationPolicyDnsResolver resolver = + new WebhookUrlValidator.DestinationPolicyDnsResolver(host -> addresses, "10.0.0.0/8", true); + Assert.assertArrayEquals(addresses, resolver.resolve("webhook.test")); + } + + @Test + public void testDestinationPolicyDnsResolverFailsClosedWhenAnyAddressIsBlocked() throws Exception { + InetAddress[] addresses = {InetAddress.getByName("8.8.8.8"), InetAddress.getByName("10.1.1.1")}; + WebhookUrlValidator.DestinationPolicyDnsResolver resolver = + new WebhookUrlValidator.DestinationPolicyDnsResolver(host -> addresses, "10.0.0.0/8", true); + try { + resolver.resolve("webhook.test"); + Assert.fail("Expected BlockedDestinationException"); + } catch (WebhookUrlValidator.BlockedDestinationException e) { + Assert.assertTrue(e.getMessage().contains("webhook.test")); + } + } + + @Test(expected = WebhookUrlValidator.BlockedDestinationException.class) + public void testDestinationPolicyDnsResolverBlocksLoopback() throws Exception { + InetAddress[] addresses = {InetAddress.getByName("127.0.0.1")}; + new WebhookUrlValidator.DestinationPolicyDnsResolver(host -> addresses, "", true) + .resolve("webhook.test"); + } + + @Test(expected = UnknownHostException.class) + public void testDestinationPolicyDnsResolverFailsOnEmptyResolution() throws Exception { + new WebhookUrlValidator.DestinationPolicyDnsResolver(host -> new InetAddress[0], "", true) + .resolve("webhook.test"); + } + @Test public void testGetNormalizedBlocklist() { String[] cidrs = WebhookUrlValidator.getNormalizedBlocklist(" 127.0.0.0/8, ::1/128 ,, 192.168.0.0/16 ");