feat: extend secrets management — macOS Keychain provider + expanded in-scope fields#28984
feat: extend secrets management — macOS Keychain provider + expanded in-scope fields#28984samshields-oc wants to merge 3 commits intoopenclaw:mainfrom
Conversation
…in-scope fields [anthropic/claude-haiku-4-5]
06012ba to
81ab040
Compare
|
Opening a discussion thread per CONTRIBUTING.md: https://github.com/openclaw/openclaw/discussions/28992 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06012ba3d8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -304,6 +304,372 @@ function collectConfigSecrets(params: { | |||
| accountId, | |||
| ); | |||
| } | |||
|
|
|||
| // Collect channel tokens (telegram, discord, slack, bluebubbles) | |||
There was a problem hiding this comment.
Run new secret checks even when Google Chat is unset
The newly added token/header/plugin scans are placed after two early returns in collectConfigSecrets, so they never execute unless channels.googlechat exists and channels.googlechat.accounts is an object. In common configs without Google Chat, secrets audit will skip all of these new in-scope fields and report false confidence instead of flagging plaintext or unresolved refs.
Useful? React with 👍 / 👎.
| } | ||
| if (candidate.type === "models.providers.headers") { | ||
| return ( | ||
| segments.length === 4 && |
There was a problem hiding this comment.
Validate provider header targets with the correct path shape
models.providers.headers currently accepts only 4 segments, but real header targets are emitted as models.providers.<provider>.headers.<header> (5 segments). This makes valid header targets fail plan validation/apply, so the new header secret migration path is effectively unusable.
Useful? React with 👍 / 👎.
| } | ||
| if (candidate.type === "plugins.entries.config") { | ||
| return ( | ||
| segments.length === 4 && |
There was a problem hiding this comment.
Validate plugin config targets with the full config key path
plugins.entries.config is also validated as a 4-segment path, while actual secret-bearing entries are plugins.entries.<plugin>.config.<key> (5 segments). As written, plans for plugin config secrets are rejected even when the target path is correct.
Useful? React with 👍 / 👎.
Greptile SummaryExtended secrets management with macOS Keychain provider and expanded audit coverage for channel tokens, gateway/hooks, talk/TTS API keys, model provider headers, and plugin config values. Key changes:
Critical issues found:
Confidence Score: 2/5
Last reviewed commit: 81ab040 |
| if (candidate.type === "models.providers.headers") { | ||
| return ( | ||
| segments.length === 4 && | ||
| segments[0] === "models" && | ||
| segments[1] === "providers" && | ||
| segments[3] !== "" && | ||
| (candidate.providerId === undefined || candidate.providerId.trim().length === 0 || candidate.providerId === segments[2]) | ||
| ); |
There was a problem hiding this comment.
path validation logic will never match actual paths created by audit.ts
The validation checks segments.length === 4, but audit.ts creates paths like models.providers.openai.headers.authorization which split into 5 segments: ["models", "providers", "openai", "headers", "authorization"].
| if (candidate.type === "models.providers.headers") { | |
| return ( | |
| segments.length === 4 && | |
| segments[0] === "models" && | |
| segments[1] === "providers" && | |
| segments[3] !== "" && | |
| (candidate.providerId === undefined || candidate.providerId.trim().length === 0 || candidate.providerId === segments[2]) | |
| ); | |
| if (candidate.type === "models.providers.headers") { | |
| return ( | |
| segments.length === 5 && | |
| segments[0] === "models" && | |
| segments[1] === "providers" && | |
| segments[3] === "headers" && | |
| segments[4] !== "" && | |
| (candidate.providerId === undefined || candidate.providerId.trim().length === 0 || candidate.providerId === segments[2]) | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/secrets/plan.ts
Line: 194-201
Comment:
path validation logic will never match actual paths created by audit.ts
The validation checks `segments.length === 4`, but audit.ts creates paths like `models.providers.openai.headers.authorization` which split into 5 segments: ["models", "providers", "openai", "headers", "authorization"].
```suggestion
if (candidate.type === "models.providers.headers") {
return (
segments.length === 5 &&
segments[0] === "models" &&
segments[1] === "providers" &&
segments[3] === "headers" &&
segments[4] !== "" &&
(candidate.providerId === undefined || candidate.providerId.trim().length === 0 || candidate.providerId === segments[2])
);
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (candidate.type === "plugins.entries.config") { | ||
| return ( | ||
| segments.length === 4 && | ||
| segments[0] === "plugins" && | ||
| segments[1] === "entries" && | ||
| segments[3] !== "" && | ||
| (candidate.providerId === undefined || candidate.providerId.trim().length === 0 || candidate.providerId === segments[2]) | ||
| ); | ||
| } |
There was a problem hiding this comment.
path validation logic will never match actual paths created by audit.ts
The validation checks segments.length === 4, but audit.ts creates paths like plugins.entries.homeassistant.config.apiToken which split into 5 segments: ["plugins", "entries", "homeassistant", "config", "apiToken"].
| if (candidate.type === "plugins.entries.config") { | |
| return ( | |
| segments.length === 4 && | |
| segments[0] === "plugins" && | |
| segments[1] === "entries" && | |
| segments[3] !== "" && | |
| (candidate.providerId === undefined || candidate.providerId.trim().length === 0 || candidate.providerId === segments[2]) | |
| ); | |
| } | |
| if (candidate.type === "plugins.entries.config") { | |
| return ( | |
| segments.length === 5 && | |
| segments[0] === "plugins" && | |
| segments[1] === "entries" && | |
| segments[3] === "config" && | |
| segments[4] !== "" && | |
| (candidate.providerId === undefined || candidate.providerId.trim().length === 0 || candidate.providerId === segments[2]) | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/secrets/plan.ts
Line: 203-211
Comment:
path validation logic will never match actual paths created by audit.ts
The validation checks `segments.length === 4`, but audit.ts creates paths like `plugins.entries.homeassistant.config.apiToken` which split into 5 segments: ["plugins", "entries", "homeassistant", "config", "apiToken"].
```suggestion
if (candidate.type === "plugins.entries.config") {
return (
segments.length === 5 &&
segments[0] === "plugins" &&
segments[1] === "entries" &&
segments[3] === "config" &&
segments[4] !== "" &&
(candidate.providerId === undefined || candidate.providerId.trim().length === 0 || candidate.providerId === segments[2])
);
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (segments.length === 3 && segments[0] === "channels" && segments[1] === "discord" && segments[2] === "token") { | ||
| return true; | ||
| } | ||
| if (segments.length === 5 && segments[0] === "channels" && segments[1] === "discord" && segments[2] === "accounts" && segments[4] === "token") { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
inconsistent account validation compared to telegram pattern
Telegram validates candidate.accountId for account-specific paths (line 149), but discord/slack/bluebubbles just return true without checking. Should either validate accountId consistently or document why these channels differ.
For consistency with telegram:
| if (segments.length === 3 && segments[0] === "channels" && segments[1] === "discord" && segments[2] === "token") { | |
| return true; | |
| } | |
| if (segments.length === 5 && segments[0] === "channels" && segments[1] === "discord" && segments[2] === "accounts" && segments[4] === "token") { | |
| return true; | |
| } | |
| if (segments.length === 5 && segments[0] === "channels" && segments[1] === "discord" && segments[2] === "accounts" && segments[4] === "token") { | |
| return candidate.accountId === undefined || candidate.accountId.trim().length === 0 || candidate.accountId === segments[3]; | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/secrets/plan.ts
Line: 153-158
Comment:
inconsistent account validation compared to telegram pattern
Telegram validates `candidate.accountId` for account-specific paths (line 149), but discord/slack/bluebubbles just return `true` without checking. Should either validate accountId consistently or document why these channels differ.
For consistency with telegram:
```suggestion
if (segments.length === 5 && segments[0] === "channels" && segments[1] === "discord" && segments[2] === "accounts" && segments[4] === "token") {
return candidate.accountId === undefined || candidate.accountId.trim().length === 0 || candidate.accountId === segments[3];
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.|
Thanks for the thorough reviews! All issues addressed in the latest commit: P1 — audit.ts early return (Codex): Fixed. The new channel/gateway/hooks/plugin/header checks were incorrectly placed after the P2 — models.providers.headers segment count (Codex + Greptile): Fixed. Changed from 4→5 segments and added explicit P2 — plugins.entries.config segment count (Codex + Greptile): Fixed. Same fix — 4→5 segments with explicit Inconsistent accountId validation (Greptile): Fixed. discord, slack, and bluebubbles account-scoped paths now validate Build passes cleanly. Will push the updated branch shortly (working through a GitHub OAuth scope issue on the fork account). |
…untId consistency [anthropic/claude-sonnet-4-6]
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cb6567039
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,107 @@ | |||
| #!/usr/bin/env npx tsx | |||
There was a problem hiding this comment.
Use a valid shebang for keychain-resolver execution
This script is intended to be run directly as an exec-provider command, but #!/usr/bin/env npx tsx is not a valid multi-argument env shebang; env treats npx tsx as a single executable name (and exits before your protocol code runs). In practice, calling scripts/keychain-resolver.ts by path fails immediately, so all keychain-backed secret resolutions will fail for the documented configuration.
Useful? React with 👍 / 👎.
| | "skills.entries.apiKey" | ||
| | "channels.googlechat.serviceAccount"; | ||
| | "channels.googlechat.serviceAccount" | ||
| | "channels.telegram.botToken" |
There was a problem hiding this comment.
Restrict new plan target types to runtime-supported paths
These newly accepted target types allow plans to write SecretRef objects into channel/gateway/plugin fields, but runtime secret materialization still only resolves models.providers.*.apiKey, skills.entries.*.apiKey, and Google Chat service-account paths (collectConfigAssignments in src/secrets/runtime.ts). So when a plan includes one of these new types (for example channels.telegram.botToken or gateway.auth.token), secrets apply can persist unresolved ref objects that are never converted back to strings at runtime, breaking consumers that expect plain tokens.
Useful? React with 👍 / 👎.
|
Hi! OpenClaw maintainer here. Thanks for opening this. Some thoughts:
See the updated coverage surface area (and currently unsupported credentials) here: https://docs.openclaw.ai/reference/secretref-credential-surface Credentials that are unsupported are currently on our radar for a future release.
If you have any other issues, please open a new issue or PR. Thanks! |
|
Thanks Josh — and nice work on #29580, the coverage surface is much more comprehensive now. Two quick notes: 1. Keychain resolver already works as an exec provider. The 2. Coverage gap check against the new surface. Cross-referencing my deployment with the SecretRef credential surface:
Opened issues for the two remaining gaps:
Thanks for the clear docs page — makes it easy to audit. |
Motivation
This PR extends secrets management (landed in 2026.2.26) with additions from a real-world personal assistant deployment.
Background: Before native secrets existed, @mrkshields (Mark Shields) built a custom
keychain-secretsOpenClaw plugin for his personal deployment. That plugin handled macOS Keychain integration, config header injection, and channel/gateway token secrets entirely outside the config file. When native secrets landed, it revealed exactly which fields are missing from the v1 in-scope list.This PR brings parity for those gaps.
Missing from v1
Real deployments store secrets in fields
secrets auditcurrently misses:channels.telegram.botToken,channels.discord.token,channels.slack.botToken,channels.bluebubbles.passwordgateway.auth.token,hooks.tokengateway.talk.apiKey,gateway.talk.providers.*.apiKeymodels.providers.*.headers.*(e.g.cf-aig-authorizationfor CF AI Gateway)plugins.entries.*.config.*(e.g. Home Assistant API tokenhaToken)Without this coverage,
secrets auditgives false confidence — it misses the most commonly exposed secrets in a typical personal assistant deployment.Changes
scripts/keychain-resolver.ts(new)macOS Keychain exec provider implementing the exec protocol via the
securityCLI. Joins 1Password, HashiCorp Vault, and sops as documented provider examples.Store a secret:
security add-generic-password -U -a "openclaw" -s "TELEGRAM_BOT_TOKEN" -w "your-token"src/secrets/audit.tsAdded
PLAINTEXT_FOUNDchecks for all newly in-scope fields.src/secrets/resolve.ts+src/secrets/apply.tsSecretRef resolution support for all newly in-scope fields.
Config types + Zod schema
Newly in-scope fields typed as
SecretInput(string | SecretRef).🤖 AI-Assisted PR
pnpm buildpasses, existing test patterns extended, but not fully integration-tested on a live deployment beyond the original plugin which this is based onsamshields-oc)