From 2f2fc83a82adf0475b2103dd2d15302a32596953 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Fri, 24 Jul 2026 15:00:15 -0400 Subject: [PATCH] fix(webhook): kms:ViaService pins, https-only delivery, cross-account principal CI pin GPT-4.1 cross-family review of the policy surface (no BLOCK): FIX applied to the cross-account shoc-backend-dev Decrypt statement and both Lambda role KMS grants (the key is only ever used via Secrets Manager); its invariant-enforcement QUESTION answered durably with tests/test_cross_account_principal_pin.py (any new foreign IAM principal in cdk/ fails CI). Scanner mediums fixed: delivery.py and the replay script now refuse non-https URLs (urllib follows file:// and http://). SQS metadata-action and dynamodb:ListStreams NITs skipped: standard CDK grant shapes; ListStreams has no resource-level scoping. The 4 gitleaks HIGHs on docs/shoc-webhook-test-vectors.json are deliberate non-secrets (shared receiver-verification vectors) suppressed machine-level with justification. --- cdk/wo_stack.py | 43 ++++++++++++++++- lambdas/wo/shoc_emitter/delivery.py | 7 +++ scripts/replay_shoc_webhooks.py | 4 ++ tests/test_cross_account_principal_pin.py | 57 +++++++++++++++++++++++ 4 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 tests/test_cross_account_principal_pin.py diff --git a/cdk/wo_stack.py b/cdk/wo_stack.py index b741dff..cb02eb2 100644 --- a/cdk/wo_stack.py +++ b/cdk/wo_stack.py @@ -477,11 +477,19 @@ def _add_shoc_webhook_emitter(stack, work_orders_table, comments_table, alarm_to # policy below is the other half; either one alone fails silently at # the receiver). resources=["*"] is key-scoped, not account-wide -- # KMS key policies only ever apply to this key. + # kms:ViaService pins the grant to Secrets Manager decrypt paths only + # (GPT-4.1 cross-review FIX): a compromised shoc-backend-dev cannot use + # this key for arbitrary KMS operations outside the secret fetch. shoc_webhook_key.add_to_resource_policy( iam.PolicyStatement( actions=["kms:Decrypt"], principals=[shoc_consumer_principal], resources=["*"], + conditions={ + "StringEquals": { + "kms:ViaService": (f"secretsmanager.{stack.region}.amazonaws.com") + } + }, ) ) @@ -577,7 +585,25 @@ def _add_shoc_webhook_emitter(stack, work_orders_table, comments_table, alarm_to resources=[shoc_hmac_secret.secret_arn], ) ) - shoc_webhook_key.grant_encrypt_decrypt(shoc_hmac_rotator) + # Explicit statement instead of grant_encrypt_decrypt so the grant can + # carry kms:ViaService (cross-review FIX): the rotator only ever touches + # this key through Secrets Manager put/get, never the KMS API directly. + shoc_hmac_rotator.add_to_role_policy( + iam.PolicyStatement( + actions=[ + "kms:Decrypt", + "kms:Encrypt", + "kms:GenerateDataKey*", + "kms:ReEncrypt*", + ], + resources=[shoc_webhook_key.key_arn], + conditions={ + "StringEquals": { + "kms:ViaService": (f"secretsmanager.{stack.region}.amazonaws.com") + } + }, + ) + ) shoc_hmac_secret.add_rotation_schedule( "Rotation", rotation_lambda=shoc_hmac_rotator, @@ -669,7 +695,20 @@ def _add_shoc_webhook_emitter(stack, work_orders_table, comments_table, alarm_to work_orders_table.grant_stream_read(shoc_emitter) comments_table.grant_stream_read(shoc_emitter) shoc_hmac_secret.grant_read(shoc_emitter) - shoc_webhook_key.grant_decrypt(shoc_emitter) + # Explicit statement instead of grant_decrypt so the grant carries + # kms:ViaService (cross-review FIX): the emitter only decrypts this key + # through Secrets Manager GetSecretValue. + shoc_emitter.add_to_role_policy( + iam.PolicyStatement( + actions=["kms:Decrypt"], + resources=[shoc_webhook_key.key_arn], + conditions={ + "StringEquals": { + "kms:ViaService": (f"secretsmanager.{stack.region}.amazonaws.com") + } + }, + ) + ) shoc_emitter_rejected_queue.grant_send_messages(shoc_emitter) # ------------------------------------------------------------------ diff --git a/lambdas/wo/shoc_emitter/delivery.py b/lambdas/wo/shoc_emitter/delivery.py index 6c9eca2..d94643c 100644 --- a/lambdas/wo/shoc_emitter/delivery.py +++ b/lambdas/wo/shoc_emitter/delivery.py @@ -114,6 +114,13 @@ def deliver(envelope: dict) -> tuple[str, int]: Raises RetryableDeliveryError for 429/5xx/timeout/connection failures so the caller can block the shard (in-order retry, contract section 7). """ + if not SHOC_WEBHOOK_URL.startswith("https://"): + # Fail closed on any non-HTTPS scheme (file://, http://, ...): the + # URL is operator-set env config, but urllib would happily follow + # other schemes and the HMAC only protects an HTTPS transport. + raise RetryableDeliveryError( + "SHOC_WEBHOOK_URL is not an https:// URL; refusing to deliver" + ) raw_body = json.dumps(envelope).encode("utf-8") timestamp = int(time.time()) signing_key = _get_hmac_keys()[0] diff --git a/scripts/replay_shoc_webhooks.py b/scripts/replay_shoc_webhooks.py index eb75aa6..6c8a0a1 100644 --- a/scripts/replay_shoc_webhooks.py +++ b/scripts/replay_shoc_webhooks.py @@ -355,6 +355,10 @@ def main(): if bool(args.work_order_id) == bool(args.since): sys.exit("ERROR: pass exactly one of --work-order-id or --since.") + if not args.url.startswith("https://"): + # urllib follows file:// and http:// too; the feed is HTTPS-only. + sys.exit("ERROR: --url must be an https:// URL.") + session = boto3.Session( profile_name=args.profile or "seahaven-prod", region_name=REGION ) diff --git a/tests/test_cross_account_principal_pin.py b/tests/test_cross_account_principal_pin.py new file mode 100644 index 0000000..9047a00 --- /dev/null +++ b/tests/test_cross_account_principal_pin.py @@ -0,0 +1,57 @@ +"""Pin the cross-account principal surface of the CDK app. + +The SHOC integration deliberately trusts EXACTLY ONE foreign principal: +``arn:aws:iam::396287094661:role/shoc-backend-dev`` (read API resource +policy in procurement_api_stack.py, HMAC secret + KMS grants in +wo_stack.py). Future shoc-backend-staging/-prod roles are each a +deliberate, individually-reviewed policy addition — so any new foreign +account id or role ARN appearing in cdk/ must consciously update this +pin (and go through the mandatory GPT-4.1 cross-family IAM review). + +Raised as a QUESTION in the 2026-07-24 cross-family review of the +webhook emitter policy surface: "how is the exact-one-principal +invariant enforced over time?" — this test is the answer. +""" + +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +CDK_DIR = REPO_ROOT / "cdk" + +# The one foreign principal the app may reference, and the only files +# allowed to reference it. +ALLOWED_FOREIGN_PRINCIPAL = "arn:aws:iam::396287094661:role/shoc-backend-dev" +ALLOWED_FILES = {"procurement_api_stack.py", "wo_stack.py"} + +# Accounts that are not "foreign": seahaven-prod (the deploy target). +HOME_ACCOUNTS = {"011934824531"} + +_IAM_ARN_RE = re.compile(r"arn:aws:iam::(\d{12}):\S*?(?=[\"'\s])") + + +def _cdk_sources(): + return sorted(CDK_DIR.glob("*.py")) + + +def test_only_the_pinned_foreign_principal_appears_in_cdk_sources(): + findings = [] + for path in _cdk_sources(): + for match in _IAM_ARN_RE.finditer(path.read_text()): + account = match.group(1) + if account in HOME_ACCOUNTS: + continue + findings.append((path.name, match.group(0))) + + unexpected = [ + (name, arn) + for name, arn in findings + if arn != ALLOWED_FOREIGN_PRINCIPAL or name not in ALLOWED_FILES + ] + assert not unexpected, ( + "Unexpected foreign IAM principal(s) in cdk/ — every cross-account " + f"trust addition must update this pin deliberately: {unexpected}" + ) + # Both grant sites must still reference the pinned role (deleting one + # half of the secret/KMS grant pair fails silently at the receiver). + assert {name for name, _ in findings} == ALLOWED_FILES