diff --git a/specs/milestone-3.md b/specs/milestone-3.md index ed651f7..2efb416 100644 --- a/specs/milestone-3.md +++ b/specs/milestone-3.md @@ -1011,3 +1011,13 @@ review rounds; findings went 9 → 3 → 0): `OutOfMemory` is a local resource, not a peer fault. - `transport.zig`: `Endpoint.parse` rejects userinfo/query/fragment delimiters (`@`, `?`, `#`) in the authority, and `?`/`#` in a DoH path. +- `dot_client.zig` (follow-up, tls_name commit): `DotClient.init` takes a + `tls_name`; it is the SNI and certificate-verification name, while the dial + target stays `endpoint.host`. Empty keeps the endpoint host, which is the + behavior described above. The same follow-up fixed a send bug this file + had from the start, invisible until a DoT handshake first succeeded: + `tls.Client.flush` only encrypts into the socket writer's buffer and never + flushes it, so the query never left the process and the peer eventually + closed the connection (`ReceiveFailed`/`EndOfStream`). `TlsStream.flush` now + does both flushes and `exchange` calls it; a hermetic loopback test in + `tls_client_integration_test.zig` covers it. diff --git a/specs/milestone-4.md b/specs/milestone-4.md index b8ec22e..527262a 100644 --- a/specs/milestone-4.md +++ b/specs/milestone-4.md @@ -1920,3 +1920,18 @@ refinements (three review rounds; findings went 8 → 3 → 1 low): stock 0.16 — the `-Dlive` DoT case fails with `CertificateHostMismatch` against 1.1.1.1 while live DoH passes. Planned follow-up (own commit): a per-upstream `tls_name` for SNI + verification while dialing the IP. + +### Follow-up: per-upstream `tls_name` + +The stdlib limitation above is now worked around the way stubby and unbound +do it. `UpstreamServer` (not the `upstream` timeout section) gains +`tls_name: []const u8 = ""`, migration step 2 adds +`upstreams.tls_name TEXT NOT NULL DEFAULT ''`, and the ZON round trip carries +it like every other field. `validate.zig` adds `BadTlsName` (must pass +`dns.name.fromText`) and `TlsNameOnNonTlsUpstream` (a `tls_name` on a DoH +upstream is a config error, because DoH verifies by its url host and would +ignore the field). `DotClient` uses it for SNI and verification while still +dialing `endpoint.host`, and `cli.zig`'s check probe passes it through. The +`-Dlive` DoT test now uses `1.1.1.1` with `one.one.one.one` and passes; making +it pass also uncovered and fixed a missing socket flush in the DoT send path +(see the milestone-3 note). diff --git a/src/cli.zig b/src/cli.zig index 8100117..c186e6b 100644 --- a/src/cli.zig +++ b/src/cli.zig @@ -647,7 +647,7 @@ fn probeUpstreams(r: Runner, cfg: model.Config) !usize { break :doh doh.client(); }, .dot => dot: { - dot = dot_client.DotClient.init(endpoint, r.gpa, &bundle, &bundle_lock, .{ + dot = dot_client.DotClient.init(endpoint, server.tls_name, r.gpa, &bundle, &bundle_lock, .{ .tls_read = tls_buffers[0..chunk], .tls_write = tls_buffers[chunk .. 2 * chunk], .stream_read = tls_buffers[2 * chunk .. 3 * chunk], diff --git a/src/config/export.zig b/src/config/export.zig index f63fe5d..a4f0534 100644 --- a/src/config/export.zig +++ b/src/config/export.zig @@ -161,7 +161,7 @@ const seed_source: [:0]const u8 = \\ .groups = .{ .{ .name = "default" }, .{ .name = "kids", .safe_search = true } }, \\ .upstreams = .{ \\ .{ .url = "https://dns.example/dns-query", .priority = 10 }, - \\ .{ .url = "tls://dot.example:853", .priority = 20, .enabled = false }, + \\ .{ .url = "tls://dot.example:853", .priority = 20, .enabled = false, .tls_name = "dot.example" }, \\ }, \\ .clients = .{ .{ .ip = "fd00::1", .name = "tablet", .group = "kids" } }, \\ .client_prefixes = .{ .{ .prefix = "192.168.1.0/24", .group = "kids", .priority = 50 } }, @@ -252,6 +252,11 @@ test "readConfig, writeConfig, import and readConfig again produce an equal conf try testing.expectEqual(a.logging.level, b.logging.level); try testing.expectEqualStrings(a.web.password_hash, b.web.password_hash); try testing.expectEqual(a.groups.len, b.groups.len); + try testing.expectEqual(a.upstreams.len, b.upstreams.len); + for (a.upstreams, b.upstreams) |left, right| { + try testing.expectEqualStrings(left.url, right.url); + try testing.expectEqualStrings(left.tls_name, right.tls_name); + } try testing.expectEqual(a.rules.len, b.rules.len); try testing.expectEqualStrings(a.clients[0].ip, b.clients[0].ip); try testing.expectEqualStrings(a.forward_zones[0].resolver, b.forward_zones[0].resolver); diff --git a/src/config/import.zig b/src/config/import.zig index 99587fe..e02116e 100644 --- a/src/config/import.zig +++ b/src/config/import.zig @@ -388,7 +388,7 @@ const full_source: [:0]const u8 = \\ .groups = .{ .{ .name = "default" }, .{ .name = "kids", .safe_search = true } }, \\ .upstreams = .{ \\ .{ .url = "https://dns.example/dns-query", .priority = 10 }, - \\ .{ .url = "tls://dot.example:853", .priority = 20, .enabled = false }, + \\ .{ .url = "tls://dot.example:853", .priority = 20, .enabled = false, .tls_name = "dot.example" }, \\ }, \\ .clients = .{ .{ .ip = "FD00:0:0:0:0:0:0:1", .name = "tablet", .group = "kids" } }, \\ .client_prefixes = .{ .{ .prefix = "192.168.1.0/24", .group = "kids", .priority = 50 } }, @@ -449,6 +449,14 @@ test "importSource seeds a migrated database and group 'default' keeps id 1" { try testing.expectEqual(@as(i64, 2), try database.queryInt("SELECT count(*) FROM upstreams")); // The v6 client address was written in canonical form, not as typed. try testing.expectEqual(@as(i64, 1), try database.queryInt("SELECT count(*) FROM clients WHERE ip = 'fd00::1'")); + try testing.expectEqual( + @as(i64, 1), + try database.queryInt("SELECT count(*) FROM upstreams WHERE tls_name = 'dot.example'"), + ); + try testing.expectEqual( + @as(i64, 1), + try database.queryInt("SELECT count(*) FROM upstreams WHERE tls_name = ''"), + ); } test "applyToDb without force refuses a configured database and changes nothing" { diff --git a/src/config/model.zig b/src/config/model.zig index 804e119..6dbfe63 100644 --- a/src/config/model.zig +++ b/src/config/model.zig @@ -204,7 +204,16 @@ pub const BlocklistUpdate = struct { enabled: bool = true, interval_hours: u16 = pub const Group = struct { name: []const u8, safe_search: bool = false }; -pub const UpstreamServer = struct { url: []const u8, priority: i32 = 100, enabled: bool = true }; +pub const UpstreamServer = struct { + url: []const u8, + priority: i32 = 100, + enabled: bool = true, + /// DoT only. The DNS name used for SNI and certificate verification while + /// the connection still dials the URL's host. `std.crypto.Certificate` + /// matches dNSName SANs only, so a `tls://` upstream written as an IP + /// literal cannot verify without one. Empty means "verify by the URL host". + tls_name: []const u8 = "", +}; pub const Client = struct { ip: []const u8, name: []const u8 = "", group: []const u8 = "default" }; diff --git a/src/config/validate.zig b/src/config/validate.zig index c4e4fa9..56dffd9 100644 --- a/src/config/validate.zig +++ b/src/config/validate.zig @@ -31,6 +31,8 @@ pub const ValidateError = error{ NoUpstreams, BadUpstreamUrl, DuplicateUpstreamUrl, + BadTlsName, + TlsNameOnNonTlsUpstream, MissingDefaultGroup, DuplicateGroupName, UnknownGroup, @@ -400,6 +402,40 @@ fn canonical(scratch: Allocator, value: anytype) error{OutOfMemory}![]u8 { return scratch.dupe(u8, w.buffered()); } +/// A `tls_name` overrides SNI and certificate verification for a DoT upstream, +/// which is the only transport that needs it: DoH verifies by the url host and +/// the http client would ignore this field, so a `tls_name` there is a config +/// error rather than a setting with no effect. +fn checkTlsName( + diags: *Diagnostics, + server: model.UpstreamServer, + scheme: transport.Scheme, + index: usize, +) error{OutOfMemory}!void { + if (server.tls_name.len == 0) return; + + if (scheme != .dot) { + try diags.add( + error.TlsNameOnNonTlsUpstream, + "upstreams[{d}].tls_name", + .{index}, + "tls_name is only for a tls:// upstream; '{s}' verifies by its url host", + .{server.url}, + ); + return; + } + + _ = dns_name.fromText(server.tls_name) catch { + try diags.add( + error.BadTlsName, + "upstreams[{d}].tls_name", + .{index}, + "'{s}' is not a valid domain name", + .{server.tls_name}, + ); + }; +} + fn checkCollections(cfg: Config, diags: *Diagnostics, scratch: Allocator) error{OutOfMemory}!void { var group_names: StringSet = .empty; var has_default = false; @@ -430,7 +466,12 @@ fn checkCollections(cfg: Config, diags: *Diagnostics, scratch: Allocator) error{ var upstream_urls: StringSet = .empty; var enabled_upstreams: usize = 0; for (cfg.upstreams, 0..) |server, i| { - _ = transport.Endpoint.parse(server.url) catch { + // The scheme decides whether `tls_name` is meaningful, so the parse + // result is kept rather than discarded. An unparseable url reports only + // `BadUpstreamUrl`: what its scheme would have been is unknown. + if (transport.Endpoint.parse(server.url)) |endpoint| { + try checkTlsName(diags, server, endpoint.scheme, i); + } else |_| { try diags.add( error.BadUpstreamUrl, "upstreams[{d}].url", @@ -438,7 +479,7 @@ fn checkCollections(cfg: Config, diags: *Diagnostics, scratch: Allocator) error{ "'{s}' is not an https:// or tls:// endpoint", .{server.url}, ); - }; + } if (try markSeen(&upstream_urls, scratch, server.url)) { try diags.add( error.DuplicateUpstreamUrl, @@ -905,6 +946,37 @@ test "error.DuplicateUpstreamUrl" { try expectProblem(cfg, error.DuplicateUpstreamUrl, "upstreams[1].url"); } +test "a tls_name on a tls:// upstream validates cleanly" { + var cfg = baseConfig(); + cfg.upstreams = &.{.{ .url = "tls://1.1.1.1:853", .tls_name = "one.one.one.one" }}; + try expectClean(cfg); +} + +test "error.BadTlsName on a malformed name" { + var cfg = baseConfig(); + cfg.upstreams = &.{.{ .url = "tls://1.1.1.1:853", .tls_name = "one..one.one" }}; + try expectProblem(cfg, error.BadTlsName, "upstreams[0].tls_name"); + + var too_long = baseConfig(); + too_long.upstreams = &.{.{ .url = "tls://1.1.1.1:853", .tls_name = "a" ** 64 ++ ".example" }}; + try expectProblem(too_long, error.BadTlsName, "upstreams[0].tls_name"); +} + +test "error.TlsNameOnNonTlsUpstream on a DoH upstream" { + var cfg = baseConfig(); + cfg.upstreams = &.{.{ .url = "https://dns.example/dns-query", .tls_name = "dns.example" }}; + try expectProblem(cfg, error.TlsNameOnNonTlsUpstream, "upstreams[0].tls_name"); +} + +test "an empty tls_name is accepted on every scheme" { + var cfg = baseConfig(); + cfg.upstreams = &.{ + .{ .url = "https://dns.example/dns-query" }, + .{ .url = "tls://9.9.9.9:853" }, + }; + try expectClean(cfg); +} + test "error.MissingDefaultGroup" { var cfg = baseConfig(); cfg.groups = &.{.{ .name = "kids" }}; diff --git a/src/platform/tls_client.zig b/src/platform/tls_client.zig index fe5cc78..0843d12 100644 --- a/src/platform/tls_client.zig +++ b/src/platform/tls_client.zig @@ -233,6 +233,18 @@ pub const TlsStream = struct { return &self.client.writer; } + /// Pushes buffered plaintext all the way to the socket. + /// + /// Both flushes are required. `tls.Client.flush` only encrypts what the + /// plaintext writer holds into the socket writer's buffer and calls + /// `advance` (crypto/tls/Client.zig:999); it never flushes that writer, so + /// on its own it leaves the record sitting in this process. A caller that + /// then waits for a reply waits until the peer gives up. + pub fn flush(self: *TlsStream) std.Io.Writer.Error!void { + try self.client.writer.flush(); + try self.stream_writer.interface.flush(); + } + /// Sends close_notify and flushes the socket. Does not close the underlying /// stream; the caller owns it. pub fn close(self: *TlsStream) void { diff --git a/src/platform/tls_client_integration_test.zig b/src/platform/tls_client_integration_test.zig index 40cfd94..496a28e 100644 --- a/src/platform/tls_client_integration_test.zig +++ b/src/platform/tls_client_integration_test.zig @@ -108,3 +108,120 @@ test "live DoT handshake against cloudflare-dns.com" { }, } } + +// --------------------------------------------------------------------------- +// Hermetic: loopback only, no name resolution, no external host. +// --------------------------------------------------------------------------- + +const tls_server = @import("tls_server.zig"); + +/// The fixture certificate carries `DNS:localhost`, so this is the only name a +/// `TlsStream` can verify against it. +const fixture_host = "localhost"; + +const echo_message = "nxdns tls flush probe"; + +/// Reads one message and echoes it back. +fn echoOnce( + gpa: std.mem.Allocator, + ctx: *tls_server.ServerContext, + io: std.Io, + server: *net.Server, +) anyerror!void { + var stream = try server.accept(io); + defer stream.close(io); + + var read_buffer: [4096]u8 = undefined; + var write_buffer: [4096]u8 = undefined; + var server_stream: tls_server.ServerStream = undefined; + try server_stream.accept(gpa, ctx, io, &stream, &read_buffer, &write_buffer); + defer server_stream.close(gpa); + + var received: [echo_message.len]u8 = undefined; + try server_stream.reader().readSliceAll(&received); + try server_stream.writer().writeAll(&received); + try server_stream.writer().flush(); +} + +/// Writes through `TlsStream.flush` and waits for the echo. With only +/// `client.writer.flush()` the record would never leave this process and this +/// read would block until the budget expired. +fn flushAndEcho(io: std.Io, gpa: std.mem.Allocator, address: net.IpAddress) anyerror!void { + var stream = try address.connect(io, .{ .mode = .stream }); + defer stream.close(io); + + var bundle: Certificate.Bundle = .empty; + defer bundle.deinit(gpa); + var bundle_lock: std.Io.RwLock = .init; + + var read_buffer: [4096]u8 = undefined; + var write_buffer: [4096]u8 = undefined; + var stream_read_buffer: [tls.Client.min_buffer_len]u8 = undefined; + var stream_write_buffer: [tls.Client.min_buffer_len]u8 = undefined; + + var client: tls_client.TlsStream = undefined; + // The fixture certificate is self-signed, so the chain cannot verify; the + // host name still must match, which is what this test needs it to do. + try client.init(io, &stream, &bundle, &bundle_lock, gpa, .{ + .host = fixture_host, + .ca = .insecure_skip_verify, + .read_buffer = &read_buffer, + .write_buffer = &write_buffer, + .stream_read_buffer = &stream_read_buffer, + .stream_write_buffer = &stream_write_buffer, + }); + defer client.close(); + + try client.writer().writeAll(echo_message); + try client.flush(); + + var echoed: [echo_message.len]u8 = undefined; + try client.reader().readSliceAll(&echoed); + try std.testing.expectEqualStrings(echo_message, &echoed); +} + +test "TlsStream.flush puts the record on the wire" { + if (!build_options.integration) return error.SkipZigTest; + + const fixtures = @import("test_fixtures"); + const gpa = std.testing.allocator; + + var threaded: std.Io.Threaded = .init(gpa, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var ctx = try tls_server.ServerContext.init(gpa, fixtures.cert_pem, fixtures.key_pem); + defer ctx.deinit(gpa); + + const listen_address: net.IpAddress = .{ .ip4 = .loopback(0) }; + var server = try listen_address.listen(io, .{ .reuse_address = true }); + defer server.deinit(io); + + var server_task = try io.concurrent(echoOnce, .{ gpa, &ctx, io, &server }); + + // Raced against the budget: the failure this test guards against is a + // record that never leaves the process, which without a deadline would + // hang the run instead of failing it. + var outcomes: [2]Outcome = undefined; + var race: std.Io.Select(Outcome) = .init(io, &outcomes); + defer race.cancelDiscard(); + try race.concurrent(.exchange, flushAndEcho, .{ io, gpa, server.socket.address }); + try race.concurrent(.expiry, expire, .{ io, budget }); + + const client_result: anyerror!void = switch (try race.await()) { + .exchange => |result| result, + .expiry => |result| blk: { + try result; + break :blk error.TlsEchoTimedOut; + }, + }; + + // A client that never connects would leave the server blocked in `accept`. + const server_result = if (client_result) |_| + server_task.await(io) + else |_| + server_task.cancel(io); + + try client_result; + try server_result; +} diff --git a/src/storage/migrations.zig b/src/storage/migrations.zig index 8b9860b..1b471b7 100644 --- a/src/storage/migrations.zig +++ b/src/storage/migrations.zig @@ -18,10 +18,19 @@ const log = std.log.scoped(.migrations); pub const Step = struct { version: u32, sql: [:0]const u8 }; +/// Append only. Editing a released step — or `config_schema.ddl_v1` — would make +/// a fresh database and an upgraded one disagree, and nothing would detect it. pub const steps = [_]Step{ .{ .version = 1, .sql = config_schema.ddl_v1 }, + .{ .version = 2, .sql = ddl_v2 }, }; +/// The DoT verification name (`upstreams.tls_name`). Empty keeps the pre-step-2 +/// behavior: verify the certificate against the url host. +const ddl_v2: [:0]const u8 = + \\ALTER TABLE upstreams ADD COLUMN tls_name TEXT NOT NULL DEFAULT ''; +; + pub const target_version: u32 = steps[steps.len - 1].version; comptime { @@ -236,6 +245,63 @@ test "a stepwise upgrade applies only the new steps" { try testing.expectEqual(@as(i64, 1), try database.queryInt("SELECT count(*) FROM groups")); } +fn columnExists(database: *db.Db, table: []const u8, column: []const u8) !bool { + var stmt = try database.prepare("SELECT count(*) FROM pragma_table_info(?1) WHERE name = ?2"); + defer stmt.deinit(); + try stmt.bindText(1, table); + try stmt.bindText(2, column); + if (!try stmt.step()) return error.SqliteError; + return stmt.columnInt(0) != 0; +} + +test "a fresh database reaches version 2 with the tls_name column" { + var database = try openMigrated(); + defer database.close(); + + try testing.expectEqual(@as(u32, 2), try migrate(&database)); + try testing.expectEqual(@as(u32, 2), target_version); + try testing.expect(try columnExists(&database, "upstreams", "tls_name")); +} + +test "a version 1 database upgrades to 2 and keeps its rows with an empty tls_name" { + var database = try openMigrated(); + defer database.close(); + + const first = [_]Step{.{ .version = 1, .sql = config_schema.ddl_v1 }}; + try testing.expectEqual(@as(u32, 1), try migrateSteps(&database, &first)); + try testing.expect(!try columnExists(&database, "upstreams", "tls_name")); + try database.exec("INSERT INTO upstreams (url, priority, enabled) VALUES ('tls://1.1.1.1:853', 10, 1);"); + + try testing.expectEqual(@as(u32, 2), try migrate(&database)); + try testing.expectEqual(@as(u32, 2), try readVersion(&database)); + try testing.expect(try columnExists(&database, "upstreams", "tls_name")); + + var stmt = try database.prepare("SELECT url, tls_name FROM upstreams"); + defer stmt.deinit(); + try testing.expect(try stmt.step()); + try testing.expectEqualStrings("tls://1.1.1.1:853", stmt.columnText(0)); + try testing.expectEqualStrings("", stmt.columnText(1)); +} + +test "a failing step after step 2 rolls back the whole upgrade from version 1" { + var database = try openMigrated(); + defer database.close(); + + const first = [_]Step{.{ .version = 1, .sql = config_schema.ddl_v1 }}; + _ = try migrateSteps(&database, &first); + + const broken = [_]Step{ + steps[0], + steps[1], + .{ .version = 3, .sql = "CREATE TABLE third (" }, + }; + try testing.expectError(error.Unexpected, migrateSteps(&database, &broken)); + + // One transaction: the ALTER TABLE of step 2 went back with step 3. + try testing.expect(!try columnExists(&database, "upstreams", "tls_name")); + try testing.expectEqual(@as(u32, 1), try readVersion(&database)); +} + test "delete_order and content_tables name exactly the tables the schema creates" { var database = try openMigrated(); defer database.close(); diff --git a/src/storage/repositories/upstreams_repo.zig b/src/storage/repositories/upstreams_repo.zig index 7095a85..1549ae3 100644 --- a/src/storage/repositories/upstreams_repo.zig +++ b/src/storage/repositories/upstreams_repo.zig @@ -18,7 +18,9 @@ const InsertContext = context.InsertContext; /// Every string in the result is a heap copy owned by `gpa`. pub fn listUpstreams(database: *db.Db, gpa: Allocator) db.Error!std.ArrayList(model.UpstreamServer) { - var stmt = try database.prepare("SELECT url, priority, enabled FROM upstreams ORDER BY priority, url"); + var stmt = try database.prepare( + "SELECT url, priority, enabled, tls_name FROM upstreams ORDER BY priority, url", + ); defer stmt.deinit(); var out: std.ArrayList(model.UpstreamServer) = .empty; @@ -31,22 +33,35 @@ pub fn listUpstreams(database: *db.Db, gpa: Allocator) db.Error!std.ArrayList(mo const url = try stmt.columnTextAlloc(gpa, 0); errdefer gpa.free(url); const priority = std.math.cast(i32, stmt.columnInt(1)) orelse return error.Mismatch; - try out.append(gpa, .{ .url = url, .priority = priority, .enabled = stmt.columnBool(2) }); + const tls_name = try stmt.columnTextAlloc(gpa, 3); + errdefer gpa.free(tls_name); + try out.append(gpa, .{ + .url = url, + .priority = priority, + .enabled = stmt.columnBool(2), + .tls_name = tls_name, + }); } return out; } pub fn freeUpstreams(gpa: Allocator, items: []const model.UpstreamServer) void { - for (items) |item| gpa.free(item.url); + for (items) |item| { + gpa.free(item.url); + gpa.free(item.tls_name); + } } pub fn insertUpstream(database: *db.Db, item: model.UpstreamServer, ctx: InsertContext) db.Error!void { _ = ctx; - var stmt = try database.prepare("INSERT INTO upstreams (url, priority, enabled) VALUES (?1, ?2, ?3)"); + var stmt = try database.prepare( + "INSERT INTO upstreams (url, priority, enabled, tls_name) VALUES (?1, ?2, ?3, ?4)", + ); defer stmt.deinit(); try stmt.bindText(1, item.url); try stmt.bindInt(2, item.priority); try stmt.bindBool(3, item.enabled); + try stmt.bindText(4, item.tls_name); try stmt.exec(); } @@ -75,7 +90,12 @@ fn openMigrated() !db.Db { fn seedUpstreams(database: *db.Db) !void { const ctx: InsertContext = .{}; try insertUpstream(database, .{ .url = "https://dns.example/dns-query", .priority = 50 }, ctx); - try insertUpstream(database, .{ .url = "tls://1.1.1.1:853", .priority = 10, .enabled = false }, ctx); + try insertUpstream(database, .{ + .url = "tls://1.1.1.1:853", + .priority = 10, + .enabled = false, + .tls_name = "one.one.one.one", + }, ctx); try insertUpstream(database, .{ .url = "https://a.example/dns-query", .priority = 50 }, ctx); } @@ -92,7 +112,10 @@ test "upstreams round-trip in priority then url order" { try testing.expectEqualStrings("tls://1.1.1.1:853", items.items[0].url); try testing.expectEqual(@as(i32, 10), items.items[0].priority); try testing.expect(!items.items[0].enabled); + try testing.expectEqualStrings("one.one.one.one", items.items[0].tls_name); try testing.expectEqualStrings("https://a.example/dns-query", items.items[1].url); + // An upstream inserted without one reads back as the empty column default. + try testing.expectEqualStrings("", items.items[1].tls_name); try testing.expectEqual(@as(i32, 50), items.items[1].priority); try testing.expect(items.items[1].enabled); try testing.expectEqualStrings("https://dns.example/dns-query", items.items[2].url); diff --git a/src/upstream/dot_client.zig b/src/upstream/dot_client.zig index 7b78db0..5c1a4d9 100644 --- a/src/upstream/dot_client.zig +++ b/src/upstream/dot_client.zig @@ -45,6 +45,12 @@ pub fn resolveAddress(endpoint: transport.Endpoint) ResolveError!net.IpAddress { pub const DotClient = struct { endpoint: transport.Endpoint, + /// SNI, and the name matched against the leaf certificate. The dial target + /// stays `endpoint.host`, so this is what lets an upstream configured as an + /// IP literal verify: `std.crypto.Certificate.Parsed.verifyHostName` matches + /// dNSName SANs only and never an IP SAN, so `tls://1.1.1.1:853` alone is + /// always `error.CertificateHostMismatch`. Borrowed, like `endpoint`. + verify_name: []const u8, gpa: std.mem.Allocator, /// Caller-owned, shared across endpoints. bundle: *Certificate.Bundle, @@ -66,8 +72,12 @@ pub const DotClient = struct { /// A `.doh` endpoint or an undersized buffer is a wiring bug in this /// process, not a runtime condition, so both are assertions. + /// + /// An empty `tls_name` keeps the endpoint's own host as the verification + /// name, which is correct whenever the url already carries a DNS name. pub fn init( endpoint: transport.Endpoint, + tls_name: []const u8, gpa: std.mem.Allocator, bundle: *Certificate.Bundle, bundle_lock: *std.Io.RwLock, @@ -80,6 +90,7 @@ pub const DotClient = struct { std.debug.assert(buffers.stream_write.len >= tls.Client.min_buffer_len); return .{ .endpoint = endpoint, + .verify_name = if (tls_name.len == 0) endpoint.host else tls_name, .gpa = gpa, .bundle = bundle, .bundle_lock = bundle_lock, @@ -149,7 +160,7 @@ pub const DotClient = struct { tls_stream.stream_reader.err = null; tls_stream.stream_writer.err = null; tls_stream.init(io, &stream, self.bundle, self.bundle_lock, self.gpa, .{ - .host = self.endpoint.host, + .host = self.verify_name, .ca = .system, .read_buffer = self.buffers.tls_read, .write_buffer = self.buffers.tls_write, @@ -157,8 +168,9 @@ pub const DotClient = struct { .stream_write_buffer = self.buffers.stream_write, }) catch |err| { const cause = concreteHandshake(&tls_stream, err); - log.warn("dot upstream {s}: TLS handshake failed: {s} ({t})", .{ + log.warn("dot upstream {s}: TLS handshake as \"{s}\" failed: {s} ({t})", .{ self.endpoint.url, + self.verify_name, @errorName(cause), tls_client.classify(cause), }); @@ -170,7 +182,10 @@ pub const DotClient = struct { const prefix = framePrefix(@intCast(query.len)); writer.writeAll(&prefix) catch |err| return sendFailure(&tls_stream, err); writer.writeAll(query) catch |err| return sendFailure(&tls_stream, err); - writer.flush() catch |err| return sendFailure(&tls_stream, err); + // `TlsStream.flush`, not `writer.flush`: the latter leaves the encrypted + // record in the socket writer's buffer and the query never leaves this + // process. + tls_stream.flush() catch |err| return sendFailure(&tls_stream, err); const reader = tls_stream.reader(); var prefix_bytes: [prefix_len]u8 = undefined; @@ -463,7 +478,7 @@ test "DotClient satisfies the Client interface" { // `init` asserts `endpoint.scheme == .dot`; a `.doh` endpoint trips // `std.debug.assert`, which a test cannot catch in-process. - var dot: DotClient = .init(try .parse("tls://9.9.9.9:853"), gpa, &bundle, &bundle_lock, .{ + var dot: DotClient = .init(try .parse("tls://9.9.9.9:853"), "", gpa, &bundle, &bundle_lock, .{ .tls_read = buffer[0..chunk], .tls_write = buffer[chunk .. 2 * chunk], .stream_read = buffer[2 * chunk .. 3 * chunk], @@ -476,3 +491,34 @@ test "DotClient satisfies the Client interface" { const iface: transport.Client = dot.client(); try testing.expectEqual(@as(*anyopaque, @ptrCast(&dot)), iface.ptr); } + +test "a tls_name replaces the verification name and leaves the dial target alone" { + const gpa = testing.allocator; + + const buffer = try gpa.alloc(u8, 4 * tls.Client.min_buffer_len); + defer gpa.free(buffer); + const chunk = tls.Client.min_buffer_len; + + var bundle: Certificate.Bundle = .empty; + defer bundle.deinit(gpa); + var bundle_lock: std.Io.RwLock = .init; + + const buffers: DotClient.Buffers = .{ + .tls_read = buffer[0..chunk], + .tls_write = buffer[chunk .. 2 * chunk], + .stream_read = buffer[2 * chunk .. 3 * chunk], + .stream_write = buffer[3 * chunk ..], + }; + + const endpoint: transport.Endpoint = try .parse("tls://1.1.1.1:853"); + const named: DotClient = .init(endpoint, "one.one.one.one", gpa, &bundle, &bundle_lock, buffers); + try testing.expectEqualStrings("one.one.one.one", named.verify_name); + try testing.expectEqualStrings("1.1.1.1", named.endpoint.host); + + const address = try resolveAddress(named.endpoint); + try testing.expectEqualSlices(u8, &.{ 1, 1, 1, 1 }, &address.ip4.bytes); + try testing.expectEqual(@as(u16, 853), address.ip4.port); + + const plain: DotClient = .init(endpoint, "", gpa, &bundle, &bundle_lock, buffers); + try testing.expectEqualStrings("1.1.1.1", plain.verify_name); +} diff --git a/src/upstream/dot_client_live_test.zig b/src/upstream/dot_client_live_test.zig index f35ce06..d4aee30 100644 --- a/src/upstream/dot_client_live_test.zig +++ b/src/upstream/dot_client_live_test.zig @@ -26,10 +26,14 @@ const query_bytes = "\x07example\x03com\x00\x00\x01\x00\x01"; /// This machine's IPv6 egress is dead and upstream name resolution is out of -/// scope, so the documented anycast IPv4 literal is used. Cloudflare's -/// certificate carries 1.1.1.1 as an IP SAN, so full verification still applies. +/// scope, so the documented anycast IPv4 literal is used. const upstream_url = "tls://1.1.1.1:853"; +/// The name Cloudflare publishes for this endpoint. Without it the handshake is +/// `error.CertificateHostMismatch`: `std.crypto.Certificate` matches dNSName +/// SANs only, so the IP SAN on the leaf certificate is never consulted. +const upstream_tls_name = "one.one.one.one"; + const Outcome = union(enum) { exchange: anyerror!usize, expiry: std.Io.Cancelable!void, @@ -47,6 +51,7 @@ fn runExchange(io: std.Io, params: Params) anyerror!usize { const endpoint: transport.Endpoint = try .parse(upstream_url); var client: dot_client.DotClient = .init( endpoint, + upstream_tls_name, params.gpa, params.bundle, params.bundle_lock,