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

dnsdist: DSTPortRule #6813

Merged
merged 5 commits into from Aug 3, 2018
Merged

dnsdist: DSTPortRule #6813

merged 5 commits into from Aug 3, 2018

Conversation

phonedph1
Copy link
Contributor

Short description

Allows matching based on the destination port of the question. This allows one to apply more powerful rules to ports bound for DoT for instance.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this pull request! Code looks good, would you mind adding the new rule to g_consoleKeywords in dnsdist-console.cc so the completion is working?

}
private:
uint16_t d_port;
uint16_t d_port_htons;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how cheap htons() is, keeping the two values here feels a bit wrong.

@zeha
Copy link
Collaborator

zeha commented Jul 30, 2018

I felt like the rule should match the entire local address, not just the port... and/or match the object returned from addLocal() (if it does return something)

@rgacogne
Copy link
Member

Unless I'm mistaken, you can already use NetmaskGroupRule if you want to match the destination address.

@zeha
Copy link
Collaborator

zeha commented Jul 30, 2018

Yeah, but somehow it's weird that you cannot do both things with one matcher. I'd kinda expect something like this: LocalRule("*:443")
Exact semantics not thought through.

@phonedph1
Copy link
Contributor Author

@zeha while I don't disagree with one potentially being nice - the already existing combo of using a NetmaskGroupRule for that purpose seems just as easy to combine into whatever you want and might provide more re-use for some people.

FWIW we use NMGs to shove a number of destination addresses into a single group, so re-using that group for any port matching we do would still be nice for us. The semantics of something like AndRule({nmg group, LocalAddr("*:whatever")}) seem less intuitive.

range = newNMG()
range:addMask("8.0.0.0/24")
addAction(AndRule({NetmaskGroupRule(range, false), DSTPortRule(911)}), PoolAction("woot"))

localR = newNMG()
localR:addMask("10.10.10.10")
addAction(AndRule({NetmaskGroupRule(localR, false), DSTPortRule(911)}), PoolAction("woot"))

> showRules()
#     Matches Rule                                                     Action
0           0 (Dst: 8.0.0.0/24) && (dst port==911)                     to pool woot
1           0 (Dst: 10.10.10.10/32) && (dst port==911)                 to pool woot
>

}
private:
uint16_t d_port;
uint16_t d_port_htons;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed the code using this member, but somehow forgot to remove the member itself ;-)

@rgacogne
Copy link
Member

rgacogne commented Aug 2, 2018

Unless someone is really motivated to work on the more advanced LocalRule suggested by @zeha, which looks nice, I'd be in favor of merging this one. The code is simple enough, it has documentation and tests, and doesn't prevent adding a more advanced rule later if we want to. Any thought?

@zeha
Copy link
Collaborator

zeha commented Aug 2, 2018

Agreed

@rgacogne rgacogne merged commit 1369e46 into PowerDNS:master Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants