diff --git a/app/src-tauri/src/aws/environments.rs b/app/src-tauri/src/aws/environments.rs index cd589f2..430b536 100644 --- a/app/src-tauri/src/aws/environments.rs +++ b/app/src-tauri/src/aws/environments.rs @@ -48,6 +48,15 @@ const RECORD_SCHEMA: u64 = 1; const MAX_RECORDS: usize = 500; const MAX_OBJECTS_PER_PREFIX: usize = 5000; const MAX_LOCKS: usize = 200; +/// A state object larger than this is not read to see whether it is empty. +/// What `tofu destroy` leaves is about 180 bytes plus about 110 per +/// checkable object (OpenTofu 1.13 keeps `check_results` for each resource +/// or data source with a pre/postcondition): 453 bytes for two, measured. +/// The modules have at most a handful, so 2 KiB leaves a wide margin. +const EMPTY_STATE_MAX_BYTES: i64 = 2048; +/// At most this many small state objects are read per scan; the rest count +/// as deployments. +const MAX_STATE_PEEKS: usize = 200; /// An environment's record, schema 1 (`aws/terraform/environment/main.tf`). #[derive(Deserialize, Serialize, Clone, Debug, PartialEq)] @@ -583,6 +592,27 @@ pub(crate) fn is_empty_state(text: &str) -> bool { .unwrap_or(false) } +/// Whether a state object of `size` bytes is read to see if it is empty: +/// only a small one, and only the first [`MAX_STATE_PEEKS`] per scan. +fn may_be_empty_state(size: i64, peeks_so_far: usize) -> bool { + (0..=EMPTY_STATE_MAX_BYTES).contains(&size) && peeks_so_far < MAX_STATE_PEEKS +} + +/// Whether a read state object still counts as a deployment. Fails closed: +/// only a state read in full and found empty is dropped (or one deleted +/// since the listing); unreadable, denied or failed reads keep it, and one +/// failed read never fails the whole scan. +fn keep_state(key: &str, read: Result, AwsError>) -> bool { + match read { + Ok(None) => false, + Ok(Some(text)) => !is_empty_state(&text), + Err(e) => { + log::warn!("Could not read the state object {key} ({e}); counting it as a deployment"); + true + } + } +} + /// What a failed lookup means. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum Lookup { @@ -805,6 +835,7 @@ impl DetectApi for SdkDetectApi { async fn state_objects(&self, bucket: &str) -> Result, AwsError> { let mut objects = StateObjects::default(); let mut lock_keys = Vec::new(); + let mut peeks = 0usize; for prefix in ["environments/", "users/", "boundary/"] { let Some(listed) = self.list(bucket, prefix).await? else { return Ok(None); @@ -817,11 +848,10 @@ impl DetectApi for SdkDetectApi { // own Destroy) would read as Incomplete for ever, and a // removed onboarding as Joined. Only a small object can // be empty, so only those are read. - if (0..=MAX_UNTRUSTED_BYTES as i64).contains(&size) { - match self.lock_contents(bucket, &key, size).await? { - None => continue, - Some(text) if is_empty_state(&text) => continue, - Some(_) => {} + if may_be_empty_state(size, peeks) { + peeks += 1; + if !keep_state(&key, self.lock_contents(bucket, &key, size).await) { + continue; } } objects.states.push(key); diff --git a/app/src-tauri/src/aws/environments/tests.rs b/app/src-tauri/src/aws/environments/tests.rs index 90c2751..b485bf9 100644 --- a/app/src-tauri/src/aws/environments/tests.rs +++ b/app/src-tauri/src/aws/environments/tests.rs @@ -839,3 +839,26 @@ fn only_a_state_that_manages_nothing_is_empty() { assert!(!is_empty_state(odd), "{odd:?}"); } } + +#[test] +fn only_small_states_are_read_and_only_up_to_the_cap() { + assert!(may_be_empty_state(0, 0)); + assert!(may_be_empty_state(453, 0)); + assert!(may_be_empty_state(EMPTY_STATE_MAX_BYTES, MAX_STATE_PEEKS - 1)); + assert!(!may_be_empty_state(EMPTY_STATE_MAX_BYTES + 1, 0)); + assert!(!may_be_empty_state(-1, 0)); + assert!(!may_be_empty_state(100, MAX_STATE_PEEKS)); +} + +#[test] +fn a_state_that_cannot_be_read_still_counts() { + let empty = r#"{"version":4,"outputs":{},"resources":[],"check_results":[{"object_kind":"resource","config_addr":"terraform_data.b","status":"unknown","objects":null}]}"#; + assert!(!keep_state("k", Ok(Some(empty.into())))); + // Deleted since the listing. + assert!(!keep_state("k", Ok(None))); + // Denied reads arrive as Some("") from lock_contents. + assert!(keep_state("k", Ok(Some(String::new())))); + assert!(keep_state("k", Ok(Some(r#"{"resources":[{"type":"x"}]}"#.into())))); + // An S3 error on one object degrades that entry, not the scan. + assert!(keep_state("k", Err(AwsError::Other { message: "boom".into() }))); +} diff --git a/app/src-tauri/src/aws/live_tests.rs b/app/src-tauri/src/aws/live_tests.rs index 77be47b..803aea8 100644 --- a/app/src-tauri/src/aws/live_tests.rs +++ b/app/src-tauri/src/aws/live_tests.rs @@ -1237,8 +1237,29 @@ mod environment { .expect("child env") } + /// `ensure_state_bucket` as the environment admin. The bucket is + /// marked for deletion only if this call is what creates it: a bucket + /// that already exists (a fixed `TRIPLEC_E2E_BUCKET_SUFFIX`) is never + /// deleted by the cleanup. async fn ensure_bucket(&self, cfg: &SdkConfig, who: &CallerIdentity) { - *self.bucket_created.lock().unwrap() = true; + use aws_sdk_s3::error::ProvideErrorMetadata; + let head = aws_sdk_s3::Client::new(&self.e.cfg) + .head_bucket() + .bucket(&self.bucket) + .send() + .await; + match head { + Ok(_) => eprintln!("{} already exists; the cleanup leaves it", self.bucket), + Err(e) + if e.raw_response().map(|r| r.status().as_u16()) == Some(404) + || e.code() == Some("NotFound") => + { + // Set before the create, so a create that half-succeeds + // is still cleaned up. + *self.bucket_created.lock().unwrap() = true; + } + Err(e) => panic!("HeadBucket {}: {e:?}", self.bucket), + } let b = ensure_named_state_bucket(cfg, &who.account, &self.e.region, &self.bucket) .await .unwrap_or_else(|e| panic!("ensure_state_bucket as the environment admin: {e}")); @@ -1252,10 +1273,19 @@ mod environment { "arn:aws:iam::{}:policy/{PERMISSIONS_BOUNDARY_NAME}", self.e.account ); - if self.e.iam.get_policy().policy_arn(&arn).send().await.is_ok() { - eprintln!("the account already has the boundary; using it"); - return; + match self.e.iam.get_policy().policy_arn(&arn).send().await { + Ok(_) => { + eprintln!("the account already has the boundary; using it"); + return; + } + Err(e) if e.as_service_error().is_some_and(|s| s.is_no_such_entity_exception()) => {} + // Anything else is not "absent": never take ownership of a + // boundary this run can't see. + Err(e) => panic!("iam:GetPolicy on the boundary: {e:?}"), } + // Ownership is the module's state: the cleanup removes the + // boundary only with `tofu destroy` of that state, so a failed + // apply (EntityAlreadyExists in a race) destroys nothing. *self.boundary_created.lock().unwrap() = true; let (_, outcome, denied) = self .run( @@ -1270,9 +1300,9 @@ mod environment { } /// As the account admin: destroy whatever is still applied (newest - /// first), the bucket with every version, and the boundary if this - /// test made it. Logs rather than fails, so one error strands nothing - /// else. + /// first; the boundary only through its own module's state), then the + /// bucket with every version if this run created it. Logs rather than + /// fails, so one error strands nothing else. async fn cleanup(&self) { let pending: Vec = self .applied @@ -1304,29 +1334,19 @@ mod environment { other => eprintln!("cleanup: could not start destroy: {other:?}"), } } + // No direct delete: the boundary module's `tofu destroy` above is + // the only thing that removes it, because its state proves this + // run created it. If that failed, say so loudly. if *self.boundary_created.lock().unwrap() { let arn = format!( "arn:aws:iam::{}:policy/{PERMISSIONS_BOUNDARY_NAME}", self.e.account ); if self.e.iam.get_policy().policy_arn(&arn).send().await.is_ok() { - eprintln!("cleanup: deleting the boundary directly"); - if let Ok(out) = self.e.iam.list_entities_for_policy().policy_arn(&arn).send().await { - for r in out.policy_roles() { - if let Some(name) = r.role_name() { - let _ = self - .e - .iam - .delete_role_permissions_boundary() - .role_name(name) - .send() - .await; - } - } - } - if let Err(e) = self.e.iam.delete_policy().policy_arn(&arn).send().await { - eprintln!("cleanup: boundary: {e:?}"); - } + eprintln!( + "cleanup: WARNING: {arn} still exists after its module's destroy; \ + remove it by hand if this run created it" + ); } } if *self.bucket_created.lock().unwrap() { diff --git a/app/src-tauri/src/aws/policies.rs b/app/src-tauri/src/aws/policies.rs index f943b43..c1bfe33 100644 --- a/app/src-tauri/src/aws/policies.rs +++ b/app/src-tauri/src/aws/policies.rs @@ -540,7 +540,8 @@ const ENVIRONMENT_ADMIN_CHECKS: &[Check] = &[ write("ec2:ReleaseAddress", PROBE_ELASTIC_IP, COMPONENT_TAGGED), // Destroying the NAT gateway's address disassociates it first; the NAT // gateway is gone by then, so EC2 authorizes the call against the - // literal `*/*` (verified live), which is all the policy allows. + // literal `*/*` (verified live), which is all the policy allows. That a + // live association never matches it is expected, not verified. write("ec2:DisassociateAddress", PROBE_GONE_ASSOCIATION, &[]), write("ec2:DeleteNatGateway", PROBE_NAT_GATEWAY, COMPONENT_TAGGED), write( @@ -1849,8 +1850,10 @@ mod tests { ME, ME )); - // Only the literal `*/*` of an association that is already gone: a - // live address, Triple-C's or anyone's, is never disassociated. + // Only the literal `*/*` of an association that is already gone. That + // a live association is authorized against its address and interface + // ARNs (so never matches) is expected, NOT live-verified: see "Still + // open" in aws/README.md for the probe. assert!(allowed( tier, &write("ec2:DisassociateAddress", PROBE_GONE_ASSOCIATION, &[]), diff --git a/aws/README.md b/aws/README.md index 84af0b6..7f3774c 100644 --- a/aws/README.md +++ b/aws/README.md @@ -203,8 +203,10 @@ account-wide: nothing else (`${*}` is IAM's escaped `*`). Destroying the NAT gateway's address disassociates it first; by then the NAT gateway and its association are gone and EC2 authorizes the call against `*/*` (verified live: denied without this, then `InvalidAssociationID.NotFound`, which - the provider ignores). A live association names its address and interface, so no real address - can be disassociated through this statement. + the provider ignores). **Expected, not live-verified:** a live association should be + authorized against its address and interface ARNs, which `${*}/${*}` does not match, so no real + address should be disassociable through this statement. Only the gone-association case was + observed; the probe that would settle it is listed under "Still open" below. - `ec2:AuthorizeSecurityGroupIngress`/`Egress` on `security-group-rule/*`: authorizing names the new, untagged rule as well as the group, and the group must still be tagged. - `ec2:CreateNetworkInterface` and `ec2:DeleteNetworkInterface` on `*`, but only when EFS makes @@ -436,8 +438,9 @@ record, joinable), self-onboard (Joined, both roles carry the boundary), update destroy the onboarding, then the environment. If the account has no boundary, the account admin applies the boundary module first and destroys it at the end. `the_private_nat_mode_deploys` does the same for `private-nat` (about $0.05: one NAT gateway for a few minutes). A guard destroys -whatever is still applied, as the account admin, and deletes the bucket with every version, on -failure too; every denied action is printed by name. The test drives the same functions as the +whatever is still applied, as the account admin (the boundary only through its module's state, so +a boundary the run did not create is never touched), and deletes the bucket with every version if +the run created it, on failure too; every denied action is printed by name. The test drives the same functions as the commands (`ensure_tofu`, `materialize`, `child_env`, the runner, `follow`, `scan`, the refusal checks, the conditional lock delete); the Tauri commands' own bookkeeping (launch slots, the progress events) is not exercised. @@ -452,8 +455,18 @@ progress events) is not exercised. | NAT address on destroy | ✗→fixed: the provider disassociates the address before releasing it, after the NAT gateway (and the association) are gone, and EC2 authorizes that call against the literal `*/*`. `DisassociateGoneAddresses` allows exactly `arn:aws:ec2:*:*:${*}/${*}`; CloudTrail shows the retried call reaching `InvalidAssociationID.NotFound` | | Conditional unlock | ✓ on the versioned state bucket, `DeleteObject` with `If-Match` and a mismatched ETag gets 412 (the app's "lock changed" answer) and leaves the lock; with the current ETag it deletes it. A fresh foreign lock scans as Deploying with its `Who` | | Update | ✓ re-applying with the network read back from the record: "0 added, 0 changed, 0 destroyed" | -| After `tofu destroy` | ✗→fixed (app): the state object stays, empty, so a destroyed environment kept scanning as Incomplete (and a removed onboarding as Joined). Detection now reads a small state object and treats one with no resources as absent | +| After `tofu destroy` | ✗→fixed (app): the state object stays, empty, so a destroyed environment kept scanning as Incomplete (and a removed onboarding as Joined). Detection now reads small state objects (up to 2 KiB, at most 200 per scan) and treats one with no resources as absent; a read that fails counts as a deployment | | Cleanup | ✓ nothing `triple-c` left in us-west-2 (roles, policies, log groups, clusters, VPCs, subnets, gateways, NAT gateways, addresses, security groups, file systems, parameters, aliases, buckets); five environment KMS keys pending deletion (7 days); the boundary, which the account did not have, deleted again | -Not deployed live: the `existing` network mode, and an onboarding by a workspace user from an -admin bundle (the same module, applied with admin credentials). +The environment-admin role in these tests carries **no permissions boundary**, deliberately: the +boundary is the ceiling for the task and execution roles the modules create, not for admins (it +allows only what a task does, so an admin under it could administer nothing). The test instead +asserts that every role the admin creates carries the boundary. + +Still open: + +- the `existing` network mode against a user's own VPC, and an onboarding by a workspace user + from an admin bundle (the same module, applied with admin credentials); +- `DisassociateGoneAddresses` cannot touch a **live** association. Probe: allocate an untagged + Elastic IP, associate it with a test ENI, and call `ec2:DisassociateAddress` on that association + as the environment admin; it must be denied (then release both). diff --git a/docs/superpowers/specs/2026-11-13-aws-ecs-host-design.md b/docs/superpowers/specs/2026-11-13-aws-ecs-host-design.md index 5215ae9..1c77bbc 100644 --- a/docs/superpowers/specs/2026-11-13-aws-ecs-host-design.md +++ b/docs/superpowers/specs/2026-11-13-aws-ecs-host-design.md @@ -333,9 +333,10 @@ updates use the existing "new image available" flow: the next start uses the new | **Hybrid** (default) | task storage | EFS | Settings, sessions, memory, history: yes. Working tree: only what's pushed | Fast builds, fast git | Small EFS footprint | | **EFS** | EFS | EFS | Everything | git, `node_modules` and builds are noticeably slower over NFS | Grows with repo and dependency size | -- **One EFS file system per environment, one access point per workspace.** Access point root: +- **One EFS file system per user (D2), one access point per workspace.** The file system comes + from the user's onboarding module and its policy admits only their task role. Access point root: `/triple-c//`, with POSIX user and group 1000. The access point is created by - the user and tagged with the owner (see policies). + the user, on their own file system, and tagged with the owner (see policies). - **WIP push is on by default** (`wip_push_on_stop: true`, with a serde default per the `default_full_permissions` pattern) and can be turned off per project. - **Ephemeral and Hybrid have a stop guard.** Before stopping, the app asks the agent for @@ -440,7 +441,7 @@ before release.** | Mode | For | Task role | EC2 options | Isolation | |---|---|---|---|---| -| **Strict** (**default, v1**) | Everyone, from a solo account to a team | **Per-user** task role, created by the user's onboarding module (self-deployed, or by an admin) | Privileged and Docker allowed **only on a per-user capacity provider** (dedicated EC2 instances, never shared with another owner) | Per-user IAM all the way down. EFS file system policy allows each access point only to its owner's task role | +| **Strict** (**default, v1**) | Everyone, from a solo account to a team | **Per-user** task role, created by the user's onboarding module (self-deployed, or by an admin) | Privileged and Docker allowed **only on a per-user capacity provider** (dedicated EC2 instances, never shared with another owner) | Per-user IAM all the way down. Each user has their own EFS file system (D2), whose policy admits only that user's task role | | **Standard** (opt-in, later) | Large teams that want fewer IAM roles | Shared, minimal (logs, SSM messages) | **Fargate only**, or EC2 with **no** privileged mode, Docker socket or `NET_ADMIN` | Containers are unprivileged and can't mount NFS, can't read secrets, and can't reach each other over the network | There is no separate "personal" mode. A solo user on strict tenancy is simply the only onboarded