Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ require (
golang.org/x/net v0.38.0
google.golang.org/genproto/googleapis/api v0.0.0-20250218202821-56aae31c358a
google.golang.org/grpc v1.71.0
google.golang.org/grpc/security/advancedtls v1.0.0
google.golang.org/protobuf v1.36.6
gopkg.in/yaml.v3 v3.0.1
helm.sh/helm/v3 v3.17.2
Expand Down
4 changes: 4 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1670,6 +1670,10 @@ google.golang.org/grpc v1.71.0 h1:kF77BGdPTQ4/JZWMlb9VpJ5pa25aqvVqogsxNHHdeBg=
google.golang.org/grpc v1.71.0/go.mod h1:H0GRtasmQOh9LkFoCPDu3ZrwUtD1YGE+b2vYBYd/8Ec=
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.5.1 h1:F29+wU6Ee6qgu9TddPgooOdaqsxTMunOoj8KA5yuS5A=
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.5.1/go.mod h1:5KF+wpkbTSbGcR9zteSqZV6fqFOWBl4Yde8En8MryZA=
google.golang.org/grpc/examples v0.0.0-20201112215255-90f1b3ee835b h1:NuxyvVZoDfHZwYW9LD4GJiF5/nhiSyP4/InTrvw9Ibk=
google.golang.org/grpc/examples v0.0.0-20201112215255-90f1b3ee835b/go.mod h1:IBqQ7wSUJ2Ep09a8rMWFsg4fmI2r38zwsq8a0GgxXpM=
google.golang.org/grpc/security/advancedtls v1.0.0 h1:/KQ7VP/1bs53/aopk9QhuPyFAp9Dm9Ejix3lzYkCrDA=
google.golang.org/grpc/security/advancedtls v1.0.0/go.mod h1:o+s4go+e1PJ2AjuQMY5hU82W7lDlefjJA6FqEHRVHWk=
google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=
google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQJ+fmap5saPgwCLgHXTUD7jkjRqWcaiX5VyM=
Expand Down
65 changes: 44 additions & 21 deletions internal/extension/registry/extension_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/security/advancedtls"
"google.golang.org/grpc/test/bufconn"
corev1 "k8s.io/api/core/v1"
k8scli "sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -245,18 +246,6 @@ func (m *Manager) CleanupHookConns() {
}
}

func parseCA(caSecret *corev1.Secret) (*x509.CertPool, error) {
caCertPEMBytes, ok := caSecret.Data[corev1.TLSCertKey]
if !ok {
return nil, errors.New("no cert found in CA secret")
}
cp := x509.NewCertPool()
if ok := cp.AppendCertsFromPEM(caCertPEMBytes); !ok {
return nil, errors.New("failed to append certificates")
}
return cp, nil
}

func setupGRPCOpts(ctx context.Context, client k8scli.Client, ext *egv1a1.ExtensionManager, namespace string) ([]grpc.DialOption, error) {
// These two errors shouldn't happen since we check these conditions when loading the extension
if ext == nil {
Expand All @@ -267,20 +256,16 @@ func setupGRPCOpts(ctx context.Context, client k8scli.Client, ext *egv1a1.Extens
}

var opts []grpc.DialOption
var creds credentials.TransportCredentials
if ext.Service.TLS != nil {
certRef := ext.Service.TLS.CertificateRef
secret, secretNamespace, err := kubernetes.ValidateSecretObjectReference(ctx, client, &certRef, namespace)
// Sanity check to ensure that the extension manager has a valid certificate reference
_, err := getCertPoolFromSecret(ctx, client, ext, namespace)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to get root CA certificates: %w", err)
}

cp, err := parseCA(secret)
creds, err := getGRPCCredentials(ctx, client, ext, namespace)
if err != nil {
return nil, fmt.Errorf("error parsing cert in Secret %s in namespace %s", string(certRef.Name), secretNamespace)
return nil, fmt.Errorf("failed to get gRPC TLS credentials: %w", err)
}

creds = credentials.NewClientTLSFromCert(cp, "")
opts = append(opts, grpc.WithTransportCredentials(creds))
} else {
opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials()))
Expand All @@ -300,3 +285,41 @@ func setupGRPCOpts(ctx context.Context, client k8scli.Client, ext *egv1a1.Extens

return opts, nil
}

