Skip to content

feat(system-tests): let the driver reach the local ic-gateway by name over verified HTTPS - #11276

Open
basvandijk wants to merge 13 commits into
bas/local-backend-nestedfrom
bas/local-backend-driver-dns
Open

feat(system-tests): let the driver reach the local ic-gateway by name over verified HTTPS#11276
basvandijk wants to merge 13 commits into
bas/local-backend-nestedfrom
bas/local-backend-driver-dns

Conversation

@basvandijk

@basvandijk basvandijk commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Third PR in the stack. Builds on #11270 (the local ic-gateway serving a dev-CA
certificate on a resolvable domain) and #11265 (nested nodes registering through
it).

On the Local backend, a driver-side client could not reach the gateway the way it
does on Farm. Two workarounds covered for that — resolve_override_for_url for
the name, and root_certificate() + add_root_certificate for the certificate —
and both had to be repeated at every call site. This PR makes name resolution and
trust properties of the environment instead, so all of it goes away. Neither
nns_dapp_test nor the sdk asset helper now carries any backend-specific client
setup: they build a plain client and GET the URL.

1. The driver had no resolver at all

ensure_administrable_netns puts the driver in a network namespace it owns, but
not a mount namespace — so it kept reading the host's /etc/resolv.conf, whose
nameserver is unreachable from that netns. Driver-side DNS was not merely
overridden by the .resolve() callers; it was dead.

It now unshares CLONE_NEWNS as well and bind-mounts a generated resolv.conf
naming IPV6_NAME_SERVERS over /etc/resolv.conf, so the driver resolves
in-group names through the group's own dnsmasq exactly like the VMs do —
dnsmasq already binds those addresses on the group bridge inside that very
netns.

2. dnsmasq answered no wildcards

add_dns_record appends to the --addn-hosts file, which holds exact names
only. Farm's playnet DNS gives the gateway domain an apex record plus * and
*.raw CNAMEs, because clients address canisters as <canister id>.<domain>
and <canister id>.raw.<domain>. Locally those subdomains resolved nowhere —
for clients inside a VM as much as for the driver.

add_wildcard_dns_record answers for a name and every subdomain of it at any
depth: one --address option covering all three Farm records. It takes the whole
address set at once because a record, not an address, is what costs a restart.

A restart is what it costs, since --address is a command-line option and
nothing re-reads those (SIGHUP re-reads the hosts-shaped files and
--servers-file, and a servers-file admits nothing but server/rev-server).
So the records live in a file start_dnsmasq turns back into options,
create_group takes over truncating them, and stop_dnsmasq now waits for
dnsmasq to exit — it binds its listeners before daemonizing, so a restart
racing the old instance for UDP port 53 fails outright. Restarting is safe for
VMs already up: GuestOS nodes are statically configured and never consult the RA,
a SLAAC'd address outlives the gap by far, DHCPv4 leases live in the lease-file
dnsmasq re-reads at startup, and the only real loss is its DNS cache.

3. Trust was configured per client, not per environment

Every dev IC-OS image trusts the dev root CA, via update-ca-certificates in the
output_dev stage of the GuestOS and HostOS Dockerfiles. The driver did not, so
each of its clients had to add the CA to its own roots.

It now installs the CA once, at startup, by bind-mounting
/etc/ssl/certs/ca-certificates.crt with the CA appended over the original — in
the mount namespace section 1 already created. That path is load-bearing through
a chain nothing type-checks (ClientBuilder::build
rustls_platform_verifier::Verifierrustls_native_certs
openssl_probe::probe), so the const carries it in a comment; rs/tests/BUILD.bazel
already builds a bundle at the same path for the colocated driver image for the
same reason.

It does not set SSL_CERT_FILE, which would be the wrong tool:
rustls-native-certs then loads only what that names and skips the platform store
entirely, so it would narrow the driver's trust rather than widen it, breaking
the clients that need public roots (Farm::new, and the log-upload client in
group.rs). std::env::set_var is also unsafe in edition 2024 and racy against
threads building clients, which a mount is not.

