Skip to content

Commit bfbb5ac

Browse files
yankawailexfrei
yankawai
authored and
lexfrei
committed
fix(openbao): constrain the seal values that reach the server config
The upstream chart passes the server config through tpl, and its start script replaces upper case placeholders such as HOSTNAME anywhere in the copied file. A key identifier with template delimiters would run as a template inside helm-controller, which runs as cluster-admin, and an upper case one could come out different on every pod. Both key identifiers now take lowercase alphanumerics, hyphens, dots and underscores. The Secret names become pod volume names of the form userconfig-<name>, which Kubernetes validates as DNS labels. Both name fields now take a DNS label of at most 52 characters, and the volume entries are quoted. The apps API only defaults against the generated schema, so Helm enforces these patterns when it installs or upgrades the release. A previous Secret equal to the current one mounts the same volume twice, which the API server rejects for the StatefulSet, and OpenBAO refuses to start when equal key identifiers name different key material. Both cases now fail at render. The deepcopy functions for the Seal type are regenerated as well. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]>
1 parent 0c52ded commit bfbb5ac

8 files changed

Lines changed: 218 additions & 44 deletions

File tree

‎api/apps/v1alpha1/openbao/types.go‎

Lines changed: 13 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/apps/v1alpha1/openbao/zz_generated.deepcopy.go‎

Lines changed: 16 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/apps/openbao/README.md‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -77,13 +77,13 @@ Keep a copy of the key outside the cluster: a Raft snapshot or a data PVC is unr
7777

7878
### Seal
7979

80-
| Name | Description | Type | Value |
81-
| ------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | -------- |
82-
| `seal` | Seal configuration. Defaults to Shamir key shares; set `type: static` for auto-unseal. | `object` | `{}` |
83-
| `seal.type` | Seal type. `shamir` keeps the current behaviour; `static` enables auto-unseal from `secretName`. | `string` | `shamir` |
84-
| `seal.secretName` | Existing Secret in the release namespace whose key `key` holds the base64 text of 32 random bytes (for example `openssl rand -base64 32`). Required when `type` is `static`. | `string` | `""` |
85-
| `seal.keyId` | Permanent identifier of the current key. Change it whenever the key material changes; OpenBAO refuses a key whose identifier it has already seen with different material. Required when `type` is `static`. | `string` | `""` |
86-
| `seal.previousSecretName` | Secret holding the previous key during an n-1 key rotation. Keep it until the pods have been replaced and the active node has logged `upgrading stored keys`; see the README. | `string` | `""` |
87-
| `seal.previousKeyId` | Identifier of the previous key. Required when `previousSecretName` is set. | `string` | `""` |
88-
| `seal.allowMigration` | Acknowledge a seal migration. The chart refuses to change `type` on an existing instance unless this is `true`; set it only while running `bao operator unseal -migrate`, then remove it. | `bool` | `false` |
80+
| Name | Description | Type | Value |
81+
| ------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | -------- |
82+
| `seal` | Seal configuration. The default is Shamir key shares. Set `type: static` for auto-unseal. | `object` | `{}` |
83+
| `seal.type` | Seal type. `shamir` keeps the current behaviour, `static` enables auto-unseal from `secretName`. | `string` | `shamir` |
84+
| `seal.secretName` | Existing Secret in the tenant namespace whose key `key` holds the base64 text of 32 random bytes (for example `openssl rand -base64 32`). A cluster administrator creates it, because tenant roles cannot create Secrets. Lowercase alphanumerics and hyphens, at most 52 characters, because the pod volume is named `userconfig-<name>` and a volume name is a DNS label. Required when `type` is `static`. | `string` | `""` |
85+
| `seal.keyId` | Permanent identifier of the current key. OpenBAO stores it with the data the key wraps and picks the key by it, so change it whenever the key material changes. Lowercase alphanumerics, `-`, `.` and `_`, starting and ending with an alphanumeric, because the upstream chart evaluates the server config as a template and rewrites upper case placeholders such as `HOSTNAME` in it. Required when `type` is `static`. | `string` | `""` |
86+
| `seal.previousSecretName` | Secret holding the previous key during an n-1 key rotation, with the same name constraints as `secretName`. Keep it until the pods have been replaced and the active node has logged `upgrading stored keys`, see the README. | `string` | `""` |
87+
| `seal.previousKeyId` | Identifier of the previous key, with the same character constraints as `keyId`. Required when `previousSecretName` is set. | `string` | `""` |
88+
| `seal.allowMigration` | Acknowledge a seal migration. The chart refuses to change `type` on an existing instance unless this is `true`. Set it only while running `bao operator unseal -migrate`, then remove it. | `bool` | `false` |
8989

‎packages/apps/openbao/templates/openbao.yaml‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,18 @@
1515
{{- if and $seal.previousSecretName (not $static) }}
1616
{{- fail "seal.previousSecretName only applies to seal.type \"static\"" }}
1717
{{- end }}
18+
{{- if and $seal.previousSecretName (eq $seal.previousSecretName $seal.secretName) }}
19+
{{- fail "seal.previousSecretName must name a different Secret than seal.secretName" }}
20+
{{- end }}
21+
{{- if and $seal.previousKeyId (eq $seal.previousKeyId $seal.keyId) }}
22+
{{- fail "seal.previousKeyId must differ from seal.keyId" }}
23+
{{- end }}
1824
{{- /*
1925
Refuse a silent seal change on an existing instance. Changing the seal of an
20-
initialized OpenBAO is a seal migration (`bao operator unseal -migrate`); a
21-
plain config change instead leaves a barrier nobody can decrypt. The seal in
22-
use is visible in the config ConfigMap the previous release rendered. `lookup`
23-
returns nothing on first install and on dry-run, so the check is a no-op there.
26+
initialized OpenBAO is a seal migration (`bao operator unseal -migrate`), and a
27+
plain config change leaves an instance that cannot unseal. The seal in use is
28+
visible in the config ConfigMap the previous release rendered. `lookup` returns
29+
nothing on first install and on dry-run, so the check is a no-op there.
2430
*/}}
2531
{{- $cm := lookup "v1" "ConfigMap" .Release.Namespace (printf "%s-config" .Release.Name) }}
2632
{{- if $cm }}
@@ -36,7 +42,7 @@ direction rather than emit a config the migration cannot run against.
3642
{{- fail "migration from seal.type \"static\" to \"shamir\" is not supported by this chart" }}
3743
{{- end }}
3844
{{- if and (ne $liveSeal $seal.type) (not $seal.allowMigration) }}
39-
{{- fail (printf "seal.type %q differs from the seal in use (%q). Changing the seal of an existing instance is a seal migration (bao operator unseal -migrate); set seal.allowMigration: true only while performing it." $seal.type $liveSeal) }}
45+
{{- fail (printf "seal.type %q differs from the seal in use (%q). Changing the seal of an existing instance is a seal migration (bao operator unseal -migrate). Set seal.allowMigration: true only while performing it." $seal.type $liveSeal) }}
4046
{{- end }}
4147
{{- end }}
4248
apiVersion: helm.toolkit.fluxcd.io/v2
@@ -93,10 +99,10 @@ spec:
9399
# The upstream chart mounts each Secret at /openbao/userconfig/<name>/.
94100
extraVolumes:
95101
- type: secret
96-
name: {{ $seal.secretName }}
102+
name: {{ $seal.secretName | quote }}
97103
{{- with $seal.previousSecretName }}
98104
- type: secret
99-
name: {{ . }}
105+
name: {{ . | quote }}
100106
{{- end }}
101107
{{- end }}
102108
{{- if gt (int .Values.replicas) 1 }}

‎packages/apps/openbao/tests/seal_test.yaml‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,30 @@ tests:
154154
- failedTemplate:
155155
errorMessage: "seal.previousSecretName is required when seal.previousKeyId is set"
156156

