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

Add ability to add DNS resolvers to a site #104

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

0ff
Copy link

@0ff 0ff commented Dec 28, 2022

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.

Brian Picciano and others added 3 commits December 28, 2022 07:46
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
@CLAassistant
Copy link

CLAassistant commented Dec 28, 2022

CLA assistant check
All committers have signed the CLA.

Copy link

@numinit numinit left a 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) }
Copy link

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.

Copy link

@numinit numinit Dec 31, 2022

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.

@gpaterno
Copy link

Any way to put this forward? Or, at least, some solution addressing this issue?THanks

@0ff
Copy link
Author

0ff commented Jan 29, 2023

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.

@numinit
Copy link

numinit commented Feb 6, 2023

I've confirmed it works fine on my Android device.

@numinit
Copy link

numinit commented Feb 6, 2023

That being said, would you mind implementing this comment? It definitely causes a DNS leak otherwise.

https://github.com/DefinedNet/mobile_nebula/pull/104/files/97dca45ac2fe0568ddb541cbc3d1808f3ba563d3#r1059683727

@johnmaguire johnmaguire mentioned this pull request May 22, 2023
@LennyPenny
Copy link

Maybe you could apply for this job and get the PR merged that way ;) https://www.defined.net/jobs/#senior-software-engineer---mobile

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