Repository navigation
Conversation
Add a DNS backend for the DNSimple API v2. - Write SVCB and TXT records. The DNS-AID private-use keys (key65400 to key65409) go directly into the SVCB record. - Remove the trailing dot from the SVCB target. The DNSimple API refuses a target with a trailing dot. - The DNSimple API has no upsert. Delete the records with the same name and type, then create the new records. - Create one API record for each TXT value. Decode the TXT content when records are read. - Follow all result pages when records are read. - Get the account ID from /v2/whoami when DNSIMPLE_ACCOUNT_ID is not set. Register the backend in the backend factory, the CLI backend registry and doctor. Add the "dnsimple" extra. Configuration: DNSIMPLE_TOKEN (required), DNSIMPLE_ACCOUNT_ID and DNSIMPLE_BASE_URL (optional). Signed-off-by: Anthony Eden <anthony.eden@dnsimple.com>
| content value. | ||
| """ | ||
| existing = await self._find_records(domain, name, record_type) | ||
| await self._delete_records(domain, existing) |
There was a problem hiding this comment.
Thanks for adding DNSimple support. One failure-path concern in _replace_records() (reviewed at bbfc735):
A local stateful httpx.MockTransport reproduction through the public publish() API produces this sequence:
Existing SVCB record
→ GET succeeds
→ DELETE returns 204
→ POST returns 429
→ publish() returns success=False, but no SVCB record remains
The error is reported, but the previous working record has already been deleted and is not restored. Once cached answers expire, this can interrupt agent discovery until the record is restored or publication succeeds again.
Could we update the existing record by ID rather than delete it first for the single-SVCB case, and add a regression test that checks preservation of the existing record after a rejected replacement? Multi-value TXT replacement also needs partial-failure coverage: deleting the old values and then failing on a later POST can leave only part of the new set.
This was reproduced with a mocked API, not against a live DNSimple zone.
Minimal reproduction through public publish()
Run against the reviewed PR checkout with its dependencies installed. All backend HTTP requests are intercepted by MockTransport; the token is a dummy value.
import asyncio
from unittest.mock import patch
import httpx
from dns_aid.backends.dnsimple import DNSimpleBackend
from dns_aid.core.publisher import publish
async def main():
records = [{
"id": 1,
"name": "chat",
"type": "SVCB",
"content": "1 old.example.com",
"ttl": 3600,
}]
def api(request):
if request.method == "GET":
data = (
records.copy()
if request.url.path.endswith("/records")
else {"id": 1}
)
return httpx.Response(200, json={"data": data})
if request.method == "DELETE":
records.clear()
return httpx.Response(204)
return httpx.Response(429, json={"message": "quota exceeded"})
backend = DNSimpleBackend(token="dummy", account_id="1")
async with httpx.AsyncClient(
base_url="https://mock.invalid",
transport=httpx.MockTransport(api),
) as client:
with patch.object(backend, "_get_client", return_value=client):
result = await publish(
"chat",
"example.com",
"mcp",
"new.example.com",
backend=backend,
)
assert not result.success
assert records == [] # Demonstrates loss, not successful preservation.
asyncio.run(main())The last assertion describes the unwanted state. A preservation regression test should instead require the existing SVCB to remain after the replacement is explicitly rejected.
There was a problem hiding this comment.
Confirmed against the live DNSimple API. A new live test (tests/integration/test_dnsimple_replace_live.py) creates records, then makes the API refuse the replacement with HTTP 400. On bbfc735, the SVCB test ends with no SVCB record. The TXT test ends with only the first new value. I will fix _replace_records() so that a refused replacement keeps the existing records, and add this test to the PR.
There was a problem hiding this comment.
Thank you for this finding. I confirmed it against the live DNSimple API, not only with a mock. The API refused a replacement with HTTP 400. At bbfc735, the SVCB case ended with no SVCB record. The TXT case ended with only the first new value.
Fixed in 420d1ad. _replace_records() now does the changes in this order:
- Keep each existing record that has a new value. If the TTL is different, update only the TTL.
- Update the other existing records in place by ID (PATCH).
- Create a record for each new value that is still remaining.
- Delete the existing records that are not necessary, as the last step.
If a request in steps 1–3 fails, the backend deletes the records it created, sets the updated records back to their old content and TTL, and raises the first error. Step 1 is necessary because the DNSimple API refuses a record that is equal to an existing record ("Matching record already exists").
Tests:
- Unit tests for the new order, a refused SVCB update, a TXT POST failure after a partial change (rollback), and an undo failure that does not hide the first error.
- A live regression test,
tests/integration/test_dnsimple_replace_live.py. It makes the API refuse the replacement, then checks that the existing SVCB record and the existing TXT set did not change. The test failed atbbfc735and passes at 420d1ad. It runs only whenDNS_AID_LIVE_TESTS=1,DNSIMPLE_TOKENandDNSIMPLE_TEST_ZONEare set.
One limit: if the undo also fails (for example during a long API outage), the record set can be partly changed. The DNSimple API has no atomic replace for one name and type, so the backend logs each failed undo request.
Before, _replace_records() deleted all records with the same name and type, then created the new records. If the API refused a create (for example 400 or 429), the old records were gone. For TXT, only part of the new set was left. Now the replace does the changes in this order: 1. Keep each existing record that has a new value. If the TTL is different, update only the TTL. 2. Update the other existing records in place (PATCH). 3. Create a record for each new value that is still remaining. 4. Delete the existing records that are not necessary. If a request in steps 1-3 fails, delete the created records, set the updated records back to their old content and TTL, and raise the first error. Step 1 is necessary because the API refuses a record that is equal to an existing record. Add unit tests for the new order and for the undo. Add a live test (tests/integration/test_dnsimple_replace_live.py) that makes the DNSimple API refuse a replace and then checks that the existing SVCB and TXT records did not change. The live test runs only when DNS_AID_LIVE_TESTS=1, DNSIMPLE_TOKEN and DNSIMPLE_TEST_ZONE are set. Signed-off-by: Anthony Eden <anthony.eden@dnsimple.com>
|
Code style:
These lines follow the pattern of lines that are already on
I did not change these lines. If the maintainers want the 100-character limit to apply, I can split the two new lines. Alternatively, E501 can be enabled in |
Before, _undo_replace() sent the decoded TXT value to restore an updated record. The API reads a value that starts with a quote as serialized TXT content. Thus, a TXT record with literal quotes (for example "quoted") lost the quotes after the undo. Now the undo sends the content as the API gave it. Add a unit test that makes sure the undo keeps the literal quotes. Signed-off-by: Anthony Eden <anthony.eden@dnsimple.com>
Before, _format_svcb_content() put each param value in double quotes
and did not examine the value. A value with a double quote stopped the
quoted string. A value with a backslash changed when the API read it.
Thus, the API refused the record or kept a different value.
Escaping does not solve this problem. The DNSimple API refuses a
backslash in an SVCB param value ("must not contain backslashes"), and
a double quote stops the quoted value, also when it is escaped. Thus,
the API cannot keep such a value.
Now _format_svcb_content() raises ValueError for a param value that
has a double quote or a backslash. The error occurs before the backend
sends a request, thus the existing records do not change.
Add a unit test that makes sure the backend sends no write request for
such a value. Add a live test
(tests/integration/test_dnsimple_svcb_live.py) that makes sure that
the API refuses the escaped value, and that the existing SVCB record
does not change when the backend refuses a value. The live test runs
only when DNS_AID_LIVE_TESTS=1, DNSIMPLE_TOKEN and DNSIMPLE_TEST_ZONE
are set.
Signed-off-by: Anthony Eden <anthony.eden@dnsimple.com>
|
Thanks for the SVCB validation update in 419a827. The quote/backslash case also came up in my earlier offline review. I kept the initial discussion focused on record preservation and TXT rollback so those fixes could be addressed and verified separately. There are two other observations from that review:
These were reproduced offline on 420d1ad. The relevant code is still present in 419a827, although I have not rerun those cases on the new head. Would you prefer the focused reproductions as separate comments here, or as follow-up issues where appropriate? These are separate from the two record-preservation fixes already verified. Thanks again for working through those. |
Before, _replace_records() did the last step (delete the records that are not necessary) outside the undo block. If a DELETE failed, the backend raised the error, but the new values and the old values that were not deleted were all in the zone. Now the deletes are in the undo block. If a request fails, _undo_replace() does these steps in this order: 1. Delete the created records. 2. Set the updated records back to their old content and TTL. 3. Create the deleted records again, with the content that the API gave. This order prevents a request that makes a record that is equal to an existing record. A record that is created again gets a new ID. A DELETE that gives 404 is not an error, and the undo does not create that record again. Add a unit test for a failed DELETE in the last step. Add a live test to tests/integration/test_dnsimple_replace_live.py. The API has no input that makes a DELETE fail, thus the live test makes the DELETE requests of the backend fail with a network error. All other requests go to the real API. Signed-off-by: Anthony Eden <anthony.eden@dnsimple.com>
…etes Add "dnsimple" to the allowed backend values of all MCP tools that have a backend parameter. Before this change, the MCP input validation refused backend="dnsimple" before the backend was called. Add "dnsimple" to the backend list in the publish_agent_to_dns docstring. In the DNSimple live tests, examine the status of each cleanup DELETE. A 404 is not an error (the record is already gone). Other error statuses now cause the test to fail, so that a record that the test did not remove is not ignored. Signed-off-by: Anthony Eden <anthony.eden@dnsimple.com>
Thank you. I confirmed both observations on 32ccf05 and fixed them in ea70f2f:
Separate reproductions are not necessary for these two items. If you find more items in your offline review, separate comments here are good. |
|
Tests on ea70f2f:
A full
The error is |
Add tests/integration/test_dnsimple.py. It uses the real DNSimple API, like the live tests of the other backends: - zone_exists for a zone that exists and for a zone that does not exist. - The private-use SVCB keys stay on the SVCB record. - publish_agent writes a native SVCB record and the TXT values. - A second publish_agent updates the SVCB record in place (same ID) and replaces the TXT values. It does not add records. - TXT values with spaces and quotes come back as separate values. - list_records finds a record. delete_record removes it. The cleanup tries all deletes, then makes the test fail if a delete failed. The test runs only when DNS_AID_LIVE_TESTS=1, DNSIMPLE_TOKEN and DNSIMPLE_TEST_ZONE are set. Add unit tests for the undo paths that had no tests (a failed DELETE of a created record, with 404 and 500, and a failed create of a deleted record) and for a new client when the event loop changes. The unit test coverage of the DNSimple backend is now 100%. Signed-off-by: Anthony Eden <anthony.eden@dnsimple.com>
Add the DNSimple backend to the documentation that CONTRIBUTING.md requires for a new backend: - .env.example: a DNSimple section, and dnsimple in the DNS_AID_BACKEND comment. - README.md: dnsimple in the list of backend extras, and a row in the DNS backends table. - docs/getting-started.md: the dnsimple install line, and dnsimple in the environment variable tables. - docs/api-reference.md: dnsimple in the DNS_AID_BACKEND description, and a DNSimple environment variable table. - CHANGELOG.md: an entry under [Unreleased]. - CONTRIBUTING.md: DNSimple in the backends list of the project tree. - publisher.py: dnsimple in the get_default_backend() docstring. Also add a "DNSimple Setup" section to docs/getting-started.md and a DNSimpleBackend section to docs/api-reference.md, like the sections for the other backends. These sections tell about the token types, the sandbox, the SVCB target and param value limits, and how a replace works. Signed-off-by: Anthony Eden <anthony.eden@dnsimple.com>
| i = 0 | ||
| while i < len(content): | ||
| c = content[i] | ||
| if c == "\\" and i + 1 < len(content): |
There was a problem hiding this comment.
Thanks — the offline re-audit at baadbd5 confirms the MCP selection, cleanup status handling, and final-DELETE compensation in the tested cases. The earlier raw-TXT rollback checks also still pass.
One remaining issue is in _decode_txt_content() at dnsimple.py:43–45: it drops the backslash from RFC 1035 decimal octet escapes instead of decoding the three digits.
Minimal decoder check in the PR environment (no API calls):
import dns.rdata
from dns_aid.backends.dnsimple import _decode_txt_content
content = r'"a\032b"' # Raw string: one backslash, then decimal digits 032.
expected = b"".join(dns.rdata.from_text("IN", "TXT", content).strings)
actual = _decode_txt_content(content).encode("ascii")
print("expected:", expected, "actual:", actual)
assert actual == expectedAt baadbd5, this prints expected: b'a b' actual: b'a032b' and fails. Ordinary quoted text and escaped quote/backslash controls still match the DNS parser.
There is also a false-no-op consequence in an offline HTTP reproduction: when GET returns r'"description=a\032b"', get_record() / list_records() report description=a032b. With an otherwise matching SVCB and TTL, publish(description="a032b", ...) then returns success=True with only GET requests, while the stored TXT still means description=a b.
Provider boundary: I have not captured this representation from a live DNSimple GET. The mock assumes that an already-quoted RFC 1035 value can be returned verbatim, consistent with the documented serialized-input handling. Could you confirm that round-trip behavior and, where applicable, decode decimal escapes correctly and add read/list plus replacement regression coverage? The original API content should still be retained for rollback.
References:
| params_str = " ".join(f'{key}="{value}"' for key, value in params.items()) | ||
|
|
||
| if params_str: | ||
| return f"{priority} {target} {params_str}" |
There was a problem hiding this comment.
One additional P3 finding from a separate offline security follow-up on baadbd5, independent of the TXT decoder issue.
TargetName validation gap affecting SVCB serialization
The shared AgentRecord.target_host / SvcbRecord.target validation does not ensure that the supplied value is a single DNS TargetName.
Through the public Python publish() API and the CLI, this endpoint is accepted:
agent.example.test key65400="injected"
With no explicit cap_uri, the DNSimple formatter generates:
1 agent.example.test key65400="injected" alpn="mcp" port="443" mandatory="alpn,port"
An independent dnspython parser interprets key65400 as an additional SvcParamKey, rather than part of the target hostname.
I reproduced this through the actual Python publisher and Typer CLI using an in-process mocked DNSimple API. The normal MCP publish_agent_to_dns tool correctly rejects the same malformed input via validate_endpoint, before creating the backend or sending requests.
This is not solely a DNSimple regression. The shared validation gap exists on main, and a different, unquoted variant also produces an extra SvcParamKey through the existing Route53 formatter. DNSimple exposes the quoted variant because of its trailing-dot handling.
Scope: No live DNSimple API acceptance, unprivileged caller path, authorization bypass, or downstream security exploit has been demonstrated. I would classify this as P3 defensive validation / DNS record-integrity hardening, not a confirmed exploitable vulnerability.
Could we consider:
- Validating TargetName independently from SvcParams at the shared model/publisher boundary.
- Adding a defense-in-depth target grammar check for direct DNSimple backend calls.
- Adding regression tests for Python
publish(), CLI, MCP validation, and malformed TargetName inputs.
The fix should preserve legitimate root (.), trailing-dot, and explicitly permitted internal-underscore semantics where applicable.
The audit was offline only; no real DNS zones were modified.
|
|
||
| **Limits**: The backend removes a trailing dot from the SVCB target, because the API refuses it. The backend raises `ValueError` for an SVCB param value with a double quote or a backslash, because the API cannot keep it. | ||
|
|
||
| **Replace**: The API has no atomic replace. `create_svcb_record()` and `create_txt_record()` keep equal records, update the other records in place, and create and delete records as the last steps. If a request fails, the backend undoes its changes and raises the first error. |
There was a problem hiding this comment.
One additional P3 documentation observation from a focused offline review of baadbd5.
The final-DELETE compensation is a useful improvement, and the normal failure-and-restore cases behave as intended in the tested scenarios.
However, the current wording that the backend "undoes its changes" on request failure reads as an unconditional restoration guarantee.
A separate stateful reproduction confirmed several limits:
- A failed restore POST can leave a partial record set.
- A committed DELETE with a lost acknowledgment is not recorded for restoration.
- Successfully recreated records receive new IDs.
- SVCB and TXT publication is not one atomic transaction.
Could we describe this as best-effort compensation of recorded changes, rather than guaranteed rollback?
Suggested wording:
The backend uses individual record endpoints and attempts best-effort compensation of recorded changes on failure. Restoration is not guaranteed; recreated records receive new IDs.
The statement that the DNSimple API has no atomic replace also seems broader than necessary: DNSimple documents an Enterprise/public-preview atomic batch-change endpoint. This backend uses the individual record endpoints, so its replacement sequence is non-atomic.
It would be useful to align the getting-started guide, changelog, implementation docstring, and PR description with that distinction. The SVCB parameter-value guard also runs after a zone lookup, so "before any mutating request" is more precise.
This is a documentation-level P3 suggestion, not a new functional regression. All reproductions were offline; no live DNSimple operations were performed.
This branch has not been deployed
2 participants
Description
Add a DNS backend for the DNSimple API v2.
Register the backend in the backend factory, the CLI backend registry, doctor and the MCP tools. Add the "dnsimple" extra.
Configuration: DNSIMPLE_TOKEN (required), DNSIMPLE_ACCOUNT_ID and DNSIMPLE_BASE_URL (optional).
Tests:
tests/unit/test_dnsimple_backend.py). The unit test coverage of the backend is 100%.tests/integration/test_dnsimple.py,test_dnsimple_replace_live.py,test_dnsimple_svcb_live.py). They run only when DNS_AID_LIVE_TESTS=1, DNSIMPLE_TOKEN and DNSIMPLE_TEST_ZONE are set.Documentation: README.md, docs/getting-started.md (with a "DNSimple Setup" section), docs/api-reference.md (with a DNSimpleBackend section), .env.example, CHANGELOG.md and CONTRIBUTING.md.
Type of Change
Checklist
pytest)ruff checkandruff format --checkwith no errorsmypy src/with no errorsSigned-off-byline (DCO)Related Issues