nic-6443 commented on code in PR #2826:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2826#discussion_r3710333577
##########
internal/adc/translator/gatewayproxy.go:
##########
@@ -56,6 +57,17 @@ func (t *Translator) TranslateGatewayProxyToConfig(tctx
*provider.TranslateConte
cfg.TlsVerify = *cp.TlsVerify
}
+ if cp.CaBundle != "" {
+ // reject unusable CA material here rather than at connect time
+ if !x509.NewCertPool().AppendCertsFromPEM([]byte(cp.CaBundle)) {
+ return nil, errors.New("invalid caBundle: no
PEM-encoded certificate found")
+ }
Review Comment:
I would not require `IsCA` here. A self-signed endpoint certificate can be
supplied directly as a TLS trust anchor even when its Basic Constraints do not
mark it as a CA, and enforcing `IsCA` would reject that supported case. Parsing
every `CERTIFICATE` block is the right validation boundary.
##########
api/v1alpha1/gatewayproxy_types.go:
##########
@@ -120,6 +120,7 @@ type ControlPlaneAuth struct {
// ControlPlaneProvider defines configuration for control plane provider.
// +kubebuilder:validation:XValidation:rule="has(self.endpoints) !=
has(self.service)"
// +kubebuilder:validation:XValidation:rule="oldSelf == null ||
(!has(self.mode) && !has(oldSelf.mode)) || self.mode ==
oldSelf.mode",message="mode is immutable"
+// +kubebuilder:validation:XValidation:rule="!has(self.caBundle) ||
self.caBundle.contains('-----BEGIN CERTIFICATE-----')",message="caBundle must
be a PEM-encoded certificate"
Review Comment:
This remains valid after the translator parser follow-up: a value containing
only `-----BEGIN CERTIFICATE-----` still passes admission and then fails during
reconciliation. Please make the CEL rule require a complete PEM block (at least
the BEGIN and END markers) and add admission coverage for truncated input.
##########
internal/adc/client/executor_test.go:
##########
@@ -70,6 +70,37 @@ func TestHTTPADCExecutorBuildHTTPRequestBypassCache(t
*testing.T) {
assert.NotContains(t, raw, "bypassCache")
}
+func TestHTTPADCExecutorBuildHTTPRequestCaCert(t *testing.T) {
+ e := &HTTPADCExecutor{
+ serverURL: "http://127.0.0.1:3000";,
+ log: logr.Discard(),
+ }
+
+ build := func(config adctypes.Config) (ADCServerOpts, string) {
+ req, err := e.buildHTTPRequest(context.Background(),
"https://apisix:9180";, config, nil, nil,
+ &adctypes.Resources{}, http.MethodPut, pathSync)
+ require.NoError(t, err)
+ body, err := io.ReadAll(req.Body)
+ require.NoError(t, err)
+ var parsed ADCServerRequest
+ require.NoError(t, json.Unmarshal(body, &parsed))
+ return parsed.Task.Opts, string(body)
+ }
+
+ // Without a CA bundle the request stays what an ADC server that
predates caCert
+ // already accepts.
+ opts, raw := build(adctypes.Config{Name: "GatewayProxy/ns/name",
TlsVerify: true})
+ assert.Empty(t, opts.CaCert)
+ assert.NotContains(t, raw, "caCert")
Review Comment:
`adctypes.Config.MarshalJSON` is not used to build the ADC request:
`ADCServerTask.Config` is an `adctypes.Resources`. The `hasCaBundle` field is
therefore log-only, while the existing raw-body assertion that `caCert` is
absent already covers wire compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
