feat(api): initial version of check-delegation management command #816
Conversation
peterthomassen
force-pushed
the
20230928_check_delegation
branch
7 times, most recently
from
b2fef24 to
52b1215
Compare
peterthomassen
force-pushed
the
20230928_check_delegation
branch
from
d0e286c to
690331a
Compare
peterthomassen
force-pushed
the
20230928_check_delegation
branch
from
690331a to
916ae8d
Compare
nils-wisiol
force-pushed
the
20230928_check_delegation
branch
2 times, most recently
from
afa92f9 to
493f14e
Compare
nils-wisiol
force-pushed
the
20230928_check_delegation
branch
2 times, most recently
from
a1030fb to
53a4f98
Compare
nils-wisiol
force-pushed
the
20230928_check_delegation
branch
from
e505269 to
76503d0
Compare
| PARTIAL = 2 | ||
| EXCLUSIVE = 3 | ||
| MULTI = 4 |
There was a problem hiding this comment.
I supposed PARTIAL means that one, but not all deSEC NS are there, and MULTI means that there are additional foreign NS. What if both is the case?
This suggests splitting up into DelegationStatusOur (no/partial/full) and DelegationStatusForeign (no/yes) or something, for more granularity.
| delegation_status_touched = models.DateTimeField( | ||
| default=None, null=True, blank=True | ||
| ) | ||
| delegation_status_changed = models.DateTimeField( | ||
| default=None, null=True, blank=True | ||
| ) | ||
| security_status = models.IntegerField( | ||
| choices=SecurityStatus.choices, | ||
| default=None, | ||
| null=True, | ||
| blank=True, | ||
| ) | ||
| security_status_touched = models.DateTimeField(default=None, null=True, blank=True) | ||
| security_status_changed = models.DateTimeField(default=None, null=True, blank=True) |
There was a problem hiding this comment.
I don't think the _touched/_changed members need to be nullable, as the status itself can be null. That's redundant, so we can just have the fields set to the current time on creation (which probably is the default anyway).
| """ | ||
| Queries the DNS to determine the delegation status of this domian and | ||
| update the delegation status on record. | ||
| The delegation status is evaluated by trying to determine the NS records | ||
| as defined in the parent zone to the this `Domain`. | ||
| The parent zone's apex is determined by walking up the DNS tree, querying | ||
| NS records until an answer is provided. Once the apex and nameservers are | ||
| found, they are queried for the nameservers of `self.name`. | ||
| """ |
There was a problem hiding this comment.
This docstring needs some proofreading :)
| rr.address | ||
| for name in settings.DEFAULT_NS | ||
| for rrtype in {"A", "AAAA"} | ||
| for rr in list(resolver_CD.resolve(name, rrtype)) |
There was a problem hiding this comment.
| for rr in list(resolver_CD.resolve(name, rrtype)) | |
| for rr in resolver_CD.resolve(name, rrtype) |
| else: | ||
| parent_apex_candidate = parent_apex_candidate.parent() | ||
|
|
||
| # TODO what if no parnet_nameservers? what if len(parent_apex_candidate) == 0? |
There was a problem hiding this comment.
If no parent nameservers, then the domain is not delegated (because the TLD does not exist, so it is unregisterable). Should we have a special status for that? (I can see arguments both ways.)
Is the second questions about len(parent_apex_candidate) == 0 the same?
| elif auth_ns_addrs == set(): | ||
| # empty | ||
| self.delegation_status = self.DelegationStatus.NOT_DELEGATED | ||
| elif auth_ns_addrs is None: |
There was a problem hiding this comment.
How can this branch be reached? (auth_ns_addrs is assigned as a set comprehension)
There was a problem hiding this comment.
likely due to refactoring
| if self.delegation_status not in [ | ||
| self.DelegationStatus.MULTI, | ||
| self.DelegationStatus.EXCLUSIVE, | ||
| ]: | ||
| self.security_status = None | ||
| return None |
There was a problem hiding this comment.
Should also include DelegationStatus.PARTIAL for folks who accidentally use ns2.desec.io (instead of .org)
| self.security_status = self.SecurityStatus.SECURE_EXCLUSIVE | ||
| elif our_ds_set < auth_ds: | ||
| self.security_status = self.SecurityStatus.SECURE | ||
| elif auth_ds != set(): |
There was a problem hiding this comment.
| elif auth_ds != set(): | |
| elif auth_ds: |
| auth_ds = set( | ||
| resolver_CD.resolve( | ||
| self.name, dns.rdatatype.DS, raise_on_no_answer=False | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I think here we should check the AD bit (because otherwise the parent is insecure, and effectively the zone is not delegated securely). This means that we probably can't use the CD flag / resolver_CD, because (at least during an ad-hoc test), I did not get AD bits when setting CD. (As per RFC 6840 Section 5.7, it seems like requesting AD bit during CD should work (so you get unauthenticated data instead of SERVFAIL on validation failure), but it seems like nobody implements this.)
Perhaps we should store this as an extra column (boolean security_chain_complete?), so we can take different actions when the domain itself has no DS records vs. when it is under an insecure TLD (or similar)?
There was a problem hiding this comment.
yes should add another column
| resolver_CD = dns.resolver.Resolver(configure=False) | ||
| resolver_CD.nameservers = settings.RESOLVERS | ||
| resolver_CD.flags = ( | ||
| (resolver_CD.flags or 0) | dns.flags.CD | dns.flags.AD | dns.flags.RD |
There was a problem hiding this comment.
I have not succeeded in getting an AD bit in a response when querying with CD, even when also querying with AD. (See RFC 6840 Section 5.7-5.9, and also my comment below in update_dns_security_status().)
As a result, I'm not sure if this is a meaningful combination of query flags.
There was a problem hiding this comment.
Flags should instruct resolve to answer with DNSSEC records but omit checking
No description provided.