Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions api/adc/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,11 @@ type Config struct {
TlsVerify bool
BackendType string

// CaBundle is a PEM-encoded CA certificate (or bundle) used to verify the
// control plane, in place of the system trust store. Only meaningful when
// TlsVerify is true.
CaBundle string

// BypassCache makes the ADC server drop the in-memory baseline it holds for this
// cacheKey and re-derive it from the data plane before computing the diff. It is a
// per-request flag set on the sync path, not part of the translated configuration.
Expand All @@ -820,10 +825,12 @@ func (c Config) MarshalJSON() ([]byte, error) {
Name string `json:"name"`
ServerAddrs []string `json:"serverAddrs"`
TlsVerify bool `json:"tlsVerify"`
HasCaBundle bool `json:"hasCaBundle"`
}{
Name: c.Name,
ServerAddrs: c.ServerAddrs,
TlsVerify: c.TlsVerify,
HasCaBundle: c.CaBundle != "",
})
}

Expand Down
8 changes: 8 additions & 0 deletions api/v1alpha1/gatewayproxy_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Copy link
Copy Markdown
Member

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.

type ControlPlaneProvider struct {
// Mode specifies the mode of control plane provider.
// Can be `apisix` or `apisix-standalone`.
Expand All @@ -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"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Update the Helm-bundled CRD before exposing this field. apache/apisix-helm-chart still has no caBundle property in charts/apisix-ingress-controller/crds/apisixic-crds.yaml. With that standard installation the API server prunes this unknown field, so the controller never receives it. Please add the paired Helm chart PR and release dependency.


// Auth specifies the authentication configuration.
// +kubebuilder:validation:Required
Auth ControlPlaneAuth `json:"auth"`
Expand Down
10 changes: 10 additions & 0 deletions config/crd/bases/apisix.apache.org_gatewayproxies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,13 @@ spec:
- message: adminKey must be specified when type is AdminKey
rule: 'self.type == ''AdminKey'' ? has(self.adminKey) :
true'
caBundle:
description: |-
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.
type: string
endpoints:
description: Endpoints specifies the list of control plane
endpoints.
Expand Down Expand Up @@ -158,6 +165,9 @@ spec:
- message: mode is immutable
rule: oldSelf == null || (!has(self.mode) && !has(oldSelf.mode))
|| self.mode == oldSelf.mode
- message: caBundle must be a PEM-encoded certificate
rule: '!has(self.caBundle) || self.caBundle.contains(''-----BEGIN
CERTIFICATE-----'')'
type:
description: Type specifies the type of provider. Can only be
`ControlPlane`.
Expand Down
1 change: 1 addition & 0 deletions docs/en/latest/reference/api-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ ControlPlaneProvider defines configuration for control plane provider.
| `endpoints` _string array_ | Endpoints specifies the list of control plane endpoints. |
| `service` _[ProviderService](#providerservice)_ | |
| `tlsVerify` _boolean_ | TlsVerify specifies whether to verify the TLS certificate of the control plane. |
| `caBundle` _string_ | 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. |
| `auth` _[ControlPlaneAuth](#controlplaneauth)_ | Auth specifies the authentication configuration. |


Expand Down
9 changes: 8 additions & 1 deletion internal/adc/client/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,11 @@
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"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Ship an ADC version that honors caCert. This repository and its Helm chart still use ADC 0.27.1, while api7/adc#537 is open and unreleased; the current sidecar accepts this unknown option but ignores it. The CRD can therefore be accepted while private-CA verification still fails in every normal install. Please merge and release the ADC change, bump the pinned/chart image, and exercise this TLS path end to end before merging this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

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
Expand All @@ -103,6 +107,7 @@
"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(),
}
Expand Down Expand Up @@ -349,7 +354,7 @@
}

// 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) {

Check failure on line 357 in internal/adc/client/executor.go

View workflow job for this annotation

GitHub Actions / lint

(*HTTPADCExecutor).buildHTTPRequest - method always receives http.MethodPut ("PUT") (unparam)
// Prepare request body
tlsVerify := config.TlsVerify
bypassCache := path == pathSync && config.BypassCache
Expand All @@ -362,6 +367,7 @@
LabelSelector: labels,
IncludeResourceType: types,
TlsSkipVerify: ptr.To(!tlsVerify),
CaCert: config.CaBundle,
CacheKey: config.Name,
BypassCache: bypassCache,
},
Expand All @@ -385,6 +391,7 @@
"labelSelector", labels,
"includeResourceType", types,
"tlsSkipVerify", !tlsVerify,
"hasCaCert", config.CaBundle != "",
)

// Create HTTP request
Expand Down
31 changes: 31 additions & 0 deletions internal/adc/client/executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Comment thread
nic-6443 marked this conversation as resolved.

const caCert = "-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----"
opts, raw = build(adctypes.Config{Name: "GatewayProxy/ns/name", TlsVerify: true, CaBundle: caCert})
assert.Equal(t, caCert, opts.CaCert)
assert.Contains(t, raw, "caCert")
// verification stays on, otherwise the bundle would be pointless
assert.Equal(t, false, *opts.TlsSkipVerify)
}

// confVersionError is what a push carrying a conf_version older than the data plane's
// comes back as, once the ADC server has relayed the rejection to us.
func confVersionError() error {
Expand Down
12 changes: 12 additions & 0 deletions internal/adc/translator/gatewayproxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package translator

import (
"crypto/x509"
"fmt"
"net"
"strconv"
Expand Down Expand Up @@ -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")
}
Comment thread
nic-6443 marked this conversation as resolved.
if !cfg.TlsVerify {
t.Log.Info("caBundle is ignored because tlsVerify is disabled", "gatewayproxy", utils.NamespacedNameKind(gatewayProxy))
}
cfg.CaBundle = cp.CaBundle
}

if cp.Auth.Type == v1alpha1.AuthTypeAdminKey && cp.Auth.AdminKey != nil {
if cp.Auth.AdminKey.ValueFrom != nil && cp.Auth.AdminKey.ValueFrom.SecretKeyRef != nil {
secretRef := cp.Auth.AdminKey.ValueFrom.SecretKeyRef
Expand Down
101 changes: 101 additions & 0 deletions internal/adc/translator/gatewayproxy_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// 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)
})

t.Run("rejects a CA bundle that is not PEM encoded", func(t *testing.T) {
tr := &Translator{Log: logr.Discard()}
tctx := provider.NewDefaultTranslateContext(context.Background())

cfg, err := tr.TranslateGatewayProxyToConfig(tctx, newGatewayProxy(ptr.To(true), "not-a-certificate"), false)
require.Error(t, err)
assert.Contains(t, err.Error(), "invalid caBundle")
assert.Nil(t, cfg)
})

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)
})
}
Loading