chore(adr): ratify the credential platform decisions before implementation - #530
Merged
Conversation
…ementation branch These decisions are contested independently of the code that implements them, and ADR numbers are a shared resource: leaving them unpublished forces every concurrent branch to renumber around work it cannot see. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryLow Risk Overview Docs wiring: Reviewed by Cursor Bugbot for commit fc0410e. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughAdded ADRs 0045–0052 and supporting guidance. The decisions define event-sourced service structure, project-scoped credential resources and metadata, plaintext exposure, revocation timing, signed authentication, request binding, and production OpenBao sealing. ChangesArchitecture decisions
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0045-event-sourced-service-module-layout.md`:
- Around line 57-58: Resolve the conflicting snapshot-policy locations in the
ADR by aligning the `snapshot` entry with the
`trogon-scheduler/commands/snapshot.rs` reference, or explicitly marking that
path as a legacy exception. Update the related layout descriptions consistently
so implementers have one unambiguous prescribed location.
- Around line 62-63: Update the ADR’s module-layout guidance around the
processor definition and its related boundary section to introduce a nested
processor/checkpoint adapter boundary. Clarify that projection logic remains
free of persistence SDKs, while the checkpoint adapter owns SDK-specific
persistence and its rebuildable store.
In `@docs/adr/0046-project-anchored-resource-hierarchy.md`:
- Around line 71-76: Update the “Public resource names are parent-scoped”
section to state that admission derives `{project}` from the authenticated
caller context and rejects signed requests when the path’s `{project}` does not
match it, optionally cross-referencing the existing authentication ADRs.
In `@docs/adr/0047-event-sourced-credential-metadata.md`:
- Around line 37-39: Update the rebuildability statement in ADR#0047 to account
for retention purging events below the snapshot-derived watermark. Clarify that
new projections rebuild from available snapshots or prior projections plus
retained stream history, or explicitly limit rebuildability to the retained
event range rather than claiming the full stream is always available.
In `@docs/adr/0048-one-time-plaintext-exposure.md`:
- Around line 44-45: Update the idempotency/KV ledger statement in the ADR to
remove the claim that it is out of scope for secret-redaction auditing. State
that the metadata-only ledger contract prohibits plaintext while explicitly
retaining the ledger’s schema and replay paths in security-test and audit
coverage, consistent with the later ADR requirements.
In `@docs/adr/0049-revocation-latency-target.md`:
- Line 43: Update the sentence in the ADR to use the hyphenated compound
modifier “5-second target” instead of “5 second target,” leaving the rest of the
sentence unchanged.
- Around line 30-33: Update the revocation latency SLO and alerting definitions
in the ADR to specify the aggregation/evaluation window for both the 5-second
target and 10-second alert, and define a minimum sample-count guard before
evaluating p99. Ensure dashboards and alerts use these same window and sample
thresholds, with low-volume periods not triggering unstable evaluations.
In `@docs/adr/0051-fully-bound-request-signing.md`:
- Around line 107-108: Update the replay-store capacity statement in the fully
bound request-signing ADR to account for the validity ceiling plus clock skew:
express the bound as request rate multiplied by 150 seconds, including record
overhead, or derive it from the configured validity and skew values.
- Around line 46-50: Make the request-target serialization normative in ADR 0051
by defining the canonical HTTP and NATS target byte rules inline, including
default ports, casing, percent-encoding, and the exact subject/operation fields.
If the API contracts remain authoritative instead, add a stable contract
reference and canonicalization test vectors covering both transports.
- Around line 32-35: Revise the security statement around full binding to say it
prevents request modification and reuse of the same token, while acknowledging
that an attacker can submit an unspent captured request first or race the
legitimate caller. Narrow the platform-breach claim to signed-key material only,
and avoid asserting that the client private key is the sole remaining system
asset or that provider plaintext exposure is eliminated.
- Around line 53-54: Update the replay-store handling for each (key_id, jti) to
use the NATS KV conditional-create operation as one atomic consume, rather than
separate read and write steps. Treat an already-present entry as a replay
rejection, and preserve fail-closed behavior for all other store-contact errors
described in section 4.
In `@docs/adr/0052-cloud-kms-production-seal.md`:
- Around line 12-17: Update the introductory key-hierarchy description to state
that stored data is encrypted by the keyring, the keyring is encrypted by the
root key, and the seal protects the root key. Revise the Shamir wording to
describe splitting the unseal key into quorum shares, without implying that the
barrier or seal stanza is encrypted by the barrier.
- Around line 19-22: Update the production auto-unseal decision in ADR 0052 to
require HSM-backed key configuration rather than treating all cloud KMS options
as equivalent. Document provider-specific requirements, including Google Cloud
KMS HSM protection and Azure Managed HSM, along with the necessary IAM and audit
configuration.
- Around line 47-59: Update the KMS-outage and Shamir-migration sections of the
ADR to define the recovery boundary: state that recovery keys cannot unseal
auto-unseal clusters, migration requires the current KMS seal to be reachable
and involves downtime, and migration is impossible after permanent loss of the
seal key even when backups exist. Clarify that migration reversal is available
only while the current seal mechanism remains accessible.
- Around line 42-43: Update ADR 0052’s key lifecycle requirements to replace
provider-default rotation with explicit AWS, Google Cloud, and Azure rotation
schedules and historical-version retention. Add documented purge-protection
settings, restart/reopen procedures after key-material changes, and the exact
multi-region failover path.
In `@rsworkspace/crates/AGENTS.md`:
- Around line 25-26: Update the event-sourced service layout guidance in
AGENTS.md so the stream domain contents are described as “value objects” only.
Remove “and events” from the domain description, leaving the rest of the module
layout unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 16a7fd80-8951-4f1a-a34f-db15f2fcc8f2
📒 Files selected for processing (10)
docs/adr/0045-event-sourced-service-module-layout.mddocs/adr/0046-project-anchored-resource-hierarchy.mddocs/adr/0047-event-sourced-credential-metadata.mddocs/adr/0048-one-time-plaintext-exposure.mddocs/adr/0049-revocation-latency-target.mddocs/adr/0050-signed-first-caller-authentication.mddocs/adr/0051-fully-bound-request-signing.mddocs/adr/0052-cloud-kms-production-seal.mddocs/adr/index.mdrsworkspace/crates/AGENTS.md
…corpus and the reference implementation Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8bbb88e. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0045-event-sourced-service-module-layout.md`:
- Around line 44-46: Clarify the terminology in the module-layout section by
explicitly defining how stream, workflow, and aggregate relate, including which
one is the crate/module unit and persisted-identifier naming axis. Update the
surrounding examples and references to use the chosen mapping consistently,
avoiding ambiguous or interchangeable terms.
- Around line 132-134: Update the “Decision and projection logic” statement to
explicitly identify the transport and persistence representations that
infrastructure adapters convert at the boundary, rather than leaving “Convert at
the boundary” implicit. Clarify that decider event and state values remain
generated protobuf types, consistent with the constraint on the decision and
projection logic.
In `@docs/adr/0049-revocation-latency-target.md`:
- Line 48: Update the consequences statement in ADR 0049 to reference the
defined checkpointed projection refresh event-driven invalidation mechanism
instead of “Outbox-driven invalidation,” or explicitly document how the outbox
path relates to broker events and projection refresh.
- Around line 31-38: Update the ADR’s revocation latency target and alerting
definitions around gateway.credential.revocation.latency to state that each
secrets-service and model-access replica emits its own histogram, with
aggregation and evaluation requiring every replica to meet the p99 target.
Define the all-replica alert behavior so a single slow replica pages when
sustained, while preserving the existing sample-floor and missing-data semantics
per replica.
In `@docs/adr/0052-cloud-kms-production-seal.md`:
- Around line 41-46: The AWS KMS requirement in the production seal guidance
must not claim all AWS KMS keys are HSM-backed by construction. Either qualify
the chain-of-trust claim to AWS KMS-generated key material only, explicitly
covering imported, CloudHSM-backed custom key store, and external key store
cases, or require production seal keys to use AWS KMS-generated material; update
the provider requirement near the GCP and Azure protection-level rules
accordingly.
In `@docs/architecture/key-custody.md`:
- Around line 49-51: Update the key-custody description around the OpenBao
root-key protection flow to state that production managed keys terminate at the
platform-controlled HSM-backed cloud KMS and deployment-attested IAM boundary
through auto-unseal. Limit quorum-held Shamir shares to air-gapped or
restricted-egress deployments and manual restarts, referencing ADR#0052.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e640da2-66cf-42c5-8bf5-c04f4bb4fed3
📒 Files selected for processing (9)
docs/adr/0045-event-sourced-service-module-layout.mddocs/adr/0046-project-anchored-resource-hierarchy.mddocs/adr/0047-event-sourced-credential-metadata.mddocs/adr/0048-one-time-plaintext-exposure.mddocs/adr/0049-revocation-latency-target.mddocs/adr/0051-fully-bound-request-signing.mddocs/adr/0052-cloud-kms-production-seal.mddocs/architecture/key-custody.mdrsworkspace/crates/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/adr/0048-one-time-plaintext-exposure.md
- rsworkspace/crates/AGENTS.md
- docs/adr/0051-fully-bound-request-signing.md
- docs/adr/0046-project-anchored-resource-hierarchy.md
…ntracts Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 4738311 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
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
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.
Summary
The credential platform had a set of open questions (metadata ownership, resource naming, caller authentication, plaintext exposure, revocation timing, production seal posture) that were being answered implicitly by whichever slice shipped first, so writing them down now keeps the implementation branch from re-litigating them or drifting apart.
Note
ADR#0045 also promotes the module layout
trogon-scheduleralready follows into a stated convention, so newer event-sourced crates start from the same shape instead of each inventing one.Related