rec: Generate EDE in more cases, specifically on unreachable auths or sythesized results.#12334
Conversation
…hesized results. As there is no specific EDE for synthesised, use noError with a text. We have to be careful here: a single client query can lead to multiple beginResolve calls. Some of these are done after the main result has been looked up, for example to validate the result. These subsequent calls can generate EDE's but we do not want to copy the EDE to the main result in those cases. A typical example would be an absent DS for an Insecure domain. Nothing wrong with these but we do not want the potential absent DS EDE (which could be synthesize) to be returned with the main query, To solve this, mimic the processing of validation state and add an extra argument to a few methods. I am not terribly happy with the extra argument. Maybe we should move to an object holding the parameters and result status of the nested or subsequent calls. This would also avoid some of the saveX, setX, beginResolve, restore X sequences. So marking this as Draft for now.
My rule of thumb is, when I'm pondering adding an object to group values it's pretty much always the right call :) |
Later, more fields that apply to a specific beginResolve call might be added.
omoerbeek
force-pushed
the
rec-more-edns
branch
from
3198242
to
718c531
Compare
![github-advanced-security[bot]](https://cdn.sessionspy.com:443/index.php?https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -2854,13 +2865,13 @@ | |||
|
|
|||
| if (giveNegative) { | |||
|
|
|||
| state = cachedState; | |||
| context.state = cachedState; | |||
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable
pdns/recursordist/syncres.cc
Outdated
| @@ -2822,6 +2826,10 @@ bool SyncRes::doCacheCheck(const DNSName& qname, const DNSName& authname, bool w | |||
| else { | |||
| LOG(prefix << qname << ": Entire name '" << qname << "' is negatively cached via '" << ne.d_auth << "' for another " << sttl << " seconds" << endl); | |||
| } | |||
| if (s_addExtendedResolutionDNSErrors) { | |||
| // XXX Do we want this? | |||
| context.extendedError = EDNSExtendedError{0, "Result from negative cache"}; | |||
There was a problem hiding this comment.
That would make sense to me, at least when the entire name is negatively cached (and it would be nice to convey that in our EDE message).
There was a problem hiding this comment.
A push a commit making the distinction
| @@ -2992,7 +3003,7 @@ bool SyncRes::doCacheCheck(const DNSName& qname, const DNSName& authname, bool w | |||
| if (!giveNegative) | |||
| res = 0; | |||
| LOG(prefix << qname << ": updating validation state with cache content for " << qname << " to " << cachedState << endl); | |||
There was a problem hiding this comment.
Perhaps we might want to add an EDE status on positive hits as well? But that might be a bit too much?
There was a problem hiding this comment.
IMO that would be a bit too much
Examples:
As there is no specific EDE for synthesized, use noError with a text for now.
We have to be careful here: a single client query can lead to multiple beginResolve calls. Some of these are done after the main result has been looked up, for example to validate the result. These subsequent calls can generate EDE's but we do not want to copy the EDE to the main result in those cases. A typical example would be an absent DS for an Insecure domain. Nothing wrong with these but we do not want the potential absent DS EDE (which could be synthesized) to be returned with the main query,
Update: In the meantime, a EDE was requested and assigned: 29: "Synthesized" (https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#extended-dns-error-codes).
This EDE is intended for cases where the recursor decided to synthesise (construct an answer without getting it verbatim from an authoritative server previously), based on e.g. the methods described in RFC8020 (NXDOMAIN: There Really Is Nothing Underneath https://www.rfc-editor.org/rfc/rfc8020) and RFC 8198 (Aggressive Use of DNSSEC-Validated Cache https://www.rfc-editor.org/rfc/rfc8198.html).
Short description
Checklist
I have: