From 8b0836a0333628bd5f8fbb6d7b0345a20e7f8796 Mon Sep 17 00:00:00 2001 From: Ben Fellows Date: Sun, 2 Aug 2026 21:05:40 +1200 Subject: [PATCH 1/2] SNS Topic filter security incoming messages --- app/.env.example | 8 + .../Console/Commands/ProcessSqsMessages.php | 253 ++++++++++-- .../Console/Commands/ShowLinkRejections.php | 93 +++++ .../Controllers/Api/AwsAccountsController.php | 23 +- app/app/Services/SnsSignatureVerifier.php | 14 +- app/config/services.php | 9 + .../Feature/AwsAccountsControllerTest.php | 60 +++ app/tests/Unit/ProcessSqsMessagesTest.php | 382 +++++++++++++++++- docker/app/entrypoint.sh | 8 + docker/installer/scripts/install-messaging.sh | 84 +++- .../sns-topic-publish-authorization.md | 117 +++++- docs/roadmap.md | 16 +- infra/cloud-stack/stackset/sns.topic.cfn.yaml | 118 +++++- templates/iam.role.audit.account.cfn.yaml | 13 + templates/iam.role.child.account.cfn.yaml | 20 +- tests/install-messaging.test.sh | 117 ++++++ 16 files changed, 1261 insertions(+), 74 deletions(-) create mode 100644 app/app/Console/Commands/ShowLinkRejections.php diff --git a/app/.env.example b/app/.env.example index 246ea4e2..c6c12f5c 100644 --- a/app/.env.example +++ b/app/.env.example @@ -102,6 +102,14 @@ TOPS_SQS_NAME= TOPS_SQS_ARN= TOPS_SQS_DLQ_NAME= TOPS_SQS_DLQ_ARN= +# Minted once by the installer and written to generated/teemops.env. Leave blank +# here — setting it is the deliberate rotation path, not routine configuration. +TOPS_INSTALL_ID= +# How many hours a "Connect AWS account" link stays usable (N-11). Until the +# stack is run, the pending record is a live target for anyone who can publish to +# the SNS topic, so it expires; the user just reconnects from the UI to get a new +# one. Raise it if your approvals take longer than a day. 0 disables expiry. +TOPS_ACCOUNT_LINK_WINDOW_HOURS=24 # AWS_DEFAULT_REGION is set once, further up with the other AWS values. Defining # it twice here would blank it, since the later definition wins. diff --git a/app/app/Console/Commands/ProcessSqsMessages.php b/app/app/Console/Commands/ProcessSqsMessages.php index 2d4819a7..39e4620f 100644 --- a/app/app/Console/Commands/ProcessSqsMessages.php +++ b/app/app/Console/Commands/ProcessSqsMessages.php @@ -3,7 +3,9 @@ namespace App\Console\Commands; use App\Models\AwsAccount; +use App\Services\SnsSignatureVerifier; use Illuminate\Console\Command; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Http; use Aws\Sqs\SqsClient; @@ -11,6 +13,24 @@ class ProcessSqsMessages extends Command { + /** + * Stable log field so rejections can be picked out of the stream by name + * rather than by matching on message text that is free to change. + */ + public const REJECTION_EVENT = 'aws_account_link_rejected'; + + /** Cache key prefix for the rejection counters. Read by `aws:link-rejections`. */ + public const REJECTION_CACHE_PREFIX = 'tops:aws:link-rejections:'; + + /** + * Counters live for 30 days from the *first* rejection, not the last — the TTL + * is set by the Cache::add that seeds the key and is not refreshed by the + * increments that follow. So each key is a rolling 30-day window that then + * starts over, which is what an operator wants from an alarm signal: a problem + * fixed two months ago stops showing up as a live number on its own. + */ + public const REJECTION_CACHE_TTL = 60 * 60 * 24 * 30; + /** * The name and signature of the console command. * @@ -136,17 +156,42 @@ private function processMessage(array $message, SqsClient $sqsClient, string $qu return; } - // Handle SNS message format (SNS forwards to SQS) - // The Message field contains the CloudFormation custom resource request (JSON-encoded string) - $cloudFormationMessage = null; - if (isset($body['Type']) && $body['Type'] === 'Notification') { - // SNS notification wrapper - extract the Message field which is JSON-encoded - $cloudFormationMessage = json_decode($body['Message'], true); - } else { - // Direct message (shouldn't happen but handle it) - $cloudFormationMessage = $body; + // N-11: verify the envelope before anything inside it is trusted. + // + // The queue policy already restricts SendMessage to our topic ARN, so this + // is defence in depth rather than the only control — but the HTTP callback + // has verified since N-6 and this path had nothing at all, which made the + // queue the weaker of the two ways into the same account-linking code. + // + // The previous "direct message (shouldn't happen but handle it)" fallback is + // gone deliberately. Nothing legitimate reaches this queue except through the + // SNS subscription, and a body with no envelope has no signature to check — + // keeping it would have left an unsigned path straight past the check below. + if (($body['Type'] ?? null) !== 'Notification') { + $this->recordRejection('not_an_sns_notification', [ + 'type' => $body['Type'] ?? null, + ]); + $this->deleteMessage($sqsClient, $queueUrl, $receiptHandle); + return; + } + + // Resolved from the container so tests can substitute the validator, the + // same way the HTTP callback in AwsAccountsController does. + if (!app(SnsSignatureVerifier::class)->verifyPayload($body)) { + // The verifier logs why. Leave the message on the queue: a verification + // failure can be a transient inability to fetch the signing certificate, + // and redelivery (then the DLQ after maxReceiveCount) is the right shape + // for that. A genuinely forged message simply fails again. + $this->recordRejection('signature_verification_failed', [ + 'topic_arn' => $body['TopicArn'] ?? null, + ]); + return; } + // The Message field contains the CloudFormation custom resource request + // (a JSON-encoded string). + $cloudFormationMessage = json_decode($body['Message'] ?? '', true); + if (!$cloudFormationMessage) { $this->warn('Invalid CloudFormation message format, deleting from queue'); $this->deleteMessage($sqsClient, $queueUrl, $receiptHandle); @@ -263,6 +308,11 @@ private function handleCreateRequest(?string $roleArn, ?string $uniqueId, ?strin { if (!$roleArn || !$externalId || !$uniqueId) { $this->warn('Create message missing required fields (TopsRoleArn, TopsExternalId, TopsUniqueId)'); + $this->recordRejection('missing_required_fields', [ + 'has_role_arn' => !empty($roleArn), + 'has_unique_id' => !empty($uniqueId), + 'has_external_id' => !empty($externalId), + ]); $response = $this->sendCloudFormationResponse($responseUrl, 'FAILED', 'Missing required fields in ResourceProperties', $stackId, $requestId, $logicalResourceId, $physicalResourceId); if (app()->environment(['local', 'dev'])) { @@ -277,6 +327,58 @@ private function handleCreateRequest(?string $roleArn, ?string $uniqueId, ?strin return; } + $awsAccountId = $this->extractAwsAccountIdFromRoleArn($roleArn); + + if (!$awsAccountId) { + $this->error("Could not extract AWS Account ID from Role ARN: {$roleArn}"); + $this->recordRejection('invalid_role_arn', [ + 'unique_id' => $uniqueId, + 'role_arn' => $roleArn, + ]); + $response = $this->sendCloudFormationResponse($responseUrl, 'FAILED', "Invalid Role ARN format: {$roleArn}", $stackId, $requestId, $logicalResourceId, $physicalResourceId); + + if (app()->environment('local')) { + Log::info('AWS account activated via SQS message (dev verbose)', [ + 'unique_id' => $uniqueId, + 'external_id' => $externalId, + 'role_arn' => $roleArn, + 'request_type' => 'Create', + 'response_url' => $responseUrl, + 'response' => $response, + ]); + } + return; + } + + // N-11 gap 1: the message names the child account twice — once in StackId, + // which CloudFormation itself sets, and once inside TopsRoleArn, which is + // whatever the publisher chose to write. Only the first is authoritative. + // Without comparing them, a role ARN pointing at an account the stack-runner + // does not own is accepted and scanned. + $stackAccountId = $this->extractAwsAccountIdFromStackId($stackId); + + if (!$stackAccountId || !hash_equals($stackAccountId, $awsAccountId)) { + $this->error("Role ARN account {$awsAccountId} does not match stack account " . ($stackAccountId ?? 'unknown')); + $this->recordRejection('stack_account_mismatch', [ + 'unique_id' => $uniqueId, + 'role_arn_account_id' => $awsAccountId, + 'stack_account_id' => $stackAccountId, + 'stack_id' => $stackId, + ]); + $response = $this->sendCloudFormationResponse($responseUrl, 'FAILED', 'Role ARN account does not match the account that created the stack', $stackId, $requestId, $logicalResourceId, $physicalResourceId); + + if (app()->environment(['local', 'dev'])) { + Log::info('Create request rejected (stack account mismatch, dev verbose)', [ + 'unique_id' => $uniqueId, + 'external_id' => $externalId, + 'role_arn' => $roleArn, + 'response_url' => $responseUrl, + 'response' => $response, + ]); + } + return; + } + // Find account by unique_id and external_id // Note: unique_id is derived from orgId, so we need both to match $account = AwsAccount::where('unique_id', $uniqueId) @@ -289,10 +391,13 @@ private function handleCreateRequest(?string $roleArn, ?string $uniqueId, ?strin 'external_id' => $externalId, 'request_type' => 'Create', ]); + $this->recordRejection('account_not_found', [ + 'unique_id' => $uniqueId, + ]); $this->warn("Account not found for unique_id: {$uniqueId}, external_id: {$externalId}"); // Send FAILED response to CloudFormation $response = $this->sendCloudFormationResponse($responseUrl, 'FAILED', "Account not found for unique_id: {$uniqueId}", $stackId, $requestId, $logicalResourceId, $physicalResourceId); - + if (app()->environment(['local', 'dev'])) { Log::info('AWS account activated via SQS message (dev verbose)', [ 'unique_id' => $uniqueId, @@ -306,23 +411,38 @@ private function handleCreateRequest(?string $roleArn, ?string $uniqueId, ?strin return; } - $awsAccountId = $this->extractAwsAccountIdFromRoleArn($roleArn); + // N-11 gap 2: only a record still waiting to be linked may be linked. Without + // this, a replayed or forged Create repoints an account that is already live + // at a different role — the credentials the scanner uses are swapped under it. + if ($account->status !== 'pending') { + $this->warn("Account {$account->id} is not pending (status: {$account->status}), refusing to relink"); + $this->recordRejection('account_not_pending', [ + 'account_id' => $account->id, + 'unique_id' => $uniqueId, + 'status' => $account->status, + ]); + $this->sendCloudFormationResponse($responseUrl, 'FAILED', 'Account is not awaiting linking', $stackId, $requestId, $logicalResourceId, $physicalResourceId); + return; + } - if (!$awsAccountId) { - $this->error("Could not extract AWS Account ID from Role ARN: {$roleArn}"); - $response = $this->sendCloudFormationResponse($responseUrl, 'FAILED', "Invalid Role ARN format: {$roleArn}", $stackId, $requestId, $logicalResourceId, $physicalResourceId); - - if (app()->environment('local')) { - Log::info('AWS account activated via SQS message (dev verbose)', [ - 'account_id' => $account->id, - 'unique_id' => $uniqueId, - 'external_id' => $externalId, - 'role_arn' => $roleArn, - 'request_type' => 'Create', - 'response_url' => $responseUrl, - 'response' => $response, - ]); - } + // N-11 gap 3: a pending record stays linkable forever, so an onboarding link + // that was created and abandoned months ago is still a live target. Bound it. + // + // Measured from updated_at, not created_at: the controller reuses one pending + // row per organization and touches it each time it hands out a link, so + // updated_at is when the link the user is holding was actually issued. + // Using created_at would expire links the user had only just been given. + $windowHours = (int) config('services.aws.account_link_window_hours'); + + if ($windowHours > 0 && $account->updated_at->addHours($windowHours)->isPast()) { + $this->warn("Account {$account->id} pending link window expired"); + $this->recordRejection('link_window_expired', [ + 'account_id' => $account->id, + 'unique_id' => $uniqueId, + 'link_issued_at' => $account->updated_at->toIso8601String(), + 'window_hours' => $windowHours, + ]); + $this->sendCloudFormationResponse($responseUrl, 'FAILED', "Linking window of {$windowHours}h has expired — start the connection again from TOPS", $stackId, $requestId, $logicalResourceId, $physicalResourceId); return; } @@ -405,8 +525,29 @@ private function handleUpdateRequest(?string $roleArn, ?string $uniqueId, ?strin if ($currentRoleArn !== $roleArn) { $awsAccountId = $this->extractAwsAccountIdFromRoleArn($roleArn); + $stackAccountId = $this->extractAwsAccountIdFromStackId($stackId); - if ($awsAccountId) { + if (!$awsAccountId) { + $this->warn("Could not extract AWS Account ID from Role ARN: {$roleArn}"); + $this->recordRejection('invalid_role_arn', [ + 'account_id' => $account->id, + 'unique_id' => $uniqueId, + 'role_arn' => $roleArn, + 'request_type' => 'Update', + ]); + } elseif (!$stackAccountId || !hash_equals($stackAccountId, $awsAccountId)) { + // Update repoints a live account at a new role, so it needs the + // same StackId cross-check as Create — otherwise closing the gap + // on Create only moves the forged-ARN path one RequestType over. + $this->warn("Update rejected: role ARN account {$awsAccountId} does not match stack account " . ($stackAccountId ?? 'unknown')); + $this->recordRejection('stack_account_mismatch', [ + 'account_id' => $account->id, + 'unique_id' => $uniqueId, + 'role_arn_account_id' => $awsAccountId, + 'stack_account_id' => $stackAccountId, + 'request_type' => 'Update', + ]); + } else { // Update the account with new IAM Role ARN and AWS Account ID $account->fill([ 'iam_role_arn' => $roleArn, @@ -422,13 +563,6 @@ private function handleUpdateRequest(?string $roleArn, ?string $uniqueId, ?strin 'external_id' => $externalId, 'request_type' => 'Update', ]); - } else { - $this->warn("Could not extract AWS Account ID from Role ARN: {$roleArn}"); - Log::warning('Update request: Invalid Role ARN format', [ - 'role_arn' => $roleArn, - 'unique_id' => $uniqueId, - 'external_id' => $externalId, - ]); } } else { $this->info("IAM Role ARN unchanged for account {$account->id}"); @@ -544,6 +678,57 @@ private function extractAwsAccountIdFromRoleArn(string $roleArn): ?string return null; } + /** + * Pull the account that owns the CloudFormation stack out of its StackId. + * + * StackId is set by CloudFormation, not by the template, so this is the one + * account identifier in the message the publisher did not choose. Everything + * under ResourceProperties is caller-supplied and has to agree with it. + * + * Shape: arn:aws:cloudformation:::stack// + * The partition varies (aws, aws-cn, aws-us-gov), so it is matched loosely. + */ + private function extractAwsAccountIdFromStackId(string $stackId): ?string + { + if (preg_match('#^arn:[a-z0-9-]+:cloudformation:[a-z0-9-]+:(\d{12}):stack/#', $stackId, $matches) === 1) { + return $matches[1]; + } + return null; + } + + /** + * Count a rejected account-linking message under a stable, readable key. + * + * A `Log::warning` alone is not something an operator can alarm on without a + * log pipeline, and a self-hosted install may have none. These counters live in + * the cache store (`database` by default, so they survive a restart) and are + * read back with `php artisan aws:link-rejections`. + * + * Counting is best-effort by design: failing to record a metric must never stop + * a message being rejected, which is the part that actually matters. + */ + private function recordRejection(string $reason, array $context = []): void + { + Log::warning('Account-linking message rejected', [ + 'event' => self::REJECTION_EVENT, + 'reason' => $reason, + ] + $context); + + try { + foreach ([self::REJECTION_CACHE_PREFIX . 'total', self::REJECTION_CACHE_PREFIX . $reason] as $key) { + // The database cache store cannot increment a key that does not + // exist yet, so seed it first. add() is a no-op once it does. + Cache::add($key, 0, self::REJECTION_CACHE_TTL); + Cache::increment($key); + } + } catch (\Throwable $e) { + Log::warning('Could not record link-rejection counter', [ + 'reason' => $reason, + 'error' => $e->getMessage(), + ]); + } + } + /** * Delete message from SQS queue. * Rethrows AwsException so callers can decide whether to retry (message remains visible). diff --git a/app/app/Console/Commands/ShowLinkRejections.php b/app/app/Console/Commands/ShowLinkRejections.php new file mode 100644 index 00000000..3002c204 --- /dev/null +++ b/app/app/Console/Commands/ShowLinkRejections.php @@ -0,0 +1,93 @@ +option('json')) { + $this->line((string) json_encode([ + 'total' => $total, + 'reasons' => $counts, + ], JSON_PRETTY_PRINT)); + } else { + $this->table( + ['Reason', 'Count'], + collect($counts) + ->map(fn (int $count, string $reason) => [$reason, $count]) + ->values() + ->push(['TOTAL', $total]) + ->all() + ); + + if ($total === 0) { + $this->info('No account-linking messages have been rejected.'); + } + } + + if ($this->option('reset')) { + foreach ([...self::REASONS, 'total'] as $key) { + Cache::forget($prefix . $key); + } + $this->info('Counters reset.'); + } + + return Command::SUCCESS; + } +} diff --git a/app/app/Http/Controllers/Api/AwsAccountsController.php b/app/app/Http/Controllers/Api/AwsAccountsController.php index b97b7041..806e6e20 100644 --- a/app/app/Http/Controllers/Api/AwsAccountsController.php +++ b/app/app/Http/Controllers/Api/AwsAccountsController.php @@ -62,6 +62,17 @@ public function init(Request $request, string $orgId): JsonResponse 'status' => 'pending', 'unique_id' => $uniqueId, // Derived from orgId, same for all accounts in this org ]); + } else { + // Re-issuing the link restarts its expiry window (N-11). Without this, + // reuse and expiry combine into a trap: the record is reused from the + // first click, so an org that abandoned onboarding a week ago would get + // a fresh-looking link backed by a week-old row, and every retry would + // be rejected as expired with nothing in the UI to explain it. + // + // updated_at is the timestamp because a pending row is only ever written + // by this method and by the callback that completes it — so on a pending + // account it means precisely "when the link was last handed out". + $account->touch(); } $cloudFormationUrl = $this->buildInitCloudFormationUrl( @@ -96,6 +107,14 @@ private function awsMessagingConfigError(): ?string return 'AWS messaging not configured (missing TOPS_DEPLOYMENT_REGION). Set it in .env and run ./install.sh.'; } + // Without it the child stack's ping is filtered out at the topic and simply + // never arrives — the stack hangs for an hour and rolls back with nothing + // logged here. Refusing to hand out a link that cannot work is the only + // point at which that is still explainable. + if (! config('services.aws.install_id')) { + return 'AWS messaging not configured (missing TOPS_INSTALL_ID). Run ./install.sh --aws-only to generate it.'; + } + return null; } @@ -104,15 +123,17 @@ private function buildInitCloudFormationUrl(string $externalId, string $uniqueId $deploymentRegion = config('services.aws.deployment_region'); $parentAccountId = config('services.aws.parent_account_id'); $templateUrl = config('services.aws.cloudformation_template_url'); + $installId = config('services.aws.install_id'); return sprintf( - 'https://console.aws.amazon.com/cloudformation/home?region=%s#/stacks/quickcreate?templateUrl=%s&stackName=tops-vendor-audit¶m_ParentAWSAccountId=%s¶m_ParentDeploymentRegion=%s¶m_ExternalId=%s¶m_UniqueId=%s', + 'https://console.aws.amazon.com/cloudformation/home?region=%s#/stacks/quickcreate?templateUrl=%s&stackName=tops-vendor-audit¶m_ParentAWSAccountId=%s¶m_ParentDeploymentRegion=%s¶m_ExternalId=%s¶m_UniqueId=%s¶m_TopsInstallId=%s', urlencode($deploymentRegion), urlencode($templateUrl), urlencode($parentAccountId), urlencode($deploymentRegion), urlencode($externalId), urlencode($uniqueId), + urlencode($installId), ); } diff --git a/app/app/Services/SnsSignatureVerifier.php b/app/app/Services/SnsSignatureVerifier.php index 50eea046..7b389f05 100644 --- a/app/app/Services/SnsSignatureVerifier.php +++ b/app/app/Services/SnsSignatureVerifier.php @@ -44,8 +44,20 @@ public function __construct(?MessageValidator $validator = null) public function verify(Request $request): bool { - $payload = $this->payload($request); + return $this->verifyPayload($this->payload($request)); + } + /** + * Verify an SNS envelope that has already been decoded to an array. + * + * The SQS path (N-11, `aws:process-sqs`) reads the same envelope out of a + * queue message body rather than an HTTP request, and needs exactly the two + * checks above it — signature, then topic. Keeping one implementation means + * the queue path cannot drift into being the weaker of the two, which is how + * it came to have no verification at all. + */ + public function verifyPayload(array $payload): bool + { if ($payload === []) { return $this->reject('body was empty or not JSON'); } diff --git a/app/config/services.php b/app/config/services.php index c90ee858..71c8e825 100644 --- a/app/config/services.php +++ b/app/config/services.php @@ -48,6 +48,11 @@ 'parent_account_id' => env('AWS_PARENT_ACCOUNT_ID'), 'cloudformation_template_url' => env('TOPS_CFN_TEMPLATE_URL'), 'deployment_region' => env('TOPS_DEPLOYMENT_REGION', env('AWS_DEFAULT_REGION', 'us-east-1')), + // How long a pending AWS account stays linkable after it is created (N-11). + // A pending record is a live target for anyone who can publish to the SNS + // topic, so the window is bounded; re-connecting from the UI mints a fresh + // one. Set to 0 to disable expiry. + 'account_link_window_hours' => (int) env('TOPS_ACCOUNT_LINK_WINDOW_HOURS', 24), 'sqs_name' => env('TOPS_SQS_NAME'), 'sqs_arn' => env('TOPS_SQS_ARN'), // Dead-letter queue for teemops_main. Messages land here after @@ -55,6 +60,10 @@ 'sqs_dlq_name' => env('TOPS_SQS_DLQ_NAME', env('TOPS_SQS_NAME') ? env('TOPS_SQS_NAME').'_dlq' : null), 'sqs_dlq_arn' => env('TOPS_SQS_DLQ_ARN'), 'sns_arn' => env('TOPS_SNS_ARN'), + // Install-scoped filter secret (N-11). Minted once by the installer and + // written to generated/teemops.env; the parent topic's subscription only + // forwards child-account pings that carry it. + 'install_id' => env('TOPS_INSTALL_ID'), 'audit_sqs_name' => env('TOPS_AUDIT_SQS_NAME', 'teemops_audit'), 'audit_sqs_arn' => env('TOPS_AUDIT_SQS_ARN'), 'audit_region_sqs_name' => env('TOPS_AUDIT_REGION_SQS_NAME', 'teemops_audit_region'), diff --git a/app/tests/Feature/AwsAccountsControllerTest.php b/app/tests/Feature/AwsAccountsControllerTest.php index 496982eb..8598e714 100644 --- a/app/tests/Feature/AwsAccountsControllerTest.php +++ b/app/tests/Feature/AwsAccountsControllerTest.php @@ -23,6 +23,7 @@ private function configureMessaging(string $region = 'us-west-2'): void Config::set('services.aws.parent_account_id', '123456789012'); Config::set('services.aws.cloudformation_template_url', "https://test-123456789012-tops-deploy.s3.{$region}.amazonaws.com/templates/iam.role.child.account.cfn.yaml"); Config::set('services.aws.deployment_region', $region); + Config::set('services.aws.install_id', 'test-install-id-0000'); } public function test_init_returns_503_when_messaging_not_configured(): void @@ -67,6 +68,31 @@ public function test_init_returns_cloudformation_url_with_pinned_region_when_con $this->assertStringContainsString('param_ParentAWSAccountId=123456789012', $url); $this->assertStringContainsString('param_ExternalId=' . urlencode($account->external_id), $url); $this->assertStringContainsString('param_UniqueId=' . urlencode($account->unique_id), $url); + // N-11 phase 2: without it the child stack's ping is filtered out at the + // topic and onboarding hangs for an hour before rolling back. + $this->assertStringContainsString('param_TopsInstallId=test-install-id-0000', $url); + } + + /** + * A link built without the install id would be filtered out at the SNS topic + * and leave no trace anywhere — the child stack just waits for CloudFormation's + * custom-resource timeout and rolls back. Refusing to issue it is the last + * point at which the operator can be told what is actually wrong. + */ + public function test_init_returns_503_when_install_id_is_missing(): void + { + $this->configureMessaging(); + Config::set('services.aws.install_id', null); + + $user = User::factory()->create(); + $organization = Organization::factory()->create(['user_id' => $user->id]); + + $response = $this->actingAs($user) + ->postJson("/api/organizations/{$organization->org_id}/aws-accounts/init"); + + $response->assertStatus(503); + $this->assertStringContainsString('TOPS_INSTALL_ID', $response->json('error')); + $this->assertDatabaseCount('aws_accounts', 0); } public function test_init_reuses_existing_pending_account_instead_of_creating_duplicates(): void @@ -90,6 +116,40 @@ public function test_init_reuses_existing_pending_account_instead_of_creating_du $this->assertSame(1, AwsAccount::where('organization_id', $organization->id)->count()); } + /** + * Reuse and the N-11 link window combine into a trap if the timestamp is not + * restarted: the row is reused from the first click, so an organization that + * abandoned onboarding and comes back later would be handed a fresh-looking + * link that the poller then rejects as expired — permanently, since every + * retry reuses the same stale row. + */ + public function test_init_restarts_the_link_window_when_it_reuses_a_pending_account(): void + { + $this->configureMessaging(); + + $user = User::factory()->create(); + $organization = Organization::factory()->create(['user_id' => $user->id]); + + $account = AwsAccount::factory()->pending()->create([ + 'organization_id' => $organization->id, + 'unique_id' => $organization->org_id, + ]); + AwsAccount::where('id', $account->id)->update([ + 'created_at' => now()->subDays(7), + 'updated_at' => now()->subDays(7), + ]); + + $this->actingAs($user) + ->postJson("/api/organizations/{$organization->org_id}/aws-accounts/init") + ->assertStatus(200) + ->assertJsonPath('accountId', $account->id); + + $this->assertTrue( + $account->refresh()->updated_at->greaterThan(now()->subMinute()), + 'Handing out the link again must restart its expiry window' + ); + } + public function test_init_forbidden_for_member_without_add_permission(): void { $this->configureMessaging(); diff --git a/app/tests/Unit/ProcessSqsMessagesTest.php b/app/tests/Unit/ProcessSqsMessagesTest.php index 98b8c746..4358b1cd 100644 --- a/app/tests/Unit/ProcessSqsMessagesTest.php +++ b/app/tests/Unit/ProcessSqsMessagesTest.php @@ -5,6 +5,8 @@ use App\Console\Commands\ProcessSqsMessages; use App\Models\AwsAccount; use App\Models\Organization; +use App\Services\SnsSignatureVerifier; +use Illuminate\Support\Facades\Cache; use Aws\Sqs\SqsClient; use Aws\Result; use Aws\Exception\AwsException; @@ -30,6 +32,78 @@ protected function setUp(): void Config::set('services.aws.sqs_name', 'test-queue'); Config::set('services.aws.sqs_arn', 'arn:aws:sqs:us-east-1:123456789012:test-queue'); Config::set('services.aws.region', 'us-east-1'); + Config::set('services.aws.account_link_window_hours', 24); + + // Every test below feeds a hand-built envelope with no real AWS signature. + // Default to a verifier that accepts, so each test exercises the check it is + // actually about; the signature tests override this with fakeSnsVerifier(false). + $this->fakeSnsVerifier(true); + } + + /** + * Swap the SNS verifier for one with a fixed answer. + * + * Signature validation is covered on its own in SnsSignatureVerifierTest against + * real AWS-signed fixtures. What matters here is only whether processMessage + * honours the verdict, so a stub is the honest boundary. + */ + private function fakeSnsVerifier(bool $verdict): void + { + $this->app->bind(SnsSignatureVerifier::class, fn () => new class($verdict) extends SnsSignatureVerifier { + public function __construct(private bool $verdict) + { + // Deliberately does not call parent::__construct(): the real + // MessageValidator would reach out to AWS for a signing certificate. + } + + public function verifyPayload(array $payload): bool + { + return $this->verdict; + } + }); + } + + /** + * Build an SNS-wrapped CloudFormation custom-resource message. + * + * Mirrors the shape of the real captured messages in references/samples/: the + * CloudFormation request is a JSON *string* in the envelope's Message field, and + * the account that owns the stack appears in StackId as well as in TopsRoleArn. + */ + private function snsMessageBody(array $cloudFormationMessage): string + { + return json_encode([ + 'Type' => 'Notification', + 'TopicArn' => 'arn:aws:sns:us-east-1:123456789012:teemops-sns', + 'Message' => json_encode($cloudFormationMessage), + ]); + } + + /** + * Run processMessage against one message, asserting it is deleted from the queue. + */ + private function processOne(string $body, string $receiptHandle = 'test-receipt-handle', bool $expectDelete = true): void + { + $command = $this->createCommandWithOutput(); + $method = (new \ReflectionClass($command))->getMethod('processMessage'); + $method->setAccessible(true); + + $sqsClient = Mockery::mock(SqsClient::class); + $queueUrl = 'https://sqs.us-east-1.amazonaws.com/123456789012/test-queue'; + + $sqsClient->shouldReceive('deleteMessage') + ->times($expectDelete ? 1 : 0) + ->andReturn(new Result([])); + + $method->invoke($command, [ + 'ReceiptHandle' => $receiptHandle, + 'Body' => $body, + ], $sqsClient, $queueUrl); + } + + private function rejectionCount(string $reason): int + { + return (int) Cache::get(ProcessSqsMessages::REJECTION_CACHE_PREFIX . $reason, 0); } protected function tearDown(): void @@ -93,7 +167,9 @@ public function test_process_create_request_successfully(): void $cloudFormationMessage = [ 'RequestType' => 'Create', 'ResponseURL' => 'https://cloudformation-custom-resource-response.s3.amazonaws.com/test', - 'StackId' => 'arn:aws:cloudformation:us-west-2:123456789012:stack/test-stack/123', + // Same account as TopsRoleArn below — CloudFormation sets StackId, so a + // legitimate message always agrees with itself here. + 'StackId' => 'arn:aws:cloudformation:us-west-2:660228977852:stack/test-stack/123', 'RequestId' => 'test-request-id-123', 'LogicalResourceId' => 'TopsCustomNotifier', 'PhysicalResourceId' => 'test-physical-resource-id', @@ -249,7 +325,7 @@ public function test_process_create_request_when_account_not_found(): void $cloudFormationMessage = [ 'RequestType' => 'Create', 'ResponseURL' => 'https://cloudformation-custom-resource-response.s3.amazonaws.com/test-not-found', - 'StackId' => 'arn:aws:cloudformation:us-west-2:123456789012:stack/test-stack/789', + 'StackId' => 'arn:aws:cloudformation:us-west-2:660228977852:stack/test-stack/789', 'RequestId' => 'test-request-id-789', 'LogicalResourceId' => 'TopsCustomNotifier', 'PhysicalResourceId' => 'test-physical-resource-id', @@ -486,4 +562,306 @@ public function test_process_create_request_with_invalid_role_arn(): void $account->refresh(); $this->assertEquals('pending', $account->status); } + + // --------------------------------------------------------------------- + // N-11 phase 1 (#100): the message body is not trusted on its face. + // --------------------------------------------------------------------- + + /** + * Build a valid-looking Create message, overridable per test. + */ + private function createMessage(string $uniqueId, string $externalId, array $overrides = []): array + { + return array_replace([ + 'RequestType' => 'Create', + 'ResponseURL' => 'https://cloudformation-custom-resource-response.s3.amazonaws.com/n11', + 'StackId' => 'arn:aws:cloudformation:us-west-2:660228977852:stack/tops-vendor-audit/abc', + 'RequestId' => 'n11-request-id', + 'LogicalResourceId' => 'TopsCustomNotifier', + 'ResourceProperties' => [ + 'TopsRoleArn' => 'arn:aws:iam::660228977852:role/tops-vendor-audit-TeemOps-abc', + 'TopsExternalId' => $externalId, + 'TopsUniqueId' => $uniqueId, + 'TopsType' => 'ops', + ], + ], $overrides); + } + + private function pendingAccount(array $attributes = []): AwsAccount + { + $organization = Organization::factory()->create(); + + return AwsAccount::factory()->pending()->create(array_replace([ + 'organization_id' => $organization->id, + 'unique_id' => $organization->org_id, + 'external_id' => 'n11-external-id', + 'name' => 'Pending AWS Account', + ], $attributes)); + } + + /** + * Gap 1 — the account is never cross-checked. + * + * StackId is set by CloudFormation; TopsRoleArn is set by whoever published the + * message. When they disagree, the role ARN is pointing at an account the + * stack-runner does not own, and linking it would have TOPS assume a role + * chosen by the attacker. + */ + public function test_create_is_rejected_when_role_arn_account_differs_from_stack_account(): void + { + $account = $this->pendingAccount(); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $this->processOne($this->snsMessageBody($this->createMessage( + $account->unique_id, + $account->external_id, + // Stack ran in 111111111111; the role ARN claims 660228977852. + ['StackId' => 'arn:aws:cloudformation:us-west-2:111111111111:stack/tops-vendor-audit/abc'], + ))); + + $account->refresh(); + $this->assertSame('pending', $account->status, 'A mismatched role ARN must not link the account'); + $this->assertNull($account->iam_role_arn); + $this->assertSame(1, $this->rejectionCount('stack_account_mismatch')); + + Http::assertSent(fn ($request) => $request->data()['Status'] === 'FAILED' + && str_contains($request->data()['Reason'], 'does not match the account that created the stack')); + } + + /** + * A StackId that is not a parseable CloudFormation ARN gives us nothing to + * compare against, so it fails closed rather than skipping the check. + */ + public function test_create_is_rejected_when_stack_id_is_unparseable(): void + { + $account = $this->pendingAccount(); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $this->processOne($this->snsMessageBody($this->createMessage( + $account->unique_id, + $account->external_id, + ['StackId' => 'not-an-arn'], + ))); + + $this->assertSame('pending', $account->refresh()->status); + $this->assertSame(1, $this->rejectionCount('stack_account_mismatch')); + } + + /** + * Gap 2 — status is not checked. + * + * Without this, a replayed or forged Create repoints an account that is already + * linked, swapping the credentials the scanner uses out from under it. + */ + public function test_create_is_rejected_when_account_is_not_pending(): void + { + $organization = Organization::factory()->create(); + $account = AwsAccount::factory()->completed()->create([ + 'organization_id' => $organization->id, + 'unique_id' => $organization->org_id, + 'external_id' => 'n11-already-linked', + 'iam_role_arn' => 'arn:aws:iam::999999999999:role/the-real-one', + ]); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $this->processOne($this->snsMessageBody( + $this->createMessage($account->unique_id, $account->external_id) + )); + + $account->refresh(); + $this->assertSame('completed', $account->status); + $this->assertSame( + 'arn:aws:iam::999999999999:role/the-real-one', + $account->iam_role_arn, + 'An already-linked account must not be repointed at a new role' + ); + $this->assertSame(1, $this->rejectionCount('account_not_pending')); + + Http::assertSent(fn ($request) => $request->data()['Status'] === 'FAILED' + && str_contains($request->data()['Reason'], 'not awaiting linking')); + } + + /** + * Gap 3 — the pending window never expires. + */ + public function test_create_is_rejected_when_pending_link_window_has_expired(): void + { + $account = $this->pendingAccount(['updated_at' => now()->subHours(25)]); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $this->processOne($this->snsMessageBody( + $this->createMessage($account->unique_id, $account->external_id) + )); + + $this->assertSame('pending', $account->refresh()->status); + $this->assertSame(1, $this->rejectionCount('link_window_expired')); + + Http::assertSent(fn ($request) => $request->data()['Status'] === 'FAILED' + && str_contains($request->data()['Reason'], 'has expired')); + } + + /** + * The expiry is measured from when the link was last issued, not when the row + * was first created. The controller keeps one pending row per organization and + * reuses it, so an org that started onboarding last week and comes back today + * must not be handed a link that is already expired. + */ + public function test_a_reissued_link_is_not_expired_by_the_original_created_at(): void + { + $account = $this->pendingAccount([ + 'created_at' => now()->subDays(7), + 'updated_at' => now()->subMinutes(5), + ]); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $this->processOne($this->snsMessageBody( + $this->createMessage($account->unique_id, $account->external_id) + )); + + $this->assertSame('completed', $account->refresh()->status); + $this->assertSame(0, $this->rejectionCount('link_window_expired')); + } + + public function test_create_is_accepted_inside_the_link_window(): void + { + $account = $this->pendingAccount(['updated_at' => now()->subHours(23)]); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $this->processOne($this->snsMessageBody( + $this->createMessage($account->unique_id, $account->external_id) + )); + + $this->assertSame('completed', $account->refresh()->status); + $this->assertSame(0, $this->rejectionCount('link_window_expired')); + } + + /** + * An operator whose approval process runs longer than the default can turn the + * window off, so the setting has to actually be honoured at 0. + */ + public function test_link_window_of_zero_disables_expiry(): void + { + Config::set('services.aws.account_link_window_hours', 0); + + $account = $this->pendingAccount(['updated_at' => now()->subYear()]); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $this->processOne($this->snsMessageBody( + $this->createMessage($account->unique_id, $account->external_id) + )); + + $this->assertSame('completed', $account->refresh()->status); + } + + /** + * Gap 4 — the SNS signature is not verified on the SQS path. + * + * The message stays on the queue: a verification failure can be a transient + * inability to fetch the signing certificate, and SQS redelivery (then the DLQ) + * is the right shape for that. A forged message simply fails again. + */ + public function test_message_with_an_invalid_signature_is_rejected_and_left_on_the_queue(): void + { + $this->fakeSnsVerifier(false); + $account = $this->pendingAccount(); + + Http::fake(); + + $this->processOne( + $this->snsMessageBody($this->createMessage($account->unique_id, $account->external_id)), + expectDelete: false, + ); + + $this->assertSame('pending', $account->refresh()->status); + $this->assertSame(1, $this->rejectionCount('signature_verification_failed')); + Http::assertNothingSent(); + } + + /** + * A body with no SNS envelope has no signature to check. The old code treated + * that as a "direct message" and processed it anyway, which was a way straight + * past verification. + */ + public function test_message_without_an_sns_envelope_is_rejected(): void + { + $account = $this->pendingAccount(); + + Http::fake(); + + // The raw CloudFormation request, unwrapped. + $this->processOne((string) json_encode( + $this->createMessage($account->unique_id, $account->external_id) + )); + + $this->assertSame('pending', $account->refresh()->status); + $this->assertSame(1, $this->rejectionCount('not_an_sns_notification')); + Http::assertNothingSent(); + } + + /** + * The rejection counter is the part an operator can alarm on, so it has to + * survive being read back — a Log::warning alone was the gap. + */ + public function test_rejections_increment_a_readable_counter(): void + { + $account = $this->pendingAccount(); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $mismatched = $this->snsMessageBody($this->createMessage( + $account->unique_id, + $account->external_id, + ['StackId' => 'arn:aws:cloudformation:us-west-2:111111111111:stack/tops-vendor-audit/abc'], + )); + + $this->processOne($mismatched, 'handle-1'); + $this->processOne($mismatched, 'handle-2'); + + $this->assertSame(2, $this->rejectionCount('stack_account_mismatch')); + $this->assertSame(2, $this->rejectionCount('total')); + + $this->artisan('aws:link-rejections') + ->expectsOutputToContain('stack_account_mismatch') + ->assertSuccessful(); + } + + /** + * Update repoints a live account at a new role, so closing the gap on Create + * alone would only move the forged-ARN path one RequestType over. + */ + public function test_update_is_rejected_when_role_arn_account_differs_from_stack_account(): void + { + $organization = Organization::factory()->create(); + $account = AwsAccount::factory()->completed()->create([ + 'organization_id' => $organization->id, + 'unique_id' => $organization->org_id, + 'external_id' => 'n11-update', + 'iam_role_arn' => 'arn:aws:iam::999999999999:role/the-real-one', + ]); + + Http::fake(['https://cloudformation-custom-resource-response.s3.amazonaws.com/*' => Http::response('', 200)]); + + $this->processOne($this->snsMessageBody($this->createMessage( + $account->unique_id, + $account->external_id, + [ + 'RequestType' => 'Update', + 'StackId' => 'arn:aws:cloudformation:us-west-2:111111111111:stack/tops-vendor-audit/abc', + ], + ))); + + $this->assertSame( + 'arn:aws:iam::999999999999:role/the-real-one', + $account->refresh()->iam_role_arn, + 'A mismatched Update must not repoint the role' + ); + $this->assertSame(1, $this->rejectionCount('stack_account_mismatch')); + } } diff --git a/docker/app/entrypoint.sh b/docker/app/entrypoint.sh index a2599789..e9b0b386 100644 --- a/docker/app/entrypoint.sh +++ b/docker/app/entrypoint.sh @@ -52,6 +52,14 @@ sync_env_from_compose AWS_PARENT_ACCOUNT_ID sync_env_from_compose TOPS_CFN_TEMPLATE_URL sync_env_from_compose TOPS_SQS_NAME sync_env_from_compose TOPS_SQS_ARN +# The web tier builds the onboarding quick-create URL, and the install id is one of +# its parameters. Without it here, php-fpm hands out links whose ping the SNS topic +# filters out — the child stack then hangs for an hour and rolls back, and nothing +# is logged on our side because the message never arrives (N-11). +sync_env_from_compose TOPS_INSTALL_ID +# Read by SnsSignatureVerifier, which fails closed when it cannot tell whose topic +# a message came from. The legacy HTTP callback route lives in this tier. +sync_env_from_compose TOPS_SNS_ARN # Queue connections come from generated/teemops.env (env_file). Bake them into the # container .env too so the web tier (php-fpm) resolves them the same as CLI, # regardless of php-fpm's environment handling. Scans use the database queue; diff --git a/docker/installer/scripts/install-messaging.sh b/docker/installer/scripts/install-messaging.sh index 22267dcc..6919786f 100755 --- a/docker/installer/scripts/install-messaging.sh +++ b/docker/installer/scripts/install-messaging.sh @@ -119,6 +119,62 @@ deploy_core_docker() { export CORE_STACK="$stack_name" } +# Read one KEY's value back out of a previously written generated/teemops.env. +# +# Deliberately not `source` — same reasoning as load_dotenv above. +env_file_value() { + local key="$1" line + [[ -f "$ENV_FILE" ]] || return 0 + + while IFS= read -r line || [[ -n "$line" ]]; do + if [[ "$line" =~ ^[[:space:]]*${key}=(.*)$ ]]; then + printf '%s' "${BASH_REMATCH[1]}" + return 0 + fi + done < "$ENV_FILE" +} + +# The install-scoped filter secret (N-11). +# +# This must NEVER be regenerated once an installation has one. Minting a fresh +# GUID on a re-run of `install.sh --aws-only` would invalidate every onboarding +# link already issued *and* silently filter out the Delete ping from every account +# already linked — so unlinking would stop working with no error anywhere. That is +# the single worst thing this script could do, hence the explicit precedence: +# +# 1. TOPS_INSTALL_ID set in .env or the environment — the manual rotation path. +# 2. Whatever the last successful install wrote to generated/teemops.env. +# 3. Only then, a new one. +# +# generated/teemops.env is rewritten wholesale by write_env_file, so step 2 has to +# happen before that — and generated/ must be part of any backup for step 2 to +# survive a restore. +resolve_install_id() { + if [[ -n "${TOPS_INSTALL_ID:-}" ]]; then + log "Using TOPS_INSTALL_ID from the environment." + elif TOPS_INSTALL_ID="$(env_file_value TOPS_INSTALL_ID)" && [[ -n "$TOPS_INSTALL_ID" ]]; then + log "Reusing the existing install id from ${ENV_FILE} — onboarding links stay valid." + else + TOPS_INSTALL_ID="$(python3 -c 'import uuid; print(uuid.uuid4())')" + log "Minted a new install id for this installation." + fi + + export TOPS_INSTALL_ID + + # During a rotation, set TOPS_INSTALL_ID to the new value and + # TOPS_INSTALL_ID_PREVIOUS to the old one. Both are accepted by the filter + # policy until the next install drops the previous one, so onboarding that is + # already in flight is not broken by the rotation. + if [[ -n "${TOPS_INSTALL_ID_PREVIOUS:-}" ]]; then + TOPS_INSTALL_IDS="${TOPS_INSTALL_ID},${TOPS_INSTALL_ID_PREVIOUS}" + log "Rotation in progress: the previous install id is still accepted." + else + TOPS_INSTALL_IDS="${TOPS_INSTALL_ID}" + fi + + export TOPS_INSTALL_IDS +} + deploy_sns() { local environment="${TOPS_ENVIRONMENT:-test}" local stack_name="${TOPS_SNS_STACK:-teemops-messaging-${environment}}" @@ -135,6 +191,7 @@ deploy_sns() { --no-fail-on-empty-changeset \ --parameter-overrides \ "SQSLabel=teemops_main" \ + "TopsInstallIds=${TOPS_INSTALL_IDS}" \ 2>&1 | tee -a "$LOG_FILE" export SNS_STACK="$stack_name" @@ -160,7 +217,7 @@ upload_child_account_template() { write_env_file() { local environment="${TOPS_ENVIRONMENT:-test}" - local main_name main_arn main_dlq_name main_dlq_arn audit_name audit_arn region_name region_arn bucket_name sns_arn + local main_name main_arn main_dlq_name main_dlq_arn audit_name audit_arn region_name region_arn bucket_name sns_arn quarantine_name quarantine_arn main_name="$(cf_output "$CORE_STACK" TopsMainQueueName)" main_arn="$(cf_output "$CORE_STACK" TopsMainQueueArn)" @@ -172,8 +229,10 @@ write_env_file() { region_arn="$(cf_output "$CORE_STACK" TopsAuditRegionQueueArn)" bucket_name="$(cf_output "$CORE_STACK" DeploymentBucketName)" sns_arn="$(cf_output "$SNS_STACK" TopicArn)" + quarantine_name="$(cf_output "$SNS_STACK" QuarantineQueueName)" + quarantine_arn="$(cf_output "$SNS_STACK" QuarantineQueueArn)" - for var_name in main_name main_arn main_dlq_name main_dlq_arn audit_name audit_arn region_name region_arn bucket_name sns_arn; do + for var_name in main_name main_arn main_dlq_name main_dlq_arn audit_name audit_arn region_name region_arn bucket_name sns_arn quarantine_name quarantine_arn; do if [[ -z "${!var_name}" || "${!var_name}" == "None" ]]; then die "Missing CloudFormation output: ${var_name}" fi @@ -204,6 +263,24 @@ TOPS_AUDIT_REGION_SQS_ARN=${region_arn} TOPS_SNS_ARN=${sns_arn} TOPS_CFN_TEMPLATE_URL=${TOPS_CFN_TEMPLATE_URL} +# Install-scoped filter secret (N-11). Minted once, on the first AWS install, and +# reused on every re-run — see resolve_install_id in install-messaging.sh. +# +# BACK THIS UP. It is not recoverable from AWS: if generated/ is lost and this +# value is regenerated, every onboarding link already issued stops working, and +# the Delete ping from every account already linked is silently filtered out, so +# unlinking fails with no error. To rotate deliberately, set TOPS_INSTALL_ID to +# the new value and TOPS_INSTALL_ID_PREVIOUS to this one in .env, then re-run +# ./install.sh --aws-only. +TOPS_INSTALL_ID=${TOPS_INSTALL_ID} + +# Holds account-link pings the install-id filter rejected, so a wrong or missing +# id is visible instead of vanishing. Should normally be empty: +# aws sqs get-queue-attributes --queue-name ${quarantine_name} \\ +# --attribute-names ApproximateNumberOfMessages +TOPS_QUARANTINE_SQS_NAME=${quarantine_name} +TOPS_QUARANTINE_SQS_ARN=${quarantine_arn} + # Scan processing runs on the local database queue for the single-server Docker # deployment: the app (php-fpm) needs no AWS credentials to enqueue a scan, and # the worker consumes these queues on the 'database' connection. SQS is used only @@ -224,6 +301,9 @@ main() { load_dotenv require_region validate_aws_auth + # Before deploy_sns (it needs the id) and before write_env_file (which rewrites + # the file the previous id is read back from). + resolve_install_id deploy_core_docker deploy_sns write_env_file diff --git a/docs/features/sns-topic-publish-authorization.md b/docs/features/sns-topic-publish-authorization.md index e1e72000..cfd5aac3 100644 --- a/docs/features/sns-topic-publish-authorization.md +++ b/docs/features/sns-topic-publish-authorization.md @@ -1,21 +1,54 @@ # Locking down the account-linking SNS topic -> ## 🔴 HIGH PRIORITY — security review, not yet built +> ## 🟠 BUILT — both phases, awaiting live AWS verification > -> The parent account's `teemops-sns` topic accepts `sns:Publish` from **any AWS principal +> The parent account's `teemops-sns` topic accepted `sns:Publish` from **any AWS principal > on the internet**. This is the one inbound path into a TOPS install, so it is the first -> thing a security reviewer probes. Nothing here is implemented yet; this document records -> the research, the options, and the reasoning so the decision is made once. +> thing a security reviewer probes. Both phases below are now implemented; the research, +> the options and the reasoning are kept so the decision is not remade. +> +> **The topic policy itself is unchanged, and deliberately so** — see +> [Research findings](#research-findings) for why `sns:Publish` cannot be narrowed there. +> Narrowing happens on the subscription and in the consumer. + +Status: **implemented 2026-08-02, one criterion outstanding.** Everything below is covered by +automated tests except the two criteria that require a real AWS account — payload-based +filtering has not been proven against a live CloudFormation custom-resource message. That +matters more than it sounds: **a filter that matches nothing is indistinguishable from a +working integration**, so this is not "done" until an account links end to end. See +[Verifying against a real account](#verifying-against-a-real-account). -Status: **awaiting agreement.** Raised 2026-08-02 while producing the AWS integration -architecture diagram for CISO/CTO audiences — the diagram describes SNS as "the one inbound -path into TOPS", which invites exactly this question. +Raised 2026-08-02 while producing the AWS integration architecture diagram for CISO/CTO +audiences — the diagram describes SNS as "the one inbound path into TOPS", which invites +exactly this question. Tracked as **N-11** in [the roadmap](../roadmap.md#n-11--lock-down-the-account-linking-sns-topic), split across two issues: [#100 — consumer-side validation](https://github.com/teemops/tops/issues/100) (phase 1) and [#101 — install-scoped filter secret](https://github.com/teemops/tops/issues/101) (phase 2). +### What shipped, and where + +| Phase | Change | File | +| --- | --- | --- | +| 1 | `StackId` account cross-checked against `TopsRoleArn`, on `Create` **and** `Update` | `app/app/Console/Commands/ProcessSqsMessages.php` | +| 1 | Only `pending` records are linkable; the pending window expires (`TOPS_ACCOUNT_LINK_WINDOW_HOURS`, default 24h, `0` disables) | same | +| 1 | SNS signature verified on the SQS path; the unsigned "direct message" fallback is deleted | same, plus `SnsSignatureVerifier::verifyPayload()` | +| 1 | Alarmable rejection counters, readable with `php artisan aws:link-rejections` | `app/app/Console/Commands/ShowLinkRejections.php` | +| 2 | `TopsInstallId` filter policy on the subscription, plus a quarantine subscription for what it drops | `infra/cloud-stack/stackset/sns.topic.cfn.yaml` | +| 2 | Install id minted once, never regenerated, persisted to `generated/teemops.env` | `docker/installer/scripts/install-messaging.sh` | +| 2 | `NoEcho` parameter threaded through both child templates and the quick-create URL | `templates/*.cfn.yaml`, `AwsAccountsController` | +| 2 | The dead `sns:MessageAttributes.*` condition deleted, with the reasoning left in its place | `sns.topic.cfn.yaml` | + +**One trap found while building, worth recording.** `init` keeps a single pending row per +organization and reuses it, so measuring the link window from `created_at` would have made +expiry permanent: an organization that abandoned onboarding and came back would get a +fresh-looking link backed by a week-old row, and every retry would reuse that same row and be +rejected. The window is therefore measured from `updated_at`, and issuing a link touches the +record — on a pending row nothing else writes to it, so `updated_at` means exactly "when the +link the user is holding was handed out". This is the failure mode the feature was most likely +to ship with, because both halves are individually correct. + ## The exposure [`infra/cloud-stack/stackset/sns.topic.cfn.yaml`](../../infra/cloud-stack/stackset/sns.topic.cfn.yaml) @@ -266,33 +299,75 @@ Skip the account allowlist unless cross-org linking becomes a requirement — th ## User acceptance criteria -**Consumer-side hardening (phase 1)** +**Consumer-side hardening (phase 1)** — all covered in `app/tests/Unit/ProcessSqsMessagesTest.php` -- [ ] Given a `Create` message whose `TopsRoleArn` account differs from the account in +- [x] Given a `Create` message whose `TopsRoleArn` account differs from the account in `StackId`, when it is processed, then it is rejected and logged — both values are already in the message and neither is checked today -- [ ] Given a `Create` message matching an account whose status is not `pending`, when it is +- [x] Given a `Create` message matching an account whose status is not `pending`, when it is processed, then it is rejected rather than repointing an existing link -- [ ] Given a pending record older than the configured link window, when a matching `Create` +- [x] Given a pending record older than the configured link window, when a matching `Create` arrives, then it is rejected as expired -- [ ] Given any message failing to match a record, when it is processed, then a counter is +- [x] Given any message failing to match a record, when it is processed, then a counter is incremented that an operator can alarm on — not only a `Log::warning` -- [ ] Given an SNS envelope with an invalid signature, when the poller reads it, then it is +- [x] Given an SNS envelope with an invalid signature, when the poller reads it, then it is rejected, for parity with `SnsSignatureVerifier` on the HTTP path +Two beyond the original list, because closing the gaps as written would have left the same +hole one step away: + +- [x] The `StackId` cross-check applies to `Update` as well as `Create` — `Update` repoints a + *live* account's role, so leaving it out would have moved the forged-ARN path rather + than closed it +- [x] A queue message with no SNS envelope is rejected rather than processed as a "direct + message" — that fallback was an unsigned path straight past the signature check + **Install-scoped filter secret (phase 2)** -- [ ] Given a fresh install, when the AWS step runs, then a `TopsInstallId` GUID is generated, +- [x] Given a fresh install, when the AWS step runs, then a `TopsInstallId` GUID is generated, persisted to `generated/teemops.env`, and used in the subscription filter policy -- [ ] Given an install that already has a `TopsInstallId`, when `install.sh --aws-only` is +- [x] Given an install that already has a `TopsInstallId`, when `install.sh --aws-only` is re-run, then the existing value is reused and no onboarding link is invalidated -- [ ] Given a quick-create URL, when an admin opens it, then the install id is present as a +- [x] Given a quick-create URL, when an admin opens it, then the install id is present as a `NoEcho` parameter and reaches TOPS in the message body -- [ ] Given a message carrying a wrong or absent install id, when it is published, then it is - delivered to the quarantine queue and does not reach `teemops_main` -- [ ] Given a message carrying a *correct* install id, when it is published, then linking - completes exactly as it does today — proven end to end against a real AWS account, not - a synthetic message +- [x] Given the filter policy, when it is written, then it accepts a list of ids so the value + can be rotated without breaking in-flight onboarding +- [ ] **Not verified.** Given a message carrying a wrong or absent install id, when it is + published, then it is delivered to the quarantine queue and does not reach `teemops_main` +- [ ] **Not verified.** Given a message carrying a *correct* install id, when it is published, + then linking completes exactly as it does today — proven end to end against a real AWS + account, not a synthetic message + +## Verifying against a real account + +The two unticked criteria above cannot be closed from a test suite, because what is unproven +is AWS's own behaviour: **payload-based filtering has not been run against a real +CloudFormation custom-resource message.** CloudFormation publishes the request as a JSON +string, which should parse, but "should" is doing real work in that sentence. + +What has been checked without an AWS account: + +- All three templates pass `aws cloudformation validate-template`, so `!Ref TopsInstallIds` + is accepted inside the `FilterPolicy` and the `NoEcho` parameters resolve. +- The filter policy is built from the shape of the captured messages in `references/samples/`, + which is where `ResourceType` and the nesting of `ResourceProperties` come from. + +What to do on one real account, in order: + +1. `./install.sh --aws-only`, then confirm `generated/teemops.env` has a `TOPS_INSTALL_ID`. +2. Re-run `./install.sh --aws-only` and confirm the value is **unchanged**. This is the one + that silently destroys an install if it regresses. +3. Link an account through the UI. It should complete exactly as before. +4. Check the quarantine queue is empty: + `aws sqs get-queue-attributes --queue-url --attribute-names ApproximateNumberOfMessages` +5. Deliberately break it: edit the quick-create URL's `param_TopsInstallId` to a wrong GUID + and run the stack. The ping should land in **quarantine**, not `teemops_main`, and the + child stack should fail rather than hang. +6. `php artisan aws:link-rejections` to confirm nothing unexpected was rejected along the way. + +If step 3 hangs instead of completing, the filter is matching nothing — watch +`NumberOfNotificationsFilteredOut-InvalidMessageBody` on the topic, which distinguishes "the +body did not parse" from "the body parsed and did not match". ## Adjacent findings diff --git a/docs/roadmap.md b/docs/roadmap.md index 3db3cb8c..0f2a4a3b 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -409,7 +409,7 @@ fixes are what made that run succeed, which is the sequencing argument justifyin | # | Feature | Why it's here | Size | | --- | --- | --- | :---: | -| **N-11** | **Lock down the account-linking SNS topic** | 🔴 **Security, open.** `teemops-sns` accepts `sns:Publish` from any AWS principal on the internet — the one inbound path into a TOPS install, and the first thing a reviewer probes. Two phases: consumer-side validation ([#100](https://github.com/teemops/tops/issues/100)), then an install-scoped filter secret ([#101](https://github.com/teemops/tops/issues/101)). Research, options and acceptance criteria in [the feature doc](features/sns-topic-publish-authorization.md). | S + M | +| **N-11** | **Lock down the account-linking SNS topic** | 🟠 **Built 2026-08-02, awaiting live verification.** Both phases implemented: consumer-side validation ([#100](https://github.com/teemops/tops/issues/100)) and an install-scoped filter secret ([#101](https://github.com/teemops/tops/issues/101)). Everything is covered by tests except the one thing a test cannot cover — that SNS payload filtering matches a real CloudFormation message. Until an account links end to end, a filter matching nothing looks exactly like a working integration. [Feature doc](features/sns-topic-publish-authorization.md#verifying-against-a-real-account). | S + M | | ~~**N-1**~~ | ~~Fix the clean-checkout build~~ | ✅ **Done** 2026-07-29 — `@vitejs/plugin-vue` on `^6`, `npm ci` clean, frontend CI job added. | — | | ~~**N-2**~~ | ~~Choose and add a licence~~ | ✅ **Done** 2026-07-29 — Apache-2.0, trademark held separately, DCO for contributions. See D-7. | — | | ~~**N-6**~~ | ~~SNS signature verification~~ | ✅ **Done** 2026-07-30 — real signature verification via AWS's validator package, plus a topic allowlist that fails closed. Promoted from X-2 that morning when going public expired its deferral. | — | @@ -629,6 +629,20 @@ authentication boundary — the value is shared with every account admin onboard removes internet-wide unauthenticated access without putting AWS credentials back into the web tier or capping the product at SNS's 200-principal quota. +**Both shipped on 2026-08-02**, ahead of the original sequencing, because phase 1 turned out +to be the load-bearing half: the topic policy is unchanged and cannot be narrowed, so what +actually validates a link request is the consumer. Two checks were added beyond the written +acceptance criteria — the `StackId` cross-check on `Update` as well as `Create`, and the +removal of the unsigned "direct message" fallback — because closing the gaps exactly as +specified would have left the same hole one step to the side. + +**What is still open is a verification, not a build.** SNS payload filtering has not been run +against a real CloudFormation custom-resource message, and a filter that matches nothing is +indistinguishable from a working integration. The steps are written down in +[the feature doc](features/sns-topic-publish-authorization.md#verifying-against-a-real-account); +the riskiest one is re-running `install.sh --aws-only` and confirming the install id does +*not* change, because regenerating it silently breaks every existing link. + Full research, the four options considered and why three were rejected, the open issues on the proposed design, and acceptance criteria for both phases: **[docs/features/sns-topic-publish-authorization.md](features/sns-topic-publish-authorization.md)**. diff --git a/infra/cloud-stack/stackset/sns.topic.cfn.yaml b/infra/cloud-stack/stackset/sns.topic.cfn.yaml index edf38c5d..1dd9e3cf 100644 --- a/infra/cloud-stack/stackset/sns.topic.cfn.yaml +++ b/infra/cloud-stack/stackset/sns.topic.cfn.yaml @@ -5,6 +5,22 @@ Parameters: Type: String Description: 'Teemops SQS Label' Default: 'teemops_main' + TopsInstallIds: + Type: CommaDelimitedList + NoEcho: true + Description: >- + Install-scoped filter secret (N-11). One GUID minted once per installation, + or "current,previous" while rotating — accepting a list from day one is what + makes rotation possible without breaking onboarding that is already in flight. + Child stacks echo it back in the custom-resource body, and the subscription + below only forwards messages that carry a value from this list. + + This is a speed bump, not an authentication boundary. It is shared with every + account admin who is onboarded, travels in a URL query string, and lands as a + CloudFormation parameter readable by anyone with cloudformation:DescribeStacks + in the child account. It raises the bar from "anyone on the internet" to + "anyone who has ever been given an onboarding link" — the consumer-side checks + in aws:process-sqs (N-11 phase 1) are what actually validate a link request. Resources: TopsSNS: Type: AWS::SNS::Topic @@ -65,15 +81,29 @@ Resources: Condition: StringEquals: 'AWS:SourceOwner': !Ref "AWS::AccountId" + # Any AWS account being onboarded has to be able to publish here — that is + # the whole inbound path, and the publisher is a CloudFormation stack in an + # account we have never seen before, so there is no principal to name. + # + # This cannot be narrowed in the topic policy itself. sns:Publish supports + # no message-content condition keys (SNS defines only sns:Endpoint and + # sns:Protocol, both Subscribe-only), and CloudFormation's custom-resource + # publish carries no message attributes at all — every captured sample in + # references/samples/ puts the whole request in the body. A condition on + # sns:MessageAttributes.* used to sit here commented out; it was dead on + # arrival twice over and has been deleted rather than revived. + # + # Narrowing happens on the subscription instead (TopsSubscriber, below) and + # in the consumer (aws:process-sqs). Note what is *not* granted here: the + # sibling statement above is scoped by AWS:SourceOwner, so nobody outside + # this account can subscribe, delete the topic, or rewrite this policy. + # See docs/features/sns-topic-publish-authorization.md. - Sid: allow-all-aws-users Effect: Allow Principal: AWS: "*" Action: sns:Publish Resource: "*" - # Condition: - # StringEquals: - # 'sns:MessageAttributes.TopsRawTopsRootAccountHash': 'teemops-123' Topics: - !Ref TopsSNS TopsSubscriber: @@ -81,6 +111,19 @@ Resources: Properties: TopicArn: !Ref TopsSNS Protocol: sqs + # Payload-based filtering: CloudFormation publishes the custom-resource + # request as a JSON string in the message body, so there is nothing to match + # on in message attributes. + # + # Matching ResourceType alone would be worthless — the child template is + # served publicly, so its shape is public. TopsInstallId is what turns this + # from a shape check into a shared-secret check. + FilterPolicyScope: MessageBody + FilterPolicy: + ResourceType: + - "Custom::TeemopsPingSNS" + ResourceProperties: + TopsInstallId: !Ref TopsInstallIds Endpoint: Fn::Join: - '' @@ -92,8 +135,73 @@ Resources: - !Ref 'AWS::AccountId' - ':' - !Ref SQSLabel - + + # A filter that matches nothing looks exactly like a working integration, and a + # filtered-out message leaves no trace at all: the child stack just hangs until + # CloudFormation's custom-resource timeout (~1 hour) and rolls back with an + # unactionable error, while TOPS logs nothing because nothing arrived. + # + # That silent-drop failure — a typo, a stale bookmarked onboarding link, a + # reinstall, a restore from backup — is a likelier problem than an attacker. So + # everything the filter above rejects is kept here instead of vanishing. + # Inspect it with: aws sqs receive-message --queue-url + TopsQuarantineSQS: + Type: AWS::SQS::Queue + Properties: + QueueName: teemops_quarantine + VisibilityTimeout: 300 + # Two weeks: long enough that a message dropped on a Friday is still there + # when somebody investigates a failed onboarding the following week. + MessageRetentionPeriod: 1209600 + + TopsQuarantineSQSPolicy: + Type: AWS::SQS::QueuePolicy + Properties: + Queues: + - !Ref TopsQuarantineSQS + PolicyDocument: + Version: "2012-10-17" + Statement: + - Sid: allow-sns-quarantine + Effect: Allow + Principal: + Service: sns.amazonaws.com + Action: SQS:SendMessage + Resource: !GetAtt TopsQuarantineSQS.Arn + Condition: + ArnEquals: + "aws:SourceArn": !Ref TopsSNS + + TopsQuarantineSubscriber: + Type: AWS::SNS::Subscription + Properties: + TopicArn: !Ref TopsSNS + Protocol: sqs + FilterPolicyScope: MessageBody + # The inverse of TopsSubscriber: a TOPS ping whose install id is either + # absent or not one of ours. The two conditions are a list because SNS + # treats multiple conditions on one key as OR — "exists: false" alone would + # miss a wrong id, and "anything-but" alone would miss a missing one. + # + # ResourceType is still required, so unrelated traffic on this topic (the + # EC2 state-change events from TopsCWRule) is dropped rather than filling + # this queue. + FilterPolicy: + ResourceType: + - "Custom::TeemopsPingSNS" + ResourceProperties: + TopsInstallId: + - exists: false + - anything-but: !Ref TopsInstallIds + Endpoint: !GetAtt TopsQuarantineSQS.Arn + Outputs: TopicArn: Description: Teemops SNS Topic - Value: !Ref TopsSNS \ No newline at end of file + Value: !Ref TopsSNS + QuarantineQueueName: + Description: Queue holding account-link pings rejected by the install-id filter (N-11) + Value: !GetAtt TopsQuarantineSQS.QueueName + QuarantineQueueArn: + Description: ARN of teemops_quarantine + Value: !GetAtt TopsQuarantineSQS.Arn \ No newline at end of file diff --git a/templates/iam.role.audit.account.cfn.yaml b/templates/iam.role.audit.account.cfn.yaml index 32ed0e38..9ede72a7 100644 --- a/templates/iam.role.audit.account.cfn.yaml +++ b/templates/iam.role.audit.account.cfn.yaml @@ -22,6 +22,16 @@ Parameters: AllowedPattern: "[a-zA-Z0-9-]*" MinLength: 10 MaxLength: 200 + TopsInstallId: + Type: String + NoEcho: true + Description: >- + Identifies the TOPS installation you are connecting to DO NOT CHANGE. The + parent SNS subscription only accepts pings carrying this value, so a wrong or + missing one means this stack will sit waiting and then roll back. + AllowedPattern: "[a-zA-Z0-9-]*" + MinLength: 10 + MaxLength: 200 Resources: TopsCustomNotifier: Type: Custom::TeemopsPingSNS @@ -41,6 +51,9 @@ Resources: TopsUniqueId: !Ref UniqueId TopsType: "audit" TopsVersion: "1.1" + # Lands in the custom-resource message body, where the parent topic's + # subscription filter matches on it (N-11). + TopsInstallId: !Ref TopsInstallId TopsCWEventRole: Type: "AWS::IAM::Role" Properties: diff --git a/templates/iam.role.child.account.cfn.yaml b/templates/iam.role.child.account.cfn.yaml index bdad0bfb..a4317c6b 100644 --- a/templates/iam.role.child.account.cfn.yaml +++ b/templates/iam.role.child.account.cfn.yaml @@ -25,12 +25,16 @@ Parameters: AllowedPattern: "[a-zA-Z0-9-]*" MinLength: 10 MaxLength: 200 - # RootAccountHash: - # Type: String - # Description: "Root Tops Account Hash DO NOT CHANGE. Used for SNS topic sending to root account." - # AllowedPattern: "[a-zA-Z0-9-]*" - # MinLength: 10 - # MaxLength: 200 + TopsInstallId: + Type: String + NoEcho: true + Description: >- + Identifies the TOPS installation you are connecting to DO NOT CHANGE. The + parent SNS subscription only accepts pings carrying this value, so a wrong or + missing one means this stack will sit waiting and then roll back. + AllowedPattern: "[a-zA-Z0-9-]*" + MinLength: 10 + MaxLength: 200 Resources: TopsCustomNotifier: Type: Custom::TeemopsPingSNS @@ -49,7 +53,9 @@ Resources: TopsExternalId: !Ref ExternalId TopsUniqueId: !Ref UniqueId TopsType: "ops" - # TopsRootAccountHash: !Ref RootAccountHash + # Lands in the custom-resource message body, where the parent topic's + # subscription filter matches on it (N-11). + TopsInstallId: !Ref TopsInstallId TopsCWEventRole: Type: "AWS::IAM::Role" Properties: diff --git a/tests/install-messaging.test.sh b/tests/install-messaging.test.sh index fb46a341..b03ba37d 100755 --- a/tests/install-messaging.test.sh +++ b/tests/install-messaging.test.sh @@ -303,6 +303,123 @@ for stack in CORE:"$CORE_TEMPLATE" SNS:"$SNS_TEMPLATE"; do done <<<"$required" done +# --------------------------------------------------------------------------- +step "The install id is minted once and never regenerated" + +# N-11 phase 2. Regenerating this on a re-run of `install.sh --aws-only` would +# invalidate every onboarding link already issued *and* silently filter out the +# Delete ping from every account already linked — unlinking would stop working +# with no error anywhere. There is no AWS-side record to recover it from, so the +# reuse path is the only thing standing between a routine re-run and that. + +# Runs resolve_install_id in a throwaway root and prints one variable. +resolve_and_get() { + local root="$1" var="$2" + ( + ROOT="$root" + TEEMOPS_ROOT="$root" + # shellcheck source=/dev/null + source "$INSTALLER" + load_dotenv + resolve_install_id >/dev/null 2>&1 + printf '%s' "${!var-}" + ) +} + +FRESH="$WORK/install-id-fresh" +mkdir -p "$FRESH/generated" + +first="$(resolve_and_get "$FRESH" TOPS_INSTALL_ID)" +if [[ "$first" =~ ^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$ ]]; then + pass "a fresh install mints a v4 UUID ($first)" +else + fail "a fresh install produced '$first', which is not a v4 UUID" +fi + +# Simulate what write_env_file leaves behind, then re-run as `--aws-only` would. +printf 'TOPS_SNS_ARN=arn:aws:sns:us-west-2:1234:teemops-sns\nTOPS_INSTALL_ID=%s\n' "$first" \ + > "$FRESH/generated/teemops.env" + +second="$(resolve_and_get "$FRESH" TOPS_INSTALL_ID)" +if [[ "$second" == "$first" ]]; then + pass "a re-run reuses the id from generated/teemops.env" +else + fail "a re-run changed the install id from '$first' to '$second' — every onboarding link would break" +fi + +# An explicit value in .env is the rotation path and has to win. +ROTATE="$WORK/install-id-rotate" +mkdir -p "$ROTATE/generated" +printf 'TOPS_INSTALL_ID=%s\n' "$first" > "$ROTATE/generated/teemops.env" +printf 'TOPS_INSTALL_ID=%s\nTOPS_INSTALL_ID_PREVIOUS=%s\n' "new-id-0000" "$first" > "$ROTATE/.env" + +rotated="$(resolve_and_get "$ROTATE" TOPS_INSTALL_ID)" +if [[ "$rotated" == "new-id-0000" ]]; then + pass "TOPS_INSTALL_ID in .env overrides the stored value" +else + fail "rotation ignored: got '$rotated', expected 'new-id-0000'" +fi + +# Both ids reach the filter policy during a rotation, so onboarding already in +# flight against the old id still completes. +rotated_list="$(resolve_and_get "$ROTATE" TOPS_INSTALL_IDS)" +if [[ "$rotated_list" == "new-id-0000,${first}" ]]; then + pass "the filter policy receives both ids while rotating" +else + fail "filter policy list is '$rotated_list', expected 'new-id-0000,${first}'" +fi + +single_list="$(resolve_and_get "$FRESH" TOPS_INSTALL_IDS)" +if [[ "$single_list" == "$first" ]]; then + pass "without a rotation in progress only the current id is accepted" +else + fail "filter policy list is '$single_list', expected '$first'" +fi + +# --------------------------------------------------------------------------- +step "The install id reaches the SNS stack and the child templates" + +if grep -q 'TopsInstallIds=' "$INSTALLER"; then + pass "the installer passes TopsInstallIds to the SNS stack" +else + fail "the installer never passes TopsInstallIds — the filter policy would deploy empty" +fi + +if grep -q 'TOPS_INSTALL_ID=' "$INSTALLER"; then + pass "the installer records TOPS_INSTALL_ID in generated/teemops.env" +else + fail "the installer does not persist TOPS_INSTALL_ID — the next re-run would mint a new one" +fi + +for template in "$REPO_ROOT/templates/iam.role.child.account.cfn.yaml" \ + "$REPO_ROOT/templates/iam.role.audit.account.cfn.yaml"; do + name="$(basename "$template")" + + if grep -q 'TopsInstallId: !Ref TopsInstallId' "$template"; then + pass "$name passes TopsInstallId into the custom resource" + else + fail "$name does not put TopsInstallId in the message body — its ping would be filtered out" + fi + + # NoEcho does not make it secret (it is readable via DescribeStacks by anyone + # with the permission the TOPS role itself grants), but leaving it off would + # print the value in console output for no benefit at all. + if awk '/^ TopsInstallId:/{found=1} found && /NoEcho: true/{print; exit}' "$template" | grep -q NoEcho; then + pass "$name marks TopsInstallId NoEcho" + else + fail "$name does not mark TopsInstallId NoEcho" + fi +done + +# The dead condition this replaces must not come back as live YAML. Comments are +# stripped first: the template explains at length why sns:MessageAttributes cannot +# work, and that prose is the point — it is what stops the line being revived. +if sed 's/#.*//' "$SNS_TEMPLATE" | grep -q 'sns:MessageAttributes'; then + fail "sns:MessageAttributes is live in the SNS template — sns:Publish supports no such condition key" +else + pass "the dead sns:MessageAttributes condition is gone" +fi + # --------------------------------------------------------------------------- printf '\n%s%d passed, %d failed%s\n\n' "$c_bold" "$PASS" "$FAIL" "$c_off" (( FAIL == 0 )) || exit 1 From 1591b92a16a994096ab86d2db87a85cbacd9cc8a Mon Sep 17 00:00:00 2001 From: Ben Fellows Date: Mon, 3 Aug 2026 14:45:45 +1200 Subject: [PATCH 2/2] Report quarantine depth in aws:link-rejections Found on the live AWS verification of N-11, not in the test suite. With a deliberately-wrong TopsInstallId, the ping was correctly filtered out of teemops_main and landed in the quarantine queue -- and then `aws:link-rejections` reported "No account-linking messages have been rejected." Both numbers were individually right. The cache counters are written by aws:process-sqs and only ever see messages that reached the poller; a message filtered at the SNS topic never gets that far. But the command an operator is told to run was giving a falsely reassuring answer about the exact failure the quarantine queue exists to make visible. The silent failure this feature set out to close had moved one layer out. The command now reports quarantine depth alongside the counters and only gives the all-clear when both are positively known to be empty. A queue it cannot read reports as unknown rather than as zero, because those are different things -- an install that never ran the AWS step still gets its counters instead of an error. The SQS client resolves from the container when one is bound, so both the populated and the unreachable branches are covered by tests rather than skipped the way ProcessSqsMessages' client is. Verified against the live queue that exposed it: 2 quarantined, reported. Co-Authored-By: Claude Opus 5 --- app/.env.example | 4 + .../Console/Commands/ShowLinkRejections.php | 85 +++++++- app/config/services.php | 5 + app/tests/Unit/ShowLinkRejectionsTest.php | 185 ++++++++++++++++++ docker/app/entrypoint.sh | 1 + .../sns-topic-publish-authorization.md | 41 ++-- docs/roadmap.md | 20 +- 7 files changed, 322 insertions(+), 19 deletions(-) create mode 100644 app/tests/Unit/ShowLinkRejectionsTest.php diff --git a/app/.env.example b/app/.env.example index c6c12f5c..31c487d1 100644 --- a/app/.env.example +++ b/app/.env.example @@ -105,6 +105,10 @@ TOPS_SQS_DLQ_ARN= # Minted once by the installer and written to generated/teemops.env. Leave blank # here — setting it is the deliberate rotation path, not routine configuration. TOPS_INSTALL_ID= +# Written by the installer. Holds link pings rejected by the install-id filter; +# `php artisan aws:link-rejections` reports its depth. +TOPS_QUARANTINE_SQS_NAME= +TOPS_QUARANTINE_SQS_ARN= # How many hours a "Connect AWS account" link stays usable (N-11). Until the # stack is run, the pending record is a live target for anyone who can publish to # the SNS topic, so it expires; the user just reconnects from the UI to get a new diff --git a/app/app/Console/Commands/ShowLinkRejections.php b/app/app/Console/Commands/ShowLinkRejections.php index 3002c204..3e4b4d48 100644 --- a/app/app/Console/Commands/ShowLinkRejections.php +++ b/app/app/Console/Commands/ShowLinkRejections.php @@ -3,6 +3,7 @@ namespace App\Console\Commands; use App\Console\Commands\ProcessSqsMessages; +use Aws\Sqs\SqsClient; use Illuminate\Console\Command; use Illuminate\Support\Facades\Cache; @@ -21,6 +22,11 @@ * onboarding run — worth looking at. A non-zero `account_not_found` on its own is * usually benign: a stack deleted and re-run, or an abandoned onboarding link. * + * The counters only see messages that actually reached the poller. A ping carrying + * a wrong install id is filtered out at the SNS topic and never gets that far, so + * the quarantine queue depth is reported alongside them — otherwise this command + * would answer "nothing rejected" while onboarding was silently failing. + * * Read these as rejected *deliveries*, not distinct messages. Every reason except * `signature_verification_failed` deletes the message, so those count one-for-one; * a message that fails signature verification is deliberately left on the queue for @@ -60,11 +66,14 @@ public function handle(): int $counts[$reason] = (int) Cache::get($prefix . $reason, 0); } $total = (int) Cache::get($prefix . 'total', 0); + $quarantine = $this->quarantineDepth(); if ($this->option('json')) { $this->line((string) json_encode([ 'total' => $total, 'reasons' => $counts, + 'quarantined' => $quarantine['count'], + 'quarantine_status' => $quarantine['status'], ], JSON_PRETTY_PRINT)); } else { $this->table( @@ -73,15 +82,41 @@ public function handle(): int ->map(fn (int $count, string $reason) => [$reason, $count]) ->values() ->push(['TOTAL', $total]) + ->push(['quarantined (filtered at the topic)', $quarantine['count'] ?? $quarantine['status']]) ->all() ); - if ($total === 0) { + // Saying "nothing was rejected" while messages sit in quarantine would be + // the same silent failure this whole feature exists to prevent — the two + // numbers come from different layers and only one of them is in the cache. + // + // A null count means the queue could not be read, which is not the same as + // zero. The all-clear is only given when both halves are positively known + // to be empty; otherwise the warning below says what could not be checked. + if ($total === 0 && $quarantine['count'] === 0) { $this->info('No account-linking messages have been rejected.'); } + + if (($quarantine['count'] ?? 0) > 0) { + $this->warn(sprintf( + "%d message(s) were filtered out at the SNS topic and never reached the poller.\n" + . "That is a wrong or missing TopsInstallId — a stale onboarding link, a reinstall,\n" + . "or someone publishing to the topic. Inspect one with:\n" + . " aws sqs receive-message --queue-url \$(aws sqs get-queue-url --queue-name %s --query QueueUrl --output text) --visibility-timeout 0", + $quarantine['count'], + config('services.aws.quarantine_sqs_name') ?: 'teemops_quarantine', + )); + } + + if ($quarantine['count'] === null) { + $this->warn('Quarantine depth unavailable: ' . $quarantine['status']); + } } if ($this->option('reset')) { + // Only the cache counters. The quarantine queue is left alone on purpose: + // clearing it would discard the messages themselves, which are the only + // evidence of what was filtered and why. foreach ([...self::REASONS, 'total'] as $key) { Cache::forget($prefix . $key); } @@ -90,4 +125,52 @@ public function handle(): int return Command::SUCCESS; } + + /** + * How many rejected pings are sitting in the quarantine queue. + * + * These never reach `aws:process-sqs`, so they are invisible to the cache + * counters above — which is exactly why reporting them here matters. A run of + * this command that says "nothing rejected" while the queue is filling up would + * be worse than no command at all. + * + * Degrades to a status string rather than failing: an install that never ran the + * AWS step, or a container without credentials, should still get its counters. + * + * @return array{count: int|null, status: string} + */ + private function quarantineDepth(): array + { + $queueName = config('services.aws.quarantine_sqs_name'); + + if (!$queueName) { + return ['count' => null, 'status' => 'not configured (run ./install.sh --aws-only)']; + } + + try { + // Resolved from the container when something has bound one, so tests can + // substitute a client instead of reaching AWS. ProcessSqsMessages builds + // its client inline and its tests skip that path as a result; this is the + // same client with a seam, because both branches here need covering. + $sqs = app()->bound(SqsClient::class) ? app(SqsClient::class) : new SqsClient([ + 'version' => 'latest', + 'region' => config('services.aws.deployment_region', config('services.aws.region', 'us-east-1')), + 'http' => ['connect_timeout' => 5, 'timeout' => 10], + ]); + + $url = $sqs->getQueueUrl(['QueueName' => $queueName])->get('QueueUrl'); + + $attributes = $sqs->getQueueAttributes([ + 'QueueUrl' => $url, + 'AttributeNames' => ['ApproximateNumberOfMessages'], + ])->get('Attributes'); + + return [ + 'count' => (int) ($attributes['ApproximateNumberOfMessages'] ?? 0), + 'status' => 'ok', + ]; + } catch (\Throwable $e) { + return ['count' => null, 'status' => $e->getMessage()]; + } + } } diff --git a/app/config/services.php b/app/config/services.php index 71c8e825..871c3be3 100644 --- a/app/config/services.php +++ b/app/config/services.php @@ -64,6 +64,11 @@ // written to generated/teemops.env; the parent topic's subscription only // forwards child-account pings that carry it. 'install_id' => env('TOPS_INSTALL_ID'), + // Holds account-link pings the install-id filter rejected. Read by + // aws:link-rejections — these messages never reach the poller, so this queue + // is the only place they are visible. + 'quarantine_sqs_name' => env('TOPS_QUARANTINE_SQS_NAME'), + 'quarantine_sqs_arn' => env('TOPS_QUARANTINE_SQS_ARN'), 'audit_sqs_name' => env('TOPS_AUDIT_SQS_NAME', 'teemops_audit'), 'audit_sqs_arn' => env('TOPS_AUDIT_SQS_ARN'), 'audit_region_sqs_name' => env('TOPS_AUDIT_REGION_SQS_NAME', 'teemops_audit_region'), diff --git a/app/tests/Unit/ShowLinkRejectionsTest.php b/app/tests/Unit/ShowLinkRejectionsTest.php new file mode 100644 index 00000000..f20b1422 --- /dev/null +++ b/app/tests/Unit/ShowLinkRejectionsTest.php @@ -0,0 +1,185 @@ +shouldReceive('getQueueUrl')->andReturn(new Result([ + 'QueueUrl' => 'https://sqs.us-west-2.amazonaws.com/123456789012/teemops_quarantine', + ])); + $sqs->shouldReceive('getQueueAttributes')->andReturn(new Result([ + 'Attributes' => ['ApproximateNumberOfMessages' => (string) $depth], + ])); + + $this->app->instance(SqsClient::class, $sqs); + } + + private function runJson(): array + { + Artisan::call('aws:link-rejections', ['--json' => true]); + + return json_decode(Artisan::output(), true); + } + + public function test_it_reports_each_reason_and_the_total(): void + { + $this->fakeQuarantine(0); + $this->seedCounter('stack_account_mismatch', 3); + $this->seedCounter('total', 3); + + $json = $this->runJson(); + + $this->assertSame(3, $json['total']); + $this->assertSame(3, $json['reasons']['stack_account_mismatch']); + } + + /** + * A reason that has never fired must still be reported as 0 — "no such cache key" + * and "never happened" are indistinguishable otherwise, and only one is reassuring. + */ + public function test_reasons_that_never_fired_are_still_listed(): void + { + $this->fakeQuarantine(0); + + $json = $this->runJson(); + + $this->assertSame(0, $json['reasons']['link_window_expired']); + $this->assertSame(0, $json['reasons']['signature_verification_failed']); + } + + public function test_it_says_nothing_was_rejected_when_both_layers_are_empty(): void + { + $this->fakeQuarantine(0); + + $this->artisan('aws:link-rejections') + ->expectsOutputToContain('No account-linking messages have been rejected.') + ->assertSuccessful(); + } + + public function test_it_does_not_claim_all_clear_when_counters_are_non_zero(): void + { + $this->fakeQuarantine(0); + $this->seedCounter('account_not_pending', 1); + $this->seedCounter('total', 1); + + $this->artisan('aws:link-rejections') + ->doesntExpectOutputToContain('No account-linking messages have been rejected.') + ->assertSuccessful(); + } + + /** + * The regression this fix exists for. Counters are all zero because the message + * never reached the poller — but it was still rejected, and saying otherwise sent + * an operator looking in the wrong place. + */ + public function test_it_does_not_claim_all_clear_when_messages_sit_in_quarantine(): void + { + $this->fakeQuarantine(1); + + $this->artisan('aws:link-rejections') + ->doesntExpectOutputToContain('No account-linking messages have been rejected.') + ->expectsOutputToContain('filtered out at the SNS topic') + ->assertSuccessful(); + } + + public function test_quarantine_depth_is_reported_in_json(): void + { + $this->fakeQuarantine(4); + + $json = $this->runJson(); + + $this->assertSame(4, $json['quarantined']); + $this->assertSame('ok', $json['quarantine_status']); + } + + /** + * An install that never ran the AWS step still gets its counters, and is told the + * quarantine could not be read rather than being given a silent all-clear. + */ + public function test_an_unconfigured_quarantine_queue_degrades_without_failing(): void + { + Config::set('services.aws.quarantine_sqs_name', null); + $this->seedCounter('total', 2); + + $json = $this->runJson(); + + $this->assertSame(2, $json['total']); + $this->assertNull($json['quarantined']); + $this->assertStringContainsString('not configured', $json['quarantine_status']); + } + + /** + * A queue that cannot be reached must not take the whole command down with it — + * the counters are still worth reporting, and "unknown" is reported as unknown. + */ + public function test_an_unreachable_quarantine_queue_degrades_without_failing(): void + { + Config::set('services.aws.quarantine_sqs_name', 'teemops_quarantine'); + + $sqs = Mockery::mock(SqsClient::class); + $sqs->shouldReceive('getQueueUrl')->andThrow(new \RuntimeException('no credentials')); + $this->app->instance(SqsClient::class, $sqs); + + $json = $this->runJson(); + + $this->assertNull($json['quarantined']); + $this->assertStringContainsString('no credentials', $json['quarantine_status']); + } + + /** + * Reset clears the counters. It deliberately does not touch the quarantine queue, + * whose messages are the only evidence of what was filtered and why. + */ + public function test_reset_clears_the_counters_but_not_the_queue(): void + { + $this->fakeQuarantine(2); + $this->seedCounter('total', 5); + $this->seedCounter('stack_account_mismatch', 5); + + $this->artisan('aws:link-rejections', ['--reset' => true])->assertSuccessful(); + + $this->assertSame(0, (int) Cache::get(ProcessSqsMessages::REJECTION_CACHE_PREFIX . 'total', 0)); + $this->assertSame(0, (int) Cache::get(ProcessSqsMessages::REJECTION_CACHE_PREFIX . 'stack_account_mismatch', 0)); + + // Still visible after a reset, because the messages themselves are untouched. + $this->assertSame(2, $this->runJson()['quarantined']); + } +} diff --git a/docker/app/entrypoint.sh b/docker/app/entrypoint.sh index e9b0b386..ac27c58a 100644 --- a/docker/app/entrypoint.sh +++ b/docker/app/entrypoint.sh @@ -60,6 +60,7 @@ sync_env_from_compose TOPS_INSTALL_ID # Read by SnsSignatureVerifier, which fails closed when it cannot tell whose topic # a message came from. The legacy HTTP callback route lives in this tier. sync_env_from_compose TOPS_SNS_ARN +sync_env_from_compose TOPS_QUARANTINE_SQS_NAME # Queue connections come from generated/teemops.env (env_file). Bake them into the # container .env too so the web tier (php-fpm) resolves them the same as CLI, # regardless of php-fpm's environment handling. Scans use the database queue; diff --git a/docs/features/sns-topic-publish-authorization.md b/docs/features/sns-topic-publish-authorization.md index cfd5aac3..012a75c0 100644 --- a/docs/features/sns-topic-publish-authorization.md +++ b/docs/features/sns-topic-publish-authorization.md @@ -1,6 +1,6 @@ # Locking down the account-linking SNS topic -> ## 🟠 BUILT — both phases, awaiting live AWS verification +> ## ✅ DONE — both phases, verified end to end on a real AWS account > > The parent account's `teemops-sns` topic accepted `sns:Publish` from **any AWS principal > on the internet**. This is the one inbound path into a TOPS install, so it is the first @@ -11,11 +11,11 @@ > [Research findings](#research-findings) for why `sns:Publish` cannot be narrowed there. > Narrowing happens on the subscription and in the consumer. -Status: **implemented 2026-08-02, one criterion outstanding.** Everything below is covered by -automated tests except the two criteria that require a real AWS account — payload-based -filtering has not been proven against a live CloudFormation custom-resource message. That -matters more than it sounds: **a filter that matches nothing is indistinguishable from a -working integration**, so this is not "done" until an account links end to end. See +Status: **implemented and verified 2026-08-03.** Payload-based filtering was the one thing a +test suite could not prove, because what was unknown was AWS's own behaviour — and it works: +on account `848310106659` / `us-west-2`, a correct `TopsInstallId` linked exactly as before, +and a deliberately wrong one (`tops-vendor-audit-failtest`) was filtered out of `teemops_main` +and landed in the quarantine queue. See [Verifying against a real account](#verifying-against-a-real-account). Raised 2026-08-02 while producing the AWS integration architecture diagram for CISO/CTO @@ -332,11 +332,10 @@ hole one step away: `NoEcho` parameter and reaches TOPS in the message body - [x] Given the filter policy, when it is written, then it accepts a list of ids so the value can be rotated without breaking in-flight onboarding -- [ ] **Not verified.** Given a message carrying a wrong or absent install id, when it is - published, then it is delivered to the quarantine queue and does not reach `teemops_main` -- [ ] **Not verified.** Given a message carrying a *correct* install id, when it is published, - then linking completes exactly as it does today — proven end to end against a real AWS - account, not a synthetic message +- [x] Given a message carrying a wrong or absent install id, when it is published, then it is + delivered to the quarantine queue and does not reach `teemops_main` — verified 2026-08-03 +- [x] Given a message carrying a *correct* install id, when it is published, then linking + completes exactly as it does today — verified 2026-08-03 against a real AWS account ## Verifying against a real account @@ -369,6 +368,26 @@ If step 3 hangs instead of completing, the filter is matching nothing — watch `NumberOfNotificationsFilteredOut-InvalidMessageBody` on the topic, which distinguishes "the body did not parse" from "the body parsed and did not match". +### What the live run found + +Step 5 worked, and then step 6 got it wrong. With a message sitting in quarantine, +`aws:link-rejections` reported **"No account-linking messages have been rejected."** + +Both numbers were individually correct — the cache counters only see messages that reached the +poller, and a filtered message never does — but the command an operator is told to run gave a +falsely reassuring answer about the exact failure the quarantine queue exists to make visible. +The silent failure had simply moved one layer out. + +`aws:link-rejections` now reports quarantine depth alongside the counters, and only gives the +all-clear when **both** are positively known to be empty. A queue it cannot read reports as +unknown rather than as zero, because those are different things. + +**Also worth knowing before deleting a test stack:** a stack created with a wrong install id +has its `Delete` ping filtered out too, so it will hang until CloudFormation's custom-resource +timeout. Delete such stacks with `--retain-resources` on the custom resource, or expect to +wait. This is the migration hazard in reverse, and it applies to any stack created before the +filter existed. + ## Adjacent findings Noticed while researching; not part of this work, but they touch the same files. diff --git a/docs/roadmap.md b/docs/roadmap.md index 0f2a4a3b..734e4c41 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -409,7 +409,7 @@ fixes are what made that run succeed, which is the sequencing argument justifyin | # | Feature | Why it's here | Size | | --- | --- | --- | :---: | -| **N-11** | **Lock down the account-linking SNS topic** | 🟠 **Built 2026-08-02, awaiting live verification.** Both phases implemented: consumer-side validation ([#100](https://github.com/teemops/tops/issues/100)) and an install-scoped filter secret ([#101](https://github.com/teemops/tops/issues/101)). Everything is covered by tests except the one thing a test cannot cover — that SNS payload filtering matches a real CloudFormation message. Until an account links end to end, a filter matching nothing looks exactly like a working integration. [Feature doc](features/sns-topic-publish-authorization.md#verifying-against-a-real-account). | S + M | +| ~~**N-11**~~ | ~~Lock down the account-linking SNS topic~~ | ✅ **Done** 2026-08-03 — consumer-side validation ([#100](https://github.com/teemops/tops/issues/100)) and an install-scoped filter secret ([#101](https://github.com/teemops/tops/issues/101)), verified end to end on a real account: a correct install id links as before, a wrong one is filtered to quarantine and never reaches `teemops_main`. The live run also caught `aws:link-rejections` reporting "nothing rejected" while a message sat in quarantine — the silent failure moved one layer out, and is now fixed. [Feature doc](features/sns-topic-publish-authorization.md). | S + M | | ~~**N-1**~~ | ~~Fix the clean-checkout build~~ | ✅ **Done** 2026-07-29 — `@vitejs/plugin-vue` on `^6`, `npm ci` clean, frontend CI job added. | — | | ~~**N-2**~~ | ~~Choose and add a licence~~ | ✅ **Done** 2026-07-29 — Apache-2.0, trademark held separately, DCO for contributions. See D-7. | — | | ~~**N-6**~~ | ~~SNS signature verification~~ | ✅ **Done** 2026-07-30 — real signature verification via AWS's validator package, plus a topic allowlist that fails closed. Promoted from X-2 that morning when going public expired its deferral. | — | @@ -636,12 +636,18 @@ acceptance criteria — the `StackId` cross-check on `Update` as well as `Create removal of the unsigned "direct message" fallback — because closing the gaps exactly as specified would have left the same hole one step to the side. -**What is still open is a verification, not a build.** SNS payload filtering has not been run -against a real CloudFormation custom-resource message, and a filter that matches nothing is -indistinguishable from a working integration. The steps are written down in -[the feature doc](features/sns-topic-publish-authorization.md#verifying-against-a-real-account); -the riskiest one is re-running `install.sh --aws-only` and confirming the install id does -*not* change, because regenerating it silently breaks every existing link. +**Verified on a real account 2026-08-03.** SNS payload filtering does match a live +CloudFormation custom-resource message: a correct install id linked as before, and a +deliberately wrong one was filtered to the quarantine queue without reaching `teemops_main`. + +**The live run earned its keep.** It found `aws:link-rejections` answering "No account-linking +messages have been rejected" while a rejected message sat in quarantine — both numbers +individually correct, since the cache counters only see what reached the poller, but the +command an operator is told to run was reassuring them about the exact failure the quarantine +queue exists to expose. The silent failure had moved one layer out rather than being closed. +Fixed by reporting quarantine depth alongside the counters, and giving the all-clear only when +both are positively known to be empty — a queue that cannot be read now reports as unknown, +not as zero. Full research, the four options considered and why three were rejected, the open issues on the proposed design, and acceptance criteria for both phases: