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.

Sign up for GitHub

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

Jump to bottom

WIP ZONEVERSION (RFC9660) support #14818

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

WIP ZONEVERSION (RFC9660) support #14818

wants to merge 14 commits into from
+203 −21

Conversation

Habbie
Copy link
Member

@Habbie Habbie commented Nov 4, 2024 •
edited
Loading

Short description

$ pdns/sdig 127.0.0.1 5300 www.example.com soa   zoneversion
Reply to question for qname='www.example.com.', qtype=SOA
Rcode: 0 (No Error), RD: 0, QR: 1, TC: 0, AA: 1, opcode: 0
1	example.com.	3005	IN	SOA	a.misconfigured.dns.server.invalid. hostmaster.example.com. 1731417315 10800 3600 604800 3600
2	.	0	IN	OPT	ABMABgIAZzNU4wATAAYC9nilCsY=
EDNS Zone Version (SOA serial) for labelcount 2: 1731417315
EDNS Zone Version (type 246) for labelcount 2: 2024082118

SOA serial is the served serial. type 246 is the unedited serial in our backend storage.

TODO:

  • support in auth
  • tests
  • meson updates
  • good code
  • an enum for version types

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)
  • checked that this code was merged to master

@coveralls
Copy link

coveralls commented Nov 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11799714388

Details

  • 10 of 56 (17.86%) changed or added relevant lines in 3 files are covered.
  • 76 unchanged lines in 13 files lost coverage.
  • Overall coverage decreased (-0.02%) to 64.664%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/packethandler.cc 2 5 40.0%
pdns/dnspacket.cc 8 23 34.78%
pdns/ednszoneversion.cc 0 28 0.0%
Files with Coverage Reduction New Missed Lines %
ext/json11/json11.cpp 1 64.49%
pdns/recursordist/recursor_cache.cc 1 84.13%
pdns/misc.cc 2 63.6%
pdns/sstuff.hh 2 56.83%
modules/lmdbbackend/lmdbbackend.cc 2 73.53%
pdns/recursordist/test-syncres_cc2.cc 3 88.85%
pdns/stubresolver.cc 3 77.58%
pdns/packethandler.cc 3 72.53%
pdns/iputils.cc 3 55.91%
pdns/signingpipe.cc 5 83.38%
Totals Coverage Status
Change from base Build 11796572634: -0.02%
Covered Lines: 125770
Relevant Lines: 163653
💛 - Coveralls

@Habbie Habbie added this to the auth-5 milestone Nov 4, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 7, 2024
View reviewed changes
pdns/dnspacket.cc
auto& edited_serial = auth_serial.second.second;

uint8_t labelcount = name.countLabels();
EDNSZoneVersion unedited = {labelcount, 0 /* FIXME enum */, unedited_serial};

Check notice

Code scanning / CodeQL

FIXME comment Note

FIXME comment: enum
pdns/dnspacket.cc Fixed Show fixed Hide fixed
pdns/ednszoneversion.cc
{
uint8_t labelcount;
uint8_t type;
char version[256]; // FIXME they can be bigger

Check notice

Code scanning / CodeQL

FIXME comment Note

FIXME comment: they can be bigger
pdns/ednszoneversion.cc

bool getEDNSZoneVersionFromString(const string& options, EDNSZoneVersion& zoneversion)
{
// cerr<<"options.size:"<<options.size()<<endl;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
pdns/ednszoneversion.cc
bool getEDNSZoneVersionFromString(const char* options, unsigned int len, EDNSZoneVersion& zoneversion)
{
EDNSZoneVersionWire zoneversionw{};
// static_assert(sizeof(zoneversionw) == 4, "sizeof(EDNSSubnetOptsWire) must be 4 bytes");

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
pdns/ednszoneversion.cc
EDNSZoneVersionWire zoneversionw{};
// static_assert(sizeof(zoneversionw) == 4, "sizeof(EDNSSubnetOptsWire) must be 4 bytes");
if (len > sizeof(zoneversionw)) {
return false; // FIXME this silently breaks on >256 bytes of version

Check notice

Code scanning / CodeQL

FIXME comment Note

FIXME comment: this silently breaks on >256 bytes of version
pdns/sdig.cc
} else if (iter->first == EDNSOptionCode::ZONEVERSION) {
EDNSZoneVersion zoneversion;
if (getEDNSZoneVersionFromString(iter->second, zoneversion)) {
if (zoneversion.type == 0) { // FIXME enum

Check notice

Code scanning / CodeQL

FIXME comment Note

FIXME comment: enum
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 7, 2024
View reviewed changes
pdns/dnspacket.cc

uint8_t labelcount = name.countLabels();
EDNSZoneVersion unedited = {labelcount, 0 /* FIXME enum */, unedited_serial};
EDNSZoneVersion edited = {labelcount, 246 /* FIXME enum and wrong number */, edited_serial};

Check notice

Code scanning / CodeQL

FIXME comment Note

FIXME comment: enum and wrong number
@Habbie
WIP ZONEVERSION (RFC9660) support
e821cdd 
this works:

./sdig 200.1.122.30 53 dateserial.example.com A zoneversion
Reply to question for qname='dateserial.example.com.', qtype=A
Rcode: 0 (No Error), RD: 0, QR: 1, TC: 0, AA: 1, opcode: 0
1	dateserial.example.com.	43200	IN	SOA	ns.dateserial.example.com. hsalgado.nic.cl. 2023050202 21600 7200 2592000 43200
2	.	0	IN	OPT	ABMABgMAeJVL2g==
EDNS Zone Version for labelcount 3: 2023050202

missing:
* support in auth
* meson updates
* good code
@Habbie
note type check fixme
5aaaf40
@Habbie
check type
57eed0d
@Habbie
stash partial work
59e2b52
@Habbie
auth: emit zoneversion +fake other type -always-
c8eab61
@Habbie
forgot these files
b38708a
@Habbie
use private point
c8d903d
@Habbie
sdig: print labelcount for non-0 types
b6cc735
@Habbie
only send (and even generate) zoneversion data when requested
a5c5ad8
@Habbie
use serial/edited serial correctly
496d869
@Habbie
explicitly set d_wantszoneversion
cb0c75f
@Habbie
format
9e99aa6
@Habbie
meson build, remotebackend autotools build
edfd25b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
auth feature request
Projects
None yet
Milestone
auth-5
Development

Successfully merging this pull request may close these issues.
2 participants
@Habbie @coveralls