Skip to content
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.

Sign up for GitHub

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

Added support for upstream verification for TCPProxy #6079

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Added support for upstream verification for TCPProxy #6079

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6773a1a
added support for upstream verification for TCPProxy
tsaarni Jan 14, 2024
e0938e2
update after review
tsaarni Jan 19, 2024
9450ff5
resolved merge conflicts
tsaarni Jan 27, 2024
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
4 changes: 4 additions & 0 deletions changelogs/unreleased/6079-tsaarni-minor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## Upstream TLS validation and client certificate for TCPProxy

TCPProxy now supports validating server certificate and using client certificate for upstream TLS connections.
Set `httpproxy.spec.tcpproxy.services.validation.caSecret` and `subjectName` to enable optional validation and `tls.envoy-client-certificate` configuration file field or `ContourConfiguration.spec.envoy.clientCertificate` to set the optional client certificate.
52 changes: 41 additions & 11 deletions internal/dag/httpproxy_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,17 +951,8 @@

var uv *PeerValidationContext
if (protocol == "tls" || protocol == "h2") && service.UpstreamValidation != nil {
caCertNamespacedName := k8s.NamespacedNameFrom(service.UpstreamValidation.CACertificate, k8s.DefaultNamespace(proxy.Namespace))
// we can only validate TLS connections to services that talk TLS
uv, err = p.source.LookupUpstreamValidation(service.UpstreamValidation, caCertNamespacedName, proxy.Namespace)
if err != nil {
if _, ok := err.(DelegationNotPermittedError); ok {
validCond.AddErrorf(contour_api_v1.ConditionTypeTLSError, "CACertificateNotDelegated",
"service.UpstreamValidation.CACertificate Secret %q is not configured for certificate delegation", caCertNamespacedName)
} else {
validCond.AddErrorf(contour_api_v1.ConditionTypeServiceError, "TLSUpstreamValidation",
"Service [%s:%d] TLS upstream validation policy error: %s", service.Name, service.Port, err)
}
uv = p.peerValidationContext(validCond, proxy, service)
if uv == nil {
return nil
}
}
Expand Down Expand Up @@ -1212,6 +1203,25 @@
return false
}

var uv *PeerValidationContext
if (protocol == "tls" || protocol == "h2") && service.UpstreamValidation != nil {
uv = p.peerValidationContext(validCond, httpproxy, service)
if uv == nil {
return false
}

Check warning on line 1211 in internal/dag/httpproxy_processor.go

View check run for this annotation

Codecov / codecov/patch

internal/dag/httpproxy_processor.go#L1210-L1211 

Added lines #L1210 - L1211 were not covered by tests
}

var clientCertSecret *Secret
if p.ClientCertificate != nil {
// Since the client certificate is configured by admin, explicit delegation is not required.
clientCertSecret, err = p.source.LookupTLSSecretInsecure(*p.ClientCertificate)
if err != nil {
validCond.AddErrorf(contour_api_v1.ConditionTypeTLSError, "SecretNotValid",
"tls.envoy-client-certificate Secret %q is invalid: %s", p.ClientCertificate, err)
return false
}

Check warning on line 1222 in internal/dag/httpproxy_processor.go

View check run for this annotation

Codecov / codecov/patch

internal/dag/httpproxy_processor.go#L1219-L1222 

Added lines #L1219 - L1222 were not covered by tests
}

proxy.Clusters = append(proxy.Clusters, &Cluster{
Upstream: s,
Weight: uint32(service.Weight),
Expand All @@ -1220,6 +1230,9 @@
TCPHealthCheckPolicy: healthPolicy,
SNI: s.ExternalName,
tsaarni marked this conversation as resolved.
Show resolved Hide resolved
TimeoutPolicy: ClusterTimeoutPolicy{ConnectTimeout: p.ConnectTimeout},
UpstreamTLS: p.UpstreamTLS,
UpstreamValidation: uv,
ClientCertificate: clientCertSecret,
skriss marked this conversation as resolved.
Show resolved Hide resolved
})
}

Expand Down Expand Up @@ -1482,6 +1495,23 @@
return nil
}

func (p *HTTPProxyProcessor) peerValidationContext(validCond *contour_api_v1.DetailedCondition, httpproxy *contour_api_v1.HTTPProxy, service contour_api_v1.Service) *PeerValidationContext {
caCertNamespacedName := k8s.NamespacedNameFrom(service.UpstreamValidation.CACertificate, k8s.DefaultNamespace(httpproxy.Namespace))
// we can only validate TLS connections to services that talk TLS
uv, err := p.source.LookupUpstreamValidation(service.UpstreamValidation, caCertNamespacedName, httpproxy.Namespace)
if err != nil {
if _, ok := err.(DelegationNotPermittedError); ok {
validCond.AddErrorf(contour_api_v1.ConditionTypeTLSError, "CACertificateNotDelegated",
"service.UpstreamValidation.CACertificate Secret %q is not configured for certificate delegation", caCertNamespacedName)
} else {
validCond.AddErrorf(contour_api_v1.ConditionTypeServiceError, "TLSUpstreamValidation",
"Service [%s:%d] TLS upstream validation policy error: %s", service.Name, service.Port, err)
}
return nil
}
return uv
}

