Extended dns settings

[HorvathAkosPeter]

• TCP or UDP connection is now configurable and not automatic
• Adds support for newer DNS protocols (DoT, DoH, DoQ)
• DNS ports can now be determined in the domain form
• Secret key names can now be determined as in the bind configuration
• If there is a DNS connection/update problem, it gives the detailed error message in the form error field.

Consider this as a draft, I am very open for suggestions, or a wishlist.

[HorvathAkosPeter]

Ok I see you have triggered a lint, another push which cleans up all the reports follows soon.

[ThomasWaldmann]

Did you ever try git rebase -i master for your PR branches?

Avoids all these merge commits and one can also reword or squash some commits, if needed, and other cleanups.

[HorvathAkosPeter]

I think you dislike the complex branch histories. That I can agree.

I dislike rebase because it deviates from chronological order, and creates retroactively never existed states. But I could do it if you wish. What I would more prefer, that is the nearly unknown git merge --squash.

Normally it is not so bad, but this branch had already a lot of reverse merges, to keep it uptodate.

I have now force-pushed a squashed merge into this PR. As you can see, the changes are exactly the same, but it is only a single commit.

I preserved my old branch, right before the squash, with its really complex history, it is here: https://github.com/HorvathAkosPeter/nsupdate.info/tree/extended_dns_settings_orig .

In my opinion, the main advantage of the git merge --squash is that it unifies the often complex and unneeded history of a PR into a single commit. In my ideal world, I believe, the best would be if the master branch would have exclusively such squashed merges. 1 PR, 1 commit, so.

[HorvathAkosPeter]

@ThomasWaldmann Sorry for the inconvenience, may I ask for a review? It is the last PR in this serie, and also the most important (all my former ones were needed to make this PR possible).

[ThomasWaldmann]

Please be patient, I already told you that major new features will have to wait until after the pending 0.13 release.

[ThomasWaldmann]

Finally reviewed this. Looks good overall, just some minor nitpicks.

Guess there are no automated tests at all for the new code, could we test this somehow?

Did you manually test it?

[ThomasWaldmann]

keep the change minimal, do not rename "domain".

[HorvathAkosPeter]

It was also a conceptional change. "domain" was a variable refering a domain, thus it could be a string. Now domain is an entity with properties, among these properties are the "name" as a unique identifier. This is why I renamed, if you wish I can rename it back but I think it would be a quality decrease.

Alternatively, maybe a bit of re-organization of the large set of utility functions in dnstools.py into classes? That might be also a future task.

[ThomasWaldmann]

logger.debug should get a formatted string as param, not a tuple.

[HorvathAkosPeter]

Yes but the tuple was auto-converted to a formatted string. :-) I think that logger.debug is not really needed, it was better to remove.

[ThomasWaldmann]

maybe it would be a bit cleaner to do the form processing at the callers site and just have separate params for these instead of giving the form dict into here.

[HorvathAkosPeter]

Yes, my goal was here a minimal change. Actually I see a larger refactor option here, namely that dnstools.py looks like a bit unstructured function set. I think it is already enough big to give it maybe some OO or so.

However, this check_domain function is doing also persistent entity operations, I think it would be ugly to do such a thing in a form.py, it would also violate MVC.

My consideration was a minimal change now, and a cleanup initiative in the future.

However, these 6 lines could be significantly simplified, like a for loop or as some like a partial merge operation.

[ThomasWaldmann]

Simplicity is also worth something. Prefer simple assignments, either here or at the callers site, using kwargs.

[HorvathAkosPeter]

Uh I remember, as I can see, it is actually updating the database while the name of the function is "check_domain". I am not sure, how should it work, but afaik that is already misleading. I have now renamed "d" to "domain", to mirror better the concept ("domain", "domain_name" and "domain_data" are 3 different things). But I am not sure, how further.

Afaik here was some trick to use maybe the model validators to check the data? I am not sure.

[ThomasWaldmann]

would it be possible for the user to leave some required fields empty?

[HorvathAkosPeter]

No, ip, port and protocol must be all filled and valid.

[ThomasWaldmann]

See previous Meta feedback.

[ThomasWaldmann]

ok, so the key name is identical with the domain name.

[HorvathAkosPeter]

Yes, this is the default, what the generate_ns_secret function is making.

If you do not generate the key with the internal key generator functionality, or if you alter it later, then it will be different.

For example, I actually had already a zone key and secret in my bind configuration, and it was different from the domain name. Then I have spent hours to debug, why it does not accept the key.

But, the important thing is, that it should be configurable in the name server configuration. Reason is that very often, the configuration is, that you have a single key to manage all the dynamical zones.

[ThomasWaldmann]

Note: I am working on CI support for DoT / DoH testing, see PRs.

Update: it is merged into master branch now, please rebase again.

[HorvathAkosPeter]

Finally reviewed this. Looks good overall, just some minor nitpicks.

Guess there are no automated tests at all for the new code, could we test this somehow?

Did you manually test it?

Yes, I did all with hand. I do not even know yet, where are the tests. But I will catch it up soon and I am back with an improved PR.

[ThomasWaldmann]

The tests are found in the _tests subfolders, close to the code they test.

[HorvathAkosPeter]

Ok, tests really don't go through and I think it might have a connection to the recent testing improvements. My current best bet is to play with it on my local system until all the tests pass. That might result also subsequent testing PRs. If you have a better idea, please share it with me.

