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" + ] + } }