Skip to content

Commit cb4d05f

Browse files
bsquizzbsquizz
committed
fix(helm): address PR review feedback for BackendTLSPolicy
- Read backend CA from the authoritative server Secret instead of the in-memory PKI bundle so enabling BackendTLSPolicy on an existing release uses the CA that actually signed the server certificate. - Reconcile the backend CA ConfigMap on every hook run (compare and update) instead of skipping when it already exists, so CA rotations propagate automatically. - Remove hook annotations from cert-manager Issuer/Certificate resources so they remain regular release objects managed by Helm lifecycle. Split the cert-manager backend CA ConfigMap creation into a separate post-install/post-upgrade hook Job that polls after cert-manager Certificate resources are applied. - Update architecture/gateway.md, docs/reference/gateway-config.mdx, debug-openshell-cluster skill, and helm-dev-environment skill with BackendTLSPolicy, backend CA ConfigMap, enableMtls, and timeout documentation. Signed-off-by: Brandon Squizzato <[email protected]> Signed-off-by: Brandon Squizzato <[email protected]>
1 parent 7d8aaa9 commit cb4d05f

7 files changed

Lines changed: 203 additions & 64 deletions

File tree

‎.agents/skills/helm-dev-environment/SKILL.md‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,35 @@ Envoy Gateway is already installed by Skaffold (the `envoy-gateway` Helm release
225225
service for the proxy; klipper-lb binds it to hostPort 80, reachable via the
226226
`8080:80` load balancer port mapping.
227227

228+
### BackendTLSPolicy (end-to-end TLS)
229+
230+
To enable end-to-end TLS between the Gateway proxy and the gateway pod, add
231+
BackendTLSPolicy values to the Helm install:
232+
233+
```bash
234+
helm upgrade --install openshell deploy/helm/openshell \
235+
--set grpcRoute.enabled=true \
236+
--set grpcRoute.backendTLSPolicy.enabled=true \
237+
--set server.tls.enableMtls=false \
238+
...
239+
```
240+
241+
This requires `server.tls.enableMtls=false` because ingress proxies cannot
242+
present client certificates to the backend. The certgen hook creates a backend
243+
CA ConfigMap from the server Secret's `ca.crt` key. With cert-manager, a
244+
separate post-install Job polls for the cert-manager-issued certificate (up to
245+
`pkiInitJob.timeoutSeconds`); with built-in PKI the ConfigMap is created in the
246+
same pre-install hook. The ConfigMap is reconciled on every upgrade so CA
247+
rotations propagate automatically.
248+
249+
Key Helm values:
250+
- `grpcRoute.backendTLSPolicy.enabled`: create the BackendTLSPolicy resource
251+
- `grpcRoute.backendTLSPolicy.caCertificateConfigMapName`: override ConfigMap name
252+
- `grpcRoute.backendTLSPolicy.hostname`: override backend validation hostname
253+
- `server.tls.enableMtls`: must be `false` for BackendTLSPolicy
254+
- `pkiInitJob.timeoutSeconds`: polling duration for cert-manager mode
255+
- `pkiInitJob.failOnTimeout`: fail install if cert-manager times out
256+
228257
### Keycloak OIDC
229258

230259
One-time setup — only needed once per cluster lifetime:

‎architecture/gateway.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,6 +675,25 @@ requested present -> generate and write. This guards continuity across restarts
675675
and upgrades while still recovering cleanly if an operator deletes everything
676676
and starts over.
677677

678+
When `grpcRoute.backendTLSPolicy.enabled=true`, the certgen hook also creates a
679+
`ConfigMap` containing the CA certificate (`ca.crt`) used by the Gateway proxy
680+
to validate the backend pod's TLS certificate. The CA is always read from the
681+
authoritative server Secret (not the in-memory bundle) so that enabling
682+
BackendTLSPolicy on an existing release uses the CA that actually signed the
683+
server certificate. The ConfigMap is reconciled on every hook run: if the CA
684+
changes (rotation, re-issue), the ConfigMap is updated in place. In built-in PKI
685+
mode the ConfigMap is created in the same pre-install hook. In cert-manager
686+
mode, a separate post-install/post-upgrade hook Job polls for the cert-manager-
687+
issued server Secret and then creates or updates the ConfigMap, because
688+
cert-manager Certificate resources are regular release objects applied after
689+
pre-install hooks.
690+
691+
The `server.tls.enableMtls` value controls whether the gateway requires client
692+
certificates. When `enableMtls` is `false`, the gateway runs HTTPS-only without
693+
client certificate verification (use OIDC for identity instead). BackendTLSPolicy
694+
requires `enableMtls=false` because the ingress proxy cannot present client
695+
certificates to the backend.
696+
678697
Operators who manage TLS PKI with cert-manager enable `certManager.enabled`;
679698
cert-manager takes precedence over built-in TLS generation and the chart still
680699
renders the JWT-only hook. Operators who pre-create all TLS and JWT Secrets can

‎crates/openshell-server/src/certgen.rs‎

Lines changed: 71 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,9 @@ pub struct CertgenArgs {
8080
dry_run: bool,
8181

8282
/// Name of a `ConfigMap` to create containing the CA certificate (key: ca.crt)
83-
/// for `BackendTLSPolicy` backend validation. In full PKI mode, the CA comes
84-
/// from the generated bundle. In --jwt-only mode, the CA is read from
85-
/// --backend-ca-source-secret.
83+
/// for `BackendTLSPolicy` backend validation. The CA is always read from the
84+
/// authoritative server Secret: --server-secret-name in full PKI mode,
85+
/// --backend-ca-source-secret in --jwt-only mode.
8686
#[arg(long, value_name = "NAME")]
8787
backend_ca_configmap_name: Option<String>,
8888

@@ -127,7 +127,7 @@ pub async fn run(args: CertgenArgs) -> Result<()> {
127127
run_kubernetes(&args, &bundle).await?;
128128

129129
if let Some(ref cm_name) = args.backend_ca_configmap_name {
130-
create_backend_ca_configmap_if_needed(&args, &bundle, cm_name).await?;
130+
create_backend_ca_configmap_if_needed(&args, cm_name).await?;
131131
}
132132

133133
Ok(())
@@ -326,9 +326,21 @@ async fn create_tls_secrets(
326326
Ok(())
327327
}
328328

329+
fn extract_ca_from_secret(secret: &Secret, name: &str) -> Result<String> {
330+
let data = secret
331+
.data
332+
.as_ref()
333+
.ok_or_else(|| miette::miette!("secret {name} has no data"))?;
334+
let ca = data
335+
.get("ca.crt")
336+
.ok_or_else(|| miette::miette!("secret {name} has no ca.crt key"))?;
337+
String::from_utf8(ca.0.clone())
338+
.into_diagnostic()
339+
.wrap_err("ca.crt is not valid UTF-8")
340+
}
341+
329342
async fn create_backend_ca_configmap_if_needed(
330343
args: &CertgenArgs,
331-
bundle: &PkiBundle,
332344
configmap_name: &str,
333345
) -> Result<()> {
334346
let namespace = args
@@ -340,31 +352,13 @@ async fn create_backend_ca_configmap_if_needed(
340352
.await
341353
.into_diagnostic()
342354
.wrap_err("failed to construct Kubernetes client for backend CA ConfigMap")?;
343-
let api: Api<ConfigMap> = Api::namespaced(client.clone(), namespace);
344-
345-
if api
346-
.get_opt(configmap_name)
347-
.await
348-
.into_diagnostic()
349-
.wrap_err_with(|| format!("failed to read configmap {configmap_name}"))?
350-
.is_some()
351-
{
352-
info!(
353-
namespace = %namespace,
354-
configmap = %configmap_name,
355-
"Backend CA ConfigMap already exists, skipping."
356-
);
357-
return Ok(());
358-
}
359-
360-
let ca_pem = if !args.jwt_only {
361-
bundle.ca_cert_pem.clone()
362-
} else if let Some(source_secret) = &args.backend_ca_source_secret {
363-
let secret_api: Api<Secret> = Api::namespaced(client, namespace);
355+
let secret_api: Api<Secret> = Api::namespaced(client.clone(), namespace);
356+
let cm_api: Api<ConfigMap> = Api::namespaced(client, namespace);
364357

365-
// Poll for the cert-manager Secret with timeout to handle cert issuance delay.
366-
// The Helm chart sets this to (Job activeDeadlineSeconds - 30) leaving
367-
// margin for ConfigMap creation and hook completion.
358+
// Resolve the CA from the authoritative server Secret rather than the
359+
// in-memory bundle so upgrades that enable BackendTLSPolicy on an
360+
// existing release use the CA that actually signed the server cert.
361+
let ca_pem = if let Some(source_secret) = &args.backend_ca_source_secret {
368362
let poll_timeout = std::time::Duration::from_secs(args.backend_ca_poll_timeout_seconds);
369363
let poll_interval = std::time::Duration::from_secs(2);
370364
let start = std::time::Instant::now();
@@ -416,23 +410,58 @@ async fn create_backend_ca_configmap_if_needed(
416410
elapsed_secs = start.elapsed().as_secs(),
417411
"cert-manager TLS certificate found."
418412
);
419-
420-
let data = secret
421-
.data
422-
.ok_or_else(|| miette::miette!("secret {source_secret} has no data"))?;
423-
let ca = data
424-
.get("ca.crt")
425-
.ok_or_else(|| miette::miette!("secret {source_secret} has no ca.crt key"))?;
426-
String::from_utf8(ca.0.clone())
413+
extract_ca_from_secret(&secret, source_secret)?
414+
} else if let Some(server_secret) = &args.server_secret_name {
415+
let secret = secret_api
416+
.get(server_secret)
417+
.await
427418
.into_diagnostic()
428-
.wrap_err("ca.crt is not valid UTF-8")?
419+
.wrap_err_with(|| format!("failed to read server secret {server_secret}"))?;
420+
extract_ca_from_secret(&secret, server_secret)?
429421
} else {
430422
return Err(miette::miette!(
431-
"--backend-ca-source-secret is required with --jwt-only \
432-
and --backend-ca-configmap-name"
423+
"--backend-ca-source-secret or --server-secret-name is required \
424+
with --backend-ca-configmap-name"
433425
));
434426
};
435427

428+
// Reconcile: create or update so the backend CA stays current across
429+
// CA rotations and upgrades.
430+
if let Some(existing) = cm_api
431+
.get_opt(configmap_name)
432+
.await
433+
.into_diagnostic()
434+
.wrap_err_with(|| format!("failed to read configmap {configmap_name}"))?
435+
{
436+
let up_to_date = existing
437+
.data
438+
.as_ref()
439+
.and_then(|d| d.get("ca.crt"))
440+
.map(String::as_str)
441+
== Some(&ca_pem);
442+
if up_to_date {
443+
info!(
444+
namespace = %namespace,
445+
configmap = %configmap_name,
446+
"Backend CA ConfigMap is up-to-date, skipping."
447+
);
448+
return Ok(());
449+
}
450+
let mut updated = existing;
451+
updated.data = Some(BTreeMap::from([("ca.crt".to_string(), ca_pem)]));
452+
cm_api
453+
.replace(configmap_name, &PostParams::default(), &updated)
454+
.await
455+
.into_diagnostic()
456+
.wrap_err_with(|| format!("failed to update configmap {configmap_name}"))?;
457+
info!(
458+
namespace = %namespace,
459+
configmap = %configmap_name,
460+
"Backend CA ConfigMap updated with current CA."
461+
);
462+
return Ok(());
463+
}
464+
436465
let configmap = ConfigMap {
437466
metadata: ObjectMeta {
438467
name: Some(configmap_name.to_string()),
@@ -442,7 +471,8 @@ async fn create_backend_ca_configmap_if_needed(
442471
..Default::default()
443472
};
444473

445-
api.create(&PostParams::default(), &configmap)
474+
cm_api
475+
.create(&PostParams::default(), &configmap)
446476
.await
447477
.into_diagnostic()
448478
.wrap_err_with(|| format!("failed to create configmap {configmap_name}"))?;

‎deploy/helm/openshell/templates/cert-manager-pki.yaml‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,6 @@ metadata:
88
namespace: {{ .Release.Namespace }}
99
labels:
1010
{{- include "openshell.labels" . | nindent 4 }}
11-
annotations:
12-
helm.sh/hook: pre-install,pre-upgrade
13-
helm.sh/hook-weight: "-30"
1411
spec:
1512
selfSigned: {}
1613
---
@@ -21,9 +18,6 @@ metadata:
2118
namespace: {{ .Release.Namespace }}
2219
labels:
2320
{{- include "openshell.labels" . | nindent 4 }}
24-
annotations:
25-
helm.sh/hook: pre-install,pre-upgrade
26-
helm.sh/hook-weight: "-30"
2721
spec:
2822
isCA: true
2923
commonName: openshell-ca
@@ -43,9 +37,6 @@ metadata:
4337
namespace: {{ .Release.Namespace }}
4438
labels:
4539
{{- include "openshell.labels" . | nindent 4 }}
46-
annotations:
47-
helm.sh/hook: pre-install,pre-upgrade
48-
helm.sh/hook-weight: "-30"
4940
spec:
5041
ca:
5142
secretName: {{ .Values.certManager.caSecretName | quote }}
@@ -75,9 +66,6 @@ metadata:
7566
namespace: {{ .Release.Namespace }}
7667
labels:
7768
{{- include "openshell.labels" . | nindent 4 }}
78-
annotations:
79-
helm.sh/hook: pre-install,pre-upgrade
80-
helm.sh/hook-weight: "-30"
8169
spec:
8270
secretName: {{ .Values.server.tls.certSecretName | quote }}
8371
duration: {{ .Values.certManager.certificateDuration | quote }}
@@ -119,9 +107,6 @@ metadata:
119107
namespace: {{ .Release.Namespace }}
120108
labels:
121109
{{- include "openshell.labels" . | nindent 4 }}
122-
annotations:
123-
helm.sh/hook: pre-install,pre-upgrade
124-
helm.sh/hook-weight: "-30"
125110
spec:
126111
secretName: {{ include "openshell.fullname" . }}-server-external-tls
127112
duration: {{ .Values.certManager.certificateDuration | quote }}
@@ -153,9 +138,6 @@ metadata:
153138
namespace: {{ .Release.Namespace }}
154139
labels:
155140
{{- include "openshell.labels" . | nindent 4 }}
156-
annotations:
157-
helm.sh/hook: pre-install,pre-upgrade
158-
helm.sh/hook-weight: "-30"
159141
spec:
160142
secretName: {{ .Values.server.tls.clientTlsSecretName | quote }}
161143
duration: {{ .Values.certManager.certificateDuration | quote }}

‎deploy/helm/openshell/templates/certgen.yaml‎

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ rules:
3434
{{- if .Values.grpcRoute.backendTLSPolicy.enabled }}
3535
- apiGroups: [""]
3636
resources: ["configmaps"]
37-
verbs: ["get", "create"]
37+
verbs: ["get", "create", "update"]
3838
{{- end }}
3939
---
4040
apiVersion: rbac.authorization.k8s.io/v1
@@ -119,14 +119,65 @@ spec:
119119
- --server-san={{ . }}
120120
{{- end }}
121121
{{- end }}
122-
{{- if .Values.grpcRoute.backendTLSPolicy.enabled }}
122+
{{- if and .Values.grpcRoute.backendTLSPolicy.enabled (not .Values.certManager.enabled) }}
123+
- --backend-ca-configmap-name={{ include "openshell.backendCaConfigMapName" . }}
124+
{{- end }}
125+
{{- if and .Values.certManager.enabled .Values.grpcRoute.backendTLSPolicy.enabled }}
126+
---
127+
# Post-install Job that polls for the cert-manager-issued server Secret and
128+
# creates / reconciles the backend CA ConfigMap. Runs after regular resources
129+
# (including the cert-manager Certificate objects) are applied so cert-manager
130+
# has started issuing before the poll begins.
131+
apiVersion: batch/v1
132+
kind: Job
133+
metadata:
134+
name: {{ $hookName }}-backend-ca
135+
namespace: {{ $ns }}
136+
labels:
137+
{{- include "openshell.labels" . | nindent 4 }}
138+
annotations:
139+
helm.sh/hook: post-install,post-upgrade
140+
helm.sh/hook-weight: "0"
141+
helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded
142+
spec:
143+
backoffLimit: 3
144+
activeDeadlineSeconds: {{ add .Values.pkiInitJob.timeoutSeconds 30 }}
145+
ttlSecondsAfterFinished: 300
146+
template:
147+
metadata:
148+
labels:
149+
{{- include "openshell.selectorLabels" . | nindent 8 }}
150+
spec:
151+
restartPolicy: OnFailure
152+
serviceAccountName: {{ $hookName }}
153+
{{- with .Values.imagePullSecrets }}
154+
imagePullSecrets:
155+
{{- toYaml . | nindent 8 }}
156+
{{- end }}
157+
containers:
158+
- name: certgen
159+
image: {{ include "openshell.image" . | quote }}
160+
imagePullPolicy: {{ .Values.image.pullPolicy }}
161+
securityContext:
162+
allowPrivilegeEscalation: false
163+
capabilities:
164+
drop:
165+
- ALL
166+
env:
167+
- name: POD_NAMESPACE
168+
valueFrom:
169+
fieldRef:
170+
fieldPath: metadata.namespace
171+
command: ["/usr/local/bin/openshell-gateway"]
172+
args:
173+
- generate-certs
174+
- --jwt-only
175+
- --jwt-secret-name={{ include "openshell.sandboxJwtSecretName" . }}
123176
- --backend-ca-configmap-name={{ include "openshell.backendCaConfigMapName" . }}
124-
{{- if .Values.certManager.enabled }}
125177
- --backend-ca-source-secret={{ .Values.server.tls.certSecretName }}
126178
- --backend-ca-poll-timeout-seconds={{ .Values.pkiInitJob.timeoutSeconds }}
127179
{{- if .Values.pkiInitJob.failOnTimeout }}
128180
- --backend-ca-fail-on-timeout
129181
{{- end }}
130-
{{- end }}
131-
{{- end }}
182+
{{- end }}
132183
{{- end }}

‎docs/reference/gateway-config.mdx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,9 @@ compute_drivers = ["kubernetes"]
464464
[openshell.gateway.tls]
465465
cert_path = "/etc/openshell-tls/server/tls.crt"
466466
key_path = "/etc/openshell-tls/server/tls.key"
467+
# client_ca_path is only rendered when server.tls.enableMtls is true (the
468+
# default). When enableMtls is false — required for BackendTLSPolicy — the
469+
# gateway runs HTTPS-only and this line is omitted by Helm.
467470
client_ca_path = "/etc/openshell-tls/client-ca/ca.crt"
468471
# When cert-manager serverIssuerRef is configured, these are populated by Helm:
469472
# external_cert_path = "/etc/openshell-tls/server-external/tls.crt"

0 commit comments

Comments
 (0)