func getGRPCCredentials(ctx context.Context, client k8scli.Client, ext *egv1a1.ExtensionManager, namespace string) (credentials.TransportCredentials, error) {
return advancedtls.NewClientCreds(&advancedtls.Options{
RootOptions: advancedtls.RootCertificateOptions{
// A callback function that dynamically loads root CA certificates from secret
GetRootCertificates: createGetRootCertificatesHandler(ctx, client, ext, namespace),
},
})
}

func createGetRootCertificatesHandler(ctx context.Context, client k8scli.Client, ext *egv1a1.ExtensionManager, namespace string) func(*advancedtls.ConnectionInfo) (*advancedtls.RootCertificates, error) {
return func(params *advancedtls.ConnectionInfo) (*advancedtls.RootCertificates, error) {
cp, err := getCertPoolFromSecret(ctx, client, ext, namespace)

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 function will run in the background in a goroutine inside advancedtls, correct?

If so, I suggest you don't pass in the parent context to this function, as someone may change the parent context to cancel or timeout in the future.

Instead, create a new context in this function ctx := context.Background()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

if err != nil {
return nil, err
}

return &advancedtls.RootCertificates{TrustCerts: cp}, nil
}
}

func getCertPoolFromSecret(ctx context.Context, client k8scli.Client, ext *egv1a1.ExtensionManager, namespace string) (*x509.CertPool, error) {
certRef := ext.Service.TLS.CertificateRef
secret, _, err := kubernetes.ValidateSecretObjectReference(ctx, client, &certRef, namespace)
if err != nil {
return nil, fmt.Errorf("failed to validate TLS certificate reference: %w", err)
}

caCertPEMBytes, ok := secret.Data[corev1.TLSCertKey]
if !ok {
return nil, errors.New("no TLS certificate found in CA secret")

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.

Suggested change
return nil, errors.New("no TLS certificate found in CA secret")
return nil, fmt.Errorf("no CA certificate found in Kubernetes Secret %s in namespace %s", secret.GetName(), secret.GetNamespace())
  1. This is parsing a CA certificate, not TLS certificate. The pre-existing naming of key corev1.TLSCertKey is incorrect unfortunately, but we need to support it for backwards compatibility.
  2. Let's include secret name and namespace for debuggability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I fixed the error message

}
cp := x509.NewCertPool()
if ok := cp.AppendCertsFromPEM(caCertPEMBytes); !ok {
return nil, errors.New("failed to append certificates from CA secret")
}
return cp, nil
}
110 changes: 110 additions & 0 deletions internal/extension/registry/extension_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,28 @@ package registry

import (
"context"
"crypto/tls"
"crypto/x509"
"fmt"
"math"
"net"
"os"
"testing"

"github.com/stretchr/testify/require"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"

egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
"github.com/envoyproxy/gateway/internal/envoygateway"
"github.com/envoyproxy/gateway/proto/extension"
v3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
)

func TestGetExtensionServerAddress(t *testing.T) {
Expand Down Expand Up @@ -150,3 +160,103 @@ func Test_setupGRPCOpts(t *testing.T) {
})
}
}

type testServer struct {
extension.UnimplementedEnvoyGatewayExtensionServer
}

func (s *testServer) PostRouteModify(ctx context.Context, req *extension.PostRouteModifyRequest) (*extension.PostRouteModifyResponse, error) {
return &extension.PostRouteModifyResponse{
Route: req.Route,
}, nil
}

