-
Notifications
You must be signed in to change notification settings - Fork 20
Switch to new hickory-resolver release and harden security settings #554
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
Conversation
|
I've started on the dependency security review, see internal documentation. |
e95e7b2 to
2daeb50
Compare
|
This PR will require another rebase after #539 is merged. |
2daeb50 to
0216d1d
Compare
|
@r-n-o @mark-nesbitt I've finished the dependency security review, see internal discussion and documentation. |
r-n-o
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
| [[package]] | ||
| name = "rustc_version" | ||
| version = "0.4.1" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "cfcb3a22ef46e85b45de6ee7e79d063319ebb6594faafcf1c225ea92ab6e9b92" | ||
| dependencies = [ | ||
| "semver", | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see this listed in the internal documentation, but this might be because it's already part of qos (in qos_enclave's Cargo.lock)
| [[package]] | ||
| name = "tracing-attributes" | ||
| version = "0.1.27" | ||
| name = "tracing-core" | ||
| version = "0.1.32" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "34704c8d6ebcbc939824180af020566b01a7c01f80641264eba0999f6c2b6be7" | ||
| checksum = "c06d3da6113f116aaee68e4d601191614c9053067f9ab7f6edbcb161237daa54" | ||
| dependencies = [ | ||
| "proc-macro2", | ||
| "quote", | ||
| "syn 2.0.100", | ||
| "once_cell", | ||
| "valuable", | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: not listed in the sheet but fine given it's already in src/qos_enclave/Cargo.lock 👍
| /// Check how the upstream resolver deals with a known-bad DNSSEC protected domain | ||
| /// It does NOT actively test the security behavior of our local DNSSEC verification | ||
| #[test] | ||
| fn test_lookup_domain_bad_dnssec_record() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test coverage here!
|
@r-n-o thank you. As you already mentioned, crate versions that we already carry in trusted lockfiles are automatically trusted for changes in other lockfiles, and so aren't considered as a change of the trust situation (= not listed in the change set). I can consider options for calling those out automatically, but I'll have to see if the corresponding addition of complexity in the tooling is worth it. |
|
Also, for the record: c0aac25 avoids a footgun situation where DNSSEC is requested but not actually enabled. This is now reported and fixed upstream via hickory-dns/hickory-dns#3089. hickory-dns/hickory-dns#3090 outlines the report and upstream bug fixes for a problematic edge case that we ran into during testing. Based on the developer's feedback, the corresponding fixes will likely only become available to us once we switch to future |
Summary & Motivation (Problem vs. Solution)
This PR has three aspects:
hickory-resolver0.24.4to0.25.2, and the significant transitive dependency changes that requires.hickory-resolverversion.The motivation for 1) and 3) are recommendations from a recent Trail of Bits audit of the
qos_netcomponent. The0.25.xbranch came out in March and has some notable improvements. Step 2) is a necessary side effect.Via 3), we now enable opportunistic DNSSEC validation as well as case randomization, which helps under some circumstances.
For 2), it is now necessary to have an explicit Tokio runtime handle. We create it on the
Proxylevel, since instantiating it ad-hoc at theProxyConnectionlevel is too costly. Since we block synchronously on the (formally async) DNS request, threading should effectively work as it did before.Note for the code reviewer: the Tokio runtime needs to
dropto clean up resources. By my understanding, it will do so automatically once thestruct Proxydoes, but this may be worth double-checking.Note for dependency security review: I haven't looked at the crate changes yet, and will document that in the comments.
How I Tested These Changes
Local tests.
Due to the changes to the DNS resolution behavior (notably DNSSEC usage), and potential related performance changes, this PR should have some regression testing in pre-prod environments before production usage.
If the proxy attempts to resolve a domain that has broken DNSSEC settings, we now expect this to fail. In most combinations, it would have failed already before this change (namely by the upstream resolver understanding DNSSEC and triggering a
SERVFAILon its own), but in theory this could now locally spot and reject problematic records that were previously accepted.Pre merge check list