Skip to content

Commit 93c3473

Browse files
jhjaggarsjhjaggars
committed
fix(helm): address blocking review items — client-CA guard, wildcard Route, serverIssuerRef gate
1. Remove the obsolete guard rejecting serverIssuerRef + clientCaFromServerTlsSecret=true. The internal server certificate is always signed by the chart CA (the same CA that signs the client cert), so clientCaFromServerTlsSecret=true is correct — its filtered ca.crt is exactly the right trust anchor. The old workaround (mounting openshell-ca-tls directly) unnecessarily exposed the CA private key to the gateway container. Remove the client-CA overrides from docs, CI overlay, and production examples. 2. Route host validation now supports wildcard certificates per RFC 6125: single-level wildcards like *.example.com match gateway.example.com but not deep.sub.example.com. Require an explicit openshiftRoute.host when an external issuer is configured — without one, OpenShift generates a hostname absent from serverDnsNames. 3. Reject serverIssuerRef.name when certManager.enabled is false — the external certificate, its Secret mount, and the gateway TLS config all require cert-manager to be enabled. Validated on ROSA (dev.dyee.p3) with branch-built images: - Fresh install with letsencrypt-prod ClusterIssuer - SNI dual-cert: external hostname served Let's Encrypt cert - Supervisor mTLS via internal cert path: ConnectSupervisor accepted - Client CA volume: filtered ca.crt from internal server secret (no key) - CLI connected via Route + OIDC Helm tests: 81 pass across 7 suites. Signed-off-by: Jesse Jaggars <[email protected]>
1 parent 1362c65 commit 93c3473

11 files changed

Lines changed: 103 additions & 54 deletions

‎deploy/helm/openshell/README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,11 +144,11 @@ add `ci/values-spire.yaml` to the OpenShell release values files.
144144
| certManager.caSecretName | string | `"openshell-ca-tls"` | Secret created for the intermediate CA (Certificate with isCA: true). |
145145
| certManager.certificateDuration | string | `"8760h"` | Duration for cert-manager-issued certificates. |
146146
| certManager.certificateRenewBefore | string | `"720h"` | Renewal window for cert-manager-issued certificates. |
147-
| certManager.clientCaFromServerTlsSecret | bool | `true` | Mount gateway client CA from the server TLS secret's ca.crt (populated by cert-manager for certs issued by a CA Issuer). Set to false when serverIssuerRef points at an external issuer (its secret's ca.crt would be that issuer's chain, not the CA that signs the client cert). When false, also set server.tls.clientCaSecretName to a secret containing the actual client CA — for example caSecretName's value, since that's the CA the client certificate above is issued from by default. |
147+
| certManager.clientCaFromServerTlsSecret | bool | `true` | Mount gateway client CA from the internal server TLS secret's ca.crt. The internal server certificate is always signed by the chart CA — the same CA that signs the client (mTLS) certificate — so the default (true) is correct for all configurations, including when serverIssuerRef is set. Only set to false if you mount the client CA from a separate secret via server.tls.clientCaSecretName. |
148148
| certManager.enabled | bool | `false` | Create cert-manager Issuer and Certificate resources. When enabled, cert-manager owns TLS and the chart runs a JWT-only certgen hook to create the sandbox JWT signing Secret that cert-manager does not manage. |
149149
| certManager.serverDnsNames | list | `["openshell","openshell.openshell.svc","openshell.openshell.svc.cluster.local","localhost","openshell.localhost","*.openshell.localhost","host.docker.internal"]` | DNS SANs on the cert-manager-issued server certificate. |
150150
| certManager.serverIpAddresses | list | `["127.0.0.1"]` | IP SANs on the cert-manager-issued server certificate. |
151-
| certManager.serverIssuerRef | object | `{"group":"","kind":"","name":""}` | Override the issuerRef for the server Certificate (e.g. a real ACME ClusterIssuer for a publicly-trusted cert on an external hostname). Leave name empty to use the chart's own self-signed CA issuer (default). |
151+
| certManager.serverIssuerRef | object | `{"group":"","kind":"","name":""}` | Override the issuerRef for the external server Certificate (e.g. a real ACME ClusterIssuer for a publicly-trusted cert on an external hostname). When set, the chart creates a second server certificate from this issuer with only the hostnames in serverDnsNames; the internal server certificate is always signed by the chart's own CA. Leave name empty to use the chart CA for all server certificates (default). Requires certManager.enabled=true. |
152152
| fullnameOverride | string | `""` | Override the full generated resource name. |
153153
| grpcRoute.enabled | bool | `false` | Create a Gateway API GRPCRoute for the gateway service. |
154154
| grpcRoute.gateway.className | string | `"eg"` | GatewayClass to reference. Envoy Gateway installs one named "eg". |

‎deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,18 +9,16 @@
99
# The ClusterIssuer name below is a placeholder for render coverage; a real
1010
# deployment must reference an Issuer/ClusterIssuer that's actually installed
1111
# and Ready in the target cluster. See docs/kubernetes/managing-certificates.mdx.
12+
#
13+
# clientCaFromServerTlsSecret defaults to true and is correct here: the
14+
# internal server certificate is always signed by the chart CA, so its
15+
# ca.crt is exactly the CA that signed the client (mTLS) certificate.
1216

1317
server:
1418
disableTls: false
15-
tls:
16-
# Must name a secret containing the actual client CA when
17-
# clientCaFromServerTlsSecret is false — here, the same CA secret
18-
# cert-manager creates for signing the client (mTLS) certificate.
19-
clientCaSecretName: openshell-ca-tls
2019

2120
certManager:
2221
enabled: true
23-
clientCaFromServerTlsSecret: false
2422
serverIssuerRef:
2523
name: letsencrypt-prod
2624
kind: ClusterIssuer

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,6 @@ spec:
4646
{{- if and (not .Values.certManager.clientCaFromServerTlsSecret) (eq .Values.server.tls.clientCaSecretName "openshell-server-client-ca") }}
4747
{{- fail "certManager.clientCaFromServerTlsSecret is false but server.tls.clientCaSecretName is still the default (openshell-server-client-ca), which nothing creates when cert-manager owns TLS. Set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls), or set it to empty to disable mTLS client verification." }}
4848
{{- end }}
49-
{{- if and $externalServerIssuer .Values.certManager.clientCaFromServerTlsSecret }}
50-
{{- fail "certManager.serverIssuerRef.name is set but certManager.clientCaFromServerTlsSecret is still true \u2014 the server cert Secret's ca.crt comes from the external issuer, not the CA that signs the client (mTLS) certificate. Set certManager.clientCaFromServerTlsSecret=false and set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls)." }}
51-
{{- end }}
5249
{{- if $externalServerIssuer }}
5350
{{- if not .Values.certManager.serverDnsNames }}
5451
{{- fail "certManager.serverIssuerRef.name is set but certManager.serverDnsNames is empty — the external certificate requires at least one externally-resolvable DNS name." }}