func Test_TLS(t *testing.T) {
testDir := "testdata"
caFile := testDir + "/ca.pem"
certFile := testDir + "/cert.pem"
keyFile := testDir + "/key.pem"

cert, err := tls.LoadX509KeyPair(certFile, keyFile)
require.NoError(t, err)

caCert, err := os.ReadFile(caFile)
require.NoError(t, err)
caPool := x509.NewCertPool()
ok := caPool.AppendCertsFromPEM(caCert)
require.True(t, ok)

lis, err := net.Listen("tcp", "localhost:0")
require.NoError(t, err)
defer lis.Close()

port := lis.Addr().(*net.TCPAddr).Port
server := grpc.NewServer(grpc.Creds(credentials.NewTLS(&tls.Config{
Certificates: []tls.Certificate{cert},
ClientAuth: tls.NoClientCert,
})))
extension.RegisterEnvoyGatewayExtensionServer(server, &testServer{})
go func() {
_ = server.Serve(lis)
defer server.GracefulStop()
}()

extManager := &egv1a1.ExtensionManager{
Service: &egv1a1.ExtensionService{
BackendEndpoint: egv1a1.BackendEndpoint{
IP: &egv1a1.IPEndpoint{
Address: "localhost",
Port: int32(port),
},
},
TLS: &egv1a1.ExtensionTLS{
CertificateRef: gwapiv1.SecretObjectReference{
Name: "cert",
Namespace: ptr.To(gwapiv1.Namespace("default")),
},
},
},
}

certData, err := os.ReadFile(certFile)
require.NoError(t, err)
keyData, err := os.ReadFile(keyFile)
require.NoError(t, err)

secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "cert",
Namespace: "default",
},
Type: corev1.SecretTypeTLS,
Data: map[string][]byte{
corev1.TLSCertKey: certData,

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.

Unfortunately this doesn't seem right. extension_manager.go line 316 is reading CA cert from corev1.TLSCertKey (yes it is incorrectly named, but that is a legacy behavior we need to support).

If so, I'm not entirely sure how this test is working (?) Because the CA cert is not in the correct place.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I checked the test, and it worked because both the CA and the server certificate were accepted — see: https://stackoverflow.com/questions/77084841/server-certificate-used-as-ca-cert-why-does-it-work/77084918#77084918

I've now updated the test to use a self-signed certificate, so the certificate acts as both the server cert and the CA.

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.

Wow, did not know that. Thanks for the explanation!

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.

I recommend modifying the test to only pass in the exact necessary data. This makes it clearer to readers that the current naming is incorrect.

Specifically, change the secret to just this, the test should still pass:

Data: map[string][]byte{
  corev1.TLSCertKey:       caCert,
},

Otherwise the PR looks good.

@sapirpol sapirpol Apr 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I updated the secret to include only the necessary data

corev1.TLSPrivateKeyKey: keyData,
"ca.crt": caCert,
},
}

fakeClient := fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).WithObjects(secret).Build()

opts, err := setupGRPCOpts(context.Background(), fakeClient, extManager, "test-ns")
require.NoError(t, err)
require.NotEmpty(t, opts)

conn, err := grpc.DialContext(context.Background(), fmt.Sprintf("localhost:%d", port),
opts...,
)
require.NoError(t, err)
defer conn.Close()

client := extension.NewEnvoyGatewayExtensionClient(conn)
require.NotNil(t, client)

