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

Implement custom DNS resolver #3988

Merged
merged 2 commits into from
Apr 27, 2024
Merged

Implement custom DNS resolver #3988

merged 2 commits into from
Apr 27, 2024

Conversation

dani-garcia
Copy link
Owner

The goal of this change is to protect us more thoroughly against DNS rebinding attacks and redirects in the icons service, previously we did a manual lookup check before doing the initial HTTP request, which would leave us vulnerable, by inserting a middleware DNS resolver into reqwest we should be protected against all cases.

Also I noticed that with the hickory-resolver crate we can enable DNS over TLS and DNS over HTTPS, which seems like a great option, but we'd need to test further.

Also moved the is_global_ip functions to utils, because it was crowding the already big icons file.

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

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

Other then the missing cached proc-macro it looks ok too me.
I tested it with DDoSing the Favicon endpoint, that seems to work ok.

I also did a longer test which test around 300 domains (including some duplicates with/without redirects). These all seem to work as on the current code.

src/api/icons.rs Show resolved Hide resolved
@BlackDex
Copy link
Collaborator

Did some further testing and seems to work fine.
I only wonder if we do not want to have this custom resolver as a default for everything.
It can also be useful for the calls to HIBP, Bitwarden Push, Calls to DUO or YubiKey OTP.

Other than that, it's all good.

@PKizzle
Copy link
Contributor

PKizzle commented Dec 31, 2023

I just had a brief look at the PR so maybe I missed it but is it possible to disable this feature in the configuration? I am already using a custom secure DNS solution and rather use that one as it gives me a more fine-grain control.

@BlackDex
Copy link
Collaborator

I just had a brief look at the PR so maybe I missed it but is it possible to disable this feature in the configuration? I am already using a custom secure DNS solution and rather use that one as it gives me a more fine-grain control.

It still uses the DNS servers provided by the host, so no need i think.

@dani-garcia dani-garcia force-pushed the icons_dns branch 2 times, most recently from 99d1fab to 0dd13e8 Compare March 23, 2024 14:54
BlackDex
BlackDex previously approved these changes Apr 6, 2024
src/api/icons.rs Show resolved Hide resolved
@dani-garcia dani-garcia merged commit 27dc67f into main Apr 27, 2024
8 checks passed
@dani-garcia dani-garcia deleted the icons_dns branch April 27, 2024 18:25
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