Repository navigation
Conversation
Folds source-cooperative/source.coop#491 into the ADR series. About half of that epic re-derived decisions the corpus already records; this keeps what was new and cites what was not. New: - ADR-014, federated subject resolution. An account is found by (issuer, subject), not a bare subject. Answers the organisation-subject question ADR-010 defers, and closes a collision where two issuers share one subject namespace — which also keys the proxy's cache. - ADR-015, service accounts. A principal for unattended software, with an owner, many sign-in methods, and grants through ordinary memberships. - ADR-016, delete as an action distinct from write. No layer can express "may upload, may not delete" today. Amended: - ADR-013 keeps its JWT mechanism but drops optional expiry (undeliverable on a two-key JWKS), moves to a dedicated signing key rather than reusing the ADR-006 federation key, issues keys to service accounts, and records opaque keys and managed key storage as considered and rejected. - ADR-009 marks issuer namespacing a precondition rather than a follow-up, and states that empty audience and subject lists must mean deny. - ADR-010 points the organisation-subject problem at ADR-014 and adds a _read_only built-in. - ADR-004 corrects the claim that aws-actions/configure-aws-credentials works; it calls GetCallerIdentity, which is not implemented. - RFC-001 future work and decision index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGZ598AJ1F7NWoyPpgepBt
Contributor
|
Claude finished @alukach's task in 16s —— View job ❌ Changes requested — see findings below. The PR now adds only
The substance is sound. It keeps Simplify (ponytail)
💰 Estimated review cost: $0.11 · 0m15s · 5 turns |
|
🚀 Latest commit deployed to https://source-data-proxy-pr-219.source-coop.workers.dev
|
Out of scope for ADR-004, which specifies the exchange rather than the compatibility of any one client. The gap is already tracked in #184 and developmentseed/multistore#126. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGZ598AJ1F7NWoyPpgepBt
Two numbers carried over from source.coop#491 were wrong. The account-type branch sites are ~48 across 20 files, not 47 across 19, and the binary isIndividualAccount / isOrganizationalAccount helpers are slightly under half of them rather than the majority. A sealed ceiling goes stale for up to a session, and production configures STS_MAX_SESSION_DURATION_SECS at 43200 — 12 hours, not the one hour the multistore default suggests. That is the number that matters for how long a revoked delete keeps working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGZ598AJ1F7NWoyPpgepBt
`_default` and `_read_only` read as internal identifiers in a user-facing UI. Name the built-ins `FullAccess` and `ReadOnly` instead. The leading underscore was doing real work — it kept built-in names out of the account-authorable namespace. Uppercase does the same job, because account-authored Role names are validated lowercase-only, so `FullAccess` cannot be created through the API either. `_default` stays accepted as an alias. It is not just a name in this document: it is in deployed client configuration as `AWS_ROLE_ARN=arn:aws:iam::000000000000:role/_default` and is matched by `is_default_role`, so renaming it would break every existing caller. Also merges the old "The `_default` Role, Restated" section into the new built-ins section, which had become a duplicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGZ598AJ1F7NWoyPpgepBt
Service accounts ship with FullAccess and ReadOnly hardcoded in the proxy. ADR-010 keeps the account-owned Role design on record but carries a scope note saying that Role CRUD, per-Role trust policies and user-authored permission statements are deferred. This removes the Source API lookup from the credential-minting path, and it resolves where a per-service-account list of permitted Roles lives: nowhere. A Role can only subtract, so any caller may name either one safely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGZ598AJ1F7NWoyPpgepBt
The previous wording said account-authored Role names "are validated against a lowercase pattern", present tense. Nothing validates them: Role creation is the deferred half of this ADR and was never implemented, so that code path does not exist. What is true today is simpler — the proxy knows three names and rejects everything else. The lowercase rule is a property to preserve if account-owned Roles are ever built, not a mechanism protecting the built-ins now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGZ598AJ1F7NWoyPpgepBt
Expiry lived in the token's `exp` claim and was mandatory, justified partly on the grounds that a JWKS publishes only the current and previous key, so an `exp`-less token would stop verifying at an unpredictable moment. That constraint was an artefact of the rotation policy, not of JWTs. Rotation here adds a key rather than replacing one: new keys are signed with the newest private key, and every key ever used stays published for verification under its own `kid`. Removing a key becomes the emergency lever for a suspected signing-key compromise instead of routine maintenance. With that settled, expiry moves to the `jti` record the exchange already reads for revocation. Keys may have no expiry, expiry can be changed after issuance, and expired and revoked collapse into one check returning one client-visible outcome — which the oracle argument wants anyway. Keys still default to a bounded expiry, now for the honest reason: an indefinite credential outlives the person who created it and nothing ever forces a review. "Never expires" is an explicit choice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGZ598AJ1F7NWoyPpgepBt
ADR-016 proposed a third permission value so a grant could say "may upload, never delete". Narrowed to record the opposite decision. A writer can already destroy data without DeleteObject, because PutObject overwrites. Splitting the action reads as stronger protection than it is, and it spends a public API change plus a migration on a guarantee that only holds against accidents. Roles carry a per-action list and can only subtract, so "may upload, never delete" is expressible there when account-owned Roles ship — with no change to the permission vocabulary and nothing new for a product owner to understand. Keeps two things from the original: AbortMultipartUpload must stay with write, or a writer cannot clean up its own failed uploads; and GetObjectVersion is currently misclassified as a write and should be fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGZ598AJ1F7NWoyPpgepBt
main has since landed the decisions this branch proposed, in different form: ADR-014 service accounts with per-account trusts (#232, #237), ADR-013 opaque API keys (#234, #242), and the FullAccess/ReadOnly roles (#236). Take main's ADR-009, 010, 013 and RFC-001; drop this branch's federated-subject-resolution and service-accounts ADRs. That also resolves both review findings on ADR-013 (the signing-key contradiction and the per-Role API-key opt-in), since main's text replaces them. The delete ADR has no counterpart on main and answers a decision the epic still lists as owed, so it stays as ADR-015. Its Context no longer claims GetObjectVersion is already a read, and the classifier fix now covers the ReadOnly ceiling, which omits it too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alukach
changed the title
alukach
marked this pull request as ready for review
October 6, 2026 02:43
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds ADR-015: Deletion Is Part of Write. A
writegrant permitsDeleteObject, and "may upload, never delete" is left to Roles, which can only subtract (ADR-011). There's no third permission value in the API and no grant migration.This answers one of the decisions source-cooperative/source.coop#491 lists as still owed: "Is
deletea grant a product owner sets, or only something a role can subtract?" The answer is that only a Role can subtract it. That becomes possible when account-owned Roles ship; neither built-in Role (FullAccess,ReadOnly) withholds deletion.The ADR also records a classifier bug on
main.GetObjectVersion(multistore 0.7.2+) is missing from the read list inis_write_action(src/authz.rs) and from theReadOnlyceiling (src/sts.rs). So version reads are gated as writes, and aReadOnlysession can't read a version at all. Both fail safe, and they should be fixed together.Scope
The other ADRs this PR originally proposed have landed on
mainin a different form and were dropped from it:main(issuer, subject)trusts, with the account named inRoleArn(#232, #237)sck_keys resolved by hash (#234, #242)Both points from the earlier review (ADR-013's signing-key contradiction, and its per-Role API-key opt-in, which doesn't exist while Roles are hardcoded) are resolved by
main's text. ADR-014 now says there's no per-key Role binding and that any key may name either Role.🤖 Generated with Claude Code