Copilot commented on code in PR #2826:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2826#discussion_r3665203976
##########
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:
`AppendCertsFromPEM` only validates that *some* certificates can be parsed;
it does not ensure they are appropriate CA certificates. If the intent is
specifically a CA bundle, consider parsing the certs and rejecting bundles that
contain no certs with `IsCA == true` (or otherwise documenting that any
certificate can be used as a trust anchor). This reduces configuration pitfalls
and aligns behavior with the field description.
##########
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:
The CEL validation rule is very permissive: any string containing
`-----BEGIN CERTIFICATE-----` will pass, including non-PEM or truncated values.
To better match the error message and intent, consider tightening the rule to
require both BEGIN/END markers (and ideally allow surrounding whitespace) using
`matches()` with a regex, or at least check for `-----END CERTIFICATE-----` as
well.
##########
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:
Given the compatibility requirement, this test should also assert that the
request body does not gain any new fields when no bundle is set (e.g.,
`hasCaBundle` if present in the serialized `Config`). Adding an explicit
`assert.NotContains(t, raw, \"hasCaBundle\")` (or equivalent structural
assertion) would prevent accidental wire-format drift.
--
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]