response, err := client.PostRouteModify(context.Background(), &extension.PostRouteModifyRequest{
Route: &v3.Route{
Name: "test-route",
},
})
require.NoError(t, err)
require.Equal(t, response.Route.Name, "test-route")

}
19 changes: 19 additions & 0 deletions internal/extension/registry/testdata/ca.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-----BEGIN CERTIFICATE-----
MIIDBzCCAe+gAwIBAgIUKnUpaE+Gcg0BGZrwDv5qQyJfLDwwDQYJKoZIhvcNAQEL
BQAwEjEQMA4GA1UEAwwHVGVzdCBDQTAgFw0yNTA0MDEwNjU1MTdaGA8yMTI1MDMw
ODA2NTUxN1owEjEQMA4GA1UEAwwHVGVzdCBDQTCCASIwDQYJKoZIhvcNAQEBBQAD
ggEPADCCAQoCggEBALQ4wfwp+HKnQNHIs6VsazmL1XqB17S1PgIJOvg/wnPcKAPm
2TkdsUMaP1N7hkJSxS/LoylSAuu7FEdFe6dnzsRaA845+G/NRNkUevZMaZc5p2ii
2tGyh7vB+J9jj7sV8/rjcNiR1LCAaPR3XeE8tzQ18BoI2TbTLAO6O4STBO9cRblR
0nvPhaUzr/ulf0CARzL2uNlePgwNWb+SgYX6WtfvH0LsKjYlsBqAkuamkWiXcyYl
NQf1aMaDm04Hx0XmElPdZMylqoMQZiI5mmua5xy/rJ1EIQdjpIzBWtK18cTn8fcN
DjKHd20HPp+58V4+2ey8hn+uI1tU6JKXTO29ERcCAwEAAaNTMFEwHQYDVR0OBBYE
FCMo17hRba5WoIWd6KFJAs3RMW3CMB8GA1UdIwQYMBaAFCMo17hRba5WoIWd6KFJ
As3RMW3CMA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBAHwASP0v
mxI1T/WTfTocrWYEXxxZdt3FXwtnxWK2uYSz8+4TI3z0oIJ+IqAJC/ie1KvIhQqF
+R3aOqrsEzy+JB4620zJrba4cSQaBL/DDZVokTVoGHQX8jh437rMUqYwF/6vV6fR
KdeIfzGoNL8je11qjYN+jFrUGXB5qoybYxkVTUHAUdTrVs3jjRwuWhGoEEveCqbd
NxmoHAA1JBg8b089omRQUXqdJeyxdc6odVwRBt8dKNz9w5pVBO6nhiUdUXVSxyoj
vPDBzXi0GB45bnkwY4IGn9cEwUjDR0wYvxgG+99f8ewBHxKr2umDhqDhc1QbINV4
aiWmSFOPyrcbBFE=
-----END CERTIFICATE-----
20 changes: 20 additions & 0 deletions internal/extension/registry/testdata/cert.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-----BEGIN CERTIFICATE-----
MIIDMDCCAhigAwIBAgIUGvKQgfPkihl3s/f1p9JDD1L8pkowDQYJKoZIhvcNAQEL
BQAwEjEQMA4GA1UEAwwHVGVzdCBDQTAgFw0yNTA0MDEwNjU4MzdaGA8yMTI1MDMw
ODA2NTgzN1owFDESMBAGA1UEAwwJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEF
AAOCAQ8AMIIBCgKCAQEAzpjyBfMdUOf5Dosvc2WW/CEhHMVWhMi6+Q1BB0b3d1h9
MZYruB99Uq0svC/GfOeCV+ear7yDE/DBISDIpPgsX6qSIEL6cX3P3lUL5kGX0ywb
im6aCC+3/GgKXGsCztRBe76M5DPAi0Oe4yv9x8u5L6nRdZfDiW+WkqMeGqnSArAy
PyIwbL7RIZ4nN1tcfV3ulo7+Bk3fGvi/5Ixoeo3+XF/yFyFGtTbLy7Pi5ITcO4ql
Cst8crZtVU1k1UqykIO1lE/Tu8upuiqK34vMd8PUBZo3kX6pYf8ahJTomUL7b1YI
gkC4VuJur9CoxMYAg9Hd8CJSINZM9HzDLrRItuuZDwIDAQABo3oweDALBgNVHQ8E
BAMCBaAwEwYDVR0lBAwwCgYIKwYBBQUHAwEwFAYDVR0RBA0wC4IJbG9jYWxob3N0
MB0GA1UdDgQWBBQAlp2UN7oJi8fhv6k6s3gMNQdt/DAfBgNVHSMEGDAWgBQjKNe4
UW2uVqCFneihSQLN0TFtwjANBgkqhkiG9w0BAQsFAAOCAQEAD16EQ+ALm5PrXJKr
RBwrMwSQ1aMr2TA8Ub250vzLhFOxrQXRSNruD6Hikhe4zM1Dg6658O+jJTMVvOSi
GofxOpawJdVQJh7vAqn5dTGpLBS839lWyBTbLoBZRHNBmjVcYaVhHRruU2c8DDeF
zsDO1NGQ1CIltIrtGZb7A2CZsuazaEe1/129o8pstuGokRnhfJ1f5H6OFa7+CHET
xpoqRPV399i+JW26JS0uYAKGm3hxBt1jUc84AuT9NZqLsbprZ3S6aHePkVqZNF2c
rhk+cLuElWM+rYwcsic7zkjq1yBLUHM1lM6pCDqGOtuODKPq333E1N7ax7LaGpeS
HW8apQ==
-----END CERTIFICATE-----
28 changes: 28 additions & 0 deletions internal/extension/registry/testdata/key.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----
MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQDOmPIF8x1Q5/kO
iy9zZZb8ISEcxVaEyLr5DUEHRvd3WH0xliu4H31SrSy8L8Z854JX55qvvIMT8MEh
IMik+CxfqpIgQvpxfc/eVQvmQZfTLBuKbpoIL7f8aApcawLO1EF7vozkM8CLQ57j
K/3Hy7kvqdF1l8OJb5aSox4aqdICsDI/IjBsvtEhnic3W1x9Xe6Wjv4GTd8a+L/k
jGh6jf5cX/IXIUa1NsvLs+LkhNw7iqUKy3xytm1VTWTVSrKQg7WUT9O7y6m6Korf
i8x3w9QFmjeRfqlh/xqElOiZQvtvVgiCQLhW4m6v0KjExgCD0d3wIlIg1kz0fMMu
tEi265kPAgMBAAECggEAA8dxPE6gJw5Q1F7gNVPF/6kTemIM6eZ9SET6/yKuY1tJ
BV+Vcr7D6hk+cj3/FK9sOo0DNVrunHsqhtKbGPlsX/hplKsYLa9H3odFVA1RggyS
f5iNh/M0TsR21aeKqGhIvymGWXF6gJGlSTG3m+OY6Mt9O16iHKETHgeOnp1wnzMi
R959RiHlM+f3OE5i/UttTVXY91fRO/hK+LEUa7RU41Wvuw7AUmkmjvj8c3WQOKBd
4Xc6Ada8Q4tvfV1H/urmGUBR6p6qcZHuJ7jMm7ZKEtKKpRQvwchhNONBc0B/8Sr2
YrMhDQqUhMc5FOvYDPhto9KNe0/MtR+wqVYW6REscQKBgQDnD0tOWl7nLqiKETTM
gYKeUO10yeEt2gYjQMnSegU/9taTfEO5ClgWrnct5VOTQNraYHCwOGCxHPFcqh/c
g7qJVdToMkAvfIRnaSOy1dQ6G0Fgjzf2iw4fpcZqCO1xGUsIB27IJvxts5cOqZdF
oR2hLa8Nuy/+WEf7eKwI/r+aawKBgQDk5bPJw/eA10pddDjSVjnqYB8fZ3qpyEQ9
9M/CVnnBHamEB8HBAyJ+a34XDEIuSb6L1UEzA+93ka+xNl0HNWzJ+v7QQkREdkmE
ko/Vs20Kr9/qTyV2imB/FYv3aIoo298xJ7nRZmOT9QUDkUoyuZnMS4GYZIWWa8Y1
bQJ5IU3s7QKBgFBL4Vi2URq3/TwV5KpZK4JHD24xpf5gMRfZMQni+6YR6tnQKlzI
unoPYT1i35thw2x2bVLgFMIYE8ynFnF0mcOWP2n95I5cEEs7n3tLkfgrdpnOy5zz
EBJxcrVwqstOBeQhaR9HZRveeFVRHE/HQNu9W4NDFNen6EP82JQ7q2xHAoGAARRI
Lbpbz2K0eg5TOkKn0UAUxXwiauyDzdr6o8rulgeCxhmXQ0a1ge0V0hv/r+IvIM+n
mAr7tQ+dyXMdKyyIT8d8LhUx5zS7kgFy5ioLaJJ9tsgawB1U6SR73XXDuP+YZM3w
JVmbPJZaKLMlq2mOnRT6DnLi4lutupDCAzox+N0CgYBNtbkyTnXtU0yIY/GlE+bh
ARFnYK4fvmkGXwEUdCVpdwa8jejdRu0E2CHhyMfoX2cbroACM55XsJGSARk1mJ5f
u3C59mdMwrcplnWD8XBwx3MRSifcBxT0NfNOwZM8yF+GeCCunIElbT93tymwsGAH
Iy5QbwwvgaOjg1nGoh6wcQ==
-----END PRIVATE KEY-----