fix(webhook): Safer defaults and more config for webhook URLs #4333

jcavanagh · 2022-11-11T18:20:41Z

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.

jcavanagh

I don't expect any breakage from these default filters, since it would be pretty unusual to be using a name or address within these patterns.

Feedback on additional default exclusions is welcome!

jcavanagh · 2022-11-11T18:21:21Z

orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java


-  public UserConfiguredUrlRestrictions(
+  protected UserConfiguredUrlRestrictions(


The visibility of this constructor was reduced to mandate the use of the Builder

jcavanagh · 2022-11-11T18:22:46Z

orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java

@@ -144,37 +237,42 @@ 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() + ".");


A couple of these messages were trimmed down, since there is no need to expose that configuration.

jcavanagh · 2022-11-11T18:23:59Z

...e/src/test/groovy/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictionsSpec.groovy

@@ -5,16 +5,21 @@ 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) {


Most of the builder arguments in these tests were to dodge code paths that attempted to resolve names, so that has been mocked out and the non-relevant builder arguments removed from the below tests. Those code paths are now more exercised than before.

jcavanagh · 2022-11-11T18:26:12Z

...e/src/test/groovy/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictionsSpec.groovy

+        'https://192.168.0.1',
+        'http://172.16.0.1',
+        'http://10.0.0.1',
+        'https://fd12:3456:789a:1::1',


The ipv6 standard is pretty weird, since the : is rather overloaded - in any case, the code within can now exclude them in both forms.

jasonmcintosh · 2022-11-11T18:32:50Z

orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java

+    // 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");


I could see adding... localdomain svc.local and some other common patterns, but a good start ;)

svc.local is already covered by local. I'm not familiar with anything that uses localdomain, though - which tooling typically uses that?

Localdomain was an oldddd unix local reference. Still around I think PRETTY regularly.
https://bbs.archlinux.org/viewtopic.php?id=156064 for example.... swear I've seen it even in hosts recently

I've added localdomain by default - doesn't seem to me like there is any downside.

jasonmcintosh · 2022-11-11T18:36:28Z

orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java

+  boolean isLinkLocal(InetAddress addr) {
+    return addr.isLinkLocalAddress();
+  }


PERSONALLY not a fan of one line function wrappers like this... I'd rather have the link local direct, as it's a bit clearer to me...

This allows the tests to mock this functionality more easily, since it doesn't need to capture calls to InetAddress, just itself

ahh ok yeah missed that!

jasonmcintosh · 2022-11-11T18:41:56Z

...e/src/test/groovy/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictionsSpec.groovy

+        'https://orca',
+        'https://clouddriver.svc.cluster.local',
+        'https://echo.internal',
+        'https://orca.spinnaker',


Sould check on https://spin-orca/ as I believe that's a very common one of the patterns... just on principle (looks like the regex SHOULD catch it).

I've added spin-orca to that test

jasonmcintosh · 2022-11-11T18:48:51Z

Note assuming merge... we'll want to call out this change in release notes, as it'd cause any hook to an internal call in another namespace to likely break. Can ALSO impact hooks to other private CIDRs like private VPCs on a TGW/peer... so LIKELY a breaking change for most.

jcavanagh · 2022-11-11T19:11:20Z

as it'd cause any hook to an internal call in another namespace to likely break

Other k8s namespaces should work just fine, so long as they are not named spinnaker, local or internal.

Can ALSO impact hooks to other private CIDRs like private VPCs on a TGW/peer... so LIKELY a breaking change for most.

Unfortunately, specifying private IPs verbatim in the webhook bypasses ___domain filtering, and could allow resolution of internal Spinnaker services. Alternatively, we could just disallow any literal IP address in the webhook configuration.

jasonmcintosh

