milestone 13 discrepancies: redact credentials from urls in logs, metrics and cli output

src/upstream/dot_client.zig

@@ -16,11 +16,59 @@ const net = std.Io.net;
 const tls = std.crypto.tls;
 const Certificate = std.crypto.Certificate;
 
+const safe_url = @import("../safe_url.zig");
 const transport = @import("transport.zig");
 const tls_client = @import("../platform/tls_client.zig");
 
 const log = std.log.scoped(.dot_client);
 
+/// One diagnostic line about one client, as a value rather than a format string
+/// repeated at each call site.
+///
+/// Two reasons, in that order. The url is redacted in exactly one place, so a
+/// line added later cannot print it whole — the defect this type closes was four
+/// call sites each formatting `endpoint.url` with `{s}`, missed by three review
+/// rounds because each looked like the three beside it. And a `std.log` line is
+/// not readable from a unit test under the default test runner, which installs
+/// its own `std_options`; the tests below read this value instead of stderr.
+const Diagnostic = struct {
+    endpoint: transport.Endpoint,
+    detail: Detail,
+
+    const Detail = union(enum) {
+        /// `resolveAddress` refused the host.
+        not_an_ip_literal,
+        connect_failed: anyerror,
+        handshake_failed: Handshake,
+        bundle_load_failed: anyerror,
+
+        const Handshake = struct {
+            verify_name: []const u8,
+            cause: anyerror,
+        };
+    };
+
+    pub fn format(self: Diagnostic, w: *std.Io.Writer) std.Io.Writer.Error!void {
+        try w.print("dot upstream {f}: ", .{safe_url.redactQuoted(self.endpoint.url)});
+        switch (self.detail) {
+            // The redacted url ends in the host and the port, so naming the host
+            // again would add nothing but an unredacted copy of it.
+            .not_an_ip_literal => try w.writeAll("host is not an IP literal"),
+            .connect_failed => |err| try w.print("connect failed: {s}", .{@errorName(err)}),
+            // `verify_name` is a host name rather than a url, so it carries no
+            // component redaction could drop. It goes through `quoteText` for
+            // what that does to any operator-supplied string: it delimits it,
+            // escapes it and bounds it.
+            .handshake_failed => |hs| try w.print("TLS handshake as {f} failed: {s} ({t})", .{
+                safe_url.quoteText(hs.verify_name),
+                @errorName(hs.cause),
+                tls_client.classify(hs.cause),
+            }),
+            .bundle_load_failed => |err| try w.print("CA bundle load failed: {s}", .{@errorName(err)}),
+        }
+    }
+};
+
 pub const ResolveError = error{ConnectFailed};
 
 /// DoT endpoints take IP literals. Name resolution for upstreams is out of
@@ -89,6 +137,10 @@ pub const DotClient = struct {
         return .{ .ptr = self, .exchangeFn = exchangeFn };
     }
 
+    fn diagnose(self: *const DotClient, detail: Diagnostic.Detail) Diagnostic {
+        return .{ .endpoint = self.endpoint, .detail = detail };
+    }
+
     fn exchangeFn(
         ptr: *anyopaque,
         io: std.Io,
@@ -119,20 +171,14 @@ pub const DotClient = struct {
         if (query.len > transport.max_message_len) return error.BufferTooSmall;
 
         const address = resolveAddress(self.endpoint) catch |err| {
-            log.warn("dot upstream {s}: host \"{s}\" is not an IP literal", .{
-                self.endpoint.url,
-                self.endpoint.host,
-            });
+            log.warn("{f}", .{self.diagnose(.not_an_ip_literal)});
             return err;
         };
 
         try self.ensureBundle(io);
 
         var stream = address.connect(io, .{ .mode = .stream }) catch |err| {
-            log.debug("dot upstream {s}: connect failed: {s}", .{
-                self.endpoint.url,
-                @errorName(err),
-            });
+            log.debug("{f}", .{self.diagnose(.{ .connect_failed = err })});
             return mapPhase(err, error.ConnectFailed);
         };
         defer closeStream(io, &stream);
@@ -155,12 +201,10 @@ 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 as \"{s}\" failed: {s} ({t})", .{
-                self.endpoint.url,
-                self.verify_name,
-                @errorName(cause),
-                tls_client.classify(cause),
-            });
+            log.warn("{f}", .{self.diagnose(.{ .handshake_failed = .{
+                .verify_name = self.verify_name,
+                .cause = cause,
+            } })});
             return mapPhase(cause, error.TlsFailed);
         };
         defer closeTls(io, &tls_stream);
@@ -214,10 +258,7 @@ pub const DotClient = struct {
         self.bundle.rescan(self.gpa, io, std.Io.Clock.real.now(io)) catch |err| {
             self.bundle.deinit(self.gpa);
             self.bundle.* = .empty;
-            log.warn("dot upstream {s}: CA bundle load failed: {s}", .{
-                self.endpoint.url,
-                @errorName(err),
-            });
+            log.warn("{f}", .{self.diagnose(.{ .bundle_load_failed = err })});
             return mapPhase(err, error.TlsFailed);
         };
     }
@@ -286,6 +327,102 @@ fn receiveFailure(stream: *tls_client.TlsStream, err: anyerror) transport.Exchan
 
 const testing = std.testing;
 
+fn expectDiagnostic(expected: []const u8, url: []const u8, detail: Diagnostic.Detail) !void {
+    var buf: [8 * safe_url.max_len]u8 = undefined;
+    const line = try std.fmt.bufPrint(&buf, "{f}", .{Diagnostic{
+        .endpoint = .{ .scheme = .dot, .url = url, .host = "host.example", .port = 853, .path = "/" },
+        .detail = detail,
+    }});
+    try testing.expectEqualStrings(expected, line);
+}
+
+test "every diagnostic line redacts the url it names the upstream by" {
+    // The endpoints are built by hand rather than parsed, on purpose.
+    // `Endpoint.parse` refuses `@`, `?` and `#` in the authority and refuses a
+    // `.dot` path other than "/", so no url reaching this client through it can
+    // carry a credential in a component `redact` drops. That is a property of a
+    // parser one file away, not of this file, and these lines used to print
+    // whatever `endpoint.url` held. The redaction is what keeps the parser's
+    // rules from being load-bearing here.
+    try expectDiagnostic(
+        "dot upstream 'tls://dns.example': host is not an IP literal",
+        "tls://user:hunter2@dns.example/abcd12",
+        .not_an_ip_literal,
+    );
+    try expectDiagnostic(
+        "dot upstream 'tls://dns.example:8853': connect failed: ConnectionRefused",
+        "tls://dns.example:8853/abcd12",
+        .{ .connect_failed = error.ConnectionRefused },
+    );
+    try expectDiagnostic(
+        "dot upstream 'tls://dns.example': CA bundle load failed: FileNotFound",
+        "tls://dns.example/abcd12?apikey=s3cr3t",
+        .{ .bundle_load_failed = error.FileNotFound },
+    );
+    try expectDiagnostic(
+        "dot upstream 'tls://dns.example': TLS handshake as 'one.one.one.one' failed: " ++
+            "CertificateHostMismatch (certificate)",
+        "tls://token@dns.example/abcd12",
+        .{ .handshake_failed = .{
+            .verify_name = "one.one.one.one",
+            .cause = error.CertificateHostMismatch,
+        } },
+    );
+}
+
+test "a diagnostic escapes and bounds the operator-supplied text it prints" {
+    // A control byte in either field would forge a second record on the one
+    // output `std.log`'s own sink does not reach, and an unbounded host or
+    // verification name would write an unbounded line.
+    try expectDiagnostic(
+        "dot upstream 'tls://dns.example\\n2026-01-01 ERROR forged': host is not an IP literal",
+        "tls://dns.example\n2026-01-01 ERROR forged",
+        .not_an_ip_literal,
+    );
+    try expectDiagnostic(
+        "dot upstream 'tls://dns.example': TLS handshake as 'a\\nb' failed: TlsAlert (handshake)",
+        "tls://dns.example",
+        .{ .handshake_failed = .{ .verify_name = "a\nb", .cause = error.TlsAlert } },
+    );
+
+    // Doubling every operator-supplied field writes the same line, and each
+    // field is built from the three byte costs at once: a printable byte spends
+    // one character of the budget, `\n` spends two and `\x00` spends four. That
+    // expansion is why `safe_url.max_len` counts printed characters rather than
+    // source bytes, so the bound holds against the widest escape rather than in
+    // spite of it.
+    const long = "h\n\x00" ** (2 * safe_url.max_len);
+    const longer = long ** 2;
+    var short_buf: [16 * safe_url.max_len]u8 = undefined;
+    var long_buf: [16 * safe_url.max_len]u8 = undefined;
+    try testing.expectEqualStrings(
+        try std.fmt.bufPrint(&short_buf, "{f}", .{Diagnostic{
+            .endpoint = .{ .scheme = .dot, .url = "tls://" ++ long, .host = long, .port = 853, .path = "/" },
+            .detail = .{ .handshake_failed = .{ .verify_name = long, .cause = error.TlsAlert } },
+        }}),
+        try std.fmt.bufPrint(&long_buf, "{f}", .{Diagnostic{
+            .endpoint = .{ .scheme = .dot, .url = "tls://" ++ longer, .host = longer, .port = 853, .path = "/" },
+            .detail = .{ .handshake_failed = .{ .verify_name = longer, .cause = error.TlsAlert } },
+        }}),
+    );
+}
+
+test "a diagnostic keeps what a parsed DoT url carries" {
+    // The limit, pinned so it stays visible: a NextDNS DoT upstream is
+    // `tls://abcd12.dns.nextdns.io`, whose hostname is the whole account
+    // identifier. Redaction cannot remove it without leaving no host and an
+    // unactionable line. See `safe_url.SafeUrl`.
+    var buf: [8 * safe_url.max_len]u8 = undefined;
+    const line = try std.fmt.bufPrint(&buf, "{f}", .{Diagnostic{
+        .endpoint = try .parse("tls://abcd12.dns.nextdns.io"),
+        .detail = .not_an_ip_literal,
+    }});
+    try testing.expectEqualStrings(
+        "dot upstream 'tls://abcd12.dns.nextdns.io': host is not an IP literal",
+        line,
+    );
+}
+
 test "resolveAddress accepts IP literals" {
     const v4 = try resolveAddress(try .parse("tls://1.1.1.1:853"));
     try testing.expectEqual(@as(u16, 853), v4.ip4.port);