Conversation
There was a problem hiding this comment.
Pull request overview
Adds RFC9230 Oblivious DoH (ODoH) support across the project, including a target mode in dohd, a new standalone proxy (dohproxyd), and an ODoH-capable client gateway (ns2dohd), plus accompanying tooling, tests, docs, and CI/install targets.
Changes:
- Implement ODoH message/config handling, key generation tooling, and proxy authorization allowlisting.
- Refactor event loop to epoll and introduce fixed-size memory pools + O(1) client lookup in
dohd. - Add unit/integration/stress test harnesses, manpages, install targets, and CI self-test workflow.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/odoh-keygen.c | New key/config generator producing dohd-compatible ODoH materials |
| tools/Makefile | Build targets for odoh-keygen (release/debug/asan/clean) |
| test/valgrind_test.sh | Valgrind-based leak detection runner for dohd |
| test/test_url64_extended.c | Additional url64 decoder edge-case unit tests |
| test/test_request_leaks.sh | Integration script to exercise request lifecycle/memory behavior |
| test/test_mempool.c | Unit tests for the new fixed-size memory pool |
| test/test_heap.c | Unit tests for heap timer scheduling behavior |
| test/test_dns_parser.c | Unit tests for DNS parsing/TTL extraction logic |
| test/test_concurrent.sh | Concurrent request stress test script |
| test/test_client_lifecycle.sh | Client connect/disconnect lifecycle stress test |
| test/stress_test.sh | Multi-worker stress runner that builds/launches dohd |
| test/stress_flood.sh | Connection flood stress runner for resource exhaustion behavior |
| test/stress_escalate.sh | Escalating load test to find failure thresholds |
| test/stress_chaos.sh | Chaos-style randomized request pattern stress runner |
| test/run_asan_test.sh | ASAN runner script for dohd |
| test/gen_test_certs.sh | Helper to generate self-signed TLS certs for tests |
| test/README.md | Documentation for unit/integration/stress/leak tests |
| test/Makefile | Expanded test build + check/integration/stress/valgrind targets |
| src/proxy_auth.h | API for proxy certificate public-key hash allowlist |
| src/proxy_auth.c | Loads allowlist from directory + checks peer certificate key hash |
| src/odoh.h | Public ODoH config/message APIs and context structs |
| src/odoh.c | HPKE-based ODoH query/response encryption/decryption implementation |
| src/mempool.h | Fixed-size pool API and stats struct |
| src/mempool.c | Pool implementation with freelist + bitmap double-free detection |
| src/libevquick.c | Switch event multiplexing from poll-style to epoll + deferred frees |
| src/dohd.c | ODoH target mode, proxy auth gate, request timeouts, mempool + client hash table integration |
| src/Makefile | Link in new modules (mempool/odoh/proxy_auth) for dohd |
| proxy/Makefile | Build rules for new dohproxyd binary |
| ns2dohd/README.md | Docs for ns2dohd including ODoH mode usage |
| ns2dohd/Makefile | Build rules for new ns2dohd binary (including odoh.o) |
| man/odoh-keygen.1 | Manpage for ODoH key/config generator |
| man/ns2dohd.8 | Manpage for the DNS-to-DoH gateway daemon |
| man/dohproxyd.8 | Manpage for the DoH/ODoH proxy daemon |
| man/dohd.8 | Manpage updates documenting ODoH target mode and proxy allowlist |
| examples/odoh/selftest-proxy-target-curl.sh | Local proxy+target self-test script using curl |
| examples/odoh/dodh_targets | Example legacy target list file for proxy |
| examples/odoh/deploy-target-example.sh | Example target deployment script/flags |
| examples/odoh/deploy-proxy-example.sh | Example proxy deployment script/flags |
| README.md | Top-level docs for ns2dohd/dohproxyd + ODoH guidance and examples |
| Makefile | Top-level build/test/stress/install/uninstall targets + subdir builds |
| .gitignore | Ignore additional built artifacts and test outputs |
| .github/workflows/pr-build-install-selftest.yml | CI: build+install to staging prefix + run ODoH self-test |
Comments suppressed due to low confidence (1)
src/dohd.c:680
- dns_send_request_h2() returns -1 for several error cases (e.g., ODoH decrypt failure, non-query DNS header) without destroying the req_slot or sending an HTTP/2 error response. Since the caller ignores the return value, this leaves the stream/user_data and request resources hanging until client disconnect. Ensure these error paths submit an appropriate HTTP error (e.g., 400/415/500) and call dohd_destroy_request(req).
if (oblivion_mode) {
if (!req->is_odoh)
return -1;
if (odoh_target_decrypt_query(&odoh_target,
req->h2_request_buffer, (uint16_t)req->h2_request_len,
plain_dns, &plain_len, &req->odoh_ctx) != 0) {
return -1;
}
if (plain_len > sizeof(req->h2_request_buffer))
return -1;
memcpy(req->h2_request_buffer, plain_dns, plain_len);
req->h2_request_len = plain_len;
}
/* Parse DNS header: only check the qr flag. */
hdr = (struct dns_header *)req->h2_request_buffer;
if (hdr->qr != 0) {
return -1;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| req->ev_dns = evquick_addevent(req->dns_sd, EVQUICK_EV_READ, dohd_reply, | ||
| NULL, req); | ||
| ret = sendto(req->dns_sd, req->h2_request_buffer, req->h2_request_len, 0, |
There was a problem hiding this comment.
dns_send_request_h2() does not check whether evquick_addevent() succeeded. If event registration fails, the request will never receive a DNS reply and will only complete via timeout, even if the upstream responds. Treat a NULL return as a hard error (increment stats as needed) and destroy the request immediately (and ideally send an HTTP error).
| ctx = calloc(1, sizeof(struct evquick_ctx)); | ||
| if (!ctx) | ||
| return NULL; | ||
| ctx->giveup = 0; | ||
| ctx->timers = heap_init(); | ||
| if (!ctx->timers) | ||
| return NULL; | ||
| if(pipe(ctx->time_machine) < 0) | ||
| return NULL; | ||
| (void)yes; | ||
| fcntl(ctx->time_machine[1], F_SETFL, O_NONBLOCK); | ||
|
|
||
| /* Create epoll instance */ | ||
| ctx->epfd = epoll_create1(0); | ||
| if (ctx->epfd < 0) { | ||
| perror("epoll_create1"); | ||
| return NULL; | ||
| } | ||
|
|
||
| /* Add time_machine pipe to epoll for timer wakeups */ | ||
| ev.events = EPOLLIN; | ||
| ev.data.ptr = NULL; /* NULL ptr indicates time_machine */ | ||
| if (epoll_ctl(ctx->epfd, EPOLL_CTL_ADD, ctx->time_machine[0], &ev) < 0) { | ||
| perror("epoll_ctl time_machine"); | ||
| close(ctx->epfd); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
evquick_init() has multiple early-return error paths after allocating ctx/timers/pipe/epoll that don't free already-acquired resources (ctx, heap, time_machine fds, epfd). This leaks resources if initialization fails. Consider a single cleanup label that closes fds, frees heap/timers, and frees ctx before returning NULL.
| asan: CFLAGS+=-fsanitize=address | ||
| asan: LDFLAGS+=-fsanitize=address | ||
| asan: odoh-keygen |
There was a problem hiding this comment.
In the tools Makefile, the ASAN targets append flags without a separating space (CFLAGS+=-fsanitize=address, LDFLAGS+=-fsanitize=address). This results in concatenated/invalid compiler flags. Add a space after += (matching the other targets).
| int fd = open(path, O_WRONLY | O_APPEND, 0644); | ||
| ssize_t w; | ||
| if (fd < 0) { | ||
| fprintf(stderr, "Cannot append to %s: %s\n", path, strerror(errno)); | ||
| return -1; | ||
| } | ||
| w = write(fd, pub, pub_len); | ||
| if (w != (ssize_t)pub_len) { | ||
| close(fd); | ||
| fprintf(stderr, "Cannot append public key to %s\n", path); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
write_odoh_config() appends the public key with a single write() and assumes it writes pub_len bytes. write() can legally return a short write; this would truncate the config blob. Use the same looped-write logic as write_file() (or write header+pub in one buffered write) when appending the public key.
1 participant
• Added RFC9230 ODoH support:
dohd -Oacts as Target (accepts application/oblivious-dns-message)ns2dohd -Oacts as Clientdohproxydis Proxy (ODoH + legacy DoH).Includes proxy auth allowlist, key/config tooling, docs/manpages, install targets, and self-test scripts.