[ThomasWaldmann]

This review was performed by Claude Opus 4.6 (Thinking).

---

Critical Issues

1. check_domain() doesn't restore all temporarily-modified fields

The finally block only restores available and nameserver_ip, but the PR now also mutates nameserver_port, nameserver_protocol, nameserver2_ip, nameserver2_port, and nameserver2_protocol. If an exception is raised, those fields are left permanently overwritten in the database.

# currently in finally block:
    domain.available = domain_available_state
    domain.nameserver_ip = domain_nameserver_ip
    domain.save()
# Missing: restore of nameserver_port, nameserver_protocol,
#          nameserver2_ip, nameserver2_port, nameserver2_protocol

2. nameserver2_ip can be None → make_nameserver() will crash

nameserver2_ip is blank=True, null=True. When it's None, make_nameserver() passes None to UdpNameServer(None, port) / dnspython constructors, which will raise. get_ns_info() unconditionally calls make_nameserver() for both ns1 and ns2:

ns2 = make_nameserver(d.nameserver2_protocol, d.nameserver2_ip, d.nameserver2_port)

There is no None guard. The existing code handled this correctly by letting nameserver2 be None and checking if nameserver2: in query_ns().

3. query_ns() — resolver.nameservers expects IP strings, not nameserver objects

Since get_ns_info() now returns custom nameserver objects, but resolver.nameservers expects a list of plain IP address strings, all DNS queries will break. The nameserver objects approach is only valid for dns.query.* / dns.asyncquery.* calls, not for dns.resolver.Resolver.

Also, the original prefer_primary / fallback logic for ns2 was removed.

4. CreateDomainForm now exposes public + available at creation time

Previously the create form had: ['name', 'nameserver_ip', 'nameserver2_ip', 'nameserver_update_algorithm', 'comment']

Now it includes public, available, and nameserver_update_secret. This bypasses the connectivity check that only runs in EditDomainForm.clean(). A user could create a domain marked as available + public without ever passing check_domain() validation.

5. generate_ns_secret() return-type change is a breaking API change

# Before:
return secret_base64
# After:
return self.nameserver_update_key_name, secret_base64

Any other caller of generate_ns_secret() (management commands, tests) will break. Only the view was updated to destructure the tuple.

---

Security Concern

The model_to_dict(d) and str(cleaned_data) debug logging will leak the TSIG shared secret to log files:

logger.debug("get_ns_info: domain: " + str(model_to_dict(d)))  # leaks secret
logger.debug("cleaned_data: " + str(cleaned_data))              # leaks secret

---

Moderate Issues

• import traceback in dnstools.py is unused.
• import dns.nameserver in dnstools.py is unused (classes come from nsupdate.utils.dnspython).
• from django.forms.models import model_to_dict is only used for one debug log line — should be removed with that line.
• RangeIntegerField uses model-level validators but doesn't override formfield(), so HTML5 min/max attributes won't appear on the widget.
• Merge migration 0017 depends on ('main', '0016_alter_domain_nameserver_ip') which is not included in this PR.
• nameserver_update_key_name defaults to '' — existing domains will have empty key names, which could cause TSIG failures if an update is attempted before setting the key name.
• async_query parameter defaults are inconsistent between UdpNameServer (source: str | None = None) and TcpNameServer (source: str | None — no default).
• UdpNameServer.__init__ and TcpNameServer.__init__ just call super().__init__() with no added logic — they're redundant and can be removed.
• DNS updates via DoT/DoH/DoQ are unverified — unclear if dnspython's nameserver .query() implementations handle dns.update.Update messages correctly for all protocol backends.

Style

• Several leftover debug log calls remain from the previous review round (in add(), query_ns(), update_ns(), get_ns_info()).
• The logger.error("DNS error, raising upward: ...") in check_domain() should be debug or removed — the caller handles the error.
• Pure formatting change on the "performing %s..." log line (unwrapping) — previously flagged as unnecessary.

Missing Pieces

• No automated tests.
• No data migration for existing rows (empty key_name on existing domains).
• No admin integration for the new fields.

[HorvathAkosPeter]

To make this PR test-clean, I need some additional work. I temporarily convert it back to a draft.

[HorvathAkosPeter]

@ThomasWaldmann Hi, I have tuned this PR a lot. Probably many of the problems of the claude were solved. Beside that, testcases are running all - except two, the DoH and DoT query/update.

At least, not here. They run without any problem in my own test environment (which is not tox/docker based, it is simply using a test-bind in a VM). Here they say "connection refused", which is not clear, why. The first failure can not find the record it created right before.

Neither problem appear on my system and I can not really track, what is going on in the github actions.

What I would ask for:

1. Create another report from the claude. I think I am still far from a merge, but I would need a feedback, what to focus yet.
2. Consider disabling the problematic testcases. As I can see, some testcases are already disabled in the CI. On my testsystem, all of them pass, but I can not track this, I can not even get a more verbose log output.
3. How about extending the docker based testsystem? As I can see, here is tox running, which is (yet) another python test suite, but I am not even sure if it is running in containers or not. My current best idea is to use a mixed container system. There, the testcases would run on the host system (enabling, for example, pycharm line tracking / breakpoints), but the test django and the containerized bind would run in docker. My idea is to have exactly the same in the github actions.