[WIP] Managed implementation of DNS resolver #6104

rzikm · 2024-10-04T11:54:32Z

Description

This PR brings in a C# implementation of a DNS resolver that is able to signal the TTL information together with the query results.

Main features

Async network I/O, fully cancellable
Mockable
Resolves IP Addresses (A/AAAA records) and Service records (SRV + related A/AAAA)
Transparent fallback to TCP
Autodetection of OS settings (i.e. reads nameservers from /etc/resolv.conf file)
Thread-safe

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No
Does the change require an update in our Aspire docs?
- Yes
  - Link to aspire-docs issue:
- No

Microsoft Reviewers: Open in CodeFlow

BrennanConroy

Only reviewed a subset of files. It's a large PR and has a lot of RFC reading attached to it. Will review more later.

src/Microsoft.Extensions.ServiceDiscovery.Dns/DnsServiceEndpointProvider.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/DnsServiceEndpointProviderBase.Log.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/DnsSrvServiceEndpointProvider.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsDataReader.cs

BrennanConroy · 2025-01-24T21:46:34Z

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsPrimitives.cs

+        //     padding is used.
+        //
+
+        if (!Encoding.ASCII.TryGetBytes(name, destination.IsEmpty ? destination : destination.Slice(1), out int length) || destination.Length < length + 2)


If the destination is empty, we should just return.

Do we need to add validation that only A-Z, a-z, 0-9, -, " ", and . are allowed?

Add 255 length check?

names 255 octets or less

The domain name is already validated at this point, it is done by DnsResolver.GetNormalizedHostName(name) (which throws if any of the domain name limits is violated)

I will add Debug.Asserts for some of the properties

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsPrimitives.cs

BrennanConroy · 2025-01-28T20:03:29Z

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsPrimitives.cs

+
+    }
+
+    internal static bool TryReadQName(ReadOnlySpan<byte> messageBuffer, int offset, [NotNullWhen(true)] out string? name, out int bytesRead)


messageBuffer doesn't have any endianness normalization when it's passed in. Is that problematic?

The only multibyte information is the offset in pointers and we read higher and lower byte separately and combine them correctly. Otherwise we only ever read individual bytes or (ascii) strings, so endianness should not matter.

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/NetworkInfo.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/ResolvConf.cs

BrennanConroy · 2025-01-29T18:39:04Z

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsResolver.cs

+                {
+                    // the message is not a response for our query.
+                    // don't dispose reader, we will reuse the buffer
+                    continue;


This wouldn't use the bytes past DnsMessageHeader.HeaderLength if we read more than that.

This is UDP path, the DNS message is received in entirety in a single ReceiveAsync call, and here we checked that we are not interested in this particular message.

BrennanConroy · 2025-01-29T18:43:18Z

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsResolver.cs

+            int bytesRead = 0;
+            while (responseLength < 0 || bytesRead < length + 2)
+            {
+                int read = await socket.ReceiveAsync(buffer.AsMemory(bytesRead), SocketFlags.None, cancellationToken).ConfigureAwait(false);


Need to check if (read == 0)?

BrennanConroy · 2025-01-29T18:51:10Z

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsResolver.cs

+
+                    if (responseLength > buffer.Length)
+                    {
+                        var largerBuffer = ArrayPool<byte>.Shared.Rent(responseLength);


This is kind of dangerous, user controlled pre-allocation. Since we're limited to 65k here you could argue it's not too risky, but we should probably explicitly call it out in a comment and make sure we're okay with the risk.

Technically the array pool (ConfigurableArrayPool) could return a 65 * 2 * 2 (or more) buffer since the implementation is free to search multiple bucket sizes to find a pooled byte[].

I am curious, if we were to decide it's too risky, what would you suggest as a mitigation? We need the entire message in memory for parsing purposes (domain name compression via pointers with offsets), so I don't see us reading the contents of the message from a stream.

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsResolver.cs

BrennanConroy · 2025-01-29T22:15:50Z

tests/Microsoft.Extensions.ServiceDiscovery.Dns.Tests/Resolver/ResolvConfTests.cs

+        ResolverOptions options = ResolvConf.GetOptions(reader);
+
+        IPEndPoint ipAddress = Assert.Single(options.Servers);
+        Assert.Equal(new IPEndPoint(IPAddress.Parse("10.96.0.10"), 53), ipAddress);


Verify search domains as well?

danmoseley · 2025-02-02T20:36:03Z

@rzikm could you set expectations on this one ?

rzikm · 2025-02-03T14:37:04Z

@rzikm could you set expectations on this one ?

in parallel to this, I am also finishing Threat Model Review and will need to setup fuzzing. If there are no major distractions I hope to finish the work this month.

…on server.

rzikm · 2025-02-13T16:21:41Z

@BrennanConroy I pushed a substantial update which should resolve most of the comments, please take another look.

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 4, 2024

build-analysis bot mentioned this pull request Oct 10, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

rzikm added 8 commits October 21, 2024 13:47

First integration of Resolver sources

2c217a3

Allow mocking

704e33c

Fix dispose

aff6d17

Fix tests

15db8a1

Actually run ResolvConf tests

6bda235

Add retry functionality

6fc61e0

Add logging

2b6d91e

Fix Microsoft.Extensions.ServiceDiscovery.Yarp.Tests

2137954

davidfowl requested a review from ReubenBond October 29, 2024 21:16

rzikm added 5 commits November 26, 2024 14:46

Merge remote-tracking branch 'upstream/main' into managed-resolver

c204b01

Activity WIP

2fc7de4

Fix reading CNAME with pointers in domain name segments

9b4de29

Reenable telemetry

3ad5a75

Last changes to telemetry

40f8f9b

rzikm force-pushed the managed-resolver branch from 32d94c5 to 40f8f9b Compare November 28, 2024 16:07

Use strong RNG for Transaction ID generation

e385605

BrennanConroy reviewed Jan 24, 2025

View reviewed changes

MihaZupan mentioned this pull request Jan 27, 2025

Stalled getaddrinfo syscall can lead to massive HttpClient timeouts dotnet/runtime#81023

Open

BrennanConroy reviewed Jan 29, 2025

View reviewed changes

danmoseley added area-service-discovery needs-author-action An issue or pull request that requires more info or actions from the author. labels Feb 2, 2025

rzikm added 2 commits February 3, 2025 13:44

Check against too long domain name

3cf0a9d

Disallow pointer to pointer.

6016e41

dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 3, 2025

rzikm added 2 commits February 3, 2025 17:18

Code review feedback (easy fixes)

f8c189d

More code review feedback

d929889

Fix increment

00e41d7

build-analysis bot mentioned this pull request Feb 6, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

rzikm added 7 commits February 7, 2025 09:53

Detect CNAME loops

7267fb7

Handle empty Tcp fallback responses and lack of TCP failover support …

c1b76d3

…on server.

Dispose result.Response when overwritten by another result.

86490cc

Move DnsMessageHeader parsing to DnsPrimitives

92b2905

Rework and add tests to retries and failover

fdf1904

Test failover after exhausting attempts on one server

61814e7

Streamline options, remove unsupported options from ResolverOptions

82a8108

rzikm force-pushed the managed-resolver branch from 0fa2406 to 82a8108 Compare February 13, 2025 14:36

Better handling of malformed responses

fa627fd

rzikm force-pushed the managed-resolver branch from 3721d48 to fa627fd Compare February 13, 2025 16:19

rzikm requested a review from BrennanConroy February 13, 2025 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Managed implementation of DNS resolver #6104

[WIP] Managed implementation of DNS resolver #6104

rzikm commented Oct 4, 2024 •

edited

Loading

BrennanConroy left a comment

BrennanConroy Jan 24, 2025

BrennanConroy Jan 24, 2025

BrennanConroy Jan 24, 2025

rzikm Feb 5, 2025

rzikm Feb 5, 2025

BrennanConroy Jan 28, 2025

rzikm Feb 5, 2025

BrennanConroy Jan 29, 2025

rzikm Feb 7, 2025

BrennanConroy Jan 29, 2025

BrennanConroy Jan 29, 2025

rzikm Feb 7, 2025

BrennanConroy Jan 29, 2025

danmoseley commented Feb 2, 2025

rzikm commented Feb 3, 2025

rzikm commented Feb 13, 2025


		}

		internal static bool TryReadQName(ReadOnlySpan<byte> messageBuffer, int offset, [NotNullWhen(true)] out string? name, out int bytesRead)

[WIP] Managed implementation of DNS resolver #6104

Are you sure you want to change the base?

[WIP] Managed implementation of DNS resolver #6104

Conversation

rzikm commented Oct 4, 2024 • edited Loading

Description

Checklist

Microsoft Reviewers: Open in CodeFlow

BrennanConroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Feb 2, 2025

rzikm commented Feb 3, 2025

rzikm commented Feb 13, 2025

rzikm commented Oct 4, 2024 •

edited

Loading