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

Add the basic EDE (RFC8914) cases #604

Merged
merged 103 commits into from
May 6, 2022
Merged

Add the basic EDE (RFC8914) cases #604

merged 103 commits into from
May 6, 2022

Conversation

TCY16
Copy link
Contributor

@TCY16 TCY16 commented Jan 12, 2022

this branch is a split from #504 which implements all the easier EDE cases.

wtoorop and others added 30 commits June 24, 2021 11:20
when refusing to answer authoritatively.
Also remove TODO comments that were already done
Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Code looks good! Hopefully the review fixes a couple of issues.

util/config_file.c Outdated Show resolved Hide resolved
util/config_file.c Outdated Show resolved Hide resolved
validator/val_utils.c Outdated Show resolved Hide resolved
Copy link
Member

@gthess gthess left a comment

Choose a reason for hiding this comment

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

Nice work! Here is the first part of the review (missing review of validator.c and all the test cases).
The review comments are mostly nits but there are also some suggestions and some changes that I would like to see.
I'll start working on the second part of the review.

daemon/worker.c Outdated Show resolved Hide resolved
doc/unbound.conf.5.in Outdated Show resolved Hide resolved
services/localzone.c Outdated Show resolved Hide resolved
services/localzone.c Outdated Show resolved Hide resolved
Comment on lines +1295 to +1296
m->s.env->need_to_validate && (!(r->qflags&BIT_CD) ||
m->s.env->cfg->ignore_cd) && rep &&
Copy link
Member

@gthess gthess Mar 30, 2022

Choose a reason for hiding this comment

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

@rijswijk suggested to have this functionality configured. Default would be to not attach if CD is set.

Edit: Through discussion with @TCY16 and @gthess we've decided that we will leave this for when we implement storing EDE in the cached records.

validator/val_utils.c Outdated Show resolved Hide resolved
validator/val_utils.c Outdated Show resolved Hide resolved
validator/val_utils.c Outdated Show resolved Hide resolved
validator/val_utils.c Outdated Show resolved Hide resolved
validator/val_utils.c Outdated Show resolved Hide resolved
@TCY16 TCY16 changed the title Add the simple EDE (RFC8914) cases Add the basic EDE (RFC8914) cases Apr 25, 2022
Copy link
Member

@gthess gthess left a comment

Choose a reason for hiding this comment

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

Finished the second part of the review.
Mostly nits (with clickable suggestions) but I believe a couple of tests should expect a different EDE value.

testdata/black_key_entry.rpl Outdated Show resolved Hide resolved
testdata/black_ds_entry.rpl Outdated Show resolved Hide resolved
testdata/black_prime_entry.rpl Outdated Show resolved Hide resolved
testdata/ede_cache_snoop_noth_auth.rpl Outdated Show resolved Hide resolved
testdata/ede_acl_refused.rpl Outdated Show resolved Hide resolved
testdata/serve_original_ttl.rpl Outdated Show resolved Hide resolved
testdata/val_faildnskey.rpl Outdated Show resolved Hide resolved
testdata/val_nsec3_nods_badsig.rpl Outdated Show resolved Hide resolved
testdata/ede.tdir/ede.test Outdated Show resolved Hide resolved
testdata/ede.tdir/ede.conf Outdated Show resolved Hide resolved
Copy link
Member

@gthess gthess left a comment

Choose a reason for hiding this comment

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

Seems good!
Only thing that is pending is changing serve-expired-ede to ede-serve-expired and checking the global ede option together with that option.
Maybe also renaming the global option to just ede for easier grouping with the (future) ede options/toggles as discussed.

Copy link
Member

@gthess gthess left a comment

Choose a reason for hiding this comment

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

LGTM, ready for merging!

@TCY16 TCY16 merged commit 0ce36e8 into master May 6, 2022
jedisct1 added a commit to jedisct1/unbound that referenced this pull request May 25, 2022
* nlnet/master:
  - Fix some lint type warnings.
  - Fix ede test to not use default pidfile, and use local interface.
  - Fix to silence test for ede error output to the console from the   test setup script.
  - Fix typos in config_set_option for the 'num-threads' and   'ede-serve-expired' options.
  - Fix NLnetLabs#678: [FR] modify behaviour of unbound-control rpz_enable zone,   by updating unbound-control's documentation.
  - For NLnetLabs#677: Added tls-system-cert to config parser and documentation. - Changelog note for NLnetLabs#677.
  Allow using system certificates not only on Windows
  - Fix NLnetLabs#417: prefetch and ECS causing cache corruption when used   together.
  - Fix NLnetLabs#673: DNS over TLS: error: SSL_handshake syscall: No route to   host.
  - Fix Python build in non-source directory; based on patch by   Michael Tokarev.
  Changelog entry for NLnetLabs#604: Add the basic EDE (RFC8914) cases
  Add the basic EDE (RFC8914) cases (NLnetLabs#604)
  - Fix NLnetLabs#670: SERVFAIL problems with unbound 1.15.0 running on   OpenBSD 7.1.
@gthess gthess deleted the features/ede-basic branch July 9, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants