Moodle

Included PEAR Crypt_CHAP library uses obsolete mhash() code which breaks RADIUS auth support in some PHP 5.3 installs

Details

  • Database:
    Any
  • Testing Instructions:
    Hide

    Requirements

    1. a RADIUS server
    2. the PHP RADIUS extension installed and available on the web server of your Moodle testing site

    Testing steps

    1. log into your Moodle test site as a system administrator
    2. configure the RADIUS auth plugin in Moodle by going to the following path in the Settings block – Site administration -> Plugins -> Authentication -> Manage authentication
    3. enable and configure the RADIUS server plugin so that it communicates with your RADIUS server
    4. log out of the Moodle site
    5. attempt to log into the Moodle site using the credentials for a test user from the RADIUS server
    6. you should be able to successfully log in and have a new account created for you if you have never logged in with that user before
    Show
    Requirements a RADIUS server if you don't have one available you can setup one using FreeRADIUS – http://freeradius.org/ the PHP RADIUS extension installed and available on the web server of your Moodle testing site Testing steps log into your Moodle test site as a system administrator configure the RADIUS auth plugin in Moodle by going to the following path in the Settings block – Site administration -> Plugins -> Authentication -> Manage authentication enable and configure the RADIUS server plugin so that it communicates with your RADIUS server log out of the Moodle site attempt to log into the Moodle site using the credentials for a test user from the RADIUS server you should be able to successfully log in and have a new account created for you if you have never logged in with that user before
  • URL:
  • Affected Branches:
    MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
  • Fixed Branches:
    MOODLE_23_STABLE, MOODLE_24_STABLE
  • Pull 2.4 Branch:
    MDL-37625-m24
  • Pull 2.4 Diff URL:
  • Pull Master Branch:
    MDL-37625-master
  • Pull Master Diff URL:

Description

The PHP mhash module is obsoleted by the hash module (the latter of which is a required extension for current Moodle installs).

The Crypt_CHAP PEAR library that is currently included with Moodle uses mhash() calls which breaks on some PHP 5.3 installs due to the mhash extension not being available. This was a bug reported, fixed and released for this PEAR library in 2010.

Note: It looks like Red Hat Linux does not even have the mhash module available for install but Debian-based distros do include it (even with PHP 5.4). This is output from my Debian workstation:

  • $ php --version
PHP 5.4.4-12 (cli) (built: Jan 21 2013 10:32:30) 
Copyright (c) 1997-2012 The PHP Group
Zend Engine v2.4.0, Copyright (c) 1998-2012 Zend Technologies
    with Xdebug v2.2.1, Copyright (c) 2002-2012, by Derick Rethans

$ php -m | grep hash
hash
mhash

Issue Links

testing discovered

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-38373 Strict standards error with auth_radius

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
will help resolve

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-32000 RADIUS authentification

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Justin Filip added a comment -

My MacPorts-based PHP 5.4.11 install also has the mhash module available.

Show
Justin Filip added a comment - My MacPorts-based PHP 5.4.11 install also has the mhash module available.
Hide
Permalink
Justin Filip added a comment -

Adding patch information for a commit with the updated library version.

Show
Justin Filip added a comment - Adding patch information for a commit with the updated library version.
Hide
Permalink
Michael de Raadt added a comment -

Thanks for reporting and patching that, Justin. Feel free to request a peer review.

Petr: perhaps you could peer review this.

Show
Michael de Raadt added a comment - Thanks for reporting and patching that, Justin. Feel free to request a peer review. Petr: perhaps you could peer review this.
Hide
Permalink
Petr Škoda added a comment -

+1, there is not much we can review, the code is from upstream, I think testing is enough

Show
Petr Škoda added a comment - +1, there is not much we can review, the code is from upstream, I think testing is enough
Hide
Permalink
Justin Filip added a comment -

Testing is the tough one. It's what has prevented me from sending this for review so far. We have a client who is currently suffering from this problem. We can check that the problem goes away and their RADIUS auth starts working correctly for them. I checked the QA tests from 2.4 and there were no tests for this auth plug-in. Is it worth while to come up with some instructions for how to setup a test RADIUS server to validate that this plug-in works?

Show
Justin Filip added a comment - Testing is the tough one. It's what has prevented me from sending this for review so far. We have a client who is currently suffering from this problem. We can check that the problem goes away and their RADIUS auth starts working correctly for them. I checked the QA tests from 2.4 and there were no tests for this auth plug-in. Is it worth while to come up with some instructions for how to setup a test RADIUS server to validate that this plug-in works?
Hide
Permalink
Petr Škoda added a comment -

I am planning to set up some read only test VMs with all these external services necessary for testing of auth plugins. Hopefully it will be available for HQ CI servers before 2.5 release.

Show
Petr Škoda added a comment - I am planning to set up some read only test VMs with all these external services necessary for testing of auth plugins. Hopefully it will be available for HQ CI servers before 2.5 release.
Hide
Permalink
Justin Filip added a comment -

Sounds good to me. We'll get my branch tested on our clent's site that is broken right now to verify if it works and then I'll push this through the rest of the process.

Show
Justin Filip added a comment - Sounds good to me. We'll get my branch tested on our clent's site that is broken right now to verify if it works and then I'll push this through the rest of the process.
Hide
Permalink
Justin Filip added a comment -

I'm requesting a peer review on this one as we can now confirm that this fix has allowed the RADIUS auth plugin to work with our affected client installation.

