-
Notifications
You must be signed in to change notification settings - Fork 4
feat: support a CA bundle for the control plane connection #447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
867db94
8090806
256b91c
6d905a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
| type ControlPlaneProvider struct { | ||
| // Mode specifies the mode of control plane provider. | ||
| // Can be `apisix` or `apisix-standalone`. | ||
|
|
@@ -136,6 +137,13 @@ type ControlPlaneProvider struct { | |
| // +optional | ||
| TlsVerify *bool `json:"tlsVerify,omitempty"` | ||
|
|
||
| // CaBundle is a PEM-encoded CA certificate (or bundle) used to verify the | ||
| // control plane's TLS certificate, in place of the system trust store. | ||
| // Set it when the control plane uses a self-signed or private CA certificate. | ||
| // It has no effect when tlsVerify is false. | ||
| // +optional | ||
| CaBundle string `json:"caBundle,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Update the Helm-bundled CRD before exposing this field. |
||
|
|
||
| // Auth specifies the authentication configuration. | ||
| // +kubebuilder:validation:Required | ||
| Auth ControlPlaneAuth `json:"auth"` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,7 +84,11 @@ type ADCServerOpts struct { | |
| LabelSelector map[string]string `json:"labelSelector,omitempty"` | ||
| IncludeResourceType []string `json:"includeResourceType,omitempty"` | ||
| TlsSkipVerify *bool `json:"tlsSkipVerify,omitempty"` | ||
| CacheKey string `json:"cacheKey"` | ||
| // CaCert is the PEM-encoded CA certificate (or bundle) the ADC server verifies | ||
| // the control plane against. Older ADC servers ignore it, and omitempty keeps | ||
| // requests without a CA bundle byte for byte what they were. | ||
| CaCert string `json:"caCert,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Ship an ADC version that honors |
||
| CacheKey string `json:"cacheKey"` | ||
| // BypassCache is only accepted by the /sync task of ADC >= 0.27.0. Both ADC task | ||
| // schemas reject unknown fields, so omitempty is what keeps every other request -- | ||
| // /validate, and every sync that is not recovering from a rejection -- byte for byte | ||
|
|
@@ -103,6 +107,7 @@ func (r ADCServerRequest) MarshalLog() any { | |
| "labelSelector": r.Task.Opts.LabelSelector, | ||
| "includeResourceType": r.Task.Opts.IncludeResourceType, | ||
| "tlsSkipVerify": r.Task.Opts.TlsSkipVerify, | ||
| "hasCaCert": r.Task.Opts.CaCert != "", | ||
| "cacheKey": r.Task.Opts.CacheKey, | ||
| "config": r.Task.Config.MarshalLog(), | ||
| } | ||
|
|
@@ -244,7 +249,7 @@ func (e *HTTPADCExecutor) runHTTPSyncForSingleServer(ctx context.Context, server | |
| } | ||
|
|
||
| // Build HTTP request | ||
| req, err := e.buildHTTPRequest(ctx, serverAddr, config, labels, types, resources, http.MethodPut, pathSync) | ||
| req, err := e.buildHTTPRequest(ctx, serverAddr, config, labels, types, resources, pathSync) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to build HTTP request: %w", err) | ||
| } | ||
|
|
@@ -278,7 +283,7 @@ func (e *HTTPADCExecutor) runHTTPValidateForSingleServer(ctx context.Context, se | |
| return fmt.Errorf("failed to load resources from file %s: %w", filePath, err) | ||
| } | ||
|
|
||
| req, err := e.buildHTTPRequest(ctx, serverAddr, config, labels, types, resources, http.MethodPut, pathValidate) | ||
| req, err := e.buildHTTPRequest(ctx, serverAddr, config, labels, types, resources, pathValidate) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to build validate request: %w", err) | ||
| } | ||
|
|
@@ -349,7 +354,7 @@ func (e *HTTPADCExecutor) loadResourcesFromFile(filePath string) (*adctypes.Reso | |
| } | ||
|
|
||
| // buildHTTPRequest builds the HTTP request for ADC Server | ||
| func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr string, config adctypes.Config, labels map[string]string, types []string, resources *adctypes.Resources, method string, path string) (*http.Request, error) { | ||
| func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr string, config adctypes.Config, labels map[string]string, types []string, resources *adctypes.Resources, path string) (*http.Request, error) { | ||
| // Prepare request body | ||
| tlsVerify := config.TlsVerify | ||
| bypassCache := path == pathSync && config.BypassCache | ||
|
|
@@ -362,6 +367,7 @@ func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr strin | |
| LabelSelector: labels, | ||
| IncludeResourceType: types, | ||
| TlsSkipVerify: ptr.To(!tlsVerify), | ||
| CaCert: config.CaBundle, | ||
| CacheKey: config.Name, | ||
| BypassCache: bypassCache, | ||
| }, | ||
|
|
@@ -385,10 +391,11 @@ func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr strin | |
| "labelSelector", labels, | ||
| "includeResourceType", types, | ||
| "tlsSkipVerify", !tlsVerify, | ||
| "hasCaCert", config.CaBundle != "", | ||
| ) | ||
|
|
||
| // Create HTTP request | ||
| req, err := http.NewRequestWithContext(ctx, method, e.serverURL+path, bytes.NewBuffer(jsonData)) | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPut, e.serverURL+path, bytes.NewBuffer(jsonData)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create HTTP request: %w", err) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| // Licensed to the Apache Software Foundation (ASF) under one | ||
| // or more contributor license agreements. See the NOTICE file | ||
| // distributed with this work for additional information | ||
| // regarding copyright ownership. The ASF licenses this file | ||
| // to you under the Apache License, Version 2.0 (the | ||
| // "License"); you may not use this file except in compliance | ||
| // with the License. You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, | ||
| // software distributed under the License is distributed on an | ||
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| package translator | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/utils/ptr" | ||
|
|
||
| "github.com/apache/apisix-ingress-controller/api/v1alpha1" | ||
| "github.com/apache/apisix-ingress-controller/internal/provider" | ||
| ) | ||
|
|
||
| func newGatewayProxy(tlsVerify *bool, caBundle string) *v1alpha1.GatewayProxy { | ||
| return &v1alpha1.GatewayProxy{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: "default", | ||
| Name: "gp", | ||
| }, | ||
| Spec: v1alpha1.GatewayProxySpec{ | ||
| Provider: &v1alpha1.GatewayProxyProvider{ | ||
| Type: v1alpha1.ProviderTypeControlPlane, | ||
| ControlPlane: &v1alpha1.ControlPlaneProvider{ | ||
| Endpoints: []string{"https://cp.example.com:9180"}, | ||
| TlsVerify: tlsVerify, | ||
| CaBundle: caBundle, | ||
| Auth: v1alpha1.ControlPlaneAuth{ | ||
| Type: v1alpha1.AuthTypeAdminKey, | ||
| AdminKey: &v1alpha1.AdminKeyAuth{ | ||
| Value: "admin-key", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func TestTranslateGatewayProxyToConfigCaBundle(t *testing.T) { | ||
| t.Run("carries the CA bundle into the config", func(t *testing.T) { | ||
| tr := &Translator{Log: logr.Discard()} | ||
| tctx := provider.NewDefaultTranslateContext(context.Background()) | ||
|
|
||
| cfg, err := tr.TranslateGatewayProxyToConfig(tctx, newGatewayProxy(ptr.To(true), testCACert), false) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, cfg) | ||
| assert.True(t, cfg.TlsVerify) | ||
| assert.Equal(t, testCACert, cfg.CaBundle) | ||
| }) | ||
|
|
||
| t.Run("leaves the CA bundle empty when unset", func(t *testing.T) { | ||
| tr := &Translator{Log: logr.Discard()} | ||
| tctx := provider.NewDefaultTranslateContext(context.Background()) | ||
|
|
||
| cfg, err := tr.TranslateGatewayProxyToConfig(tctx, newGatewayProxy(ptr.To(true), ""), false) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, cfg) | ||
| assert.Empty(t, cfg.CaBundle) | ||
| }) | ||
|
|
||
| // every certificate is parsed: x509.CertPool silently skips the blocks it | ||
| // cannot decode, which would let a broken one through to the ADC server. | ||
| for name, caBundle := range map[string]string{ | ||
| "not PEM at all": "not-a-certificate", | ||
| "a header with no certificate": "-----BEGIN CERTIFICATE-----", | ||
| "an unparseable body": "-----BEGIN CERTIFICATE-----\nAAAA\n-----END CERTIFICATE-----", | ||
| "a key rather than a certificate": "-----BEGIN RSA PRIVATE KEY-----\nAAAA\n-----END RSA PRIVATE KEY-----", | ||
| "one good and one broken certificate": testCACert + | ||
| "\n-----BEGIN CERTIFICATE-----\nAAAA\n-----END CERTIFICATE-----", | ||
| } { | ||
| t.Run("rejects a CA bundle that is "+name, func(t *testing.T) { | ||
| tr := &Translator{Log: logr.Discard()} | ||
| tctx := provider.NewDefaultTranslateContext(context.Background()) | ||
|
|
||
| cfg, err := tr.TranslateGatewayProxyToConfig(tctx, newGatewayProxy(ptr.To(true), caBundle), false) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "invalid caBundle") | ||
| assert.Nil(t, cfg) | ||
| }) | ||
| } | ||
|
|
||
| t.Run("accepts a bundle of several certificates", func(t *testing.T) { | ||
| tr := &Translator{Log: logr.Discard()} | ||
| tctx := provider.NewDefaultTranslateContext(context.Background()) | ||
|
|
||
| bundle := testCACert + "\n" + testCACert | ||
| cfg, err := tr.TranslateGatewayProxyToConfig(tctx, newGatewayProxy(ptr.To(true), bundle), false) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, cfg) | ||
| assert.Equal(t, bundle, cfg.CaBundle) | ||
| }) | ||
|
|
||
| t.Run("still carries the CA bundle when verification is off", func(t *testing.T) { | ||
| tr := &Translator{Log: logr.Discard()} | ||
| tctx := provider.NewDefaultTranslateContext(context.Background()) | ||
|
|
||
| cfg, err := tr.TranslateGatewayProxyToConfig(tctx, newGatewayProxy(ptr.To(false), testCACert), false) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, cfg) | ||
| assert.False(t, cfg.TlsVerify) | ||
| assert.Equal(t, testCACert, cfg.CaBundle) | ||
| }) | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 13909
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 469
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 246
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 166
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 253
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 253
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 8992
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 12292
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 253
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 9191
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 318
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 710
🏁 Script executed:
Repository: api7/api7-ingress-controller
Length of output: 16366
🌐 Web query:
Go crypto/x509 AppendCertsFromPEM trailing garbage behavior💡 Result:
In the Go programming language, the crypto/x509.CertPool.AppendCertsFromPEM method handles PEM-encoded data by attempting to parse a series of certificates [1][2][3]. Its behavior regarding "garbage" (data that is not a valid PEM block or a valid certificate) is permissive: it silently skips any data that does not conform to the expected format [2][3]. Specifically, the function operates in a loop that uses pem.Decode to extract PEM blocks [2][4][3]: 1. If pem.Decode returns nil (indicating no more PEM blocks are found), the function breaks the loop [2][4][3]. Any remaining data at that point is treated as "trailing garbage" and is ignored without returning an error [2][3]. 2. If a PEM block is found but it does not have the type "CERTIFICATE" or contains headers, the function ignores that block and continues to the next one [2][3]. 3. If the block is a valid PEM "CERTIFICATE" but cannot be parsed by x509.ParseCertificate, the error is ignored, and the function continues processing the remaining data [2][3]. Because of this silent skipping, AppendCertsFromPEM will return true as long as at least one valid certificate is found and added to the pool [2][3]. It does not report whether invalid data, non-certificate blocks, or malformed certificate blocks were present [2][3]. Developers requiring strict validation of a PEM bundle (e.g., to ensure no invalid or unexpected data exists) should implement manual parsing using encoding/pem and x509.ParseCertificate to inspect every block individually [5].
Citations:
Tighten
caBundlevalidation. The admission rule only checks for aCERTIFICATEheader, andAppendCertsFromPEMaccepts bundles with at least one valid cert even when trailing garbage or non-certificate PEM blocks remain, so malformed bundle data can still be stored and forwarded unchanged. If strict PEM is intended, validate every block and regenerate the CRD.📍 Affects 3 files
api/v1alpha1/gatewayproxy_types.go#L123-L123(this comment)config/crd/bases/apisix.apache.org_gatewayproxies.yaml#L168-L170internal/adc/translator/gatewayproxy.go#L60-L64🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 51590
🏁 Script executed:
Length of output: 13887
@jarvis9443, confirmed. The current CEL rule accepts a truncatedcaBundlebecause it checks only the BEGIN marker.ControlPlaneProviderCEL rule.caBundlewith only-----BEGIN CERTIFICATE-----.You are interacting with an AI system.