-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Remove TXT record validation of custom DNS zones in VNet #46709
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,12 +92,10 @@ type DialOptions struct { | |
|
||
// TCPAppResolver implements [TCPHandlerResolver] for Teleport TCP apps. | ||
type TCPAppResolver struct { | ||
appProvider AppProvider | ||
clusterConfigCache *ClusterConfigCache | ||
customDNSZoneChecker *customDNSZoneValidator | ||
slog *slog.Logger | ||
clock clockwork.Clock | ||
lookupTXT lookupTXTFunc | ||
appProvider AppProvider | ||
clusterConfigCache *ClusterConfigCache | ||
slog *slog.Logger | ||
clock clockwork.Clock | ||
} | ||
|
||
// NewTCPAppResolver returns a new *TCPAppResolver which will resolve full-qualified domain names to | ||
|
@@ -118,7 +116,6 @@ func NewTCPAppResolver(appProvider AppProvider, opts ...tcpAppResolverOption) (* | |
} | ||
r.clock = cmp.Or(r.clock, clockwork.NewRealClock()) | ||
r.clusterConfigCache = cmp.Or(r.clusterConfigCache, NewClusterConfigCache(r.clock)) | ||
r.customDNSZoneChecker = newCustomDNSZoneValidator(r.lookupTXT) | ||
return r, nil | ||
} | ||
|
||
|
@@ -131,13 +128,6 @@ func withClock(clock clockwork.Clock) tcpAppResolverOption { | |
} | ||
} | ||
|
||
// withLookupTXTFunc is a functional option to override the DNS TXT record lookup function (for tests). | ||
func withLookupTXTFunc(lookupTXT lookupTXTFunc) tcpAppResolverOption { | ||
return func(r *TCPAppResolver) { | ||
r.lookupTXT = lookupTXT | ||
} | ||
} | ||
|
||
// WithClusterConfigCache is a functional option to override the cluster config cache. | ||
func WithClusterConfigCache(clusterConfigCache *ClusterConfigCache) tcpAppResolverOption { | ||
return func(r *TCPAppResolver) { | ||
|
@@ -194,8 +184,8 @@ func (r *TCPAppResolver) clusterClientForAppFQDN(ctx context.Context, profileNam | |
return nil, errNoMatch | ||
} | ||
|
||
if isSubdomain(fqdn, profileName) { | ||
// The queried app fqdn is direct subdomain of this cluster proxy address. | ||
if isDescendantSubdomain(fqdn, profileName) { | ||
// The queried app fqdn is a subdomain of this cluster proxy address. | ||
return rootClient, nil | ||
} | ||
|
||
|
@@ -221,25 +211,9 @@ func (r *TCPAppResolver) clusterClientForAppFQDN(ctx context.Context, profileNam | |
continue | ||
} | ||
for _, zone := range clusterConfig.DNSZones { | ||
if !isSubdomain(fqdn, zone) { | ||
// The queried app fqdn is not a subdomain of this zone, skip it. | ||
continue | ||
} | ||
|
||
// Found a matching cluster. | ||
|
||
if zone == clusterConfig.ProxyPublicAddr { | ||
// We don't need to validate a custom DNS zone if this is the proxy public address, this is a | ||
// normal app public_addr. | ||
if isDescendantSubdomain(fqdn, zone) { | ||
return clusterClient, nil | ||
} | ||
// The queried app fqdn is a subdomain of this custom zone. Check if the zone is valid. | ||
if err := r.customDNSZoneChecker.validate(ctx, clusterConfig.ClusterName, zone); err != nil { | ||
// Return an error here since the FQDN does match this custom zone, but the zone failed to | ||
// validate. | ||
return nil, trace.Wrap(err, "validating custom DNS zone %q matching queried FQDN %q", zone, fqdn) | ||
} | ||
return clusterClient, nil | ||
} | ||
} | ||
return nil, errNoMatch | ||
|
@@ -392,8 +366,11 @@ func (i *appCertIssuer) IssueCert(ctx context.Context) (tls.Certificate, error) | |
return cert.(tls.Certificate), trace.Wrap(err) | ||
} | ||
|
||
func isSubdomain(appFQDN, suffix string) bool { | ||
return strings.HasSuffix(appFQDN, "."+fullyQualify(suffix)) | ||
// isDescendantSubdomain checks if appFQDN belongs in the hierarchy of zone. For example, both | ||
// foo.bar.baz.example.com and bar.baz.example.com belong in the hierarchy of baz.example.com, but | ||
// quux.example.com does not. | ||
func isDescendantSubdomain(appFQDN, zone string) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renaming this as I was already confused by this in the past. For me "subdomain" means like a direct descendant, e.g. foo.example.com is a subdomain of example.com. |
||
return strings.HasSuffix(appFQDN, "."+fullyQualify(zone)) | ||
} | ||
|
||
// fullyQualify returns a fully-qualified domain name from [domain]. Fully-qualified domain names always end | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,10 +74,6 @@ type testPack struct { | |
type testPackConfig struct { | ||
clock clockwork.FakeClock | ||
appProvider AppProvider | ||
|
||
// customDNSZones is a map where the keys are custom DNS zones that should be valid, and the values are a | ||
// slice of all Teleport clusters they should be valid for. | ||
customDNSZones map[string][]string | ||
} | ||
|
||
func newTestPack(t *testing.T, ctx context.Context, cfg testPackConfig) *testPack { | ||
|
@@ -130,21 +126,7 @@ func newTestPack(t *testing.T, ctx context.Context, cfg testPackConfig) *testPac | |
|
||
dnsIPv6 := ipv6WithSuffix(vnetIPv6Prefix, []byte{2}) | ||
|
||
lookupTXT := func(ctx context.Context, customDNSZone string) ([]string, error) { | ||
clusters, ok := cfg.customDNSZones[customDNSZone] | ||
if !ok { | ||
return nil, nil | ||
} | ||
txtRecords := make([]string, 0, len(clusters)) | ||
for _, cluster := range clusters { | ||
txtRecords = append(txtRecords, clusterTXTRecordPrefix+cluster) | ||
} | ||
return txtRecords, nil | ||
} | ||
|
||
tcpHandlerResolver, err := NewTCPAppResolver(cfg.appProvider, | ||
withClock(cfg.clock), | ||
withLookupTXTFunc(lookupTXT)) | ||
tcpHandlerResolver, err := NewTCPAppResolver(cfg.appProvider, withClock(cfg.clock)) | ||
require.NoError(t, err) | ||
|
||
// Create the VNet and connect it to the other side of the TUN. | ||
|
@@ -480,13 +462,9 @@ func TestDialFakeApp(t *testing.T) { | |
"echo.myzone.example.com", | ||
"echo.nested.myzone.example.com", | ||
"not.in.a.custom.zone", | ||
"in.an.invalid.zone", | ||
}, | ||
customDNSZones: []string{ | ||
"myzone.example.com", | ||
// This zone will not have a valid TXT record because it is not included in the | ||
// customDNSZones passed to newTestPack. | ||
"an.invalid.zone", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After removing the TXT record requirement, this custom domain becomes yet another custom DNS zone in use. It's not that much different in behavior to |
||
}, | ||
cidrRange: "192.168.2.0/24", | ||
leafClusters: map[string]testClusterSpec{ | ||
|
@@ -511,9 +489,6 @@ func TestDialFakeApp(t *testing.T) { | |
p := newTestPack(t, ctx, testPackConfig{ | ||
clock: clock, | ||
appProvider: appProvider, | ||
customDNSZones: map[string][]string{ | ||
"myzone.example.com": {"root1.example.com", "another.random.cluster.example.com"}, | ||
}, | ||
}) | ||
|
||
validTestCases := []struct { | ||
|
@@ -602,7 +577,6 @@ func TestDialFakeApp(t *testing.T) { | |
invalidTestCases := []string{ | ||
"not.an.app.example.com.", | ||
"not.in.a.custom.zone", | ||
"in.an.invalid.zone", | ||
} | ||
for _, fqdn := range invalidTestCases { | ||
t.Run(fqdn, func(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be basically "Given the FQDN of an app, if you find a matching custom DNS zone configured in the cluster, validate its TXT records and return the cluster client, unless it's a zone of the proxy service, then just skip the validation and return the client".
Now we do not care about the TXT records, so we just return the client if the FQDN on the app matches a zone for this cluster.