It does, however, honour the variable. system_ca_bundle resolves the bundle
the way rustls_native_certs::load_native_certs does — SSL_CERT_FILE when set,
openssl_probe's first Linux candidate otherwise — and the append, the mount and
the read-back all target that path, so the runner's own roots are kept rather than
replaced. An earlier revision refused to run when the variable was set, which
passed locally and failed every _local target on CI, where the Namespace runner
sets it. Hard-coding the default is safe because it is probe()'s first candidate:
if it exists, it is the one chosen, so the mount cannot land on a bundle the
verifier ignores.

The one shape that still refuses is SSL_CERT_DIR without SSL_CERT_FILE:
rustls-native-certs then reads only those directories and no bundle at all, so
there is nothing to append to — and a file cannot be added to a directory the
driver does not own. It fails loudly rather than quietly omitting the CA.

What the mount does not reach: a consumer using OpenSSL CApath alone, which
resolves <subject hash>.0 symlinks and never scans the bundle. Nothing in the
driver's process tree does that, and a plainly named file in the directory would
not have served it either — only a hash-named symlink would, which is what
update-ca-certificates generates and what a bind mount cannot create.

The append is verbatim-then-add, so it can only widen trust. The result is read
back before returning, because every step can succeed while leaving the CA
unreachable — a mount that did not take, a short write, a stale target — and none
of that would surface until a TLS handshake failed six minutes into a test.

Verification

rust-lint.sh, buildifier and bazel build //... --nobuild all clean, and:

target
//rs/tests/sdk:dfx_smoke_test_local PASSED 486.8s
//rs/tests/nns:nns_dapp_test_local PASSED 483.2s
//rs/tests/idx:basic_health_test_local PASSED 368.6s
//rs/tests/sdk:dfx_smoke_test (Farm regression) PASSED 153.0s
//rs/tests/nested:registration_local PASSED 824.0s

The CI environment shape is covered too, reproduced locally with
--test_env=SSL_CERT_FILE=<a bundle outside /etc>: dfx_smoke_test_local (gateway)
and basic_health_test_local (no gateway — the majority case) both pass, the
shadowed file is left untouched on the host, and both pass again with the variable
unset.

The two gateway tests are the sharpest: each fetches
https://<canister id>.ic-gateway.ic.net/… with no resolve override and no
roots added, so they fail unless the mount, the wildcard, the trust store and the
certificate all hold. basic_health_test_local is the control — it stands up no
gateway, so it shows the unconditional trust-store install does not disturb the
~366 local targets that never touch one.

The logs confirm the chain rather than just a green tick: dnsmasq is started
exactly twice (create_group plus the one restart, with VMs already booted),
await_status_is_healthy of ic-gateway succeeds against the domain, and the
canister-subdomain asset fetch returns.

A few things were checked directly before writing the code, since each could have
sunk the approach:

  • The bind mount works in a nested userns+mntns — the Bazel sandbox layer and
    then the driver's — with the host's files unaffected. _local tests
    deliberately run sandboxed with requires-network absent.
  • --address=/d/a answers d, x.d and x.raw.d for both A and AAAA.
  • glibc returns the AAAA when the paired A query comes back REFUSED, which is
    exactly what an IPv6-only wildcard produces — and the local gateway has no
    IPv4, so that is the real configuration rather than a corner case.
  • The composed bundle verifies the dev CA and still verifies a public root, so
    the append really is additive.

Not done here

No danger_accept_invalid_certs call is removed. All 14 in rs/tests/ dial an
IC node or API boundary node — by IP literal, or by a domain registered with no
DNS server and pinned with .resolve() — or plain HTTP. A trust anchor cannot
help any of them, and none targets the gateway. The reasoning behind them moved
from root_certificate()'s doc to install_dev_root_ca's rather than being
deleted with the method. await_api_bn_healthy_async and status_async likewise
keep their .resolve() calls: an API BN's domain comes from its registry record
and is registered with no DNS server on either backend, and nothing in the tree
overrides uses_dns().