‎deploy/helm/openshell/templates/gateway-config.yaml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ One value is intentionally NOT rendered here:
1212
when server.externalDbSecret is set, otherwise
1313
--db-url arg for SQLite
1414
*/}}
15+
{{- if and .Values.certManager.serverIssuerRef.name (not .Values.certManager.enabled) }}
16+
{{- fail "certManager.serverIssuerRef.name is set but certManager.enabled is false \u2014 the external server certificate, its Secret mount, and the gateway TLS configuration all require cert-manager to be enabled. Set certManager.enabled=true or remove certManager.serverIssuerRef.name." }}
17+
{{- end }}
1518
apiVersion: v1
1619
kind: ConfigMap
1720
metadata:

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,26 @@
55
{{- if .Values.server.disableTls }}
66
{{- fail "openshiftRoute.enabled=true requires TLS (server.disableTls must be false) \u2014 a passthrough Route forwards encrypted traffic by SNI, so the gateway must terminate its own TLS." }}
77
{{- end }}
8-
{{- if and .Values.openshiftRoute.host .Values.certManager.serverIssuerRef.name .Values.certManager.serverDnsNames (not (has .Values.openshiftRoute.host .Values.certManager.serverDnsNames)) }}
9-
{{- fail (printf "openshiftRoute.host %q is not listed in certManager.serverDnsNames %v — the Route will forward SNI for a hostname the external certificate does not cover, causing TLS verification failures for CLI clients." .Values.openshiftRoute.host .Values.certManager.serverDnsNames) }}
8+
{{- if and .Values.certManager.serverIssuerRef.name (not .Values.openshiftRoute.host) }}
9+
{{- fail "openshiftRoute.enabled=true with certManager.serverIssuerRef requires an explicit openshiftRoute.host \u2014 without one, OpenShift generates a hostname absent from certManager.serverDnsNames, causing the gateway to serve its internal certificate to external clients." }}
10+
{{- end }}
11+
{{- if and .Values.openshiftRoute.host .Values.certManager.serverIssuerRef.name .Values.certManager.serverDnsNames }}
12+
{{- $routeHost := .Values.openshiftRoute.host }}
13+
{{- $hostCovered := false }}
14+
{{- range .Values.certManager.serverDnsNames }}
15+
{{- if eq . $routeHost }}
16+
{{- $hostCovered = true }}
17+
{{- else if hasPrefix "*." . }}
18+
{{- $wildcardSuffix := trimPrefix "*" . }}
19+
{{- $prefix := trimSuffix $wildcardSuffix $routeHost }}
20+
{{- if and (ne $prefix $routeHost) (gt (len $prefix) 0) (not (contains "." $prefix)) }}
21+
{{- $hostCovered = true }}
22+
{{- end }}
23+
{{- end }}
24+
{{- end }}
25+
{{- if not $hostCovered }}
26+
{{- fail (printf "openshiftRoute.host %q is not covered by certManager.serverDnsNames %v \u2014 the Route will forward SNI for a hostname the external certificate does not cover, causing TLS verification failures for CLI clients. Exact names and single-level wildcards (e.g. *.example.com) are checked." $routeHost .Values.certManager.serverDnsNames) }}
27+
{{- end }}
1028
{{- end }}
1129
apiVersion: route.openshift.io/v1
1230
kind: Route

‎deploy/helm/openshell/tests/cert_manager_pki_test.yaml‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,6 @@ tests:
5959
certManager.enabled: true
6060
certManager.serverIssuerRef.name: letsencrypt-prod
6161
certManager.serverIssuerRef.kind: ClusterIssuer
62-
certManager.clientCaFromServerTlsSecret: false
63-
server.tls.clientCaSecretName: openshell-ca-tls
6462
certManager.serverDnsNames:
6563
- openshell.example.com
6664
asserts:
@@ -98,8 +96,6 @@ tests:
9896
certManager.enabled: true
9997
certManager.serverIssuerRef.name: letsencrypt-prod
10098
certManager.serverIssuerRef.kind: ClusterIssuer
101-
certManager.clientCaFromServerTlsSecret: false
102-
server.tls.clientCaSecretName: openshell-ca-tls
10399
certManager.serverDnsNames:
104100
- openshell.example.com
105101
asserts:
@@ -136,26 +132,11 @@ tests:
136132
content: localhost
137133
documentIndex: 4
138134

