[AI assisted] Plan D: Consolidated Approach SVCB + dnsRecordStyle#13
Open
scourtney-godaddy wants to merge 10 commits into
Open
[AI assisted] Plan D: Consolidated Approach SVCB + dnsRecordStyle#13scourtney-godaddy wants to merge 10 commits into
scourtney-godaddy wants to merge 10 commits into
Conversation
This was referenced May 16, 2026
There was a problem hiding this comment.
Pull request overview
Adds support for a new DNS record emission mode (“Consolidated Approach” SVCB) and introduces a dnsRecordStyle selector on the V2 register flow so operators can choose between legacy _ans TXT discovery, consolidated SVCB discovery, or both, with persistence via a new SQLite migration.
Changes:
- Add
dnsRecordStyleto the V2 register request (OpenAPI + handler/service plumbing) and persist it onagent_registrations. - Extend required-DNS-record computation to optionally emit SVCB records (and update record-type enums/docs).
- Add DNS lookup verification for SVCB and propagate DNSSEC AD-bit handling for HTTPS/SVCB.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/api-spec-v2.yaml | Adds dnsRecordStyle to the V2 register schema and extends DNS record type enum to include SVCB. |
| internal/adapter/docsui/openapi/ra.yaml | Mirrors the OpenAPI changes used by the docs UI. |
| internal/ra/handler/registration.go | Accepts dnsRecordStyle in the JSON request and forwards it to the service layer. |
| internal/ra/service/registration.go | Applies the requested DNS record style during registration creation. |
| internal/ra/service/helpers.go | Implements style normalization/validation (applyDNSRecordStyle). |
| internal/ra/service/helpers_test.go | Adds unit tests for applyDNSRecordStyle validation and error messaging. |
| internal/domain/dnsrecords.go | Introduces DNSRecordStyle, adds SVCB record type, and emits per-style DNS record sets. |
| internal/domain/dnsrecords_test.go | Adds tests covering style matrix emission, SVCB param composition, and helper functions. |
| internal/adapter/dns/lookup.go | Adds SVCB lookup verification and propagates AD-bit for HTTPS/SVCB. |
| internal/adapter/dns/dns_test.go | Adds SVCB verifier tests and AD-bit propagation tests for HTTPS/SVCB. |
| internal/ra/service/lifecycle.go | Expands DNSSEC-mismatch hard-fail rules to include SVCB/HTTPS. |
| internal/port/dns.go | Updates DNSSECVerified field documentation to include SVCB/HTTPS semantics. |
| internal/domain/agent.go | Stores DNSRecordStyle on the registration aggregate. |
| internal/adapter/store/sqlite/migrations/007_agent_dns_record_style.sql | Adds/persists dns_record_style with CHECK + backfill behavior. |
| internal/adapter/store/sqlite/agent.go | Reads/writes the new dns_record_style column. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+299
to
+303
| got := formatHTTPSValue(svcb) | ||
| if r.Actual == "" { | ||
| r.Actual = got | ||
| } | ||
| if normalizeHTTPS(got) == wantNorm { |
| Type: DNSRecordSVCB, | ||
| Value: value, | ||
| Purpose: PurposeDiscovery, | ||
| Required: false, |
Comment on lines
+52
to
+54
| // for this registration. One of "consolidated" (default, | ||
| // recommended), "legacy" (original `_ans` TXT shape), "both" | ||
| // (transition union). Empty/missing → consolidated. Invalid |
Comment on lines
+91
to
+92
| // in dnsRecordsProvisioned and tells the operator to publish. | ||
| // "consolidated" (default), "legacy", or "both". Empty value is |
Comment on lines
+120
to
+122
| // for this registration: "consolidated" (Consolidated Approach | ||
| // SVCB rows, default), "legacy" (the original `_ans` TXT shape), | ||
| // or "both" (the transition union). Empty at the domain layer |
Comment on lines
+15
to
+19
| -- Nullable to allow rows that pre-date this migration to load. The | ||
| -- backfill below sets every such row to LEGACY because every agent | ||
| -- registered before this PR shipped received the original `_ans` TXT | ||
| -- shape — defaulting them to CONSOLIDATED would silently demand SVCB | ||
| -- records they were never told to publish. CHECK matches the |
Comment on lines
+1122
to
+1124
| Set of DNS record families the RA emits in the 202 register | ||
| response's dnsRecords[] and in the AGENT_REGISTERED TL | ||
| event's attestations.dnsRecordsProvisioned[]. Not echoed on |
Comment on lines
+1122
to
+1125
| Set of DNS record families the RA emits in the 202 register | ||
| response's dnsRecords[] and in the AGENT_REGISTERED TL | ||
| event's attestations.dnsRecordsProvisioned[]. Not echoed on | ||
| GET /v2/ans/agents/{agentId}. |
Comment on lines
+19
to
+23
| -- Nullable to allow rows that pre-date this migration to load. The | ||
| -- backfill below sets every such row to ["ANS_TXT"] because every | ||
| -- agent registered before this PR shipped received the original | ||
| -- `_ans` TXT shape — defaulting them to ["ANS_SVCB"] would silently | ||
| -- demand SVCB records they were never told to publish. CHECK uses |
Comment on lines
+1116
to
+1120
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/DNSRecordStyle' | ||
| uniqueItems: true | ||
| minItems: 1 |
Comment on lines
+606
to
+615
| // Required flag. `r.Found` is true only when the actual | ||
| // matched after type-specific normalization, so | ||
| // `DNSSECVerified && !Found` captures "response was signed, | ||
| // but its content disagreed with what we issued" — the exact | ||
| // attack we block (an attacker rewrote a record in a signed | ||
| // zone). Applies to TLSA (cert binding), SVCB (capability | ||
| // locator with card-sha256), and HTTPS (service binding). | ||
| if r.DNSSECVerified && !r.Found { | ||
| switch r.Record.Type { | ||
| case domain.DNSRecordTLSA, domain.DNSRecordSVCB, domain.DNSRecordHTTPS: |
Comment on lines
+1026
to
+1030
| DNSRecordStyle: | ||
| type: string | ||
| enum: [ANS_SVCB, ANS_TXT] | ||
| description: | | ||
| Names one DNS record family the RA can emit for an agent |
| Type: DNSRecordSVCB, | ||
| Value: value, | ||
| Purpose: PurposeDiscovery, | ||
| Required: false, |
Extends domain.ComputeRequiredDNSRecords to emit one SVCB record per protocol at the agent bare FQDN, alongside the existing _ans TXT family. The SVCB row carries: alpn=PROTOCOL from endpoint.Protocol port=443 ServiceMode SvcPriority 1 at the FQDN wk=SUFFIX A2A: agent-card.json; MCP: mcp.json card-sha256=BASE64URL base64url of reg.CapabilitiesHash when set card-sha256 and capabilities_hash are the section 4.4.2 cross-check encodings of the same SHA-256 (DNS uses base64url, TL uses hex). When the operator did not submit agentCardContent, the SvcParam is absent and verifiers fall back to TOFU on first Trust Card fetch. Adds verifySVCB to LookupVerifier mirroring verifyHTTPS. Tests cover present-matching, absent (zone has different name), and wrong-target cases (AliasMode where ServiceMode was expected). Provisional SvcParams (wk, card-sha256) are unit-tested at the domain layer because miekg/dns rejects them in zone-file form until IANA registration; the verifier- level test exercises only registered SvcParamKeys (alpn, port). Required=false: section 4.4.2 marks Consolidated Approach SVCB as MAY, opt-in during the _ans TXT transition. Signed-off-by: kperry <kperry@godaddy.com>
Adds dnsRecordStyle to the V2 RegistrationRequest with three values: "consolidated" (default, recommended), "legacy" (original _ans TXT shape), "both" (transition union). Empty -> consolidated. Invalid -> 422 INVALID_DNS_RECORD_STYLE. The default points new integrations at the lean Consolidated Approach shape per section 4.4.2 SHOULD: one SVCB record at the bare FQDN per protocol, plus shared _ans-prefixed records and TLSA. Operators on existing zone-edit tooling for _ans TXT pick "legacy" explicitly. Migration operators set "both" for a defined window then flip back to "consolidated". V1 lane pins to "legacy" regardless of the request because V1 callers predate the Consolidated Approach and their tooling expects the original shape. V1 has no dnsRecordStyle field on the wire. Migration 007 adds the dns_record_style column on agent_registrations. Nullable for backwards compatibility with pre-Plan-D rows. Tests: - "both" emits 2x _ans TXT + 2x SVCB + shared records (existing test updated to set DNSRecordStyleBoth so it exercises the union path). - New tests cover "consolidated" (no _ans TXT), "legacy" (no SVCB), and "both" (union); the SvcParam wk/card-sha256 tests already covered the consolidated path implicitly. - Lint: extracted applyDNSRecordStyle helper to keep RegisterAgent under the funlen ceiling. Signed-off-by: kperry <kperry@godaddy.com>
Closes a long-standing spec/impl gap: ANS_SPEC.md section A.8.1 lists the HTTPS RR (RFC 9460 type 65) at the agent FQDN as RA-generated content the AHP provisions, but ComputeRequiredDNSRecords had never emitted it. The DNSRecordHTTPS enum value and verifyHTTPS verifier were already in place; this commit wires the emission. Generated only for the legacy + both styles, not for consolidated: the SVCB rows the consolidated form publishes already carry the same alpn/port/ECH SvcParams the HTTPS RR would, so emitting both would duplicate content and risk the two records drifting (section A.8.2 explicitly notes this). Operators on the consolidated path who still want HTTPS-RR-aware clients (typically browsers) to see the metadata can publish their own HTTPS RR as a side addition. Required=false: HTTPS RR is blocked by CNAME at the agent FQDN per RFC 1034 section 3.6.2. AHPs whose apex is fronted via CNAME cannot publish it at the same name; the RA does not block verify-dns on its absence. Tests pin: legacy style includes HTTPS RR + no SVCB; consolidated style includes SVCB + no HTTPS RR; both style includes both families. Signed-off-by: kperry <kperry@godaddy.com>
…alidation Signed-off-by: kperry <kperry@godaddy.com>
…nhance validation Signed-off-by: kperry <kperry@godaddy.com>
…t reclaimed) PR12 (capabilities_hash) shipped migration 006 in the upstream stack that this branch was originally based on. Now that PR12 is dropped, the 006 slot is open and this branch's only migration moves into it to keep the migration sequence dense (001 → 006, no gap). Filename also pluralized to dns_record_styles.sql to match the column this PR ships (a JSON array of CONSTANT_CASE values, not a singleton string). Signed-off-by: kperry <kperry@godaddy.com>
The OpenAPI schemas declare minItems: 1 and uniqueItems: true, and
their description says duplicates are rejected. The server previously
did neither — it normalized empty arrays to the default and silently
deduped duplicates — so the spec was making promises the service
layer didn't keep.
Tighten the service layer to match the contract:
- Field omitted (nil) still defaults to ["ANS_SVCB"]; the field
isn't in `required`, so omission is legal.
- Field present but empty (`"dnsRecordStyles": []`) now returns 422
INVALID_DNS_RECORD_STYLE. Matches `minItems: 1`. A caller who
explicitly sends an empty list is signalling intent the schema
doesn't permit; defaulting silently would mask a likely client
bug.
- Duplicates now return 422 INVALID_DNS_RECORD_STYLE. Matches
`uniqueItems: true`. Silent dedup would persist a state the
caller didn't quite request.
- Invalid element behavior unchanged: 422 with the canonical valid
set in the message.
The handler-side toDomainDNSRecordStyles now preserves the nil-vs-
empty distinction so applyDNSRecordStyles can tell field omission
(JSON null or missing) from explicit empty array (`[]`). JSON
unmarshal of null/missing yields a nil slice; explicit `[]` yields a
non-nil zero-length slice; the conversion preserves both shapes
unchanged for the service layer to discriminate on.
Tests updated:
- v2_explicit_empty_slice_rejected (replaces
v2_empty_slice_normalizes_to_default).
- v2_duplicate_elements_rejected (replaces
v2_duplicate_elements_deduped).
- Existing v2_nil_normalizes_to_default and unset_schema cases
still pass — they cover the field-omitted path.
Addresses Copilot review feedback C1+C2 on PR #25 by aligning
the server with the spec rather than relaxing the spec.
ComputeRequiredDNSRecords previously emitted SVCB rows with Required=false unconditionally, with the comment justifying it as "§4.4.2 marks the Consolidated Approach as MAY, opt-in alongside the `_ans` TXT family during the transition." That assumption only holds in union mode. The default style is ["ANS_SVCB"] (SVCB-sole), so the default registration was emitting zero Required=true PurposeDiscovery records. The badge TXT (PurposeBadge) keeps verify-dns from passing on an empty zone, but a SVCB-sole agent could publish only the badge, skip SVCB entirely, and verify-dns would still pass — a registered agent that is undiscoverable via the discovery family the operator opted into. Fix: when ANS_SVCB is the only selected style, mark SVCB Required=true. When emitted alongside ANS_TXT (the union/transition mode), keep Required=false because the legacy `_ans` TXT family above carries the required signal — that preserves the §4.4.2 MAY-during-transition framing for operators running both families. The TestComputeRequiredDNSRecords_StyleMatrix matrix gains a wantSVCBRequired column covering both paths: SVCB-sole, default (empty styles → ["ANS_SVCB"]), and the all-invalid fallback (also ["ANS_SVCB"]) all assert Required=true; the union case asserts Required=false. The original WithoutCert test fixture uses union mode and keeps Required=false with an updated comment pointing readers at the matrix for the SVCB-sole path. Addresses Copilot review feedback C3 on PR #25. Signed-off-by: kperry <kperry@godaddy.com>
verifySVCB previously did full normalized-string equality
(`normalizeHTTPS(got) == wantNorm`) while its docstring claimed
RFC 9460 §8 unknown-key ignore semantics. The two diverged: a live
record carrying extra SvcParams from a coexisting agentic spec
(DNS-AID, etc.) would not match, and under DNSSEC AD=true the
mismatch trips the SVCB_DNSSEC_MISMATCH hard fail in the lifecycle
layer — defeating the entire point of the Consolidated Approach
(multi-spec coexistence in a single SVCB row).
Switch to subset matching:
- parseSVCBValue parses "<priority> <target> [k=v]..." into a
structured (priority, target, params-map) form. Used by both the
expected and actual sides.
- matchesSVCBSubset returns true iff priority and target are equal
and every expected SvcParam is present in the live record with an
equal value. Additional SvcParams in the live record are ignored.
- verifySVCB calls parseSVCBValue on rec.Value once, then
parseSVCBValue+matchesSVCBSubset on each candidate SVCB rrset.
Tests added to TestLookupVerifier_SVCB:
- extra-svcparams-tolerated-rfc9460-section-8: live record carries
`mandatory=alpn` not in the expected; still matches.
- missing-expected-param-fails-subset-match: live record omits
expected `port=443`; does not match.
verifyHTTPS keeps strict-equality matching (its docstring is honest
about that): HTTPS RR is a companion to legacy `_ans` TXT, not the
multi-spec coexistence target. SVCB is where the §8 tolerance
matters.
Addresses Copilot review feedback C5 on PR #25.
Signed-off-by: kperry <kperry@godaddy.com>
…record computation Signed-off-by: kperry <kperry@godaddy.com>
kperry-godaddy
force-pushed
the
feat/plan-d-consolidated-svcb
branch
from
4d2b2a0 to
e00928a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the Consolidated Approach SVCB record format and a
dnsRecordStylefield on the register request, so an operator can choose between the legacy four-record set and the single-record SVCB style described in the DNS-AID consensus discussion.The V2 register endpoint accepts an optional
dnsRecordStylefieldselecting which DNS records the RA emits in
dnsRecordsProvisionedand surfaces in the registration response for the operator to publish.
Three values:
consolidated(default, recommended): one SVCB record per protocolendpoint at the agent's bare FQDN, carrying connection hints and
capability locators in a single DNS lookup. Aligns with the
Consolidated Approach DNSAID consensus discussion at
github.com/jmozley-infoblox/DNS_for_agents_collaboration.
legacy: the original_ansTXT family the RA has emitted sincev0.1.x, plus an HTTPS RR (RFC 9460 type 65) at the agent FQDN
carrying ALPN signalling for clients that don't speak ANS protocol
detection.
both: the union of consolidated and legacy, the transition shapefor AHPs migrating from one to the other.
Empty or missing
dnsRecordStylenormalizes toconsolidated. Aninvalid value surfaces as
INVALID_DNS_RECORD_STYLE(HTTP 422).The SVCB row at the bare FQDN follows RFC 9460 ServiceMode
(SvcPriority 1, TargetName
.) so address resolution stays at theagent's FQDN. SvcParams emitted:
alpn,port,wk(well-knownmetadata path),
card-sha256(the base64url-encodedcapabilitiesHashfrom PR #12 in this stack). SvcParams fromDNS-AID, ANS, and other agentic specs coexist in the same record
per RFC 9460 §8 unknown-key ignore semantics.
Migration 007 adds
dns_record_style(nullable) toagent_registrations. Pre-Plan-D rows load with the field empty;ComputeRequiredDNSRecordstreats empty asconsolidated.The DNS verifier (
internal/adapter/dns/lookup.go:verifySVCB)mirrors the existing HTTPS-RR verifier, comparing the resolver's
parsed record against the RA's expected SvcParam set after
normalization. Quoted vs unquoted SvcParamValue forms normalize
to the same equality check; provisional SvcParamKey names that
the resolver-side formatter doesn't yet recognize surface as
non-blocking integrity findings (
Required=false).Tests cover SVCB emission for each style, the cross-record
hash consistency check (SVCB
card-sha256matches the TL'scapabilitiesHash), and the verifier round-trip against themiekg/dns resolver.
Stacks on #12 (Plans A + C). Merge order: #12 → this.
Test plan
make check(gofmt + golangci-lint + 90% coverage gate)card-sha256↔ TLcapabilitiesHash)🤖 Generated with Claude Code