feat(loader): validate wasm config #659

FrankYang0529 · 2022-07-25T14:35:05Z

resolves #658

itowlson · 2022-07-25T20:01:05Z

crates/loader/src/local/mod.rs

@@ -93,6 +98,37 @@ fn error_on_duplicate_ids(components: Vec<RawComponentManifest>) -> Result<()> {
    Ok(())
 }

+fn validate_raw_app_manifest(raw: &RawAppManifestAnyVersion) -> Result<()> {
+    match raw {
+        RawAppManifestAnyVersion::V1(raw) => {


Rather than validating each raw storage format, would it make sense to validate the Application object? This is the common object which the rest of Spin works with and to which all raw formats get rendered.

I'm also wondering if we should set up more general validation infrastructure..

I agree that this would not be the best place to put validator, as each new version would require some copy and paste of previous version's validations. Building a general validation infras would be hard, but I like the idea. It needs to be versioned too.

Hi @itowlson and @Mossaka, thanks for the suggestion. Currently, if we want to set validation on the Application object, only up and trigger commands will be validated. Other commands like build or deploy use RawAppManifest directly. For validating spin.toml before runtime, I think adding validation in the Application object is enough. If you all agree with this, I can update this PR later.

build shouldn't care about configuration validity, unless used with --up, and I'd expect it that part to go through Application. Good point about deploy (and publish) though, we'd want to catch it there and they don't build Application abstractions.

Given that, the best course is, I think, to go ahead with the validation where you have it, and raise an issue to revisit the consolidation issue as part of a general validation infrastructure, which might require more invasive changes. @Mossaka does that make sense to you?

Mossaka · 2022-07-26T01:07:56Z

crates/loader/src/local/mod.rs

+        let _ = domains
+            .iter()
+            .map(|d| {
+                Url::parse(d).with_context(|| format!("can't parse {} in allowed_http_hosts", d))


Suggested change

Url::parse(d).with_context(|| format!("can't parse {} in allowed_http_hosts", d))

Url::parse(d).with_context(|| format!("Can't parse {} in allowed_http_hosts", d))

Updated it. Thank you.

Mossaka · 2022-07-26T01:12:57Z

crates/loader/src/local/mod.rs

@@ -93,6 +98,37 @@ fn error_on_duplicate_ids(components: Vec<RawComponentManifest>) -> Result<()> {
    Ok(())
 }

+fn validate_raw_app_manifest(raw: &RawAppManifestAnyVersion) -> Result<()> {
+    match raw {
+        RawAppManifestAnyVersion::V1(raw) => {


I agree that this would not be the best place to put validator, as each new version would require some copy and paste of previous version's validations. Building a general validation infras would be hard, but I like the idea. It needs to be versioned too.

itowlson

This looks good apart from a couple of minor changes, but it does hit a snag in our local loader flow. If you put the validation in the raw manifest deserialiser, it gets run during build, which feels iffy. But if you don't put it in the deserialiser, it doesn't get run during deploy or bindle prepare/publish, which is even worse.

We can bodge around this by manually calling it from expand_manifest, and I think that would work. But then we have multiple places where validation happens, and as the number of entry points grows it gets hard to make sure we're catching it everywhere. So we're probably going to need to do some rework on the loading pipeline.

My suggestion for now would be to remove the validation call from raw_manifest_from_file (which really is meant to do nothing more than deserialise the TOML) and call it instead from from_file and expand_manifest. I think that would pick up all the cases.

As always I'm very open to discussion - I am really not entirely sure what the right thing to do is!

itowlson · 2022-07-28T21:25:17Z

crates/loader/src/bindle/mod.rs

+        let _ = domains
+            .iter()
+            .map(|d| {
+                Url::parse(d).with_context(|| format!("can't parse {} in allowed_http_hosts", d))


Suggested change

Url::parse(d).with_context(|| format!("can't parse {} in allowed_http_hosts", d))

Url::parse(d).with_context(|| format!("Can't parse {} in allowed_http_hosts", d))

Updated it. Thank you.

itowlson · 2022-07-28T21:28:14Z

crates/loader/src/local/mod.rs

+    Ok(())
+}
+
+fn validate_raw_wasm_config(raw: &RawWasmConfig) -> Result<()> {


This is the same function as in the bindle loader. I know the RawWasmConfig types are different between the two, but the allowed_http_hosts types are the same. Could we have a helper function that takes &Option<Vec<String>> (or similar) and has all the logic in it, and call that function from both places?

Thanks for your suggestion. I add a validate_allowed_http_hosts function and remove validate_raw_wasm_config in both local and bindle loader.

itowlson · 2022-07-28T21:36:52Z

crates/loader/src/local/mod.rs

@@ -60,6 +64,7 @@ pub async fn raw_manifest_from_file(app: &impl AsRef<Path>) -> Result<RawAppMani
        .with_context(|| anyhow!("Cannot read manifest file from {:?}", app.as_ref()))?;

    let manifest: RawAppManifestAnyVersion = toml::from_slice(&buf)?;
+    validate_raw_app_manifest(&manifest)?;


Placing this here will cause it to run on build, which normally only cares about compilation... I'm not sure of a good solution to this. We could place this call in the from_file and from_bindle wrappers used by up, and call it manually from deploy. But then it starts to feel a bit like whack-a-mole...

Yeah, I agree with it. We should have a place to transfer RawAppManifest to Application. Also, other functions/commands should rely on the Application object, so we don't need to put the validating function everywhere.

itowlson

One final request but it's an easy one I hope - conceptually HTTP host validation is not part of the assets subsystem - I can't see an existing home so maybe create a new file/module for it? Thanks! Otherwise looks good to go.

itowlson · 2022-08-04T20:44:12Z

crates/loader/src/assets.rs

@@ -103,6 +104,24 @@ pub(crate) async fn change_file_permission(
    Ok(())
 }

+pub fn validate_allowed_http_hosts(http_hosts: &Option<Vec<String>>) -> Result<()> {


nit: I don't think this belongs in the assets module; if there's no existing home for it then maybe create a new validation module?

Yeah, add a validation module for it. Thank you.

Signed-off-by: Frank Yang <yangpoan@gmail.com>

itowlson

Awesome, thanks! LGTM

itowlson mentioned this pull request Jul 26, 2022

Missing protocol in allowed_http_hosts gives misleading error at runtime #658

Closed

itowlson reviewed Jul 26, 2022

View reviewed changes

Mossaka reviewed Jul 26, 2022

View reviewed changes

itowlson requested changes Jul 28, 2022

View reviewed changes

FrankYang0529 force-pushed the add-validation-for-wasm-config branch from 93d9b8c to 272973d Compare August 2, 2022 15:02

FrankYang0529 requested a review from itowlson August 4, 2022 15:21

itowlson requested changes Aug 4, 2022

View reviewed changes

FrankYang0529 force-pushed the add-validation-for-wasm-config branch 2 times, most recently from 9644738 to cf202dc Compare August 5, 2022 13:53

feat(loader): validate wasm config

5931531

Signed-off-by: Frank Yang <yangpoan@gmail.com>

FrankYang0529 force-pushed the add-validation-for-wasm-config branch from cf202dc to 5931531 Compare August 5, 2022 13:54

itowlson approved these changes Aug 8, 2022

View reviewed changes

itowlson merged commit 4023d83 into fermyon:main Aug 8, 2022

FrankYang0529 deleted the add-validation-for-wasm-config branch August 8, 2022 01:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(loader): validate wasm config #659

feat(loader): validate wasm config #659

FrankYang0529 commented Jul 25, 2022

itowlson Jul 25, 2022

itowlson Jul 26, 2022

Mossaka Jul 26, 2022

FrankYang0529 Jul 27, 2022

itowlson Jul 28, 2022

Mossaka Jul 26, 2022

FrankYang0529 Aug 2, 2022

Mossaka Jul 26, 2022

itowlson left a comment

itowlson Jul 28, 2022

FrankYang0529 Aug 2, 2022

itowlson Jul 28, 2022

FrankYang0529 Aug 2, 2022

itowlson Jul 28, 2022

FrankYang0529 Aug 2, 2022

itowlson left a comment

itowlson Aug 4, 2022

FrankYang0529 Aug 5, 2022

itowlson left a comment

	Url::parse(d).with_context(\|\| format!("can't parse {} in allowed_http_hosts", d))
	Url::parse(d).with_context(\|\| format!("Can't parse {} in allowed_http_hosts", d))

feat(loader): validate wasm config #659

feat(loader): validate wasm config #659

Conversation

FrankYang0529 commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson left a comment

Choose a reason for hiding this comment