prometheus_vm writes <canister id>.raw.<domain> scrape targets, and those now
resolve — but the ledger-canister and bitcoin-*/dogecoin-* jobs are
scheme: https with no tls_config, unlike the neighbouring nested_hosts and
node_exporter jobs, so they would verify the dev-CA leaf against the Prometheus
UVM's
stock trust store and fail. Finishing that means installing the CA there
too; those are mainnet canister ids that only exist on testnets, and it is a
separate, independently testable change.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Enables driver-side DNS resolution and wildcard gateway domains on the local system-test backend.

Changes:

  • Installs an isolated resolver configuration in the driver’s mount namespace.
  • Adds wildcard DNS records through managed dnsmasq restarts.
  • Removes obsolete client-side resolution overrides.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
rs/tests/sdk/src/asset.rs Removes gateway DNS overrides.
rs/tests/nns/nns_dapp_test.rs Uses local wildcard DNS resolution.
rs/tests/driver/src/driver/local_backend.rs Adds resolver mounting and wildcard DNS support.
rs/tests/driver/src/driver/ic_gateway_vm.rs Registers wildcard gateway records and removes override APIs.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rs/tests/driver/src/driver/local_backend.rs Outdated
@basvandijk basvandijk added the CI_ALL_BAZEL_TARGETS Runs all bazel targets label Aug 24, 2026
@basvandijk
basvandijk requested a balanced review from Copilot August 24, 2026 09:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread rs/tests/driver/src/driver/local_backend.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@basvandijk basvandijk changed the title feat(system-tests): let the driver resolve in-group names on the local backend feat(system-tests): reach the local ic-gateway by name over verified HTTPS Aug 24, 2026
@basvandijk
basvandijk requested a balanced review from Copilot August 24, 2026 11:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Suppressed comments (2)

rs/tests/driver/src/driver/local_backend.rs:486

  • This intra-doc link also refers to the nonexistent SYSTEM_CA_BUNDLE constant; link to DEFAULT_SYSTEM_CA_BUNDLE instead.
    /// of [`SYSTEM_CA_BUNDLE`] with that CA appended over the original.

rs/tests/driver/src/driver/local_backend.rs:482

  • When neither SSL variable is set, rustls-native-certs follows openssl_probe::probe() and uses the first existing CA file from a platform list; it does not always select this Debian path. On systems such as RHEL, this either fails startup because the file is absent or modifies a bundle that TLS clients never read. Resolve the bundle from openssl_probe::probe().cert_file (and report when no file is found) so the mounted file matches the verifier's choice.
        Ok(PathBuf::from(DEFAULT_SYSTEM_CA_BUNDLE))

Comment thread rs/tests/driver/src/driver/local_backend.rs Outdated
Comment thread rs/tests/driver/src/driver/local_backend.rs Outdated
Comment thread rs/tests/driver/src/driver/local_backend.rs
@basvandijk

Copy link
Copy Markdown
Collaborator Author

placeholder

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

rs/tests/driver/src/driver/mod.rs:7

  • Extracting the CA type into this private module leaves the existing [DevRootCa] intra-doc link in ic_gateway_vm.rs:286 unresolved: that module now imports only dev_root_ca, so the type is no longer in scope. Please update the link to the fully qualified crate::driver::dev_root_ca::DevRootCa path.
mod dev_root_ca;

@basvandijk
basvandijk marked this pull request as ready for review August 24, 2026 19:43
@basvandijk
basvandijk requested review from a team as code owners August 24, 2026 19:43

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):

  1. Update unreleased_changelog.md (if there are behavior changes, even if they are
    non-breaking).

  2. Are there BREAKING changes?

  3. Is a data migration needed?

  4. Security review?

How to Satisfy This Automatic Review

  1. Go to the bottom of the pull request page.

  2. Look for where it says this bot is requesting changes.

  3. Click the three dots to the right.

  4. Select "Dismiss review".

  5. In the text entry box, respond to each of the numbered items in the previous
    section, declare one of the following:

  • Done.

  • $REASON_WHY_NO_NEED. E.g. for unreleased_changelog.md, "No
    canister behavior changes.", or for item 2, "Existing APIs
    behave as before.".

Brief Guide to "Externally Visible" Changes

"Externally visible behavior change" is very often due to some NEW canister API.

Changes to EXISTING APIs are more likely to be "breaking".

If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.

If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.

Reference(s)