SO after further looking - this breaks the IP restriction where if a "PASS" host lands in a "REJECT" Ip range, the request would fail. This PR makes that no longer happen :(

jasonmcintosh · 2022-11-11T20:46:39Z

@dbyron-sf or @kskewes-sf FYI on this one...

jcavanagh · 2022-11-11T22:26:12Z

@jasonmcintosh I've made the following changes to the implementation:

Verbatim IP addresses are now rejected by default
IP filtering is again considered against the resolved address
No IP ranges are excluded by default

Trying to filter internal requests by IP is an unwinnable game of whack-a-mole. I'll give some thought to how we might recharacterize user-initiated requests in general, so that they can be rejected by internal APIs without any special configuration.

dbyron-sf · 2022-11-11T22:53:01Z

In general I support this. I'm a little hesitant because I get the feeling it changes existing behavior. Could you @jcavanagh perhaps write the release note for it so we can see how it affects users? Are we releasing versions of spinnaker fast enough to do this in two stages -- one adds the knobs, but doesn't change behavior, and the second one changes the defaults?

dbyron-sf · 2022-11-11T22:55:21Z

orca-core/src/main/java/com/netflix/spinnaker/orca/config/UserConfiguredUrlRestrictions.java

+    // 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");


My reflex is to leave these defaults out...I guess they're no-ops for people outside of k8s / not using istio...but somebody somewhere is gonna have these set and we're gonna mess them up. If they do stay in, I'd love links to docs about them in comments.

dbyron-sf · 2022-11-11T22:57:48Z

The description implies this, but I want to go ahead and write it out -- the idea here is that someone making calls into spinnaker itself in a webhook stage is probably doing something unsavory, and so let's forbid it by default, right?

jcavanagh · 2022-11-12T00:16:56Z

The description implies this, but I want to go ahead and write it out -- the idea here is that someone making calls into spinnaker itself in a webhook stage is probably doing something unsavory, and so let's forbid it by default, right?

Yes, that's correct. This is not a complete fix, but does enhance the ability of operators to mitigate the most probable vector (Webhook stages) specifically. The main issues here are toning down the flexibility of k8s DNS, and disallowing exhaustive attacks within a generally-constrained IP space.

perhaps write the release note for it so we can see how it affects users? Are we releasing versions of spinnaker fast enough to do this in two stages -- one adds the knobs, but doesn't change behavior, and the second one changes the defaults?

I'm not opposed to that - we could move those env-var values to the release notes as recommendations instead.

Disallowing any IP address by default could certainly be a breaking change, but I don't think there's any real justification for such a configuration. If an operator wishes, the option can be disabled.

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.

jasonmcintosh · 2022-11-14T17:32:31Z

ORIGINALLY the plan was "ok let's break this and make it a DENY ALL by default, then REQUIRE end users to whitelist any supported webhook targets". AKA you'd want to manually add either a list or a regex. BUT make it secure default vs. not.

The current implemetnation allows calls to even internal API's which it really really shouldn't as this lets you bypass some restrictions. Link-local blocks help a TON (aka blocking ec2 metadata endpoints), but... ideally should block cluster addresses as well for these, and only allow cluster-external calls.

jcavanagh · 2022-11-14T18:45:13Z

ORIGINALLY the plan was "ok let's break this and make it a DENY ALL by default, then REQUIRE end users to whitelist any supported webhook targets". AKA you'd want to manually add either a list or a regex. BUT make it secure default vs. not.

When is "originally"? This change was never a deny-all implementation, nor is that something that I think operators would want. I might be missing some context here.

The current implemetnation allows calls to even internal API's which it really really shouldn't as this lets you bypass some restrictions. Link-local blocks help a TON (aka blocking ec2 metadata endpoints), but... ideally should block cluster addresses as well for these, and only allow cluster-external calls.

I think you may have missed the most recent changes - see my other comment here.

jasonmcintosh · 2022-11-15T22:05:35Z

#4214 was a PR to discuss changing the IP/DNS restrictions. However, the "Break it and require overrides to be set" was backed out to prevent a breaking change. Via commit: 4e25319#diff-6d7eea546c2be1a74db8daa4f32860c679584686c8cc7cd1c3f6fe6a199f8918R40

dbyron-sf · 2022-12-13T22:11:21Z

@Mergifyio update

mergify · 2022-12-13T22:11:57Z

update

✅ Branch has been successfully updated

jcavanagh commented Nov 11, 2022

View reviewed changes

jasonmcintosh approved these changes Nov 11, 2022

View reviewed changes

jasonmcintosh requested changes Nov 11, 2022

View reviewed changes

jcavanagh force-pushed the upstream-webhook-mitigations branch 2 times, most recently from f1a47b1 to d0b44da Compare November 11, 2022 22:17

dbyron-sf reviewed Nov 11, 2022

View reviewed changes

jcavanagh force-pushed the upstream-webhook-mitigations branch from d0b44da to 7dede2c Compare November 12, 2022 00:19

jcavanagh force-pushed the upstream-webhook-mitigations branch from 7dede2c to 9548ead Compare November 12, 2022 00:26

Merge branch 'master' into upstream-webhook-mitigations

76e7d76

Merge branch 'master' into upstream-webhook-mitigations

e874ec9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(webhook): Safer defaults and more config for webhook URLs #4333

fix(webhook): Safer defaults and more config for webhook URLs #4333

jcavanagh commented Nov 11, 2022 •

edited

Loading

jcavanagh left a comment

jcavanagh Nov 11, 2022

jcavanagh Nov 11, 2022

jcavanagh Nov 11, 2022

jcavanagh Nov 11, 2022

jasonmcintosh Nov 11, 2022

jcavanagh Nov 11, 2022

jasonmcintosh Nov 11, 2022

jcavanagh Nov 11, 2022

jasonmcintosh Nov 11, 2022

jcavanagh Nov 11, 2022

jasonmcintosh Nov 11, 2022

jasonmcintosh Nov 11, 2022

jcavanagh Nov 11, 2022

jasonmcintosh commented Nov 11, 2022

jcavanagh commented Nov 11, 2022

jasonmcintosh left a comment

jasonmcintosh commented Nov 11, 2022

jcavanagh commented Nov 11, 2022 •

edited

Loading

dbyron-sf commented Nov 11, 2022

dbyron-sf Nov 11, 2022

dbyron-sf commented Nov 11, 2022

jcavanagh commented Nov 12, 2022

jasonmcintosh commented Nov 14, 2022

jcavanagh commented Nov 14, 2022

jasonmcintosh commented Nov 15, 2022

dbyron-sf commented Dec 13, 2022

mergify bot commented Dec 13, 2022


		public UserConfiguredUrlRestrictions(
		protected UserConfiguredUrlRestrictions(

fix(webhook): Safer defaults and more config for webhook URLs #4333

Are you sure you want to change the base?

fix(webhook): Safer defaults and more config for webhook URLs #4333

Conversation

jcavanagh commented Nov 11, 2022 • edited Loading

jcavanagh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmcintosh commented Nov 11, 2022

jcavanagh commented Nov 11, 2022

jasonmcintosh left a comment

Choose a reason for hiding this comment

jasonmcintosh commented Nov 11, 2022

jcavanagh commented Nov 11, 2022 • edited Loading

dbyron-sf commented Nov 11, 2022

Choose a reason for hiding this comment

dbyron-sf commented Nov 11, 2022

jcavanagh commented Nov 12, 2022

jasonmcintosh commented Nov 14, 2022

jcavanagh commented Nov 14, 2022

jasonmcintosh commented Nov 15, 2022

dbyron-sf commented Dec 13, 2022

mergify bot commented Dec 13, 2022

✅ Branch has been successfully updated

jcavanagh commented Nov 11, 2022 •

edited

Loading

jcavanagh commented Nov 11, 2022 •

edited

Loading