-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add ability to add DNS resolvers to a site #104
base: main
Are you sure you want to change the base?
Conversation
For most changes I simply copied whatever was being done for `unsafeRoutes` and fitted it to work for dns resolvers. TODO This does not currently work for iOS, only android, as I only have an android device to test with. # Conflicts: # lib/models/Site.dart # lib/screens/siteConfig/AdvancedScreen.dart # nebula/mobileNebula.go
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.
Very nice!
|
||
val cm = getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager | ||
cm.allNetworks.forEach { network -> | ||
cm.getLinkProperties(network).dnsServers.forEach { builder.addDnsServer(it) } |
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.
cm.getLinkProperties(network)?.dnsServers?.forEach { builder.addDnsServer(it) }
Would not compile otherwise.
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.
Come to think of it, lines 129 to 132 should be removed entirely, as they cause a DNS leak on Android when tested with https://mullvad.net/ (but no others, interestingly). Removing these lines fixes the DNS leak. They are also based on a false assumption that the device will not roam between networks.
Any way to put this forward? Or, at least, some solution addressing this issue?THanks |
I don't have an android device to check this on and there seems to be no interest at all from the maintainer to get this merged, so I fear you'd have to take it into your own hands. I'd love to see this going upstream, as for now I have to maintain my own copy of nebula to use the feature, so if I can do anything to get it merged, let me know. |
I've confirmed it works fine on my Android device. |
That being said, would you mind implementing this comment? It definitely causes a DNS leak otherwise. |
Maybe you could apply for this job and get the PR merged that way ;) https://www.defined.net/jobs/#senior-software-engineer---mobile |
This PR builds upon #18 and updates it to the latest mobile_nebula version.
It also adds iOS Support, so that this can finally be used on both platforms.
As such, it also solves #9.