diff --git a/app/src-tauri/src/aws/environments.rs b/app/src-tauri/src/aws/environments.rs index c163671..20a78d9 100644 --- a/app/src-tauri/src/aws/environments.rs +++ b/app/src-tauri/src/aws/environments.rs @@ -142,6 +142,9 @@ pub fn parse_environment_record( .collect::>>() }) .ok_or_else(|| "subnet_ids is not exactly 2 subnet ids".to_string())?; + if subnet_ids[0] == subnet_ids[1] { + return Err("subnet_ids must be two different subnets".into()); + } let assign_public_ip = field(obj, "assign_public_ip")? .as_bool() .ok_or_else(|| "assign_public_ip is not true or false".to_string())?; @@ -386,11 +389,16 @@ fn detect_one( } else if lock.as_ref().is_some_and(|l| l.age_secs < LIVE_LOCK_SECS) { (EnvironmentState::Deploying { lock }, vec![]) } else if lock.is_some() { - // Stale, and not ours (a local runner was handled above). - ( - EnvironmentState::Incomplete { lock }, - vec![Repair, Destroy, Unlock], - ) + // Stale, and not ours (a local runner was handled above). A valid + // record with our state is a working environment whose last run + // (an Update, a plan) died: never Destroy it from the app (D6). + let working = info.is_some() && has_state; + let actions = if working { + vec![Repair, Unlock] + } else { + vec![Repair, Destroy, Unlock] + }; + (EnvironmentState::Incomplete { lock }, actions) } else if let Some(reason) = reason { // A re-apply rewrites the record, so Repair is offered when the // state is ours. Never Destroy: the environment may be working (D6). @@ -586,8 +594,10 @@ fn lookup_outcome(code: Option<&str>, status: Option) -> Lookup { (Some(c), _) if MISSING.contains(&c) => Lookup::Missing, (Some(c), _) if DENIED.contains(&c) => Lookup::Denied, (None, Some(404)) => Lookup::Missing, + // A 403 with no body (HeadBucket). With a code that isn't access + // denied (InvalidAccessKeyId, SignatureDoesNotMatch, ExpiredToken, + // ...) it is an error the caller must see. (None, Some(403)) => Lookup::Denied, - (_, Some(403)) => Lookup::Denied, _ => Lookup::Error, } } @@ -850,6 +860,36 @@ fn create_bucket_outcome(code: Option<&str>, bucket: &str) -> Option, bucket: &str) -> Option { + (status == Some(403)).then(|| AwsError::StateBucketTaken { + bucket: bucket.to_string(), + }) +} + +/// The bucket's tag set with ours added, keeping every other tag, or `None` +/// when `triple-c:component=state` is already there (nothing to put). A +/// different `triple-c:component` value is replaced: no resource overrides +/// the component tag. +fn tags_with_state(existing: &[(String, String)]) -> Option> { + if existing + .iter() + .any(|(k, v)| k == STATE_TAG.0 && v == STATE_TAG.1) + { + return None; + } + let mut tags: Vec<(String, String)> = existing + .iter() + .filter(|(k, _)| k != STATE_TAG.0) + .cloned() + .collect(); + tags.push((STATE_TAG.0.to_string(), STATE_TAG.1.to_string())); + Some(tags) +} + fn state_tag() -> Tag { Tag::builder() .key(STATE_TAG.0) @@ -920,25 +960,54 @@ pub async fn ensure_state_bucket( } // us-east-1 answers a create of a bucket this account owns with success, // not BucketAlreadyOwnedByYou: confirm ownership before configuring. - s3.head_bucket() + if let Err(e) = s3 + .head_bucket() .bucket(&bucket) .expected_bucket_owner(account) .send() .await - .map_err(|e| classify_sdk_error("s3:ListBucket", &e))?; + { + let (_, status) = error_parts(&e); + return Err(head_after_create_error(status, &bucket) + .unwrap_or_else(|| classify_sdk_error("s3:ListBucket", &e))); + } - s3.put_bucket_tagging() + let existing = match s3 + .get_bucket_tagging() .bucket(&bucket) .expected_bucket_owner(account) - .tagging( - Tagging::builder() - .tag_set(state_tag()) - .build() - .expect("tag set is set"), - ) .send() .await - .map_err(|e| classify_sdk_error("s3:PutBucketTagging", &e))?; + { + Ok(out) => out + .tag_set() + .iter() + .map(|t| (t.key().to_string(), t.value().to_string())) + .collect(), + Err(e) if e.code() == Some("NoSuchTagSet") => Vec::new(), + Err(e) => return Err(classify_sdk_error("s3:GetBucketTagging", &e)), + }; + if let Some(tags) = tags_with_state(&existing) { + let tag_set = tags + .into_iter() + .map(|(k, v)| Tag::builder().key(k).value(v).build()) + .collect::, _>>() + .map_err(|e| AwsError::Other { + message: format!("Could not build the state bucket's tags: {e}"), + })?; + s3.put_bucket_tagging() + .bucket(&bucket) + .expected_bucket_owner(account) + .tagging( + Tagging::builder() + .set_tag_set(Some(tag_set)) + .build() + .expect("tag set is set"), + ) + .send() + .await + .map_err(|e| classify_sdk_error("s3:PutBucketTagging", &e))?; + } s3.put_bucket_versioning() .bucket(&bucket) .expected_bucket_owner(account) diff --git a/app/src-tauri/src/aws/environments/tests.rs b/app/src-tauri/src/aws/environments/tests.rs index 6a62f06..ecf3108 100644 --- a/app/src-tauri/src/aws/environments/tests.rs +++ b/app/src-tauri/src/aws/environments/tests.rs @@ -129,6 +129,15 @@ fn untrusted_environment_records_are_rejected() { with("dev", "subnet_ids", json!(["subnet-0123abcd", secret])), "subnet_ids", ), + ( + "the same subnet twice", + with( + "dev", + "subnet_ids", + json!(["subnet-0123abcd", "subnet-0123abcd"]), + ), + "two different subnets", + ), ( "uppercase subnet hex", with( @@ -308,17 +317,58 @@ fn interrupted_deployments_get_the_right_actions() { ] ); - // A stale lock on an environment that has a record is still Incomplete. + // A stale lock on a working environment (valid record and our state): + // Incomplete, but never Destroy (D6) — an interrupted Update or plan. let envs = merge( vec![("a".into(), Ok(parsed("a", 1)))], Some(&objs), &nobody_local, 1, ); + let a = find(&envs, "a"); assert!(matches!( - find(&envs, "a").state, + a.state, EnvironmentState::Incomplete { lock: Some(_) } )); + assert_eq!( + a.actions, + vec![EnvironmentAction::Repair, EnvironmentAction::Unlock] + ); + + // A stale lock with an unreadable record keeps Destroy: there is no + // valid record, so nothing says the environment works. + let envs = merge( + vec![("a".into(), Err("tenancy is not strict".into()))], + Some(&objs), + &nobody_local, + 1, + ); + assert_eq!( + find(&envs, "a").actions, + vec![ + EnvironmentAction::Repair, + EnvironmentAction::Destroy, + EnvironmentAction::Unlock + ] + ); + + // A stale lock with a record but no state of ours (a first apply that + // died before writing state) keeps Destroy too. + let no_state = objects(&[], &[(env_key("a"), stale)]); + let envs = merge( + vec![("a".into(), Ok(parsed("a", 1)))], + Some(&no_state), + &nobody_local, + 1, + ); + assert_eq!( + find(&envs, "a").actions, + vec![ + EnvironmentAction::Repair, + EnvironmentAction::Destroy, + EnvironmentAction::Unlock + ] + ); // A fresh lock (a first apply: no state object yet): Deploying, nothing // to do but wait. @@ -620,6 +670,19 @@ fn denied_lookups_are_unknown_not_missing() { Lookup::Missing ); assert_eq!(lookup_outcome(Some("Throttling"), Some(400)), Lookup::Error); + // A 403 that is a credential failure is an error, never Unknown. + for code in [ + "InvalidClientTokenId", + "SignatureDoesNotMatch", + "InvalidAccessKeyId", + "ExpiredToken", + ] { + assert_eq!( + lookup_outcome(Some(code), Some(403)), + Lookup::Error, + "{code}" + ); + } assert_eq!(lookup_outcome(None, None), Lookup::Error); } @@ -662,6 +725,68 @@ fn only_buckets_owned_by_you_count_as_created() { assert_eq!(create_bucket_outcome(None, &bucket), None); } +#[test] +fn a_bucket_of_another_account_after_create_is_taken() { + let bucket = state_bucket_name(ACCOUNT, REGION); + assert_eq!( + head_after_create_error(Some(403), &bucket), + Some(AwsError::StateBucketTaken { + bucket: bucket.clone() + }) + ); + assert_eq!(head_after_create_error(Some(500), &bucket), None); + assert_eq!(head_after_create_error(None, &bucket), None); +} + +#[test] +fn only_a_missing_component_tag_is_put_and_other_tags_are_kept() { + let t = |k: &str, v: &str| (k.to_string(), v.to_string()); + assert_eq!( + tags_with_state(&[t("team", "infra"), t("triple-c:component", "state")]), + None + ); + assert_eq!( + tags_with_state(&[]), + Some(vec![t("triple-c:component", "state")]) + ); + assert_eq!( + tags_with_state(&[t("team", "infra"), t("cost", "42")]), + Some(vec![ + t("team", "infra"), + t("cost", "42"), + t("triple-c:component", "state") + ]) + ); + // A different component value is replaced, not duplicated. + assert_eq!( + tags_with_state(&[t("triple-c:component", "other"), t("team", "infra")]), + Some(vec![t("team", "infra"), t("triple-c:component", "state")]) + ); +} + +#[test] +fn hostile_state_keys_name_no_environment() { + for key in [ + "environments/a/b/terraform.tfstate", + "environments/Bad/terraform.tfstate", + "environments/a/terraform.tfstate.tflock.x", + "other/a/terraform.tfstate", + "environments//terraform.tfstate", + "environments/../terraform.tfstate", + "environments/a/terraform.tfstate.tflock.tflock", + ] { + assert_eq!(environment_of_key(key), None, "{key}"); + } + assert_eq!( + environment_of_key("environments/dev/terraform.tfstate"), + Some("dev") + ); + assert_eq!( + environment_of_key("environments/dev/terraform.tfstate.tflock"), + Some("dev") + ); +} + #[test] fn the_state_bucket_is_configured_as_specified() { // No LocationConstraint in us-east-1; the region everywhere else. The