wasm: add restrictions #2797
wasm: add restrictions #2797
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
🤔 🐛 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. |
istio-testing
added
the
size/L
|
@kyessenov: The following test failed, say
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. |
| // The following ABI calls are permitted: | ||
| // * logging; | ||
| // * property read. | ||
| DEFAULT = 0; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fair enough. Perhaps istioctl could check for WasmPlugin and EnvoyFilter with Wasm extensions CRDs and warn about it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed, although it's been alpha for a long time and there is number of customers (including big deployments) that are using it.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Capability restriction enforces limits on the Wasm ABI calls. | ||
| message CapabilityRestriction { |
There was a problem hiding this comment.
What does this look like on Envoy side? Do they have the same concept?
There was a problem hiding this comment.
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.
|
@PiotrSikora you probably have some thoughts on making Wasm modules more portable. |
istio-testing
added
the
needs-rebase
|
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. |
There was a problem hiding this comment.
cc @mpwarres @martijneken to see if they have any comments based on their experience with limiting ABI surface.
| // 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; | ||
| } |
There was a problem hiding this comment.
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).
| // The following ABI calls are permitted: | ||
| // * logging; | ||
| // * property read. | ||
| DEFAULT = 0; |
There was a problem hiding this comment.
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.
|
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. |
|
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:
***@***.***>
|
Fixes istio/istio#45118.