For a more comprehensive checklist, see here.

GOVERNANCE_CHECKLIST_REMINDER_DEDUP

@basvandijk basvandijk changed the title feat(system-tests): reach the local ic-gateway by name over verified HTTPS feat(system-tests): let the driver reach the local ic-gateway by name over verified HTTPS Aug 25, 2026
basvandijk and others added 11 commits August 25, 2026 07:59
`ensure_administrable_netns` puts the driver in a network namespace it owns,
but not a mount namespace -- so the driver kept reading the *host's*
`/etc/resolv.conf`, whose nameserver is unreachable from that netns. Driver-side
DNS was therefore not merely overridden by the callers that pass `reqwest` a
`.resolve()` override: it was dead.

Unshare `CLONE_NEWNS` as well and bind-mount a generated `resolv.conf` naming
`IPV6_NAME_SERVERS` over `/etc/resolv.conf`, so the driver resolves in-group
names through the group's own `dnsmasq` exactly like the VMs do. `dnsmasq`
already binds those addresses on the group bridge inside this very netns, so
nothing else has to change.

`/` is remounted `MS_REC | MS_PRIVATE` first: a host that leaves `/` shared --
the systemd default -- would otherwise have its real `/etc/resolv.conf` replaced
for as long as the mount lived. The mount is done in-process so `CAP_SYS_ADMIN`
never enters the ambient set the unprivileged `ip`/`dnsmasq`/QEMU children
inherit, and `mount(2)` is called through `libc` because the workspace builds
`nix` without its `mount` feature.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`add_dns_record` appends to `dnsmasq`'s `--addn-hosts` file, which is a hosts
file and so holds exact names only. The IC gateway needs more than that: Farm's
playnet DNS gives its domain an apex record plus `*` and `*.raw` CNAMEs, because
clients address canisters as `<canister id>.<domain>` and
`<canister id>.raw.<domain>`. Locally those subdomains resolved nowhere, for
clients in a VM as much as for the driver.

Add `add_wildcard_dns_record`, which answers for a name and every subdomain of
it at any depth -- one `--address` option covering all three Farm records. It
takes the whole address set at once because a record, not an address, is what
costs a restart.

A restart is what it costs, since `--address` is a command-line option and
nothing re-reads those: `SIGHUP` re-reads the hosts-shaped files and
`--servers-file`, and a servers-file admits nothing but `server` and
`rev-server`. So the records move to a file `start_dnsmasq` turns back into
options, `create_group` takes over truncating them (they have to survive a
restart), and `stop_dnsmasq` now waits for `dnsmasq` to exit -- `dnsmasq` binds
its listeners before daemonizing, so a restart racing the old instance for UDP
port 53 fails outright.

That wait escalates to `SIGKILL`, so it first confirms the pid is still a live
`dnsmasq` of ours, by looking for the `--pid-file=<path>` argument it was started
with in `/proc/<pid>/cmdline`: the pid comes from a pid-file a `dnsmasq` that
died on its own never got to remove, and a zombie's `cmdline` reads back empty.
Deliberately not the process *name* -- Bazel links the `dnsmasq` runfiles entry
under a hashed basename, so `comm` is a truncation of that hash.

Restarting is safe for VMs that are already up. IC GuestOS nodes are statically
configured and never consult the RA; a VM that SLAAC'd its address holds it far
longer than the gap; DHCPv4 leases live in the lease-file `dnsmasq` re-reads at
startup; and the only real loss is its DNS cache.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With the driver sharing the group's resolver and `dnsmasq` answering for the
gateway's subdomains, a driver-side client can reach the gateway the same way it
does on Farm. Register the domain as a wildcard record rather than an apex-only
one, and drop `resolve_override_for_url`, its parameter on
`await_status_is_healthy`, and both call sites.

The certificate is the only thing the Local backend still needs to be told
about, and only because the dev root CA issued it rather than a public one.

Left alone deliberately, since each looks like an oversight otherwise:
`await_api_bn_healthy_async` and `status_async` both keep their `.resolve()`
calls, which are Farm machinery rather than local-backend workarounds -- an API
boundary node's domain comes from its registry record and is registered with no
DNS server on either backend, and nothing in the tree overrides `uses_dns()`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`stop_dnsmasq` sent `SIGKILL` and returned immediately, so `restart_dnsmasq`
could launch the replacement while the killed process still owned UDP port 53 --
the bind race the wait exists to prevent. Signals are asynchronous, `SIGKILL`
included, so give it its own wait.

An unconfirmed exit is now an error rather than something to start a second
`dnsmasq` on top of: `stop_dnsmasq` returns `Result`, `restart_dnsmasq`
propagates it, and only teardown ignores it -- there the bridge and its addresses
are deleted immediately afterwards, so a `dnsmasq` that outlives its `SIGKILL`
has nothing left to serve.

The wait watches the process's `argv` disappear, which the kernel does a hair
before it closes the process's files, so the doc says so rather than claiming
more than it observes. Waiting for the pid itself to disappear would be worse:
`dnsmasq` daemonizes, so it lingers as a zombie until whatever it was reparented
to reaps it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`dnsmasq_is_alive` searched `/proc/<pid>/cmdline` for the `--pid-file=<path>`
bytes, which also accepts the argument embedded in a longer one, or our path as
the prefix of a longer path. Split on NUL and compare whole arguments instead.

The weaker check undercut the point of having it: a false positive is what gets
an unrelated process SIGTERM'd, and then SIGKILL'd, on the strength of a stale
pid-file. Verified both directions against a live `dnsmasq` -- started with
`--pid-file=<path>.longer`, the substring search matches and the split does not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pure move, no behaviour change. The dev root CA is about to gain a second
consumer -- the local backend, which installs it into the driver's own trust
store -- and it belongs to neither that nor the gateway: what trusts it is the
dev IC-OS images. Putting it in `driver/dev_root_ca.rs` keeps the certificate
issuance machinery (`rcgen`, `rsa`) out of the QEMU/networking module, which
would otherwise own code it never calls.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tore

Every dev IC-OS image trusts the dev root CA, via `update-ca-certificates` in the
`output_dev` stage of the GuestOS and HostOS Dockerfiles. The driver did not: each
of its clients had to add the CA to its own roots to reach the local ic-gateway
over verified HTTPS. Install it once instead, so trust is a property of the
environment rather than of each call site -- the same move the resolver install
made for DNS.

The mechanism is a bind mount of `/etc/ssl/certs/ca-certificates.crt` with the CA
appended, in the mount namespace `ensure_administrable_netns` already owns. That
path is load-bearing through a chain nothing type-checks -- `ClientBuilder::build`
-> `rustls_platform_verifier::Verifier` -> `rustls_native_certs` ->
`openssl_probe::probe` -- so the const carries it in a comment, and
`rs/tests/BUILD.bazel` already builds a bundle at the same path for the colocated
driver image for the same reason.

Deliberately not `SSL_CERT_FILE`: `rustls-native-certs` loads *only* what that
names and skips the platform store entirely, so it would narrow the driver's trust
rather than widen it, breaking the clients that need public roots. It is checked
for and refused for that reason. `std::env::set_var` is also `unsafe` in edition
2024 and racy against threads building clients, which the mount is not.

Appending, never replacing: the original bytes are copied verbatim, so this can
only widen trust. The mount-propagation change moves out of the resolv.conf
install into the caller, so neither installer depends on the other having run
first. And the result is read back, because every step can succeed while leaving
the CA unreachable, and that would not surface until a TLS handshake failed six
minutes into a test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With the dev root CA in the driver's trust store, a client reaching the local
ic-gateway needs no configuration of its own, so `root_certificate()` and all
three `add_root_certificate` call sites go. Neither `nns_dapp_test` nor the sdk
asset helper now carries any backend-specific client setup -- after the resolve
override went too, both just build a plain client and GET the URL.

`root_certificate`'s doc carried a paragraph that was not about the method: it
explains why clients dialling an API boundary node directly still need
`danger_accept_invalid_certs`, which is the reasoning behind fourteen surviving
call sites. That paragraph moves to `install_dev_root_ca`, where trust is now
arranged, rather than being deleted with the method.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…not work

Three comment fixes in the dev-root-CA path, no code change.

`install_dev_root_ca` argued for the bind mount by ruling out *writing* a `.crt`
into `/etc/ssl/certs`. That left the variant a reader actually reaches for
unaddressed: bind-mounting the CA as a *new* file there, which escapes no
namespace. It does not work for a different reason -- `mount(2)` will not create a
directory entry, and the driver cannot create one, since root owns
`/etc/ssl/certs` and is not mapped into its user namespace -- so name that
instead, and demote the `CApath` point to the secondary reason it is.

The module doc justified `dev_root_ca.rs` existing with "what trusts it is the
IC-OS images", which does not follow: who trusts a certificate says nothing about
where the code reading it belongs, and the line above already stated it. Replaced
with the reason that holds -- neither consumer can host it without owning code it
never calls, or depending on the wrong module.

And the note on `self_signed` shrinks from ten lines to three. The subject/authority
key identifier detail was `rcgen` trivia that does not bear on whether the code is
right; what does is that the re-signed certificate is not the checked-in one and is
never served, which is what stops someone "simplifying" `chain_pem`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every `_local` target failed on CI with

    SSL_CERT_FILE is set ("/namespace/worker/etc/ssl/certs/ca-certificates.crt"),
    which would make the driver's TLS clients read only what it names and so never
    see the dev root CA this installs in /etc/ssl/certs/ca-certificates.crt

which was this code refusing to run. The Namespace runner sets `SSL_CERT_FILE` in
its own environment, so it reaches the driver even though nothing in the repo sets
it -- checking `env_inherit` in `rs/tests` was the wrong place to look, and the
conclusion drawn from it ("nothing sets them today") was wrong.

Refusing was also the wrong response. An environment that designates a different
bundle is a legitimate configuration, not a misconfiguration, so honour it:
`system_ca_bundle` now resolves the bundle the way
`rustls_native_certs::load_native_certs` does -- `SSL_CERT_FILE` when set,
`openssl_probe`'s first Linux candidate otherwise -- and the append, the mount and
the read-back all target that path. The dev root CA lands in whichever bundle the
driver's clients actually read, and the runner's own roots are kept rather than
replaced.

One shape still refuses: `SSL_CERT_DIR` without `SSL_CERT_FILE`. `rustls-native-certs`
then reads only those directories and no bundle at all, so there is no file to
append to, and a file cannot be added to a directory the driver does not own.
Nothing sets it that way; it fails loudly rather than quietly omitting the CA.

`mount`'s target becomes a `&Path`, since `SSL_CERT_FILE` need not be UTF-8.

Verified by reproducing the CI shape locally with
`--test_env=SSL_CERT_FILE=<a bundle outside /etc>`: green for a gateway target and
a gateway-free one, with the shadowed file left untouched on the host, and green
again with the variable unset.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three review findings, all doc-level bar one error string.

Two intra-doc links still named `SYSTEM_CA_BUNDLE` after that constant became
`DEFAULT_SYSTEM_CA_BUNDLE`, so rustdoc could not resolve them. Both now name
`system_ca_bundle` instead, which is the right referent anyway: the target is
resolved at run time, not fixed by the constant. Nothing local catches this --
rustdoc is not part of `rust-lint.sh`, and the repo already carries older broken
links -- so it took review to spot.

The paragraph on shadowing the bundle had the OpenSSL argument backwards. It
claimed appending to the bundle reaches `CApath` consumers; it does not. `CApath`
resolves `<subject hash>.0` symlinks and never scans the bundle. What appending
reaches is bundle (`CAfile`) readers: `rustls-native-certs` for the driver's own
clients, and OpenSSL and `curl` by default on Debian, where the default `CAfile`
is this same path. The `CApath`-only case is now documented as the limitation it
is, along with the note that a plainly named file in the directory would not have
served it either -- only a hash-named symlink would.

Also spell out why hard-coding the Debian path is safe rather than merely
convenient: it is `openssl_probe`'s *first* candidate, so if it exists it is the
one `probe()` returns and the mount cannot land on a bundle the verifier ignores.
The residual exposure is a platform without it, which fails loudly on the read --
and `SSL_CERT_FILE` is the supported way out now that it is honoured, so the error
says so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@basvandijk
basvandijk requested a review from a team as a code owner August 25, 2026 08:00
@basvandijk
basvandijk force-pushed the bas/local-backend-driver-dns branch from 4a3cd87 to 22451ba Compare August 25, 2026 08:00
@github-actions github-actions Bot added the @node label Aug 25, 2026
@basvandijk
basvandijk requested a balanced review from Copilot August 25, 2026 08:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comment thread rs/tests/driver/src/driver/local_backend.rs Outdated
Comment thread rs/tests/driver/src/driver/ic_gateway_vm.rs
Both are links this stack broke, in the same way and for the same reason: the
referent stopped being in the linking module's scope, and rustdoc resolves an
intra-doc link by name resolution, so it silently stopped resolving.

`[`Farm::new`]` in `install_dev_root_ca` names a type this module never imports
-- `local_backend` takes only `VMCreateResponse` and `VmSpec` from `farm`. It now
carries the path, matching the module doc above it, which already writes
`[`crate::driver::farm::Farm`]` in full for exactly this reason.

`[`DevRootCa`]` in `load_or_create_local_playnet` was written when the type lived
in `ic_gateway_vm`; 9f9181a moved it to `dev_root_ca` and left the link
behind, importing the `dev_root_ca()` function but not the struct. It now names
`crate::driver::dev_root_ca::DevRootCa`.

Confirmed with `cargo doc --no-deps --document-private-items -p
ic-system-test-driver`, which is what it takes to see these at all: both
`install_dev_root_ca` and `load_or_create_local_playnet` are private, so a
default `cargo doc` does not document them and never checks their links. The run
goes from 20 unresolved links to 18, removing exactly these two and adding none.
The remaining 18 all blame to commits that predate this stack (`start_vm` from
#10642, `bridge_name` and `DISK_REQUEST_ALIGNMENT` from #10253, and older ones in
`test_env_api` and `generic_workload_engine`), which is why
`-D rustdoc::broken_intra_doc_links` still cannot simply be turned on as a gate.

The surrounding paragraphs are re-wrapped because the explicit paths are long,
not because the prose changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comment thread rs/tests/driver/src/driver/local_backend.rs
`start_dnsmasq` composed its whole command line into a string and ran it
through `/bin/sh -c`. Among those words were the `--address` options
carrying wildcard record names, which originate in a VM name a test hands
to `IcGatewayVm::new` and nothing validates — so whitespace or a shell
metacharacter in a malformed name became shell syntax, and DNS
configuration runs well before the `Url::parse` that would have rejected
it.

`dnsmasq` is now spawned directly with `Command::args`, so every argument
reaches it as one argv entry and no shell parses any of them. That also
removes a latent quoting bug the old form had independently of any name:
the paths were interpolated unquoted, so a working directory containing a
space would have split them. `path_arg` builds each `--<option>=<path>`
as an `OsString`, which `Path::display` could not do without being lossy
about a non-UTF-8 path.

The names are validated as well, because the shell was not the only thing
they could confuse: both record files are line-based `<address> <name>`
pairs, and `dnsmasq_wildcard_args` splits a line back apart on
whitespace, so a name with a space registers a *different* record and one
with a newline registers a second record nobody asked for.
`validate_dns_name` accepts a plain ASCII hostname and is called by
`add_dns_record` and `add_wildcard_dns_record` before either persists
anything.

`run_shell` keeps its remaining callers — `ip` invocations whose every
word is a literal or backend-derived — and its doc now says that is the
limit of what it is for.

Verified: `//rs/tests/driver:unit_tests` PASSED (the two new cases),
`//rs/tests/sdk:dfx_smoke_test_local` PASSED 208.6s,
`//rs/tests/nns:nns_dapp_test_local` PASSED 172.6s and
`//rs/tests/idx:basic_health_test_local` PASSED 171.4s. The two gateway
runs show `dnsmasq` started exactly twice with the wildcard registered
and `https://ic-gateway.ic.net/api/v2/status` healthy; the control shows
the empty-wildcard start path, which the old format string reached by
interpolating an empty tail.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants