Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
17 changes: 12 additions & 5 deletions internal/adc/client/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`

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 @@ 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(),
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand All @@ -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,
},
Expand All @@ -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)
}
Expand Down
34 changes: 32 additions & 2 deletions internal/adc/client/executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"encoding/json"
"errors"
"io"
"net/http"
"testing"

"github.com/go-logr/logr"
Expand All @@ -41,7 +40,7 @@ func TestHTTPADCExecutorBuildHTTPRequestBypassCache(t *testing.T) {

build := func(config adctypes.Config, path string) (ADCServerOpts, string) {
req, err := e.buildHTTPRequest(context.Background(), "http://apisix:9180", config, nil, nil,
&adctypes.Resources{}, http.MethodPut, path)
&adctypes.Resources{}, path)
require.NoError(t, err)
body, err := io.ReadAll(req.Body)
require.NoError(t, err)
Expand Down Expand Up @@ -70,6 +69,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{}, 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
38 changes: 38 additions & 0 deletions internal/adc/translator/gatewayproxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
package translator

import (
"crypto/x509"
"encoding/pem"
"fmt"
"net"
"strconv"
Expand Down Expand Up @@ -56,6 +58,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 err := validateCaBundle(cp.CaBundle); err != nil {
return nil, err
}
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 Expand Up @@ -142,3 +155,28 @@ func (t *Translator) TranslateGatewayProxyToConfig(tctx *provider.TranslateConte

return &cfg, nil
}

// validateCaBundle parses every certificate in the bundle. x509.CertPool skips
// blocks it cannot decode, so a bundle whose second certificate is broken would
// otherwise reach the ADC server and fail there instead.
func validateCaBundle(caBundle string) error {
var count int
for rest := []byte(caBundle); len(rest) > 0; {
var block *pem.Block
block, rest = pem.Decode(rest)
if block == nil {
break
}
if block.Type != "CERTIFICATE" {
return fmt.Errorf("invalid caBundle: expected a CERTIFICATE block, got %s", block.Type)
}
if _, err := x509.ParseCertificate(block.Bytes); err != nil {
return fmt.Errorf("invalid caBundle: %w", err)
}
count++
}
if count == 0 {
return errors.New("invalid caBundle: no PEM-encoded certificate found")
}
return nil
}
123 changes: 123 additions & 0 deletions internal/adc/translator/gatewayproxy_test.go
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)
})
}
Loading