wasm: add restrictions #2797

kyessenov · 2023-05-25T19:29:04Z

Signed-off-by: Kuat Yessenov <[email protected]>

istio-policy-bot · 2023-05-25T19:29:07Z

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

Signed-off-by: Kuat Yessenov <[email protected]>

istio-testing · 2023-05-25T19:31:31Z

@kyessenov: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
release-notes_api	`bbdb217`	link	false	`/test release-notes`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

howardjohn · 2023-05-25T20:33:05Z

extensions/v1alpha1/wasm.proto

+  // The following ABI calls are permitted:
+  // * logging;
+  // * property read.
+  DEFAULT = 0;


I am a bit worried about making the default a reduced set. I get we are in alpha so technically we can make breaking changes here, but I worry this is actually used pretty heavily in production given we have been telling users to stop using EnvoyFilter in favor of WasmPlugin.

My assumption here is most WASM plugin users would break upon upgrade to 1.19 if we do this. Is that accurate?

Yes, I share the concern.

I don't think we have that many users, and the longer we wait, the harder it is to add restrictions. Wasm ABI is not stable, and in fact, a new version is coming which is likely to create transition problems for users regardless.

IMHO, the default should be unrestricted, at least initially.

Perhaps add a warning for a few releases (so that people have time to configure for their needs) and then change it to something more restrictive?

I believe that some Istio providers force update their customers, in which case this would break Istio deployments with Wasm in a pretty bad way.

If we change the default to default-secure and document how people can change the default if they need to, that strikes me as reasonable.

Warning is hard to enforce. Maybe we need some auditing mechanism at runtime that would warn without breaking. I'm only considering this option because it's an alpha API and that's the whole purpose of alpha is to make it better before beta.

Fair enough. Perhaps istioctl could check for WasmPlugin and EnvoyFilter with Wasm extensions CRDs and warn about it?

I'm not necessarily advocating that we don't warn, but WasmFilters are an Alpha feature, which literally states "may be removed or changed without notice" - we don't owe anyone a warning outside of release notes if we change the defaults to something more secure.

Agreed, although it's been alpha for a long time and there is number of customers (including big deployments) that are using it.

I get that - but that is the cost they agreed to bear when they adopted an alpha API. If we know them, notify them as a favor, and change it.

The feature status coin we keep talking about has two sides - if we want to be stricter about moving things up or out, we also need to be stricter about alpha meaning what our public-facing docs say it means - that is, not stable, no compat guarantees, breaks and changes without notice.

If that causes disruption for someone because they weren't checking release notes, then we have a discussion with them as interested parties about how to move the feature to a stability status they are comfortable with.

howardjohn · 2023-05-25T20:44:34Z

extensions/v1alpha1/wasm.proto

+}
+
+// Capability restriction enforces limits on the Wasm ABI calls.
+message CapabilityRestriction {


What does this look like on Envoy side? Do they have the same concept?

This is a proxy-wasm concept - https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/master/include/proxy-wasm/wasm.h#L46, and it's exposed through xDS.

kyessenov · 2023-05-26T17:08:54Z

@PiotrSikora you probably have some thoughts on making Wasm modules more portable.

istio-testing · 2023-06-02T22:03:05Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

PiotrSikora

cc @mpwarres @martijneken to see if they have any comments based on their experience with limiting ABI surface.

PiotrSikora · 2023-05-26T17:58:27Z

extensions/v1alpha1/wasm.proto

+// Event restrictions enforces limits on the callback to the Wasm module.
+message EventRestriction {
+  // Trigger Wasm execution on receiving the request headers.
+  bool OnRequestHeaders = 1;
+  // Trigger Wasm execution on receiving the request body.
+  bool OnRequestBody = 2;
+  // Trigger Wasm execution on receiving the response headers.
+  bool OnResponseHeaders = 3;
+  // Trigger Wasm execution on receiving the response body.
+  bool OnResponseBody = 4;
+}


Please add HTTP trailers and TCP events.

Also, I cannot tell from the description whether setting value to true allows given event to happen or forces it to be ignored (restricted).

PiotrSikora · 2023-05-26T17:59:41Z

extensions/v1alpha1/wasm.proto

+  // The following ABI calls are permitted:
+  // * logging;
+  // * property read.
+  DEFAULT = 0;


IMHO, the default should be unrestricted, at least initially.

Perhaps add a warning for a few releases (so that people have time to configure for their needs) and then change it to something more restrictive?

I believe that some Istio providers force update their customers, in which case this would break Istio deployments with Wasm in a pretty bad way.

hzxuzhonghu · 2023-06-12T01:23:24Z

I think this capability is necessary, but not sure whether it should be in WasmPlugin. If a user can create wasmplugin, he could also set any capability set, how could it really restrict malicious user.

If this could be enforce to per proxy or mesh globally, it can really restrict.

howardjohn · 2023-06-12T14:15:08Z

Similar applies to Pod security context, can restrict with other tooling. I am not opposed to this API, I think the things we will need to resolve though are: * What are the restrictions/groups/etc. I am not an expert here but seems we have a lot of WASM knowledge already in this PR * How to handle the defaulting

…

On Sun, Jun 11, 2023 at 6:23 PM Zhonghu Xu ***@***.***> wrote: I think this capability is necessary, but not sure whether it should be in WasmPlugin. If a user can create wasmplugin, he could also set any capability set, how could it really restrict malicious user. If this could be enforce to per proxy or mesh globally, it can really restrict. — Reply to this email directly, view it on GitHub <#2797 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXNDPGCC2ZETTVIT5LLXKZVRNANCNFSM6AAAAAAYPJDWFY> . You are receiving this because your review was requested.Message ID: ***@***.***>

wasm: add restrictions

d396474

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov requested a review from a team as a code owner May 25, 2023 19:29

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 25, 2023

fix

bbdb217

Signed-off-by: Kuat Yessenov <[email protected]>

howardjohn reviewed May 25, 2023

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 2, 2023

PiotrSikora reviewed Jun 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasm: add restrictions #2797

wasm: add restrictions #2797

kyessenov commented May 25, 2023

istio-policy-bot commented May 25, 2023

istio-testing commented May 25, 2023

howardjohn May 25, 2023

kyessenov May 25, 2023

PiotrSikora May 26, 2023

bleggett May 31, 2023 •

edited

Loading

kyessenov Jun 9, 2023

PiotrSikora Jun 9, 2023

bleggett Jun 9, 2023 •

edited

Loading

PiotrSikora Jun 9, 2023

bleggett Jun 12, 2023 •

edited

Loading

howardjohn May 25, 2023

kyessenov May 25, 2023

kyessenov commented May 26, 2023

istio-testing commented Jun 2, 2023

PiotrSikora left a comment

PiotrSikora May 26, 2023

PiotrSikora May 26, 2023

hzxuzhonghu commented Jun 12, 2023

howardjohn commented Jun 12, 2023 via email

wasm: add restrictions #2797

Are you sure you want to change the base?

wasm: add restrictions #2797

Conversation

kyessenov commented May 25, 2023

istio-policy-bot commented May 25, 2023

istio-testing commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented May 26, 2023

istio-testing commented Jun 2, 2023

PiotrSikora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Jun 12, 2023

howardjohn commented Jun 12, 2023 via email

bleggett May 31, 2023 •

edited

Loading

bleggett Jun 9, 2023 •

edited

Loading

bleggett Jun 12, 2023 •

edited

Loading