multi: add HashiCorp Vault KV v2 secret support - #92
Conversation
calvinrzachman
left a comment
calvinrzachman
left a comment
There was a problem hiding this comment.
Vault support 🔒 I tested this in a cluster against our lnd chart and the happy path works end to end. The seed and password make it to Vault, wallet unlocks, and the node comes online, with no Kubernetes wallet secret created.
A couple of small hardening items where the Vault path deviates from the k8s backend predecessor. Neither fires in our current deployment (single replica, write-once text secrets) and both are small fixes, so they're more for k8s backend parity rather than anything blocking.
|
|
||
| logger.Infof("Attempting to write entry %s of secret %s in vault", | ||
| opts.SecretKeyName, opts.SecretPath) | ||
| if _, err := kv.Put(ctx, opts.SecretPath, data); err != nil { |
There was a problem hiding this comment.
k8s fails loud on a concurrent write, since Update carries a resourceVersion. Vault's Put here is unconditional, so two processes writing different entries of the same secret can overwrite one another and lose an entry with no error. Ran a test with two concurrent stores against the same path, and the Vault path silently dropped a key, while the same race on --target=k8s failed noticeably. A check-and-set on the version gives Vault the same loud-on-conflict behavior:
if _, err := kv.Put(ctx, opts.SecretPath, data,
api.WithCheckAndSet(version)); err != nil {
return err
}This needs two initializers writing the same path at once, which a single-replica deployment avoids, so it is not a live bug given the init script we're using. But lndinit is a general tool, and a custom init script that calls store-secret concurrently would hit it, so matching the k8s backend's behavior seems worth having.
There was a problem hiding this comment.
Addressed in the latest push, the Put now uses check-and-set so a raced write fails loudly instead of clobbering.
| opts.SecretKeyName, opts.SecretPath, errTargetExists) | ||
| } | ||
|
|
||
| data[opts.SecretKeyName] = content |
There was a problem hiding this comment.
KV v2 stores values as JSON strings, so a non-UTF-8 value has its invalid bytes replaced with U+FFFD and cannot be recovered. As an example an admin.macaroon stored via --batch --target=vault comes back corrupted. The k8s path has --k8s.base64 for this, so we may want to match that with a --vault.base64 option (encode on write, decode on read) here. A tls.cert/key round-trips clean, so it is a binary vs text thing, not macaroon-specific.
This is not hit today, since the chart keeps only text seed and password in Vault, but it would be a prerequisite if we later move the RPC secrets into Vault too (e.g. VSO-syncing them out to consumer namespaces).
There was a problem hiding this comment.
Addressed in the latest push, added a --base64 option so binary values round-trip instead of being corrupted.
In this commit, we add `vault` as a third secret source and target alongside the existing `file` and `k8s` backends, wired into the `init-wallet`, `store-secret` and `load-secret` commands. This lets an lnd deployment keep its wallet seed and password in HashiCorp Vault directly, so the secrets never need to be materialized as a Kubernetes Secret in etcd. The implementation targets the KV v2 secrets engine using the official `vault/api` KV v2 helper: reads and writes go through `client.KVv2(mount)`, which handles the nested `data/` path shaping that KV v2 requires. Writes are read-modify-write merges so that storing one entry (e.g. the seed) never clobbers a sibling entry (e.g. the password) in the same secret. The `errTargetExists` sentinel is preserved on the write path so the existing idempotency contract (`--error-on-existing`) keeps working across restarts, which is important to make sure we never silently rotate a seed out from under an initialized wallet. Authentication uses the Kubernetes auth method via the official `vault/api/auth/kubernetes` login helper. The pod's projected ServiceAccount token is exchanged for a Vault token bound to a role and policy. The auth mount, role and token path are all configurable; the Vault server address is taken from the `VAULT_ADDR` environment variable or the `--vault.addr` flag. A companion `example-init-wallet-vault.sh` and a README section round out the change.
In this commit, we add the design doc that motivates the KV v2 Vault variant: how our production Vault in lightning-infra is actually set up (KV v2, Kubernetes auth, VSO), the gap versus the earlier KV v1 approach, the locked design decisions, and the companion infra wiring needed to roll it out.
readVault trims trailing newlines from an entry and returns the trimmed value, but the check that rejects empty entries ran before the trim. It inspected the untrimmed entry while the caller received the trimmed one, so the check and the returned value operated on different strings. Move the trim ahead of the empty check so both operate on the same string. An entry consisting only of trailing newlines then fails the empty check instead of being returned as an empty value.
saveVault read the secret, merged in a single entry, and wrote the whole object back with an unconditional Put. Two processes writing different entries of the same secret at the same time could each read the same version and then overwrite one another, so one entry was lost with no error returned. The Kubernetes backend does not have this problem because its update carries a resource version and the API server rejects a stale write, so a concurrent writer fails loudly instead of silently clobbering. Read the current version alongside the data and pass it as a check-and-set on the Put, so a write that raced with another writer is rejected rather than silently overwriting it. A version of zero makes the initial write create-only. This matches the Kubernetes backend's conflict detection and needs only create and update capabilities, so it works under a least-privilege policy that does not grant patch. The test Vault now tracks versions and enforces check-and-set so the existing round-trip tests exercise the guarded path, and a new test covers the stale-write rejection.
KV v2 stores entries as JSON strings, so a value that is not valid UTF-8, such as a raw macaroon, has its invalid bytes replaced with the Unicode replacement character on write and cannot be recovered. Text values like a seed, a wallet password, or a PEM certificate are unaffected, but binary values are silently corrupted. The Kubernetes backend already avoids this with its base64 option; the Vault path had no equivalent. Add a base64 option to the Vault secret flags that encodes the value on write and decodes it on read, wired into the store-secret, load-secret and init-wallet commands so a caller can store binary values safely. It is off by default, so text values are still stored verbatim.
53c41c2 to
a878c21
Compare
|
@Roasbeef, remember to re-request review from reviewers when ready |
3 participants
In this PR, we add
vaultas a third secret source and target alongside the existingfileandk8sbackends, wired into theinit-wallet,store-secretandload-secretcommands. This lets an lnd deployment keep its wallet seed and password in HashiCorp Vault directly, so the secrets never need to be materialized as a Kubernetes Secret in etcd.This supersedes #62, which was a straight port of an older
lnd-fork branch written against a KV v1 Vault with a hand-rolled Kubernetes login. Our production Vault targets the KV v2 secrets engine, so that version wouldn't work as-is: a KV v2 read comes back with the values nested underdata, which the KV v1Logical().Readpath doesn't unwrap.What's here
client.KVv2(mount), which handles the nesteddata/path shaping KV v2 requires. Writes are read-modify-write merges, so storing one entry (e.g. the seed) never clobbers a sibling entry (e.g. the password) in the same secret. UsingPuton a merged map (rather thanPatch) keeps the required policy to justcreate/read/update.errTargetExistssentinel is kept on the write path, so--error-on-existingkeeps working across restarts. This matters: we never want to silently rotate a seed out from under an already-initialized wallet.vault/api/auth/kuberneteslogin helper. The pod's projected ServiceAccount token is exchanged for a Vault token bound to a role and policy. The auth mount, role and token path are configurable; the server address comes fromVAULT_ADDRor--vault.addr.example-init-wallet-vault.shstartup script, and avault_test.gosuite (a mock KV v2 + k8s-auth server covering round-trip, no-clobber merge, the overwrite guard, newline trimming, and the not-found/missing-key/empty-value error paths).The
docs/vault-integration-design.mdfile captures how our production Vault inlightning-infrais set up (KV v2, Kubernetes auth, VSO), the gap versus the KV v1 approach, and the companion infra wiring (an lnd policy scoped tosecret/data/lnd/*, a k8s auth role, and the lnd chart init-script) that will follow in a separate lightning-infra PR.