Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Isn't
allowBypass
required only for apps that don't offer full routing (unlike Intra which does route all of the IPv4 space)?I read the source and I see either
RULE_PRIORITY_SECURE_VPN
orRULE_PRIORITY_BYPASSABLE_VPN
fwmark
s set onnetId
andsocket
s depending on if the VPN issecure
(which it is whenallowBypass
is not set or the VPN is in "lockdown mode" aka the "Block connections without VPN" mode settable by the user through Android's VPN settings), which (I am not sure) are used to make routing decisions later?Regardless, it remains unclear to me whether webrtc ICE sessions benefit from
allowBypass
? Is there a way to validate this?An unintended effect of setting
allowBypass
is the VPN is without Internet connectivity if the user also chooses to start it in "lockdown mode": Ref; and so,allowBypass
must be unset when the VPN is locked-down, like so celzero/rethink-app@71e1a840b, to unblock connectivity to the Internet.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.
In addition to this, it might be worth taking a look at
IntraVpnService#setUnderlyingNetworks
which only adds one network, the current active-network, but could also add all other networks the OS is connected to (with active-network in the first position in the array).Also:
allowBypass
, from the documentation, lets an app bind its sockets to any network, but if all networks are also bound to VPN, then they might not need to bypass the VPN in the first place?