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

[WIP] Add experimental XPF support #5594

Closed
wants to merge 9 commits into from

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Aug 11, 2017

Short description

Don't merge this while it's still using a private code point!

This PR adds experimental support for XPF1 to the recursor and dnsdist. It adds a new option to dnsdist to send the source and destination IPs and ports to the recursor via a XPF record.
It also adds a new option to the recursor to whitelist the source IPs allowed to send XPF records. Those records are then decoded before the gettag hook and the source and destination IPs are replaced by the one provided.
The PR is based on and contains @Habbie's work to add XPF support to sdig and the authoritative server's bind backend.

Checklist

I have:

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

pdns/xpf.hh Outdated
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#ifndef PDNS_XPF_HH
Copy link
Contributor

Choose a reason for hiding this comment

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

#pragma once?

@rgacogne
Copy link
Member Author

rgacogne commented Nov 2, 2017

Rebased on master, and made the XPF code point configurable in dnsdist and the rec.

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

Some review comments/questions.

@@ -295,20 +296,29 @@ void MOADNSParser::init(bool query, const char *packet, unsigned int len)

d_answers.push_back(make_pair(dr, pr.d_pos));

/* XXX: XPF records should be allowed after TSIG as soon as the actual XPF option code has been assigned:
Copy link
Member

Choose a reason for hiding this comment

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

Why would assignment change this rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this function is used in the auth, rec and dnsdist so using a variable is painful, while of course not impossible. This becomes very easy once we have a fixed value to allow.

pdns/xpf.cc Outdated

#include "xpf.hh"

std::string generateXPFPayload(bool tcp, const ComboAddress& remote, const ComboAddress& local)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename remote/local to source/destination? That mirrors the XPF spec text and seems less confusing to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks better indeed, I'll do it after your changes have landed on this branch.

pdns/qtype.hh Outdated
@@ -125,7 +125,8 @@ public:
CAA=257,
DLV=32769,
ADDR=65400,
ALIAS=65401
ALIAS=65401,
XPF=65422
Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess that would break mastermake. Now I see why fixing sdig is harder.

pdns/qtype.hh Outdated
@@ -217,6 +218,7 @@ private:
qtype_insert("DLV", 32769);
qtype_insert("ADDR", 65400);
qtype_insert("ALIAS", 65401);
qtype_insert("XPF", 65422);
Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

The server will trust XPF records found in queries sent from those netmasks (both IPv4 and IPv6),
and will adjust queries' source and destination accordingly. This is especially useful when the recursor
is placed behind a proxy like dnsdist.
Note that the `allow-from`_ setting is still applied to the original source address, and thus access restriction
Copy link
Member

Choose a reason for hiding this comment

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

'original source address' is a bit confusing, but I'm struggling to find better words.

Copy link
Contributor

Choose a reason for hiding this comment

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

"IP header source address", possibly? (If that is the correct interpretation)

The note removes what seems like probably the single most obvious application for this. Maybe name some example(s) where it can be used?

@rgacogne rgacogne deleted the ddist-xpf branch February 12, 2018 09:13
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

4 participants