Improve CIDR validation logic for IPv4 addresses

[ryannewington]

Valid CIDRs that start with 0.0.0.0 are being filtered out by the library.

Line 135 of the IPv4 validation rejects 0.0.0.0 (with a note saying that it is only valid in CIDR), but the CIDR function is just trying to validate the CIDR as an IP.

DPC/DPCLibrary/Utils/Validate.cs

Line 135 in c7bcd9f

return false; //reject all 0s as its only valid as a CIDR

This PR enhances the validation logic to allow CIDRs that start with 0.0.0.0/x

[ld0614]

HI Ryan, firstly, thanks so much for taking the time to create the first community pull request on the project! It really is appreciated and nice to see others actively engaging with DPC

Thanks for spotting this bug although I'm struggling to see why you'd ever need to use 0.0.0.0/0 in a real world context as that would normally just require the use of the force tunnel attribute instead?

I'm also not familiar with the 0.0.0.0/1 notation, I've only ever seen it displayed as 0.0.0.0/0. Is there something I'm missing here?

[ld0614]

Also, I notice that you're attempting to pull into the main branch, could I ask that you instead pull into the development branch please as this is where the next release is being developed?

[ryannewington]

Hey @ld0614,

First, thanks for the work on the excellent tool. It's what MS should have shipped themselves.

And apologies for not being clear in the PR. Let me try explain a little more.

0.0.0.0/[1-32] is a valid CIDR range. Eg 0.0.0.0/24 represents IP addresses from 0.0.0.1-0.0.0.255.

The current logic prevents valid CIDR ranges from being used, so this PR was to address that.

Note this is different from 0.0.0.0/0 which as we know as shorthand for a default gateway/route.

I must admit I'm looking to use this as a workaround. I'm trying to get a device full tunnel working. (No need for a user tunnel). We're able to do this in the profile XML manually, but looking to move to DPC, where it forces split tunnel. It won't accept a route of 0.0.0.0/0, so thought we'd split it into two routes 0.0.0.0/1 and 128.0.0.0/1 which represent all IP addresses. The 0.0.0.0/1 route is currently rejected by DPC.

[ryannewington]

Oh yep will resubmit to that branch. Sorry.

[ld0614]

ok so I think there are really 2 things that need improving in that case:

1. 0.0.0.0/0 should be accepted as a valid CIDR
2. 0.0.0.0/24 (as an example) should be accepted as this can cover valid CIDR ranges

Looking at the code, I think the only thing we need to do to fix both is to ament the CIDRVal 0 check to only return false if 0.0.0.0 isn't the first part and then add some more Test examples in for 0.0.0.0 as well. Are you happy to do that or shall I merge and make the rest of the changes?

Are you on the discord so that I can shout you out when the release goes live?

[ryannewington]

Updated as requested.

Is there any reason for using the long Regex's instead of IPAddress.TryParse? I think there's an opportunity to streamline a lot of the logic if we use native IPAddress objects, but I didn't want to make too many changes to your code. Happy to submit a separate draft PR with these ideas if you are interested. I notice the same problem exists with the IPv6 CIDR, but that's a little tricker to fix in the current model.

[ld0614]

Thanks for the updates, I'm always up for improving the code and very much welcome additional contributors to the project. Good spot on the duplicated test data, thats been there since I originally wrote those tests back in 2020...

Looking at the original code, IPv6 does use IPAddress as manually validating all the ways IPv6 form is crazy complicated. IPv6 references did come later so I suspect that the IPv4 method could be simplified, it just wasn't that complicated and worked so I probably didn't see a need to move to the IP Address approach at the time. Less regex is always good IMO so providing the tests all keep passing and the overall methods are maintained feel free to draft a PR 😄

[ld0614]

Running a full Battery of tests has exposed an assumption in

DPC/DPCLibrary/Models/Route.cs

Line 76 in 6ca9cc0

tempAddress = tempAddressSplit[0];

and

DPC/DPCLibrary/Models/IPv4Address.cs

Line 36 in 6ca9cc0

public void LoadFromString(string address)

We'll also probably have to fix up the test

DPC/ServiceIntergrationTests/UserProfileTests.cs

Line 1941 in 6ca9cc0

public void BasicUserProfileWithSillyRoutes(ProfileType profileType)

Happy to help with this as it gets into some of the guts of the profile comparison code, my initial feeling is that a Accept0s Bool on the validators might be the most elegant solution but it is 11pm so needs some more thought when I'm not as tired...

[ryannewington]

Ah yep. Let me take a look this morning.
Perhaps a IsIPv4Network vs IsIPv4Address. There are different rules for each.

[ld0614]

Closing as this has been superseded by #44