Skip to content

fix: limit cached-read grants to object reads - #248

Open
perfloop[bot] wants to merge 10 commits into
mainfrom
perfloop-pr-open-0pnf306gcd
Open

perfloop[bot] wants to merge 10 commits into
mainfrom
perfloop-pr-open-0pnf306gcd

Conversation

@perfloop

@perfloop perfloop Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Transparent mode no longer treats a successful write or another non-object-read response as permission to read private cached data. TAG still learns signing keys from successful replies, but grants bucket-level cached-read authorization only after a recognized GetObject or HeadObject succeeds upstream.

The response interceptor now separates key learning from authorization. It rejects other S3 subresources, including GetObjectAttributes, and unknown or malformed query selectors even when they use GET. The internal signing-key header is still stripped on every response. No S3 route or configuration is added.

The README, security guide, and transparent-proxy maintenance notes now describe this boundary. Routed tests keep multipart initiation and two-day signing-key learning intact, verify authorized GET/HEAD cache hits, and ensure that multipart and GetObjectAttributes replies do not grant access when the origin denies GetObject. The tests also cover bucket isolation, authorization expiry and 403 revocation, and signing-key header failure paths.

The routed correctness comparison found the assertion violated at 4df832ad38e8d9cc6cea919603be1ccd7657b75f and satisfied on the final source. The fixture uses a controlled origin policy; it does not establish a production incident or its frequency, and it does not address finer-grained object policies.

The checks passed: make lint-ci, make lint, make test, make test-integration, and make test-race. Full repository CI remains pending.

Recorded proof ran with retained support that is not included in this pull request.


Generated by Perfloop. Measurements and checks. Reproduce this comparison (tenant access required).


Note

High Risk
Changes authorization semantics for transparent mode cache hits; incorrect query classification could deny legitimate reads or still over-grant, with direct impact on access to private cached objects.

Overview
Tightens transparent-proxy cached-read authorization so a successful upstream response no longer automatically grants bucket read access when TAG learns signing keys.

The response interceptor now splits identity from permission: learnSigningKeys still caches derived keys from any successful reply with the internal header, but AuthzCache.Grant runs only in grantReadAuthorization after a 2xx on a client GET/HEAD to a real object key. isReadAuthorizationQuery blocks grants for other subresources (e.g. ?attributes, ?tagging, ?uploads, unknown query keys).

Docs (README, docs/security.md, transparent-proxy rules) describe that writes and passthrough successes do not confer cached-read rights. Unit tests cover the interceptor, query classifier, and signing-key failure paths; integration tests cover multipart/tagging/attributes vs denied GetObject, HEAD cache hits, 403 revocation, and per-bucket isolation.

Reviewed by Cursor Bugbot for commit 8233c19. Bugbot is set up for automated code reviews on this repo. Configure here.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@greptile-apps

greptile-apps Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 3/5

[Critical risk] Changes authorization caching logic for object reads.

The PR is not safe to merge until the public-read authorization path and the explicit documentation requirement are addressed.

Fix All in Claude CodeFindings

  1. P1 Security Public read grants private access ▶
  2. P2 Grant explanation repeats table ▶

Summary

The PR separates signing-key learning from cached-read authorization and limits grants to successful upstream object GET and HEAD responses.

  • It adds query-selector checks to exclude other object operations.
  • It adds routed authorization-scope tests and updates the security documentation.

Reviews (1) · Last reviewed commit: "fix: infer cache grants from object read..."

return
}

f.authzCache.Grant(authInfo.AccessKey, bucket)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Public read grants private access If an access key learns signing keys from a write and then successfully reads a public object without a signing-key response header, this line grants it cached-read access to the entire bucket. A later signed GET can receive a private object cached by another reader even when the origin would deny that access key. The ocache client retrieves cached data by key; TAG checks the object's public status only for anonymous cache hits. How this was verified: A signed public-object success reaches the bucket-wide grant, while authenticated cache hits do not check the cached object's private ACL.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The grant after a successful public-object GET can authorize later private cache hits for the whole bucket, even when that response carried no signing-key header. I will restrict grants to confirmed client GetObject and HeadObject operations and keep key learning independent of the grant decision.

Source feedback

Comment thread docs/security.md
## Authorization Lifecycle

Authorization decisions are cached per `(accessKey, bucket)` pair:
Authorization decisions are cached per `(accessKey, bucket)` pair. TAG may learn signing keys from any successful response with the internal header. Those keys prove identity, not read permission. TAG grants bucket authorization only after a successful upstream `GetObject` or `HeadObject` response. Successful writes and other passthrough operations do not grant cached-read access.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Grant explanation repeats table The added paragraph states that successful GetObject and HeadObject responses grant authorization, and the adjacent lifecycle table states the same event and action. This violates the repository directive to avoid explanatory notes that repeat information already in the text. Keep the distinct key-learning explanation, but remove the repeated grant explanation before merging.

Rule Used: Avoid adding explanatory notes that simply restate information already visible in diagrams or already mentioned in the text. (source)

Learned From
tigrisdata/tigris-os-docs#265

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The added paragraph repeats the grant event and action already shown in the lifecycle table. I will remove that paragraph while retaining the separate explanation of signing-key learning.

Source feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant