Port stateful credstore gateway#4204
Conversation
|
Important Review skippedToo many files! This PR contains 104 files, which is 4 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (114)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR introduces a stateful CredStore gear with typed secret storage, tenant-aware authorization, REST CRUD operations, optimistic concurrency, persistence, reaper workflows, plugin contract changes, configuration updates, documentation, and E2E coverage. It also adds deferred OAGW upstream reconciliation and updates downstream CredStore test clients. ChangesCredStore implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 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: 9
🧹 Nitpick comments (9)
gears/system/oagw/oagw/src/infra/plugin/apikey_auth.rs (1)
188-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
Utf8ErrorCredStoretest double across two files.This struct and its
CredStoreClientV1impl (get/put/create/delete) are duplicated almost verbatim ingears/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rs(lines 475-523). Both copies needed the same 4-method update in this PR, which is exactly the maintenance cost duplication creates. Consider hoisting a sharedUtf8ErrorCredStoretest double intotest_support.rsso futureCredStoreClientV1trait changes only require one edit.🤖 Prompt for 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. In `@gears/system/oagw/oagw/src/infra/plugin/apikey_auth.rs` around lines 188 - 244, The Utf8ErrorCredStore test double is duplicated between ApiKeyAuthPlugin and the OAuth2 client cred auth tests, including the CredStoreClientV1 methods get/put/create/delete. Move this shared mock into test_support.rs (or another common test helper) and update both authenticate-related tests to use the single shared Utf8ErrorCredStore implementation so future trait changes only need one update.gears/mini-chat/mini-chat/src/infra/oagw_provisioning.rs (1)
155-177: 🩺 Stability & Availability | 🔵 TrivialConsider distinguishing transient vs. permanent errors when deferring.
create_or_reuse_upstreamtreats every non-AlreadyExistserror identically as "secret not accessible yet" and defers indefinitely (no attempt budget, by design). A genuinely permanent misconfiguration at the OAGW layer (e.g., a rejected host/auth config, not just a missing credstore secret) would be retried forever under the same generic "secret not provisioned" framing, with only a single one-time warn log after 2 minutes elapsed — making it hard for an operator to distinguish "waiting on a secret" from "will never succeed."🤖 Prompt for 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. In `@gears/mini-chat/mini-chat/src/infra/oagw_provisioning.rs` around lines 155 - 177, The fallback in create_or_reuse_upstream currently treats every non-AlreadyExists failure as if the provider is just waiting on a secret, which hides permanent OAGW misconfiguration. Update the error handling in create_or_reuse_upstream (and any helper it calls, such as create_upstream/reuse_existing_upstream) to distinguish transient secret-unavailable cases from permanent upstream config errors, and log them with different messages or outcomes so operators can tell “retry later” from “will not succeed” instead of always deferring under the same warning.testing/e2e/gears/credstore/test_types.py (1)
71-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a status assertion on the initial PUT in
test_type_is_immutable.The first PUT (creating the
api-keysecret) has no assertion. If it fails silently, the second PUT would create a new secret with type"generic"instead of triggeringTYPE_IMMUTABLE, producing a confusing 204-vs-400 failure rather than a clear creation error.🧪 Proposed fix: add assertion on initial PUT
await client.put( f"{secrets_url}/{ref}", headers=tenant_a_headers, json={"value": "sk-1", "sharing": "tenant", "type": "api-key"}, ) + assert resp.status_code == 204, resp.text🤖 Prompt for 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. In `@testing/e2e/gears/credstore/test_types.py` around lines 71 - 75, The initial PUT in test_type_is_immutable is missing a status check, so a failed secret creation can mask the real problem and make the later TYPE_IMMUTABLE assertion misleading. Update the first client.put call to assert the expected successful creation response before issuing the second PUT. Use the test_type_is_immutable flow and the client.put calls in test_types.py to locate the missing assertion.testing/e2e/gears/credstore/conftest.py (1)
107-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging the swallowed exception in the reachability check.
The bare
except Exception: passmakes transient failures invisible during debugging. Alogging.debugor♻️ Suggested improvement
except Exception as exc: - # Timeout or transient error — still try to run the tests. - pass + # Timeout or transient error — still try to run the tests. + import logging + logging.debug("credstore reachability probe failed (non-ConnectError): %s", exc)🤖 Prompt for 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. In `@testing/e2e/gears/credstore/conftest.py` around lines 107 - 118, The reachability check in _check_credstore_reachable currently swallows all non-ConnectError exceptions, which hides useful debugging information. Update the broad exception handler to log the caught exception details at debug level (or equivalent) before continuing with the fail-open behavior, so transient failures during the httpx.get probe can be diagnosed without changing the skip/run logic.Source: Linters/SAST tools
gears/credstore/credstore/src/infra/storage/repo_impl/reads.rs (1)
36-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
scope_err_to_domainduplicatesmap_scope_err.
scope_err_to_domainhandlesScopeError::Dbby callingclassify_db_err_to_domain, then delegates the rest tomap_scope_err. Butmap_scope_erralready handlesScopeError::Dbidentically. EveryScopeErrorvariant is mapped the same way in both functions, makingscope_err_to_domainfully redundant.Replace all
.map_err(scope_err_to_domain)calls with.map_err(map_scope_err), removescope_err_to_domain, and drop the now-unusedclassify_db_err_to_domainimport on line 15.♻️ Proposed refactor
-use crate::infra::canonical_mapping::classify_db_err_to_domain; use crate::infra::storage::entity; use crate::infra::storage::repo_impl::helpers::map_scope_err; use crate::infra::storage::repo_impl::{ SecretRepoImpl, entity_to_model, sharing_from_i16, sharing_to_i16, };-fn scope_err_to_domain(e: ScopeError) -> DomainError { - match e { - ScopeError::Db(db) => classify_db_err_to_domain(db), - other => map_scope_err(other), - } -}Then replace all
scope_err_to_domainreferences withmap_scope_errin the.map_err(...)calls.🤖 Prompt for 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. In `@gears/credstore/credstore/src/infra/storage/repo_impl/reads.rs` around lines 36 - 41, scope_err_to_domain is redundant because it mirrors map_scope_err for every ScopeError variant. Update the reads.rs repo implementation to replace all .map_err(scope_err_to_domain) usages with .map_err(map_scope_err), then remove the scope_err_to_domain helper entirely from the module. Also drop the now-unused classify_db_err_to_domain import and any remaining references tied to that helper.gears/credstore/credstore/src/infra/metrics_tests.rs (1)
14-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSmoke test missing
deprovisioning_reapedcall.The smoke test claims to exercise "every recording path" but doesn't call
m.deprovisioning_reaped(...). All other recording methods are called. Add the missing call for completeness.✨ Add missing call
m.provisioning_reaped(5); + m.deprovisioning_reaped(2); m.provisioning_rollback(Outcome::Success);🤖 Prompt for 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. In `@gears/credstore/credstore/src/infra/metrics_tests.rs` around lines 14 - 51, The smoke test in global_meter_records_all_instruments is missing coverage for the deprovisioning reaped recording path. Add a call to m.deprovisioning_reaped(...) alongside the existing m.provisioning_reaped, m.provisioning_rollback, and other metric recordings so this test truly exercises every CredStoreMetricsMeter recording method.gears/credstore/credstore/src/infra/tenant_resolver.rs (2)
90-106: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd test coverage for the resolver error path.
The
get_ancestorserror branch (returningDomainError::ServiceUnavailable) is untested. The fake client intenant_resolver_tests.rsreturnsTenantResolverError::TenantNotFoundfor unknown tenants, but no test exercises this path to verify the error mapping. Consider adding a test that callsancestor_chainwith an unknown tenant and asserts the result isDomainError::ServiceUnavailable.🤖 Prompt for 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. In `@gears/credstore/credstore/src/infra/tenant_resolver.rs` around lines 90 - 106, Add test coverage for the `TenantResolver::ancestor_chain` error branch that maps a dependency failure to `DomainError::ServiceUnavailable`. Use the existing fake client behavior in `tenant_resolver_tests.rs` by calling `ancestor_chain` with an unknown tenant so `get_ancestors` returns `TenantResolverError::TenantNotFound`, then assert the result is `ServiceUnavailable` and that the error path is exercised. Keep the test focused on the resolver’s error mapping and the `ancestor_chain`/`get_ancestors` flow.
62-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a TTL expiry test.
The
cache_hit_avoids_second_calltest verifies caching within the TTL window, but no test verifies that a cache entry expires after the TTL and triggers a new resolver call. Consider adding a test using#[tokio::test(start_paused = true)]with a short TTL to advance time past expiry and assert the fake client is called again.Also applies to: 112-133
🤖 Prompt for 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. In `@gears/credstore/credstore/src/infra/tenant_resolver.rs` around lines 62 - 72, Add a TTL expiry test for the resolver cache in TenantResolver so we verify an entry is reused only within the TTL and a fresh resolve happens after expiry. Update the test coverage around cache behavior by adding a tokio test (for example using #[tokio::test(start_paused = true)]) with a short TTL, advancing time past the threshold, and asserting the fake client’s resolve method is invoked again. Use the existing cache-hit test and TenantResolver::resolve / cache logic as the main reference points.gears/credstore/credstore/src/infra/sdk_error_mapping.rs (1)
117-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest should verify retry-after value, not just status code.
The test
service_unavailable_carries_retry_afteronly asserts the 503 status code for both theSome(30s)andNonecases. It doesn't verify that the retry-after seconds value is actually propagated to theCanonicalError. Consider asserting the retry-after value on the mapped error whenSome(duration)is provided.🤖 Prompt for 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. In `@gears/credstore/credstore/src/infra/sdk_error_mapping.rs` around lines 117 - 136, The `service_unavailable_carries_retry_after` test only checks the HTTP status from `status_of`, so it never verifies that `DomainError::ServiceUnavailable` with `retry_after: Some(...)` actually maps the retry-after seconds into the resulting `CanonicalError`. Update the test to inspect the mapped error produced by the `status_of`/mapping path and assert the retry-after value when `Some(Duration::from_secs(30))` is set, while keeping the `None` case as a separate status-only check if needed.
🤖 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 `@config/e2e-scope-enforcement.yaml`:
- Around line 28-36: The credstore config is missing the static backend plugin
registration, so add static-credstore-plugin to the credstore setup in the e2e
scope enforcement config. Update the credstore block in
config/e2e-scope-enforcement.yaml so the backend plugin is explicitly configured
alongside the existing vendor setting, matching the expected plugin name used by
credstore initialization.
In `@docs/api/api.json`:
- Around line 11088-11372: The Secret Store operations are missing the
optimistic-concurrency contract in the API spec, even though the version is
exposed as an ETag. Update the secret endpoint definitions around
credstore.get_secret, credstore.put_secret, and credstore.delete_secret to
document an ETag response header on GET, add an If-Match request header
parameter for PUT and DELETE, and include a 412 Precondition Failed response
where applicable so codegen clients can honor the version-based update/delete
flow.
- Around line 804-806: The secret reference fields currently only describe
validation rules in text, but the OpenAPI schema for reference and
CreateSecretRequestDto.reference is still unconstrained. Update the schema
definitions in api.json to add the same validation constraints directly on those
properties by setting pattern, minLength, and maxLength so generated clients
enforce the allowed secret reference format before requests are sent.
- Around line 797-802: Add the date-time format to the expires_at schema
wherever it is defined so it matches the RFC 3339 instant description and aligns
with the other timestamp fields. Update the schema entries in the API spec for
expires_at in the affected definitions, preserving the existing string/null
typing while adding the appropriate format so generated clients and validation
treat it as a timestamp. Use the expires_at property definitions in the API
schema as the reference points.
In `@gears/credstore/credstore-sdk/README.md`:
- Around line 9-11: The README section describing CredStoreClientV1’s get
response is incomplete; update the response metadata list to include the full
GetSecretResponse contract. In the CredStoreClientV1 description, add the
missing fields id, secret_type, and expires_at alongside owner_tenant_id,
sharing, is_inherited, and version so SDK consumers see the complete surface.
In `@gears/credstore/credstore-sdk/src/error.rs`:
- Around line 77-90: `Error::is_retryable` is currently inheriting
`NoPluginAvailable` through `is_unavailable`, which incorrectly marks an
operator misconfiguration as retryable. Update the `Error` helpers in `error.rs`
so `NoPluginAvailable` is excluded from the retryable path, either by removing
it from `is_unavailable()` or by splitting the “unavailable” and “retryable”
checks and having `is_retryable()` only return true for genuinely transient
variants like `ServiceUnavailable`.
In `@gears/credstore/credstore-sdk/src/models.rs`:
- Around line 182-185: The expiry field in WriteOptions currently uses
Option<time::OffsetDateTime>, which makes None mean both “not provided” and
“clear expiry,” causing accidental expiry removal in WriteOptions::default(),
SDK put(), and REST writes. Update the WriteOptions/expires_at modeling in
models.rs and the related put/write flow to use explicit tri-state semantics
(preserve, set, clear) so callers can omit expiry without changing the existing
stored value, and only clear it when explicitly requested.
In `@gears/credstore/docs/PRD.md`:
- Around line 369-370: Update the PRD wording for the optimistic concurrency
contract so `If-Match` uses the generation-bound ETag shape instead of a bare
version; in the GET/PUT/DELETE section and the UC-005 scenario, replace the
shorthand `"<version>"` with `"<id>.<version>"` and keep the existing `*`
behavior and conflict/validation semantics unchanged. Make sure the text
consistently reflects the gateway/E2E contract and the optimistic lock flow
described around the secret versioning and `If-Match` handling.
In `@gears/mini-chat/mini-chat/src/infra/oagw_provisioning.rs`:
- Around line 82-129: The tenant override handling in register_provider is doing
validation and upstream creation in the same HashMap iteration, which can leave
some valid overrides partially registered before a misconfigured one aborts the
loop. Move the misconfiguration check for each tenant override to an upfront
pass before any call to create_or_reuse_tenant_upstream, then only proceed to
registration if all overrides are valid. Keep the existing register_provider
flow and create_or_reuse_tenant_upstream call, but separate deterministic
validation from side-effecting registration so no valid override is skipped
based on iteration order.
---
Nitpick comments:
In `@gears/credstore/credstore/src/infra/metrics_tests.rs`:
- Around line 14-51: The smoke test in global_meter_records_all_instruments is
missing coverage for the deprovisioning reaped recording path. Add a call to
m.deprovisioning_reaped(...) alongside the existing m.provisioning_reaped,
m.provisioning_rollback, and other metric recordings so this test truly
exercises every CredStoreMetricsMeter recording method.
In `@gears/credstore/credstore/src/infra/sdk_error_mapping.rs`:
- Around line 117-136: The `service_unavailable_carries_retry_after` test only
checks the HTTP status from `status_of`, so it never verifies that
`DomainError::ServiceUnavailable` with `retry_after: Some(...)` actually maps
the retry-after seconds into the resulting `CanonicalError`. Update the test to
inspect the mapped error produced by the `status_of`/mapping path and assert the
retry-after value when `Some(Duration::from_secs(30))` is set, while keeping the
`None` case as a separate status-only check if needed.
In `@gears/credstore/credstore/src/infra/storage/repo_impl/reads.rs`:
- Around line 36-41: scope_err_to_domain is redundant because it mirrors
map_scope_err for every ScopeError variant. Update the reads.rs repo
implementation to replace all .map_err(scope_err_to_domain) usages with
.map_err(map_scope_err), then remove the scope_err_to_domain helper entirely
from the module. Also drop the now-unused classify_db_err_to_domain import and
any remaining references tied to that helper.
In `@gears/credstore/credstore/src/infra/tenant_resolver.rs`:
- Around line 90-106: Add test coverage for the `TenantResolver::ancestor_chain`
error branch that maps a dependency failure to
`DomainError::ServiceUnavailable`. Use the existing fake client behavior in
`tenant_resolver_tests.rs` by calling `ancestor_chain` with an unknown tenant so
`get_ancestors` returns `TenantResolverError::TenantNotFound`, then assert the
result is `ServiceUnavailable` and that the error path is exercised. Keep the
test focused on the resolver’s error mapping and the
`ancestor_chain`/`get_ancestors` flow.
- Around line 62-72: Add a TTL expiry test for the resolver cache in
TenantResolver so we verify an entry is reused only within the TTL and a fresh
resolve happens after expiry. Update the test coverage around cache behavior by
adding a tokio test (for example using #[tokio::test(start_paused = true)]) with
a short TTL, advancing time past the threshold, and asserting the fake client’s
resolve method is invoked again. Use the existing cache-hit test and
TenantResolver::resolve / cache logic as the main reference points.
In `@gears/mini-chat/mini-chat/src/infra/oagw_provisioning.rs`:
- Around line 155-177: The fallback in create_or_reuse_upstream currently treats
every non-AlreadyExists failure as if the provider is just waiting on a secret,
which hides permanent OAGW misconfiguration. Update the error handling in
create_or_reuse_upstream (and any helper it calls, such as
create_upstream/reuse_existing_upstream) to distinguish transient
secret-unavailable cases from permanent upstream config errors, and log them
with different messages or outcomes so operators can tell “retry later” from
“will not succeed” instead of always deferring under the same warning.
In `@gears/system/oagw/oagw/src/infra/plugin/apikey_auth.rs`:
- Around line 188-244: The Utf8ErrorCredStore test double is duplicated between
ApiKeyAuthPlugin and the OAuth2 client cred auth tests, including the
CredStoreClientV1 methods get/put/create/delete. Move this shared mock into
test_support.rs (or another common test helper) and update both
authenticate-related tests to use the single shared Utf8ErrorCredStore
implementation so future trait changes only need one update.
In `@testing/e2e/gears/credstore/conftest.py`:
- Around line 107-118: The reachability check in _check_credstore_reachable
currently swallows all non-ConnectError exceptions, which hides useful debugging
information. Update the broad exception handler to log the caught exception
details at debug level (or equivalent) before continuing with the fail-open
behavior, so transient failures during the httpx.get probe can be diagnosed
without changing the skip/run logic.
In `@testing/e2e/gears/credstore/test_types.py`:
- Around line 71-75: The initial PUT in test_type_is_immutable is missing a
status check, so a failed secret creation can mask the real problem and make the
later TYPE_IMMUTABLE assertion misleading. Update the first client.put call to
assert the expected successful creation response before issuing the second PUT.
Use the test_type_is_immutable flow and the client.put calls in test_types.py to
locate the missing assertion.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5257c98f-fdf0-4d02-a62f-69dfde4983a6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
config/e2e-local.yamlconfig/e2e-scope-enforcement.yamlconfig/e2e-tr-authz.yamlconfig/mini-chat.yamlconfig/quickstart-windows.yamlconfig/quickstart.yamlconfig/static-tenants.yamldocs/api/api.jsongears/credstore/credstore-sdk/Cargo.tomlgears/credstore/credstore-sdk/README.mdgears/credstore/credstore-sdk/src/api.rsgears/credstore/credstore-sdk/src/error.rsgears/credstore/credstore-sdk/src/error_tests.rsgears/credstore/credstore-sdk/src/gts.rsgears/credstore/credstore-sdk/src/gts_tests.rsgears/credstore/credstore-sdk/src/lib.rsgears/credstore/credstore-sdk/src/models.rsgears/credstore/credstore-sdk/src/models_tests.rsgears/credstore/credstore-sdk/src/plugin_api.rsgears/credstore/credstore-sdk/src/types.rsgears/credstore/credstore-sdk/src/types_tests.rsgears/credstore/credstore/Cargo.tomlgears/credstore/credstore/README.mdgears/credstore/credstore/src/api.rsgears/credstore/credstore/src/api/rest.rsgears/credstore/credstore/src/api/rest/dto.rsgears/credstore/credstore/src/api/rest/dto_tests.rsgears/credstore/credstore/src/api/rest/handlers.rsgears/credstore/credstore/src/api/rest/routes.rsgears/credstore/credstore/src/api/rest/routes_tests.rsgears/credstore/credstore/src/client.rsgears/credstore/credstore/src/client_tests.rsgears/credstore/credstore/src/config.rsgears/credstore/credstore/src/config_tests.rsgears/credstore/credstore/src/domain.rsgears/credstore/credstore/src/domain/authz.rsgears/credstore/credstore/src/domain/error.rsgears/credstore/credstore/src/domain/error_tests.rsgears/credstore/credstore/src/domain/local_client.rsgears/credstore/credstore/src/domain/local_client_tests.rsgears/credstore/credstore/src/domain/mod.rsgears/credstore/credstore/src/domain/ports.rsgears/credstore/credstore/src/domain/ports/metrics.rsgears/credstore/credstore/src/domain/ports/plugin.rsgears/credstore/credstore/src/domain/resolver.rsgears/credstore/credstore/src/domain/secret.rsgears/credstore/credstore/src/domain/secret/fence.rsgears/credstore/credstore/src/domain/secret/model.rsgears/credstore/credstore/src/domain/secret/repo.rsgears/credstore/credstore/src/domain/secret/service.rsgears/credstore/credstore/src/domain/secret/service_tests.rsgears/credstore/credstore/src/domain/secret/test_support.rsgears/credstore/credstore/src/domain/secret/type_resolver.rsgears/credstore/credstore/src/domain/secret/typing.rsgears/credstore/credstore/src/domain/secret/typing_tests.rsgears/credstore/credstore/src/domain/service.rsgears/credstore/credstore/src/domain/service_tests.rsgears/credstore/credstore/src/domain/test_support.rsgears/credstore/credstore/src/gear.rsgears/credstore/credstore/src/infra.rsgears/credstore/credstore/src/infra/canonical_mapping.rsgears/credstore/credstore/src/infra/error_conv.rsgears/credstore/credstore/src/infra/metrics.rsgears/credstore/credstore/src/infra/metrics_tests.rsgears/credstore/credstore/src/infra/plugin_select.rsgears/credstore/credstore/src/infra/sdk_error_mapping.rsgears/credstore/credstore/src/infra/storage.rsgears/credstore/credstore/src/infra/storage/entity.rsgears/credstore/credstore/src/infra/storage/entity/secrets.rsgears/credstore/credstore/src/infra/storage/entity/tenant_closure.rsgears/credstore/credstore/src/infra/storage/migrations.rsgears/credstore/credstore/src/infra/storage/migrations/m0001_initial_schema.rsgears/credstore/credstore/src/infra/storage/repo_impl.rsgears/credstore/credstore/src/infra/storage/repo_impl/helpers.rsgears/credstore/credstore/src/infra/storage/repo_impl/reads.rsgears/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rsgears/credstore/credstore/src/infra/storage/repo_impl/writes.rsgears/credstore/credstore/src/infra/tenant_resolver.rsgears/credstore/credstore/src/infra/tenant_resolver_tests.rsgears/credstore/credstore/src/infra/types_registry.rsgears/credstore/credstore/src/infra/types_registry_tests.rsgears/credstore/credstore/src/lib.rsgears/credstore/docs/ADR/0001-cpt-cf-credstore-adr-stateful-gateway.mdgears/credstore/docs/ADR/0002-cpt-cf-credstore-adr-deprovisioning-saga.mdgears/credstore/docs/ADR/0003-cpt-cf-credstore-adr-value-fingerprint-fence.mdgears/credstore/docs/DESIGN.mdgears/credstore/docs/PRD.mdgears/credstore/plugins/static-credstore-plugin/README.mdgears/credstore/plugins/static-credstore-plugin/src/domain/client.rsgears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rsgears/mini-chat/mini-chat/src/gear.rsgears/mini-chat/mini-chat/src/infra/oagw_provisioning.rsgears/system/oagw/oagw/src/domain/test_support.rsgears/system/oagw/oagw/src/infra/plugin/apikey_auth.rsgears/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rsgears/system/oagw/oagw/src/infra/proxy/service.rstesting/e2e/gears/credstore/__init__.pytesting/e2e/gears/credstore/conftest.pytesting/e2e/gears/credstore/test_concurrency.pytesting/e2e/gears/credstore/test_crud.pytesting/e2e/gears/credstore/test_sharing.pytesting/e2e/gears/credstore/test_types.pytesting/e2e/gears/mini_chat/config/base.yamltesting/e2e/gears/mini_chat/conftest.pytesting/e2e/gears/oagw/conftest.py
💤 Files with no reviewable changes (10)
- gears/credstore/credstore-sdk/src/models_tests.rs
- gears/credstore/credstore/src/domain/local_client_tests.rs
- gears/credstore/credstore/src/domain/local_client.rs
- gears/credstore/credstore/src/domain/mod.rs
- gears/credstore/credstore/src/domain/service_tests.rs
- gears/credstore/credstore-sdk/src/error_tests.rs
- gears/credstore/credstore/src/domain/test_support.rs
- gears/credstore/credstore/src/domain/error_tests.rs
- gears/credstore/credstore/src/config_tests.rs
- gears/credstore/credstore/src/domain/service.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
gears/credstore/docs/PRD.md (1)
125-125: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGlossary entry for
Version / ETagdoesn't reflect the generation-bound ETag shape.ADR-0003 §Decision Outcome specifies the
ETagas the generation-bound pair"<row-id>.<version>"to close the ABA lost-update. The glossary entry here describes it as only a "Monotonic per-secret counter," which is incomplete and could mislead readers into thinking the bare version is the full validator.📝 Proposed fix
-| Version / `ETag` | Monotonic per-secret counter used for optimistic concurrency via HTTP `If-Match` | +| Version / `ETag` | Monotonic per-secret version counter; the `ETag` is the generation-bound pair `"<id>.<version>"` used for optimistic concurrency via HTTP `If-Match` |🤖 Prompt for 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. In `@gears/credstore/docs/PRD.md` at line 125, Update the `Version / ETag` glossary entry in `PRD.md` so it matches the generation-bound validator defined by ADR-0003: describe `ETag` as the pair `"<row-id>.<version>"` rather than only a monotonic per-secret counter. Keep the optimistic concurrency/`If-Match` context, but make it clear that the version is only one part of the full ETag shape used by the credstore flow.
🤖 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 @.github/workflows/cfs.yml:
- Around line 73-74: The changed-files detection step in the workflow is still
able to hard-fail because the unguarded `git fetch` runs before the protected
`git diff` in the same shell. Update the fetch logic in the workflow job so it
is fault-tolerant like the rest of the step, either by handling fetch failures
explicitly or by guarding the `git fetch --no-tags origin "$base_ref"` call so
the step continues and `changed_files` can still be computed. Keep the behavior
aligned with the existing detection block that uses `base_ref` and
`changed_files`.
In `@gears/credstore/docs/PRD.md`:
- Line 776: The traceability section is missing ADR-0003, so update the ADRs
list in the PRD traceability section to include the new value-fingerprint fence
ADR alongside ADR-0001 and ADR-0002. Use the existing ADR references in the same
section and add the ADR-0003 entry so the
`cpt-cf-credstore-fr-optimistic-concurrency` and
`cpt-cf-credstore-nfr-confidentiality` links are fully traced.
---
Duplicate comments:
In `@gears/credstore/docs/PRD.md`:
- Line 125: Update the `Version / ETag` glossary entry in `PRD.md` so it matches
the generation-bound validator defined by ADR-0003: describe `ETag` as the pair
`"<row-id>.<version>"` rather than only a monotonic per-secret counter. Keep the
optimistic concurrency/`If-Match` context, but make it clear that the version is
only one part of the full ETag shape used by the credstore flow.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5a4f5b2-264c-47df-a32d-6b9141f34b70
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (108)
.github/workflows/cfs.ymlconfig/e2e-local.yamlconfig/e2e-scope-enforcement.yamlconfig/e2e-tr-authz.yamlconfig/mini-chat.yamlconfig/quickstart-windows.yamlconfig/quickstart.yamlconfig/static-tenants.yamldocs/api/api.jsongears/credstore/credstore-sdk/Cargo.tomlgears/credstore/credstore-sdk/README.mdgears/credstore/credstore-sdk/src/api.rsgears/credstore/credstore-sdk/src/error.rsgears/credstore/credstore-sdk/src/error_tests.rsgears/credstore/credstore-sdk/src/gts.rsgears/credstore/credstore-sdk/src/gts_tests.rsgears/credstore/credstore-sdk/src/lib.rsgears/credstore/credstore-sdk/src/models.rsgears/credstore/credstore-sdk/src/models_tests.rsgears/credstore/credstore-sdk/src/plugin_api.rsgears/credstore/credstore-sdk/src/types.rsgears/credstore/credstore-sdk/src/types_tests.rsgears/credstore/credstore/Cargo.tomlgears/credstore/credstore/README.mdgears/credstore/credstore/src/api.rsgears/credstore/credstore/src/api/rest.rsgears/credstore/credstore/src/api/rest/dto.rsgears/credstore/credstore/src/api/rest/dto_tests.rsgears/credstore/credstore/src/api/rest/handlers.rsgears/credstore/credstore/src/api/rest/routes.rsgears/credstore/credstore/src/api/rest/routes_tests.rsgears/credstore/credstore/src/client.rsgears/credstore/credstore/src/client_tests.rsgears/credstore/credstore/src/config.rsgears/credstore/credstore/src/config_tests.rsgears/credstore/credstore/src/domain.rsgears/credstore/credstore/src/domain/authz.rsgears/credstore/credstore/src/domain/error.rsgears/credstore/credstore/src/domain/error_tests.rsgears/credstore/credstore/src/domain/local_client.rsgears/credstore/credstore/src/domain/local_client_tests.rsgears/credstore/credstore/src/domain/mod.rsgears/credstore/credstore/src/domain/ports.rsgears/credstore/credstore/src/domain/ports/metrics.rsgears/credstore/credstore/src/domain/ports/plugin.rsgears/credstore/credstore/src/domain/resolver.rsgears/credstore/credstore/src/domain/secret.rsgears/credstore/credstore/src/domain/secret/fence.rsgears/credstore/credstore/src/domain/secret/model.rsgears/credstore/credstore/src/domain/secret/repo.rsgears/credstore/credstore/src/domain/secret/service.rsgears/credstore/credstore/src/domain/secret/service_tests.rsgears/credstore/credstore/src/domain/secret/test_support.rsgears/credstore/credstore/src/domain/secret/type_resolver.rsgears/credstore/credstore/src/domain/secret/typing.rsgears/credstore/credstore/src/domain/secret/typing_tests.rsgears/credstore/credstore/src/domain/service.rsgears/credstore/credstore/src/domain/service_tests.rsgears/credstore/credstore/src/domain/test_support.rsgears/credstore/credstore/src/gear.rsgears/credstore/credstore/src/infra.rsgears/credstore/credstore/src/infra/canonical_mapping.rsgears/credstore/credstore/src/infra/error_conv.rsgears/credstore/credstore/src/infra/metrics.rsgears/credstore/credstore/src/infra/metrics_tests.rsgears/credstore/credstore/src/infra/plugin_select.rsgears/credstore/credstore/src/infra/sdk_error_mapping.rsgears/credstore/credstore/src/infra/storage.rsgears/credstore/credstore/src/infra/storage/entity.rsgears/credstore/credstore/src/infra/storage/entity/secrets.rsgears/credstore/credstore/src/infra/storage/entity/tenant_closure.rsgears/credstore/credstore/src/infra/storage/migrations.rsgears/credstore/credstore/src/infra/storage/migrations/m0001_initial_schema.rsgears/credstore/credstore/src/infra/storage/repo_impl.rsgears/credstore/credstore/src/infra/storage/repo_impl/helpers.rsgears/credstore/credstore/src/infra/storage/repo_impl/reads.rsgears/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rsgears/credstore/credstore/src/infra/storage/repo_impl/writes.rsgears/credstore/credstore/src/infra/tenant_resolver.rsgears/credstore/credstore/src/infra/tenant_resolver_tests.rsgears/credstore/credstore/src/infra/types_registry.rsgears/credstore/credstore/src/infra/types_registry_tests.rsgears/credstore/credstore/src/lib.rsgears/credstore/docs/ADR/0001-cpt-cf-credstore-adr-stateful-gateway.mdgears/credstore/docs/ADR/0002-cpt-cf-credstore-adr-deprovisioning-saga.mdgears/credstore/docs/ADR/0003-cpt-cf-credstore-adr-value-fingerprint-fence.mdgears/credstore/docs/DESIGN.mdgears/credstore/docs/PRD.mdgears/credstore/plugins/static-credstore-plugin/README.mdgears/credstore/plugins/static-credstore-plugin/src/domain/client.rsgears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rsgears/mini-chat/mini-chat/src/gear.rsgears/mini-chat/mini-chat/src/infra/oagw_provisioning.rsgears/system/oagw/oagw/src/domain/test_support.rsgears/system/oagw/oagw/src/infra/plugin/apikey_auth.rsgears/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rsgears/system/oagw/oagw/src/infra/proxy/service.rstesting/e2e/gears/credstore/__init__.pytesting/e2e/gears/credstore/conftest.pytesting/e2e/gears/credstore/test_concurrency.pytesting/e2e/gears/credstore/test_crud.pytesting/e2e/gears/credstore/test_sharing.pytesting/e2e/gears/credstore/test_types.pytesting/e2e/gears/mini_chat/config/base.yamltesting/e2e/gears/mini_chat/conftest.pytesting/e2e/gears/oagw/conftest.py
💤 Files with no reviewable changes (10)
- gears/credstore/credstore-sdk/src/error_tests.rs
- gears/credstore/credstore/src/domain/test_support.rs
- gears/credstore/credstore/src/config_tests.rs
- gears/credstore/credstore/src/domain/service_tests.rs
- gears/credstore/credstore-sdk/src/models_tests.rs
- gears/credstore/credstore/src/domain/service.rs
- gears/credstore/credstore/src/domain/local_client.rs
- gears/credstore/credstore/src/domain/error_tests.rs
- gears/credstore/credstore/src/domain/local_client_tests.rs
- gears/credstore/credstore/src/domain/mod.rs
✅ Files skipped from review due to trivial changes (7)
- gears/credstore/credstore/src/infra/storage/entity.rs
- gears/credstore/credstore/src/domain/ports.rs
- gears/credstore/credstore/src/domain/secret.rs
- config/e2e-scope-enforcement.yaml
- gears/credstore/credstore/README.md
- gears/credstore/credstore-sdk/README.md
- gears/credstore/credstore-sdk/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (80)
- gears/credstore/credstore/src/api.rs
- gears/credstore/credstore/src/infra/storage.rs
- gears/credstore/credstore/src/domain/ports/plugin.rs
- gears/credstore/credstore/src/infra/storage/entity/tenant_closure.rs
- gears/credstore/credstore/src/infra.rs
- config/mini-chat.yaml
- gears/credstore/credstore/src/domain.rs
- gears/credstore/credstore-sdk/src/gts_tests.rs
- gears/credstore/credstore/src/client_tests.rs
- gears/credstore/credstore/src/infra/storage/migrations.rs
- gears/credstore/credstore/src/api/rest.rs
- config/static-tenants.yaml
- config/quickstart.yaml
- gears/credstore/credstore/src/infra/storage/repo_impl/helpers.rs
- config/e2e-local.yaml
- gears/credstore/credstore/src/infra/sdk_error_mapping.rs
- gears/system/oagw/oagw/src/infra/proxy/service.rs
- gears/credstore/credstore/src/lib.rs
- gears/credstore/credstore/src/domain/resolver.rs
- testing/e2e/gears/credstore/test_crud.py
- gears/system/oagw/oagw/src/domain/test_support.rs
- gears/credstore/credstore/src/infra/error_conv.rs
- gears/credstore/credstore/src/api/rest/dto_tests.rs
- testing/e2e/gears/mini_chat/config/base.yaml
- config/quickstart-windows.yaml
- gears/credstore/credstore/src/infra/tenant_resolver.rs
- gears/credstore/credstore/src/infra/types_registry_tests.rs
- gears/credstore/credstore/src/infra/storage/entity/secrets.rs
- gears/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rs
- gears/credstore/credstore/src/domain/secret/type_resolver.rs
- config/e2e-tr-authz.yaml
- gears/credstore/credstore/src/infra/canonical_mapping.rs
- gears/credstore/credstore/src/infra/metrics_tests.rs
- gears/credstore/credstore/src/client.rs
- gears/credstore/credstore/src/api/rest/routes.rs
- gears/mini-chat/mini-chat/src/gear.rs
- gears/credstore/credstore/src/domain/authz.rs
- gears/credstore/credstore-sdk/src/types_tests.rs
- gears/credstore/credstore/Cargo.toml
- gears/credstore/credstore/src/domain/secret/typing.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rs
- gears/system/oagw/oagw/src/infra/plugin/apikey_auth.rs
- gears/credstore/credstore-sdk/src/lib.rs
- gears/credstore/credstore/src/domain/ports/metrics.rs
- gears/credstore/credstore-sdk/src/plugin_api.rs
- gears/credstore/credstore/src/domain/secret/model.rs
- gears/credstore/credstore/src/domain/secret/typing_tests.rs
- gears/credstore/credstore/src/domain/secret/repo.rs
- gears/credstore/credstore/src/infra/types_registry.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/client.rs
- gears/credstore/credstore-sdk/src/api.rs
- testing/e2e/gears/credstore/test_concurrency.py
- gears/credstore/credstore/src/infra/storage/migrations/m0001_initial_schema.rs
- gears/credstore/credstore/src/infra/tenant_resolver_tests.rs
- gears/credstore/credstore/src/infra/storage/repo_impl/reads.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rs
- testing/e2e/gears/credstore/test_types.py
- gears/credstore/credstore/src/domain/error.rs
- gears/credstore/credstore/src/api/rest/routes_tests.rs
- gears/credstore/credstore/src/infra/plugin_select.rs
- gears/credstore/credstore/src/config.rs
- gears/credstore/credstore-sdk/src/error.rs
- gears/credstore/credstore/src/infra/metrics.rs
- gears/credstore/credstore-sdk/src/types.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/service.rs
- gears/credstore/credstore/src/domain/secret/fence.rs
- gears/credstore/credstore-sdk/src/models.rs
- gears/credstore/credstore/src/api/rest/handlers.rs
- gears/credstore/credstore/src/domain/secret/test_support.rs
- gears/credstore/credstore/src/infra/storage/repo_impl.rs
- gears/credstore/credstore/src/infra/storage/repo_impl/writes.rs
- gears/credstore/plugins/static-credstore-plugin/README.md
- testing/e2e/gears/credstore/test_sharing.py
- docs/api/api.json
- gears/credstore/credstore/src/api/rest/dto.rs
- gears/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rs
- gears/credstore/credstore-sdk/src/gts.rs
- gears/credstore/credstore/src/domain/secret/service.rs
- gears/credstore/credstore/src/gear.rs
- gears/mini-chat/mini-chat/src/infra/oagw_provisioning.rs
2ac2c27 to
3e40f03
Compare
| return Ok(false); | ||
| } | ||
| // Fast path: direct UUID match via Eq/In filters. | ||
| if scope.contains_uuid(pep_properties::OWNER_TENANT_ID, tenant) { |
There was a problem hiding this comment.
MAJOR
scope_includes_tenant is the sole enforcement of the PDP AccessScope on the read/write paths (resolve_for_get/find_own/find_for_write all query with allow_all), yet it only interprets OWNER_TENANT_ID Eq/In (this fast path) and InTenantSubtree filters — any other filter in a returned constraint (owner_id, resource_id, InGroup) and any sibling AND-filter within the same constraint are silently ignored.
Because a ScopeConstraint's filters are AND-ed, a scope such as OWNER_TENANT_ID = T AND owner_id = X returns true here on the tenant match alone, authorizing the secret for the whole tenant even though the PDP decision was narrower — a scope that is stricter than tenant granularity is not enforced.
Apply .scope_with(scope) to the authorization query so the full constraint is evaluated in SQL, or make scope_includes_tenant fail closed (return false) when it encounters any filter type it does not understand, instead of reducing every constraint to its tenant predicate.
There was a problem hiding this comment.
Fixed — the gate now fails closed: a constraint admits a tenant only when every filter in
it is an OWNER_TENANT_ID predicate satisfied by that tenant; any sub-tenant sibling
(owner_id/resource_id/group) or unrecognized filter makes the constraint non-admitting
(constraints still OR-ed). It stays a tenant-membership gate (row + type are resolved before
the PDP scope, and GET authorizes on the caller's tenant for inherited secrets), so
.scope_with can't replace it. Added tests for the sibling-owner_id, non-tenant-property,
and OR-alternative cases.
| /// `domain::authz::secret_type_resource(gts_id)` builds the per-operation PEP | ||
| /// `ResourceType` from the type resolved out of the types-registry, and the | ||
| /// resolver checks a type descends from this base id. | ||
| pub const SECRET_RESOURCE_TYPE: &str = "gts.cf.core.credstore.secret.v1~"; |
There was a problem hiding this comment.
Please use gts_id! macros everywhere from toolkit-gts crate.
There was a problem hiding this comment.
Done — every hard-coded "gts…" literal now goes through gts_id!("<suffix>") (or
format!("{GTS_ID_PREFIX}…") for the two intentionally-invalid test strings). Clean under
the DE0904 no_hardcoded_gts_prefix dylint.
|
|
||
| CredStore provides per-tenant secret storage and retrieval for the platform. It abstracts backend differences behind a unified API, enabling platform gears to store and access credentials without coupling to a specific storage technology. | ||
| CredStore provides per-tenant secret storage and retrieval for the platform. | ||
| The gateway owns all secret metadata (identity, sharing, ownership, lifecycle |
There was a problem hiding this comment.
Let's don't use new-lines within the same paragraph - it's hard to read..
There was a problem hiding this comment.
Done — reflowed the PRD to one line per paragraph (folded in with the C4 pass).
| 4. Walk-up exhausted — system returns not-found error (404) | ||
| 1. OAGW resolves `internal-admin-key` for the customer | ||
| 2. The resolution query only matches private rows owned by the requesting subject; PartnerAdmin's row is invisible to OAGW | ||
| 3. No row matches → 404 |
There was a problem hiding this comment.
Looks like the current PRD (as almost all our PRDs) is overloaded with technical details. For example, I'm not sure that we need to describe here
- what HTTP codes should be returned and in what cases
- what endpoints (concrete HTTP methods + paths) should gear contains
- etc.
Drawback of such approach is duplication: all technical details are already described in DESIGN.md.
Perhaps not it's a good point to simplify PRD.md doc.
There was a problem hiding this comment.
Agreed — stripped the transport/mechanism detail from the PRD (HTTP status codes, concrete
methods/paths, ETag/If-Match/Cache-Control/Location) and kept it at the WHAT/WHY
level with capability/outcome language; the concrete contract lives in DESIGN.md. Requirement
IDs, actors, and rationale unchanged. Added a PRD authoring note to keep mechanism detail in
DESIGN going forward.
| }; | ||
| } | ||
|
|
||
| submit_secret_type_schema!( |
There was a problem hiding this comment.
gts_type_schema macro submits type schemas to inventory automatically.
There was a problem hiding this comment.
Done — dropped the hand-rolled secret_type_schema_json builder and the
submit_secret_type_schema! macro; the 10 derived types are now
#[gts_type_schema(base = SecretV1, …)] unit structs, so the macro emits and submits their
schemas automatically. Traits still come from SECRET_TYPE_CATALOG (single source of truth),
pinned by every_catalog_type_has_a_registered_schema_with_matching_traits. Tests green,
dylint clean.
| /// Derived-type schema JSON for a catalog entry, in the same draft-07 | ||
| /// `allOf`/`$ref` shape the `gts_type_schema` macro generates, plus | ||
| /// `x-gts-traits` mirroring the catalog descriptor. | ||
| fn secret_type_schema_json(gts_id: &str) -> String { |
There was a problem hiding this comment.
secret_type_schema_json looks redundant as well (please check https://github.com/constructorfabric/gears-rust/pull/4204/changes#r3587894438)
There was a problem hiding this comment.
Done — dropped the hand-rolled secret_type_schema_json builder and the
submit_secret_type_schema! macro; the 10 derived types are now
#[gts_type_schema(base = SecretV1, …)] unit structs, so the macro emits and submits their
schemas automatically. Traits still come from SECRET_TYPE_CATALOG (single source of truth),
pinned by every_catalog_type_has_a_registered_schema_with_matching_traits. Tests green,
dylint clean.
| /// is preserved ([`ExpiryWrite::Preserve`](crate::ExpiryWrite::Preserve)), | ||
| /// so a value rotation never strips an existing expiry. Use | ||
| /// [`Self::put_opts`] to set or clear it explicitly. | ||
| async fn put( |
There was a problem hiding this comment.
Please correct me if I'm wrong, but looks like an optimistic lock (version field, mentioned in the DESIGN.md) should appear somewhere here, right?
There was a problem hiding this comment.
Right, it was asymmetric — version was read-exposed but the write precondition existed only
on REST (If-Match). Added WritePrecondition { Exists, Matches { id, version } } to the
SDK, an optional precondition on WriteOptions, and delete_opts(…, precondition) (with
delete delegating); the local client maps them to the domain WritePrecondition the service
already honours. Added a client test (stale validator → Conflict, current → success).
| /// `rotation_period_secs` (advisory) and `expirable`, which additionally | ||
| /// gates reads (an expired secret resolves as not-found). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct SecretTypeDescriptor { |
There was a problem hiding this comment.
Did you consider using #[derive(JsonSchema, serde::Serialize, GtsTraitsSchema)] here?
Please check an example: https://github.com/GlobalTypeSystem/gts-rust/blob/main/gts-macros/tests/golden/traits_struct_literal.rs
There was a problem hiding this comment.
That derive already lives on SecretTypeTraits — the serializable trait carrier
(#[derive(JsonSchema, Serialize, Deserialize, GtsTraitsSchema)]) used as the base's
traits_schema. The line you flagged is SecretTypeDescriptor, the compile-time catalog
entry (a const, &'static/Copy, carrying identity + traits) — deriving GtsTraitsSchema
there would fold the identity fields into the traits schema, which is wrong; it converts to
the carrier via .traits() -> SecretTypeTraits.
| /// Secret type reference (catalog type, full GTS id, or type UUID — | ||
| /// see [`SecretTypeRef`]); `None` keeps the existing secret's type on | ||
| /// overwrite and defaults to the generic type on create. | ||
| pub secret_type: Option<SecretTypeRef>, |
There was a problem hiding this comment.
Why SecretTypeRef cannot always be GtsId?
There was a problem hiding this comment.
Done — removed SecretTypeRef and made the type reference a GtsId on both surfaces: SDK
WriteOptions.secret_type: Option<GtsId> and the REST type parsed as a full GTS id
(GtsId::try_new). The gear resolves it to the type's deterministic v5 UUID at the service
boundary (unchanged internally), so "GtsId on the wire, UUID inside". Short name and raw UUID
are no longer accepted, and the GET response echoes the full GTS id (symmetric). Added
impl From<SecretType> for GtsId for ergonomics. Docs, OpenAPI, and unit + e2e tests updated;
green.
| #[derive(Clone, Debug, PartialEq, Eq, DeriveEntityModel, Scopable)] | ||
| #[sea_orm(table_name = "tenant_closure")] | ||
| #[secure(unrestricted)] | ||
| pub struct Model { |
There was a problem hiding this comment.
Could you please clarify how the corresponding table is supposed to be created?
There was a problem hiding this comment.
credstore doesn't create tenant_closure — it's owned and migrated by Account Management
(AM's m0001_initial_schema). credstore only reads it, and only in the co-located
deployment (config hierarchy.tenant_closure_colocated, default false; DESIGN §4.4):
- co-located: the gear advertises the
TenantHierarchyPDP capability, so grants compile to
InTenantSubtreepredicates resolved via atenant_closuresubquery. - not co-located (default): the PDP emits a flat
Inlist and credstore never touches the
table — so a standalone credstore DB without it is supported, not a missing migration.
Expanded the entity doc-comment to spell this out.
| status: accepted | ||
| date: 2026-07-04 | ||
| --- | ||
| # ADR-0001: Stateful Gateway with Gateway-Owned Secret Metadata |
There was a problem hiding this comment.
I would suggest stop using gateway word for the credstore gear since it's becoming to be a full-featured regular gear with pluggable backend storage.
There was a problem hiding this comment.
Agreed — dropped "gateway" across the gear (docs, ADRs, comments, READMEs, Cargo desc).
Renamed ADR-0001 to Stateful Gear with Gear-Owned Secret Metadata (file/ID
…-stateful-gear) and updated cross-refs. Left the legitimate "Gateway" uses (the OAGW actor,
the api-gateway config prefix). Text-only; tests/dylint green.
| Deferred, | ||
| /// A deterministic misconfiguration that cannot self-heal (logged at error). | ||
| /// The provider is left unavailable; retrying would not help. | ||
| Misconfigured, |
There was a problem hiding this comment.
In the misconfiguration case I would suggest using fast-fast approach to detect an issue as soon as possible.
Actually, I'm not sure that it's ok that upstreams/routes registration depends on credstore availability. Did we have such dependency before this PR?
There was a problem hiding this comment.
Factual: no — before this PR oagw_provisioning didn't touch credstore and failures were
fail-hard. This PR added both the dependency and the fail-hard→defer shift deliberately: a
provider's secret is created at runtime and may not exist at boot. For fail-fast: moved the
deterministic-misconfig check into ProviderEntry::validate() (runs at init, fails boot) — it
now rejects a tenant override lacking both host and upstream_alias. Misconfigured is now
just a defensive fallback; the "secret not yet available" path stays deferred (transient,
indistinguishable from a bad ref here).
| })) | ||
| } | ||
|
|
||
| async fn put( |
There was a problem hiding this comment.
How about having centralized mock in the credstore-sdk crate (under test-util feature flag) and use it everywhere to avoid code duplication?
There was a problem hiding this comment.
Done — added credstore_sdk::test_util::MockCredStoreClient behind a test-util feature: one
configurable double (empty / with_secrets / returning_raw_value / always_failing).
Removed the five hand-rolled doubles in oagw; test_support now re-exports the SDK mock, so
existing call sites are unchanged. Wired the feature through. Tests + dylint green.
|
|
||
| **ID**: `cpt-cf-credstore-adr-value-fingerprint-fence` | ||
|
|
||
| ## Context and Problem Statement |
There was a problem hiding this comment.
Could you please explain why we cannot always use optimistic lock to avoid data races?
There was a problem hiding this comment.
Optimistic locking doesn't close this race — it's a crosswise dual-write: value (backend) and
metadata (row) are separate stores with no shared transaction, so two concurrent PUTs can land
backend = Bob's value + row = Alice's label (cross-tenant disclosure). If-Match only guards
the metadata row's version within its own store — it can't bind the backend value to the
metadata across stores, and it's opt-in. A cross-store fence isn't available (cluster
ADR-002). Hence the read-time value fingerprint. Added a paragraph to the ADR Context.
| - [ ] `p1` - **ID**: `cpt-cf-credstore-constraint-canonical-errors` | ||
|
|
||
| All trait-boundary and REST errors follow the platform canonical error model | ||
| (ADR 0005): domain errors map to canonical categories with stable `reason` |
There was a problem hiding this comment.
Suggest using full path to ADR docs, otherwise it's unclear
There was a problem hiding this comment.
Fixed — the bare cross-doc ADR references are now full relative-path links: "ADR 0005" →
docs/arch/errors/ADR/0005-cpt-cf-adr-sdk-canonical-projection.md (×2), and "cluster ADR-002"
→ gears/system/cluster/docs/ADR/002-async-boundary-no-remote-in-critical-section.md. (In-gear
ADR references were already relative ./ADR/… links.)
| Err(CanonicalError::AlreadyExists { resource_name, .. }) => { | ||
| reuse_existing_upstream(gateway, ctx, provider_id, resource_name.as_deref()).await | ||
| } | ||
| Err(e) => { |
There was a problem hiding this comment.
Correct me, but now any upstram registration fail will lead to just warning in logs + retries. In my view it's not a reliable solution..
There was a problem hiding this comment.
It's structured, not a bare warn+loop: reconcile_deferred_upstreams_with_retry runs in the
background with exponential backoff (2s → 1min cap), is cancellation-aware, shrinks as
providers succeed, and warns once after 2min with likely causes. Unbounded by design — a
runtime-provisioned secret has no deadline, and transient-vs-permanent can't be split here
(see C12); deterministic misconfigs now fail fast at boot. If you want a stronger signal I'd
add a deferred-upstreams gauge rather than bound the retry — happy to.
There was a problem hiding this comment.
Would be nice to see openapi.yaml for the service.
There was a problem hiding this comment.
Added a credstore-scoped OpenAPI in YAML at gears/credstore/docs/api/openapi.yaml (the
gear's paths + referenced components), extracted from the platform docs/api/api.json. Both
are generated via make openapi and kept fresh by the api_contracts check. DESIGN §4.3.1
links both.
| | `GET` | `/credstore/v1/secrets/{ref}` | `200` + `ETag`, `Cache-Control: no-store` | Hierarchical read | | ||
| | `DELETE` | `/credstore/v1/secrets/{ref}` | `204` | Delete own secret; honours `If-Match` | | ||
|
|
||
| **Create Secret Request (`POST`)**: | ||
| ```json | ||
| { | ||
| "reference": "partner-openai-key", |
There was a problem hiding this comment.
Would be nice to clarify the format of the reference, I expected it to be some GTS.
On the other hand, if in some cases reference will be user-faced, requiring a GTS identifier feels too restrictive.
Something that this design should address.
There was a problem hiding this comment.
reference is intentionally not a GTS id — a caller-chosen opaque label
(^[A-Za-z0-9_-]+$, 1–255), because it's user-facing (a GTS id would be too restrictive, as
you note). Documented the format explicitly in DESIGN §4.3.1 (was only in the OpenAPI schema
before). Uniqueness is per sharing class within a tenant.
| | `GET` | `/credstore/v1/secrets/{ref}` | `200` + `ETag`, `Cache-Control: no-store` | Hierarchical read | | ||
| | `DELETE` | `/credstore/v1/secrets/{ref}` | `204` | Delete own secret; honours `If-Match` | | ||
|
|
||
| **Create Secret Request (`POST`)**: | ||
| ```json | ||
| { | ||
| "reference": "partner-openai-key", | ||
| "value": "demo-secret-value-123", |
There was a problem hiding this comment.
Can secret contain binary? A certificate?
There was a problem hiding this comment.
Yes — the value is stored as raw bytes end-to-end, so binary secrets (e.g. a DER cert or a raw
key) are supported, governed by the per-type utf8_only trait (only generic allows
non-UTF-8 today). Caveat: REST carries value as a UTF-8 JSON string, so raw binary goes via
the SDK (bytes); over REST it must be text-encoded (PEM/base64). A PEM cert is UTF-8 and works
anywhere. Spelled out in DESIGN §4.3.1.
| } | ||
| ``` | ||
|
|
||
| **Sharing mode values**: `"private"` (owner-only), `"tenant"` (tenant-wide, default), `"shared"` (hierarchical) | ||
| **Update Secret Request (`PUT`)** — same body without `reference`. | ||
| `sharing` values: `"private"`, `"tenant"` (default), `"shared"`. |
There was a problem hiding this comment.
In oagw we consider the following options:
| Parent Sharing | Child Specifies | Effective Limit |
|----------------|-----------------|---------------------------------------------|
| `private` | any | Child's limit only |
| `inherit` | none | Parent's limit |
| `inherit` | own limit | `min(parent, child)` |
| `enforce` | any | `min(parent, child)` - cannot exceed parent |
Reads better to me.
There was a problem hiding this comment.
The private/inherit/enforce values don't carry over to credstore — it's a different axis. Our sharing answers "who can read the secret within the tenant hierarchy", not a parent/child limit-inheritance policy:
private— owner only;tenant— tenant-wide, not inherited;shared— the tenant and its descendants, closest-wins (a child may shadow it with its own value).
There's no numeric limit and no parent-side policy knob here, so inherit/enforce/"Effective Limit" have nothing to map onto. (Our shared already behaves like your inherit row — the child can override; we deliberately don't have an enforce / non-overridable mode.)
And these aren't just doc wording — private/tenant/shared is the shipped contract: the SharingMode SDK enum, the REST sharing field, the credstore_secrets.sharing column (SMALLINT 1/2/3), the per-type allow_sharing trait, and what current consumers already send. Renaming to private/inherit/enforce would break the SDK, the wire contract, stored rows, and every integration — for values that mean something different anyway.
Kept the modes as-is.
| "sharing": "shared", | ||
| "is_inherited": true | ||
| "is_inherited": true, |
There was a problem hiding this comment.
Does the client needs to know inheritance story? In what scenario?
There was a problem hiding this comment.
is_inherited (with owner_tenant_id) tells the caller whether the resolved secret is the
tenant's own or inherited from an ancestor. Scenario: deciding whether to create a
tenant-local override — e.g. an admin UI showing "using the parent's shared credential" vs
"your own", so an operator can shadow it. It also lets a caller key a cache correctly.
Read-only metadata; nothing in the write path needs it.
| partial-index semantics; PostgreSQL and SQLite; MySQL fails fast): | ||
|
|
||
| ```sql | ||
| CREATE TABLE credstore_secrets ( |
There was a problem hiding this comment.
CREATE TYPE sharing AS ENUM (
'private',
'tenant',
'shared'
);
...
CREATE UNIQUE INDEX uq_credstore_nonprivate ON credstore_secrets (tenant_id, reference) WHERE sharing <> 'private';
...There was a problem hiding this comment.
sharing/status are SMALLINT + CHECK rather than a native CREATE TYPE … AS ENUM
because the schema targets both PostgreSQL and SQLite from one migration set (§8), and SQLite
has no enum type — a PG enum wouldn't port. The mapping is centralized in the entity layer
(sharing_to_i16/sharing_from_i16). I'd keep it as-is, but happy to add a comment making the
encoding explicit.
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@gears/credstore/credstore/src/infra/error_conv.rs`:
- Around line 14-23: Update the error-conversion flow around is_check_violation
so only the reference-length CHECK failure maps to InvalidSecretRef. Handle
sharing, status, and fingerprint-fence invariant violations separately, and
route unrecognized CHECK violations to Internal rather than treating them as
invalid secret references.
In `@gears/credstore/docs/api/openapi.yaml`:
- Around line 59-60: Define the components.securitySchemes.bearerAuth scheme in
the OpenAPI document, matching the existing bearer token security references
used by operations. Preserve the current security configuration and add the
scheme with the appropriate HTTP bearer settings.
In `@gears/credstore/docs/DESIGN.md`:
- Around line 88-89: Remove the duplicated “gear” in the CredStore design
description, changing “stateful gear gear” to a single “gear” while preserving
the surrounding wording.
- Around line 481-489: The error-status table incorrectly classifies non-UTF-8
stored values on GET as InvalidArgument. Update the REST error mapping
documentation around the canonical category table so this scenario is listed
under Internal with HTTP 500, matching GetSecretResponseDto::try_from_response
and DomainError::Internal.
- Around line 942-945: Update the “Failure handling” section to state that
plugin delete failures are mapped to DomainError::Internal and return 500, while
503 is reserved for unavailable backends. Preserve the existing deprovisioning,
retry, and reaper recovery behavior without guaranteeing 503 for every delete
failure.
- Around line 922-927: Correct the overwrite path’s concurrency sequence so the
guarded metadata version check does not claim the CAS before backend plugin.put;
perform plugin.put first, then apply the version-gated touch/update. Ensure a
zero-row touch without If-Match is treated as successful rather than returning
VersionConflict, while preserving 409 behavior for a moved version under
If-Match and success when the row was concurrently deleted.
- Around line 857-862: Update the API description in the design documentation to
state that custom types are addressed only by their full GTS type ID, removing
the raw UUID alternative. Keep the built-in type and trait-enforcement guidance
unchanged.
In `@gears/credstore/docs/PRD.md`:
- Line 507: Update the secret-sharing requirement in the PRD to remove the
phrase “below any isolation barrier” and state that secrets are resolvable only
by the partner and descendant tenants within the same isolation boundary.
Preserve the rule that sharing cannot cross isolation barriers.
- Line 317: Update the PDP authorization requirement to mandate one evaluation
against the resolved concrete secret GTS type for every operation, including
generic secrets, and remove the base-resource evaluation and second-evaluation
language. Preserve the existing action mapping, metadata-query enforcement,
fail-closed behavior, and indistinguishable read results for denied or
out-of-scope secrets.
In `@gears/credstore/QUICKSTART.md`:
- Line 18: Update the configuration reference in the example server
documentation from `gearss.api-gateway.config.prefix_path` to
`gears.api-gateway.config.prefix_path`, preserving the surrounding explanation.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a95b771-9882-4adb-980a-3861eaf1df9c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (97)
.github/workflows/cfs.ymlconfig/e2e-local.yamlconfig/e2e-scope-enforcement.yamlconfig/e2e-tr-authz.yamlconfig/mini-chat.yamlconfig/quickstart-windows.yamlconfig/quickstart.yamlconfig/static-tenants.yamldocs/api/api.jsongears/credstore/QUICKSTART.mdgears/credstore/credstore-sdk/Cargo.tomlgears/credstore/credstore-sdk/README.mdgears/credstore/credstore-sdk/src/api.rsgears/credstore/credstore-sdk/src/error.rsgears/credstore/credstore-sdk/src/error_tests.rsgears/credstore/credstore-sdk/src/gts.rsgears/credstore/credstore-sdk/src/gts_tests.rsgears/credstore/credstore-sdk/src/lib.rsgears/credstore/credstore-sdk/src/models.rsgears/credstore/credstore-sdk/src/models_tests.rsgears/credstore/credstore-sdk/src/plugin_api.rsgears/credstore/credstore-sdk/src/test_util.rsgears/credstore/credstore-sdk/src/types.rsgears/credstore/credstore-sdk/src/types_tests.rsgears/credstore/credstore/Cargo.tomlgears/credstore/credstore/README.mdgears/credstore/credstore/src/api.rsgears/credstore/credstore/src/api/rest.rsgears/credstore/credstore/src/api/rest/dto.rsgears/credstore/credstore/src/api/rest/dto_tests.rsgears/credstore/credstore/src/api/rest/handlers.rsgears/credstore/credstore/src/api/rest/routes.rsgears/credstore/credstore/src/api/rest/routes_tests.rsgears/credstore/credstore/src/client.rsgears/credstore/credstore/src/client_tests.rsgears/credstore/credstore/src/config.rsgears/credstore/credstore/src/config_tests.rsgears/credstore/credstore/src/domain.rsgears/credstore/credstore/src/domain/authz.rsgears/credstore/credstore/src/domain/error.rsgears/credstore/credstore/src/domain/error_tests.rsgears/credstore/credstore/src/domain/local_client.rsgears/credstore/credstore/src/domain/local_client_tests.rsgears/credstore/credstore/src/domain/mod.rsgears/credstore/credstore/src/domain/ports.rsgears/credstore/credstore/src/domain/ports/metrics.rsgears/credstore/credstore/src/domain/ports/plugin.rsgears/credstore/credstore/src/domain/resolver.rsgears/credstore/credstore/src/domain/secret.rsgears/credstore/credstore/src/domain/secret/fence.rsgears/credstore/credstore/src/domain/secret/model.rsgears/credstore/credstore/src/domain/secret/repo.rsgears/credstore/credstore/src/domain/secret/service.rsgears/credstore/credstore/src/domain/secret/service_tests.rsgears/credstore/credstore/src/domain/secret/test_support.rsgears/credstore/credstore/src/domain/secret/type_resolver.rsgears/credstore/credstore/src/domain/secret/typing.rsgears/credstore/credstore/src/domain/secret/typing_tests.rsgears/credstore/credstore/src/domain/service.rsgears/credstore/credstore/src/domain/service_tests.rsgears/credstore/credstore/src/domain/test_support.rsgears/credstore/credstore/src/gear.rsgears/credstore/credstore/src/infra.rsgears/credstore/credstore/src/infra/canonical_mapping.rsgears/credstore/credstore/src/infra/error_conv.rsgears/credstore/credstore/src/infra/metrics.rsgears/credstore/credstore/src/infra/metrics_tests.rsgears/credstore/credstore/src/infra/plugin_select.rsgears/credstore/credstore/src/infra/sdk_error_mapping.rsgears/credstore/credstore/src/infra/storage.rsgears/credstore/credstore/src/infra/storage/entity.rsgears/credstore/credstore/src/infra/storage/entity/secrets.rsgears/credstore/credstore/src/infra/storage/entity/tenant_closure.rsgears/credstore/credstore/src/infra/storage/migrations.rsgears/credstore/credstore/src/infra/storage/migrations/m0001_initial_schema.rsgears/credstore/credstore/src/infra/storage/repo_impl.rsgears/credstore/credstore/src/infra/storage/repo_impl/helpers.rsgears/credstore/credstore/src/infra/storage/repo_impl/reads.rsgears/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rsgears/credstore/credstore/src/infra/storage/repo_impl/writes.rsgears/credstore/credstore/src/infra/tenant_resolver.rsgears/credstore/credstore/src/infra/tenant_resolver_tests.rsgears/credstore/credstore/src/infra/types_registry.rsgears/credstore/credstore/src/infra/types_registry_tests.rsgears/credstore/credstore/src/lib.rsgears/credstore/docs/ADR/0001-cpt-cf-credstore-adr-stateful-gear.mdgears/credstore/docs/ADR/0002-cpt-cf-credstore-adr-deprovisioning-saga.mdgears/credstore/docs/ADR/0003-cpt-cf-credstore-adr-value-fingerprint-fence.mdgears/credstore/docs/DESIGN.mdgears/credstore/docs/PRD.mdgears/credstore/docs/api/openapi.yamlgears/credstore/plugins/static-credstore-plugin/README.mdgears/credstore/plugins/static-credstore-plugin/src/config.rsgears/credstore/plugins/static-credstore-plugin/src/domain/client.rsgears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rs
💤 Files with no reviewable changes (10)
- gears/credstore/credstore/src/domain/service.rs
- gears/credstore/credstore/src/domain/local_client_tests.rs
- gears/credstore/credstore/src/domain/test_support.rs
- gears/credstore/credstore/src/config_tests.rs
- gears/credstore/credstore/src/domain/local_client.rs
- gears/credstore/credstore-sdk/src/error_tests.rs
- gears/credstore/credstore-sdk/src/models_tests.rs
- gears/credstore/credstore/src/domain/error_tests.rs
- gears/credstore/credstore/src/domain/service_tests.rs
- gears/credstore/credstore/src/domain/mod.rs
🚧 Files skipped from review as they are similar to previous changes (60)
- gears/credstore/credstore/src/api/rest.rs
- gears/credstore/credstore/src/api.rs
- gears/credstore/credstore/src/infra/storage/entity.rs
- gears/credstore/credstore/src/domain/resolver.rs
- gears/credstore/credstore/src/infra.rs
- gears/credstore/credstore/src/domain/secret.rs
- gears/credstore/credstore/src/domain.rs
- gears/credstore/credstore/src/infra/storage/migrations.rs
- gears/credstore/credstore/README.md
- gears/credstore/credstore/src/infra/storage/entity/secrets.rs
- gears/credstore/credstore/src/domain/ports/plugin.rs
- gears/credstore/credstore/src/domain/ports.rs
- gears/credstore/credstore/src/infra/storage/entity/tenant_closure.rs
- config/e2e-scope-enforcement.yaml
- gears/credstore/credstore/src/domain/secret/type_resolver.rs
- gears/credstore/credstore-sdk/README.md
- gears/credstore/credstore/src/infra/storage.rs
- gears/credstore/credstore/src/api/rest/dto_tests.rs
- gears/credstore/credstore/Cargo.toml
- gears/credstore/credstore/src/domain/authz.rs
- gears/credstore/credstore/src/infra/sdk_error_mapping.rs
- gears/credstore/credstore/src/infra/canonical_mapping.rs
- gears/credstore/credstore/src/lib.rs
- config/static-tenants.yaml
- gears/credstore/credstore/src/client.rs
- gears/credstore/credstore/src/api/rest/routes.rs
- gears/credstore/credstore/src/infra/tenant_resolver.rs
- gears/credstore/credstore/src/infra/types_registry_tests.rs
- gears/credstore/credstore/src/domain/secret/fence.rs
- gears/credstore/credstore/src/infra/storage/repo_impl/helpers.rs
- gears/credstore/credstore/src/infra/tenant_resolver_tests.rs
- gears/credstore/credstore/src/config.rs
- gears/credstore/credstore-sdk/Cargo.toml
- gears/credstore/credstore/src/infra/plugin_select.rs
- config/quickstart.yaml
- gears/credstore/credstore/src/infra/storage/repo_impl.rs
- gears/credstore/plugins/static-credstore-plugin/README.md
- gears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rs
- gears/credstore/credstore/src/domain/ports/metrics.rs
- gears/credstore/credstore/src/infra/storage/migrations/m0001_initial_schema.rs
- gears/credstore/credstore-sdk/src/api.rs
- gears/credstore/credstore/src/domain/secret/typing.rs
- gears/credstore/credstore/src/domain/secret/model.rs
- config/e2e-tr-authz.yaml
- gears/credstore/credstore/src/api/rest/handlers.rs
- gears/credstore/credstore-sdk/src/gts_tests.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/client.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rs
- gears/credstore/credstore-sdk/src/lib.rs
- gears/credstore/credstore/src/infra/metrics.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/service.rs
- gears/credstore/credstore/src/domain/secret/repo.rs
- gears/credstore/credstore/src/api/rest/routes_tests.rs
- gears/credstore/credstore/src/domain/secret/service.rs
- gears/credstore/credstore/src/infra/types_registry.rs
- docs/api/api.json
- gears/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rs
- gears/credstore/credstore/src/domain/secret/test_support.rs
- gears/credstore/credstore/src/infra/storage/repo_impl/writes.rs
- gears/credstore/credstore-sdk/src/error.rs
…iven secret types Port the credstore gear: a tenant-scoped, PDP-enforced credential gateway over pluggable value-store backends, with hierarchical resolution, a crash-safe saga lifecycle, and dynamic GTS secret typing. Gateway & domain - Stateful metadata gateway (credstore_secrets) over a trait-agnostic value-store plugin SPI; hierarchical read resolves the closest accessible secret up the barrier-respecting tenant ancestor chain (private > tenant > shared), reading the backend once for the winner. - Crash-safe write/delete sagas (provisioning / active / deprovisioning) with a background reaper that reconciles orphaned backend values, releases wedged references, and sweeps expired secrets; optimistic concurrency via If-Match / version. Authorization - Single PDP evaluation per operation on the secret's full concrete GTS type (incl. generic), gated on the caller's tenant; hierarchical visibility decided by the resolver, not the PDP. GET denial/miss is an anti-enumeration 404; PDP outage is 503. Registry-driven secret types - Secret types are GTS types derived from gts.cf.core.credstore.secret.v1~; the types-registry is the runtime source of truth (SecretTypeResolver resolves a type's id + effective traits per operation, no local cache), so custom types register a GTS schema with no credstore release. The compiled-in catalog only seeds the built-in type schemas. - Enforced traits (allow_sharing, value_schema, max_size_bytes, expirable, utf8_only, advisory rotation_period_secs) validated on write against the resolved traits; type is immutable for a secret's lifetime. - Storage: secret_type_uuid UUID (deterministic v5 of the GTS id), default pinned to the generic type. SDK/REST accept a type as catalog short name, full GTS id, or UUID (SecretTypeRef); responses carry the resolved id. - Fail-closed: unknown/non-secret type -> 400 UNKNOWN_SECRET_TYPE on create, 503 for a stored row; registry outage/timeout/malformed traits -> 503. Value-fingerprint fence & optimistic-concurrency hardening - Each row carries value_fp = HMAC-SHA256(fence_key, value), stamped in the same atomic write as the sharing label; reads verify it against the backend value and fail closed (anti-enumeration 404) on mismatch, so two crosswise concurrent last-writer-wins PUTs can never serve one writer's value under another writer's sharing label (closes the cross-tenant disclosure). The fence key is auto-generated and stored in the backend under an API-unreachable reserved entry (split knowledge; the fingerprint never leaves the gateway, in responses or logs). - Strong ETag is generation-bound "<row-id>.<version>": a validator from a deleted-and-recreated secret's earlier generation never matches even when the version counters coincide (no ABA lost update). - Out-of-band seeded rows (value_fp NULL) are served on trust and backfilled lazily on read and by the reaper sweep. See DESIGN §4.10 and ADR-0003. Integration & docs - OAGW and mini-chat consume credstore through the SDK client; e2e and integration coverage; DESIGN.md, generated OpenAPI spec, and ADRs. Signed-off-by: Diffora <ddiffora@gmail.com>
4 participants
Summary
Ports the stateful credstore gateway into this workspace. The gateway gear now owns per-secret metadata in its own DB, while the backend plugin becomes a value-only store. Adds hierarchical tenant resolution with PDP
AccessScopeenforced in SQL, optimistic concurrency (ETag/If-Match), a crash-safe write saga + background reaper, a value-fingerprint fence binding each stored value to its metadata row, OTel metrics, and a REST API under/cf/credstore/v1/secrets. Consumers (mini-chat, oagw) are adapted to the new runtime-provisioning contract.credstore
CredStorePluginClientV1is now a pure per-tenant value store (get/put/deletebytenant_id/key/owner_id); the gateway client gains write ops (put/create/delete);GetSecretResponsegainsversion; registry-driven GTS secret typing.gears/credstore/credstore) —api/rest,infra/storage(sea-orm migrationm0001, SecureORM with PDP-in-SQL),domain/secret(service, write saga, reaper, fence, typing), tenant resolver with TTL/LRU ancestor-chain cache + isolation barriers, versioning, canonical error mapping.gears/credstore/docs/features/001-value-fingerprint-fence.md, ADR 0003) —HMAC-SHA256(fence_key, value)binds a backend value to its metadata row; a crosswise last-writer-wins interleave fails closed (404) instead of a cross-tenant disclosure. Crypto runs through AWS-LC (aws-lc-rs), FIPS-validated under--features fips.Consumers
Gear::init(). Adds unit tests for the register / defer / reuse / reconcile / paging paths.Config / e2e
Correctness hardening
reap_by_idis guarded by the observed status and claims a provisioning row before touching the backend, so a slow create'smark_activecan no longer lose its row + value to the reaper.tenant_id == req(own-tenant only), rather than relying on an unenforced authn invariant.Summary by CodeRabbit
If-Matchsafeguards.