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 DNS over TLS #558

Closed
wants to merge 29 commits into from
Closed

Conversation

tkterris
Copy link

@tkterris tkterris commented Nov 1, 2019

Implement DNS over TLS functionality. Add a field "dotEnabled" to the DNS configuration model (Dns and DnsChoice), and a step in the custom DNS creation logic to support adding the DNS server hostname (e.g. 1dot1dot1dot1.cloudflare-dns.com).

This resolves issue #198

Add DNS over TLS functionality. Outstanding issues:
- TLS certificate is not checked (DNS still works with incorrect hostname)
- TLS hostname is hard-coded to Cloudflare's, when it should be different for the various DNS options
Next is to add the DoT hostname to the application state
Add logic to use DoT server. Entrypoint and UI need to be updated to support specifying the DoT server.
Implement some of the UI changes to support DoT.
@tkterris
Copy link
Author

tkterris commented Nov 1, 2019

Too much of a performance impact, declining the PR.

@tkterris tkterris closed this Nov 1, 2019
Remove handshake step for each call, so previous session is used.
Instead of storing a separate socket address for DoT, just use the existing dnsServers socket addresses. Added a flag, dotEnabled, that determines whether a TCP (with TLS) or UDP connection should be used.
@tkterris tkterris reopened this Nov 1, 2019
@tkterris
Copy link
Author

tkterris commented Nov 1, 2019

Still experiencing performance issues. Closing again.

@tkterris tkterris closed this Nov 1, 2019
Realized new logic was tied into the flow incorrectly. Restructured, including removing the async call. Works as expected now. Going to perform some cleanup later.
Updated TLS exception handling to match pattern for UDP, removed debug log statements, and fixed a bug when changing from DoT to standard DNS
@tkterris tkterris reopened this Nov 2, 2019
@tkterris
Copy link
Author

tkterris commented Nov 2, 2019

Turns out the "performance issues" were caused by not tying the TCP connection into the main loop properly. That's been resolved.

@tkterris tkterris force-pushed the 198-dns-tls branch 2 times, most recently from 4b83cd3 to 12e25f0 Compare November 2, 2019 17:30
Simplified the class inheritance of ForwardRule, and added hostname check to TLS connection.
app/src/full/kotlin/tunnel/DnsProxy.kt Outdated Show resolved Hide resolved
app/src/ui-blokada/kotlin/core/AddDnsActivity.kt Outdated Show resolved Hide resolved
@tkterris
Copy link
Author

tkterris commented Nov 4, 2019

Should there be some indication in the UI that a DNS server is using DoT? Perhaps by having a small lock icon next to the display name in the DNS server list? As it is, once the DNS server is selected, there's no indication that Blokada is using DoT. I've just been visiting 1.1.1.1/help to confirm that DoT is being used. Thoughts?

When custom DNS is toggled off in the main UI, it instead turns of ad blocking, leaving the custom DNS enabled. Looks like this was due to a typo in some copy-pasted code. Resolved the typo, the DNS toggle now works as expected.
Remove an unneeded nullcheck on originEnvelope. Fixed code style warnings in DnsProxy. Updated method signature of forwardTls to match forwardUdp. Also fixed a small issue with the DNS creation flow, so that empty TLS servers are accepted.
Create a separate flow for DoT server setup, with a new button for DoT servers
@ps100000
Copy link

ps100000 commented Nov 6, 2019

Should there be some indication in the UI that a DNS server is using DoT? Perhaps by having a small lock icon next to the display name in the DNS server list? As it is, once the DNS server is selected, there's no indication that Blokada is using DoT. I've just been visiting 1.1.1.1/help to confirm that DoT is being used. Thoughts?

You could change the server-like icon thats used for all DNS-related settings and change it to a lock. Maybe in a different/inverted color.

@tkterris
Copy link
Author

tkterris commented Nov 7, 2019

Should there be some indication in the UI that a DNS server is using DoT? Perhaps by having a small lock icon next to the display name in the DNS server list? As it is, once the DNS server is selected, there's no indication that Blokada is using DoT. I've just been visiting 1.1.1.1/help to confirm that DoT is being used. Thoughts?

You could change the server-like icon thats used for all DNS-related settings and change it to a lock. Maybe in a different/inverted color.

Done. I left the existing server icon for most cases, but when a DoT server is selected (or listed in the DNS selection menu), it reuses the key-inside-a-shield icon. I put in a TODO, in case a new icon should be created.

I noticed that the DoT server will only be used if adblocking and custom DNS are enabled, and the VPN is disabled. This is because the changes were only made to DnsProxy. When the Blokada VPN or DNS without adblocking are used, DnsProxy doesn't intercept the connections, so conventional DNS is used (see TunnelManagerFactory). Resolving these would be architectural decisions outside the scope of this PR (the design of the Google Play Store app for non-adblocking, and the design of the Blokada VPN for connecting to the VPN).

@tkterris
Copy link
Author

tkterris commented Dec 6, 2019

Is there anything else I should do for this PR?

@ps100000
Copy link

ps100000 commented Dec 8, 2019

Is there anything else I should do for this PR?
I should finish the changes @kar requested for API v5 this week.
It would be nice if you could change your code to use the new json-based API after it's merged so we don't have breaking changes twice. That's also the reason why this is not merged by now. :)

@tkterris
Copy link
Author

Is there anything else I should do for this PR?
I should finish the changes @kar requested for API v5 this week.
It would be nice if you could change your code to use the new json-based API after it's merged so we don't have breaking changes twice. That's also the reason why this is not merged by now. :)

Whoops, I had missed that. I've merged in that branch.

@szaimen
Copy link

szaimen commented Jan 20, 2020

Any progress here? :)

@afonari
Copy link

afonari commented Jan 21, 2020

https://github.com/IngoZenz/personaldnsfilter has it implemented.

@szaimen
Copy link

szaimen commented Jan 21, 2020

https://github.com/IngoZenz/personaldnsfilter has it implemented.

could that be used for blokada?

@tkterris
Copy link
Author

tkterris commented Jan 21, 2020

Any progress here? :)

This change includes the v5 API changes, in this PR: #575 . Once that's merged in, this will be ready to go.

@tkterris
Copy link
Author

Obsolete, DOH has been implemented in the latest version of Blokada.

@tkterris tkterris closed this Jun 30, 2021
@androidacy-user
Copy link

Not really. For instance my college's wifi blocks DoH but not DoT. They're not the same protocol at all

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

5 participants