feat(secrets-vault): add mTLS support for HashiCorp Vault#142
Closed
rayanebel wants to merge 1 commit into
Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds optional mTLS / custom-CA support to the HashiCorp Vault HTTP client by reading the standard VAULT_CACERT, VAULT_CLIENT_CERT, and VAULT_CLIENT_KEY environment variables when constructing reqwest::Client. Also reconfigures the reqwest dependency to use rustls-tls instead of the default TLS backend.
Changes:
- New
build_http_client()helper inhashicorp_api.rsthat adds a root certificate and/or client identity to the reqwest builder when the corresponding env vars are set. Client::newnow usesbuild_http_client()?instead ofreqwest::Client::new().reqwestis switched todefault-features = falsewith["json", "rustls-tls"].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/secrets-vault/src/storage/hashicorp_api.rs | Adds build_http_client helper and wires it into Client::new. |
| src/secrets-vault/Cargo.toml | Disables reqwest default features and enables rustls-tls. |
Comments suppressed due to low confidence (2)
src/secrets-vault/src/storage/hashicorp_api.rs:847
- If only one of
VAULT_CLIENT_CERTorVAULT_CLIENT_KEYis set, the tuple match silently falls through and no client identity is configured. That is a confusing failure mode for an operator who misconfigures one variable: the connection will then fail at TLS handshake with an opaque "bad certificate / handshake failure" instead of a clear "VAULT_CLIENT_KEY is required when VAULT_CLIENT_CERT is set". Detect the partial-configuration case explicitly and return ananyhow::anyhow!error naming the missing variable.
if let (Ok(cert_path), Ok(key_path)) =
(std::env::var("VAULT_CLIENT_CERT"), std::env::var("VAULT_CLIENT_KEY"))
{
src/secrets-vault/src/storage/hashicorp_api.rs:854
- The cert PEM and key PEM are concatenated with no separator between them. If the certificate file does not end with a trailing newline (which is allowed by PEM and is common when files are produced by
printf, manual editing, or some PKI tooling), the resulting buffer will look like-----END CERTIFICATE-----\n-----BEGIN PRIVATE KEY-----only when the cert file happens to end with\n; if it doesn't, the boundary becomes-----END CERTIFICATE----------BEGIN PRIVATE KEY-----andIdentity::from_pemwill reject the bundle. Insert a\nbetween the two byte buffers (or push one ifcert_pemdoesn't already end in\n) to make the parsing robust.
let mut identity_pem = cert_pem;
identity_pem.extend_from_slice(&key_pem);
let identity = reqwest::Identity::from_pem(&identity_pem)?;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uuid = { version = "1", features = ["v4", "serde"] } | ||
| zeroize = "1.8" | ||
| reqwest = { version = "0.12", features = ["json"] } | ||
| reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] } |
Comment on lines
+841
to
+858
| let cert = reqwest::Certificate::from_pem(&ca_pem)?; | ||
| builder = builder.add_root_certificate(cert); | ||
| } | ||
|
|
||
| if let (Ok(cert_path), Ok(key_path)) = | ||
| (std::env::var("VAULT_CLIENT_CERT"), std::env::var("VAULT_CLIENT_KEY")) | ||
| { | ||
| let cert_pem = std::fs::read(&cert_path) | ||
| .map_err(|e| anyhow::anyhow!("Failed to read VAULT_CLIENT_CERT ({cert_path}): {e}"))?; | ||
| let key_pem = std::fs::read(&key_path) | ||
| .map_err(|e| anyhow::anyhow!("Failed to read VAULT_CLIENT_KEY ({key_path}): {e}"))?; | ||
| let mut identity_pem = cert_pem; | ||
| identity_pem.extend_from_slice(&key_pem); | ||
| let identity = reqwest::Identity::from_pem(&identity_pem)?; | ||
| builder = builder.identity(identity); | ||
| } | ||
|
|
||
| Ok(builder.build()?) |
Comment on lines
+850
to
+854
| let key_pem = std::fs::read(&key_path) | ||
| .map_err(|e| anyhow::anyhow!("Failed to read VAULT_CLIENT_KEY ({key_path}): {e}"))?; | ||
| let mut identity_pem = cert_pem; | ||
| identity_pem.extend_from_slice(&key_pem); | ||
| let identity = reqwest::Identity::from_pem(&identity_pem)?; |
Comment on lines
+832
to
+859
| /// Build an HTTP client with optional mTLS support. | ||
| /// Reads VAULT_CACERT, VAULT_CLIENT_CERT, VAULT_CLIENT_KEY from environment. | ||
| /// If not set, returns a default client. | ||
| fn build_http_client() -> anyhow::Result<reqwest::Client> { | ||
| let mut builder = reqwest::Client::builder(); | ||
|
|
||
| if let Ok(ca_path) = std::env::var("VAULT_CACERT") { | ||
| let ca_pem = std::fs::read(&ca_path) | ||
| .map_err(|e| anyhow::anyhow!("Failed to read VAULT_CACERT ({ca_path}): {e}"))?; | ||
| let cert = reqwest::Certificate::from_pem(&ca_pem)?; | ||
| builder = builder.add_root_certificate(cert); | ||
| } | ||
|
|
||
| if let (Ok(cert_path), Ok(key_path)) = | ||
| (std::env::var("VAULT_CLIENT_CERT"), std::env::var("VAULT_CLIENT_KEY")) | ||
| { | ||
| let cert_pem = std::fs::read(&cert_path) | ||
| .map_err(|e| anyhow::anyhow!("Failed to read VAULT_CLIENT_CERT ({cert_path}): {e}"))?; | ||
| let key_pem = std::fs::read(&key_path) | ||
| .map_err(|e| anyhow::anyhow!("Failed to read VAULT_CLIENT_KEY ({key_path}): {e}"))?; | ||
| let mut identity_pem = cert_pem; | ||
| identity_pem.extend_from_slice(&key_pem); | ||
| let identity = reqwest::Identity::from_pem(&identity_pem)?; | ||
| builder = builder.identity(identity); | ||
| } | ||
|
|
||
| Ok(builder.build()?) | ||
| } |
| } | ||
|
|
||
| Ok(builder.build()?) | ||
| } |
ITBear
approved these changes
May 15, 2026
Contributor
|
New PR with fixes: #143 |
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
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.
The secrets-vault library currently creates a plain
reqwest::Client::new()when connecting to HashiCorp Vault. This fails when Vault is behind an ingress that requires mutual TLS (mTLS) client certificate authentication.This PR adds support for the standard HashiCorp Vault TLS environment variables:
VAULT_CACERT— path to a custom CA certificate (PEM)VAULT_CLIENT_CERT— path to a client certificate (PEM)VAULT_CLIENT_KEY— path to a client private key (PEM)When these variables are set, the HTTP client is built with the corresponding TLS configuration. When they are not set, the client behaves exactly as before (no breaking change).