feat: add env, keyring, 1Password, AWS, Azure, and Vault secret providers#24272
Closed
akoscz wants to merge 6 commits into
Closed
feat: add env, keyring, 1Password, AWS, Azure, and Vault secret providers#24272akoscz wants to merge 6 commits into
akoscz wants to merge 6 commits into
Conversation
Comment on lines
+459
to
+461
| for (const { full, value: sv } of resolved) { | ||
| working = working.replace(full, sv); | ||
| } |
Contributor
There was a problem hiding this comment.
Secret values containing $ are corrupted during replacement
String.replace(searchValue, replaceValue) interprets special patterns in replaceValue: $& inserts the matched substring, $` and $' insert text before/after the match, and $$ inserts a literal $. If a resolved secret value contains any of these patterns (e.g., a password like pa$$word or a JSON string with $), the replacement will silently produce corrupted output.
Fix by escaping $ in the replacement value:
Suggested change
| for (const { full, value: sv } of resolved) { | |
| working = working.replace(full, sv); | |
| } | |
| for (const { full, value: sv } of resolved) { | |
| working = working.replace(full, sv.replaceAll("$", "$$$$")); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/secret-resolution.ts
Line: 459-461
Comment:
**Secret values containing `$` are corrupted during replacement**
`String.replace(searchValue, replaceValue)` interprets special patterns in `replaceValue`: `$&` inserts the matched substring, `` $` `` and `$'` insert text before/after the match, and `$$` inserts a literal `$`. If a resolved secret value contains any of these patterns (e.g., a password like `pa$$word` or a JSON string with `$`), the replacement will silently produce corrupted output.
Fix by escaping `$` in the replacement value:
```suggestion
for (const { full, value: sv } of resolved) {
working = working.replace(full, sv.replaceAll("$", "$$$$"));
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+129
to
+133
| const params: Record<string, string> = { SecretId: secretName }; | ||
| if (version) { | ||
| params.VersionId = version; | ||
| params.VersionStage = version; | ||
| } |
Contributor
There was a problem hiding this comment.
VersionId and VersionStage should not both be set to the same value
In the AWS Secrets Manager API, VersionId is a UUID (e.g., a1b2c3d4-5678-...) and VersionStage is a label (e.g., AWSCURRENT, AWSPREVIOUS). Setting both to the same version string means the API call will fail or behave unexpectedly depending on the format of the version string. AWS docs state: "If you specify both, the API will validate that the VersionStage matches the specified VersionId."
You likely want to infer which parameter to set based on the version format, or just use VersionStage:
Suggested change
| const params: Record<string, string> = { SecretId: secretName }; | |
| if (version) { | |
| params.VersionId = version; | |
| params.VersionStage = version; | |
| } | |
| const params: Record<string, string> = { SecretId: secretName }; | |
| if (version) { | |
| // UUID format → VersionId, otherwise treat as a stage label | |
| const isUuid = /^[0-9a-f]{8}-[0-9a-f]{4}-/i.test(version); | |
| if (isUuid) { | |
| params.VersionId = version; | |
| } else { | |
| params.VersionStage = version; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/aws-secret-provider.ts
Line: 129-133
Comment:
**`VersionId` and `VersionStage` should not both be set to the same value**
In the AWS Secrets Manager API, `VersionId` is a UUID (e.g., `a1b2c3d4-5678-...`) and `VersionStage` is a label (e.g., `AWSCURRENT`, `AWSPREVIOUS`). Setting both to the same `version` string means the API call will fail or behave unexpectedly depending on the format of the version string. AWS docs state: "If you specify both, the API will validate that the `VersionStage` matches the specified `VersionId`."
You likely want to infer which parameter to set based on the version format, or just use `VersionStage`:
```suggestion
const params: Record<string, string> = { SecretId: secretName };
if (version) {
// UUID format → VersionId, otherwise treat as a stage label
const isUuid = /^[0-9a-f]{8}-[0-9a-f]{4}-/i.test(version);
if (isUuid) {
params.VersionId = version;
} else {
params.VersionStage = version;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+311
to
+366
| export function buildSecretProviders( | ||
| secretsConfig: SecretsConfig | undefined, | ||
| ): Map<string, SecretProvider> { | ||
| const providers = new Map<string, SecretProvider>(); | ||
| if (!secretsConfig?.providers) { | ||
| return providers; | ||
| } | ||
|
|
||
| for (const [name, config] of Object.entries(secretsConfig.providers)) { | ||
| if (name === "gcp" && config && config.project) { | ||
| providers.set( | ||
| "gcp", | ||
| new GcpSecretProvider( | ||
| config as { project: string; cacheTtlSeconds?: number; credentialsFile?: string }, | ||
| ), | ||
| ); | ||
| } | ||
| if (name === "aws" && config && config.region) { | ||
| providers.set( | ||
| "aws", | ||
| new AwsSecretProvider({ | ||
| region: config.region, | ||
| cacheTtlSeconds: config.cacheTtlSeconds, | ||
| profile: config.profile, | ||
| credentialsFile: config.credentialsFile, | ||
| roleArn: config.roleArn, | ||
| externalId: config.externalId, | ||
| }), | ||
| ); | ||
| } | ||
| if (name === "env") { | ||
| providers.set("env", new EnvSecretProvider()); | ||
| } | ||
| if (name === "keyring") { | ||
| providers.set( | ||
| "keyring", | ||
| new KeyringSecretProvider({ | ||
| account: config?.account, | ||
| keychainPath: config?.keychainPath, | ||
| keychainPassword: config?.keychainPassword, | ||
| }), | ||
| ); | ||
| } | ||
| if (name === "1password") { | ||
| providers.set( | ||
| "1password", | ||
| new OnePasswordSecretProvider({ | ||
| vault: config?.vault, | ||
| field: config?.field, | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return providers; | ||
| } |
Contributor
There was a problem hiding this comment.
Vault and Azure providers not wired into buildSecretProviders
VaultSecretProvider and AzureSecretProvider are implemented and tested but buildSecretProviders() doesn't import or instantiate them. A user who configures vault or azure in their secrets.providers config will get no provider, and any ${vault:...} or ${azure:...} references will throw UnknownSecretProviderError at runtime.
Similarly, the Zod schema (zod-schema.ts) and TypeScript type (types.openclaw.ts) don't include vault or azure entries, so users can't configure them through the standard config path. If these providers are intended to be functional in this PR, they need to be wired in here (with matching schema/type updates). If they're intentionally deferred, consider noting that in the PR description.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/secret-resolution.ts
Line: 311-366
Comment:
**Vault and Azure providers not wired into `buildSecretProviders`**
`VaultSecretProvider` and `AzureSecretProvider` are implemented and tested but `buildSecretProviders()` doesn't import or instantiate them. A user who configures `vault` or `azure` in their `secrets.providers` config will get no provider, and any `${vault:...}` or `${azure:...}` references will throw `UnknownSecretProviderError` at runtime.
Similarly, the Zod schema (`zod-schema.ts`) and TypeScript type (`types.openclaw.ts`) don't include `vault` or `azure` entries, so users can't configure them through the standard config path. If these providers are intended to be functional in this PR, they need to be wired in here (with matching schema/type updates). If they're intentionally deferred, consider noting that in the PR description.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+197
to
+210
| // Secret doesn't exist — create it, then put value | ||
| await client.send( | ||
| new sdk.CreateSecretCommand({ | ||
| Name: secretName, | ||
| SecretString: value, | ||
| }), | ||
| ); | ||
| // CreateSecret already stores the value, but if we want to be explicit: | ||
| await client.send( | ||
| new sdk.PutSecretValueCommand({ | ||
| SecretId: secretName, | ||
| SecretString: value, | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
Redundant PutSecretValueCommand after CreateSecretCommand
CreateSecretCommand with SecretString already stores the initial secret value. The follow-up PutSecretValueCommand on lines 205-210 creates an unnecessary second version of the secret. The comment on line 204 acknowledges this. Consider removing the redundant call:
Suggested change
| // Secret doesn't exist — create it, then put value | |
| await client.send( | |
| new sdk.CreateSecretCommand({ | |
| Name: secretName, | |
| SecretString: value, | |
| }), | |
| ); | |
| // CreateSecret already stores the value, but if we want to be explicit: | |
| await client.send( | |
| new sdk.PutSecretValueCommand({ | |
| SecretId: secretName, | |
| SecretString: value, | |
| }), | |
| ); | |
| // Secret doesn't exist — create it (SecretString stores the initial value) | |
| await client.send( | |
| new sdk.CreateSecretCommand({ | |
| Name: secretName, | |
| SecretString: value, | |
| }), | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/aws-secret-provider.ts
Line: 197-210
Comment:
**Redundant `PutSecretValueCommand` after `CreateSecretCommand`**
`CreateSecretCommand` with `SecretString` already stores the initial secret value. The follow-up `PutSecretValueCommand` on lines 205-210 creates an unnecessary second version of the secret. The comment on line 204 acknowledges this. Consider removing the redundant call:
```suggestion
// Secret doesn't exist — create it (SecretString stores the initial value)
await client.send(
new sdk.CreateSecretCommand({
Name: secretName,
SecretString: value,
}),
);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+106
to
+108
| if (obj.includes("${gcp:")) { | ||
| return results; | ||
| } |
Contributor
There was a problem hiding this comment.
scanForSensitiveValues only skips GCP secret refs
The check obj.includes("${gcp:") only skips values already migrated to GCP. Values referencing other providers like ${aws:...}, ${env:...}, ${vault:...}, ${keyring:...}, or ${1password:...} will still be flagged as sensitive values needing migration. This could lead to double-migration attempts.
Suggested change
| if (obj.includes("${gcp:")) { | |
| return results; | |
| } | |
| // Skip if already a secret ref | |
| if (/\$\{[a-z]+:/.test(obj)) { | |
| return results; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/secrets.ts
Line: 106-108
Comment:
**`scanForSensitiveValues` only skips GCP secret refs**
The check `obj.includes("${gcp:")` only skips values already migrated to GCP. Values referencing other providers like `${aws:...}`, `${env:...}`, `${vault:...}`, `${keyring:...}`, or `${1password:...}` will still be flagged as sensitive values needing migration. This could lead to double-migration attempts.
```suggestion
// Skip if already a secret ref
if (/\$\{[a-z]+:/.test(obj)) {
return results;
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+511
to
+514
| const cacheTtlMs = | ||
| secretsConfig?.providers?.gcp?.cacheTtlSeconds != null | ||
| ? secretsConfig.providers.gcp.cacheTtlSeconds * 1000 | ||
| : defaultTtlMs; |
Contributor
There was a problem hiding this comment.
Cache TTL only reads from GCP provider config
The shared cache TTL is hardcoded to check only secretsConfig.providers.gcp.cacheTtlSeconds. If a user configures only AWS (or another provider) with a custom cacheTtlSeconds, it will be ignored by the shared cache layer. Consider reading the TTL from whichever provider is configured, or taking the max/min across all configured providers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/secret-resolution.ts
Line: 511-514
Comment:
**Cache TTL only reads from GCP provider config**
The shared cache TTL is hardcoded to check only `secretsConfig.providers.gcp.cacheTtlSeconds`. If a user configures only AWS (or another provider) with a custom `cacheTtlSeconds`, it will be ignored by the shared cache layer. Consider reading the TTL from whichever provider is configured, or taking the max/min across all configured providers.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+193
to
+197
| case "expired": | ||
| listener("secret:expiring-soon", r.name, { | ||
| daysUntilExpiry: r.status.daysUntilExpiry, | ||
| expired: true, | ||
| }); |
Contributor
There was a problem hiding this comment.
Wrong event name for expired rotation state
The expired branch on line 194 emits the same event name as the expiring-soon branch above it. Listeners cannot distinguish between an almost-expired and a fully-expired rotation. The event name here should be distinct for the expired case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/rotation-reminders.ts
Line: 193-197
Comment:
**Wrong event name for expired rotation state**
The `expired` branch on line 194 emits the same event name as the `expiring-soon` branch above it. Listeners cannot distinguish between an almost-expired and a fully-expired rotation. The event name here should be distinct for the expired case.
How can I resolve this? If you propose a fix, please make it concise.9f33cbc to
89edfaa
Compare
Member
|
Thanks for pushing this. Since #26155 is now merged, please rebase this branch on latest main and refactor/split the changes into smaller follow-ups (ideally provider-by-provider) so review stays tractable and overlap with merged SecretRef infrastructure is minimized. If you want, we can help define a clean split plan in-thread. |
e20c171 to
429627c
Compare
429627c to
e28289d
Compare
This was referenced Mar 12, 2026
Contributor
Author
akoscz
added a commit
to akoscz/openclaw
that referenced
this pull request
Mar 13, 2026
Adds AwsSecretProvider for resolving secrets from AWS Secrets Manager.
The AWS SDK (`@aws-sdk/client-secrets-manager`) is lazy-loaded as an
optional peer dependency — no startup cost unless the provider is used.
Authentication follows the standard AWS credential chain:
env vars → shared credentials → IAM role → instance profile.
Features:
- Version pinning: ${aws:my-secret#AWSCURRENT}
- Profile + role ARN + externalId support
- Configurable TTL caching
- Secret rotation detection
Config example:
"secrets": {
"providers": {
"aws": { "region": "us-east-1", "profile": "prod" }
}
}
Usage: ${aws:my-secret-name}
Split from openclaw#24272 per @vincentkoc's request.
akoscz
added a commit
to akoscz/openclaw
that referenced
this pull request
Mar 13, 2026
Adds AzureSecretProvider for resolving secrets from Azure Key Vault.
Uses `@azure/keyvault-secrets` + `@azure/identity` (lazy-loaded optional
peer dependencies — no startup cost unless the provider is used).
Authentication via DefaultAzureCredential chain:
- Environment variables (AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET)
- Managed Identity
- Azure CLI (`az login`)
- Service Principal
Features:
- Version pinning: ${azure:my-secret#version-id}
- Configurable TTL caching (default 5 min)
- Custom credentials file support
Config example:
"secrets": {
"providers": {
"azure": {
"vaultUrl": "https://my-vault.vault.azure.net",
"tenantId": "<tenant-id>"
}
}
}
Usage: ${azure:my-secret-name}
Split from openclaw#24272 per @vincentkoc's request.
akoscz
added a commit
to akoscz/openclaw
that referenced
this pull request
Mar 13, 2026
Adds two complementary systems for managing secret lifecycle: **RotationReminders** (`rotation-reminders.ts`): - Track rotation schedules via KV metadata labels in config - Check if secrets are due for rotation - Snooze and acknowledge rotation reminders - CLI: `openclaw secrets check-rotation` **AutoRotation** (`auto-rotation.ts`): - Automatically rotate the OpenClaw gateway token in GCP Secret Manager - Configurable rotation interval (default: 30 days) - Dry-run mode for testing - CLI: `openclaw secrets rotate` **CLI command** (`src/commands/secrets`): - `openclaw secrets list` — list all configured providers and their secrets - `openclaw secrets test` — test provider connectivity - `openclaw secrets rotate` — rotate gateway token - `openclaw secrets check-rotation` — check rotation status - `openclaw secrets purge` — remove plaintext secrets from config Split from openclaw#24272 per @vincentkoc's request.
akoscz
added a commit
to akoscz/openclaw
that referenced
this pull request
Mar 13, 2026
Adds the EnvSecretProvider which resolves secret references from environment
variables. This is useful for local development and CI pipelines that inject
secrets via env vars.
Also includes:
- Secret reference resolution infrastructure (secret-resolution.ts)
- GCP Secret Manager provider (inline in resolution layer)
- Updated config.ts exports
- Updated test setup with GCP mock
- Documentation: docs/concepts/secrets.md, docs/gateway/secrets.md
- Bootstrap scripts: scripts/secrets/
Usage in openclaw.json:
${env:MY_SECRET_VAR}
Split from openclaw#24272 per @vincentkoc's request.
akoscz
added a commit
to akoscz/openclaw
that referenced
this pull request
Mar 13, 2026
Adds two providers for local/desktop credential storage:
**KeyringSecretProvider** — OS-native keychain:
- macOS: Keychain via `keytar` (lazy-loaded optional peer dep)
- Linux: `secret-tool` CLI (libsecret)
- Supports custom keychain path and account names
**OnePasswordSecretProvider** — 1Password CLI (`op`):
- Uses `op read` to resolve secrets from 1Password vaults
- Configurable vault and field names
Config example:
"secrets": {
"providers": {
"keyring": { "account": "openclaw" },
"1password": { "vault": "OpenClaw" }
}
}
Usage: ${keyring:my-secret} or ${1password:my-item.field}
Split from openclaw#24272 per @vincentkoc's request.
akoscz
added a commit
to akoscz/openclaw
that referenced
this pull request
Mar 13, 2026
Adds VaultSecretProvider for resolving secrets from HashiCorp Vault.
Uses native `fetch()` — no SDK dependency.
Authentication methods:
- Token auth (VAULT_TOKEN / tokenFile)
- AppRole (roleId + secretId)
- Kubernetes (service account JWT)
Features:
- KV v2 with version pinning: ${vault:path/to/secret#5}
- Namespace support (Vault Enterprise)
- Configurable mount path (default: "secret")
- Lease management for dynamic secrets
- Rotation tracking via KV v2 custom_metadata
- Configurable TTL caching (default 5 min)
Config example:
"secrets": {
"providers": {
"vault": {
"address": "https://vault.example.com",
"authMethod": "token",
"token": "<token>"
}
}
}
Usage: ${vault:my/secret/path}
Split from openclaw#24272 per @vincentkoc's request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #17311
Summary
Extends the secrets manager integration with additional provider implementations, building on the GCP Secret Manager foundation established in #16663.
Providers Added
Core (env + keyring)
keytarCloud
@aws-sdk/client-secrets-manager(optional dep, dynamically imported)@azure/keyvault-secrets(optional dep, dynamically imported)Self-hosted
1Password
@1password/sdk(optional dep, dynamically imported)Extras
openclaw secretsCLI —list,get,set,deletecommands for managing secrets across providersscripts/secrets/) for bootstrapping GCP and migrating existing config valuesTests
245 tests passing across all providers and rotation logic.
Notes
anytypes, unused imports, andunbound-methodissues from upstream lint rule changes)Greptile Summary
This PR extends the secrets manager integration with six new provider implementations (env, keyring, 1Password, AWS, Azure, Vault), rotation reminders, auto-rotation for gateway tokens, CLI commands, and migration scripts. The providers are well-structured with proper lazy-loading of optional dependencies, good error handling, and comprehensive test coverage (245 tests).
resolveString()insecret-resolution.tsusesString.replace()with the resolved value as the replacement string, which interprets$&,$`,$'as special patterns — values containing dollar signs will be silently corruptedVaultSecretProviderandAzureSecretProviderare fully implemented and tested but are not registered inbuildSecretProviders(), nor included in the Zod schema or TypeScript types — users cannot configure or use thememitRotationEvents()emits the same event name for both the expiring-soon and expired cases, making it impossible for listeners to distinguish between the two statesgetSecret()sets bothVersionId(expects UUID) andVersionStage(expects label likeAWSCURRENT) to the same value, which will cause API failures for versioned lookupsscanForSensitiveValues()only checks for GCP-style references but not refs to other providers, meaning already-migrated values would be flagged againtypes.openclaw.tsonly includesgcpin the type definition while the Zod schema validates five providersConfidence Score: 2/5
src/config/secret-resolution.ts(replacement bug + missing providers),src/config/rotation-reminders.ts(wrong event), andsrc/config/aws-secret-provider.ts(version parameter misuse)Last reviewed commit: 3e59fc1