fix(mainnet): deny local file secrets store, source from env vars (#386)#459
fix(mainnet): deny local file secrets store, source from env vars (#386)#459
Conversation
…only (#386) Mainnet Host.Api now opts out of the read/write `AevatarSecretsStore` (which backs `~/.aevatar/secrets.json`) and instead receives a read-only `EnvironmentSecretsStore` sourced from `IConfiguration` + `AEVATAR_`-prefixed environment variables. `Set` / `Remove` throw `InvalidOperationException` so misconfigured callers fail at the call site rather than silently landing secrets on disk. The toggle is exposed at the host level via `AevatarDefaultHostOptions.AllowLocalFileSecretsStore` and threaded through `AddAevatarConfig` (config + DI) and `AddAevatarBootstrap`. Localnet, CLI, demos, and tests keep the default behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09a613dd13
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| options.AllowLocalFileSecretsStore = false; | ||
| configureHost?.Invoke(options); |
There was a problem hiding this comment.
This hardening can be bypassed because configureHost?.Invoke(options) runs after options.AllowLocalFileSecretsStore = false; any caller passing a customization callback can flip the flag back to true and re-enable ~/.aevatar/secrets.json loading/writing. In mainnet deployments that use AddAevatarMainnetHost(..., configureHost), this reintroduces the exact local secret persistence risk this commit is meant to eliminate.
Useful? React with 👍 / 👎.
| services.TryAddSingleton<IAevatarSecretsStore, EnvironmentSecretsStore>(); | ||
| services.TryAddSingleton<EnvironmentSecretsStore>(); |
There was a problem hiding this comment.
When allowLocalFileStore is false, DI now registers EnvironmentSecretsStore, which requires IConfiguration in its constructor, but this extension does not ensure an IConfiguration service exists. In plain ServiceCollection usage (outside WebApplicationBuilder), resolving IAevatarSecretsStore will fail at runtime unless callers manually add configuration, which is a regression introduced by the new environment-only path.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## dev #459 +/- ##
==========================================
+ Coverage 70.50% 70.91% +0.41%
==========================================
Files 1198 1210 +12
Lines 86188 87113 +925
Branches 11297 11426 +129
==========================================
+ Hits 60764 61775 +1011
+ Misses 21037 20884 -153
- Partials 4387 4454 +67
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 32 files with indirect coverage changes 🚀 New features to boost your workflow:
|
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
I found one issue worth addressing:
configureHost?.Invoke(options) runs AFTER AllowLocalFileSecretsStore = false
In src/Aevatar.Mainnet.Host.Api/Hosting/MainnetHostBuilderExtensions.cs:48-49:
options.AllowLocalFileSecretsStore = false; // line 48
configureHost?.Invoke(options); // line 49The user callback at line 49 can freely set AllowLocalFileSecretsStore = true, silently re-enabling the local file secrets store on mainnet. This contradicts the stated goal: "只要入口是 mainnet，就在框架层切断". The two lines should be swapped so false is enforced after the caller is done configuring:
configureHost?.Invoke(options); // let caller configure
options.AllowLocalFileSecretsStore = false; // then enforce mainnet invariantOtherwise the rest looks clean — the EnvironmentSecretsStore fail-fast design is good, the bool parameter threading is consistent, and the tests cover the key paths.
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
Review: mainnet deny local file secrets store
Overall well-scoped and clearly reasoned. A few issues:
1. GetAll() dumps entire IConfiguration — not just secrets
EnvironmentSecretsStore.GetAll() calls _configuration.AsEnumerable() and returns every config key-value pair. In mainnet this means connection strings, feature flags, binding URLs — all exposed through the secrets store surface. AevatarSecretsStore.GetAll() only returns entries from secrets.json.
This is a security concern (secrets API becomes a config exfiltration vector) and a semantic mismatch between the two implementations.
Suggestion: filter to AEVATAR_-prefixed keys only, or document that GetAll() is not secrets-scoped.
2. AddAevatarConfig(IServiceCollection, bool) — EnvironmentSecretsStore requires IConfiguration in DI
The TryAddSingleton<EnvironmentSecretsStore>() registration relies on IConfiguration being in the DI container. This works through WebApplicationBuilder / host builder, but if anyone calls AddAevatarConfig(allowLocalFileStore: false) on a bare ServiceCollection without IConfiguration registered, it throws at resolution time. Consider a doc comment noting the prerequisite.
3. AevatarPaths.EnsureDirectories() still runs when allowLocalFileStore: false
On a mainnet host that wants zero local file footprint, AddAevatarConfig() still creates ~/.aevatar/ directory tree. Slightly undermines the "no local file" guarantee. Consider guarding this behind the same flag.
Summary
Main actionable issue is #1. The rest are minor/defensive. The PR correctly solves the stated problem with minimal surface area change.
| // Secrets are injected via AEVATAR_-prefixed environment variables | ||
| // managed by the deploy platform; Set/Remove on the secrets store | ||
| // will throw at the call site instead of silently writing to disk. | ||
| options.AllowLocalFileSecretsStore = false; |
There was a problem hiding this comment.
P1: Mainnet AI bootstrap still reads the local secrets file
This switch only affects the config builder and the DI IAevatarSecretsStore registration. The same mainnet startup continues into builder.AddAevatarPlatform(...), which registers AI features; that path defaults AevatarAIFeatureOptions.SecretsStore to null, and CreateSecretsStoreAccessor falls back to new AevatarSecretsStore(). As a result, mainnet can still read ~/.aevatar/secrets.json when building the LLM provider factory, bypassing the environment-only store configured here. Please thread the registered/configured EnvironmentSecretsStore into AddAevatarAIFeatures for mainnet, or remove the direct file-store fallback from production composition.
- MainnetHostBuilderExtensions: enforce `AllowLocalFileSecretsStore = false` AFTER `configureHost?.Invoke(options)`, so user callbacks cannot silently re-enable the local file secrets store on mainnet (codex P1 + reviewer). - Aevatar.Bootstrap.Extensions.AI: `CreateSecretsStoreAccessor` now resolves the DI-registered `IAevatarSecretsStore` before falling back to a fresh `AevatarSecretsStore()`. Prevents the LLM provider factory from re-opening `~/.aevatar/secrets.json` on a host that opted into `EnvironmentSecretsStore` (reviewer P1). - EnvironmentSecretsStore.GetAll(): restrict to secret-shaped keys (`LLMProviders:` prefix or `*_API_KEY` suffix) instead of dumping the entire IConfiguration view. Prevents arbitrary connection strings, feature flags and binding URLs from leaking through the secrets API surface (reviewer security concern). - AevatarConfigLoader / Configuration ServiceCollectionExtensions: skip `AevatarPaths.EnsureDirectories()` when `allowLocalFileStore=false`, so mainnet keeps a zero local-file footprint (reviewer minor). - Configuration ServiceCollectionExtensions: doc-note the `IConfiguration` prerequisite for the env-only path (codex P2 + reviewer minor). - Tests: assert mainnet ignores caller override, AI bootstrap honors the DI store, GetAll filters to secret-shaped keys, and `false` does not create the `~/.aevatar/` tree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed all five review points in 27741f5.
Verification:
|
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
Fix looks solid. All issues from the first review are addressed with thoughtful guardrails:
Resolved:
- Ordering —
configureHost?.Invokenow runs beforeAllowLocalFileSecretsStore = falseinMainnetHostBuilderExtensions, enforcing the invariant regardless of caller input. GetAll()leakage — narrowed to secret-shaped keys only (LLMProviders:*/*_API_KEY), no longer dumping arbitrary config.- Directory creation on mainnet —
EnsureDirectories()is now gated behindallowLocalFileStorein bothAevatarConfigLoaderandServiceCollectionExtensions. - Bonus fix —
CreateSecretsStoreAccessorinAevatar.Bootstrap.Extensions.AInow resolves the DI-registered store viaIServiceProviderinstead of falling back tonew AevatarSecretsStore(), closing a path where AI feature bootstrap could re-opensecrets.jsonon mainnet. - Test —
MainnetSecretsStoreInvariantTestsexplicitly verifies that a caller override is ignored, plus a new test for the directory-creation guard.
LGTM.
| bool allowLocalFileSecretsStore = true) | ||
| { | ||
| services.AddAevatarConfig(); | ||
| services.AddAevatarConfig(allowLocalFileSecretsStore); |
There was a problem hiding this comment.
P2: AddAevatarBootstrap(..., false) still leaves EnvironmentSecretsStore unresolvable
The env-only path still depends on callers pre-registering IConfiguration even when they use this bootstrap overload, which already receives the configuration instance. In plain ServiceCollection usage, services.AddAevatarBootstrap(configuration, allowLocalFileSecretsStore: false) registers EnvironmentSecretsStore, but resolving IAevatarSecretsStore later fails because DI has no IConfiguration. The new test masks this by calling services.AddSingleton(configuration) before the bootstrap call; production callers of this overload should not need that extra undocumented step. Please register the passed configuration here, e.g. services.TryAddSingleton<IConfiguration>(configuration), before AddAevatarConfig(false).
…-only path resolves without caller workaround (#386) `AddAevatarBootstrap(IServiceCollection, IConfiguration, bool)` already receives the configuration instance. Forward it to DI via TryAddSingleton before AddAevatarConfig so EnvironmentSecretsStore (registered when allowLocalFileSecretsStore=false) can resolve in plain ServiceCollection usage too. TryAdd preserves any existing host-builder registration. Also drops the manual services.AddSingleton(configuration) workaround from BootstrapServiceCollectionExtensionsTests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed the new P2 in aeee185.
|
1 participant
Closes #386.
问题
AevatarSecretsStore在所有宿主中默认注册（经AddAevatarConfig()），把 secrets 落地到~/.aevatar/secrets.json。Mainnet 项目继承这一行为，生产环境因此存在把 API key 等敏感信息写到本地文件的风险。方案
不依赖运行时配置：只要入口是 mainnet，就在框架层切断本地文件 secrets store 的注册路径。其余宿主（localnet、CLI、demos、tests）保持默认行为，无破坏性变化。
改动概览
Aevatar.Configuration/EnvironmentSecretsStore.cs（新）：只读IAevatarSecretsStore实现，仅从IConfiguration（含AEVATAR_前缀环境变量）读取；Set/Remove直接抛InvalidOperationException，misconfig 在调用点炸出而非静默落盘。AevatarConfigLoader.AddAevatarConfig(IConfigurationBuilder, bool allowLocalFileStore = true)：当false时跳过secrets.json加载（config.json/mcp.json/connectors.json/env 不变）。Aevatar.Configuration.ServiceCollectionExtensions.AddAevatarConfig(IServiceCollection, bool allowLocalFileStore = true)：当false时注册EnvironmentSecretsStore替代AevatarSecretsStore。AevatarDefaultHostOptions.AllowLocalFileSecretsStore（新）：宿主层开关，AddAevatarDefaultHost透传到AddAevatarBootstrap与AddAevatarConfig。AddAevatarBootstrap也增加同名 overload。Aevatar.Mainnet.Host.Api/Hosting/MainnetHostBuilderExtensions.cs：在AddAevatarMainnetHost中显式置为false。src/Aevatar.Configuration/README.md：补充新类型与 store 选择规则。接口
IAevatarSecretsStore未改动；现有架构守卫secret_store_di_hits（禁止宿主直接AddSingleton<IAevatarSecretsStore>）继续生效。影响路径
AEVATAR_LLMProviders__Providers__deepseek__ApiKey等环境变量注入；任何Set/Remove调用立即抛错。true，行为与 dev 完全一致。验证
dotnet build src/Aevatar.Configuration/...、src/Aevatar.Bootstrap/...、src/Aevatar.Mainnet.Host.Api/...— 0 errors。dotnet test test/Aevatar.Foundation.Abstractions.Tests/Aevatar.Foundation.Abstractions.Tests.csproj— 50/50 通过（含 6 个新增EnvironmentSecretsStore用例 + 2 个AddAevatarConfig注册门禁用例）。dotnet test test/Aevatar.Bootstrap.Tests/...— 51 通过（含 2 个新增端到端用例：AddAevatarBootstrap与AddAevatarDefaultHost的 disable 路径）。dotnet test test/Aevatar.Integration.Tests/... --filter SecretsStore|ConfigurationCoverage— 18/18 通过。dotnet test test/Aevatar.GAgents.ChannelRuntime.Tests/...— 465/465 通过。secret_store_di_hits扫描宿主与 agent 扩展：no hits（pre-existing playground asset drift 与MainnetHostCompositionTests已确认在 dev 上同样失败，与本 PR 无关）。Test plan
Aevatar.Mainnet.Host.Api启动时若调用方误用_secretsStore.Set(...)，应抛InvalidOperationException。AEVATAR_*环境变量在 mainnet 中读取LLMProviders:Providers:{name}:ApiKey/LLMProviders:Default等仍然生效。~/.aevatar/secrets.json存在时 mainnet 不再加载（验证IConfiguration中无对应 key）。