From 7dede2c264081b37481a97f20bf4e72f03751d91 Mon Sep 17 00:00:00 2001 From: Joe Cavanagh Date: Fri, 11 Nov 2022 00:02:28 -0800 Subject: [PATCH] fix(webhook): Safer defaults and more config for webhook URLs Exclude by default: single-word hostnames (`https://orca`, `https://spin-orca`), the `.spinnaker` domain (a common k8s deployment namespace), common internal-name suffixes (`.local`, `.internal`, `.localdomain`), and all verbatim IP addresses. Add new configuration to specify a list of exclusion patterns. This greatly simplifies configuration, as it is not easy to do complex filtering in a single allow expression. Add new configuration to dynamically exclude domains based on the values of specified environment variables. For example, this can always exclude the k8s namespace Spinnaker is currently running in, long as there is some variable set that specifies what that is. `POD_NAMESPACE` is commonly set by providers, and is included by default along with `ISTIO_META_MESH_ID`, as names in that domain are also resolvable. Also allows `localhost` in all cases if the `rejectLocalhost` flag is `false`, disregarding the name filter. This avoids the need to change the name filter to include all forms of local names while developing. --- .../config/UserConfiguredUrlRestrictions.java | 157 ++++++++++--- .../UserConfiguredUrlRestrictionsSpec.groovy | 222 +++++++++++++++++- 2 files changed, 339 insertions(+), 40 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java index 14d2956d60d..ec657666562 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java @@ -16,18 +16,23 @@ package com.netflix.spinnaker.orca.config; +import com.google.common.net.InetAddresses; import java.net.InetAddress; import java.net.NetworkInterface; +import java.net.SocketException; import java.net.URI; +import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.regex.Pattern; +import java.util.stream.Collectors; import lombok.AllArgsConstructor; import lombok.Data; import lombok.NoArgsConstructor; @@ -37,14 +42,25 @@ public class UserConfiguredUrlRestrictions { @Data public static class Builder { - private String allowedHostnamesRegex = ".*"; + private String allowedHostnamesRegex = + ".*\\..+"; // Exclude anything without a dot, since k8s resolves single-word names private List allowedSchemes = new ArrayList<>(Arrays.asList("http", "https")); private boolean rejectLocalhost = true; private boolean rejectLinkLocal = true; + private boolean rejectVerbatimIps = true; private HttpClientProperties httpClientProperties = new HttpClientProperties(); private List rejectedIps = new ArrayList<>(); // can contain IP addresses and/or IP ranges (CIDR block) + // Blanket exclusion on certain domains + // This default pattern will exclude anything that is suffixed with the excluded domain + private String excludedDomainTemplate = "(?=.+\\.%s$).*\\..+"; + private List excludedDomains = List.of("spinnaker", "local", "localdomain", "internal"); + // Generate exclusion patterns based on the values of environment variables + // Useful for dynamically excluding all requests to the current k8s namespace, for example + private List excludedDomainsFromEnvironment = List.of(); + private List extraExcludedPatterns = List.of(); + public Builder withAllowedHostnamesRegex(String allowedHostnamesRegex) { setAllowedHostnamesRegex(allowedHostnamesRegex); return this; @@ -65,6 +81,11 @@ public Builder withRejectLinkLocal(boolean rejectLinkLocal) { return this; } + public Builder withRejectVerbatimIps(boolean rejectVerbatimIps) { + setRejectVerbatimIps(rejectVerbatimIps); + return this; + } + public Builder withRejectedIps(List rejectedIpRanges) { setRejectedIps(rejectedIpRanges); return this; @@ -75,14 +96,55 @@ public Builder withHttpClientProperties(HttpClientProperties httpClientPropertie return this; } + public Builder withExtraExcludedPatterns(List patterns) { + setExtraExcludedPatterns(patterns); + return this; + } + + String getEnvValue(String envVarName) { + return System.getenv(envVarName); + } + + List getEnvValues(List envVars) { + if (envVars == null) return List.of(); + + return envVars.stream() + .map(this::getEnvValue) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + } + + List compilePatterns(List values, String patternStr, boolean quote) { + if (values == null || patternStr == null) { + return List.of(); + } + + return values.stream() + .map(value -> quote ? Pattern.quote(value) : value) + .map(value -> Pattern.compile(String.format(patternStr, value))) + .collect(Collectors.toList()); + } + public UserConfiguredUrlRestrictions build() { + // Combine and build all excluded domains based on the specified names, env vars, and pattern + List allExcludedDomains = new ArrayList<>(); + allExcludedDomains.addAll(excludedDomains); + allExcludedDomains.addAll(getEnvValues(excludedDomainsFromEnvironment)); + + // Collect any extra patterns and provide the final list of patterns + List allExcludedPatterns = new ArrayList<>(); + allExcludedPatterns.addAll(compilePatterns(allExcludedDomains, excludedDomainTemplate, true)); + allExcludedPatterns.addAll(compilePatterns(extraExcludedPatterns, "%s", false)); + return new UserConfiguredUrlRestrictions( Pattern.compile(allowedHostnamesRegex), allowedSchemes, rejectLocalhost, rejectLinkLocal, + rejectVerbatimIps, rejectedIps, - httpClientProperties); + httpClientProperties, + allExcludedPatterns); } } @@ -90,16 +152,20 @@ public UserConfiguredUrlRestrictions build() { private final Set allowedSchemes; private final boolean rejectLocalhost; private final boolean rejectLinkLocal; + private final boolean rejectVerbatimIps; private final Set rejectedIps; private final HttpClientProperties clientProperties; + private final List excludedPatterns; - public UserConfiguredUrlRestrictions( + protected UserConfiguredUrlRestrictions( Pattern allowedHostnames, Collection allowedSchemes, boolean rejectLocalhost, boolean rejectLinkLocal, + boolean rejectVerbatimIps, Collection rejectedIps, - HttpClientProperties clientProperties) { + HttpClientProperties clientProperties, + List excludedPatterns) { this.allowedHostnames = allowedHostnames; this.allowedSchemes = allowedSchemes == null @@ -107,11 +173,40 @@ public UserConfiguredUrlRestrictions( : Collections.unmodifiableSet(new HashSet<>(allowedSchemes)); this.rejectLocalhost = rejectLocalhost; this.rejectLinkLocal = rejectLinkLocal; + this.rejectVerbatimIps = rejectVerbatimIps; this.rejectedIps = rejectedIps == null ? Collections.emptySet() : Collections.unmodifiableSet(new HashSet<>(rejectedIps)); this.clientProperties = clientProperties; + this.excludedPatterns = excludedPatterns; + } + + InetAddress resolveHost(String host) throws UnknownHostException { + return InetAddress.getByName(host); + } + + boolean isLocalhost(InetAddress addr) throws SocketException { + return addr.isLoopbackAddress() + || Optional.ofNullable(NetworkInterface.getByInetAddress(addr)).isPresent(); + } + + boolean isLinkLocal(InetAddress addr) { + return addr.isLinkLocalAddress(); + } + + boolean isValidHostname(String host) { + return allowedHostnames.matcher(host).matches() + && excludedPatterns.stream().noneMatch(p -> p.matcher(host).matches()); + } + + boolean isValidIpAddress(String host) { + var matcher = new IpAddressMatcher(host); + return rejectedIps.stream().noneMatch(matcher::matches); + } + + boolean isIpAddress(String host) { + return InetAddresses.isInetAddress(host); } public URI validateURI(String url) throws IllegalArgumentException { @@ -130,12 +225,17 @@ public URI validateURI(String url) throws IllegalArgumentException { if (host == null) { String authority = u.getAuthority(); if (authority != null) { - int portIndex = authority.indexOf(":"); - host = (portIndex > -1) ? authority.substring(0, portIndex) : authority; + // Don't attempt to colon-substring ipv6 addresses + if (isIpAddress(authority)) { + host = authority; + } else { + int portIndex = authority.indexOf(":"); + host = (portIndex > -1) ? authority.substring(0, portIndex) : authority; + } } } - if (host == null) { + if (host == null || host.isEmpty()) { throw new IllegalArgumentException("Unable to determine host for the url provided " + url); } @@ -144,37 +244,40 @@ public URI validateURI(String url) throws IllegalArgumentException { "Allowed Hostnames are not set, external HTTP requests are not enabled. Please configure 'user-configured-url-restrictions.allowedHostnamesRegex' in your orca config."); } - if (!allowedHostnames.matcher(host).matches()) { - throw new IllegalArgumentException( - "Host not allowed " + host + ". Host much match " + allowedHostnames.toString() + "."); - } + // Strip ipv6 brackets if present + // InetAddress.getHost() retains them, but other code doesn't quite understand + host = host.replace("[", "").replace("]", ""); - if (rejectLocalhost || rejectLinkLocal) { - InetAddress addr = InetAddress.getByName(host); - if (rejectLocalhost) { - if (addr.isLoopbackAddress() - || Optional.ofNullable(NetworkInterface.getByInetAddress(addr)).isPresent()) { - throw new IllegalArgumentException("invalid address for " + host); - } - } - if (rejectLinkLocal && addr.isLinkLocalAddress()) { - throw new IllegalArgumentException("invalid address for " + host); - } + if (isIpAddress(host) && rejectVerbatimIps) { + throw new IllegalArgumentException("Verbatim IP addresses are not allowed"); } - for (String ip : rejectedIps) { - IpAddressMatcher ipMatcher = new IpAddressMatcher(ip); + var addr = resolveHost(host); + var isLocalhost = isLocalhost(addr); + var isLinkLocal = isLinkLocal(addr); + + if ((isLocalhost && rejectLocalhost) || (isLinkLocal && rejectLinkLocal)) { + throw new IllegalArgumentException("Host not allowed: " + host); + } - if (ipMatcher.matches(host)) { - throw new IllegalArgumentException("address " + host + " is within rejected IPs: " + ip); + if (!isValidHostname(host) && !isIpAddress(host)) { + // If localhost or link local is allowed, that takes precedence over the name filter + // This avoids the need to include local names in the hostname pattern in addition to + // setting the local config flag + if (!(isLocalhost || isLinkLocal)) { + throw new IllegalArgumentException("Host not allowed: " + host); } } + if (!isValidIpAddress(host)) { + throw new IllegalArgumentException("Address not allowed: " + host); + } + return u; } catch (IllegalArgumentException iae) { throw iae; } catch (Exception ex) { - throw new IllegalArgumentException("URI not valid " + url, ex); + throw new IllegalArgumentException("URI not valid: " + url, ex); } } diff --git a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictionsSpec.groovy b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictionsSpec.groovy index 01edecc4107..081211d4438 100644 --- a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictionsSpec.groovy +++ b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictionsSpec.groovy @@ -5,16 +5,22 @@ import spock.lang.Unroll class UserConfiguredUrlRestrictionsSpec extends Specification { + // Don't try to actually resolve hosts, and control the result of determining whether a host is localhost or link local + def spyOn(UserConfiguredUrlRestrictions subject, isLocalhost = false, isLinkLocal = false, isValidIpAddress = true) { + def spy = Spy(subject) + spy.resolveHost(_) >> null + spy.isLocalhost(_) >> isLocalhost + spy.isLinkLocal(_) >> isLinkLocal + spy.isValidIpAddress(_) >> isValidIpAddress + spy + } + @Unroll def 'should verify uri #uri as per restrictions provided'() { given: - UserConfiguredUrlRestrictions config = new UserConfiguredUrlRestrictions.Builder() - .withAllowedHostnamesRegex('^(.+).(.+).com(.*)|^(.\\d+).(.\\d+).(.\\d+).(.\\d+)$') - .withRejectedIps([]) - .withAllowedSchemes(['https']) - .withRejectLocalhost(false) - .withRejectLinkLocal(false) - .build() + UserConfiguredUrlRestrictions config = spyOn(new UserConfiguredUrlRestrictions.Builder() + .withAllowedHostnamesRegex('^(.+).(.+).com(.*)$') + .build()) when: URI validatedUri = config.validateURI(uri) @@ -24,20 +30,106 @@ class UserConfiguredUrlRestrictionsSpec extends Specification { validatedUri where: - uri << ['https://www.test.com', 'https://201.152.178.212', 'https://www.test_underscore.com'] + uri << ['https://www.test.com', 'https://foobar.com', 'https://www.test_underscore.com'] } @Unroll def 'should verify allowedHostnamesRegex is set'() { given: - UserConfiguredUrlRestrictions config = new UserConfiguredUrlRestrictions.Builder() + UserConfiguredUrlRestrictions config = spyOn(new UserConfiguredUrlRestrictions.Builder() .withAllowedHostnamesRegex("") - .withRejectedIps([]) - .withAllowedSchemes(['https']) + .build()) + + when: + config.validateURI(uri) + + then: + thrown(IllegalArgumentException.class) + + where: + uri << ['https://www.test.com', 'https://201.152.178.212', 'https://www.test_underscore.com'] + } + + @Unroll + def 'should exclude common internal URL schemes by default'() { + given: + UserConfiguredUrlRestrictions config = spyOn(new UserConfiguredUrlRestrictions.Builder().build()) + + when: + config.validateURI(uri) + + then: + thrown(IllegalArgumentException.class) + + where: + uri << [ + 'https://orca', + 'https://spin-orca', + 'https://clouddriver.svc.cluster.local', + 'https://echo.internal', + 'https://orca.spinnaker', + 'https://orca/admin', + 'https://orca.spinnaker/admin' + ] + } + + @Unroll + def 'should allow non-internal URLs by default'() { + given: + UserConfiguredUrlRestrictions config = spyOn(new UserConfiguredUrlRestrictions.Builder().build()) + + when: + URI validatedUri = config.validateURI(uri) + + then: + noExceptionThrown() + validatedUri + + where: + uri << ['https://google.com', 'https://spinnaker.io/foo/bar', 'https://echo.external', 'http://foo.bar'] + } + + @Unroll + def 'should reject localhost by default'() { + given: + // Note that this does not use the spy, since we want to actually resolve these + UserConfiguredUrlRestrictions config = new UserConfiguredUrlRestrictions.Builder().build() + + when: + config.validateURI(uri) + + then: + thrown(IllegalArgumentException.class) + + where: + uri << ['https://localhost', 'http://localhost', 'http://127.0.0.1', 'https://::1'] + } + + @Unroll + def 'should accept localhost when configured, regardless of name filter'() { + given: + // Note that this does not use the spy, since we want to actually resolve these + UserConfiguredUrlRestrictions config = new UserConfiguredUrlRestrictions.Builder() + .withAllowedHostnamesRegex("this_definitely_doesnt_match_localhost") .withRejectLocalhost(false) - .withRejectLinkLocal(false) .build() + when: + URI validatedUri = config.validateURI(uri) + + then: + noExceptionThrown() + validatedUri + + where: + uri << ['https://localhost', 'http://localhost'] + } + + @Unroll + def 'rejects verbatim IP addresses by default'() { + given: + UserConfiguredUrlRestrictions config = spyOn(new UserConfiguredUrlRestrictions.Builder().build()) + when: config.validateURI(uri) @@ -45,7 +137,111 @@ class UserConfiguredUrlRestrictionsSpec extends Specification { thrown(IllegalArgumentException.class) where: - uri << ['https://www.test.com', 'https://201.152.178.212', 'https://www.test_underscore.com'] + uri << [ + 'https://192.168.0.1', + 'http://172.16.0.1', + 'http://10.0.0.1', + 'http://155.155.155.155', + 'https://fd12:3456:789a:1::1', + 'https://[fd12:3456:789a:1::1]', + 'https://[fd12:3456:789a:1::1]:8080' + ] + } + + @Unroll + def 'allows verbatim IP addresses if configured'() { + given: + UserConfiguredUrlRestrictions config = spyOn(new UserConfiguredUrlRestrictions.Builder() + .withRejectVerbatimIps(false) + .build()) + + when: + URI validatedUri = config.validateURI(uri) + + then: + noExceptionThrown() + validatedUri + + where: + uri << [ + 'https://192.168.0.1', + 'http://172.16.0.1', + 'http://10.0.0.1', + 'https://fd12:3456:789a:1::1', + 'https://fc12:3456:789a:1::1', + 'https://[fd12:3456:789a:1::1]:8080', + 'https://[fc12:3456:789a:1::1]:8080' + ] + } + + @Unroll + def 'excludes domains based on env vars (#envVar=#envVal) (#uri)'() { + given: + UserConfiguredUrlRestrictions.Builder builder = Spy(new UserConfiguredUrlRestrictions.Builder()) + builder.getEnvValue(envVar) >> envVal + UserConfiguredUrlRestrictions config = spyOn(builder.build()) + + when: + def isValidated = true + try { + config.validateURI(uri) + } catch(IllegalArgumentException ignored) { + isValidated = false + } + + then: + isValidated == shouldValidate + uri + + where: + envVar | envVal | uri | shouldValidate + "POD_NAMESPACE" | "kittens" | "http://fluffy.kittens" | false + "POD_NAMESPACE" | "puppies" | "http://fluffy.kittens" | true + "ISTIO_META_MESH_ID" | "istio.mesh" | "http://fluffy.kittens.istio.colander" | true + "ISTIO_META_MESH_ID" | "istio.mesh" | "http://fluffy.kittens.istio.mesh" | false + "ISTIO_META_MESH_ID" | "istio.mesh" | "http://fluffy.kittens.istiozmesh" | true + "RANDOM_ENV_VAR" | "kittens" | "http://fluffy.kittens" | true } + @Unroll + def 'excludes based on arbitrary extra patterns'() { + given: + UserConfiguredUrlRestrictions config = spyOn(new UserConfiguredUrlRestrictions.Builder() + // Test patterns that exclude any number and any hyphen + .withExtraExcludedPatterns(List.of(".+\\d+.+", ".+-+.+")) + .build()) + + when: + config.validateURI(uri) + + then: + thrown(IllegalArgumentException.class) + + where: + uri << [ + "http://asdf2345.com", + "https://foo-bar.com" + ] + } + + @Unroll + def 'validate normal URLs when arbitrary extra patterns are specified'() { + given: + UserConfiguredUrlRestrictions config = spyOn(new UserConfiguredUrlRestrictions.Builder() + .withExtraExcludedPatterns(List.of("\\d+", "-+")) + .build()) + + when: + URI validatedUri = config.validateURI(uri) + + then: + noExceptionThrown() + validatedUri + + where: + uri << [ + "http://foobar.com", + "https://barfoo.com" + ] + } }