coordinator: domain-separate transit engine keys from workload secrets#2500
coordinator: domain-separate transit engine keys from workload secrets#2500sespiros wants to merge 1 commit into
Conversation
The transit engine derived its AES keys with DeriveWorkloadSecret("<version>_<name>"),
the same HKDF namespace that hands each pod its workload secret. A pod deployed with
WorkloadSecretID "0_vault_unsealing" thus received, in its own workload secret, the
exact key the transit engine uses to wrap OpenBao's master key. That defeats the
transit API's per-workload authorization on both directions: the pod can unwrap the
auto-unseal blob offline, and (since it holds the key) forge a blob the Coordinator
will decrypt, injecting an attacker-chosen master key on OpenBao's untrusted storage.
Derive transit keys from a dedicated seed and HKDF label so their keyspace can never
alias a workload secret. No decrypt-side fallback to the old derivation is provided on
purpose: honoring the legacy key would keep the forgery path open, since the server
cannot tell an original legacy blob from an attacker-forged one. This is a breaking
change for existing OpenBao auto-unseal deployments, which must migrate their master
key (seal-migration or re-init) onto the new key on upgrade.
Signed-off-by: Spyros Seimenis <sse@edgeless.systems>
burgerdev
left a comment
burgerdev
left a comment
There was a problem hiding this comment.
Thanks for fixing this, lgtm!
As migration path, we could recommend starting a pod with the problematic 0_$ID to obtain the old sealing secret, then reseal using the transit secret engine. I don't think we need to keep a backwards-compatible implementation around.
| }, logger) | ||
| return | ||
| } | ||
| key, err := deriveEncryptionKey(r.Context(), guard, fmt.Sprintf("%d_%s", encReq.KeyVersion, workloadSecretID)) |
There was a problem hiding this comment.
Now that I think of it, I remember that we picked %d_%s because generated workloadSecretIDs don't contain an underscore: #1199 (comment). We could enforce that in manifest.Validate, but that would be even more breaking.
There was a problem hiding this comment.
Right! I didn't like that _ was essentially that load-bearing. I initially solved this by changing the HKDF info label but then I thought that a dedicated seed would split the two keyspaces even more cleanly.
| decryptReqBody, err := json.Marshal(map[string]string{"ciphertext": string(bytes.Trim(ciphertext, `"`))}) | ||
| require.NoError(err) | ||
|
|
||
| req := httptest.NewRequestWithContext(t.Context(), http.MethodPut, "/v1/transit/decrypt/"+name, bytes.NewReader(decryptReqBody)) | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| rec := httptest.NewRecorder() | ||
| mux.ServeHTTP(rec, req) | ||
| res := rec.Result() | ||
| t.Cleanup(func() { _ = res.Body.Close() }) |
There was a problem hiding this comment.
Why did you choose HTTP instead of calling the function directly?
There was a problem hiding this comment.
Which function should I be calling? The rest of the test suite exercises the API this way exercising the full end-to-end path. I initially wrote the test and it proved meaningful because in some initial version I experimented with adding various types of fallbacks which obviously all failed this test (as the comment describes).
sespiros
added
breaking change
2 participants
The transit engine derived its AES keys with DeriveWorkloadSecret("_"), the same HKDF namespace that hands each pod its workload secret. A pod deployed with WorkloadSecretID "0_vault_unsealing" thus received, in its own workload secret, the exact key the transit engine uses to wrap OpenBao's master key. That defeats the transit API's per-workload authorization on both directions: the pod can unwrap the auto-unseal blob offline, and (since it holds the key) forge a blob the Coordinator will decrypt, injecting an attacker-chosen master key on OpenBao's untrusted storage.
Derive transit keys from a dedicated seed and HKDF label so their keyspace can never alias a workload secret. No decrypt-side fallback to the old derivation is provided on purpose: honoring the legacy key would keep the forgery path open, since the server cannot tell an original legacy blob from an attacker-forged one. This is a breaking change for existing OpenBao auto-unseal deployments, which must migrate their master key (seal-migration or re-init) onto the new key on upgrade.
Fixes CON-201
TODO
Decide on rollout/backwards compatibility as this is a breaking change!This is now tagged as a breaking change for the release notes.