N-11 · Lock down the account-linking SNS topic (phases 1 and 2)

app/.env.example

@@ -102,6 +102,18 @@ 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=
+# 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
+# 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.
 

app/app/Console/Commands/ShowLinkRejections.php

@@ -0,0 +1,176 @@
+<?php
+
+namespace App\Console\Commands;
+
+use App\Console\Commands\ProcessSqsMessages;
+use Aws\Sqs\SqsClient;
+use Illuminate\Console\Command;
+use Illuminate\Support\Facades\Cache;
+
+/**
+ * Read back the account-linking rejection counters that aws:process-sqs writes.
+ *
+ * N-11 asked for a signal an operator can alarm on, rather than only a
+ * `Log::warning` that a self-hosted install may have no pipeline to collect. The
+ * counters are meaningless if nothing can read them, so this is that half:
+ *
+ *     php artisan aws:link-rejections
+ *     php artisan aws:link-rejections --json
+ *
+ * A non-zero `stack_account_mismatch` or `signature_verification_failed` means
+ * something published to the SNS topic that did not come from a legitimate
+ * 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
+ * redelivery, so one such message counts up to maxReceiveCount (5) times before it
+ * lands in the DLQ.
+ */
+class ShowLinkRejections extends Command
+{
+    protected $signature = 'aws:link-rejections
+        {--json : Emit JSON, for a monitoring agent rather than a human}
+        {--reset : Clear the counters after reading them}';
+
+    protected $description = 'Show why account-linking messages were rejected (N-11 counters)';
+
+    /**
+     * The reasons aws:process-sqs can record. Listed explicitly so a reason that
+     * has never fired still prints as 0 — "no such key" and "never happened" look
+     * identical in a cache store, and only one of them is reassuring.
+     */
+    private const REASONS = [
+        'not_an_sns_notification',
+        'signature_verification_failed',
+        'missing_required_fields',
+        'invalid_role_arn',
+        'stack_account_mismatch',
+        'account_not_found',
+        'account_not_pending',
+        'link_window_expired',
+    ];
+
+    public function handle(): int
+    {
+        $prefix = ProcessSqsMessages::REJECTION_CACHE_PREFIX;
+
+        $counts = [];
+        foreach (self::REASONS as $reason) {
+            $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(
+                ['Reason', 'Count'],
+                collect($counts)
+                    ->map(fn (int $count, string $reason) => [$reason, $count])
+                    ->values()
+                    ->push(['TOTAL', $total])
+                    ->push(['quarantined (filtered at the topic)', $quarantine['count'] ?? $quarantine['status']])
+                    ->all()
+            );
+
+            // 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);
+            }
+            $this->info('Counters reset.');
+        }
+
+        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()];
+        }
+    }
+}

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&param_ParentAWSAccountId=%s&param_ParentDeploymentRegion=%s&param_ExternalId=%s&param_UniqueId=%s',
+            'https://console.aws.amazon.com/cloudformation/home?region=%s#/stacks/quickcreate?templateUrl=%s&stackName=tops-vendor-audit&param_ParentAWSAccountId=%s&param_ParentDeploymentRegion=%s&param_ExternalId=%s&param_UniqueId=%s&param_TopsInstallId=%s',
             urlencode($deploymentRegion),
             urlencode($templateUrl),
             urlencode($parentAccountId),
             urlencode($deploymentRegion),
             urlencode($externalId),
             urlencode($uniqueId),
+            urlencode($installId),
         );
     }
 

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');
         }

app/config/services.php

@@ -48,13 +48,27 @@
         '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
         // maxReceiveCount (5) failed deliveries; aws:redrive-dlq moves them back.
         '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'),
+        // 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'),

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();