// expandPrefixMatches adds new Routes to account for the difference
// between prefix replacement when matching on '/foo' and '/foo/'.
//
Expand Down
50 changes: 46 additions & 4 deletions internal/featuretests/v3/backendcavalidation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/projectcontour/contour/internal/dag"
"github.com/projectcontour/contour/internal/featuretests"
"github.com/projectcontour/contour/internal/fixture"
"github.com/projectcontour/contour/internal/ref"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
Expand All @@ -30,7 +31,7 @@ func TestClusterServiceTLSBackendCAValidation(t *testing.T) {
rh, c, done := setup(t)
defer done()

secret := &v1.Secret{
caSecret := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "default",
Expand Down Expand Up @@ -60,7 +61,7 @@ func TestClusterServiceTLSBackendCAValidation(t *testing.T) {
}},
},
}
rh.OnAdd(secret)
rh.OnAdd(caSecret)
rh.OnAdd(svc)
rh.OnAdd(p1)

Expand Down Expand Up @@ -93,7 +94,7 @@ func TestClusterServiceTLSBackendCAValidation(t *testing.T) {
Name: svc.Name,
Port: 443,
UpstreamValidation: &contour_api_v1.UpstreamValidation{
CACertificate: secret.Name,
CACertificate: caSecret.Name,
SubjectName: "subjname",
},
}},
Expand Down Expand Up @@ -140,7 +141,7 @@ func TestClusterServiceTLSBackendCAValidation(t *testing.T) {
Name: svc.Name,
Port: 443,
UpstreamValidation: &contour_api_v1.UpstreamValidation{
CACertificate: secret.Name,
CACertificate: caSecret.Name,
SubjectName: "subjname",
},
}},
Expand Down Expand Up @@ -174,4 +175,45 @@ func TestClusterServiceTLSBackendCAValidation(t *testing.T) {
Resources: nil,
TypeUrl: secretType,
})

rh.OnDelete(hp1)

tlsSecret := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "secret",
Namespace: "default",
},
Type: v1.SecretTypeTLS,
Data: featuretests.Secretdata(featuretests.CERTIFICATE, featuretests.RSA_PRIVATE_KEY),
}
rh.OnAdd(tlsSecret)

tcpproxy := fixture.NewProxy("tcpproxy").WithSpec(
contour_api_v1.HTTPProxySpec{
VirtualHost: &contour_api_v1.VirtualHost{
Fqdn: "www.example.com",
TLS: &contour_api_v1.TLS{
SecretName: tlsSecret.Name,
},
},
TCPProxy: &contour_api_v1.TCPProxy{
Services: []contour_api_v1.Service{{
Name: svc.Name,
Port: 443,
Protocol: ref.To("tls"),
UpstreamValidation: &contour_api_v1.UpstreamValidation{
CACertificate: caSecret.Name,
SubjectName: "subjname",
},
}},
},
})
rh.OnAdd(tcpproxy)

c.Request(clusterType).Equals(&envoy_discovery_v3.DiscoveryResponse{
Resources: resources(t,
tlsCluster(cluster("default/kuard/443/c6ccd34de5", "default/kuard/securebackend", "default_kuard_443"), []byte(featuretests.CERTIFICATE), "subjname", "", nil, nil),
Copy link
Member

Choose a reason for hiding this comment

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

maybe a refactor for the future, but since we only really have one fixture for cert content, we end up asserting on the same value everywhere ([]byte(featuretests.CERTIFICATE)), would be good to maybe at least use the field from caSecret for readability? (looks like sec2 in the other test below?)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 While adding the test I was reminded that I would have preferred to remove the hardcoded test certificates & private keys from the code. I'll check if I could simplify this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the tests so that they use the field from CA secret, though this will be bit simpler if we merge #6100 as it will allow passing the secret itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now updated according to #6100

),
TypeUrl: clusterType,
})
}
31 changes: 31 additions & 0 deletions internal/featuretests/v3/backendclientauth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,37 @@ func TestBackendClientAuthenticationWithHTTPProxy(t *testing.T) {
TypeUrl: clusterType,
})

rh.OnDelete(proxy)

tcpproxy := fixture.NewProxy("tcpproxy").WithSpec(
projcontour.HTTPProxySpec{
VirtualHost: &projcontour.VirtualHost{
Fqdn: "www.example.com",
TLS: &projcontour.TLS{
SecretName: sec1.Name,
},
},
TCPProxy: &projcontour.TCPProxy{
Services: []projcontour.Service{{
Name: svc.Name,
Port: 443,
Protocol: ref.To("tls"),
UpstreamValidation: &projcontour.UpstreamValidation{
CACertificate: sec2.Name,
SubjectName: "subjname",
},
}},
},
})
rh.OnAdd(tcpproxy)

c.Request(clusterType).Equals(&envoy_discovery_v3.DiscoveryResponse{
Resources: resources(t,
tlsCluster(cluster("default/backend/443/950c17581f", "default/backend/http", "default_backend_443"), []byte(featuretests.CERTIFICATE), "subjname", "", sec1, nil),
),
TypeUrl: clusterType,
})

// Test the error branch when Envoy client certificate secret does not exist.
rh.OnDelete(sec1)
c.Request(clusterType).Equals(&envoy_discovery_v3.DiscoveryResponse{
Expand Down
Loading