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 @@ -806,6 +806,11 @@ type Config struct {
Token string
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
}

// MarshalJSON implements custom JSON marshaling for adcConfig
Expand All @@ -815,10 +820,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"

@coderabbitai coderabbitai Bot Jul 27, 2026

Copy link
Copy Markdown

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:

#!/bin/bash
set -euo pipefail

printf '\n== api/v1alpha1/gatewayproxy_types.go ==\n'
sed -n '1,220p' api/v1alpha1/gatewayproxy_types.go

printf '\n== config/crd/bases/apisix.apache.org_gatewayproxies.yaml ==\n'
sed -n '150,190p' config/crd/bases/apisix.apache.org_gatewayproxies.yaml

printf '\n== internal/adc/translator/gatewayproxy.go ==\n'
sed -n '1,120p' internal/adc/translator/gatewayproxy.go

Repository: api7/api7-ingress-controller

Length of output: 13909


🏁 Script executed:

#!/bin/bash
set -euo pipefail

cat > /tmp/pemtst.go <<'GO'
package main

import (
	"crypto/x509"
	"encoding/pem"
	"fmt"
)

func main() {
	// One minimal self-signed-ish CERTIFICATE block is not required for parser behavior;
	// any valid certificate PEM data would do. Use a real-looking cert block from docs? No,
	// just inspect parser behavior with canned inputs and report append result.
	tests := map[string]string{
		"valid_cert_only": `-----BEGIN CERTIFICATE-----
MIIBszCCAVmgAwIBAgIUH9hWQJ4zJr9fQfFQj3c1e1nXqk0wCgYIKoZIzj0EAwIw
EjEQMA4GA1UEAwwHZHVtbXkwHhcNMjQwMTAxMDAwMDAwWhcNMzQwMTAxMDAwMDAw
WjASMRAwDgYDVQQDDAdkdW1teTAwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQw
8H2Xq3dKx8VnNf8nJf6m8j2W2VQ0Wz1k2k1wP+T6R3T+8bGQG2bP1F0Q4p9j5h7W
qV6p3o+QvG9j8P2jUzBRMB0GA1UdDgQWBBQAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
MB8GA1UdIwQYMBaAFAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAwDwYDVR0TAQH/BAUw
AwEB/zAKBggqhkjOPQQDAgNJADBGAiEA0Q==
-----END CERTIFICATE-----
`,
		"valid_cert_plus_garbage": `-----BEGIN CERTIFICATE-----
MIIBszCCAVmgAwIBAgIUH9hWQJ4zJr9fQfFQj3c1e1nXqk0wCgYIKoZIzj0EAwIw
EjEQMA4GA1UEAwwHZHVtbXkwHhcNMjQwMTAxMDAwMDAwWhcNMzQwMTAxMDAwMDAw
WjASMRAwDgYDVQQDDAdkdW1teTAwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQw
8H2Xq3dKx8VnNf8nJf6m8j2W2VQ0Wz1k2k1wP+T6R3T+8bGQG2bP1F0Q4p9j5h7W
qV6p3o+QvG9j8P2jUzBRMB0GA1UdDgQWBBQAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
MB8GA1UdIwQYMBaAFAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAwDwYDVR0TAQH/BAUw
AwEB/zAKBggqhkjOPQQDAgNJADBGAiEA0Q==
-----END CERTIFICATE-----
garbage`,
		"garbage_then_valid_cert": `garbage
-----BEGIN CERTIFICATE-----
MIIBszCCAVmgAwIBAgIUH9hWQJ4zJr9fQfFQj3c1e1nXqk0wCgYIKoZIzj0EAwIw
EjEQMA4GA1UEAwwHZHVtbXkwHhcNMjQwMTAxMDAwMDAwWhcNMzQwMTAxMDAwMDAw
WjASMRAwDgYDVQQDDAdkdW1teTAwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQw
8H2Xq3dKx8VnNf8nJf6m8j2W2VQ0Wz1k2k1wP+T6R3T+8bGQG2bP1F0Q4p9j5h7W
qV6p3o+QvG9j8P2jUzBRMB0GA1UdDgQWBBQAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
MB8GA1UdIwQYMBaAFAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAwDwYDVR0TAQH/BAUw
AwEB/zAKBggqhkjOPQQDAgNJADBGAiEA0Q==
-----END CERTIFICATE-----
`,
		"header_only": `-----BEGIN CERTIFICATE-----`,
		"non_cert_pem": `-----BEGIN FOO-----
abcd
-----END FOO-----`,
	}

	for name, s := range tests {
		pool := x509.NewCertPool()
		ok := pool.AppendCertsFromPEM([]byte(s))
		fmt.Printf("%s: ok=%v subjects=%d firstBlock=%q\n", name, ok, len(pool.Subjects()), firstBlockType(s))
	}
}

func firstBlockType(s string) string {
	b, _ := pem.Decode([]byte(s))
	if b == nil {
		return "nil"
	}
	return b.Type
}
GO

go run /tmp/pemtst.go

Repository: api7/api7-ingress-controller

Length of output: 469


🏁 Script executed:

#!/bin/bash
set -euo pipefail
cat /tmp/coderabbit-shell-logs/shell-output-*.log | tail -n +1 >/dev/null

Repository: api7/api7-ingress-controller

Length of output: 246


🏁 Script executed:

#!/bin/bash
set -euo pipefail

bundle=""
for f in /etc/ssl/certs/ca-certificates.crt /etc/ssl/cert.pem /etc/pki/tls/certs/ca-bundle.crt; do
  if [ -f "$f" ]; then bundle="$f"; break; fi
done
if [ -z "$bundle" ]; then
  echo "no system CA bundle found" >&2
  exit 1
fi

cert=$(awk '
  BEGIN{p=0}
  /BEGIN CERTIFICATE/ {p=1}
  p {print}
  /END CERTIFICATE/ {exit}
' "$bundle")

cat > /tmp/pemtst.go <<'GO'
package main

import (
	"crypto/x509"
	"fmt"
)

func check(name, s string) {
	pool := x509.NewCertPool()
	ok := pool.AppendCertsFromPEM([]byte(s))
	fmt.Printf("%s: ok=%v subjects=%d\n", name, ok, len(pool.Subjects()))
}

func main() {
	valid := CERTDATA

	check("valid_only", valid)
	check("valid_plus_garbage", valid+"\nGARBAGE\n")
	check("valid_plus_noncert_pem", valid+"\n-----BEGIN FOO-----\na\n-----END FOO-----\n")
	check("garbage_plus_valid", "GARBAGE\n"+valid)
	check("two_valid", valid+"\n"+valid)
}
GO

python3 - <<'PY'
from pathlib import Path
p = Path('/tmp/pemtst.go')
s = p.read_text()
cert = Path('/tmp/cert.pem').write_text("""__CERT__""")
PY

Repository: api7/api7-ingress-controller

Length of output: 166


🏁 Script executed:

#!/bin/bash
set -euo pipefail

bundle=""
for f in /etc/ssl/certs/ca-certificates.crt /etc/ssl/cert.pem /etc/pki/tls/certs/ca-bundle.crt; do
  if [ -f "$f" ]; then bundle="$f"; break; fi
done
if [ -z "$bundle" ]; then
  echo "no system CA bundle found" >&2
  exit 1
fi

cert_file=/tmp/one_cert.pem
awk '
  BEGIN{p=0}
  /BEGIN CERTIFICATE/ {p=1}
  p {print}
  /END CERTIFICATE/ {exit}
' "$bundle" > "$cert_file"

python3 - <<'PY'
from pathlib import Path
cert = Path('/tmp/one_cert.pem').read_text()
go = f'''package main

import (
	"crypto/x509"
	"fmt"
)

func check(name, s string) {{
	pool := x509.NewCertPool()
	ok := pool.AppendCertsFromPEM([]byte(s))
	fmt.Printf("%s: ok=%v subjects=%d\\n", name, ok, len(pool.Subjects()))
}}

func main() {{
	valid := {cert!r}
	check("valid_only", valid)
	check("valid_plus_garbage", valid+"\\nGARBAGE\\n")
	check("valid_plus_noncert_pem", valid+"\\n-----BEGIN FOO-----\\na\\n-----END FOO-----\\n")
	check("garbage_plus_valid", "GARBAGE\\n"+valid)
	check("two_valid", valid+"\\n"+valid)
}}
'''
Path('/tmp/pemtst.go').write_text(go)
PY

go run /tmp/pemtst.go

Repository: api7/api7-ingress-controller

Length of output: 253


🏁 Script executed:

#!/bin/bash
set -euo pipefail

bundle=""
for f in /etc/ssl/certs/ca-certificates.crt /etc/ssl/cert.pem /etc/pki/tls/certs/ca-bundle.crt; do
  if [ -f "$f" ]; then bundle="$f"; break; fi
done
if [ -z "$bundle" ]; then
  echo "no system CA bundle found" >&2
  exit 1
fi

awk '
  BEGIN{p=0}
  /BEGIN CERTIFICATE/ {p=1}
  p {print}
  /END CERTIFICATE/ {exit}
' "$bundle" > /tmp/one_cert.pem

python3 - <<'PY'
from pathlib import Path
cert = Path('/tmp/one_cert.pem').read_text()
go = f'''package main

import (
	"crypto/x509"
	"fmt"
)

func check(name, s string) {{
	pool := x509.NewCertPool()
	ok := pool.AppendCertsFromPEM([]byte(s))
	fmt.Printf("%s: ok=%v subjects=%d\\n", name, ok, len(pool.Subjects()))
}}

func main() {{
	valid := {cert!r}
	check("valid_only", valid)
	check("valid_plus_garbage", valid+"\\nGARBAGE\\n")
	check("valid_plus_noncert_pem", valid+"\\n-----BEGIN FOO-----\\na\\n-----END FOO-----\\n")
	check("garbage_plus_valid", "GARBAGE\\n"+valid)
	check("two_valid", valid+"\\n"+valid)
}}
'''
Path('/tmp/pemtst.go').write_text(go)
PY

go run /tmp/pemtst.go

Repository: api7/api7-ingress-controller

Length of output: 253


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '== caBundle references ==\n'
rg -n "caBundle|CaBundle" api internal config test . -g '!**/vendor/**'

printf '\n== relevant tests near gatewayproxy ==\n'
fd -a 'gatewayproxy' api internal test config | sed -n '1,120p'

Repository: api7/api7-ingress-controller

Length of output: 8992


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find tests or docs around gateway proxy control-plane validation.
rg -n "caBundle|AppendCertsFromPEM|ControlPlaneProvider|GatewayProxyProvider" \
  api internal test config docs . \
  -g '!**/vendor/**' -g '!**/dist/**' -g '!**/build/**'

Repository: api7/api7-ingress-controller

Length of output: 12292


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Probe AppendCertsFromPEM with a known real certificate and trailing garbage.
bundle=""
for f in /etc/ssl/certs/ca-certificates.crt /etc/ssl/cert.pem /etc/pki/tls/certs/ca-bundle.crt; do
  if [ -f "$f" ]; then bundle="$f"; break; fi
done
if [ -z "$bundle" ]; then
  echo "no system CA bundle found" >&2
  exit 1
fi

cert_file=/tmp/one_cert.pem
awk '
  BEGIN{p=0}
  /BEGIN CERTIFICATE/ {p=1}
  p {print}
  /END CERTIFICATE/ {exit}
' "$bundle" > "$cert_file"

python3 - <<'PY'
from pathlib import Path
cert = Path('/tmp/one_cert.pem').read_text()
go = f'''package main

import (
	"crypto/x509"
	"fmt"
)

func check(name, s string) {{
	pool := x509.NewCertPool()
	ok := pool.AppendCertsFromPEM([]byte(s))
	fmt.Printf("%s: ok=%v subjects=%d\\n", name, ok, len(pool.Subjects()))
}}

func main() {{
	valid := {cert!r}
	check("valid_only", valid)
	check("valid_plus_garbage", valid+"\\nGARBAGE\\n")
	check("garbage_plus_valid", "GARBAGE\\n"+valid)
	check("valid_plus_noncert_pem", valid+"\\n-----BEGIN FOO-----\\na\\n-----END FOO-----\\n")
}}
'''
Path('/tmp/pemtst.go').write_text(go)
PY

go run /tmp/pemtst.go

Repository: api7/api7-ingress-controller

Length of output: 253


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '== internal/adc/translator/gatewayproxy_test.go ==\n'
sed -n '1,180p' internal/adc/translator/gatewayproxy_test.go

printf '\n== internal/webhook/v1/gatewayproxy_webhook_test.go ==\n'
sed -n '1,180p' internal/webhook/v1/gatewayproxy_webhook_test.go

Repository: api7/api7-ingress-controller

Length of output: 9191


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Better probe using a real PEM block and JSON-escaped Go string.
bundle=""
for f in /etc/ssl/certs/ca-certificates.crt /etc/ssl/cert.pem /etc/pki/tls/certs/ca-bundle.crt; do
  if [ -f "$f" ]; then bundle="$f"; break; fi
done
if [ -z "$bundle" ]; then
  echo "no system CA bundle found" >&2
  exit 1
fi

awk '
  BEGIN{p=0}
  /BEGIN CERTIFICATE/ {p=1}
  p {print}
  /END CERTIFICATE/ {exit}
' "$bundle" > /tmp/one_cert.pem

python3 - <<'PY'
from pathlib import Path
import json
cert = Path('/tmp/one_cert.pem').read_text()
go = f'''package main

import (
	"crypto/x509"
	"fmt"
)

func check(name, s string) {{
	pool := x509.NewCertPool()
	ok := pool.AppendCertsFromPEM([]byte(s))
	fmt.Printf("%s: ok=%v subjects=%d\\n", name, ok, len(pool.Subjects()))
}}

func main() {{
	valid := {json.dumps(cert)}
	check("valid_only", valid)
	check("valid_plus_garbage", valid+"\\nGARBAGE\\n")
	check("garbage_plus_valid", "GARBAGE\\n"+valid)
	check("valid_plus_noncert_pem", valid+"\\n-----BEGIN FOO-----\\na\\n-----END FOO-----\\n")
}}
'''
Path('/tmp/pemtst.go').write_text(go)
PY

go run /tmp/pemtst.go

Repository: api7/api7-ingress-controller

Length of output: 318


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Focus the docs/tests search on gatewayproxy validation behavior.
rg -n "invalid caBundle|caBundle.*PEM|AppendCertsFromPEM|tlsVerify is disabled|GatewayProxyToConfigCaBundle" \
  internal/adc/translator/gatewayproxy_test.go \
  internal/webhook/v1/gatewayproxy_webhook_test.go \
  internal/webhook/v1/adc_validation_test.go \
  docs/en/latest/reference/api-reference.md

Repository: api7/api7-ingress-controller

Length of output: 710


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '== internal/webhook/v1/gatewayproxy_webhook.go ==\n'
sed -n '1,220p' internal/webhook/v1/gatewayproxy_webhook.go

printf '\n== internal/webhook/v1/adc_validation_test.go ==\n'
sed -n '1,180p' internal/webhook/v1/adc_validation_test.go

printf '\n== internal/webhook/v1/gatewayproxy_webhook_test.go ==\n'
sed -n '1,180p' internal/webhook/v1/gatewayproxy_webhook_test.go

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 caBundle validation. The admission rule only checks for a CERTIFICATE header, and AppendCertsFromPEM accepts 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-L170
  • internal/adc/translator/gatewayproxy.go#L60-L64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v1alpha1/gatewayproxy_types.go` at line 123, The caBundle validation
currently accepts certificate-looking content without enforcing a strictly valid
PEM certificate bundle. Update the validation logic around the GatewayProxy
caBundle handling in internal/adc/translator/gatewayproxy.go:60-64 to parse
every PEM block, reject trailing garbage and non-certificate blocks, and accept
only valid certificates; then update the caBundle XValidation annotation in
api/v1alpha1/gatewayproxy_types.go:123 and regenerate the corresponding CRD
validation in config/crd/bases/apisix.apache.org_gatewayproxies.yaml:168-170.

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.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== API declaration =='
sed -n '115,150p' api/v1alpha1/gatewayproxy_types.go

printf '%s\n' '== Generated CRD caBundle schema =='
rg -n -A12 -B8 'caBundle|caBundle must be a PEM-encoded certificate' \
  config/crd/bases/apisix.apache.org_gatewayproxies.yaml \
  config/crd-nocel/apisix.apache.org_v2.yaml

printf '%s\n' '== GatewayProxy admission tests and helpers =='
ast-grep outline internal/webhook/v1/gatewayproxy_webhook_test.go --items all
rg -n -A12 -B8 'GatewayProxy|ControlPlaneProvider|caBundle|admission|validate' \
  internal/webhook/v1/gatewayproxy_webhook_test.go \
  internal/webhook/v1/gatewayproxy_webhook.go

Length of output: 51590


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== Admission and CRD validation test harnesses =='
rg -n -i -A8 -B5 \
  'envtest|kube-apiserver|CustomResourceDefinition|x-kubernetes-validations|CEL|admission.*valid|validat.*admission|InstallCRDs|CRDDirectoryPaths' \
  --glob '*_test.go' \
  --glob '!**/vendor/**' \
  .

printf '%s\n' '== GatewayProxy test files =='
fd -a -t f 'gatewayproxy.*(_test)?\.go|.*gatewayproxy.*\.ya?ml' . | sort

Length of output: 13887


@jarvis9443, confirmed. The current CEL rule accepts a truncated caBundle because it checks only the BEGIN marker.

  • Require both PEM boundary markers in the ControlPlaneProvider CEL rule.
  • Regenerate the CEL-enabled CRD from the kubebuilder annotation.
  • Add a CRD admission-validation test for a caBundle with only -----BEGIN CERTIFICATE-----.
  • Keep strict per-block PEM and X.509 parsing in the translator. CEL cannot perform that parsing.

  • 📌 Create a pull request with these changes

You are interacting with an AI system.

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
Contributor

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. api7/api7-helm-chart still has no caBundle property in charts/ingress-controller/crds/apisix-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
7 changes: 7 additions & 0 deletions config/crd-nocel/apisix.apache.org_v2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2273,6 +2273,13 @@ spec:
required:
- type
type: object
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
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 @@ -79,7 +79,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
Contributor

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 still pins ADC 0.27.1 and the API7 Helm chart pins 0.26.0, while api7/adc#537 is open and unreleased; both released sidecars accept this unknown option but ignore 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.

CacheKey string `json:"cacheKey"`
}

// MarshalLog implements logr.Marshaler so logging the request body redacts the
Expand All @@ -93,6 +97,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 @@ -351,6 +356,7 @@ func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr strin
LabelSelector: labels,
IncludeResourceType: types,
TlsSkipVerify: ptr.To(!tlsVerify),
CaCert: config.CaBundle,
CacheKey: config.Name,
},
Config: *resources,
Expand All @@ -372,6 +378,7 @@ func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr strin
"labelSelector", labels,
"includeResourceType", types,
"tlsSkipVerify", !tlsVerify,
"hasCaCert", config.CaBundle != "",
)

// Create HTTP request
Expand Down
63 changes: 63 additions & 0 deletions internal/adc/client/executor_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// 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 client

import (
"context"
"encoding/json"
"io"
"net/http"
"testing"

"github.com/go-logr/logr"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

adctypes "github.com/apache/apisix-ingress-controller/api/adc"
)

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, "/sync")
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")

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