139-
- it: fails when serverIssuerRef is set but clientCaFromServerTlsSecret is true
140-
template: templates/cert-manager-pki.yaml
141-
set:
142-
certManager.enabled: true
143-
certManager.serverIssuerRef.name: letsencrypt-prod
144-
certManager.serverIssuerRef.kind: ClusterIssuer
145-
certManager.clientCaFromServerTlsSecret: true
146-
certManager.serverDnsNames:
147-
- openshell.example.com
148-
asserts:
149-
- failedTemplate:
150-
errorMessage: "certManager.serverIssuerRef.name is set but certManager.clientCaFromServerTlsSecret is still true \u2014 the server cert Secret's ca.crt comes from the external issuer, not the CA that signs the client (mTLS) certificate. Set certManager.clientCaFromServerTlsSecret=false and set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls)."
151-
152135
- it: fails when serverIssuerRef is set but serverDnsNames contains internal-only names
153136
template: templates/cert-manager-pki.yaml
154137
set:
155138
certManager.enabled: true
156139
certManager.serverIssuerRef.name: letsencrypt-prod
157-
certManager.clientCaFromServerTlsSecret: false
158-
server.tls.clientCaSecretName: openshell-ca-tls
159140
# serverDnsNames is left at the default which contains "openshell.openshell.svc", "localhost", etc.
160141
asserts:
161142
- failedTemplate:
@@ -176,8 +157,6 @@ tests:
176157
certManager.enabled: true
177158
certManager.serverIssuerRef.name: letsencrypt-prod
178159
certManager.serverIssuerRef.kind: ClusterIssuer
179-
certManager.clientCaFromServerTlsSecret: false
180-
server.tls.clientCaSecretName: openshell-ca-tls
181160
certManager.serverDnsNames:
182161
- openshell.example.com
183162
asserts:

‎deploy/helm/openshell/tests/gateway_config_test.yaml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -499,3 +499,12 @@ tests:
499499
- matchRegex:
500500
path: spec.template.spec.volumes[1].name
501501
pattern: '^sandbox-jwt$'
502+
503+
- it: fails when serverIssuerRef is set but certManager is disabled
504+
template: templates/statefulset.yaml
505+
set:
506+
certManager.serverIssuerRef.name: letsencrypt-prod
507+
certManager.enabled: false
508+
asserts:
509+
- failedTemplate:
510+
errorMessage: "certManager.serverIssuerRef.name is set but certManager.enabled is false \u2014 the external server certificate, its Secret mount, and the gateway TLS configuration all require cert-manager to be enabled. Set certManager.enabled=true or remove certManager.serverIssuerRef.name."

‎deploy/helm/openshell/tests/route_test.yaml‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,54 @@ tests:
5858
- failedTemplate:
5959
errorMessage: "openshiftRoute.enabled=true requires TLS (server.disableTls must be false) \u2014 a passthrough Route forwards encrypted traffic by SNI, so the gateway must terminate its own TLS."
6060

61+
- it: fails when external issuer is set but Route host is empty
62+
set:
63+
openshiftRoute.enabled: true
64+
certManager.enabled: true
65+
certManager.serverIssuerRef.name: letsencrypt-prod
66+
certManager.serverDnsNames:
67+
- openshell.example.com
68+
asserts:
69+
- failedTemplate:
70+
errorMessage: "openshiftRoute.enabled=true with certManager.serverIssuerRef requires an explicit openshiftRoute.host \u2014 without one, OpenShift generates a hostname absent from certManager.serverDnsNames, causing the gateway to serve its internal certificate to external clients."
71+
72+
- it: accepts a Route host covered by a wildcard serverDnsNames entry
73+
set:
74+
openshiftRoute.enabled: true
75+
openshiftRoute.host: gateway.example.com
76+
certManager.enabled: true
77+
certManager.serverIssuerRef.name: letsencrypt-prod
78+
certManager.serverDnsNames:
79+
- "*.example.com"
80+
asserts:
81+
- equal:
82+
path: spec.host
83+
value: gateway.example.com
84+
85+
- it: rejects a Route host not covered by wildcard or exact serverDnsNames
86+
set:
87+
openshiftRoute.enabled: true
88+
openshiftRoute.host: gateway.other.com
89+
certManager.enabled: true
90+
certManager.serverIssuerRef.name: letsencrypt-prod
91+
certManager.serverDnsNames:
92+
- "*.example.com"
93+
asserts:
94+
- failedTemplate:
95+
errorMessage: "openshiftRoute.host \"gateway.other.com\" is not covered by certManager.serverDnsNames [*.example.com] \u2014 the Route will forward SNI for a hostname the external certificate does not cover, causing TLS verification failures for CLI clients. Exact names and single-level wildcards (e.g. *.example.com) are checked."
96+
97+
- it: rejects a multi-level subdomain against a single-level wildcard
98+
set:
99+
openshiftRoute.enabled: true
100+
openshiftRoute.host: deep.sub.example.com
101+
certManager.enabled: true
102+
certManager.serverIssuerRef.name: letsencrypt-prod
103+
certManager.serverDnsNames:
104+
- "*.example.com"
105+
asserts:
106+
- failedTemplate:
107+
errorMessage: "openshiftRoute.host \"deep.sub.example.com\" is not covered by certManager.serverDnsNames [*.example.com] \u2014 the Route will forward SNI for a hostname the external certificate does not cover, causing TLS verification failures for CLI clients. Exact names and single-level wildcards (e.g. *.example.com) are checked."
108+
61109
- it: renders custom annotations
62110
set:
63111
openshiftRoute.enabled: true

