Skip to content

Commit

Permalink
Validate retained components in smaller focused functions
Browse files Browse the repository at this point in the history 
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
  • Loading branch information
@kate-goldenring
kate-goldenring committed Sep 16, 2024
1 parent 67af2d3 commit 67d9b77
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 106 deletions.
13 changes: 7 additions & 6 deletions crates/factor-outbound-networking/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,6 @@ impl OutboundNetworkingFactor {
}
}

pub fn allowed_hosts(component: &AppComponent<'_>) -> anyhow::Result<Vec<String>> {
Ok(component
.get_metadata(ALLOWED_HOSTS_KEY)?
.unwrap_or_default())
}

impl Factor for OutboundNetworkingFactor {
type RuntimeConfig = RuntimeConfig;
type AppState = AppState;
Expand Down Expand Up @@ -147,6 +141,13 @@ impl Factor for OutboundNetworkingFactor {
}
}

/// Returns the allowed hosts for a component.
pub fn allowed_hosts(component: &AppComponent<'_>) -> anyhow::Result<Vec<String>> {
Ok(component
.get_metadata(ALLOWED_HOSTS_KEY)?
.unwrap_or_default())
}

pub struct AppState {
component_allowed_hosts: HashMap<String, Arc<[String]>>,
runtime_config: RuntimeConfig,
Expand Down
237 changes: 137 additions & 100 deletions src/commands/up.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use std::{
use anyhow::{anyhow, bail, ensure, Context, Result};
use clap::{CommandFactory, Parser};
use reqwest::Url;
use spin_app::{locked::LockedApp, AppComponent};
use spin_app::locked::LockedApp;
use spin_common::ui::quoted_path;
use spin_factor_outbound_networking::{allowed_hosts, parse_service_chaining_target};
use spin_loader::FilesMountStrategy;
Expand All @@ -36,9 +36,6 @@ const MULTI_TRIGGER_START_OFFSET: tokio::time::Duration = tokio::time::Duration:
// any exited" check.
const MULTI_TRIGGER_LET_ALL_START: tokio::time::Duration = tokio::time::Duration::from_millis(500);

// Map of component ID to trigger ID
type ComponentTriggerMap = HashMap<String, HashSet<String>>;

/// Start the Fermyon runtime.
#[derive(Parser, Debug, Default)]
#[clap(
Expand Down Expand Up @@ -175,7 +172,7 @@ impl UpCommand {
let resolved_app_source = self.resolve_app_source(&app_source, &working_dir).await?;
if self.help {
let trigger_cmds =
trigger_command_for_resolved_app_source(resolved_app_source.trigger_types())
trigger_commands_for_trigger_types(resolved_app_source.trigger_types())
.with_context(|| format!("Couldn't find trigger executor for {app_source}"))?;

let is_multi = trigger_cmds.len() > 1;
Expand Down Expand Up @@ -205,7 +202,6 @@ impl UpCommand {
retain_components(&mut locked_app, components)?;
}

// Remove duplicate triggers
let trigger_types: HashSet<&str> = locked_app
.triggers
.iter()
Expand All @@ -214,9 +210,8 @@ impl UpCommand {

ensure!(!trigger_types.is_empty(), "No triggers in app");

let trigger_cmds =
trigger_command_for_resolved_app_source(trigger_types.into_iter().collect())
.with_context(|| format!("Couldn't find trigger executor for {app_source}"))?;
let trigger_cmds = trigger_commands_for_trigger_types(trigger_types.into_iter().collect())
.with_context(|| format!("Couldn't find trigger executor for {app_source}"))?;
let is_multi = trigger_cmds.len() > 1;

self.update_locked_app(&mut locked_app);
Expand All @@ -229,6 +224,7 @@ impl UpCommand {
working_dir,
local_app_dir,
};

let trigger_processes = self.start_trigger_processes(trigger_cmds, run_opts).await?;
let pids = get_pids(&trigger_processes);

Expand Down Expand Up @@ -654,7 +650,7 @@ fn trigger_command(trigger_type: &str) -> Vec<String> {
vec!["trigger".to_owned(), trigger_type.to_owned()]
}

fn trigger_command_for_resolved_app_source(trigger_types: Vec<&str>) -> Result<Vec<Vec<String>>> {
fn trigger_commands_for_trigger_types(trigger_types: Vec<&str>) -> Result<Vec<Vec<String>>> {
trigger_types
.iter()
.map(|&t| match t {
Expand All @@ -667,85 +663,79 @@ fn trigger_command_for_resolved_app_source(trigger_types: Vec<&str>) -> Result<V
.collect()
}

// Scrubs the locked app to only contain the given list of components
// Introspects the LockedApp to find and selectively retain the triggers that correspond to those components
fn retain_components(locked_app: &mut LockedApp, components: &[String]) -> Result<()> {
/// Scrubs the locked app to only contain the given list of components
/// Introspects the LockedApp to find and selectively retain the triggers that correspond to those components
fn retain_components(locked_app: &mut LockedApp, retained_components: &[String]) -> Result<()> {
// Create a temporary app to access parsed component and trigger information
let tmp_app = spin_app::App::new("tmp", locked_app.clone());
let component_trigger_id_map = component_trigger_map(&tmp_app, components)?;
let components = component_trigger_id_map.keys().collect::<HashSet<_>>();
let triggers = component_trigger_id_map
.values()
.flatten()
.collect::<HashSet<_>>();
locked_app.components.retain(|c| components.contains(&c.id));
locked_app.triggers.retain(|t| triggers.contains(&t.id));
Ok(())
}

// Given a list of components and App, returns a map of component ID to trigger ID.
// Also validates that the components exist in the app and have valid service chaining.
fn component_trigger_map(
app: &spin_app::App,
components: &[String],
) -> Result<ComponentTriggerMap> {
let component_triggers: Vec<(AppComponent<'_>, String)> = app
validate_retained_components_exist(&tmp_app, retained_components)?;
validate_retained_components_service_chaining(&tmp_app, retained_components)?;
let (component_ids, trigger_ids): (HashSet<String>, HashSet<String>) = tmp_app
.triggers()
.filter_map(|t| match t.component() {
Ok(comp) => {
if components.contains(&comp.id().to_string()) {
Some((comp, t.id().to_string()))
} else {
None
}
Ok(comp) if retained_components.contains(&comp.id().to_string()) => {
Some((comp.id().to_owned(), t.id().to_owned()))
}
Err(_) => None,
_ => None,
})
.collect();
locked_app
.components
.retain(|c| component_ids.contains(&c.id));
locked_app.triggers.retain(|t| trigger_ids.contains(&t.id));
Ok(())
}

// Allow only service chaining among retained components
// All wildcard service chaining is disallowed
for (c, _) in &component_triggers {
let allowed_hosts = allowed_hosts(c)?;
allowed_hosts.iter().try_for_each(|host| {
let uri = host.parse::<http::Uri>().unwrap();
if let Some(local_component) = parse_service_chaining_target(&uri) {
if !components.contains(&local_component) {
if local_component == "*" {
bail!("Component selected with '--component {}' cannot use wildcard service chaining:
allowed_outbound_hosts = [\"http://*.spin.internal\"]", c.id());
/// Validates that all service chaining of an app will be satisfied by the retained components.
/// All wildcard service chaining is disallowed
fn validate_retained_components_service_chaining(
app: &spin_app::App,
retained_components: &[String],
) -> Result<()> {
app
.triggers().try_for_each(|t| {
if let Ok(component) = t.component() {
if retained_components.contains(&component.id().to_string()) {
let allowed_hosts = allowed_hosts(&component).context("failed to get allowed hosts")?;
allowed_hosts.iter().try_for_each(|host| {
// Templated URLs are not resolved at this point, so ignore unresolvable URIs
if let Ok(uri) = host.parse::<http::Uri>() {
if let Some(local_component) = parse_service_chaining_target(&uri) {
if !retained_components.contains(&local_component) {
if local_component == "*" {
bail!("Component selected with '--component {}' cannot use wildcard service chaining: allowed_outbound_hosts = [\"http://*.spin.internal\"]", component.id());
}
bail!(
"Component selected with '--component {}' cannot use service chaining to unselected component: allowed_outbound_hosts = [\"http://{}.spin.internal\"]",
component.id(), local_component
);
}
bail!(
"Component selected with '--component {}' cannot use service chaining to unselected component:
allowed_outbound_hosts = [\"http://{}.spin.internal\"]",
c.id(), local_component
);
}
}
Ok(())
})?;
}
let mut component_trigger_map: ComponentTriggerMap = HashMap::new();
component_triggers.into_iter().for_each(|(c, t)| {
component_trigger_map
.entry(c.id().to_string())
.or_insert_with(|| HashSet::from([t]));
});

// Ensure that the requested components exist in the app
let mut not_found = Vec::new();
components.iter().for_each(|c| {
if !component_trigger_map.contains_key(c) {
not_found.push(c);
Ok(())
})?;
}}
Ok(()) as Result<(), anyhow::Error>
})?;

Ok(())
}

/// Validates that all components specified to be retained actually exist in the app
fn validate_retained_components_exist(
app: &spin_app::App,
retained_components: &[String],
) -> Result<()> {
let app_components = app
.components()
.map(|c| c.id().to_string())
.collect::<HashSet<_>>();
retained_components.iter().try_for_each(|c| {
if !app_components.contains(c) {
bail!("Specified component \"{c}\" not found in application");
}
});
ensure!(
not_found.is_empty(),
"Specified components not found in application: {:?}",
not_found
);

Ok(component_trigger_map)
Ok(())
})
}

#[cfg(test)]
Expand All @@ -761,8 +751,7 @@ mod test {
}

#[tokio::test]
async fn test_filtered_and_validated_component_trigger_map_filtering_for_only_component_works()
{
async fn test_retain_components_filtering_for_only_component_works() {
let manifest = toml::toml! {
spin_manifest_version = 2

Expand All @@ -775,16 +764,19 @@ mod test {
[component.empty]
source = "does-not-exist.wasm"
};
let locked_app = build_locked_app(&manifest).await.unwrap();
let app = spin_app::App::new("test", locked_app);
let map = component_trigger_map(&app, &["empty".to_string()]).unwrap();
assert!(map.contains_key("empty"));
assert!(map.len() == 1);
let mut locked_app = build_locked_app(&manifest).await.unwrap();
retain_components(&mut locked_app, &["empty".to_string()]).unwrap();
let components = locked_app
.components
.iter()
.map(|c| c.id.to_string())
.collect::<HashSet<_>>();
assert!(components.contains("empty"));
assert!(components.len() == 1);
}

#[tokio::test]
async fn test_filtered_and_validated_component_trigger_map_filtering_for_non_existent_component_fails(
) {
async fn test_retain_components_filtering_for_non_existent_component_fails() {
let manifest = toml::toml! {
spin_manifest_version = 2

Expand All @@ -797,13 +789,19 @@ mod test {
[component.empty]
source = "does-not-exist.wasm"
};
let locked_app = build_locked_app(&manifest).await.unwrap();
let app = spin_app::App::new("test", locked_app);
assert!(component_trigger_map(&app, &["dne".to_string()]).is_err());
let mut locked_app = build_locked_app(&manifest).await.unwrap();
let Err(e) = retain_components(&mut locked_app, &["dne".to_string()]) else {
panic!("Expected component not found error");
};
assert_eq!(
e.to_string(),
"Specified component \"dne\" not found in application"
);
assert!(retain_components(&mut locked_app, &["dne".to_string()]).is_err());
}

#[tokio::test]
async fn test_filtered_and_validated_component_trigger_map_app_with_service_chaining_fails() {
async fn test_retain_components_app_with_service_chaining_fails() {
let manifest = toml::toml! {
spin_manifest_version = 2

Expand All @@ -830,25 +828,64 @@ mod test {
source = "does-not-exist.wasm"
allowed_outbound_hosts = ["http://*.spin.internal"]
};
let locked_app = build_locked_app(&manifest).await.unwrap();
let app = spin_app::App::new("test", locked_app);
assert!(component_trigger_map(&app, &["another".to_string()]).is_ok());
let Err(e) = component_trigger_map(&app, &["empty".to_string()]) else {
let mut locked_app = build_locked_app(&manifest)
.await
.expect("could not build locked app");
let Err(e) = retain_components(&mut locked_app, &["empty".to_string()]) else {
panic!("Expected service chaining to non-retained component error");
};
assert_eq!(
e.to_string(),
"Component selected with '--component empty' cannot use service chaining to unselected component:
allowed_outbound_hosts = [\"http://another.spin.internal\"]"
"Component selected with '--component empty' cannot use service chaining to unselected component: allowed_outbound_hosts = [\"http://another.spin.internal\"]"
);
let Err(e) = component_trigger_map(&app, &["third".to_string(), "another".to_string()])
else {
let Err(e) = retain_components(
&mut locked_app,
&["third".to_string(), "another".to_string()],
) else {
panic!("Expected wildcard service chaining error");
};
assert_eq!(
e.to_string(),
"Component selected with '--component third' cannot use wildcard service chaining:
allowed_outbound_hosts = [\"http://*.spin.internal\"]"
"Component selected with '--component third' cannot use wildcard service chaining: allowed_outbound_hosts = [\"http://*.spin.internal\"]"
);
assert!(retain_components(&mut locked_app, &["another".to_string()]).is_ok());
}

#[tokio::test]
async fn test_retain_components_app_with_templated_host_passes() {
let manifest = toml::toml! {
spin_manifest_version = 2

[application]
name = "test-app"

[variables]
host = { default = "test" }

[[trigger.test-trigger]]
component = "empty"

[component.empty]
source = "does-not-exist.wasm"

[[trigger.another-trigger]]
component = "another"

[component.another]
source = "does-not-exist.wasm"

[[trigger.third-trigger]]
component = "third"

[component.third]
source = "does-not-exist.wasm"
allowed_outbound_hosts = ["http://{{ host }}.spin.internal"]
};
let mut locked_app = build_locked_app(&manifest)
.await
.expect("could not build locked app");
assert!(
retain_components(&mut locked_app, &["empty".to_string(), "third".to_string()]).is_ok()
);
}

Expand Down

0 comments on commit 67d9b77

Please sign in to comment.