Fix: Frequent AccessDenied requests against manifest bucket from foreign VPC (#8118) - #8228
Conversation
nadove-ucsc
added
the
deploy:shared
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8228 +/- ##
========================================
Coverage 84.79% 84.79%
========================================
Files 168 168
Lines 25191 25191
========================================
Hits 21360 21360
Misses 3831 3831 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
coverage: 84.868%. remained the same — issues/nadove-ucsc/8118-frequent-access-denied into develop |
| _or( | ||
| '$.eventSource != "s3.amazonaws.com"', | ||
| '$.userIdentity.accountId != "anonymous"', | ||
| f'$.requestParameters.bucketName != "{aws.qualified_bucket_name("*")}"' |
There was a problem hiding this comment.
| f'$.requestParameters.bucketName != "{aws.qualified_bucket_name("*")}"' | |
| f'$.requestParameters.bucketName != "{aws.qualified_bucket_name('*')}"' |
065b039 to
fba81d3
Compare
|
(Posted by Claude Code) Test of the
|
| # | eventSource | eventName | Caller | Matches |
|---|---|---|---|---|
| 0 | s3 | HeadBucket | IAM user in 652235167018 |
yes |
| 1 | s3 | GetBucketObjectLockConfiguration | IAM user in 652235167018 |
yes |
| 2 | s3 | GetBucketVersioning | IAM user in 652235167018 |
yes |
| 3 | ce | ListCostCategoryDefinitions | config.amazonaws.com |
no |
| 4 | macie2 | ListCustomDataIdentifiers | resource-explorer-2.amazonaws.com |
no |
| 5 | ce | ListCostCategoryDefinitions | config.amazonaws.com |
no |
| 6 | s3 | ListObjects (…mirror-nadove) |
anonymous | no |
| 7 | ecr | BatchGetImage | lambda.amazonaws.com |
yes |
| 8 | ce | ListCostCategoryDefinitions | config.amazonaws.com |
no |
Event 6 is the anonymous foreign-VPC request this issue is about, and the new clause suppresses it. The service-linked callers in events 3, 4 and 5 remain suppressed by the pre-existing invokedBy clause. Events 0–2 (an authenticated cross-account IAM user denied in the console) and event 7 (an AWSService caller denied on ECR) fall outside the new clause and continue to alarm, as they should.
The test also confirms that the negated wildcard comparison on bucketName works: suppression takes effect only for bucket names matching edu-ucsc-gi-platform-hca-dev-*.us-east-1.
|
The events the pattern was tested against was produced using our canned api_unauthorized CW Logs query. |
|
(Posted by Claude Code) Security design review
Documentation of unchecked boxes… affect monitoring of the system This PR adds a third conjunct to the Note the distinction that carries most of the risk argument: CloudTrail still records these events, and the trail log group retains them for Findings below were verified empirically against Finding 1 (medium): missing-field semantics silently widen the exception beyond anonymous callersIn CloudWatch JSON filter patterns,
So Suggested fix, consistent with the idiom already used three lines above: _or(
'$.eventSource != "s3.amazonaws.com"',
'$.userIdentity.accountId NOT EXISTS',
'$.userIdentity.accountId != "anonymous"',
'$.requestParameters.bucketName NOT EXISTS',
f'$.requestParameters.bucketName != "{aws.qualified_bucket_name('*')}"'
)Finding 2 (low): same root cause on
|
hannes-ucsc
left a comment
hannes-ucsc
left a comment
There was a problem hiding this comment.
Please verify Claude's SDD findings 1 and 2.
I am not sure these fields can ever be missing.
We will accept 3.
|
Finding 1 empirically confirmed as correct. Requests without CloudWatch Log Analytics Results
|
|
Finding 2 also empirically confirmed as correct: |
1d2c0f6 to
96bcea6
Compare
|
(Contains content generated by Claude) Re-test of the
|
| # | eventSource | eventName | Caller | develop |
065b039 | 96bcea6 |
|---|---|---|---|---|---|---|
| 0 | s3 | HeadBucket (…shared) |
IAM user in 652235167018 |
yes | yes | yes |
| 1 | s3 | ListObjects (…mirror-dev) |
anonymous | yes | no | no |
| 2 | detective | ListOrganizationAdminAccounts | config.amazonaws.com |
no | no | no |
| 3 | macie2 | ListFindingsFilters | resource-explorer-2.amazonaws.com |
no | no | no |
| 4 | ecr | BatchGetImage | lambda.amazonaws.com, accountId absent |
yes | yes | yes |
| 5 | s3 | PutObject (…logs) |
s3.amazonaws.com, accountId absent |
yes | no | yes |
| 6 | s3 | GetObject (…hca-prod-logs) |
in-account role | yes | yes | yes |
| 7 | logs | GetLogGroupFields | account 289950828509 |
yes | yes | yes |
| 8 | lambda | GetFunction20150331v2 | type Unknown |
yes | yes | yes |
Event 1 remains the only event the new exception suppresses — the anonymous foreign-VPC request this issue is about. Every other event alarms exactly as it does on develop, so the fixup leaves the filter's coverage unchanged apart from the intended suppression.
Event 5 is what the NOT EXISTS guards restore. It is a real S3 inventory report delivery denied against the logs bucket, one of 65 such events in the retained year. Its userIdentity is {"type": "AWSService", "invokedBy": "s3.amazonaws.com"} with no accountId field at all; because != evaluates to false on an absent field, $.userIdentity.accountId != "anonymous" was false and the exception swallowed it at 065b039. Note that invokedBy is neither config.amazonaws.com nor resource-explorer-2.amazonaws.com, so the pre-existing exception does not cover it either.
The second guard was verified the same way: an anonymous AccessDenied carrying no requestParameters.bucketName matches at 96bcea6 and did not at 065b039. No such event occurs in the trail — all 178 anonymous S3 records in the retained year carry a bucket name — so this guard is defensive. It is included for symmetry with the accountId guard, and it cannot produce a false negative.
Both guards can only ever make the filter match more, never less, so the fixup cannot silence anything that alarms today. Relative to develop, alarming S3 AccessDenied events in dev over the retained year go 148 → 91, all 57 suppressed events being anonymous; at 065b039 they went 148 → 26. The fixup narrows the reduction to the one the issue asked for. (Counts are events rather than alarms; the alarm is Sum > 0 over a five-minute period, so several events collapse into one alarm.)
Event IDs
| # | eventTime | eventID |
|---|---|---|
| 0 | 2026-08-19T20:23:37Z | 516e0d19-f874-30b9-bf50-468c160adaac |
| 1 | 2026-09-09T04:26:55Z | be4448ef-1407-3719-80f2-be16333e2831 |
| 2 | 2026-09-10T08:56:04Z | 2d140a74-518a-4093-abc4-88c2d9afd55c |
| 3 | 2026-09-04T04:44:35Z | c9a3efe9-db02-4877-874b-d2fa9484fe47 |
| 4 | 2026-08-26T06:22:58Z | c476c29b-5c83-3224-a077-1007241e22e6 |
| 5 | 2025-12-09T11:46:28Z | 487f41b3-f0cb-35a2-bede-458e02b2fa0b |
| 6 | 2026-06-04T21:22:25Z | aa856f86-0d8a-36e2-bfae-744e8b3a0ef9 |
| 7 | 2026-09-02T04:55:00Z | b003a860-99b2-31a1-82c4-3b195d10ae49 |
| 8 | 2026-02-25T01:21:00Z | 39ee61bb-ce12-3e7d-aa88-2e853a64639f |
|
(Posted by Claude Code) Security design review, round 2 (head
|
| case | base | v1 | head | expected |
|---|---|---|---|---|
| AWSService, accountId absent, our bucket (finding 1) | alarm | — | alarm | alarm |
anonymous, requestParameters absent (finding 2) |
alarm | — | alarm | alarm |
anonymous, bucketName absent, i.e. ListBuckets (finding 2) |
alarm | — | alarm | alarm |
userIdentity absent entirely |
alarm | — | alarm | alarm |
| verbatim #8118 record, the intended suppression | alarm | — | — | suppress |
| anonymous, foreign bucket | alarm | alarm | alarm | alarm |
All five cases that regressed under v1 are restored, and the intended suppression still holds. The last of those was confirmed against the record quoted verbatim in the body of #8118, not a reduced stand-in. The rendered pattern is 507 characters against a limit of 1024, and the longest added line is 103 characters against the project limit of 120, so make pep8 is unaffected.
Correction to the first review
I rated finding 1 medium, reasoning in part that the denials against the -logs, -shared and -awsconfig buckets in the tables on #8118 were most likely AWS-service-invoked. I have since checked the data rather than inferring it. Over 90 days and 11.7M records in azul-trail-dev, every S3 denial was userIdentity.type: AWSAccount with accountId present — 58 anonymous and 9 not — and none were AWSService-shaped. The scenario is therefore real in the logic of the filter but unobserved in dev, and medium overstated what the evidence supported. Since the fix is correct and costs nothing, this changes nothing about the outcome.
That evidence is thin by nature: dev sees 67 S3 denials per 90 days, whereas the events motivating this issue occurred in prod. Running the same queries against prod would firm this up considerably, and I'm happy to do so on request.
New observation (informational, no action recommended)
NOT EXISTS is false for an explicit JSON null, and so is !=, so an accountId or bucketName of null still suppresses. This is the same class as findings 1 and 2, one step further out, and it is not far-fetched in principle, since CloudTrail does emit explicit nulls for some fields — responseElements: null appears in the #8118 record itself.
IS NULL turns out to be accepted filter syntax, whereas = NULL, != NULL and IS NOT NULL are all rejected by the API. Adding two IS NULL disjuncts closes the residue with no regressions across the matrix, at 589 characters.
My recommendation is nevertheless to leave this alone. The same 90-day query found no S3 denial with a null accountId or bucketName, so there is no evidence CloudTrail ever emits nulls in those two positions, and seven disjuncts in one expression is a poor trade against readability for an unobserved shape. Recording it here so that the decision is explicit rather than overlooked.
Still open from the first review
Finding 5 is unaddressed: the comment above the alarm is unchanged, so the second deviation from Security Hub CloudWatch.2 remains undocumented. This is a documentation ask, not a blocker.
Recommendation
Approve. The blocking finding is fixed and verified, and nothing new blocks.
96bcea6 to
41ff987
Compare
nadove-ucsc
added
the
sandbox
3 participants
Linked issues: #8118
Checklist
Author
developissues/<GitHub handle of author>/<issue#>-<slug>1 when the issue title describes a problem, the corresponding PR
title is
Fix:followed by the issue titleAuthor (partiality)
ptag to titles of partial commitspartialor completely resolves all linked issuespartiallabelAuthor (reindex)
rtag to commit title or the changes introduced by this PR will not require reindexing of any deploymentreindex:devor the changes introduced by it will not require reindexing ofdevreindex:anvildevor the changes introduced by it will not require reindexing ofanvildevreindex:anvilprodor the changes introduced by it will not require reindexing ofanvilprodreindex:prodor the changes introduced by it will not require reindexing ofprodreindex:partialand its description documents the specific reindexing procedure fordev,anvildev,anvilprodandprodor requires a full reindex or carries none of the labelsreindex:dev,reindex:anvildev,reindex:anvilprodandreindex:prodAuthor (mirror)
mirror:devor the changes introduced by it will not require mirroring ofdevmirror:anvildevor the changes introduced by it will not require mirroring ofanvildevmirror:anvilprodor the changes introduced by it will not require mirroring ofanvilprodmirror:prodor the changes introduced by it will not require mirroring ofprodmirror:partialand its description documents the specific mirroring procedure fordev,anvildev,anvilprodandprodor requires a full mirroring or carries none of the labelsmirror:dev,mirror:anvildev,mirror:anvilprodandmirror:prodAuthor (API changes)
APIor this PR does not modify a REST APIa(A) tag to commit title for backwards (in)compatible changes or this PR does not modify a REST APIapp.pyor this PR does not modify a REST APIAuthor (upgrading deployments)
make docker_images.jsonand committed the resulting changes or this PR does not modifyazul_docker_images, or any other variables referenced in the definition of that variableutag to commit title or this PR does not require upgrading deploymentsupgradeor does not require upgrading deploymentsdeploy:sharedor does not modifydocker_images.json, and does not require deploying thesharedcomponent for any other reasondeploy:gitlabor does not require deploying thegitlabcomponentdeploy:runneror does not require deploying therunnerimageAuthor (hotfixes)
Ftag to main commit title or this PR does not include permanent fix for a temporary hotfixanvilprodandprod) have temporary hotfixes for any of the issues linked to this PRAuthor (before every review)
develop, squashed fixups from prior reviewsmake requirements_updateor this PR does not modifyDockerfile,environment,requirements*.txt,common.mk,Makefileorenvironment.bootRtag to commit title or this PR does not modifyrequirements*.txtreqsor does not modifyrequirements*.txtmake integration_testpasses in personal deployment or this PR does not modify functionality that could affect the IT outcomePeer reviewer (after approval)
Note that after requesting changes, the PR must be assigned to only the author.
System administrator (after approval)
demoorno demono demono sandboxN reviewslabel is accurateOperator
reindex:…labels andrcommit title tagmirror:…labelsno demodevelopOperator (deploy
.sharedand.gitlabcomponents)_select dev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select dev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlab_select anvildev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select anvildev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlabdeploy:gitlabdeploy:gitlabSystem administrator (post-deploy of
.gitlabcomponent)dev.gitlabare complete or this PR is not labeleddeploy:gitlabanvildev.gitlabare complete or this PR is not labeleddeploy:gitlabOperator (deploy runner image)
_select dev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runner_select anvildev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runnerOperator (sandbox build)
sandboxlabel or PR is labeledno sandboxdevor PR is labeledno sandboxanvildevor PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxor this PR is not labeledupgrade, or upgrade instructions do not apply tosandboxanvilboxor this PR is not labeledupgrade, or upgrade instructions do not apply toanvilboxsandboxor this PR does not remove catalogs or otherwise causes unreferenced indices insandboxanvilboxor this PR does not remove catalogs or otherwise causes unreferenced indices inanvilboxsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevsandboxor this PR is not labeledmirror:devanvilboxor this PR is not labeledmirror:anvildevsandboxor this PR is not labeledmirror:devanvilboxor this PR is not labeledmirror:anvildevOperator (merge the branch)
pif the PR is also labeledpartialOperator (main build)
devanvildevdevdevanvildevanvildevdevor this PR is not labeledupgrade, or upgrade instructions do not apply todevanvildevor this PR is not labeledupgrade, or upgrade instructions do not apply toanvildevupgrade, or upgrade instructions do not apply to personal deployments_select dev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shared_select anvildev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shareddevanvildevOperator (reindex)
devor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevdevor this PR does not require reindexingdevdeploy_browserjob in the GitLab pipeline for this PR indevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdeploy_browserjob in the GitLab pipeline for this PR inanvildevor this PR does not require reindexinganvildevOperator (mirroring)
devor this PR is not labelledmirror:devanvildevor this PR is not labelledmirror:anvildevdevor this PR is not labelledmirror:devanvildevor this PR is not labelledmirror:anvildevdevor this PR is not labelledmirror:devanvildevor this PR is not labelledmirror:anvildevOperator
upgradeandAPIlabels to the next promotion PRs or this PR carries neither of these labelsdeploy:shared,deploy:gitlab,deploy:runner,reindex:partial,reindex:anvilprod,reindex:prod,mirror:partial,mirror:anvilprodandmirror:prodlabels to the next promotion PRs or this PR carries none of these labelsdeploy:shared,deploy:gitlab,deploy:runner,reindex:partial,reindex:anvilprod,reindex:prod,mirror:partial,mirror:anvilprodandmirror:prodlabels, from the description of this PR to that of the next promotion PRs or this PR carries none of these labelsShorthand for review comments
Lline is too longWline wrapping is wrongQbad quotesFother formatting problem