Show
Justin Filip added a comment - I'm requesting a peer review on this one as we can now confirm that this fix has allowed the RADIUS auth plugin to work with our affected client installation.
Hide
Permalink
Dan Poltawski added a comment -

Hi Justin,

It looks good to me, can't really argue with the upstream changes.

We do need some testing instructions for this though, even if its difficult to setup the test, could you give us the requirements to setup a test of it?

Also, I assume you have tested it, so it at least gives us some confidence in applying this fix

[-] Syntax
[-] Output
[-] Whitespace
[-] Language
[-] Databases
[N] Testing
[-] Security
[-] Documentation
[Y] Git
[Y] Sanity check

Show
Dan Poltawski added a comment - Hi Justin, It looks good to me, can't really argue with the upstream changes. We do need some testing instructions for this though, even if its difficult to setup the test, could you give us the requirements to setup a test of it? Also, I assume you have tested it, so it at least gives us some confidence in applying this fix [-] Syntax [-] Output [-] Whitespace [-] Language [-] Databases [N] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check
Hide
Permalink
Justin Filip added a comment -

Thanks, Dan.

I put in rough instructions. In our case, the client we host who had the problem was connecting to their own RADIUS server that was external from our network. We do not maintain any setups internally so I can't really provide any further detailed instructions on how to do the RADIUS setup. =(

We did test this, though, and it resolved the issue and allowed users to successfully authenticated into the client's Moodle site again.

Show
Justin Filip added a comment - Thanks, Dan. I put in rough instructions. In our case, the client we host who had the problem was connecting to their own RADIUS server that was external from our network. We do not maintain any setups internally so I can't really provide any further detailed instructions on how to do the RADIUS setup. =( We did test this, though, and it resolved the issue and allowed users to successfully authenticated into the client's Moodle site again.
Hide
Permalink
Sam Hemelryk added a comment -

Thanks Justin, this has been integrated now on 23, 24, and master.

Show
Sam Hemelryk added a comment - Thanks Justin, this has been integrated now on 23, 24, and master.
Hide
Permalink
David Monllaó added a comment - - edited

It passes, tested in 22, 23, 24 and master with a RADIUS VM using Microsoft CHAP 1 authentication and freeradius.

I've received a strict standards error in 23 and 24, but IIRC we are not compliant with strict standards until 24. I wait for integrators feedback to pass it or fail it.

Strict Standards: Non-static method PEAR::loadExtension() should not be called statically, assuming $this from incompatible context in /var/www/integration/MOODLE_24_STABLE/lib/pear/Auth/RADIUS.php on line 49 Strict Standards: Non-static method PEAR::isError() should not be called statically, assuming $this from incompatible context in /var/www/integration/MOODLE_24_STABLE/auth/radius/auth.php on line 108

Edit: I've just noticed that it hasn't been integrated in 22; anyway I also have mhash, that's why it hasn't failed.

Show
David Monllaó added a comment - - edited It passes, tested in 22, 23, 24 and master with a RADIUS VM using Microsoft CHAP 1 authentication and freeradius. I've received a strict standards error in 23 and 24, but IIRC we are not compliant with strict standards until 24. I wait for integrators feedback to pass it or fail it. Strict Standards: Non- static method PEAR::loadExtension() should not be called statically, assuming $ this from incompatible context in / var /www/integration/MOODLE_24_STABLE/lib/pear/Auth/RADIUS.php on line 49 Strict Standards: Non- static method PEAR::isError() should not be called statically, assuming $ this from incompatible context in / var /www/integration/MOODLE_24_STABLE/auth/radius/auth.php on line 108 Edit: I've just noticed that it hasn't been integrated in 22; anyway I also have mhash, that's why it hasn't failed.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

it seems that the class has not been changed as trunk continue showing that static behavior:

http://svn.php.net/viewvc/pear/packages/Auth_RADIUS/trunk/RADIUS.php?revision=257341&view=markup

So, or we fix that in the PEAR wrapper or we consider switching to another (pure or wrapper) lib. If this had to be voted, I'd say "pure" is better (less dependencies). Like, for example:

http://www.phpclasses.org/package/4326-PHP-Authenticate-users-with-a-RADIUS-server.html

Although I don't know much really. Adding Jonathan Harker here coz he did the initial (and current) implementation.

Ciao

Show
Eloy Lafuente (stronk7) added a comment - it seems that the class has not been changed as trunk continue showing that static behavior: http://svn.php.net/viewvc/pear/packages/Auth_RADIUS/trunk/RADIUS.php?revision=257341&view=markup So, or we fix that in the PEAR wrapper or we consider switching to another (pure or wrapper) lib. If this had to be voted, I'd say "pure" is better (less dependencies). Like, for example: http://www.phpclasses.org/package/4326-PHP-Authenticate-users-with-a-RADIUS-server.html Although I don't know much really. Adding Jonathan Harker here coz he did the initial (and current) implementation. Ciao
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

I've created MDL-38373 and provided a trivial solution there for the stricts standard thingy. So I think this can be passed safely.

Ciao

Show
Eloy Lafuente (stronk7) added a comment - I've created MDL-38373 and provided a trivial solution there for the stricts standard thingy. So I think this can be passed safely. Ciao
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

Thanks, closing as fixed!

Show
Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!
Hide
Permalink
David Monllaó added a comment -

For the record, there is a VM in the HQ servers to use in further RADIUS-related tests

Show
David Monllaó added a comment - For the record, there is a VM in the HQ servers to use in further RADIUS-related tests

People

Vote (5)
Watch (9)

Dates

  • Created:
    Updated:
    Resolved: