fix: verify control-plane TLS certificate by default - #2811
fix: verify control-plane TLS certificate by default#2811shreemaan-abhishek wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the default security posture for control-plane (Admin API) connections by ensuring TLS certificates are verified unless the user explicitly opts out via tlsVerify: false. It addresses an unsafe implicit default where omitting tlsVerify could lead to tlsSkipVerify: true and expose the AdminKey/config to MITM risks.
Changes:
- Default
GatewayProxy.spec.provider.controlPlane.tlsVerifytotrueat the CRD/schema level and in the translator (to stay correct even with older CRDs). - Add a translator unit test covering unset vs explicit
true/falsebehavior. - Update generated CRD manifest and API reference documentation to reflect the secure default and clearly document the opt-out risk.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/adc/translator/gatewayproxy.go | Sets cfg.TlsVerify to true by default and only overrides it when cp.TlsVerify is explicitly provided. |
| internal/adc/translator/gatewayproxy_test.go | Adds unit test asserting default verify behavior and explicit opt-in/out handling. |
| docs/en/latest/reference/api-reference.md | Documents tlsVerify defaulting to true and warns about the security implications of disabling verification. |
| config/crd/bases/apisix.apache.org_gatewayproxies.yaml | Regenerates CRD with default: true and expanded description for tlsVerify. |
| api/v1alpha1/gatewayproxy_types.go | Adds kubebuilder default marker and expanded field documentation for TlsVerify. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
GatewayProxy's controlPlane.tlsVerify is a *bool with no default, so omitting it left the Go zero value false, which the executor inverts to tlsSkipVerify:true. A fresh install following the shipped https examples therefore skipped certificate verification on the channel carrying the AdminKey. Default tlsVerify to true via +kubebuilder:default=true and a secure default in the translator before honoring an explicit override, so only an explicit tlsVerify:false opts out. Regenerate CRD/docs and add a translator test locking in the default. Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
aba26fb to
d635f39
Compare
There was a problem hiding this comment.
Agree with verifying by default — the population that configures https:// at all is the population whose control plane sits outside the cluster, which is exactly where verification matters. In-cluster setups use plain http:// on 9180 anyway (test/e2e/scaffold/k8s.go:316-329), so they are unaffected.
Two things to sort out first, though.
1. This breaks the examples shipped in this repo.
examples/httpbin/quickstart.yaml:94, httproute.yaml:36, ingress.yaml:28, tcproute.yaml:36 all point at https://127.0.0.1:7443 with no tlsVerify. The PR cites them as evidence of the insecure default but doesn't touch them, so after this they go from "insecure but working" to "cannot connect", failing inside ADC sync where it's hard to diagnose. They need an explicit tlsVerify: false plus a comment that it's a self-signed dev cert.
2. See the inline comment — there is no way to supply a CA.
Also needs an upgrade note: CRD defaulting applies to existing objects on read from etcd, so every GatewayProxy that omits tlsVerify flips to verify the moment the CRD is upgraded.
| // Defaults to true. Setting it to false disables certificate verification and | ||
| // exposes the AdminKey to man-in-the-middle attacks over https endpoints. | ||
| // +optional | ||
| // +kubebuilder:default=true |
There was a problem hiding this comment.
ControlPlaneProvider has no way to supply a CA — only Endpoints/Service/TlsVerify/Auth — and the ADC executor exposes just TlsSkipVerify (internal/adc/client/executor.go:81,337). With a self-signed or private-CA control plane, the only escape from verify=true is tlsVerify: false.
The likely outcome is that everyone hits the error and pastes tlsVerify: false into their manifests, which leaves us worse off than today: the opt-out becomes boilerplate nobody thinks about, and changing the default is no longer available as a lever.
Suggest landing controlPlane.caBundle (inline PEM or a Secret ref) first, then flipping the default in a release with an upgrade note.
There was a problem hiding this comment.
Agreed; this is already a concrete regression in the shipped manifests. examples/httpbin/{quickstart,httproute,ingress,tcproute}.yaml use the documented https://127.0.0.1:7443 endpoint without tlsVerify, so they stop syncing after this default changes. Since no CA material can be passed to ADC, a private or self-signed CA has no secure migration path. Please add CA support plus an upgrade path and update those examples before flipping the default.
What this PR does
GatewayProxyspec.provider.controlPlane.tlsVerifyis a*boolwith no default. When omitted (as the shippedexamples/httpbinmanifests do, while pointing athttps://Admin API endpoints), it decodes tonil, the translator leavescfg.TlsVerifyat the Go zero valuefalse, and the executor inverts that intotlsSkipVerify: true. The result is that a default install skips certificate verification on the connection that carries the AdminKey and the full gateway config.This makes verification the secure default:
+kubebuilder:default=trueon the CRD field, so an omittedtlsVerifybecomestrue.cfg.TlsVerify = truein the translator before honoring an explicit value, so behavior is correct even against an older CRD without the default. Only an explicittlsVerify: falseopts out.falsedisables certificate verification.Testing
Added a translator unit test asserting the default: unset -> verify, explicit
false-> skip, explicittrue-> verify.