Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCAction and possibly other response actions doesn't set RA and AD correct #8534

Closed
paddg opened this issue Nov 14, 2019 · 2 comments · Fixed by #8556
Closed

TCAction and possibly other response actions doesn't set RA and AD correct #8534

paddg opened this issue Nov 14, 2019 · 2 comments · Fixed by #8556
Labels
Milestone

Comments

@paddg
Copy link
Contributor

@paddg paddg commented Nov 14, 2019

  • Program: dnsdist
  • Issue type: Feature request

Short description

TCAction() never sets the RA bit in its answers. This could confuse DNS clients. Also AD is always set if AD was set in the query.

From IRC:

<winfried> If dnsdist anweres with tc flag because of a TCAction(), I get "WARNING: recursion requested but not available"
<winfried> can thist be set somehow? Behind this dnsdist is a Recursor
<winfried> Or in other words, can it confuse DNS clients?
<ahu> this could potentially confuse clients yes
<ahu> I don't think it will but it could
<winfried> ahu: so it's possibly better in this case to use any-to-tcp=yes in the Recursor instead of dnsdists TCAction
<ahu> likely, but also we should have a flag for TCAction
<ahu> perhaps we do even
<winfried> "Create answer to query with TC and RD bits set, to force the client to TCP."
<ahu> sounds wrong
<winfried> RA is missing
<ahu> you'd think it would copy in the RD bit from the query
<ahu> reading the code
<ahu> indeed
<ahu> we don't copy in the RD bit
<ahu> or do anything with it
<Maik> Just copying from the query could create another edge case when fronting an auth
<Maik> "This server only allows recursion when it blocks the query"
<ahu> I think we should let the RD bit be set by the operator
<ahu> and copy in RD from the query
<Maik> Sounds right
<ahu> I can see why we don't btw, TCAction really is one of the primordial actions
<ahu> it is special cased
<ahu> you can't really add a parameter to it
<winfried> ahu, I just noticed, the AD bit is also set if TCAction sends an answer, is this mandatory?
<ahu> that is super weird
<ahu> it might copy that in from the query
<lieter> winfried: perhaps the TruncateAction needs a setRA flag
<winfried> lieter, yes seems so
<winfried> lieter, but other actions maybe as well? What about RCodeAction or ERcodeAction
<ahu> you could do it right with other actions I think
<hawk> winfried, lieter: Yes, I think all the actions that lead to dnsdist-originated responses have the same problem
<hawk> (At least both TC and RCode do)

Usecase

In my case, dnsdist is in front of Recursors. dnsdist must always set the RA bit because the DNS clients expect that.

Description

dnsdist should give the possibility to set flags in the self generated answers such as RA or possibly other flags as well.

@rgacogne rgacogne added this to the dnsdist-1.4.0 milestone Nov 14, 2019
@rgacogne
Copy link
Member

@rgacogne rgacogne commented Nov 14, 2019

At a quick glance, we might get that wrong in:

  • ERCodeAction() ;
  • HTTPStatusAction() ;
  • RCodeAction ;
  • TCAction(), or anything that returns DNSAction.Truncate including dynamic blocks.

For all of these expect the last one, we can add optional parameters to specify whether AA, RA and AD should be set (perhaps we could just clear AD, btw). For the last one it's a bit more complicated since we can't take parameters there, I think we should just set RA = RD, clear AA and AD in that case.

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Nov 14, 2019

For what it's worth, SpoofAction sets RA = RD and clears AD, AA is untouched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants