Dstack-TEE / Dstack-TEE/dstack-examples

dstack-ingress: follow-ups after the delegation (#104) and tls-alpn-01 (#105) merges

Open
#106 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
27
Forks
26
Avg merge
1h 25m
Merged PRs (30d)
7

Description

Follow-ups from reviewing #104 and #105 after both landed. Ordered by impact.
Nothing here needs a revert; all are incremental.

## P1 — A failed HAProxy reload is never retried

`dns01.sh` / `tlsalpn.sh`:

```bash
if [ "$changed" -eq 1 ]; then
collect_evidence || echo "Evidence generation failed" >&2
build_combined_pems || echo "Combined PEM build failed" >&2
haproxy_reload || true
fi
[ "$failed" -eq 0 ]
```

The reload failure is swallowed and does not set `failed`, so the pass reports
success. Twelve hours later the ACME client says "nothing to renew", `changed`
is 0, and the reload is never attempted again. HAProxy keeps serving the
*previous* certificate — which was renewed precisely because it was near expiry
— until it expires. Same for a failed `build_combined_pems`.

#102 solved this with a `renewal-applied.stamp` and a `certs_pending_apply()`
check: certificate material newer than the stamp means the renewal has not
reached HAProxy yet, so the apply step retries on the next cycle. That PR
predates the restructure and targets `renewal-daemon.sh` / `renew-certificate.sh`,
which #105 removed, so the mechanism needs porting rather than merging — and it
can be simpler here: #102 needed a file because bootstrap and the daemon were
two processes, whereas one process now owns the whole lifecycle, so an in-memory
"apply pending" flag is enough. Credit to @pacoyang for finding it.

## P1 — Delegation CAA verification is single-resolver and fails closed

`verify_delegation_caa()` queries only `dns.google` and refuses to start when the
record is not found. Public resolvers cache *negative* answers for the zone's SOA
minimum — up to an hour — so a resolver queried before the operator created the
record keeps reporting it absent well after it exists. That is a false-positive
refusal to start, observed during #105's testing on exactly this kind of check.

The failure asymmetry also runs the wrong way:

| Situation | Current behaviour |
|---|---|
| DoH unreachable / non-zero DNS status | warn and **continue** — no CAA protection |
| Record genuinely present but cached negative | **hard fail**, container will not start |

`grep -F "accounturi=…"` on `.data` compounds it: that matches presentation-form
rdata, which is what `dns.google` returns. Adding a second resolver naively would
break it, because Cloudflare returns CAA in RFC 3597 generic hex
(`\# 47 00 05 69 73 73 75 65 …`) and the literal match silently fails — which,
being fail-closed, is an outage.

`dnsguide.py` (added in #105) already does this properly: two resolvers with
union quorum for CAA, both rdata encodings, unit-tested against strings real
resolvers returned. Delegation should call it instead of rolling its own check.

That also gets the delegation path `DNS_SETUP_MODE` for free — `wait` blocks until
the operator's records appear instead of failing on the first look, and `webhook`
POSTs them to an automation endpoint with an HMAC and a TDX quote. One record
type has to be added to `build_records`: the `_acme-challenge` CNAME. The only
awkward part is `ALLOW_MISSING_CAA`, which has no dnsguide equivalent; mapping it
to `--mode print` would also skip CNAME/TXT verification, so it likely needs its
own flag.

## P2 — `unset_txt_record` reports success when the zone lookup failed

`get_dns_records()` returns `[]` both when a name has no records and when the
zone could not be resolved, so `unset_txt_record` iterates nothing and returns
True. Observed:

```
acme-dns-alias-hook: removing challenge TXT _acme-challenge.svc.example.com.deleg.example.net
Error: Could not find zone for domain _acme-challenge.svc.example.com.deleg.example.net
Successfully unset TXT record for _acme-challenge.svc.example.com.deleg.example.net
```

Exit code 0. The hook discards cleanup failures deliberately, so the impact is a
stale TXT in the delegation zone (the next `auth` overwrites it) — but the log
states the opposite of what happened, which will cost someone an afternoon.

## P2 — The delegated issuance path has never been executed

#104 states its testing as `py_compile` plus `bash -n`. Reviewing after the fact,
the mechanism does hold up:

- `_get_zone_info` does longest-suffix matching, so
`_acme-challenge.svc.example.com.deleg.example.net` resolves to
`deleg.example.net` and not to the served zone whose name it contains;
- the hook computes the right record name, finds `dnsman.py` on `PATH`, and uses
the venv interpreter;
- a failed `auth` exits non-zero, so certbot does see the challenge fail.

But no one has run an actual delegated issuance — hook writes the TXT, Let's
Encrypt follows the CNAME, validation passes. `TESTING.md` describes how to set
this up; it needs a delegation zone.

## P3 — `set_txt_record` deletes same-name records, which SAN will break

It removes every existing TXT at the name before creating the new one. Today
`process_domain` passes a single `-d` per certbot invocation, so only one
challenge value ever exists at `_acme-challenge.` and this is fine.

It stops being fine as soon as one certificate covers several identifiers that
validate at the same name — `example.com` plus `*.example.com`, or #86's SAN
support. certbot then calls the auth hook once per identifier with the same
`CERTBOT_DOMAIN` and different `CERTBOT_VALIDATION`, both records have to exist
at once, and the second call deletes the first. Worth fixing before #86 lands:
the challenge hook wants add-without-delete semantics, distinct from the
set-and-replace the routing records want.

## P3 — `_ensure_zone_id` falls back to a stale cached zone on lookup failure

```python
zone_info = self._get_zone_info(domain)
if zone_info:
self.zone_id, self.zone_domain = zone_info
return self.zone_id # previous zone's id when the lookup failed
```

A failed lookup returns whichever zone was cached last, so a subsequent write
could land in the wrong zone. Pre-existing, and not currently reachable —
`dnsman.py` runs as a fresh process per call, so the cache is empty at the point
that matters. Worth closing anyway, since delegation mode is precisely the
feature that makes "which zone are we writing to" a security property.

Contributor guide

Open the contributing guide

Research direction

Start with dns01.sh, tlsalpn.sh, dnsman.py, dnsguide.py, and TESTING.md, then trace the delegation and renewal paths and run the existing syntax and unit checks. Confirm the retry, resolver, cleanup, zone-selection, and SAN behaviors described here, and use TESTING.md to attempt a real delegated issuance. Done means the prioritized follow-ups are implemented with tests and the delegated path succeeds end to end.

Written by the indexing model from the issue text.

Assessment

Tech stack
bash, python
Domain
devops, networking, security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.