SIP for dependency isolation - #3708
Conversation
dicej
left a comment
There was a problem hiding this comment.
Thanks for writing this up!
| The actual name of the named import doesn't matter: it is a private contract between the capabilities | ||
| composer and the host trait. What matters is: | ||
|
|
||
| 1. Dependencies should not be able to forge import names. If a malicious developer crafts a component |
There was a problem hiding this comment.
I'm not sure I understand the value the GUIDs would add here. If we were to deny-adapt dependency imports by default (regardless of whether they're named imports or not) and require that the app developer specify explicitly which imports they want to make available to the dependency in a restricted or unrestricted form, there would be no way for the dependency to forge anything.
In your example, a dependency that named-imports super-secret-db-password would have that import denied by default, and the app developer would have to explicitly indicate if they wanted it hooked up to anything.
I guess I don't understand what the "and persuades me to use it in a composition" scenario looks like and how GUIDs would help avoid it.
There was a problem hiding this comment.
Hmm, maybe you're right. I am not sure how the deny adapter would apply itself to named imports. But I worry there is still a case where a dependency has a legitimate import (dependency imports SQLite and this is known and allowed), which will be mapped to a named import, but the dependency also forges a pre-named import of the same interface. In the prototype, I link all the dep's named imports. But this could be somewhat mitigated by passing the list of "names we made up" and only linking those. But I still worry that there is a more-complicated-still scenario where Evil Dep forges a name that it guesses will be used by Good Dep and we can't tell them apart. Maybe we can do additional decoration to avoid this.
But basically my worry is kind of if we trust names as indicators of permissions, we are trusting something potentially controlled by an attacker. Perhaps I am seeing phantoms!
There was a problem hiding this comment.
To me, the key is that we would deny-adapt all dependency imports by default, regardless of whether they are named or not, and require the app manifest to specify explicitly which imports they want to connect to the host, if any. A dep that named-imports super-secret-db-password would only get access to a host resource of the same name if the app explicitly says it should by saying (in TOML-ese) "connect dep $x's super-secret-db-password to the host's super-secret-db-password". Alternatively, the app manifest could say "connect dep $x's super-secret-db-password to the host's fake-db-password-ha-ha". But unless the app manifest explicitly says what to connect that import to, it just gets denied.
The metaphor I'm thinking of is mobile (e.g. Android) app permissions. An app can say "I need access to your camera to do my job, may I?", but you as the user get to decide (and are indeed required to decide) what the answer is. Similarly, a dep component can say "I need access to such-and-such resource to do my job, may I?", and the app author can say "yes", "no" (the default), or "here's something else with the right shape (but maybe fake), make do". Indeed some aftermarket mobile OSes let you do the equivalent of the last option by giving an app access to fake location data when it asks for your location.
There was a problem hiding this comment.
Okay, I see where you're coming from. We proactively deny all named imports on each dep, and then plug unnamed imports to named imports. This will work today: I guess in the future Till envisages and Lann notes, we will want/need to rethink all this stuff anyway.
So this allows us to trust names, as any attacker controlled names will be squished (even in precomposition scenarios), and we can ditch the GUIDs in favour of predictable (or even reversible) names.
alexcrichton
left a comment
There was a problem hiding this comment.
Agreed with all the conclusions here and it all seems pretty reasonable to me 👍
|
This looks viable for the current Spin interfaces, but since @tschneidereit isn't here to do it I'll point out that in the long run it would be great to use named imports for this directly, replacing the pattern of In theory we could even make this additive with existing interfaces, e.g. package spin:key-value@3.1.0;
interface opener {
open: async func() -> result<store, error>;
}
interface key-value {
resource store {
// This stays, but can't be isolated!
open: static async func(label: string) -> result<store, error>;Dependency isolation would then be pushed back to composition time rather than run time. |
| Because the CSKs are not predictable ones like database names, we will need to maintain a separate | ||
| map of CSKs to capability sets. The flow here is something like: | ||
|
|
||
| * When loading a component, generate a CSK for each combination of dep and capability, |
There was a problem hiding this comment.
IIUC random GUIDs will make composition non-deterministic. This means the composed Wasm differs on every spin build. Given that, I'd suggest a deterministic structured key derived from (component id, dependency name, capability, interface) -- e.g. spin-cap:<component>/<dep>/<capabilitiy>/<interface>.
There was a problem hiding this comment.
If we're confident that the forgery concern is a non-issue then I'm happy to go back to this (this was what the prototype did until I got in a funk over predictable names).
|
@lann Yes, I thought about that, but that involves more rework of the SDKs than I suspect we want to take on as part of this. The flow there is certainly simpler: we validate each dependency's set of named imports agains |
5c40b2c to
025a4cc
Compare
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
025a4cc to
b214452
Compare
This commit is at least an initial stab at making the `wasmtime-wasi` and `wasmtime-wasi-http` crates compatible with "named imports" or the `implements` field in the component model. This field enables importing an interface under a kebab-name while annotating that it's additionally to be considered an import of another interface's name. One example use case for this feature is [dependency isolation][diso] when composing two components together -- if they both import the filesystem the final component will import the filesystem twice under two different kebab names which means both components can have a different view of the filesystem. Wasmtime previously gained support for named imports and `implements` in `bindgen!` as part of bytecodealliance#13513 where the `named_imports` option can be specified at `bindgen!`-time which generates traits that take an extra id-style parameter. This runtime parameter indicates which kebab-name is being invoked through which the runtime can then dispatch on. The goal here is to actually wire all this up in a way that's usable for embedders. Specifically `named_imports` bindings generation is now available for all WASIp2 and WASIp3 interfaces. Additionally all implementations of these `id`-carrying traits are routed through the previous implementations after locating the correct context to operate over. All implementations of WASI functionality are already modeled more-or-less as methods on `Wasi*CtxView`-style types which internally have a borrow to the actual state and the resource table to operate on. This fits quite cleanly with named imports where conceptually what we want is the ability to configure the context-per-kebab-name. This in theory will keep the maintenance burden managable as there's still largely one source of truth for the implementation. This neatly works for all `Host`-style traits which are literally methods on `Wasi*CtxView` types, meaning the `id`-carrying versions actually do just acquire a `Wasi*CtxView` and then delegate the method. This requires more finesse for `*WithStore` traits which work with `Access` and `Accessor`, however. The `id` parameter cannot be threaded into the `fn(..)` within the `Accessor`, so refactoring is performed where appropriate to make the implementation of each interface a one-liner to reduce duplication. The end result of all of this is that this is a very large commit but it's written in such a way that the Rust compiler in theory should catch all mistakes. In other words we're heavily relying on the type system and type checking here and don't ever rely on duplication of methods that hopefully-won't-change. There's a lot of traits and a lot of interfaces, hence the size of the commit, but conceptually everything is intended to be pretty simple. Some design decisions as part of this commit, in no particular order: * IDs are represented by `wasmtime_wasi::NamedId` which is a newtype-wrapper around `usize`. The goal here is to enable an efficient implementation of dealing with ids. This notably forces the embedder to derive some sort of string-to-id (and perhaps back) map when adding items to a linker. * All of this is opt-in and nothing is changed by default. For example the `wasmtime` CLI does not support any of this yet -- in theory that would require the ability to configure `-S` flags per-named-import as opposed to all-at-once. * Mapping a `NamedId` to a context is abstracted behind a trait rather than dictating that a `Vec` or `HashMap` or similar is required. This increases the cognitive load when reading code (more generics), but avoids making this design decision within these crates and leaves exact representations up to embedders. * The `HasData` implementation can't reuse the preexisting `WasiCli`, and this uses a new `WasiCliNamed<T>` instead. This enables threading this trait-to-find-a-context to the right location for `*WithStore` trait impls. * Some miscellaneous `bindgen!` issues have been fixed during this commit to ensure that this compiles and works correctly. * An attempt has been made at documenting all the new primitives/structs/etc here. These are sort of difficult to align correctly unless you know what you're doing, so the documentation and examples are intended to serve as a way of spreading this knowledge. * One possible alternative I ended up deciding not to do was to put some sort of map-to-context storage within each preexisting context type. For example commit would be simpler for the `*WithStore` and infrastructure if it reused the exact same `Self` type as all other impls do. My thinking though is that this requires dictating the use of a `HashMap` or something else which I was hoping to avoid. Additionally the preexisting context structures are already minimal enough that they're basically what you already want as the source for each implementation, so I wanted to lean on them as much as possible. * The main wrinkle in the new implementation is that `Accessor` carries `fn(..)` to project out it's `D::Data<'_>` which means that it can't close over any information. This feature needs to in theory close over an `id: NamedId`, however, and there's no easy way to put this square peg into a round hole. To work around this internal implementations within `wasmtime-wasi{,-http}` now have a generic `F` parameter which is a closure which projects data, but this closure is typically only ever on the stack and doesn't make its way to the heap. This was one of the more awkward things to work around in this commit. * The design here is intentionally done to help ensure that this commit is correct with minimal testing. It's not really feasible to duplicate the entire test suite just for named imports but these are duplicate trait impls which otherwise shouldn't be wrong. By ensuring that there's either strict delegation or each-function-is-at-least-one-line that the light amount of testing here is sufficient for keeping this working over time. [diso]: spinframework/spin#3708
This commit is at least an initial stab at making the `wasmtime-wasi` and `wasmtime-wasi-http` crates compatible with "named imports" or the `implements` field in the component model. This field enables importing an interface under a kebab-name while annotating that it's additionally to be considered an import of another interface's name. One example use case for this feature is [dependency isolation][diso] when composing two components together -- if they both import the filesystem the final component will import the filesystem twice under two different kebab names which means both components can have a different view of the filesystem. Wasmtime previously gained support for named imports and `implements` in `bindgen!` as part of bytecodealliance#13513 where the `named_imports` option can be specified at `bindgen!`-time which generates traits that take an extra id-style parameter. This runtime parameter indicates which kebab-name is being invoked through which the runtime can then dispatch on. The goal here is to actually wire all this up in a way that's usable for embedders. Specifically `named_imports` bindings generation is now available for all WASIp2 and WASIp3 interfaces. Additionally all implementations of these `id`-carrying traits are routed through the previous implementations after locating the correct context to operate over. All implementations of WASI functionality are already modeled more-or-less as methods on `Wasi*CtxView`-style types which internally have a borrow to the actual state and the resource table to operate on. This fits quite cleanly with named imports where conceptually what we want is the ability to configure the context-per-kebab-name. This in theory will keep the maintenance burden managable as there's still largely one source of truth for the implementation. This neatly works for all `Host`-style traits which are literally methods on `Wasi*CtxView` types, meaning the `id`-carrying versions actually do just acquire a `Wasi*CtxView` and then delegate the method. This requires more finesse for `*WithStore` traits which work with `Access` and `Accessor`, however. The `id` parameter cannot be threaded into the `fn(..)` within the `Accessor`, so refactoring is performed where appropriate to make the implementation of each interface a one-liner to reduce duplication. The end result of all of this is that this is a very large commit but it's written in such a way that the Rust compiler in theory should catch all mistakes. In other words we're heavily relying on the type system and type checking here and don't ever rely on duplication of methods that hopefully-won't-change. There's a lot of traits and a lot of interfaces, hence the size of the commit, but conceptually everything is intended to be pretty simple. Some design decisions as part of this commit, in no particular order: * IDs are represented by `wasmtime_wasi::NamedId` which is a newtype-wrapper around `usize`. The goal here is to enable an efficient implementation of dealing with ids. This notably forces the embedder to derive some sort of string-to-id (and perhaps back) map when adding items to a linker. * All of this is opt-in and nothing is changed by default. For example the `wasmtime` CLI does not support any of this yet -- in theory that would require the ability to configure `-S` flags per-named-import as opposed to all-at-once. * Mapping a `NamedId` to a context is abstracted behind a trait rather than dictating that a `Vec` or `HashMap` or similar is required. This increases the cognitive load when reading code (more generics), but avoids making this design decision within these crates and leaves exact representations up to embedders. * The `HasData` implementation can't reuse the preexisting `WasiCli`, and this uses a new `WasiCliNamed<T>` instead. This enables threading this trait-to-find-a-context to the right location for `*WithStore` trait impls. * Some miscellaneous `bindgen!` issues have been fixed during this commit to ensure that this compiles and works correctly. * An attempt has been made at documenting all the new primitives/structs/etc here. These are sort of difficult to align correctly unless you know what you're doing, so the documentation and examples are intended to serve as a way of spreading this knowledge. * One possible alternative I ended up deciding not to do was to put some sort of map-to-context storage within each preexisting context type. For example commit would be simpler for the `*WithStore` and infrastructure if it reused the exact same `Self` type as all other impls do. My thinking though is that this requires dictating the use of a `HashMap` or something else which I was hoping to avoid. Additionally the preexisting context structures are already minimal enough that they're basically what you already want as the source for each implementation, so I wanted to lean on them as much as possible. * The main wrinkle in the new implementation is that `Accessor` carries `fn(..)` to project out it's `D::Data<'_>` which means that it can't close over any information. This feature needs to in theory close over an `id: NamedId`, however, and there's no easy way to put this square peg into a round hole. To work around this internal implementations within `wasmtime-wasi{,-http}` now have a generic `F` parameter which is a closure which projects data, but this closure is typically only ever on the stack and doesn't make its way to the heap. This was one of the more awkward things to work around in this commit. * The design here is intentionally done to help ensure that this commit is correct with minimal testing. It's not really feasible to duplicate the entire test suite just for named imports but these are duplicate trait impls which otherwise shouldn't be wrong. By ensuring that there's either strict delegation or each-function-is-at-least-one-line that the light amount of testing here is sufficient for keeping this working over time. [diso]: spinframework/spin#3708
Technology POC with terrible names, poor formatting, and questionable consistency at https://github.com/itowlson/spin/tree/dep-isolation with a partial test case at https://github.com/itowlson/spin-dep-isolation-test.
(The POC doesn't yet have KV because Wasmtime 47 had a bug with named-importing interfaces with hyphens in them: Wasmtime 48 fixed that and I'll get the POC KVed up shortly.)