mirror of
https://github.com/hickory-dns/hickory-dns.git
synced 2026-04-25 03:05:51 +03:00
[GH-ISSUE #2072] trust-dns-client/dns-over-https-openssl pulling in rustls in v0.23+ #872
Labels
No labels
blocked
breaking-change
bug
bug:critical
bug:tests
cleanup
compliance
compliance
compliance
crate:all
crate:client
crate:native-tls
crate:proto
crate:recursor
crate:resolver
crate:resolver
crate:rustls
crate:server
crate:util
dependencies
docs
duplicate
easy
easy
enhance
enhance
enhance
feature:dns-over-https
feature:dns-over-quic
feature:dns-over-tls
feature:dnsssec
feature:global_lb
feature:mdns
feature:tsig
features:edns
has workaround
ops
perf
platform:WASM
platform:android
platform:fuchsia
platform:linux
platform:macos
platform:windows
pull-request
question
test
tools
tools
trust
unclear
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/hickory-dns#872
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @decathorpe on GitHub (Oct 20, 2023).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2072
It appears that with the release of trust-dns v0.23 (and still present in git main), there was some reshuffling of crate features that causes the "dns-over-https-openssl" feature of trust-dns-client to pull in "trust-dns-proto/dns-over-rustls", which seems to be unintentional:
trust-dns-client/dns-over-https-openssl -> trust-dns-client/dns-over-https
trust-dns-client/dns-over-https -> trust-dns-proto/dns-over-https
trust-dns-proto/dns-over-https -> trust-dns-proto/dns-over-rustls
To me it appears that the generic "trust-dns-proto/dns-over-https" feature should actually be called "dns-over-https-rustls"? There is also no "dns-over-https-openssl" feature in trust-dns-proto, but there is "dns-over-https-rustls".
To match the structure of the other trust-dns-* crates, I assume there would be a generic (backend-agnostic) "dns-over-https" feature, and two features for "dns-over-https-rustls" and "dns-over-https-openssl", which both depend on "dns-over-https", and with rustls-backend specific dependencies only in "dns-over-https-rustls", not in "dns-over-https".
@decathorpe commented on GitHub (Oct 20, 2023):
To clarify what I mean, I would replace these lines from the
trust-dns-proto/Cargo.tomlfeatures table:With this:
This might not be 100% correct (I'm not sure whether the dependencies in
dns-over-httpsare only dependencies in case the rustls backend is selected or if they're also required for the openssl / native-tls backends), but it ensures that rustls is no longer pulled in when using the OpenSSL or native-tls backends in other trust-dns-* crates.Applying this patch appears to work as expected when building the trust-dns-* crates for Fedora Linux (where I am responsible for packaging them as dependencies of reqwest / awc / etc.). Depending on RusTLS in is not possible in our environment, since it doesn't support all CPU architectures that we need to support, so rustls (accidentally / unnecessarily?) getting pulled in by
dns-over-https-opensslis not good.@bluejekyll commented on GitHub (Oct 20, 2023):
Yes, there might have been a bit of an oversight on some reviews in this area. Can you look at the new release in
0.24(requires the change to the new branding, hickory). I think the dependencies and features might have been fixed in this area?@decathorpe commented on GitHub (Oct 20, 2023):
Thanks for looking into it!
This is still the case in git main: https://github.com/hickory-dns/hickory-dns/blob/main/crates/proto/Cargo.toml#L50
EDIT: This is what it looks like in v0.22:
https://github.com/hickory-dns/hickory-dns/blob/v0.22.1/crates/proto/Cargo.toml#L39-L45
So it appears that there was some change between v0.22 and v0.23 that messed up the feature dependencies for the
dns-over-httpsanddns-over-rustlsfeatures.I can work on a PR to sort that out, if that would be helpful?
@djc commented on GitHub (Oct 20, 2023):
Yes please!
@bluejekyll commented on GitHub (Oct 21, 2023):
also, we have a feature called
dns-over-https-opensslbut I'm pretty sure we don't actually have https support for openssl and h2 or h3 at this point. Is OpenSSL something you're interested in? (we should remove that as an option in the crates I think, if you look at theh2impls, there's no OpenSSL support there.We'v been discussing removing OpenSSL from most runtime options in favor or Rustls and ring.
@decathorpe commented on GitHub (Oct 21, 2023):
Removing support for OpenSSL would be unfortunate, since that's by far the best supported option on most Linux distributions. Once the Rustls/ring combination supports all the CPU architectures we need (probably with rustls v0.22 and ring v0.17 from what I can tell?) that will be better, but OpenSSL would still be preferred in most cases.
For example, the OpenSSL package in Fedora Linux is maintained directly by Red Hat, with timely fixes for security vulnerabilities. On RHEL itself, OpenSSL is FIPS certified, while other crypto libraries (like ring) are not. We also have guidelines for using system-provided crypto libraries if possible, so depending on "non-standard" crypto like Rustls / ring is a no-go for core system components. Additionally, cryptography libraries like OpenSSL are configured to follow a system-wide security / crypto policy (for example, which algorithms are allowed), which Rustls / ring does not (yet) support at all.
PS: I know that h3 doesn't yet support crypto backends other than Rustls / ring, which is also one of the reasons why h3 is not packaged for Fedora Linux (and the HTTP/3 / QUIC support in other crates like reqwest is disabled).
@decathorpe commented on GitHub (Oct 21, 2023):
I've opened PRs against main and the release/0.23 branch. If the changes are accepted, it would be great to get a v0.23.2 release after the PR for v0.23 is merged.
@djc commented on GitHub (Oct 21, 2023):
I actually think the current state of the features better reflects what's actually needed? As in,
proto/src/https/https_client_stream.rsdoes unconditionally require rustls (and already did so in 0.22). So maybe the feature names are misleading, but it looks to me like the current dependency tree for them is accurate relative to the code implementing the DoH feature.Why are you packaging trust-dns-proto/hickory-dns-proto? Presumably it's for some application that needs it? Are those applications part of the core system components? Maybe it would be useful to file an issue for what it would take for rustls to apply the system-wide security/crypto policy (I had not heard of this requirement before).
Support for a FIPS-approved rustls
CryptoProvideris being tracked in https://github.com/rustls/rustls/issues/1540. ring is working on FIPS approval, and we're looking into aws-lc-rs which should become FIPS approved in the future, too.@decathorpe commented on GitHub (Oct 21, 2023):
I had not realized that, thanks for checking. For contect, the three crates in Fedora that depend on trust-dns-* are:
It looks like none of them use any of the
dns-over-httpsfeatures, so I might be able to disable those features entirely in our packages. I usually try not to do patch in changes like that (to try to make the difference between crates as shipped on crates.io and available for package builds in Fedora as small as possible), but in this case, that shounds like the "cleanest" solution?@djc commented on GitHub (Oct 23, 2023):
It's interesting because all three of those crate seem to depend on trust-dns-resolver with mostly default features, which (at least on main) doesn't seem to enable the rustls features from -proto?
@decathorpe commented on GitHub (Oct 23, 2023):
They don't, but due to the way Rust packaging works on Fedora (and I assume on debian as well), dependencies for non-defsult features need to be satisfiable as well, or explicitly disabled - which I will likely do in the future for the features of trust-dns / hickory-dns that depend on rustls/ring. Once ring/rustls support the architectures that we need, I can always revert those changes.
That said, it seems like the dependency on rustls is actually intentional, so I'll close this ticket and the PRs I opened.