157+
- it: "rejects a previous Secret that is the current one, because both would mount as the same volume"
158+
set:
159+
seal:
160+
type: static
161+
secretName: openbao-unseal
162+
keyId: key-2
163+
previousSecretName: openbao-unseal
164+
previousKeyId: key-1
165+
asserts:
166+
- failedTemplate:
167+
errorMessage: "seal.previousSecretName must name a different Secret than seal.secretName"
168+
169+
- it: "rejects a previous key identifier that is the current one"
170+
set:
171+
seal:
172+
type: static
173+
secretName: openbao-unseal-2
174+
keyId: key-1
175+
previousSecretName: openbao-unseal-1
176+
previousKeyId: key-1
177+
asserts:
178+
- failedTemplate:
179+
errorMessage: "seal.previousKeyId must differ from seal.keyId"
180+
157181
- it: "rejects previousSecretName with seal.type shamir"
158182
set:
159183
seal:
@@ -324,3 +348,113 @@ tests:
324348
- matchRegex:
325349
path: spec.values.openbao.server.ha.raft.config
326350
pattern: 'seal "static"'
351+
352+
- it: "renders hyphenated Secret names as plain strings in the volumes and in the key paths"
353+
set:
354+
seal:
355+
type: static
356+
secretName: openbao-unseal-2
357+
keyId: key-2
358+
previousSecretName: openbao-unseal-1
359+
previousKeyId: key-1
360+
documentSelector:
361+
path: kind
362+
value: HelmRelease
363+
asserts:
364+
- equal:
365+
path: spec.values.openbao.server.extraVolumes
366+
value:
367+
- type: secret
368+
name: openbao-unseal-2
369+
- type: secret
370+
name: openbao-unseal-1
371+
- matchRegex:
372+
path: spec.values.openbao.server.standalone.config
373+
pattern: 'current_key = "file:///openbao/userconfig/openbao-unseal-2/key"\n\s+previous_key_id = "key-1"\n\s+previous_key = "file:///openbao/userconfig/openbao-unseal-1/key"'
374+
375+
- it: "rejects a Secret name with a dot, because the pod volume userconfig-<name> must be a DNS label"
376+
set:
377+
seal:
378+
type: static
379+
secretName: openbao.unseal
380+
keyId: key-1
381+
asserts:
382+
- failedTemplate:
383+
errorPattern: 'seal[./]secretName.*(?i:does not match pattern)'
384+
385+
- it: "rejects a Secret name longer than 52 characters, because userconfig-<name> must fit 63"
386+
set:
387+
seal:
388+
type: static
389+
secretName: a123456789b123456789c123456789d123456789e123456789fgh
390+
keyId: key-1
391+
asserts:
392+
- failedTemplate:
393+
errorPattern: 'seal[./]secretName.*(?i:length)'
394+
395+
- it: "rejects a previous Secret name with a quote"
396+
set:
397+
seal:
398+
type: static
399+
secretName: openbao-unseal
400+
keyId: key-1
401+
previousSecretName: 'openbao"unseal'
402+
previousKeyId: key-0
403+
asserts:
404+
- failedTemplate:
405+
errorPattern: 'seal[./]previousSecretName.*(?i:does not match pattern)'
406+
407+
- it: "accepts a 52 character Secret name and key identifiers in the form OpenBAO documents"
408+
set:
409+
seal:
410+
type: static
411+
secretName: a123456789b123456789c123456789d123456789e123456789fg
412+
keyId: 20250606-1
413+
previousSecretName: openbao-unseal-1
414+
previousKeyId: key_2025.03
415+
documentSelector:
416+
path: kind
417+
value: HelmRelease
418+
asserts:
419+
- equal:
420+
path: spec.values.openbao.server.extraVolumes[0].name
421+
value: a123456789b123456789c123456789d123456789e123456789fg
422+
- matchRegex:
423+
path: spec.values.openbao.server.standalone.config
424+
pattern: 'current_key_id = "20250606-1"\n\s+current_key = "file:///openbao/userconfig/a123456789b123456789c123456789d123456789e123456789fg/key"\n\s+previous_key_id = "key_2025\.03"'
425+
426+
# The upstream chart renders the server config through tpl, so braces in a
427+
# key identifier would run as a template under the helm-controller identity.
428+
# Its start script also replaces HOSTNAME, POD_IP and similar upper case
429+
# placeholders anywhere in the config before the server reads it.
430+
- it: "rejects a key identifier that the upstream chart would evaluate as a template"
431+
set:
432+
seal:
433+
type: static
434+
secretName: openbao-unseal
435+
keyId: '{{ .Release.Name }}'
436+
asserts:
437+
- failedTemplate:
438+
errorPattern: 'seal[./]keyId.*(?i:does not match pattern)'
439+
440+
- it: "rejects an upper case key identifier that the start script would rewrite"
441+
set:
442+
seal:
443+
type: static
444+
secretName: openbao-unseal
445+
keyId: HOSTNAME
446+
asserts:
447+
- failedTemplate:
448+
errorPattern: 'seal[./]keyId.*(?i:does not match pattern)'
449+
450+
- it: "rejects a previous key identifier that the upstream chart would evaluate as a template"
451+
set:
452+
seal:
453+
type: static
454+
secretName: openbao-unseal-2
455+
keyId: key-2
456+
previousSecretName: openbao-unseal-1
457+
previousKeyId: '{{ .Values }}'
458+
asserts:
459+
- failedTemplate:
460+
errorPattern: 'seal[./]previousKeyId.*(?i:does not match pattern)'

0 commit comments

Comments
 (0)