fix(aws): review round for the live environment tests and detection

- Live tests never delete a boundary or bucket they did not create: the
  boundary goes only through its module's destroy (no direct-delete
  fallback), ownership is taken only on NoSuchEntity, and the bucket is
  deleted only if this run created it.
- Detection reads at most 200 state objects of up to 2 KiB to find empty
  ones, and a failed read keeps the entry instead of failing the scan.
- Docs: DisassociateAddress on a live association is expected-denied, not
  verified (probe listed); the e2e admin role carries no boundary, and
  why; the spec's tenancy text follows D2.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
shadowdaoandClaude Opus 5.5 committed 2026-11-14 23:38:21 -08:00
1 parent ba88323feb
commit 0b3b100a91
6 files changed
+132 -42

No files matched your search

+35 -5
View File
@@ -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<Option<String>, 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<Option<StateObjects>, 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);
@@ -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() })));
}
+44 -24
View File
@@ -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<Workdir> = 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() {
+6 -3
View File
@@ -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, &[]),
+20 -7
View File
@@ -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).
@@ -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/<owner>/<project-id>`, 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