feat(secrets): add AWS Secrets Manager provider#43643
Conversation
Greptile SummaryThis PR adds
Confidence Score: 1/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/config/aws-secret-provider.ts
Line: 97-111
Comment:
**`roleArn` and `externalId` are silently ignored**
The PR description advertises "Profile selection and role ARN assumption" and "External ID for cross-account access" as features, and `buildSecretProviders` in `secret-resolution.ts` correctly passes `roleArn` and `externalId` to the constructor — but `getClient()` never uses them. `this.roleArn` and `this.externalId` are stored and then completely discarded. Users who configure role assumption will silently fall through to the default credential chain with no warning.
To actually assume a role you'd need something like:
```typescript
import { fromTemporaryCredentials } from "@aws-sdk/credential-providers";
private async getClient(): Promise<AwsSmClient> {
if (this.clientInstance) {
return this.clientInstance;
}
const sdk = await this.getSdk();
const opts: Record<string, unknown> = { region: this.region };
if (this.profile) {
opts.profile = this.profile;
}
if (this.roleArn) {
const { fromTemporaryCredentials } = await import("@aws-sdk/credential-providers");
opts.credentials = fromTemporaryCredentials({
params: {
RoleArn: this.roleArn,
...(this.externalId ? { ExternalId: this.externalId } : {}),
},
});
}
this.clientInstance = new sdk.SecretsManagerClient(opts);
return this.clientInstance;
}
```
At minimum, if role assumption is out of scope for now, the constructor should throw (or warn) when `roleArn` is provided so users aren't silently misled.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/config/aws-secret-provider.ts
Line: 46-48
Comment:
**Module-level cache is shared across all `AwsSecretProvider` instances**
`localCache` is a module-level singleton. If the application creates two `AwsSecretProvider` instances pointing to different regions (e.g. `us-east-1` and `eu-west-1`), they will share the same cache. The cache key on line 120 is `aws:${secretName}#${ver}`, which does not include the region. A secret fetched from `us-east-1` could be returned as a cache hit for the same name requested from `eu-west-1`.
Move the cache inside the class so each instance owns its own entries:
```typescript
export class AwsSecretProvider implements SecretProvider {
private readonly cache = new Map<string, CacheEntry>();
// ...
}
```
And update `clearAwsSecretCache` (used in tests) to accept an optional provider instance, or expose an instance method instead.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/config/aws-secret-provider.ts
Line: 196-214
Comment:
**`CreateSecret` + redundant `PutSecretValue` creates an extra secret version**
`CreateSecretCommand` already stores the initial `SecretString` value as version `AWSCURRENT`. Immediately following it with `PutSecretValueCommand` using the same value creates a second, identical version of the secret. This doubles the API calls and pollutes the secret's version history with a no-op entry. The inline comment acknowledges this ("CreateSecret already stores the value") but the extra call is still executed unconditionally.
```suggestion
// Secret doesn't exist — create it (CreateSecret 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.
---
This is a comment left during a code review.
Path: src/config/secret-resolution.ts
Line: 484-489
Comment:
**Upper-layer cache TTL is hard-coded to the GCP provider's setting, ignoring AWS TTL**
The TTL used for `fetchWithCache` is read exclusively from `secretsConfig?.providers?.gcp?.cacheTtlSeconds`. When only the AWS provider is configured (no `gcp` block), this expression is `undefined` and the default 5-minute TTL is used, regardless of whatever `cacheTtlSeconds` the user set in their `aws` provider config block. The AWS-configured TTL is effectively ignored at the upper cache layer.
A more correct approach would be to derive the TTL per-provider during resolution, or fall back to the first configured provider's TTL:
```typescript
const firstProviderCfg = secretsConfig?.providers
? Object.values(secretsConfig.providers)[0]
: undefined;
const cacheTtlMs =
firstProviderCfg?.cacheTtlSeconds != null
? firstProviderCfg.cacheTtlSeconds * 1000
: defaultTtlMs;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/config/aws-secret-provider.ts
Line: 129-133
Comment:
**`VersionId` and `VersionStage` set to the same value breaks version pinning**
`VersionId` and `VersionStage` are distinct parameters in AWS Secrets Manager:
- `VersionId` — a unique rotation identifier (a hex string like those returned by the API)
- `VersionStage` — a staging label like `"AWSCURRENT"` or `"AWSPREVIOUS"`
Setting both to the same arbitrary string causes all version-pinned requests to fail:
- `${aws:my-secret#AWSCURRENT}` → sends `VersionId: "AWSCURRENT"` which is not a valid version identifier; the API rejects it.
- `${aws:my-secret#<hex-id>}` → sends `VersionStage: "<hex-id>"` which is not a valid stage label; the API also rejects it.
The PR description's own example (`${aws:my-secret#AWSCURRENT}`) will fail at runtime. The two parameters need to be sent separately based on which type the caller provides:
```typescript
if (version) {
// AWSCURRENT / AWSPREVIOUS are staging labels; anything else is a version identifier
if (/^AWS[A-Z]+$/.test(version)) {
params.VersionStage = version;
} else {
params.VersionId = version;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 889c43e |
| private async getClient(): Promise<AwsSmClient> { | ||
| if (this.clientInstance) { | ||
| return this.clientInstance; | ||
| } | ||
| const sdk = await this.getSdk(); | ||
| const opts: Record<string, unknown> = { region: this.region }; | ||
|
|
||
| // Credential chain configuration | ||
| if (this.profile) { | ||
| opts.profile = this.profile; | ||
| } | ||
|
|
||
| this.clientInstance = new sdk.SecretsManagerClient(opts); | ||
| return this.clientInstance; | ||
| } |
There was a problem hiding this comment.
roleArn and externalId are silently ignored
The PR description advertises "Profile selection and role ARN assumption" and "External ID for cross-account access" as features, and buildSecretProviders in secret-resolution.ts correctly passes roleArn and externalId to the constructor — but getClient() never uses them. this.roleArn and this.externalId are stored and then completely discarded. Users who configure role assumption will silently fall through to the default credential chain with no warning.
To actually assume a role you'd need something like:
import { fromTemporaryCredentials } from "@aws-sdk/credential-providers";
private async getClient(): Promise<AwsSmClient> {
if (this.clientInstance) {
return this.clientInstance;
}
const sdk = await this.getSdk();
const opts: Record<string, unknown> = { region: this.region };
if (this.profile) {
opts.profile = this.profile;
}
if (this.roleArn) {
const { fromTemporaryCredentials } = await import("@aws-sdk/credential-providers");
opts.credentials = fromTemporaryCredentials({
params: {
RoleArn: this.roleArn,
...(this.externalId ? { ExternalId: this.externalId } : {}),
},
});
}
this.clientInstance = new sdk.SecretsManagerClient(opts);
return this.clientInstance;
}At minimum, if role assumption is out of scope for now, the constructor should throw (or warn) when roleArn is provided so users aren't silently misled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/aws-secret-provider.ts
Line: 97-111
Comment:
**`roleArn` and `externalId` are silently ignored**
The PR description advertises "Profile selection and role ARN assumption" and "External ID for cross-account access" as features, and `buildSecretProviders` in `secret-resolution.ts` correctly passes `roleArn` and `externalId` to the constructor — but `getClient()` never uses them. `this.roleArn` and `this.externalId` are stored and then completely discarded. Users who configure role assumption will silently fall through to the default credential chain with no warning.
To actually assume a role you'd need something like:
```typescript
import { fromTemporaryCredentials } from "@aws-sdk/credential-providers";
private async getClient(): Promise<AwsSmClient> {
if (this.clientInstance) {
return this.clientInstance;
}
const sdk = await this.getSdk();
const opts: Record<string, unknown> = { region: this.region };
if (this.profile) {
opts.profile = this.profile;
}
if (this.roleArn) {
const { fromTemporaryCredentials } = await import("@aws-sdk/credential-providers");
opts.credentials = fromTemporaryCredentials({
params: {
RoleArn: this.roleArn,
...(this.externalId ? { ExternalId: this.externalId } : {}),
},
});
}
this.clientInstance = new sdk.SecretsManagerClient(opts);
return this.clientInstance;
}
```
At minimum, if role assumption is out of scope for now, the constructor should throw (or warn) when `roleArn` is provided so users aren't silently misled.
How can I resolve this? If you propose a fix, please make it concise.| // Cache for internal use (provider-level, separate from shared cache in secret-resolution.ts) | ||
| type CacheEntry = { value: string; expiresAt: number }; | ||
| const localCache = new Map<string, CacheEntry>(); |
There was a problem hiding this comment.
Module-level cache is shared across all AwsSecretProvider instances
localCache is a module-level singleton. If the application creates two AwsSecretProvider instances pointing to different regions (e.g. us-east-1 and eu-west-1), they will share the same cache. The cache key on line 120 is aws:${secretName}#${ver}, which does not include the region. A secret fetched from us-east-1 could be returned as a cache hit for the same name requested from eu-west-1.
Move the cache inside the class so each instance owns its own entries:
export class AwsSecretProvider implements SecretProvider {
private readonly cache = new Map<string, CacheEntry>();
// ...
}And update clearAwsSecretCache (used in tests) to accept an optional provider instance, or expose an instance method instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/aws-secret-provider.ts
Line: 46-48
Comment:
**Module-level cache is shared across all `AwsSecretProvider` instances**
`localCache` is a module-level singleton. If the application creates two `AwsSecretProvider` instances pointing to different regions (e.g. `us-east-1` and `eu-west-1`), they will share the same cache. The cache key on line 120 is `aws:${secretName}#${ver}`, which does not include the region. A secret fetched from `us-east-1` could be returned as a cache hit for the same name requested from `eu-west-1`.
Move the cache inside the class so each instance owns its own entries:
```typescript
export class AwsSecretProvider implements SecretProvider {
private readonly cache = new Map<string, CacheEntry>();
// ...
}
```
And update `clearAwsSecretCache` (used in tests) to accept an optional provider instance, or expose an instance method instead.
How can I resolve this? If you propose a fix, please make it concise.| if ((err as Record<string, unknown>)?.name === "ResourceNotFoundException") { | ||
| // 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, | ||
| }), | ||
| ); | ||
| } else { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
CreateSecret + redundant PutSecretValue creates an extra secret version
CreateSecretCommand already stores the initial SecretString value as version AWSCURRENT. Immediately following it with PutSecretValueCommand using the same value creates a second, identical version of the secret. This doubles the API calls and pollutes the secret's version history with a no-op entry. The inline comment acknowledges this ("CreateSecret already stores the value") but the extra call is still executed unconditionally.
| if ((err as Record<string, unknown>)?.name === "ResourceNotFoundException") { | |
| // 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, | |
| }), | |
| ); | |
| } else { | |
| throw err; | |
| } | |
| } | |
| // Secret doesn't exist — create it (CreateSecret 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: 196-214
Comment:
**`CreateSecret` + redundant `PutSecretValue` creates an extra secret version**
`CreateSecretCommand` already stores the initial `SecretString` value as version `AWSCURRENT`. Immediately following it with `PutSecretValueCommand` using the same value creates a second, identical version of the secret. This doubles the API calls and pollutes the secret's version history with a no-op entry. The inline comment acknowledges this ("CreateSecret already stores the value") but the extra call is still executed unconditionally.
```suggestion
// Secret doesn't exist — create it (CreateSecret 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.| // Default cache TTL: 5 minutes | ||
| const defaultTtlMs = 300_000; | ||
| const cacheTtlMs = | ||
| secretsConfig?.providers?.gcp?.cacheTtlSeconds != null | ||
| ? secretsConfig.providers.gcp.cacheTtlSeconds * 1000 | ||
| : defaultTtlMs; |
There was a problem hiding this comment.
Upper-layer cache TTL is hard-coded to the GCP provider's setting, ignoring AWS TTL
The TTL used for fetchWithCache is read exclusively from secretsConfig?.providers?.gcp?.cacheTtlSeconds. When only the AWS provider is configured (no gcp block), this expression is undefined and the default 5-minute TTL is used, regardless of whatever cacheTtlSeconds the user set in their aws provider config block. The AWS-configured TTL is effectively ignored at the upper cache layer.
A more correct approach would be to derive the TTL per-provider during resolution, or fall back to the first configured provider's TTL:
const firstProviderCfg = secretsConfig?.providers
? Object.values(secretsConfig.providers)[0]
: undefined;
const cacheTtlMs =
firstProviderCfg?.cacheTtlSeconds != null
? firstProviderCfg.cacheTtlSeconds * 1000
: defaultTtlMs;Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/secret-resolution.ts
Line: 484-489
Comment:
**Upper-layer cache TTL is hard-coded to the GCP provider's setting, ignoring AWS TTL**
The TTL used for `fetchWithCache` is read exclusively from `secretsConfig?.providers?.gcp?.cacheTtlSeconds`. When only the AWS provider is configured (no `gcp` block), this expression is `undefined` and the default 5-minute TTL is used, regardless of whatever `cacheTtlSeconds` the user set in their `aws` provider config block. The AWS-configured TTL is effectively ignored at the upper cache layer.
A more correct approach would be to derive the TTL per-provider during resolution, or fall back to the first configured provider's TTL:
```typescript
const firstProviderCfg = secretsConfig?.providers
? Object.values(secretsConfig.providers)[0]
: undefined;
const cacheTtlMs =
firstProviderCfg?.cacheTtlSeconds != null
? firstProviderCfg.cacheTtlSeconds * 1000
: defaultTtlMs;
```
How can I resolve this? If you propose a fix, please make it concise.| const params: Record<string, string> = { SecretId: secretName }; | ||
| if (version) { | ||
| params.VersionId = version; | ||
| params.VersionStage = version; | ||
| } |
There was a problem hiding this comment.
VersionId and VersionStage set to the same value breaks version pinning
VersionId and VersionStage are distinct parameters in AWS Secrets Manager:
VersionId— a unique rotation identifier (a hex string like those returned by the API)VersionStage— a staging label like"AWSCURRENT"or"AWSPREVIOUS"
Setting both to the same arbitrary string causes all version-pinned requests to fail:
${aws:my-secret#AWSCURRENT}→ sendsVersionId: "AWSCURRENT"which is not a valid version identifier; the API rejects it.${aws:my-secret#<hex-id>}→ sendsVersionStage: "<hex-id>"which is not a valid stage label; the API also rejects it.
The PR description's own example (${aws:my-secret#AWSCURRENT}) will fail at runtime. The two parameters need to be sent separately based on which type the caller provides:
if (version) {
// AWSCURRENT / AWSPREVIOUS are staging labels; anything else is a version identifier
if (/^AWS[A-Z]+$/.test(version)) {
params.VersionStage = version;
} else {
params.VersionId = 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` set to the same value breaks version pinning**
`VersionId` and `VersionStage` are distinct parameters in AWS Secrets Manager:
- `VersionId` — a unique rotation identifier (a hex string like those returned by the API)
- `VersionStage` — a staging label like `"AWSCURRENT"` or `"AWSPREVIOUS"`
Setting both to the same arbitrary string causes all version-pinned requests to fail:
- `${aws:my-secret#AWSCURRENT}` → sends `VersionId: "AWSCURRENT"` which is not a valid version identifier; the API rejects it.
- `${aws:my-secret#<hex-id>}` → sends `VersionStage: "<hex-id>"` which is not a valid stage label; the API also rejects it.
The PR description's own example (`${aws:my-secret#AWSCURRENT}`) will fail at runtime. The two parameters need to be sent separately based on which type the caller provides:
```typescript
if (version) {
// AWSCURRENT / AWSPREVIOUS are staging labels; anything else is a version identifier
if (/^AWS[A-Z]+$/.test(version)) {
params.VersionStage = version;
} else {
params.VersionId = version;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 889c43eb8f
ℹ️ 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".
|
|
||
| // Credential chain configuration | ||
| if (this.profile) { | ||
| opts.profile = this.profile; | ||
| } |
There was a problem hiding this comment.
Apply assume-role options when constructing AWS client
The provider accepts credentialsFile, roleArn, and externalId in AwsProviderConfig, but getClient() only passes region (and profile) into SecretsManagerClient. In deployments that rely on cross-account role assumption or non-default credential files, ${aws:...} will still run under the default credential chain and fail with access errors even though the config appears valid.
Useful? React with 👍 / 👎.
| if (version) { | ||
| params.VersionId = version; | ||
| params.VersionStage = version; | ||
| } |
There was a problem hiding this comment.
Map secret version to either VersionId or VersionStage
getSecret() writes the same version token into both VersionId and VersionStage, but these are different selectors in Secrets Manager. This breaks versioned references for common inputs (for example staging labels vs immutable version IDs), so ${aws:name#...} can fail at runtime with invalid request/parameter errors instead of resolving the secret.
Useful? React with 👍 / 👎.
| secretsConfig?.providers?.gcp?.cacheTtlSeconds != null | ||
| ? secretsConfig.providers.gcp.cacheTtlSeconds * 1000 | ||
| : defaultTtlMs; |
There was a problem hiding this comment.
Read cache TTL from the referenced provider config
The shared resolution cache TTL is derived only from secrets.providers.gcp.cacheTtlSeconds, so AWS-backed references ignore secrets.providers.aws.cacheTtlSeconds and silently fall back to 5 minutes. When operators tune AWS TTLs (especially shorter TTLs for rotation), resolved values stay stale longer than configured.
Useful? React with 👍 / 👎.
ab3f93a to
6a0a6d4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a0a6d41e1
ℹ️ 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".
| export async function resolveConfigSecrets( | ||
| obj: unknown, | ||
| secretsConfig: SecretsConfig | undefined, | ||
| providersOverride?: Map<string, SecretProvider>, |
There was a problem hiding this comment.
Invoke secret resolution in the config read pipeline
resolveConfigSecrets is added but never integrated into the normal config load flow: createConfigIO.loadConfig() still resolves includes/env vars and validates directly (src/config/io.ts:698-773) without calling this function, so ${aws:...}/${gcp:...} placeholders remain literal at runtime. This makes the new external secret reference feature non-functional for standard openclaw.json loading.
Useful? React with 👍 / 👎.
| if (name === "aws" && config && config.region) { | ||
| providers.set( | ||
| "aws", | ||
| new AwsSecretProvider({ | ||
| region: config.region, |
There was a problem hiding this comment.
Accept AWS/GCP provider entries in secrets schema
buildSecretProviders expects provider objects shaped like secrets.providers.aws.region / secrets.providers.gcp.project, but config validation still requires secrets.providers.* to match the existing discriminated union (source: env|file|exec) in src/config/zod-schema.core.ts:142-155, and validation runs during config load (src/config/io.ts:772). As a result, the new AWS/GCP provider configs are rejected before this builder can ever run.
Useful? React with 👍 / 👎.
| const cacheKey = `${provider.name}:${name}#${version ?? "latest"}`; | ||
| const cached = secretCache.get(cacheKey); |
There was a problem hiding this comment.
Include provider config fields in shared cache key
The shared cache key only uses provider.name, secret name, and version, so different AWS regions/profiles/roles (or different GCP projects) collide under the same key. After provider config changes (for example, config reload from one account/region to another), stale values can be returned from secretCache until TTL expiry, yielding the wrong secret for the active environment.
Useful? React with 👍 / 👎.
6a0a6d4 to
9f0ad88
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0ad88c05
ℹ️ 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".
| async getSecret(secretName: string, version?: string): Promise<string> { | ||
| // Check local cache | ||
| const ver = version ?? "latest"; | ||
| const cacheKey = `aws:${secretName}#${ver}`; |
There was a problem hiding this comment.
Include region/profile in AWS cache key
getSecret caches entries under aws:${secretName}#${ver} without any provider configuration in the key, and the cache is shared at module scope, so switching to a different AWS region/profile/role can return a stale value from the previous account until TTL expiry. This can resolve the wrong secret after config reloads or multi-environment runs; scope the cache per provider instance or include config identity (region/profile/role) in the key.
Useful? React with 👍 / 👎.
| // Matches ${provider:name} or ${provider:name#version} | ||
| // Provider: lowercase alpha. Name: alphanum, hyphens, underscores, slashes, dots. | ||
| // Version (optional): after #, alphanumeric. | ||
| const SECRET_REF_PATTERN = /\$\{([a-z]+):([a-zA-Z0-9_\-/.]+?)(?:#([a-zA-Z0-9]+))?\}/g; |
There was a problem hiding this comment.
Accept hyphenated version IDs in secret refs
The secret reference regex only allows alphanumeric version suffixes (#([a-zA-Z0-9]+)), but AWS Secrets Manager VersionId values are UUID-like and include hyphens, so references like ${aws:my-secret#123e4567-e89b-12d3-a456-426614174000} are not matched and remain unresolved literals. This breaks immutable version pinning for AWS even when #version syntax is configured.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4158b88589
ℹ️ 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".
| @@ -343,6 +343,7 @@ | |||
| "@buape/carbon": "0.0.0-beta-20260216184201", | |||
| "@clack/prompts": "^1.1.0", | |||
| "@discordjs/voice": "^0.19.1", | |||
| "@google-cloud/secret-manager": "^6.1.1", | |||
There was a problem hiding this comment.
Add AWS SDK dependency for Secrets Manager provider
AwsSecretProvider.getSdk() dynamically imports @aws-sdk/client-secrets-manager (src/config/aws-secret-provider.ts), but this commit only adds @google-cloud/secret-manager to dependencies, so standard installs do not include the AWS client package. In packaged CLI deployments, any ${aws:...} resolution will fail at runtime with the "Please install @aws-sdk/client-secrets-manager" error instead of resolving secrets.
Useful? React with 👍 / 👎.
| for (const { full, value: sv } of resolved) { | ||
| working = working.replace(full, sv); |
There was a problem hiding this comment.
Replace refs by position, not repeated first-match replace
The replacement loop uses working.replace(full, sv) for each original match, which always replaces the first current occurrence. If an earlier resolved value contains another placeholder token from the same string, later iterations can replace the injected text and leave the original placeholder unresolved (or mis-resolved), producing incorrect config values in strings like ${aws:a}${aws:b} when a resolves to text containing ${aws:b}.
Useful? React with 👍 / 👎.
4158b88 to
87be2e4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87be2e4a77
ℹ️ 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".
| for (const [name, config] of Object.entries(secretsConfig.providers)) { | ||
| if (name === "gcp" && config && config.project) { | ||
| providers.set( | ||
| "gcp", | ||
| new GcpSecretProvider( |
There was a problem hiding this comment.
Register AWS provider in provider factory
When resolveConfigSecrets builds providers from config, buildSecretProviders only adds gcp and env, so a config that declares secrets.providers.aws still cannot resolve ${aws:...} references and will fail with UnknownSecretProviderError unless callers manually inject providersOverride (as tests do). This leaves the AWS provider effectively unusable in the default resolution path.
Useful? React with 👍 / 👎.
| [[ -v SECRETS_TO_MIGRATE["openclaw-main-github-pat"] ]] && \ | ||
| purge_file "$HOME/.git-credentials" "openclaw-main-github-pat" |
There was a problem hiding this comment.
Preserve unrelated entries when purging git credentials
If a GitHub PAT is migrated, the purge step rewrites the entire ~/.git-credentials file to a migration note, which deletes any other stored credentials in that file (for other hosts/accounts) after a single confirmation. This can cause unintended credential loss outside the secrets being migrated.
Useful? React with 👍 / 👎.
87be2e4 to
fe45a1b
Compare
fe45a1b to
124a140
Compare
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.
VersionId is for UUID version identifiers, VersionStage is for labels like AWSCURRENT/AWSPREVIOUS. Previously both were set to the same value.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 124a140778
ℹ️ 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".
| '@google-cloud/secret-manager': | ||
| specifier: ^6.1.1 | ||
| version: 6.1.1 |
There was a problem hiding this comment.
Remove unsynced GCP specifier from root lockfile importer
Adding @google-cloud/secret-manager to the root importer without a matching package.json dependency makes the lockfile inconsistent and breaks reproducible installs: pnpm install --frozen-lockfile fails with ERR_PNPM_OUTDATED_LOCKFILE because the lockfile specifiers no longer match the manifest. This blocks CI/install workflows until the lockfile and manifest are synchronized.
Useful? React with 👍 / 👎.
| async setSecret(secretName: string, value: string): Promise<void> { | ||
| const sdk = await this.getSdk(); | ||
| const client = await this.getClient(); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Invalidate AWS cache entries after setSecret writes
setSecret updates Secrets Manager but never clears or refreshes localCache, so a process that previously called getSecret can keep returning stale values for aws:<name>#latest until TTL expiry (default 5 minutes). This breaks read-after-write behavior in rotation/update flows where callers set a secret and immediately read it back.
Useful? React with 👍 / 👎.
124a140 to
2089b6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2089b6e4a8
ℹ️ 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".
| * Authentication follows the AWS credential chain: env vars → shared credentials → IAM role → instance profile. | ||
| */ | ||
|
|
||
| import { type SecretProvider } from "./secret-resolution.js"; |
There was a problem hiding this comment.
Point SecretProvider import at an existing module
AwsSecretProvider imports SecretProvider from ./secret-resolution.js, but this commit does not include a src/config/secret-resolution.ts/.js module, so module resolution for this file fails in type-check/build flows. As written, the new provider cannot be compiled in this snapshot until the type import targets a real module.
Useful? React with 👍 / 👎.
| await client.send( | ||
| new sdk.PutSecretValueCommand({ | ||
| SecretId: secretName, | ||
| SecretString: value, | ||
| }), |
There was a problem hiding this comment.
Remove redundant PutSecretValue after CreateSecret
In the ResourceNotFoundException fallback, CreateSecret is already called with SecretString, then a second PutSecretValue is sent unconditionally. That extra write is unnecessary and can make setSecret fail in least-privilege IAM setups that allow CreateSecret but not PutSecretValue, even though the secret was successfully created by the first call.
Useful? React with 👍 / 👎.
| try { | ||
| const sdk = await this.getSdk(); | ||
| const client = await this.getClient(); | ||
| await client.send(new sdk.ListSecretsCommand({ MaxResults: 1 })); |
There was a problem hiding this comment.
Avoid ListSecrets-only check in testConnection
testConnection() currently treats ListSecrets as the health probe, which returns a false failure when policies grant scoped GetSecretValue/PutSecretValue access but intentionally deny ListSecrets. In those least-privilege environments, secret resolution can still work while this method reports { ok: false }, so the check is stricter than the provider’s primary read/write path.
Useful? React with 👍 / 👎.
|
Thanks for the contribution! After reviewing this PR alongside the related secrets provider PRs (#43640, #43641, #43643, #43644, #43645, #43646), we need to close these for an architectural rework before they can land. The issue: parallel implementation instead of extending the existing systemOpenClaw already has a mature secrets infrastructure in These PRs introduce a new parallel system — a new There's also a hidden dependency across the PR set: the aws, azure, keyring, and vault branches all import What we need insteadNew cloud-backend providers (AWS Secrets Manager, Azure Key Vault, HashiCorp Vault, OS keyring, env vars) should be implemented as resolvers within the existing
We'd love to see these re-submitted with the corrected architecture. |
|
Hi @akoscz , just one more question here: is it possible to change to use openclaw plugin to implement each secret provider rather than placing it in core code ? See issue #71593 , currently, if we want to cover some core fields, we'll still need to change the core codes which is not a good way, just wonder if it's possible to make this pluggable, thanks. |
|
@Colstuwjx Yes — agreed, and I just opened #72548 for this. Closes #71593. ShapeMirrors
What this delivers vs what it doesn't
Verified end-to-end against real backendsBoth reference plugins were live-tested against real backends, not just mocks: Linux libsecret (kwalletd6), macOS Keychain ( Happy to walk through any part of the branch in either direction. |
Summary
Adds
AwsSecretProviderfor 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 configured.Authentication
Standard AWS credential chain: env vars → shared credentials file → IAM role → EC2 instance profile.
Features
${aws:my-secret#AWSCURRENT}Config
{ "secrets": { "providers": { "aws": { "region": "us-east-1", "profile": "prod", "roleArn": "arn:aws:iam::123456789:role/openclaw" } } } }Usage:
${aws:my-secret-name}Notes
Split from #24272 per @vincentkoc's request.