feat(secrets): pluggable SecretRef sources via plugin SDK seam#72548
feat(secrets): pluggable SecretRef sources via plugin SDK seam#72548akoscz wants to merge 31 commits into
Conversation
Greptile SummaryThis PR introduces a pluggable Confidence Score: 5/5Safe to merge. No P0 or P1 issues found; all previously flagged race conditions and silent-failure paths have been addressed. The implementation is thorough: built-in dispatch uses proper type guards, the resolver cache correctly handles misses with a null sentinel, partial factory failures surface via console.warn rather than silent drops, and the gateway negative-lookahead pattern prevents plugin-arm rescue of malformed built-in refs. TypeScript types and zod schemas are consistent. All three previously raised review issues are resolved. No files require special attention. Reviews (3): Last reviewed commit: "fix(secrets-plugins): address greptile P..." | Re-trigger Greptile |
339b9ad to
7bdee2b
Compare
|
Hi @akoscz , thanks for proposing this PR, just wonder if this PR could cover all config fields in openclaw.json no matter it's built-in config fields, e.g. skills.entries.xxx.env.* , or the other fields that might introduced from 3rd party plugins. Thanks. |
BTW, just want to confirm whether you'll have follow-up PR to add all fields support, including skills.entries..env, plugins.entries..config, channel credential schemas etc. Since we have a lot of secret configs stored in skill env vars or self develop plugin config and channel config, it's really matter to me to support to cover all of these fields by using a secret provider e.g. aws sm provider to mask these info and make sure openclaw LLM itself can't read these credentials. |
dc7b922 to
fe8ccd8
Compare
|
@greptileai review |
|
Hey @Colstuwjx — good question, and you've put your finger on exactly the gap. What this PR does: opens the backend — What it doesn't do: widen the frontend — i.e. teach more openclaw.json fields to accept
The unsupported group is what you're calling out, and it's the right next step. I scoped this PR narrowly so it's reviewable as a pure backend seam — adding SecretRef acceptance to those I'm not promising a follow-up PR myself right now — I want this one to land first and want to see what maintainers think the right next step is — but the seam this PR adds is the prerequisite for that work, and once it's in, widening |
|
Round-2 P2 findings addressed in 12d9251e01. 1. GCP sequential calls ( 2. Misshaped factory return silently dropped ( All touched-surface tests green; |
|
@greptileai review again plz |
…ret-input subpaths
…tiate Linux keyring errors
…source-id uniqueness invariant
…nion arms in mapper
…secrets-keyring pages
…single-newline strip, ENOENT via err.code, drop unused signal, doc + test polish)
- secrets-keyring: add curly braces in stripCliTrailingNewline (oxlint curly) - secrets-gcp: cache in-flight init promise so concurrent first-callers share one client; clear promise on init failure to allow retry - secret-provider-public-artifacts: warn on partial-success factory errors instead of silently dropping - secret-provider-resolver: cache negative lookups (null sentinel) so missing sources don't re-scan the artifact on every call
- secrets-gcp: fan out per-ref accessSecretVersion calls in parallel via Promise.all, deduped by id. Cuts latency from N round-trips to 1 for multi-ref configs. Added test asserting parallel in-flight count and dedup of duplicate ids. - secret-provider-public-artifacts: surface shape-check failures so factories that return misshaped objects get a diagnostic instead of silently disappearing. Updated test to assert the throw + cause when no factory in the artifact passes the shape check, plus a new test asserting the partial-success warn path.
Addresses clawsweeper P2/medium-security finding openclaw#1: previously a SecretRef referencing a plugin-owned source could execute the owning plugin's artifact even when the plugin was disabled (via plugins.enabled: false, plugins.deny, entry enabled: false, allowlist miss, or default-off bundled state). The artifact can spawn credential CLIs (secret-tool, security) or open cloud SDK clients, so this is a real privilege escalation. Resolver now consults isPluginEnabled({ pluginId, config }) after pluginId lookup and before artifact load. Cache rules: - Successful artifact loads: cached (artifact is static) - 'No bundled plugin owns this source' / 'no matching factory': cached (stable conditions) - Activation result: NEVER cached — config can change between calls, so we recheck isPluginEnabled on every lookup, including cache hits config: OpenClawConfig is threaded from resolvePluginSecretRefs in src/secrets/resolve.ts (already required on every public secrets entry via ResolveSecretRefOptions). Existing 'Unknown secret provider source... Install or enable a plugin...' error message already reads correctly for the disabled case; no copy change needed. Tests: 6 new cases covering denylist/disabled-state, no cache poisoning on enable→disable→re-enable cycles, activation re-check on cache hits, and config forwarding through dispatch. Defers clawsweeper finding openclaw#2 (resolve installed/workspace plugins, not just bundled) to a follow-up PR — that's a real SDK-completion gap but doesn't block landing the security fix.
- src/plugins/installed-plugin-index-record-builder.ts: take HEAD side; upstream commit 7ac23ee deleted hasRuntimeContractSurface() and isLegacyImplicitStartupSidecar() entirely as part of dropping the legacy implicit-startup-sidecar fallback. Our addition of secretProviders to the helper is moot. - docs/plugins/manifest.md: keep both the new secretProviders contract row (ours) and upstream's simplified tools description. - src/plugins/manifest-tool-availability.ts: switch the env narrowing from a string-literal check to the isEnvSecretProviderConfig type guard. The open PluginSecretProviderConfig variant has source: string (not a literal), so providerConfig.source !== "env" doesn't narrow it out, and providerConfig.allowlist falls back to unknown. The type guard narrows the union cleanly. - pnpm-lock.yaml: regenerated against current package.json set after rebase + reinstall. - docs/.generated/*: baselines regenerated.
…e callers - src/commands/daemon-install-helpers.ts: switch the exec narrowing from `provider.source !== 'exec'` to isExecSecretProviderConfig(). The open PluginSecretProviderConfig variant has source: string (not a literal), so a string-literal check leaves the plugin variant in the union and the subsequent .passEnv access fails with TS2488. - extensions/qqbot/src/bridge/config.ts: same fix using isEnvSecretProviderConfig() (exposed via openclaw/plugin-sdk/secret-input). The .allowlist access on the un-narrowed union failed with TS2339. These two callers were previously fine because the SecretProviderConfig union was discriminated and closed; opening it for plugin-owned sources turns string-literal narrowing into a type-checking gap. The isXSecretProviderConfig guards already exist for exactly this purpose. pnpm-lock.yaml: regenerated; baselines: regenerated.
- extensions/clickclack/src/accounts.ts: switch env narrowing from literal check to isEnvSecretProviderConfig() guard, same pattern as prior fixes for qqbot, daemon-install-helpers, and manifest-tool-availability. New upstream code adds yet another caller that hit the open PluginSecretProviderConfig variant. - src/config/schema.base.generated.ts: deleted (upstream removed it). - docs/docs.json, docs/plugins/manifest.md: trivial conflict resolutions keeping both upstream's neighboring entries and our new ones. - pnpm-lock.yaml + docs/.generated/* baselines: regenerated.
d8a06bf to
cfe5425
Compare
Dependency Changes DetectedThis PR changes dependency-related files. Maintainers should confirm these changes are intentional. Changed files:
Maintainer follow-up:
|
Summary
env/file/exec). Every new vendor backend (GCP Secret Manager, OS keyring, AWS Secrets Manager, HashiCorp Vault, native 1Password) required a core PR that touched config types, zod schemas, gateway wire schemas, the resolver dispatch, and validation. Issue [Feature]: support to use plugin to implement secret ref provider and cover core schemas #71593 (cited by the Codex automated review on the issue) and the @Colstuwjx follow-up on closed PR feat(secrets): add AWS Secrets Manager provider #43643 ask for a manifest-first plugin seam so contributors can ship new sources without touching core.contracts.secretProviders: string[], new public SDK subpathopenclaw/plugin-sdk/secret-provider, lazy public-artifact loader (src/plugins/secret-provider-public-artifacts.ts), source-id → plugin-id helper in the contract registry, and a one-line dispatch fall-through insrc/secrets/resolve.ts. Two reference bundled plugins:extensions/secrets-gcp/(Google Cloud Secret Manager) andextensions/secrets-keyring/(macOS Keychain viasecurity, Linux libsecret viasecret-tool). Config zod schemas + Gateway wire-protocol schemas open additively to accept plugin-owned sources while keeping strict per-source validation for the built-in literals.env/file/execresolution paths are byte-unchanged. Existing configs that don't reference plugin sources behave identically. The "broader SecretRef field-coverage" half of [Feature]: support to use plugin to implement secret ref provider and cover core schemas #71593 (makingskills.entries.*.env,plugins.entries.*.config, channel credential schemas all accept SecretRef objects) is explicitly deferred.openclaw secrets configureinteractive flow doesn't yet list plugin sources (documented in plugin pages). No env/file/exec fields in core config types were modified.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
N/A — feature work, not a bug fix.
Regression Test Plan (if applicable)
N/A — feature work. New tests cover the new behavior; see Evidence below.
Reproduction Steps (if applicable)
N/A — feature work. End-to-end usage is documented in
docs/plugins/secrets-gcp.mdanddocs/plugins/secrets-keyring.md.Evidence
Targeted test counts (all green):
src/plugins/manifest-registry.test.ts(manifest contract)src/plugins/contracts/registry.contract.test.tssrc/plugins/contracts/boundary-invariants.test.tssrc/plugins/bundled-capability-metadata.test.ts(uniqueness invariant)src/plugins/secret-provider-public-artifacts.test.tssrc/plugins/secret-provider-resolver.test.ts(cache + dispatch)src/secrets/resolve.test.ts(incl. real env+file+plugin batch integration)src/secrets/exec-secret-ref-id-parity.test.ts(zod ↔ wire parity)src/config/types.secrets.plugin-source.test.ts(openSecretRefSource)src/config/zod-schema.plugin-secret-providers.test.ts(open zod schemas)src/config/config.secrets-schema.test.ts(per-arm error preservation)src/config/schema.base.generated.test.ts(introspection unchanged)src/gateway/protocol/primitives.secretref.test.ts(open wire schemas)extensions/secrets-gcp/secret-provider.test.tsextensions/secrets-keyring/secret-provider.test.tsLive end-to-end smokes against real backends (not just mocks):
secret-tool store ...throughcreateKeyringSecretProvider().resolve(). Validation guards rejected leading--service / shell metas / relativekeychainPath/ wrong suffix / leading--ref id before any spawn.security add-generic-password -s openclaw -a openai-api-key -w ...through the plugin. The plugin uses macOS's native vocabulary, so users with existing Keychain entries don't need to re-store. Validation guards rejected the same set as Linux.createGcpSecretProvider().resolve()for bothlatestand explicitversionSuffix: "1". Test secret deleted after; gcloud account context restored.Human Verification (required)
What I personally verified (not just CI):
secret-toolagainst runningkwalletd6.security's native-s service -a idflag mapping.latestversion + explicitversionSuffix: "1".execFilespawn.project(too short, leading digit, uppercase, path traversal, trailing dash), malformedversionSuffix(bogus,0,-1), wrongsource.secrets.providers.myGcp = { source: "gcp", project: "<your-project-id>" }andapiKey: { source: "gcp", provider: "myGcp", id: "..." }parses through zod, dispatches through the resolver, and returns the value from real GCP.pnpm tsgo:allclean.pnpm docs:check-mdx(473 files),pnpm docs:check-links(3,644 internal links, 0 broken),pnpm config:docs:check,pnpm config:schema:check,pnpm plugin-sdk:api:checkall green.exec-secret-ref-id-parity.test.ts34/34) and rejects malformed built-in refs (no silent rescue by the plugin arm).{source:"env", id:"bad"}) does NOT get rescued by the plugin arm on either zod or wire side; it fails at the strict env arm with the original specific error message ("Env secret reference id must match...").\nis preserved by keyring resolvers.secret-tool"command not found" produces an actionable install message; "secret missing in libsecret" produces a distinct error pointing atsecret-tool store.pnpm check:changedtypecheck + lint + import cycles all green; the only "test" failures in a full sweep run were 3 unrelated extension tests (amazon-bedrock) plus a worker OOM on a flaky shard, none touched by this PR._resetSecretProviderResolverCache()); production reload viasecrets.reloadre-walks the dispatch.Review Conversations
Compatibility / Migration
contracts.secretProvidersmanifest field and new optionalsecrets.providers.<alias> = { source: "<plugin-source>", ... }shape.plugins.entries.secrets-gcp.enabled = trueand similar).Risks and Mitigations
superRefine-routed schema insrc/config/zod-schema.core.tsis a non-standard zod pattern compared to the pure discriminated-union approach. It exists to preserve per-arm error messages downstream ofvalidation.ts:mapZodIssueToConfigIssue(the validation pipeline's union-error flattener doesn't drill into nestedinvalid_unionissues by default).extractDeepestUnionArmIssuehelper tovalidation.tsthat drills into nested invalid_union issues so error messages survive the flattening.config.secrets-schema.test.ts(13 tests) locks in the per-arm message preservation behavior. JSON Schema introspection is preserved (schema.base.generated.test.tspasses; the generated baseline is unchanged for built-in source shapes).patternwith a negative lookahead (^(?!(?:env|file|exec)$).+$) to prevent the plugin arm from rescuing a malformed built-in source. Lookaheads are valid ECMA 262 regex but historically vary across JSON Schema validators.does not let the plugin arm silently rescue malformed built-in refsproves this works in practice.secret-provider.tsfactory could accidentally export a function whose name matches the discovery convention (/^create.*SecretProvider$/) but returns a malformed object.loadBundledSecretProviderEntriesFromDirvalidates the factory output viaisSecretProviderPlugin(id, label, resolve presence) and returns null on shape failure.secret-provider-resolver.tscross-checksentry.id === sourceand returns undefined on mismatch, leading to a clear "Unknown secret provider source" error rather than silent acceptance.