Conversation
|
|
||
| /// The fully-qualified domain name with trailing dot. | ||
| public var description: String { | ||
| labels.isEmpty ? "." : labels.joined(separator: ".") + "." |
There was a problem hiding this comment.
Question: some lookup paths appear to use plain string (no normalization), will this change cause any issue? For example in AttachmentAllocator;
// store
func allocate(hostname: String)..
// lookup
func lookup(hostname: String) async throws -> UInt32? {
hostnames[hostname]
}
There was a problem hiding this comment.
@manojmahapatra Thanks for looking at this! I converted it to draft.
This one was done by telling Claude to create the types we needed using the bit-fiddling code that ContainerizationNetlink uses, and I pushed it primarily to back the change up and get something started for the issue.
I still need to review the work myself and sanity check it, but I'll bet I would have missed this!
I'll have a look through the relevant RFCs as I review this myself. This isn't an area I'm super expert in so if you've got experience and suggestions I'd welcome them!
There was a problem hiding this comment.
I'm not an expert in this area either, but I reviewed the PR in detail against current behavior. Overall this is moving in the right direction.
I see two gaps worth addressing here:
- Hostname normalization consistency (e.g.
foovsfoo.) across parse/store/lookup paths. (additional focused unit tests will be nice) - Compression-pointer safety in
DNSNamedecode to avoid malformed packet loops. Something like this maybe;
// Calculate pointer offset from message start
let pointerLocation = offset
let pointer = Int(length & 0x3F) << 8 | Int(buffer[offset + 1])
let pointerTarget = messageStart + pointer
guard pointerTarget < pointerLocation else {
throw DNSBindError.unmarshalFailure(type: "DNSName", field: "compression pointer not prior")
}
offset = pointerTarget
jglogan
force-pushed
the
dns-records
branch
3 times, most recently
from
0d90e59 to
32eac7e
Compare
- Closes apple#1268. - The types we were using weren't very usable with Swift 6 structured concurrency. - Use notImplemented instead of formatError for unknown record types. - Use pure actor for LocalhostDNSHandler now that we have sendable types.
- Require all input domain names to carry trailing dot.
- Fold labels to lowercase. - Reject compression pointer recursion.
|
@manojmahapatra I've addressed what you called out, and also we're now folding to lowercase when creating domain name records. The larger change (requiring a trailing dot on all the DNS APIs) is in the first follow-up commit, and the smaller one for case folding and compression pointer recursion preventions is in the second one. Finally got myself out from under some other obligations which've kept me busy elsewhere. I'll look over your apple/containerization#577 early tomorrow. Thank you again for your scrutiny on this PR! |
| /// | ||
| /// - Parameter dnsHostname: A DNS-format hostname, optionally with a trailing dot | ||
| /// (e.g. `"example.com."` or `"example.com"`). The trailing dot is stripped before lookup. | ||
| public func lookup(dnsHostname: String) async throws -> Attachment? { |
There was a problem hiding this comment.
Question: Looks like it's a public API break due to the function rename? Do we need a shim?
| /// Handler that uses table lookup to resolve hostnames. | ||
| /// | ||
| /// All keys in `hosts4` must be canonical DNS names — fully-qualified with a | ||
| /// trailing dot (e.g. `"example.com."`). This matches the canonical form used |
| public init(_ string: String) { | ||
| // Remove trailing dot if present, then split | ||
| let normalized = string.hasSuffix(".") ? String(string.dropLast()) : string | ||
| self.labels = normalized.isEmpty ? [] : normalized.split(separator: ".").map { String($0).lowercased() } |
There was a problem hiding this comment.
Question: So here labels are lowercases when parsing names. However, HostTableResolver does not normalize keys on init, which I think might be an issue. can you please confirm?
|
@jglogan overall the changes looks good, just two questions. |
| /// | ||
| /// - Parameter hosts4: A dictionary mapping fully-qualified domain names (with trailing dot) | ||
| /// to IPv4 addresses. Keys without a trailing dot will not match wire-decoded queries. | ||
| /// - Parameter ttl: The TTL in seconds to set on answer records. |
There was a problem hiding this comment.
nitpick: The TTL in seconds to set on answer records. Defaults to 300.
2 participants
Type of Change
Testing