From d2935a6397cd8497e10b32cf3fd86fe32a2fe3da Mon Sep 17 00:00:00 2001 From: captainpacket Date: Tue, 21 Jul 2026 18:01:07 -0500 Subject: [PATCH] Add removal blast-radius safeguards --- .github/RELEASE_NOTES_TEMPLATE.md | 2 + README.md | 38 +++++++++++- cmd/awssync/main.go | 86 ++++++++++++++++++++------- docs/architecture-flow.md | 19 ++++-- docs/aws-account-sync-procedure.md | 22 ++++++- docs/govcloud-workflow.md | 6 ++ docs/quick-start.md | 15 ++++- internal/app/account_manifest_test.go | 2 + internal/app/apply_plan.go | 35 +++++++---- internal/app/apply_plan_test.go | 41 +++++++++++++ internal/app/preflight.go | 10 ++++ internal/app/preflight_test.go | 13 ++++ internal/app/removal_limits.go | 77 ++++++++++++++++++++++++ internal/app/removal_limits_test.go | 55 +++++++++++++++++ internal/app/run.go | 11 ++++ internal/app/run_test.go | 2 + 16 files changed, 391 insertions(+), 43 deletions(-) create mode 100644 internal/app/removal_limits.go create mode 100644 internal/app/removal_limits_test.go diff --git a/.github/RELEASE_NOTES_TEMPLATE.md b/.github/RELEASE_NOTES_TEMPLATE.md index 1fc71c2..f6b1e02 100644 --- a/.github/RELEASE_NOTES_TEMPLATE.md +++ b/.github/RELEASE_NOTES_TEMPLATE.md @@ -2,6 +2,8 @@ ### Highlights +- Added `--max-removals` and `--max-removal-percent` blast-radius ceilings across NQE sync, manifest sync, saved-plan apply, preflight, and webhook workflows. +- Added release installation, checksum, provenance verification, automation audit handling, and explicit External ID rollback guidance. - Fixed the release checksum manifest so downloaded assets verify directly with `sha256sum -c sha256sums.txt`. - Added reversible one-time External ID migration for existing AWS setups with `external-id --value` and `external-id --clear`. - Added AWS GovCloud workflows for both regular Forward Organizations/NQE discovery and reviewed standalone-account manifests. diff --git a/README.md b/README.md index 378fcd3..2593cf0 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ An existing setup can add, replace, or clear its per-account External ID without --yes ``` -The value is written to every existing `assumeRoleInfos` entry for that setup. Review and apply the Forward payload first, test a representative account, and then update the target-role trust policies to require the identical value. After the migration PATCH, normal syncs preserve the stored External ID without rerunning this command. Use `external-id --clear` for an intentional rollback to null. Stored IAM access keys and secrets are not included in or changed by the PATCH. +The value is written to every existing `assumeRoleInfos` entry for that setup. Review and apply the Forward payload first, test a representative account, and then update the target-role trust policies to require the identical value. After the migration PATCH, normal syncs preserve the stored External ID without rerunning this command. For rollback, relax or remove the mandatory `sts:ExternalId` trust-policy condition first, confirm the role can still be assumed, and only then apply `external-id --clear`. Stored IAM access keys and secrets are not included in or changed by the PATCH. ## Procedure @@ -63,6 +63,30 @@ For GovCloud Organizations and standalone-account workflows, including collector make build ``` +## Install a Release + +Prefer the tarball because it preserves the executable bit. Download the tarball and checksum manifest for the required platform, verify both the checksum and GitHub build provenance, then extract it: + +```bash +VERSION=v2.1.2 +PLATFORM=linux-amd64 + +gh release download "$VERSION" \ + --repo forwardnetworks/aws-sync \ + --pattern "awssync-${PLATFORM}.tar.gz" \ + --pattern sha256sums.txt + +grep " awssync-${PLATFORM}.tar.gz$" sha256sums.txt | sha256sum -c - +gh attestation verify "awssync-${PLATFORM}.tar.gz" \ + --repo forwardnetworks/aws-sync \ + --signer-workflow forwardnetworks/aws-sync/.github/workflows/release.yml + +tar -xzf "awssync-${PLATFORM}.tar.gz" +./"awssync-${PLATFORM}" --help +``` + +On macOS, use `PLATFORM=darwin-amd64` or `PLATFORM=darwin-arm64` and replace `sha256sum -c -` with `shasum -a 256 -c -`. A raw binary downloaded directly from GitHub may need `chmod +x`; the tarball does not. + ## Usage Set common inputs through environment variables: @@ -160,10 +184,22 @@ Type 'apply' to continue: ``` If the plan removes accounts from a Forward setup, `--apply` fails unless `--allow-removals` is also provided. +Use `--max-removals` to cap the aggregate removal count across all selected setups and `--max-removal-percent` to cap each setup independently. Both are optional, apply-time safety ceilings; a value of `0` disables that limit. If removals are included and no uncollected candidate rows are visible, add `--allow-no-candidates` only after confirming AWS Organizations discovery. If removals are included and there is no candidate or Organizational Unit signal, add `--allow-no-org-evidence` only after independent discovery verification. In a run with multiple `--setup-id` values, this is enforced per setup and the check output includes the setup IDs that are missing signals. +For example, an approved removal run can still be limited to no more than 10 accounts overall and no more than 5% of any setup: + +```bash +./bin/awssync \ + --apply \ + --yes \ + --allow-removals \ + --max-removals 10 \ + --max-removal-percent 5 +``` + Apply a reviewed payload file without recomputing the plan: ```bash diff --git a/cmd/awssync/main.go b/cmd/awssync/main.go index 734f7bc..c1ab326 100644 --- a/cmd/awssync/main.go +++ b/cmd/awssync/main.go @@ -80,6 +80,8 @@ func newRootCommand() *cobra.Command { Insecure: v.GetBool("insecure"), Timeout: v.GetDuration("timeout"), AllowRemovals: flagBool(cmd, v, "allow-removals"), + MaxRemovals: flagInt(cmd, v, "max-removals"), + MaxRemovalPercent: flagFloat64(cmd, v, "max-removal-percent"), AllowNoCandidates: flagBool(cmd, v, "allow-no-candidates"), AllowNoOrgEvidence: flagBool(cmd, v, "allow-no-org-evidence"), MaxSnapshotAge: flagDuration(cmd, v, "max-snapshot-age"), @@ -112,6 +114,8 @@ func newRootCommand() *cobra.Command { Timeout: v.GetDuration("timeout"), Apply: apply, AllowRemovals: flagBool(cmd, v, "allow-removals"), + MaxRemovals: flagInt(cmd, v, "max-removals"), + MaxRemovalPercent: flagFloat64(cmd, v, "max-removal-percent"), AllowNoCandidates: flagBool(cmd, v, "allow-no-candidates"), AllowNoOrgEvidence: flagBool(cmd, v, "allow-no-org-evidence"), MaxSnapshotAge: flagDuration(cmd, v, "max-snapshot-age"), @@ -230,12 +234,16 @@ func bindPreflightFlags(v *viper.Viper, flags *pflag.FlagSet) { flags.String("query-setup-param", "", "optional saved-query String parameter name to receive the single selected --setup-id") flags.StringSlice("setup-id", nil, "optional Forward AWS setup ID to sync; repeatable") flags.Bool("allow-no-org-evidence", false, "allow removals when no AWS Organizations evidence is visible in NQE") + flags.Int("max-removals", 0, "maximum aggregate account removals allowed during apply; 0 disables the limit") + flags.Float64("max-removal-percent", 0, "maximum removal percentage allowed per setup during apply; 0 disables the limit") flags.Duration("max-snapshot-age", 0, "fail if latest processed snapshot is older than this duration; 0 disables the check") mustBind(v, flags, "snapshot-id") mustBind(v, flags, "query-id") mustBind(v, flags, "query-setup-param") mustBind(v, flags, "setup-id") mustBind(v, flags, "allow-no-org-evidence") + mustBind(v, flags, "max-removals") + mustBind(v, flags, "max-removal-percent") mustBind(v, flags, "max-snapshot-age") } @@ -248,6 +256,8 @@ func bindProcessingFlags(v *viper.Viper, flags *pflag.FlagSet) { flags.Bool("apply", false, "PATCH the generated setup payloads back into Forward") flags.Bool("yes", false, "skip apply confirmation prompt") flags.Bool("allow-removals", false, "allow planned account removals during apply") + flags.Int("max-removals", 0, "maximum aggregate account removals allowed during apply; 0 disables the limit") + flags.Float64("max-removal-percent", 0, "maximum removal percentage allowed per setup during apply; 0 disables the limit") flags.Bool("allow-no-candidates", false, "allow removals when no uncollected candidate accounts are visible") flags.Bool("allow-no-org-evidence", false, "allow removals when no AWS Organizations evidence is visible in NQE") flags.Duration("max-snapshot-age", 0, "fail if latest processed snapshot is older than this duration; 0 disables the check") @@ -259,6 +269,8 @@ func bindProcessingFlags(v *viper.Viper, flags *pflag.FlagSet) { mustBind(v, flags, "apply") mustBind(v, flags, "yes") mustBind(v, flags, "allow-removals") + mustBind(v, flags, "max-removals") + mustBind(v, flags, "max-removal-percent") mustBind(v, flags, "allow-no-candidates") mustBind(v, flags, "allow-no-org-evidence") mustBind(v, flags, "max-snapshot-age") @@ -296,6 +308,8 @@ func newPreflightCommand(v *viper.Viper) *cobra.Command { Insecure: v.GetBool("insecure"), Timeout: v.GetDuration("timeout"), AllowNoOrgEvidence: flagBool(cmd, v, "allow-no-org-evidence"), + MaxRemovals: flagInt(cmd, v, "max-removals"), + MaxRemovalPercent: flagFloat64(cmd, v, "max-removal-percent"), MaxSnapshotAge: flagDuration(cmd, v, "max-snapshot-age"), }) if err != nil { @@ -361,15 +375,17 @@ func newApplyPlanCommand(v *viper.Viper) *cobra.Command { return err } summary, err := app.ApplyPlan(cmd.Context(), app.ApplyPlanConfig{ - Host: v.GetString("host"), - Username: v.GetString("username"), - Password: password, - NetworkID: networkID, - PlanPath: flagString(cmd, v, "plan"), - APIPrefix: v.GetString("api-prefix"), - Insecure: v.GetBool("insecure"), - Timeout: v.GetDuration("timeout"), - AllowRemovals: flagBool(cmd, v, "allow-removals"), + Host: v.GetString("host"), + Username: v.GetString("username"), + Password: password, + NetworkID: networkID, + PlanPath: flagString(cmd, v, "plan"), + APIPrefix: v.GetString("api-prefix"), + Insecure: v.GetBool("insecure"), + Timeout: v.GetDuration("timeout"), + AllowRemovals: flagBool(cmd, v, "allow-removals"), + MaxRemovals: flagInt(cmd, v, "max-removals"), + MaxRemovalPercent: flagFloat64(cmd, v, "max-removal-percent"), }) if err != nil { return err @@ -381,9 +397,13 @@ func newApplyPlanCommand(v *viper.Viper) *cobra.Command { cmd.Flags().String("plan", "aws_sync_payload.json", "reviewed payload file to apply") cmd.Flags().Bool("yes", false, "confirm applying the reviewed payload file") cmd.Flags().Bool("allow-removals", false, "allow reviewed commercial-partition account removals; GovCloud removals must use their source workflow") + cmd.Flags().Int("max-removals", 0, "maximum aggregate account removals allowed; 0 disables the limit") + cmd.Flags().Float64("max-removal-percent", 0, "maximum removal percentage allowed per setup; 0 disables the limit") mustBind(v, cmd.Flags(), "plan") mustBind(v, cmd.Flags(), "yes") mustBind(v, cmd.Flags(), "allow-removals") + mustBind(v, cmd.Flags(), "max-removals") + mustBind(v, cmd.Flags(), "max-removal-percent") return cmd } @@ -656,18 +676,20 @@ func newSyncAccountsCommand(v *viper.Viper) *cobra.Command { return err } summary, err := app.SyncAWSAccountManifest(cmd.Context(), app.Config{ - Host: v.GetString("host"), - Username: v.GetString("username"), - Password: password, - NetworkID: networkID, - SetupIDs: setupIDs, - Output: flagString(cmd, v, "output"), - ManualOutput: flagString(cmd, v, "manual-output"), - APIPrefix: v.GetString("api-prefix"), - Insecure: v.GetBool("insecure"), - Timeout: v.GetDuration("timeout"), - Apply: apply, - AllowRemovals: flagBool(cmd, v, "allow-removals"), + Host: v.GetString("host"), + Username: v.GetString("username"), + Password: password, + NetworkID: networkID, + SetupIDs: setupIDs, + Output: flagString(cmd, v, "output"), + ManualOutput: flagString(cmd, v, "manual-output"), + APIPrefix: v.GetString("api-prefix"), + Insecure: v.GetBool("insecure"), + Timeout: v.GetDuration("timeout"), + Apply: apply, + AllowRemovals: flagBool(cmd, v, "allow-removals"), + MaxRemovals: flagInt(cmd, v, "max-removals"), + MaxRemovalPercent: flagFloat64(cmd, v, "max-removal-percent"), }, accounts) if err != nil { return err @@ -683,7 +705,9 @@ func newSyncAccountsCommand(v *viper.Viper) *cobra.Command { cmd.Flags().Bool("apply", false, "PATCH the generated setup payload into Forward") cmd.Flags().Bool("yes", false, "skip apply confirmation prompt") cmd.Flags().Bool("allow-removals", false, "allow reviewed manifest entries to remove accounts from the setup") - for _, name := range []string{"accounts-file", "setup-id", "output", "manual-output", "apply", "yes", "allow-removals"} { + cmd.Flags().Int("max-removals", 0, "maximum aggregate account removals allowed; 0 disables the limit") + cmd.Flags().Float64("max-removal-percent", 0, "maximum removal percentage allowed for the setup; 0 disables the limit") + for _, name := range []string{"accounts-file", "setup-id", "output", "manual-output", "apply", "yes", "allow-removals", "max-removals", "max-removal-percent"} { mustBind(v, cmd.Flags(), name) } return cmd @@ -722,6 +746,8 @@ func newServeWebhookCommand(v *viper.Viper) *cobra.Command { Timeout: v.GetDuration("timeout"), Apply: flagBool(cmd, v, "apply"), AllowRemovals: flagBool(cmd, v, "allow-removals"), + MaxRemovals: flagInt(cmd, v, "max-removals"), + MaxRemovalPercent: flagFloat64(cmd, v, "max-removal-percent"), AllowNoCandidates: flagBool(cmd, v, "allow-no-candidates"), AllowNoOrgEvidence: flagBool(cmd, v, "allow-no-org-evidence"), MaxSnapshotAge: flagDuration(cmd, v, "max-snapshot-age"), @@ -1043,6 +1069,22 @@ func flagBool(cmd *cobra.Command, v *viper.Viper, name string) bool { return v.GetBool(name) } +func flagInt(cmd *cobra.Command, v *viper.Viper, name string) int { + if flag := cmd.Flags().Lookup(name); flag != nil && flag.Changed { + value, _ := cmd.Flags().GetInt(name) + return value + } + return v.GetInt(name) +} + +func flagFloat64(cmd *cobra.Command, v *viper.Viper, name string) float64 { + if flag := cmd.Flags().Lookup(name); flag != nil && flag.Changed { + value, _ := cmd.Flags().GetFloat64(name) + return value + } + return v.GetFloat64(name) +} + func flagDuration(cmd *cobra.Command, v *viper.Viper, name string) time.Duration { if flag := cmd.Flags().Lookup(name); flag != nil && flag.Changed { value, _ := cmd.Flags().GetDuration(name) diff --git a/docs/architecture-flow.md b/docs/architecture-flow.md index 8e00db2..a8c0a62 100644 --- a/docs/architecture-flow.md +++ b/docs/architecture-flow.md @@ -23,6 +23,7 @@ flowchart TD removals{"Plan contains removals?"} review["Review exact account IDs"] approve["Explicit --allow-removals\nall removal paths"] + blast{"Within --max-removals\nand --max-removal-percent?"} apply["PATCH Forward setup"] block["BLOCK\nno empty/unproven inventory apply"] @@ -32,7 +33,9 @@ flowchart TD removals -- "no" --> apply removals -- "yes, NQE evidence present" --> review removals -- "yes, authoritative manifest" --> review - review --> approve --> apply + review --> approve --> blast + blast -- "yes" --> apply + blast -- "no" --> block removals -- "yes, NQE evidence absent" --> block classDef neutral fill:#F1EFE8,stroke:#5F5E5A,color:#2C2C2A; @@ -41,7 +44,7 @@ flowchart TD classDef blocked fill:#FCEBEB,stroke:#A32D2D,color:#501313; class start,snapshot,org_check,nqe,manifest,removals,review neutral; - class approve,apply safe; + class approve,blast,apply safe; class block blocked; ``` @@ -63,6 +66,7 @@ flowchart TB subgraph plan_apply["awssync — plan and apply"] plan["plan / dry-run\nPOST /nqe + GET /cloudAccounts"] disk["payload.json\nwritten to disk before any change"] + safety["Removal gates\nexplicit approval + count/% ceilings"] apply["--apply\nPATCH /cloudAccounts/{setupId}"] apply_plan["apply-plan\nreload current state + validate\nGovCloud removals refused"] end @@ -81,8 +85,9 @@ flowchart TB preflight -- "read-only" --> fwd plan --> disk - disk --> apply + disk --> safety --> apply disk --> apply_plan + apply_plan --> safety apply --> patch_accts apply_plan --> patch_accts plan --> nqe @@ -94,7 +99,7 @@ flowchart TB classDef artifact fill:#FAEEDA,stroke:#854F0B,color:#412402; class cli,cron,preflight neutral; - class plan,apply,apply_plan neutral; + class plan,safety,apply,apply_plan neutral; class nqe,get_accts,patch_accts,get_snap fwdnode; class disk artifact; ``` @@ -170,13 +175,14 @@ flowchart TB removal{"Any removals?"} patch["--apply --yes\nPATCH /cloudAccounts/{setupId}"] approved["--allow-removals\nexplicit approval"] + blast["--max-removals\n--max-removal-percent"] end manifest --> validate validate --> onboard --> create_files --> post validate --> sync --> diff --> removal removal -- "no" --> patch - removal -- "yes" --> approved --> patch + removal -- "yes" --> approved --> blast --> patch classDef neutral fill:#F1EFE8,stroke:#5F5E5A,color:#2C2C2A; classDef fwdnode fill:#E6F1FB,stroke:#185FA5,color:#042C53; @@ -186,7 +192,7 @@ flowchart TB class manifest,validate,onboard,sync,diff,removal neutral; class create_files artifact; class post,patch fwdnode; - class approved safe; + class approved,blast safe; ``` For GovCloud, use `--partition aws-us-gov` when onboarding. Existing-setup sync derives and preserves the partition from the current role ARNs. Mixed partitions or a mismatch between role ARNs and configured regions fail before a payload can be applied. @@ -404,6 +410,7 @@ flowchart LR - Static-key collector secrets are only included in the create payload when explicitly supplied. Without the secret, the file contains a placeholder and is marked not POST-ready. - Removals require explicit `--allow-removals` flag; `awssync` will not silently remove accounts from a Forward setup. +- Optional `--max-removals` and `--max-removal-percent` ceilings limit aggregate and per-setup removal blast radius and are rechecked immediately before apply. - GovCloud NQE removals additionally require positive Organizations evidence. Generic no-evidence flags cannot override this gate. - Manifest removals require an authoritative complete manifest plus `--allow-removals`. - `apply-plan` reloads current state and refuses GovCloud removals, so a saved payload cannot bypass the source workflow's safety checks. diff --git a/docs/aws-account-sync-procedure.md b/docs/aws-account-sync-procedure.md index 7ceafcd..16cb0e7 100644 --- a/docs/aws-account-sync-procedure.md +++ b/docs/aws-account-sync-procedure.md @@ -440,7 +440,9 @@ The clear payload omits `externalId` from every `assumeRoleInfos` entry, which s ```bash ./bin/awssync preflight \ - --max-snapshot-age 24h + --max-snapshot-age 24h \ + --max-removals 10 \ + --max-removal-percent 5 ``` Expected result: `ready` is `true`, `nqe_aws_accounts` passes, `patch_plan` passes, and `account_removals` either passes or is understood and approved. @@ -450,6 +452,7 @@ If `management_account_discovery` fails, the snapshot did not show any uncollect `aws_organizations_evidence` reports if the plan has either candidate visibility or OU ID visibility for each selected setup. In multi-setup runs, the check lists the setup IDs that lack this signal. Treat both as supporting evidence only. The safer destructive-sync guard is: - `account_removals` requires `--allow-removals`. +- `removal_blast_radius` confirms the aggregate count and per-setup percentage remain within the operator-supplied ceilings. - `management_account_discovery` fails: add `--allow-no-candidates` only after confirming discovery is complete. - `aws_organizations_evidence` fails: add `--allow-no-org-evidence` only after independent verification that Forward has complete AWS Organizations visibility for that setup. @@ -469,6 +472,21 @@ Expected result: the command prints `patched_setup_count` greater than zero and If the plan includes account removals, `--apply` fails unless `--allow-removals` is included. Use that flag only after reviewing `removed_accounts`. +For an approved removal, also set both blast-radius ceilings. `--max-removals` applies to the total across every selected setup, while `--max-removal-percent` applies independently to each setup's current configured-account count: + +```bash +./bin/awssync \ + --max-snapshot-age 24h \ + --output aws_sync_payload.json \ + --apply \ + --yes \ + --allow-removals \ + --max-removals 10 \ + --max-removal-percent 5 +``` + +Choose limits from the reviewed plan, leaving enough room only for the approved account IDs. The command stops before any PATCH if either ceiling is exceeded. The same flags are enforced by `sync-accounts`, `apply-plan`, and webhook-driven apply runs. + If the plan includes account removals and no uncollected candidate accounts are visible, `--apply` also requires `--allow-no-candidates`. Use that flag only after confirming the management or delegated discovery account is collected and AWS Organizations discovery is working. If the plan includes account removals and the same setup has neither candidate rows nor OU rows visible, `--apply` also requires `--allow-no-org-evidence`. Use that flag only after an independent validation that the setup is collecting from the expected AWS Organization. @@ -521,7 +539,7 @@ If a new account appears in the Forward setup but fails collection, the most lik Run `awssync` on a schedule or after AWS account lifecycle events. -The recommended automation policy is to allow routine additions while keeping removals review-gated. Run scheduled automation without `--allow-removals`; a plan containing removals will stop before changing Forward. After an operator verifies the account lifecycle in AWS and reviews `removed_accounts`, apply the reviewed plan with explicit removal approval. +The recommended automation policy is to allow routine additions while keeping removals review-gated. Run scheduled automation without `--allow-removals`; a plan containing removals will stop before changing Forward. Treat any nonzero exit as an alert requiring operator review. Retain the JSON plan, its `payload_sha256`, and the `.applied.json` audit copy from successful applies according to the customer's audit policy. After an operator verifies the account lifecycle in AWS and reviews `removed_accounts`, apply the reviewed plan with explicit removal approval and narrow `--max-removals` and `--max-removal-percent` ceilings. Recommended sequence: diff --git a/docs/govcloud-workflow.md b/docs/govcloud-workflow.md index 7877962..70afbb0 100644 --- a/docs/govcloud-workflow.md +++ b/docs/govcloud-workflow.md @@ -41,6 +41,8 @@ Configure at least `us-gov-west-1` in the Forward AWS setup. Run a Forward conne --network-id NETWORK_ID \ --setup-id GOVCLOUD_SETUP \ --max-snapshot-age 24h \ + --max-removals 5 \ + --max-removal-percent 5 \ --format human ``` @@ -134,9 +136,13 @@ If removals are intentional, the apply is blocked unless the operator also suppl --output govcloud-sync-plan.json \ --apply \ --allow-removals \ + --max-removals 5 \ + --max-removal-percent 5 \ --yes ``` +Set the ceilings to the reviewed change, not to the full account population. `--max-removals` limits the total removals in the run, and `--max-removal-percent` prevents a single setup from losing more than the approved percentage. Exceeding either value blocks before PATCH. + After any update, run a Forward connectivity test for representative accounts, run a new snapshot, and inspect per-account collection errors. Do not use `apply-plan` to bypass these source checks. `apply-plan` reloads the current Forward setup before patching and refuses GovCloud account removals; rerun the NQE or manifest workflow that produced the inventory instead. diff --git a/docs/quick-start.md b/docs/quick-start.md index b1012f2..18268f4 100644 --- a/docs/quick-start.md +++ b/docs/quick-start.md @@ -94,7 +94,9 @@ This is a one-time change separate from the AWS Organizations setup checklist. T Confirm the dry run reports the expected prior and target state and verify every generated account entry has the expected `externalId`. Apply the reviewed command and test one account. Then configure the identical `sts:ExternalId` condition in each target collection role trust policy, preferably through the existing StackSet or account-vending automation, and test again before broad rollout. -Run the command once per Forward AWS setup if different setups require different values. It does not replace or expose the IAM user's stored access key or secret. After the migration PATCH, later syncs preserve the stored External ID without rerunning it. To roll back intentionally, replace `--value VALUE` with `--clear`, review the dry run, and apply it. +Run the command once per Forward AWS setup if different setups require different values. It does not replace or expose the IAM user's stored access key or secret. After the migration PATCH, later syncs preserve the stored External ID without rerunning it. + +Rollback order matters: first relax or remove the mandatory `sts:ExternalId` condition from the target-role trust policies and confirm a representative role can still be assumed. Then replace `--value VALUE` with `--clear`, review the dry run, apply it, and test collection again. Clearing Forward first while AWS still requires the External ID will interrupt collection. ## Discover Before Onboarding @@ -148,9 +150,18 @@ terraform -chdir=examples/terraform/forward-collection-role-stackset apply If removals are expected: ```bash -./bin/awssync --max-snapshot-age 24h --output aws_sync_payload.json --apply --yes --allow-removals +./bin/awssync \ + --max-snapshot-age 24h \ + --output aws_sync_payload.json \ + --apply \ + --yes \ + --allow-removals \ + --max-removals 10 \ + --max-removal-percent 5 ``` +`--max-removals` is the aggregate ceiling across selected setups. `--max-removal-percent` is evaluated separately for each setup against its current configured-account count. Preflight accepts the same limits and reports `removal_blast_radius` before anything is patched. + If removals are expected and no uncollected candidate accounts are visible, also add `--allow-no-candidates` only after confirming AWS Organizations discovery is working. If removals are expected and there is no candidate signal and no OU signal for a setup, also add `--allow-no-org-evidence` only after independent verification that Forward’s discovery account is still collecting a complete AWS Organization account list. diff --git a/internal/app/account_manifest_test.go b/internal/app/account_manifest_test.go index 6edfc39..af82e21 100644 --- a/internal/app/account_manifest_test.go +++ b/internal/app/account_manifest_test.go @@ -159,6 +159,8 @@ func TestSyncAWSAccountManifestDryRunReportsRemovalAndApplyRequiresApproval(t *t approved := blocked approved.AllowRemovals = true + approved.MaxRemovals = 1 + approved.MaxRemovalPercent = 50 approved.Output = filepath.Join(t.TempDir(), "approved.json") if _, err := SyncAWSAccountManifest(context.Background(), approved, accounts); err != nil { t.Fatalf("approved apply error = %v", err) diff --git a/internal/app/apply_plan.go b/internal/app/apply_plan.go index 87db309..3c28b8e 100644 --- a/internal/app/apply_plan.go +++ b/internal/app/apply_plan.go @@ -14,15 +14,17 @@ import ( ) type ApplyPlanConfig struct { - Host string - Username string - Password string - NetworkID string - PlanPath string - APIPrefix string - Insecure bool - Timeout time.Duration - AllowRemovals bool + Host string + Username string + Password string + NetworkID string + PlanPath string + APIPrefix string + Insecure bool + Timeout time.Duration + AllowRemovals bool + MaxRemovals int + MaxRemovalPercent float64 } type ApplyPlanSummary struct { @@ -35,6 +37,9 @@ type ApplyPlanSummary struct { } func ApplyPlan(ctx context.Context, cfg ApplyPlanConfig) (*ApplyPlanSummary, error) { + if err := validateRemovalLimitValues(cfg.MaxRemovals, cfg.MaxRemovalPercent); err != nil { + return nil, err + } client, err := api.NewClient(cfg.Host, cfg.APIPrefix, cfg.Username, cfg.Password, cfg.Insecure, cfg.Timeout) if err != nil { return nil, err @@ -78,6 +83,7 @@ func ApplyPlan(ctx context.Context, cfg ApplyPlanConfig) (*ApplyPlanSummary, err for _, account := range cloudAccounts { currentByName[strings.TrimSpace(account.Name)] = account } + removalStats := make([]removalStat, 0, len(setupIDs)) for _, setupID := range setupIDs { current, ok := currentByName[setupID] if !ok { @@ -97,7 +103,13 @@ func ApplyPlan(ctx context.Context, cfg ApplyPlanConfig) (*ApplyPlanSummary, err if err := validateCloudAccountPartition(planned); err != nil { return nil, fmt.Errorf("plan setup %s: %w", setupID, err) } - _, removed, _ := accountDiff(currentAccounts(current.AssumeRoleInfos), currentAccounts(payload.AssumeRoleInfos)) + currentRows := currentAccounts(current.AssumeRoleInfos) + _, removed, _ := accountDiff(currentRows, currentAccounts(payload.AssumeRoleInfos)) + removalStats = append(removalStats, removalStat{ + SetupID: setupID, + ConfiguredCount: len(currentRows), + RemovedCount: len(removed), + }) if len(removed) == 0 { continue } @@ -108,6 +120,9 @@ func ApplyPlan(ctx context.Context, cfg ApplyPlanConfig) (*ApplyPlanSummary, err return nil, fmt.Errorf("plan removes %d account(s) from setup %s; apply-plan requires --allow-removals", len(removed), setupID) } } + if err := validateRemovalStats(removalStats, cfg.MaxRemovals, cfg.MaxRemovalPercent); err != nil { + return nil, err + } sort.Strings(setupIDs) for _, setupID := range setupIDs { if err := client.PatchCloudAccount(ctx, cfg.NetworkID, setupID, payloads[setupID]); err != nil { diff --git a/internal/app/apply_plan_test.go b/internal/app/apply_plan_test.go index 059739c..eada94a 100644 --- a/internal/app/apply_plan_test.go +++ b/internal/app/apply_plan_test.go @@ -102,3 +102,44 @@ func TestApplyPlanCannotBypassGovCloudRemovalSafety(t *testing.T) { t.Fatal("unsafe GovCloud apply-plan reached PATCH") } } + +func TestApplyPlanBlocksRemovalPercentageAboveLimit(t *testing.T) { + patched := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/networks/network-1/cloudAccounts": + _, _ = w.Write([]byte(`[{"type":"AWS","name":"prod","assumeRoleInfos":[ + {"accountId":"111","roleArn":"arn:aws:iam::111:role/ForwardRole","enabled":true}, + {"accountId":"222","roleArn":"arn:aws:iam::222:role/ForwardRole","enabled":true} + ]}]`)) + case r.Method == http.MethodPatch: + patched = true + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + planPath := filepath.Join(t.TempDir(), "payload.json") + if err := os.WriteFile(planPath, []byte(`{"prod":{"type":"AWS","name":"prod","assumeRoleInfos":[ + {"accountId":"111","roleArn":"arn:aws:iam::111:role/ForwardRole","enabled":true} + ]}}`), 0o600); err != nil { + t.Fatal(err) + } + _, err := ApplyPlan(context.Background(), ApplyPlanConfig{ + Host: server.URL, + Username: "user", + Password: "pass", + NetworkID: "network-1", + PlanPath: planPath, + APIPrefix: "/api", + AllowRemovals: true, + MaxRemovalPercent: 49, + }) + if err == nil || !strings.Contains(err.Error(), "50.00%") { + t.Fatalf("expected removal percentage block, got %v", err) + } + if patched { + t.Fatal("removal above percentage limit reached PATCH") + } +} diff --git a/internal/app/preflight.go b/internal/app/preflight.go index d37219e..bd64559 100644 --- a/internal/app/preflight.go +++ b/internal/app/preflight.go @@ -32,6 +32,9 @@ type PreflightCheck struct { } func Preflight(ctx context.Context, cfg Config) (*PreflightSummary, error) { + if err := validateRemovalLimitValues(cfg.MaxRemovals, cfg.MaxRemovalPercent); err != nil { + return nil, err + } client, err := api.NewClient(cfg.Host, cfg.APIPrefix, cfg.Username, cfg.Password, cfg.Insecure, cfg.Timeout) if err != nil { return nil, err @@ -134,6 +137,13 @@ func Preflight(ctx context.Context, cfg Config) (*PreflightSummary, error) { } else { result.pass("account_removals", "no account removals planned") } + if cfg.MaxRemovals > 0 || cfg.MaxRemovalPercent > 0 { + if err := validateRemovalStats(plan.removalStats(), cfg.MaxRemovals, cfg.MaxRemovalPercent); err != nil { + result.fail("removal_blast_radius", err.Error()) + } else { + result.pass("removal_blast_radius", "planned removals are within the configured count and percentage limits") + } + } if plan.HasCandidateRisk() { result.fail("management_account_discovery", "one or more selected setups have no uncollected candidate accounts visible") } else { diff --git a/internal/app/preflight_test.go b/internal/app/preflight_test.go index f62f45c..05a4679 100644 --- a/internal/app/preflight_test.go +++ b/internal/app/preflight_test.go @@ -46,6 +46,8 @@ func TestPreflightReportsSetupSpecificOrgEvidenceFailures(t *testing.T) { SetupIDs: nil, MaxSnapshotAge: 0, AllowNoOrgEvidence: false, + MaxRemovals: 10, + MaxRemovalPercent: 40, }) if err != nil { t.Fatalf("Preflight() error = %v", err) @@ -70,6 +72,17 @@ func TestPreflightReportsSetupSpecificOrgEvidenceFailures(t *testing.T) { if !strings.Contains(checkMessage, "--allow-no-org-evidence") { t.Fatalf("expected guidance for --allow-no-org-evidence, got: %q", checkMessage) } + + var blastRadiusStatus, blastRadiusMessage string + for _, check := range summary.Checks { + if check.Name == "removal_blast_radius" { + blastRadiusStatus = check.Status + blastRadiusMessage = check.Message + } + } + if blastRadiusStatus != "fail" || !strings.Contains(blastRadiusMessage, "setup-b") { + t.Fatalf("expected setup-b percentage limit failure, got status=%q message=%q", blastRadiusStatus, blastRadiusMessage) + } } func TestPlansWithoutOrganizationEvidenceSortsSetups(t *testing.T) { diff --git a/internal/app/removal_limits.go b/internal/app/removal_limits.go new file mode 100644 index 0000000..049269e --- /dev/null +++ b/internal/app/removal_limits.go @@ -0,0 +1,77 @@ +package app + +import ( + "fmt" + "strings" +) + +type removalStat struct { + SetupID string + ConfiguredCount int + RemovedCount int +} + +func validateRemovalLimitValues(maxRemovals int, maxRemovalPercent float64) error { + if maxRemovals < 0 { + return fmt.Errorf("--max-removals cannot be negative") + } + if maxRemovalPercent < 0 || maxRemovalPercent > 100 { + return fmt.Errorf("--max-removal-percent must be between 0 and 100") + } + return nil +} + +func validateRemovalStats(stats []removalStat, maxRemovals int, maxRemovalPercent float64) error { + if err := validateRemovalLimitValues(maxRemovals, maxRemovalPercent); err != nil { + return err + } + problems := make([]string, 0) + totalRemoved := 0 + for _, stat := range stats { + totalRemoved += stat.RemovedCount + } + if maxRemovals > 0 && totalRemoved > maxRemovals { + problems = append(problems, fmt.Sprintf( + "planned removals total %d exceeds --max-removals %d", + totalRemoved, + maxRemovals, + )) + } + if maxRemovalPercent > 0 { + for _, stat := range stats { + if stat.RemovedCount == 0 { + continue + } + percent := 100.0 + if stat.ConfiguredCount > 0 { + percent = float64(stat.RemovedCount) * 100 / float64(stat.ConfiguredCount) + } + if percent > maxRemovalPercent { + problems = append(problems, fmt.Sprintf( + "setup %s removes %d of %d accounts (%.2f%%), exceeding --max-removal-percent %.2f", + stat.SetupID, + stat.RemovedCount, + stat.ConfiguredCount, + percent, + maxRemovalPercent, + )) + } + } + } + if len(problems) > 0 { + return fmt.Errorf("removal blast-radius check failed: %s", strings.Join(problems, "; ")) + } + return nil +} + +func (p *patchPlan) removalStats() []removalStat { + stats := make([]removalStat, 0, len(p.Setups)) + for _, setup := range p.Setups { + stats = append(stats, removalStat{ + SetupID: setup.SetupID, + ConfiguredCount: len(setup.CurrentAccounts), + RemovedCount: len(setup.RemovedAccounts), + }) + } + return stats +} diff --git a/internal/app/removal_limits_test.go b/internal/app/removal_limits_test.go new file mode 100644 index 0000000..4da14e6 --- /dev/null +++ b/internal/app/removal_limits_test.go @@ -0,0 +1,55 @@ +package app + +import ( + "strings" + "testing" +) + +func TestValidateRemovalStatsEnforcesAggregateAndPerSetupLimits(t *testing.T) { + stats := []removalStat{ + {SetupID: "collect_aws_all_ihsm", ConfiguredCount: 202, RemovedCount: 6}, + {SetupID: "collect_spgi_all_aws", ConfiguredCount: 363, RemovedCount: 21}, + } + + if err := validateRemovalStats(stats, 27, 6); err != nil { + t.Fatalf("expected boundary limits to pass, got %v", err) + } + if err := validateRemovalStats(stats, 26, 0); err == nil || !strings.Contains(err.Error(), "total 27") { + t.Fatalf("expected aggregate removal limit failure, got %v", err) + } + if err := validateRemovalStats(stats, 0, 5); err == nil || !strings.Contains(err.Error(), "collect_spgi_all_aws") { + t.Fatalf("expected per-setup percentage failure, got %v", err) + } +} + +func TestValidateRemovalLimitValuesRejectsInvalidLimits(t *testing.T) { + for _, tc := range []struct { + name string + maxCount int + maxPercent float64 + }{ + {name: "negative count", maxCount: -1}, + {name: "negative percent", maxPercent: -0.1}, + {name: "percent over one hundred", maxPercent: 100.1}, + } { + t.Run(tc.name, func(t *testing.T) { + if err := validateRemovalLimitValues(tc.maxCount, tc.maxPercent); err == nil { + t.Fatal("expected invalid removal limit error") + } + }) + } +} + +func TestPatchPlanRemovalStatsUseCurrentConfiguredCounts(t *testing.T) { + plan := &patchPlan{Setups: []plannedSetup{ + { + SetupID: "setup-a", + CurrentAccounts: []accountRow{{AccountID: "111"}, {AccountID: "222"}}, + RemovedAccounts: []accountRow{{AccountID: "222"}}, + }, + }} + stats := plan.removalStats() + if len(stats) != 1 || stats[0].ConfiguredCount != 2 || stats[0].RemovedCount != 1 { + t.Fatalf("unexpected removal stats: %#v", stats) + } +} diff --git a/internal/app/run.go b/internal/app/run.go index 88d8226..23e7a4b 100644 --- a/internal/app/run.go +++ b/internal/app/run.go @@ -61,6 +61,8 @@ type Config struct { Timeout time.Duration Apply bool AllowRemovals bool + MaxRemovals int + MaxRemovalPercent float64 AllowNoCandidates bool AllowNoOrgEvidence bool MaxSnapshotAge time.Duration @@ -200,6 +202,9 @@ type AWSOrganizationConfig struct { } func Run(ctx context.Context, cfg Config) (*Summary, error) { + if err := validateRemovalLimitValues(cfg.MaxRemovals, cfg.MaxRemovalPercent); err != nil { + return nil, err + } client, err := api.NewClient(cfg.Host, cfg.APIPrefix, cfg.Username, cfg.Password, cfg.Insecure, cfg.Timeout) if err != nil { return nil, err @@ -280,6 +285,12 @@ func runPlannedSync( summary.RemovalBlocked = true return summary, fmt.Errorf("planned account removals require --allow-removals") } + if cfg.Apply { + if err := validateRemovalStats(plan.removalStats(), cfg.MaxRemovals, cfg.MaxRemovalPercent); err != nil { + summary.RemovalBlocked = true + return summary, err + } + } if cfg.Apply && !cfg.AuthoritativeInput && plan.HasCandidateRemovalRisk() && !cfg.AllowNoCandidates { summary.RemovalBlocked = true return summary, fmt.Errorf("planned removals with no uncollected candidate accounts visible require --allow-no-candidates") diff --git a/internal/app/run_test.go b/internal/app/run_test.go index 5498a4e..0c5fd02 100644 --- a/internal/app/run_test.go +++ b/internal/app/run_test.go @@ -417,6 +417,8 @@ func TestRunBlocksGovCloudRemovalWithoutOrgEvidenceEvenWithBreakGlassFlags(t *te Insecure: true, Apply: true, AllowRemovals: true, + MaxRemovals: 1, + MaxRemovalPercent: 50, AllowNoCandidates: true, AllowNoOrgEvidence: true, })