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`), 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.

Author: jcavanagh

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,27 @@
 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<String> 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<String> 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<String> excludedDomains = List.of("spinnaker", "local", "internal");
+    // Generate exclusion patterns based on the values of environment variables
+    // Useful for dynamically excluding all webhook requests to the current k8s namespace, for
+    // example
+    private List<String> excludedDomainsFromEnvironment =
+        List.of("POD_NAMESPACE", "ISTIO_META_MESH_ID");
+    private List<String> extraExcludedPatterns = List.of();
+
     public Builder withAllowedHostnamesRegex(String allowedHostnamesRegex) {
       setAllowedHostnamesRegex(allowedHostnamesRegex);
       return this;
@@ -65,6 +83,11 @@ public Builder withRejectLinkLocal(boolean rejectLinkLocal) {
       return this;
     }
 
+    public Builder withRejectVerbatimIps(boolean rejectVerbatimIps) {
+      setRejectVerbatimIps(rejectVerbatimIps);
+      return this;
+    }
+
     public Builder withRejectedIps(List<String> rejectedIpRanges) {
       setRejectedIps(rejectedIpRanges);
       return this;
@@ -75,43 +98,117 @@ public Builder withHttpClientProperties(HttpClientProperties httpClientPropertie
       return this;
     }
 
+    public Builder withExtraExcludedPatterns(List<String> patterns) {
+      setExtraExcludedPatterns(patterns);
+      return this;
+    }
+
+    String getEnvValue(String envVarName) {
+      return System.getenv(envVarName);
+    }
+
+    List<String> getEnvValues(List<String> envVars) {
+      if (envVars == null) return List.of();
+
+      return envVars.stream()
+          .map(this::getEnvValue)
+          .filter(Objects::nonNull)
+          .collect(Collectors.toList());
+    }
+
+    List<Pattern> compilePatterns(List<String> 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<String> allExcludedDomains = new ArrayList<>();
+      allExcludedDomains.addAll(excludedDomains);
+      allExcludedDomains.addAll(getEnvValues(excludedDomainsFromEnvironment));
+
+      // Collect any extra patterns and provide the final list of patterns
+      List<Pattern> 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);
     }
   }
 
   private final Pattern allowedHostnames;
   private final Set<String> allowedSchemes;
   private final boolean rejectLocalhost;
   private final boolean rejectLinkLocal;
+  private final boolean rejectVerbatimIps;
   private final Set<String> rejectedIps;
   private final HttpClientProperties clientProperties;
+  private final List<Pattern> excludedPatterns;
 
-  public UserConfiguredUrlRestrictions(
+  protected UserConfiguredUrlRestrictions(
       Pattern allowedHostnames,
       Collection<String> allowedSchemes,
       boolean rejectLocalhost,
       boolean rejectLinkLocal,
+      boolean rejectVerbatimIps,
       Collection<String> rejectedIps,
-      HttpClientProperties clientProperties) {
+      HttpClientProperties clientProperties,
+      List<Pattern> excludedPatterns) {
     this.allowedHostnames = allowedHostnames;
     this.allowedSchemes =
         allowedSchemes == null
             ? Collections.emptySet()
             : 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 +227,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 +246,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);
     }
   }