Bypass LAN and reserved subnets in the VPN #311
Conversation
alalamav
commented
Oct 9, 2018
alalamav
commented
- Excludes special registry subnets from the VPN. This includes LAN and reserved subnets.
- Apple: NetworkExtension APIs support excluding subnets from the VPN.
- Android: VpnService APIs support only subnet inclusion, so we have computed the subnet compliment of the reserved subnets.
- Windows: OutlineService adds direct routes to the default gateway for the bypassed subnets.
| // retrieve the list of subnets that excludes those reserved for special use. | ||
| final ArrayList<Subnet> reservedBypassSubnets = getReservedBypassSubnets(); | ||
| for (Subnet subnet : reservedBypassSubnets) { | ||
| builder.addRoute(subnet.address, subnet.prefix); |
There was a problem hiding this comment.
Should this be builder = builder.addRoute(...)? (I expect it works either way.)
There was a problem hiding this comment.
It's not necessary to get the builder reference - works as is.
| /* Returns a subnet list that excludes reserved subnets. */ | ||
| private ArrayList<Subnet> getReservedBypassSubnets() { | ||
| final String[] subnetStrings = vpnService.getResources().getStringArray( | ||
| vpnService.getResourceId(PRIVATE_LAN_BYPASS_SUBNETS_ID, "array")); |
There was a problem hiding this comment.
I don't understand, why isn't this just R.array.reserved_bypass_subnets or whatever?
There was a problem hiding this comment.
Unfortunately, we cannot access the generated R class from Cordova plugins. Therefore we resort to retrieving resources via the Resources class.
| private ArrayList<Subnet> getReservedBypassSubnets() { | ||
| final String[] subnetStrings = vpnService.getResources().getStringArray( | ||
| vpnService.getResourceId(PRIVATE_LAN_BYPASS_SUBNETS_ID, "array")); | ||
| ArrayList<Subnet> subnets = new ArrayList<Subnet>(); |
There was a problem hiding this comment.
It seems like people generally prefer to omit the second type (new ArrayList<>()) and include an initial capacity (new ArrayList<>(subnetStrings.length)).
| @@ -157,4 +167,42 @@ public void disconnectTunnel() { | |||
| private String selectDnsResolverAddress() { | |||
| return DNS_RESOLVER_IP_ADDRESSES[new Random().nextInt(DNS_RESOLVER_IP_ADDRESSES.length)]; | |||
| } | |||
|
|
|||
| /* Returns a subnet list that excludes reserved subnets. */ | |||
There was a problem hiding this comment.
Mixing comment styles is odd.
There was a problem hiding this comment.
I have been using /**/ for method and class level comments, and // for comments in the code. I can change them all to long comments.
There was a problem hiding this comment.
In that case, I would recommend using /** */ in Javadoc style.
| @@ -530,7 +530,7 @@ private void startForegroundWithNotification( | |||
| builder = new Notification.Builder(this); | |||
| } | |||
| try { | |||
| builder.setSmallIcon(getResourceIdForDrawable("small_icon")); | |||
| builder.setSmallIcon(getResourceId("small_icon", "drawable")); | |||
There was a problem hiding this comment.
Why not just R.drawable.small_icon?
| public func IPv4String() -> String { | ||
| let ip = self | ||
| let byte1 = UInt8(ip & 0xff) | ||
| let byte2 = UInt8((ip>>8) & 0xff) |
| let byte2 = UInt8((ip>>8) & 0xff) | ||
| let byte3 = UInt8((ip>>16) & 0xff) | ||
| let byte4 = UInt8((ip>>24) & 0xff) | ||
| return "\(byte1).\(byte2).\(byte3).\(byte4)" |
There was a problem hiding this comment.
This order is backwards compared to the way we normally handle IPv4 addresses. Assuming it's correct, it deserves a comment explaining why it's backwards.
There was a problem hiding this comment.
Good catch! It was backwards, now fixed.
|
Thank you both for the review! I have addressed your comments, PTAL. |
