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

Automation network range does not always produce the correct start/end values #3073

Closed
robwdwd opened this issue Nov 7, 2019 · 9 comments
Closed
Assignees
Labels
bug Undesired behaviour resolved A fixed issue
Milestone

Comments

@robwdwd
Copy link
Contributor

robwdwd commented Nov 7, 2019

Describe the bug
With changes made for issue #2862 the code now treats network ranges differently. Previously if you had 10.1.0.1-10.1.0.6 autom8 would scan 10.1.0.1 and 10.1.0.6 and all IP in between. Now it treats them as broadcast and network IPs so only 10.1.0.2-10.1.0.5 get scanned. This case is only relevant if the network range is not on a sub net boundary. Was this intentional? do network ranges need to be on sub net boundaries?

To Reproduce
Steps to reproduce the behavior:

  1. This is reproducible in 1.2.7 release and probably in previous releases since Automation does not calculate network information correctly for single hosts #2862 was fixed.

Expected behavior
Network ranges should include the start and end IP addresses and not treat them as network and broadcast addresses.

@netniV
Copy link
Member

netniV commented Nov 8, 2019

Interestingly enough, that made it through a lot of testing if it's true. So I threw another test file together and verified that that is indeed a bug:

test                                 type            start              end          network        broadcast  cidr count
10.1.1.0/24                        subnet         10.1.1.1       10.1.1.254         10.1.1.0       10.1.1.255    24   254
10.1.83.0/22                       subnet        10.1.80.1      10.1.83.254        10.1.80.0      10.1.83.255    22  1022
10.1.1.0/255.255.255.0             subnet         10.1.1.1       10.1.1.254         10.1.1.0       10.1.1.255    24   254
10.1.1.33                          single        10.1.1.33        10.1.1.33        10.1.1.33        10.1.1.33    32     1
10.1.1.33/32                       single        10.1.1.33        10.1.1.33        10.1.1.33        10.1.1.33    32     1
10.1.1.*                           subnet         10.1.1.1       10.1.1.254         10.1.1.0       10.1.1.255    24   254
10.2.*                             subnet         10.2.0.1     10.2.255.254         10.2.0.0     10.2.255.255    16 65534
10.1.0.1-10.1.0.6                   range         10.1.0.2         10.1.0.5         10.1.0.1         10.1.0.6           4

@netniV netniV self-assigned this Nov 8, 2019
@netniV netniV added the bug Undesired behaviour label Nov 8, 2019
@netniV netniV added this to the v1.2.8 milestone Nov 8, 2019
@robwdwd
Copy link
Contributor Author

robwdwd commented Nov 8, 2019

Great thanks for verifying this. I think this might have been overlooked as network ranges in the form - are not documented (at least in the inline help) so perhaps they are not used by many people.

I did fix this to get things working for me in the latest release by doing this

diff --git a/lib/api_automation.php b/lib/api_automation.php
index 7a8c3a4..24c8f49 100644
--- a/lib/api_automation.php
+++ b/lib/api_automation.php
@@ -3078,8 +3078,8 @@ function automation_get_network_info($range) {
        } elseif (strpos($range, '-') !== false) {
                $range_parts = explode('-', $range);

-               $network   = automation_get_valid_ip($range_parts[0]);
-               $broadcast = automation_get_valid_ip($range_parts[1]);
+               $network   = automation_get_valid_ip(long2ip(ip2long($range_parts[0]) - 1));
+               $broadcast = automation_get_valid_ip(long2ip(ip2long($range_parts[1]) + 1));
        } else {
                $network   = automation_get_valid_ip($range);
                $broadcast = automation_get_valid_ip($range);

@netniV
Copy link
Member

netniV commented Nov 8, 2019

That would work but I think if someone enters 10.1.1.0-10.1.1.256, that would should fail and with your method, I think we'll see something different. Let me test that.

@netniV
Copy link
Member

netniV commented Nov 8, 2019

So this was quick to test and gave the following which I'm happy with:

test                                 type            start              end          network        broadcast  cidr count
10.1.1.0/24                        subnet         10.1.1.1       10.1.1.254         10.1.1.0       10.1.1.255    24   254
10.1.83.0/22                       subnet        10.1.80.1      10.1.83.254        10.1.80.0      10.1.83.255    22  1022
10.1.1.0/255.255.255.0             subnet         10.1.1.1       10.1.1.254         10.1.1.0       10.1.1.255    24   254
10.1.1.33                          single        10.1.1.33        10.1.1.33        10.1.1.33        10.1.1.33    32     1
10.1.1.33/32                       single        10.1.1.33        10.1.1.33        10.1.1.33        10.1.1.33    32     1
10.1.1.*                           subnet         10.1.1.1       10.1.1.254         10.1.1.0       10.1.1.255    24   254
10.2.*                             subnet         10.2.0.1     10.2.255.254         10.2.0.0     10.2.255.255    16 65534
10.1.0.1-10.1.0.6                   range         10.1.0.1         10.1.0.6         10.1.0.0         10.1.0.7           6
10.1.0.0-10.1.0.255                 range         10.1.0.0       10.1.0.255     10.0.255.255         10.1.1.0         256
10.1.0.0-10.1.0.256                 <BAD>                -                -                -                -     -     -

If you feel up to it, create a pull request with an updated CHANGELOG and then we can merge it in 👍

@netniV
Copy link
Member

netniV commented Nov 8, 2019

This even works with wider ranges

test                                 type            start              end          network        broadcast  cidr count
10.1.0.0-10.1.1.0                   range         10.1.0.0         10.1.1.0     10.0.255.255         10.1.1.1         257
10.1.0.0-10.1.1.2                   range         10.1.0.0         10.1.1.2     10.0.255.255         10.1.1.3         259

Whilst the broadcast/network addresses may look wrong at first glance they are in fact correct.

@robwdwd
Copy link
Contributor Author

robwdwd commented Nov 8, 2019

Yup I think ranges are a bit unique in the fact that they may or may not be a whole subnet but part of one. Maybe the documentation needs to reflect that about ranges so if someone has 10.1.1.0/24 and they want to reflect that in the range (although it would be easier just to put in the /24 in cidr format) it should be 10.1.1.1-10.1.1.254. On the other hand for example if the cidr was 10.1.0.0/22 adding 10.1.1.0-10.1.1.255 is acceptable because from /22 cidr 10.1.1.0 and 10.1.1.255 are valid host addresses.

Maybe the documentation should show ranges are scanned from firstip to lastip in the format -.

Any particular branch I should be creating the pull request from?

@netniV
Copy link
Member

netniV commented Nov 8, 2019

1.2.x since it is a bug 👍

@robwdwd
Copy link
Contributor Author

robwdwd commented Nov 8, 2019

Done the pull request ;)

@cigamit
Copy link
Member

cigamit commented Nov 9, 2019

Closing as the pull request has been merged.

@cigamit cigamit closed this as completed Nov 9, 2019
@netniV netniV changed the title autom8 network range treats start and end as network and broadcast Automation network range does not always produce the correct start/end values Dec 7, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Undesired behaviour resolved A fixed issue
Projects
None yet
Development

No branches or pull requests

3 participants