Implement a capability restriction system.

[ryanapilado]

Implement a mechanism to restrict which proxy-wasm ABI functions are made available to the module. Functions which are restricted are replaced with a stub.

Envoy-side implementation and tests.

Signed-off-by: Ryan Apilado [email protected]

[PiotrSikora]

Apologies for the late review.

[PiotrSikora]

I don't recall the reason, but John removed all uses of absl from this repo. Is there any reason why we need absl::flat_hash_set here instead of using std::unordered_map?

[ryanapilado]

I need to create allowed_capabilities on the Envoy side and pass it to the WasmBase constructor, but the Envoy formatter complains when I use unordered_set or unordered_map. So I had to switch it to absl::flat_hash_set in both Envoy and here.

But I see that there is some code in Envoy that uses unordered_set anyways, like here. Should I do the same?

[PiotrSikora]

Ah, OK. Let's leave it then.

[mathetake]

FYI, https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/master/src/v8/v8.cc#L28-L29 John did not remove absl from V8

[PiotrSikora]

Any thoughts on absl::flat_hash_set, @mathetake? Should we completely remove it? Removing it from V8 would be trivial.

[ryanapilado]

Re: #89 (comment)

Should I change it to a std::unordered_map then? Again, it goes against Envoy style guidelines, and at the moment, there are no uses of std::unordered_map in Envoy.

[mathetake]

OK then, let's follow Envoy's style guide and stick to absl everywhere in this repo too. What I wanted was consistency. After merging, could you please replace std::unordered_** with abls ones?

[ryanapilado]

Ok, sounds good!

[PiotrSikora]

The reason for no absl is that proxy-wasm-cpp-host can be used (at least in theory) by other proxies, not only Envoy, and we didn't want to force absl as a dependency for them when the same containers are available in std.

You should be able to "fix" the formatter warnings by adding relevant files to a whitelist (there are many of those for various formatter exclusions). Envoy has no business enforcing argument types in 3rd-party libraries.

[ryanapilado]

Ok, makes sense. Changed to std::unordered_map here and in Envoy PR.

[PiotrSikora]

@mathetake @kyessenov could you take a look as well?

[mathetake]

👍 to 2) because it’s more independent of ABI versions and developer friendly.

[mathetake]

But then should we discuss this in proxy-wasm/spec first? I think we would better have more comments from the folks including SDK developers

[ryanapilado]

I think (2) definitely needs to be done, eventually. There are dependencies between some of the capabilities which aren't obvious (e.g. proxy_on_vm_start won't work without proxy_get_property and proxy_on_context_create), so we should be bundling them together into functional packages to make it developer-friendly.

But maybe also having (1) could also be useful, for cases where more granular restriction is needed? (do such cases exist, or could we cover all reasonable restriction policies with the groups defined in (2)? I don't know). For example, maybe a user only wants to allow a subset of the gRPC capabilities, not all of them. And there are capabilities like proxy_log, proxy_get_shared_data which seem to be standalone, so they should be allowed individually. In principle, it also seems good to give the developer access to low-level control if they want it. So maybe it makes sense to have both (1) and (2)?

So maybe allowed_capabilities should just be a mix of both single capabilities (1) and groups of capabilities (2):

capability_restriction_config:
  allowed_capabilities:
    - http_request_headers_read,     // implies: on_http_request_headers, get_header_map(HttpRequestHeaders), get_header_map_value(HttpRequestHeaders), 
    - http_request_headers_write,    // implies: on_http_request_headers, get_header_map(HttpRequestHeaders), get_header_map_value(HttpRequestHeaders), set_header_map(HttpRequestHeaders), set_header_map_value(HttpRequestHeaders), 
    - http_request_body_write,       // implies: on_http_request_body, get_buffer_bytes(HttpRequest), set_buffer_bytes(HttpRequest)
    - grpc_call: ["www.google.com"], // implies: grpc_call, grpc_stream, grpc_close, grpc_cancel, grpc_send
    - proxy_log
    - proxy_get_shared_data

Internally, the capability bundles (http_request_headers_read etc.) are translated to single capabilities and used to construct the Wasm.

[ryanapilado]

Updated to make allowed_capabilities a map. The key is the capability name, and the value is a vector of strings, as above. The map value is ignored for now, but in later PRs can be used to implement either (1) or (2).

Also updated the Envoy PR to match.

[PiotrSikora]

@ryanapilado could you merge master to resolve conflicts? Thanks!

Regarding the (1) vs (2), WASI uses rights that pretty much map 1:1 with function names (although, there are a few rights that grant permission to call multiple functions), which is much closer to (1) than (2). Unfortunately, they don't limit parameters, so we need to diverge from them.

Also, (2) can be always built on top of (1), but not the vice-versa, so let's get this in, and we can worry about (2) later.

@mathetake could you review this?

[mathetake]

nit: @PiotrSikora we should have a new Wasm result like Unauthorized as WASI's acces Permission denied. (hopefully in the next ABI ). https://github.com/WebAssembly/WASI/blob/master/phases/snapshot/docs.md#variants-1

[ryanapilado]

Resolved conflicts, merged master.

[ryanapilado]

Adding a small change in anticipation of implementing (1), PTAL. It's not enough to have a vector of strings, we also need a parameter to control whether it behaves as an allowlist or denylist. So the capabilities now map to a struct SanitizationConfig, which contains a vector of strings argument_list but also an enum ListType which determines if the former is an allowlist or a denylist. This sets up the implementation of (1) and also makes the Envoy PR cleaner.

[PiotrSikora]

LGTM sans the nit.

@ryanapilado could you fix the Envoy PR? I would like to merge this, but Envoy PR doesn't even build right now. Thanks.

[ryanapilado]

LGTM sans the nit.

@ryanapilado could you fix the Envoy PR? I would like to merge this, but Envoy PR doesn't even build right now. Thanks.

My bad, Envoy PR seems ok now.

[PiotrSikora]

For WASI, I think it would be better if we had a dedicated stub that returns __WASI_ENOTCAPABLE (76), since WASI modules should be aware of that error code, and they know nothing about Proxy-Wasm return values.