Merge pull request #727 from itowlson/allowed-http-hosts-fixes

Rework allowed HTTP host validation and checking

Author: itowlson

Cargo.lock

@@ -15,7 +15,7 @@ use crate::{
         config::{RawAppManifest, RawComponentManifest},
         utils::{find_manifest, parcels_in_group},
     },
-    validation::validate_allowed_http_hosts,
+    validation::{parse_allowed_http_hosts, validate_allowed_http_hosts},
 };
 use anyhow::{anyhow, Context, Result};
 use bindle::Invoice;
@@ -68,9 +68,10 @@ async fn prepare(
         toml::from_slice(&reader.get_parcel(&find_manifest(&invoice)?).await?)?;
     log::trace!("Recreated manifest from bindle: {:?}", raw);
 
+    validate_raw_app_manifest(&raw)?;
+
     let mut config_root = raw.config.take().unwrap_or_default();
     for component in &mut raw.components {
-        validate_allowed_http_hosts(&component.wasm.allowed_http_hosts)?;
         if let Some(config) = component.config.take() {
             let path = component.id.clone().try_into().with_context(|| {
                 format!("component ID {:?} not a valid config path", component.id)
@@ -106,6 +107,12 @@ async fn prepare(
     })
 }
 
+fn validate_raw_app_manifest(raw: &RawAppManifest) -> Result<()> {
+    raw.components
+        .iter()
+        .try_for_each(|c| validate_allowed_http_hosts(&c.wasm.allowed_http_hosts))
+}
+
 /// Given a raw component manifest, prepare its assets and return a fully formed core component.
 async fn core(
     raw: RawComponentManifest,
@@ -140,7 +147,7 @@ async fn core(
         None => vec![],
     };
     let environment = raw.wasm.environment.unwrap_or_default();
-    let allowed_http_hosts = raw.wasm.allowed_http_hosts.unwrap_or_default();
+    let allowed_http_hosts = parse_allowed_http_hosts(&raw.wasm.allowed_http_hosts)?;
     let wasm = WasmConfig {
         environment,
         mounts,

crates/loader/src/bindle/mod.rs

@@ -21,7 +21,10 @@ use spin_manifest::{
 use std::{path::Path, str::FromStr, sync::Arc};
 use tokio::{fs::File, io::AsyncReadExt};
 
-use crate::{bindle::BindleConnectionInfo, validation::validate_allowed_http_hosts};
+use crate::{
+    bindle::BindleConnectionInfo,
+    validation::{parse_allowed_http_hosts, validate_allowed_http_hosts},
+};
 
 /// Given the path to a spin.toml manifest file, prepare its assets locally and
 /// get a prepared application configuration consumable by a Spin execution context.
@@ -225,7 +228,7 @@ async fn core(
         None => vec![],
     };
     let environment = raw.wasm.environment.unwrap_or_default();
-    let allowed_http_hosts = raw.wasm.allowed_http_hosts.unwrap_or_default();
+    let allowed_http_hosts = parse_allowed_http_hosts(&raw.wasm.allowed_http_hosts)?;
     let wasm = WasmConfig {
         environment,
         mounts,

crates/loader/src/local/mod.rs

@@ -185,15 +185,16 @@ async fn test_invalid_url_in_allowed_http_hosts_is_rejected() -> Result<()> {
     let dir = temp_dir.path();
     let app = from_file(MANIFEST, dir, &None, false).await;
 
-    assert!(
-        app.is_err(),
-        "Expected url can be parsed by reqwest Url, but there was invalid url"
-    );
+    assert!(app.is_err(), "Expected allowed_http_hosts parsing error");
 
     let e = app.unwrap_err().to_string();
     assert!(
-        e.contains("some-random-api.ml"),
-        "Expected error to contain invalid url `some-random-api.ml`"
+        e.contains("ftp://some-random-api.ml"),
+        "Expected allowed_http_hosts parse error to contain `ftp://some-random-api.ml`"
+    );
+    assert!(
+        e.contains("example.com/wib/wob"),
+        "Expected allowed_http_hosts parse error to contain `example.com/wib/wob`"
     );
 
     Ok(())

crates/loader/src/local/tests.rs

@@ -1,23 +1,246 @@
 #![deny(missing_docs)]
 
-use anyhow::{Context, Result};
+use anyhow::{anyhow, Result};
 use reqwest::Url;
+use spin_manifest::{AllowedHttpHost, AllowedHttpHosts};
 
-// Check whether http host can be parsed by Url
+// Checks a list of allowed HTTP hosts is valid
 pub fn validate_allowed_http_hosts(http_hosts: &Option<Vec<String>>) -> Result<()> {
-    if let Some(domains) = http_hosts.as_deref() {
-        if domains
-            .iter()
-            .any(|domain| domain == wasi_outbound_http::ALLOW_ALL_HOSTS)
-        {
-            return Ok(());
+    parse_allowed_http_hosts(http_hosts).map(|_| ())
+}
+
+// Parses a list of allowed HTTP hosts
+pub fn parse_allowed_http_hosts(raw: &Option<Vec<String>>) -> Result<AllowedHttpHosts> {
+    match raw {
+        None => Ok(AllowedHttpHosts::AllowSpecific(vec![])),
+        Some(list) => {
+            if list
+                .iter()
+                .any(|domain| domain == wasi_outbound_http::ALLOW_ALL_HOSTS)
+            {
+                Ok(AllowedHttpHosts::AllowAll)
+            } else {
+                let parse_results = list
+                    .iter()
+                    .map(|h| parse_allowed_http_host(h))
+                    .collect::<Vec<_>>();
+                let (hosts, errors) = partition_results(parse_results);
+
+                if errors.is_empty() {
+                    Ok(AllowedHttpHosts::AllowSpecific(hosts))
+                } else {
+                    Err(anyhow!(
+                        "One or more allowed_http_hosts entries was invalid:\n{}",
+                        errors.join("\n")
+                    ))
+                }
+            }
         }
-        let _ = domains
-            .iter()
-            .map(|d| {
-                Url::parse(d).with_context(|| format!("Can't parse {} in allowed_http_hosts", d))
-            })
-            .collect::<Result<Vec<_>, _>>()?;
-    }
-    Ok(())
+    }
+}
+
+fn parse_allowed_http_host(text: &str) -> Result<AllowedHttpHost, String> {
+    // If you call Url::parse, it accepts things like `localhost:3001`, inferring
+    // `localhost` as a scheme. That's unhelpful for us, so we do a crude check
+    // before trying to treat the string as a URL.
+    if text.contains("//") {
+        parse_allowed_http_host_from_schemed(text)
+    } else {
+        parse_allowed_http_host_from_unschemed(text)
+    }
+}
+
+fn parse_allowed_http_host_from_unschemed(text: &str) -> Result<AllowedHttpHost, String> {
+    // Host name parsing is quite hairy (thanks, IPv6), so punt it off to the
+    // Url type which gets paid big bucks to do it properly. (But preserve the
+    // original un-URL-ified string for use in error messages.)
+    let urlised = format!("http://{}", text);
+    let fake_url = Url::parse(&urlised)
+        .map_err(|_| format!("{} isn't a valid host or host:port string", text))?;
+    parse_allowed_http_host_from_http_url(&fake_url, text)
+}
+
+fn parse_allowed_http_host_from_schemed(text: &str) -> Result<AllowedHttpHost, String> {
+    let url =
+        Url::parse(text).map_err(|e| format!("{} isn't a valid HTTP host URL: {}", text, e))?;
+
+    if !matches!(url.scheme(), "http" | "https") {
+        return Err(format!("{} isn't a valid host or host:port string", text));
+    }
+
+    parse_allowed_http_host_from_http_url(&url, text)
+}
+
+fn parse_allowed_http_host_from_http_url(url: &Url, text: &str) -> Result<AllowedHttpHost, String> {
+    let host = url
+        .host_str()
+        .ok_or_else(|| format!("{} doesn't contain a host name", text))?;
+
+    let has_path = url.path().len() > 1; // allow "/"
+    if has_path {
+        return Err(format!(
+            "{} contains a path, should be host and optional port only",
+            text
+        ));
+    }
+
+    Ok(AllowedHttpHost::new(host, url.port()))
+}
+
+fn partition_results<T, E>(results: Vec<Result<T, E>>) -> (Vec<T>, Vec<E>) {
+    // We are going to to be OPTIMISTIC do you hear me
+    let mut oks = Vec::with_capacity(results.len());
+    let mut errs = vec![];
+
+    for result in results {
+        match result {
+            Ok(t) => oks.push(t),
+            Err(e) => errs.push(e),
+        }
+    }
+
+    (oks, errs)
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_allowed_hosts_accepts_http_url() {
+        assert_eq!(
+            AllowedHttpHost::host("spin.fermyon.dev"),
+            parse_allowed_http_host("http://spin.fermyon.dev").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host("spin.fermyon.dev"),
+            parse_allowed_http_host("http://spin.fermyon.dev/").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host("spin.fermyon.dev"),
+            parse_allowed_http_host("https://spin.fermyon.dev").unwrap()
+        );
+    }
+
+    #[test]
+    fn test_allowed_hosts_accepts_http_url_with_port() {
+        assert_eq!(
+            AllowedHttpHost::host_and_port("spin.fermyon.dev", 4444),
+            parse_allowed_http_host("http://spin.fermyon.dev:4444").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host_and_port("spin.fermyon.dev", 4444),
+            parse_allowed_http_host("http://spin.fermyon.dev:4444/").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host_and_port("spin.fermyon.dev", 5555),
+            parse_allowed_http_host("https://spin.fermyon.dev:5555").unwrap()
+        );
+    }
+
+    #[test]
+    fn test_allowed_hosts_accepts_plain_host() {
+        assert_eq!(
+            AllowedHttpHost::host("spin.fermyon.dev"),
+            parse_allowed_http_host("spin.fermyon.dev").unwrap()
+        );
+    }
+
+    #[test]
+    fn test_allowed_hosts_accepts_plain_host_with_port() {
+        assert_eq!(
+            AllowedHttpHost::host_and_port("spin.fermyon.dev", 7777),
+            parse_allowed_http_host("spin.fermyon.dev:7777").unwrap()
+        );
+    }
+
+    #[test]
+    fn test_allowed_hosts_accepts_localhost_addresses() {
+        assert_eq!(
+            AllowedHttpHost::host("localhost"),
+            parse_allowed_http_host("localhost").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host("localhost"),
+            parse_allowed_http_host("http://localhost").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host_and_port("localhost", 3001),
+            parse_allowed_http_host("localhost:3001").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host_and_port("localhost", 3001),
+            parse_allowed_http_host("http://localhost:3001").unwrap()
+        );
+    }
+
+    #[test]
+    fn test_allowed_hosts_accepts_ip_addresses() {
+        assert_eq!(
+            AllowedHttpHost::host("192.168.1.1"),
+            parse_allowed_http_host("192.168.1.1").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host("192.168.1.1"),
+            parse_allowed_http_host("http://192.168.1.1").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host_and_port("192.168.1.1", 3002),
+            parse_allowed_http_host("192.168.1.1:3002").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host_and_port("192.168.1.1", 3002),
+            parse_allowed_http_host("http://192.168.1.1:3002").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host("[::1]"),
+            parse_allowed_http_host("[::1]").unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHost::host_and_port("[::1]", 8001),
+            parse_allowed_http_host("http://[::1]:8001").unwrap()
+        );
+    }
+
+    #[test]
+    fn test_allowed_hosts_rejects_path() {
+        assert!(parse_allowed_http_host("http://spin.fermyon.dev/a").is_err());
+        assert!(parse_allowed_http_host("http://spin.fermyon.dev:6666/a/b").is_err());
+    }
+
+    #[test]
+    fn test_allowed_hosts_rejects_ftp_url() {
+        assert!(parse_allowed_http_host("ftp://spin.fermyon.dev").is_err());
+        assert!(parse_allowed_http_host("ftp://spin.fermyon.dev:6666").is_err());
+    }
+
+    fn to_vec_owned(source: &[&str]) -> Option<Vec<String>> {
+        Some(source.iter().map(|s| s.to_owned().to_owned()).collect())
+    }
+
+    #[test]
+    fn test_allowed_hosts_respects_allow_all() {
+        assert_eq!(
+            AllowedHttpHosts::AllowAll,
+            parse_allowed_http_hosts(&to_vec_owned(&["insecure:allow-all"])).unwrap()
+        );
+        assert_eq!(
+            AllowedHttpHosts::AllowAll,
+            parse_allowed_http_hosts(&to_vec_owned(&["spin.fermyon.dev", "insecure:allow-all"]))
+                .unwrap()
+        );
+    }
+
+    #[test]
+    fn test_allowed_hosts_can_be_specific() {
+        let allowed = parse_allowed_http_hosts(&to_vec_owned(&[
+            "spin.fermyon.dev",
+            "http://example.com:8383",
+        ]))
+        .unwrap();
+        assert!(allowed.allow(&Url::parse("http://example.com:8383/foo/bar").unwrap()));
+        assert!(allowed.allow(&Url::parse("https://spin.fermyon.dev/").unwrap()));
+        assert!(!allowed.allow(&Url::parse("http://example.com/").unwrap()));
+        assert!(!allowed.allow(&Url::parse("http://google.com/").unwrap()));
+    }
 }

crates/loader/src/validation.rs

@@ -6,6 +6,6 @@ trigger = { type = "http", base = "/" }
 [[component]]
 id = "hello"
 source = "path/to/wasm/file.wasm"
-allowed_http_hosts = [ "some-random-api.ml" ]
+allowed_http_hosts = [ "ftp://some-random-api.ml", "example.com/wib/wob" ]
 [component.trigger]
 route = "/hello"

crates/loader/tests/invalid-url-in-allowed-http-hosts.toml

@@ -10,3 +10,4 @@ indexmap = "1"
 serde = { version = "1.0", features = [ "derive" ] }
 spin-config = { path = "../config" }
 thiserror = "1"
+url = "2.2"