add accounts.anon_rbac_policies_json - #789
Conversation
SuperSandro2000
left a comment
SuperSandro2000
left a comment
There was a problem hiding this comment.
Not a super big fan AnonymousRBACPoliciesJSON = "" vs AnonymousRBACPoliciesJSON = []
As the comments hint at, the final intent for this is to skip issuing signed tokens for anonymous users. Instead, in the common case, anonymous users should receive a token that is basically just equal to `base64(join(audience, repo_name))`. There is precedent for this with GHCR, which also issues simple Base64-encoded strings only for anonymous users: ``` $ curl -s 'https://ghcr.io/token?service=ghcr.io&scope=repository:home-assistant/home-assistant:pull' | tee /tmp/resp.json {"token":"djE6aG9tZS1hc3Npc3RhbnQvaG9tZS1hc3Npc3RhbnQ6MTc4NzkwODAxNTc5OTYxMjExMA=="} $ </tmp/resp.json jq -r .token | base64 -d v1:home-assistant/home-assistant:1787908015799612110 ``` This would allow us to skip verifying the cryptographic signature on a JWT-style token, which currently makes up a significant amount of the CPU cost for common hot paths like GetBlob and GetManifest. However, we then need to recheck AuthZ during these hot paths. This will be made possible by including the relevant RBAC policies for anonymous users in the ReducedAccount. I have made an effort to make payloads in the new field as compact as possible, and beyond that, there is a protection to keep the field from growing too large even if a user configures a large amount of RBAC policies: The worst that will happen is that the field is not populated at all, and AuthZ will instead issue a normal JWT-style token to the anonymous user. There is a problem here, in that we don't have an easy way to populate this field for existing accounts. Since we currently still have direct DB access to all prod deployments, I plan to address this migration problem with a script that GETs and PUTs each account once, thus triggering the relevant codepath that fills the new column.
a223555 to
aa43c3f
Compare
7269af8 to
7d02e7b
Compare
I added a check constraint to enforce |
| targetAccount.RBACPoliciesJSON = string(buf) | ||
|
|
||
| buf, err := json.Marshal(anonPolicies) | ||
| if err == nil && len(buf) <= models.AnonymousRBACPoliciesJSONMaxLength { |
There was a problem hiding this comment.
Can we use some other estimation to end the loop early if it is likely to big? Right now we would build the entire string up to all to only throw it away if it is longer than 64 chars. Maybe we use an amount and say no more than 5 entries?
There was a problem hiding this comment.
This is during PutAccount, which is a rare operation, so I find this acceptable. (Also, we're handling all the RBAC policies anyway, so it's only causing more work proportional to the existing amount of work.)
The optimization target is to avoid loading lots of data in ReducedAccount when not necessary.
There was a problem hiding this comment.
🟡 Changes recommended
Critical serialization and policy-conversion defects must be fixed, and stale-policy clearing needs coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds compact anonymous RBAC policy persistence as groundwork for lightweight anonymous tokens.
Changes:
- Adds and populates
anon_rbac_policies_json. - Introduces compact anonymous-policy serialization.
- Standardizes empty policy fields as
[].
File summaries
| File | Review |
|---|---|
internal/test/setup.go |
Centralizes account defaults. Nit: use plural “fields” for subject agreement. |
internal/tasks/accounts_test.go |
Applies defaults in task tests. |
internal/tasks/account_management_test.go |
Updates account-management expectations. |
internal/processor/accounts.go |
Critical: legacy JSON marshaling serializes nonempty policies as [{}]; use JSON v2 or implement legacy MarshalJSON. |
internal/models/account.go |
Adds the storage field, defaults, and size limit. |
internal/keppel/tag_policy.go |
Removes empty-string compatibility. |
internal/keppel/rbac_policy.go |
Critical: authenticated-only policies can produce an invalid empty permission encoding; return None when no anonymous permission applies. |
internal/keppel/gc_policy.go |
Removes empty-string compatibility. |
internal/keppel/database.go |
Adds schema migrations and constraints. |
internal/api/registry/manifests_test.go |
Uses standardized empty RBAC JSON. |
internal/api/registry/blobs_test.go |
Uses standardized empty RBAC JSON. |
internal/api/keppel/api_test.go |
Uses standardized empty RBAC JSON. |
internal/api/keppel/accounts_test.go |
Moderate: the oversized-update test should verify that previously populated policy data is cleared to []. |
internal/api/auth/api_test.go |
Updates empty RBAC test setup. |
Review details
Suppressed comments (1)
internal/test/setup.go:126
- Use the plural “fields” so the subject agrees with “have”.
// some field have default values that are not the zero value
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The PR description claims ReducedAccount will carry anonymous RBAC policies for hot-path AuthZ, but the code still leaves this as a TODO, so the implementation doesn’t yet match the stated plan.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
internal/models/account.go:115
- PR description indicates anonymous AuthZ on hot paths will be enabled by including anonymous RBAC policies in ReducedAccount, but ReducedAccount still has a TODO and does not include this field yet. Either include AnonymousRBACPoliciesJSON in ReducedAccount (and update Account.Reduced()/queries accordingly) or clarify in the PR description that ReducedAccount changes will come in a follow-up.
internal/models/account.go:144 - The doc comment says the field will be "left empty" when the size limit is exceeded, but the implementation stores the empty list ("[]") (empty string is disallowed by a DB CHECK). Clarify the comment to avoid implying an empty string is used.
internal/test/setup.go:126 - Typo/grammar in comment: "some field have" should be plural and use correct verb agreement ("some fields have").
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
d429a80 to
dc0ffdd
Compare
Merging this branch changes the coverage (2 decrease, 2 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
I force-pushed to address the docstring notes from the last Copilot run. The TODO that it claimed to be missing is actually there, and I will take that in the next PR for this feature. |
As the comments hint at, the final intent for this is to skip issuing signed tokens for anonymous users. Instead, in the common case, anonymous users should receive a token that is basically just equal to
base64(join(audience, repo_name)). There is precedent for this with GHCR, which also issues simple Base64-encoded strings only for anonymous users, so this ought not break existing users:This would allow us to skip verifying the cryptographic signature on a JWT-style token, which currently makes up a significant amount of the CPU cost for common hot paths like GetBlob and GetManifest. However, we then need to recheck AuthZ during these hot paths. This will be made possible by including the relevant RBAC policies for anonymous users in the ReducedAccount.
I have made an effort to make payloads in the new field as compact as possible, and beyond that, there is a protection to keep the field from growing too large even if a user configures a large amount of RBAC policies: The worst that will happen is that the field is not populated at all, and AuthZ will instead issue a normal JWT-style token to the anonymous user.
There is a problem here, in that we don't have an easy way to populate this field for existing accounts. Since we currently still have direct DB access to all prod deployments, I plan to address this migration problem with a script that GETs and PUTs each account once, thus triggering the relevant codepath that fills the new column.