feat(loader): support loading AOT compiled components

[kate-goldenring]

No description provided.

[fibonacci1729]

LGTM!

[rylev]

I think we'll want to spell out exactly what needs to happen here for this to be safe. i.e., that the LockedComponentSource passed to Loader::load_component references a precompiled component compiled using the same version of spin_core used here and the same Engine.

[lann]

The most important point to get across is that it is a complete security meltdown if used with untrusted input.

Practically it is "safe" for any wasmtime precompiled input (and almost any non-malicious input); it will just fail to deserialize unless compiled with a compatible engine.

[kate-goldenring]

I updated the comment a little, but am still only mentioning that the requirement is wasmtime compilation (nothing Spin specific). Also, now the function is marked unsafe. @rylev do these changes look appropriate?

[rylev]

I don't think marking the constructor as unsafe is the right approach. The contract with unsafe is that if the caller of the function can verify certain invariants are met at the time of the function call then the call is safe. However, it's the call to Loader::load_component that needs to verify the invariant (in this case that the component source is valid to be used with Component::deserialize).

unsafe is IMO inappropriate because the invariant that needs to be satisfied cannot be satisfied in code. It's up to whoever is running the software to ensure the invariant holds. This is why I think a feature flag that prominently displays the safety risks is the right approach.

[rylev]

I now see @lann's comment below pointing to similar usage in cranelift. However, I think this is categorically different usage.

The usage in the cranelift API can still be verified as correct by users by verifying its usage is correct in combination with the rest of the usage of cranelift - in other words, the safety properties can be verified statically albeit manually. The usage here on the other hand, cannot be verified statically as correct - it depends on how the software is run - the same code can be correct when run in one context and incorrect when run in another. That is not what unsafe is meant to signal.

[rylev]

deserialize_file is different in that the safety check happens at the call site of the function marked unsafe. You should only call deserialize_file if you're sure the input to that function meets some invariants. That's different than calling a function and promising all future input into the system as a whole meets some invariants.

Taking a step back... we both are trying to achieve the same thing here: ensuring that users of these APIs know what the right thing to do is. I personally think the feature flag is the right approach. It says "don't build and run this software unless you know what you're doing", and I think it does it in a better way than marking the function unsafe. If others disagree though that's fine, I won't try to block merging this PR.

[lann]

I would be fine with the feature flag only if "unsafe" appears in it, but I would personally keep this unsafe as well (belt and suspenders), just so unsafe appears somewhere in the calling code. 🤷

[fibonacci1729]

just so unsafe appears somewhere in the calling code.

I tend to agree with this (being the intent originally).

[kate-goldenring]

We are aiming to get this in tomorrow before we can cut a release. @rylev can we leave unsafe on the setter? @lann does this LGTY?

[rylev]

I'm not willing to die on this hill 😉 so . It's certainly better than not marking a function as unsafe when it clearly is.

[rylev]

This comment is confusing unless you've read the code. There's no indication of which input we're actually talking about. It would be better if this comment indicated that it is the content source of the LockedComponentSource input to <TriggerLoader as Loader>::load_component that we're concerned with. Something like:

This method is safe if it can be guaranteed that <TriggerLoader as Loader>::load_component will only ever be called with a trusted LockedComponentSource.

The details about Component::deserialize are fine to keep but should be secondary to the point above IMO

[lann]

I agree that this could use a little wordsmithing but most consumers (by which I just mean spin shim 😅) probably won't have context for what LockedComponentSource or even Loader::load_component are doing.

Maybe something along the lines of:

Updates the TriggerLoader to load AOT precompiled components

**Warning: This feature may bypass important security guarantees of the Wasmtime
security sandbox if used incorrectly! Read this documentation carefully.**

Usually, components are compiled just-in-time from portable Wasm sources.
This method causes components to instead be loaded ahead-of-time as
Wasmtime-precompiled native executable binaries. Precompiled binaries must be
produced with a compatible Wasmtime engine using the same Wasmtime version and
compiler target settings - typically by a host with the same processor that will
be executing them. See the Wasmtime documentation for more information:
https://docs.rs/wasmtime/latest/wasmtime/struct.Module.html#method.deserialize

# Safety

This method is marked as `unsafe` because it enables potentially unsafe behavior if
used to load malformed or malicious precompiled binaries. Loading sources from an
incompatible Wasmtime engine will fail but is otherwise safe. **Precompiled binaries
must never be loaded from untrusted sources.**

[rylev]

most consumers (by which I just mean spin shim 😅) probably won't have context for what LockedComponentSource or even Loader::load_component are doing

This is true, but most Spin users don't need to make this choice. And if you're building a different consumer that would need to make this choice, you have to know these details in order to make the right choice.

Most people should read this and interpret this as just saying "here be dragons". Only those who really need to know will take the time to inform themselves about the details so they can make a decision about whether to enable this feature. How would one even know how to precompile binaries without understanding in detail about how they are eventually loaded?

[kate-goldenring]

I combined both of your proposed texts, ensuring @rylev's constructive comment on what exact Spin structure must be trusted (LockedComponentSource) owned the line of what makes it safe.

[radu-matei]

LGTM from the perspective of a consumer (the containerd shim).

Thank you!