Add Azure Key Vault provider (akv://) by phoem · Pull Request #132 · cachix/secretspec

phoem · 2026-07-13T23:03:30Z

Adds an akv:// provider backed by Azure Key Vault, gated behind a new akv Cargo feature (included in default features alongside gcsm/awssm/vault/bws). Convention secrets map to Azure Key Vault's [0-9a-zA-Z-] secret-name charset via secretspec--{project}--{profile}--{key}, with underscores rewritten to hyphens. Authentication is chosen via ?auth=env|cli|managed_identity|workload_identity, defaulting to a service principal from AZURE_TENANT_ID/AZURE_CLIENT_ID/AZURE_CLIENT_SECRET and falling back to the signed-in Azure CLI session.

Version pinning and user-assigned managed identity are not yet supported. Updates docs (new providers/akv.md page plus the other tracked locations) and CHANGELOG.md.

Adds an akv:// provider backed by Azure Key Vault, gated behind a new akv Cargo feature (included in default features alongside gcsm/awssm/vault/bws). Convention secrets map to Azure Key Vault's [0-9a-zA-Z-] secret-name charset via secretspec--{project}--{profile}--{key}, with underscores rewritten to hyphens. Authentication is chosen via ?auth=env|cli|managed_identity|workload_identity, defaulting to a service principal from AZURE_TENANT_ID/AZURE_CLIENT_ID/AZURE_CLIENT_SECRET and falling back to the signed-in Azure CLI session. Version pinning and user-assigned managed identity are not yet supported. Updates docs (new providers/akv.md page plus the other tracked locations) and CHANGELOG.md.

domenkozar

Review — Azure Key Vault provider (`akv://`)

Nice, well-documented provider — the whole CLAUDE.md provider-doc checklist is satisfied and the CHANGELOG entry is present. However, there are two blocking issues plus several correctness edges and cleanups. Details are in inline comments; summary below.

Blocking

Does not compile. ClientSecretCredential::new (azure_identity 1.0.0) is new(tenant_id: &str, client_id: String, secret: azure_core::credentials::Secret, ...), but the code passes tenant_id: String and a secrecy::SecretString. cargo check --features akv fails with E0308 (exit 101). Because akv is in the default feature set, cargo build, cargo test --all, and CI all fail. Fix: &tenant_id and azure_core::credentials::Secret::from(client_secret).
is_not_found_error misses real 404s. A Key Vault 404's Display is only the service message ("A secret … was not found in this key vault"); the SecretNotFound code lives in a separate error_code field, so contains("SecretNotFound") || contains("404") is false for a genuine not-found and get returns Err instead of Ok(None). That breaks check, run with optional/default secrets, and provider fallback chains. Use the typed e.http_status() == Some(StatusCode::NotFound).

Correctness (moderate)

Partial/empty AZURE_* env vars silently fall back to a different identity instead of erroring.
Secret-name mapping is non-injective (A__B and A--B collide onto one vault entry).
Native ref items skip the charset validation/rewrite that convention names get.

Design / cleanup

A fresh client + credential is built per get/set (no token caching, no get_many override) → N token fetches / az spawns for N secrets.
Managed identity is system-assigned only, but the reference docs advertise it unqualified.
Sovereign cloud is selected by a contains('.') heuristic — a bare name silently hits the public endpoint.
version coordinate is rejected though Key Vault versions secrets (known limitation).
Leftover dev NOTE comment.
uri() duplicates the auth-method↔string map.

Test / docs (non-blocking, no inline anchor)

secretspec/src/provider/tests.rs (~line 482): the generic integration harness has no akv arm, so SECRETSPEC_TEST_PROVIDERS=akv falls into the _ branch, calls try_from("akv") (bare name has no vault), and panics with a misleading "akv provider should exist". akv gets zero coverage in the shared suite. (This also confirms CI never reaches the Azure network.)

domenkozar · 2026-07-15T14:47:24Z

+                    // crate; verify this constructor's exact parameter types
+                    // with `cargo check --features akv` before relying on it,
+                    // in case the secret parameter type has changed.
+                    Ok(ClientSecretCredential::new(


Blocking — does not compile. azure_identity 1.0.0 is ClientSecretCredential::new(tenant_id: &str, client_id: String, secret: azure_core::credentials::Secret, options). Here tenant_id is a String (needs &str) and the third arg is a secrecy::SecretString (needs azure_core::credentials::Secret). cargo check --features akv → error[E0308] (exit 101), and since akv is in the default feature set this breaks cargo build / cargo test --all / CI.

Ok(ClientSecretCredential::new( &tenant_id, client_id, azure_core::credentials::Secret::from(client_secret), None, )...

Thanks, fixed. I somehow set the PR as ready ahead of it being ready. Apologies, appreciate the great comments though.

domenkozar · 2026-07-15T14:47:24Z

+    /// Checks whether an error indicates the secret was not found.
+    fn is_not_found_error(e: &impl std::error::Error) -> bool {
+        let s = e.to_string();
+        s.contains("SecretNotFound") || s.contains("404")


Blocking correctness — not-found is not detected. A Key Vault 404 is built via Error::with_message(kind, message) where message is the service's human text ("A secret with (name) was not found in this key vault"); the SecretNotFound code sits in a separate error_code field and Error's Display renders only the message. So this contains("SecretNotFound") || contains("404") is false for a genuine 404, and get_secret_async returns Err instead of Ok(None) — breaking check, run with optional/default secrets, and provider fallback for any secret not yet in the vault. The "404" substring also risks the opposite (swallowing unrelated errors that happen to contain 404).

azure_core::Error exposes a typed accessor — prefer:

e.http_status() == Some(azure_core::http::StatusCode::NotFound)

(which needs is_not_found_error to take &azure_core::Error rather than &impl std::error::Error).

Fixed — now checks e.http_status() == Some(StatusCode::NotFound) on the typed error instead of string matching.

domenkozar · 2026-07-15T14:47:24Z

+                let client_id = std::env::var("AZURE_CLIENT_ID").ok();
+                let client_secret = std::env::var("AZURE_CLIENT_SECRET").ok();
+
+                if let (Some(tenant_id), Some(client_id), Some(client_secret)) =


Correctness — partial/empty credentials silently change identity. This requires all three vars; with only two set, the code drops to the CLI/azd session (line ~288) — authenticating as the developer's personal az login identity in dev, or failing with a confusing "failed to fall back to the Azure CLI" in CI, instead of "AZURE_CLIENT_SECRET is missing". Also std::env::var().ok() returns Some("") for a blank AZURE_CLIENT_SECRET=, which counts as "set" and blocks the fallback. Suggest treating empty as unset and erroring on a partially-specified principal.

Fixed — blank vars are now treated as unset, and a partially-specified principal now errors (naming the missing var) instead of silently falling back to the CLI session.

domenkozar · 2026-07-15T14:47:24Z

+        Self::validate_name_component("profile", profile)?;
+        Self::validate_name_component("key", key)?;
+
+        let sanitize = |s: &str| s.replace('_', "-");


Correctness — name mapping is not injective. _→- sanitization plus the -- delimiter means distinct triples collide: format_secret_name("a","b__c","d") and format_secret_name("a","b","c__d") both produce secretspec--a--b--c--d; likewise key A__B collides with A--B. __ is common in real keys (e.g. .NET Section__Key), so a set on one silently versions over the other and get cross-reads. The module doc only warns about the single-underscore case. Every other provider separates with /, which components can't contain, so akv is uniquely exposed — worth at least documenting the __ case, ideally rejecting it.

Went further than documenting it — now rejects any project/profile/key component containing -- or __ before it can collide with the delimiter.

domenkozar · 2026-07-15T14:47:24Z

+
+    fn get(&self, addr: Address<'_>) -> Result<Option<SecretString>> {
+        let coords = self.resolve_coords(addr)?;
+        super::block_on(self.get_secret_async(&coords.item))


Correctness (minor) — native ref items skip validation. resolve_coords returns a Native address's item verbatim, so ref = { item = "my_secret" } reaches Azure with an underscore (no charset check, no _→- rewrite) and fails with a raw opaque API error, whereas the convention path validates and rewrites up front. Consider validating native items too (or documenting that refs must already be Azure-legal).

Fixed — native ref items are now validated against Azure's charset up front (clear error instead of an opaque API failure), but intentionally not rewritten, since silently changing a user-specified ref felt riskier than a clear error.

domenkozar · 2026-07-15T14:47:25Z

+
+        // A host containing a dot is already a full DNS name (sovereign
+        // clouds); a bare name gets the public-cloud suffix appended.
+        let vault_url = if vault_host.contains('.') {


Altitude — sovereign cloud picked by a magic heuristic. vault_host.contains('.') means a bare vault name in China/USGov/Germany silently gets the public .vault.azure.net suffix (wrong endpoint, failing auth/DNS with no hint), and a trailing-dot typo akv://myvault. becomes https://myvault./. An explicit ?cloud=/?suffix= param (mirroring ?auth=) would be a first-class, testable knob rather than an inferred special case.

Added ?suffix= as an explicit override; kept the dotted-hostname form working since it's already documented/tested behavior rather than removing it outright

domenkozar · 2026-07-15T14:47:25Z

+    /// Retrieves a secret's current value by name, mapping "not found" to `None`.
+    async fn get_secret_async(&self, name: &str) -> Result<Option<SecretString>> {
+        let client = self.create_client()?;
+        match client.get_secret(name, None).await {


Feature note — no version pinning. get_secret(name, None) hardcodes the latest version and supported_coords stays empty, so ref = { version = "…" } is rejected even though Key Vault genuinely versions secrets. Documented as a known limitation and safe-by-default (rejects rather than silently ignores), just flagging for the roadmap.

Agreed, tracked as a follow-up — will need a ref.version → Key Vault version-suffix mapping.

domenkozar · 2026-07-15T14:47:25Z

+                if let (Some(tenant_id), Some(client_id), Some(client_secret)) =
+                    (tenant_id, client_id, client_secret)
+                {
+                    // NOTE: `azure_identity` is a very new (v1.0), fast-moving


Cleanup — leftover dev note. This NOTE: block asks the reader to verify the constructor with cargo check "in case the secret parameter type has changed" — authoring uncertainty rather than documentation, and it happens to guard the exact call that fails to compile. Please drop it (keep the legitimate "fall back to CLI" comment below).

Removed the comment.

domenkozar · 2026-07-15T14:47:25Z

+    fn uri(&self) -> String {
+        let mut uri = format!("akv://{}", self.config.vault_host);
+        if self.config.auth != AuthMethod::default() {
+            let auth = match self.config.auth {


Cleanup — duplicated auth mapping. This AuthMethod→&str match is the exact inverse of the &str→AuthMethod match in TryFrom (lines ~124-134). Two matches that must stay in lockstep with no shared source of truth — add/rename an auth method and miss one, and uri() emits a string TryFrom can no longer parse. Consider a single AuthMethod::as_str() / parse pair.

consolidated into AuthMethod::as_str()/FromStr so the two directions can't drift

domenkozar · 2026-07-15T14:47:25Z

+Enter value for DATABASE_URL: postgresql://localhost/mydb
+✓ Secret 'DATABASE_URL' saved to akv (profile: default)
+
+# Import from .env


Docs — misleading example. secretspec import dotenv://.env imports from a local .env into the default provider; it has nothing to do with Azure Key Vault, so it reads oddly on the akv page (a reader might expect it to load secrets into the vault). The command is valid, just out of place here.

swapped for a relevant get example

…g brace - azure_identity::ClientSecretCredential::new expects tenant_id: &str and an azure_core::credentials::Secret, not the secrecy::SecretString used elsewhere in this provider; pass &tenant_id and construct a Secret. - provider/mod.rs was missing the closing brace for the url_tests module.

Added updated Cargo.lock file

phoem · 2026-07-15T17:07:50Z

My apologies I did not mean for this to be active yet. I am making fixes/changes and I will let you know when they're all resolved. Thanks also for the great and detailed comments.

- is_not_found_error now checks the typed HTTP status (e.http_status() == Some(StatusCode::NotFound)) instead of string-matching the error message, which missed genuine 404s. - AZURE_TENANT_ID/AZURE_CLIENT_ID/AZURE_CLIENT_SECRET: blank values are now treated as unset, and a partially-specified principal errors (naming the missing var) instead of silently falling back to the Azure CLI session. - Project/profile/key components that sanitize to a '--' (a literal '--' or a '__') are rejected, since they'd be indistinguishable from the project/profile/key delimiter and could collide with another secret. - Native ref items are now validated against Azure's secret-name charset (but never rewritten) before any network I/O. - Added an explicit ?suffix= query param for sovereign clouds, alongside the existing dotted-hostname form; AkvConfig tracks it so uri() round-trips. - AuthMethod::as_str()/FromStr replace the duplicated auth-method mappings in TryFrom and uri(). - provider/tests.rs: added an akv arm to the generic integration harness (gated on AKV_TEST_VAULT), instead of panicking via the generic fallback. - Docs: fixed the misleading dotenv-import example on the akv provider page, and the managed identity wording in reference/providers.md to say system-assigned, matching the provider page.

domenkozar reviewed Jul 15, 2026

View reviewed changes

phoem added 2 commits July 15, 2026 12:10

Missing Cargo.lock file in last commit

ea900e1

Added updated Cargo.lock file

domenkozar marked this pull request as draft July 15, 2026 20:02

phoem added 2 commits July 15, 2026 17:08

Merge branch 'main' into feat/azure-key-vault

dc6d75b

phoem marked this pull request as ready for review July 15, 2026 21:22

domenkozar merged commit 916f90e into cachix:main Jul 15, 2026
9 of 12 checks passed

Uh oh!

Conversation

phoem commented Jul 13, 2026

Uh oh!

domenkozar left a comment

Choose a reason for hiding this comment

Review — Azure Key Vault provider (akv://)

Blocking

Correctness (moderate)

Design / cleanup

Test / docs (non-blocking, no inline anchor)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoem commented Jul 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Review — Azure Key Vault provider (`akv://`)