‎deploy/helm/openshell/values.yaml‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -353,20 +353,22 @@ certManager:
353353
enabled: false
354354
# -- Secret created for the intermediate CA (Certificate with isCA: true).
355355
caSecretName: openshell-ca-tls
356-
# -- Override the issuerRef for the server Certificate (e.g. a real ACME
357-
# ClusterIssuer for a publicly-trusted cert on an external hostname). Leave
358-
# name empty to use the chart's own self-signed CA issuer (default).
356+
# -- Override the issuerRef for the external server Certificate (e.g. a real
357+
# ACME ClusterIssuer for a publicly-trusted cert on an external hostname).
358+
# When set, the chart creates a second server certificate from this issuer
359+
# with only the hostnames in serverDnsNames; the internal server certificate
360+
# is always signed by the chart's own CA. Leave name empty to use the chart
361+
# CA for all server certificates (default). Requires certManager.enabled=true.
359362
serverIssuerRef:
360363
name: ""
361364
kind: ""
362365
group: ""
363-
# -- Mount gateway client CA from the server TLS secret's ca.crt (populated by
364-
# cert-manager for certs issued by a CA Issuer). Set to false when
365-
# serverIssuerRef points at an external issuer (its secret's ca.crt would be
366-
# that issuer's chain, not the CA that signs the client cert). When false,
367-
# also set server.tls.clientCaSecretName to a secret containing the actual
368-
# client CA — for example caSecretName's value, since that's the CA the
369-
# client certificate above is issued from by default.
366+
# -- Mount gateway client CA from the internal server TLS secret's ca.crt.
367+
# The internal server certificate is always signed by the chart CA — the same
368+
# CA that signs the client (mTLS) certificate — so the default (true) is
369+
# correct for all configurations, including when serverIssuerRef is set.
370+
# Only set to false if you mount the client CA from a separate secret via
371+
# server.tls.clientCaSecretName.
370372
clientCaFromServerTlsSecret: true
371373
# -- Duration for cert-manager-issued certificates.
372374
certificateDuration: 8760h

‎docs/kubernetes/managing-certificates.mdx‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,6 @@ helm upgrade --install openshell \
7474
--version <version> \
7575
--namespace openshell \
7676
--set certManager.enabled=true \
77-
--set certManager.clientCaFromServerTlsSecret=false \
78-
--set server.tls.clientCaSecretName=openshell-ca-tls \
7977
--set certManager.serverIssuerRef.name=letsencrypt-prod \
8078
--set certManager.serverIssuerRef.kind=ClusterIssuer \
8179
--set certManager.serverDnsNames[0]=openshell.example.com
@@ -112,10 +110,10 @@ receive the ACME certificate (via SNI) which they cannot verify against the
112110
chart CA.
113111
</Note>
114112

115-
Set `certManager.clientCaFromServerTlsSecret=false` whenever `serverIssuerRef`
116-
is set, and set `server.tls.clientCaSecretName` to a secret containing the
117-
client CA — by default that's `certManager.caSecretName` (`openshell-ca-tls`),
118-
the CA the chart issues the client certificate from.
113+
The default `clientCaFromServerTlsSecret=true` is correct even when
114+
`serverIssuerRef` is set: the internal server certificate is always signed
115+
by the chart CA (the same CA that signs the client certificate), so its
116+
`ca.crt` is the right trust anchor for mTLS verification.
119117

120118
## Next Steps
121119

0 commit comments

Comments
 (0)