Moodle
  1. Moodle
  2. MDL-19509

LDAP NTLM SSO subnet matching doesn't work with 0.0.0.0/0

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9, 1.9.1, 1.9.2, 1.9.3, 1.9.4, 1.9.5
    • Fix Version/s: 1.8.10, 1.9.6
    • Component/s: Authentication
    • Labels:
      None
    • Environment:
      Moodle 1.9 with LDAP + NTLM SSO
    • Database:
      Any
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      31649

      Description

      If one requires all clients to auth via SSO, there currently is no mechanism to allow this, short of specifying the whole world in the subnet list, i.e. 1.0.0.0/0 - 254.254.254.254/0

      One would imagine that specifying 0.0.0.0/0 would achieve this but it does not.
      I've had a look at this but binary arithmetic makes my face hurt.

      Could the 'address_in_subnet'/type 1 test be modified to account for a subnet/mask of 0.0.0.0/0?
      Or would it be more sensible to have a config option to allow SSO for all clients without needing to specify an unintuitive catch-all mask of 0.0.0.0/0

      Cheers

        Activity

        Hide
        Iñaki Arenaza added a comment -

        Hi Petr,

        The modification I propose in http://moodle.org/mod/forum/discuss.php?d=80104#p551492 fixes the issue for the 0.0.0.0/0 subnet.

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Hi Petr, The modification I propose in http://moodle.org/mod/forum/discuss.php?d=80104#p551492 fixes the issue for the 0.0.0.0/0 subnet. Saludos. Iñaki.
        Hide
        Petr Škoda added a comment -

        thanks

        Show
        Petr Škoda added a comment - thanks
        Hide
        Iñaki Arenaza added a comment -

        Fixed in CVS for both 1.8.x and 1.9.x (HEAD is not affected with this issue).

        Alastair, thanks a lot Alastair for the bug report!

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Fixed in CVS for both 1.8.x and 1.9.x (HEAD is not affected with this issue). Alastair, thanks a lot Alastair for the bug report! Saludos. Iñaki.
        Hide
        Tim Hunt added a comment -

        address_in_subnet has extensive unit tests. Please run them and make sure they all pass before changing this function.

        (They don't but I think the bug is in the test not the real code. Still it needs to be thought about and fixed.)

        Thanks.

        Show
        Tim Hunt added a comment - address_in_subnet has extensive unit tests. Please run them and make sure they all pass before changing this function. (They don't but I think the bug is in the test not the real code. Still it needs to be thought about and fixed.) Thanks.
        Hide
        Iñaki Arenaza added a comment -

        The bug is clearly in the unit tests . If you have a look at RFC 4632, section 3.1.(Basic Concept and Prefix Notation), the CIDR syntax specifies that there has to be a decimal value between 0 and 32 (both included) after the '/' separator, and that '/0' is a netmask that means no significant bits at all (i.e., it matches any subnet or address). So at lest the following unit tests are wrong:

        $this->assertTrue(address_in_subnet('123.121.234.0', '123.121.234.0/')); // / is like /32.
        $this->assertFalse(address_in_subnet('123.121.234.1', '123.121.234.0/'));

        as there should be a decimal value after the subnet mask separator. Although I'd prefer to signal a syntax error (as in the case of the last unit test with an invalid netmask of '/148'), we should at least return False.

        $this->assertFalse(address_in_subnet('232.232.232.232', '123.121.234.0/0'));

        As this check should return True ('/0' is the wildcard mask for 'any address', as mentionned above).

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - The bug is clearly in the unit tests . If you have a look at RFC 4632, section 3.1.(Basic Concept and Prefix Notation), the CIDR syntax specifies that there has to be a decimal value between 0 and 32 (both included) after the '/' separator, and that '/0' is a netmask that means no significant bits at all (i.e., it matches any subnet or address). So at lest the following unit tests are wrong: $this->assertTrue(address_in_subnet('123.121.234.0', '123.121.234.0/')); // / is like /32. $this->assertFalse(address_in_subnet('123.121.234.1', '123.121.234.0/')); as there should be a decimal value after the subnet mask separator. Although I'd prefer to signal a syntax error (as in the case of the last unit test with an invalid netmask of '/148'), we should at least return False. $this->assertFalse(address_in_subnet('232.232.232.232', '123.121.234.0/0')); As this check should return True ('/0' is the wildcard mask for 'any address', as mentionned above). Saludos. Iñaki.
        Hide
        Iñaki Arenaza added a comment -

        By the way, if we agree that this test is wrong (and I do):

        $this->assertTrue(address_in_subnet('123.121.234.0', '123.121.234.0/')); // / is like /32.

        we should not only fix the unit test, but also the code in address_in_subnet().

        In the second test:

        $this->assertFalse(address_in_subnet('123.121.234.1', '123.121.234.0/'));

        if we are just going to return false for invalid syntax subnet masks (like we currently do for netmasks outside of the 0-32 range), then the unit test is right and shouldn't be fixed.

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - By the way, if we agree that this test is wrong (and I do): $this->assertTrue(address_in_subnet('123.121.234.0', '123.121.234.0/')); // / is like /32. we should not only fix the unit test, but also the code in address_in_subnet(). In the second test: $this->assertFalse(address_in_subnet('123.121.234.1', '123.121.234.0/')); if we are just going to return false for invalid syntax subnet masks (like we currently do for netmasks outside of the 0-32 range), then the unit test is right and shouldn't be fixed. Saludos. Iñaki.
        Hide
        Tim Hunt added a comment -

        The one place where this is explained in documentation that I know is http://cvs.moodle.org/moodle/lang/en_utf8/help/quiz/requiresubnet.html?revision=1.3&view=markup, so we should not do anything that breaks backwards compatibility with that.

        Fortunately, we aren't going to do that.

        I guess false on invalid input, is a sensible policy. Please can you update the tests and the code.

        Show
        Tim Hunt added a comment - The one place where this is explained in documentation that I know is http://cvs.moodle.org/moodle/lang/en_utf8/help/quiz/requiresubnet.html?revision=1.3&view=markup , so we should not do anything that breaks backwards compatibility with that. Fortunately, we aren't going to do that. I guess false on invalid input, is a sensible policy. Please can you update the tests and the code.
        Hide
        Iñaki Arenaza added a comment -

        Code and tests updated in 1.8.x and 1.9.x, using the same code and functions that are in HEAD.

        Closing now so Tim can do the QA

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Code and tests updated in 1.8.x and 1.9.x, using the same code and functions that are in HEAD. Closing now so Tim can do the QA Saludos. Iñaki.
        Hide
        Tim Hunt added a comment -

        Sorry, I am still not happy. You seem to have invented a function is_number that does exactly the same things is the PHP built-in function is_numeric.

        (Also, when you fix a bug, you should resolve it, not close it. The person who does QA closes it.)

        Show
        Tim Hunt added a comment - Sorry, I am still not happy. You seem to have invented a function is_number that does exactly the same things is the PHP built-in function is_numeric. (Also, when you fix a bug, you should resolve it, not close it. The person who does QA closes it.)
        Hide
        Iñaki Arenaza added a comment -

        Hi Tim,

        regarding the resolve vs close, you are right (I cliked on the wrong link, my fault).

        As to the is_numeric PHP built-in function, we can't/shouldn't use it, because it also accepts floats as valid values, which is wrong in this case (we should only accept integer values). On the other hand I haven't invented the is_number() function, I've copied it verbatim from HEAD and copied the address_in_subnet check for IPv4 addresses almost verbatim from HEAD.

        So if Petr (as the author of the code in HEAD according to cvs annotate) and you could make up your mind on how to do it, I'd gladly fix the code in The Right Way(tm)

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Hi Tim, regarding the resolve vs close, you are right (I cliked on the wrong link, my fault). As to the is_numeric PHP built-in function, we can't/shouldn't use it, because it also accepts floats as valid values, which is wrong in this case (we should only accept integer values). On the other hand I haven't invented the is_number() function, I've copied it verbatim from HEAD and copied the address_in_subnet check for IPv4 addresses almost verbatim from HEAD. So if Petr (as the author of the code in HEAD according to cvs annotate) and you could make up your mind on how to do it, I'd gladly fix the code in The Right Way(tm) Saludos. Iñaki.
        Hide
        Alastair Hole added a comment -

        Is 'is_int' any use?
        http://uk.php.net/is_int

        Alastair

        Show
        Alastair Hole added a comment - Is 'is_int' any use? http://uk.php.net/is_int Alastair
        Hide
        Iñaki Arenaza added a comment -

        Hi Alastair,

        I'm afraid it isn't, as the mask is actually a string (even if it represents an integer value). So is_int() would always return false.

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Hi Alastair, I'm afraid it isn't, as the mask is actually a string (even if it represents an integer value). So is_int() would always return false. Saludos. Iñaki.
        Hide
        Tim Hunt added a comment -

        Doh! sorry for messing you around Iñaki. You are right and I should have been playing more attention.

        Show
        Tim Hunt added a comment - Doh! sorry for messing you around Iñaki. You are right and I should have been playing more attention.
        Hide
        Iñaki Arenaza added a comment -

        Don't worry Tim. Better safe than sorry

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Don't worry Tim. Better safe than sorry Saludos. Iñaki.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: