From 4a70950d2c9dcb1cb4945afeab3eb12594db1f24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Wed, 18 Sep 2024 12:20:31 +0200 Subject: [PATCH 1/4] Remove TXT record validation from custom DNS zones --- lib/vnet/app_resolver.go | 47 ++++----------- lib/vnet/customdnszonevalidator.go | 95 ------------------------------ lib/vnet/vnet_test.go | 28 +-------- 3 files changed, 13 insertions(+), 157 deletions(-) delete mode 100644 lib/vnet/customdnszonevalidator.go diff --git a/lib/vnet/app_resolver.go b/lib/vnet/app_resolver.go index 4ab308b6e459..3298a40854de 100644 --- a/lib/vnet/app_resolver.go +++ b/lib/vnet/app_resolver.go @@ -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 { + return strings.HasSuffix(appFQDN, "."+fullyQualify(zone)) } // fullyQualify returns a fully-qualified domain name from [domain]. Fully-qualified domain names always end diff --git a/lib/vnet/customdnszonevalidator.go b/lib/vnet/customdnszonevalidator.go deleted file mode 100644 index 752606581eaa..000000000000 --- a/lib/vnet/customdnszonevalidator.go +++ /dev/null @@ -1,95 +0,0 @@ -// Teleport -// Copyright (C) 2024 Gravitational, Inc. -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU Affero General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Affero General Public License for more details. -// -// You should have received a copy of the GNU Affero General Public License -// along with this program. If not, see . - -package vnet - -import ( - "context" - "errors" - "net" - "slices" - "sync" - - "github.com/gravitational/trace" -) - -const ( - clusterTXTRecordPrefix = "teleport-cluster=" -) - -type lookupTXTFunc = func(ctx context.Context, domain string) (txtRecords []string, err error) - -// customDNSZoneValidator validates custom DNS zones by making sure that they have a DNS TXT record of -// "teleport-cluster=" for the cluster in which they are used. This is meant to avoid the -// possibility of a rogue application advertising a public_addr with a DNS name not controlled by the cluster -// admins, which could be used to trick VNet users. -// -// After finding that a zone is valid once, this is cached indefinitely. Invalid zones are a misconfiguration -// so we don't cache negative results -type customDNSZoneValidator struct { - lookupTXT lookupTXTFunc - validZones map[string]struct{} - mu sync.RWMutex -} - -func newCustomDNSZoneValidator(lookupTXT lookupTXTFunc) *customDNSZoneValidator { - if lookupTXT == nil { - var resolver net.Resolver - lookupTXT = resolver.LookupTXT - } - return &customDNSZoneValidator{ - lookupTXT: lookupTXT, - validZones: make(map[string]struct{}), - } -} - -// validate returns an error if [customDNSZone] is not valid for [clusterName]. -func (c *customDNSZoneValidator) validate(ctx context.Context, clusterName, customDNSZone string) error { - c.mu.RLock() - _, ok := c.validZones[customDNSZone] - c.mu.RUnlock() - if ok { - return nil - } - - requiredTXTRecord := clusterTXTRecordPrefix + clusterName - log.InfoContext(ctx, "Checking validity of custom DNS zone by querying for required TXT record.", "zone", customDNSZone, "record", requiredTXTRecord) - - records, err := c.lookupTXT(ctx, customDNSZone) - if err != nil { - var dnsErr *net.DNSError - if errors.As(err, &dnsErr) && dnsErr.IsNotFound { - return trace.Wrap(errNoTXTRecord(customDNSZone, requiredTXTRecord)) - } - return trace.Wrap(err, "unexpected error looking up TXT records for %q", customDNSZone) - } - - valid := slices.Contains(records, requiredTXTRecord) - if !valid { - return trace.Wrap(errNoTXTRecord(customDNSZone, requiredTXTRecord)) - } - - log.InfoContext(ctx, "Custom DNS zone has valid TXT record.", "zone", customDNSZone, "cluster", clusterName) - - c.mu.Lock() - defer c.mu.Unlock() - c.validZones[customDNSZone] = struct{}{} - return nil -} - -func errNoTXTRecord(customDNSZone, requiredTXTRecord string) error { - return trace.BadParameter(`custom DNS zone %q does not have the required TXT record %q`, customDNSZone, requiredTXTRecord) -} diff --git a/lib/vnet/vnet_test.go b/lib/vnet/vnet_test.go index a6c1a9655485..b7713ed21852 100644 --- a/lib/vnet/vnet_test.go +++ b/lib/vnet/vnet_test.go @@ -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", }, 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) { From d22ee6c06c4c5929789f492fb6e7f173709e4363 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Wed, 18 Sep 2024 12:51:05 +0200 Subject: [PATCH 2/4] Remove mentions of TXT records from docs --- .../application-access/guides/vnet.mdx | 43 +++---------------- 1 file changed, 6 insertions(+), 37 deletions(-) diff --git a/docs/pages/enroll-resources/application-access/guides/vnet.mdx b/docs/pages/enroll-resources/application-access/guides/vnet.mdx index 02b029330183..43d3f5b4ee2f 100644 --- a/docs/pages/enroll-resources/application-access/guides/vnet.mdx +++ b/docs/pages/enroll-resources/application-access/guides/vnet.mdx @@ -20,8 +20,7 @@ with a virtual IP address over which the connection will be proxied to the app. case, the query is forwarded to the default DNS name server used by the OS. If you want VNet to forward connections to an app that has a custom `public_addr` set, you need -to first update the VNet config in the Auth Service to include a matching DNS zone and authorize the -zone through a DNS TXT record. +to first update the VNet config in the Auth Service to include a matching DNS zone. ## Prerequisites @@ -35,7 +34,7 @@ In this guide, we'll use the example app from [TCP Application Access guide](tcp available through VNet at with as the custom DNS zone. -## Step 1/4. Configure custom DNS zone +## Step 1/3. Configure custom DNS zone Create a file called `vnet-config.yaml` which specifies the custom DNS zone. In our case the `public_addr` of the app is going to be `tcp-app.company.test`, so we're going to set @@ -60,38 +59,11 @@ vnet_config has been created `suffix` doesn't have to point to a domain that's exactly one level above the `public_addr` of an -app. Any level of nesting works. E.g., you can have an app under `tcp-app.foo.bar.qux.test` and the -suffix can be set to `bar.qux.test`. +app. Any level of nesting works. For example, you can have an app under `tcp-app.foo.bar.qux.test` +and the suffix can be set to `bar.qux.test`. -## Step 2/4. Set DNS TXT record - -Establish the name of your cluster: - -```code -$ tctl status | awk '/Cluster/ {print $2}' -teleport.example.com -``` - -Set a DNS TXT record `teleport-cluster=` on the domain provided as `suffix` in the -VNet config. This is required by VNet to prevent a rogue cluster admin from hijacking DNS resolution -of arbitrary domains for unsuspecting users. - -Wait a couple of minutes and verify that the DNS record changes have been propagated. - -```code -$ host -t TXT -company.test descriptive text "teleport-cluster=teleport.example.com" -``` - - -If the domain you chose for the suffix has the CNAME record set, you won't be able to add a TXT -record. A CNAME record must be the only record for the given name. In that scenario, you can set the -suffix to a domain one level above if possible (e.g., `company.test` instead of -`internal.company.test`) or switch from CNAME to A records. - - -## Step 3/4. Set `public_addr` of the app +## Step 2/3. Set `public_addr` of the app Set `public_addr` field of the application in the Application Service configuration file `/etc/teleport.yaml` and restart the `teleport` daemon. @@ -107,7 +79,7 @@ app_service: public_addr: "" ``` -## Step 4/4. Connect +## Step 3/3. Connect Once you [start VNet](../../../connect-your-client/vnet.mdx), you should be able to connect to the application over the custom `public_addr` using the application client you would normally use to @@ -168,9 +140,6 @@ To make a [leaf cluster](../../../admin-guides/management/admin/trustedclusters. $ tsh login --proxy= --user= ``` -The DNS TXT record of the domain provided as `suffix` needs to contain the name of the leaf cluster. -A single domain can have multiple TXT records if you want to use it in multiple clusters. - ### Accessing web apps through VNet VNet does not officially support [web apps](connecting-apps.mdx) yet. However, technically any web From 42db921d3a1bbd0d901e0628127ee9e03dc2779a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Wed, 18 Sep 2024 12:57:44 +0200 Subject: [PATCH 3/4] Outline in the RFD why domain verification was dropped --- rfd/0163-vnet.md | 53 +++++++++++++++++++----------------------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/rfd/0163-vnet.md b/rfd/0163-vnet.md index c1e53a0bee1c..7b33f76bc6b7 100644 --- a/rfd/0163-vnet.md +++ b/rfd/0163-vnet.md @@ -146,38 +146,27 @@ spec: - suffix: goteleport.com ``` -In this example, whenever the user is logged in to the cluster where this `vnet_config` -configuration resource is present, VNet will install itself as the DNS -nameserver for subdomains of `example.com` and `goteleport.com`. -When the DNS server receives a query for any subdomain of those suffixes, it -will search the cluster for an app with a matching `public_addr`. - -In the interest of being good citizens of the internet and to avoid hijacking -DNS resolution for domains which do not wish for us to do so, VNet will not -resolve requests for names under a custom DNS zone unless that zone contains a -specific DNS TXT record. -This TXT record must include the value `teleport-cluster=`, e.g. -`teleport-cluster=teleport.example.com`. -If a single DNS zone is to be served by VNet in multiple Teleport clusters, -multiple TXT records can be configured on the domain, one for each cluster. -VNet will look up this TXT record with at the default nameservers configured in -the client OS, so the records should be public or available to all clients. - -So if your Teleport proxy is hosted at `teleport.example.com` and you want -`api.example.com` to be resolvable within VNet, you need: - -1. The app registered in Teleport must have `public_addr: api.example.com`. -2. The following `vnet_config` resource: - ``` - version: v1 - kind: vnet_config - metadata: - name: vnet_config - spec: - custom_dns_zones: - - suffix: example.com - ``` -3. The following DNS TXT record on `example.com`: `teleport-cluster=teleport.example.com`. +#### Domain verification + +Initial versions of VNet required the admin to configure a special TXT record on +the domain in the form of `teleport-cluster=`. This prevented +customers from using domains not accessible over public DNS as custom DNS zones +in VNet. We wanted to change this behavior so that VNet validates the TXT +records only if the domain lookup doesn't return an NXDOMAIN response. However, +after some consideration we've decided to completely drop domain verification +for the following reasons: + +1. It had limited security benefits. DNS over UDP is an unreliable protocol, + any network MitM can spoof an NXDOMAIN response or even respond with the TXT + record to bypass this requirement. +1. It was an extra configuration burden that added time-to-value for VNet users. + Often you need another team and approval to add TXT records. Sometimes it's + just not possible. +1. We already require all custom domains to be added to the cluster vnet_config + by a Teleport cluster admin. If they already are okay with adding it there, + the TXT record is just another burden. +1. It prevents an entire class of potential VNet usecases for putting public + addresses behind VNet. #### Leaf and parallel clusters From 322feaf2f2a1b27529407e8d76e50576bffcf95e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Wed, 18 Sep 2024 18:01:05 +0200 Subject: [PATCH 4/4] Update rfd/0163-vnet.md Co-authored-by: Nic Klaassen --- rfd/0163-vnet.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfd/0163-vnet.md b/rfd/0163-vnet.md index 7b33f76bc6b7..02564576f693 100644 --- a/rfd/0163-vnet.md +++ b/rfd/0163-vnet.md @@ -151,7 +151,7 @@ spec: Initial versions of VNet required the admin to configure a special TXT record on the domain in the form of `teleport-cluster=`. This prevented customers from using domains not accessible over public DNS as custom DNS zones -in VNet. We wanted to change this behavior so that VNet validates the TXT +in VNet. We considered changing this behavior so that VNet validates the TXT records only if the domain lookup doesn't return an NXDOMAIN response. However, after some consideration we've decided to completely drop domain verification for the following reasons: