Skip to content

resolved: always return the validated answers when validating#31952

Merged
yuwata merged 1 commit intosystemd:mainfrom
rpigott:resolved-valid-subset
Mar 27, 2024
Merged

resolved: always return the validated answers when validating#31952
yuwata merged 1 commit intosystemd:mainfrom
rpigott:resolved-valid-subset

Conversation

@rpigott
Copy link
Copy Markdown
Contributor

@rpigott rpigott commented Mar 26, 2024

We normally expect sd-resolved only to return the validated subset of a validated response. In some cases we give up on validating, because we have enough information already to conclude the answer is bogus.

Let's be sure to always reply with only the validated subset in these cases too, so that we don't return bogus answers and confuse primitive clients that won't see the SERVFAIL rcode.


Fixes: #24827 #32238

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Mar 26, 2024
@yuwata
Copy link
Copy Markdown
Member

yuwata commented Mar 26, 2024

Is it possible to add a test case for this? Or already tested? cc @mrc0mmand

@yuwata yuwata added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Mar 26, 2024
@rpigott rpigott force-pushed the resolved-valid-subset branch 2 times, most recently from cd588a7 to 4e81543 Compare March 26, 2024 19:01
We normally expect sd-resolved only to return the validated subset of a
validated response. In some cases we give up on validating, because we
have enough information already to conclude the answer is bogus.

Let's be sure to always reply with only the validated subset in these
cases too, so that we don't return bogus answers and confuse primitive
clients that won't see the SERVFAIL rcode.
@rpigott rpigott force-pushed the resolved-valid-subset branch from 4e81543 to 19d7f01 Compare March 26, 2024 19:14
@rpigott
Copy link
Copy Markdown
Contributor Author

rpigott commented Mar 26, 2024

Is it possible to add a test case for this?

I don't think there is a test atm. The domains I used to test this are nosig.go.dnscheck.tools and badsig.go.dnscheck.tools, which hit the new bits in dnssec_ready and validate_dnssec respectively, if it helps. IIUC something in the untrusted.test zone from the testsuite can probably hit the same bit as nosig.go.dnscheck.tools, if you try to query the stub with dnssec=yes.

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Mar 26, 2024
@yuwata yuwata merged commit 0dfea62 into systemd:main Mar 27, 2024
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Mar 27, 2024
@rpigott rpigott deleted the resolved-valid-subset branch March 27, 2024 01:23
@poettering
Copy link
Copy Markdown
Member

Hmm, I wonder if it wouldn#t be better to simply clear the answer structure in dns_transaction_complete() for all result codes that aren't a success. Seems cleaner to me, as it would cover all result codes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

systemd-resolved includes response in SERVFAIL answer in case of dnssec bogus

5 participants