diff --git a/application/loginexternal.class.inc.php b/application/loginexternal.class.inc.php index d07869595..23e0c7850 100644 --- a/application/loginexternal.class.inc.php +++ b/application/loginexternal.class.inc.php @@ -75,13 +75,10 @@ class LoginExternal extends AbstractLoginFSMExtension } /** - * @return bool + * @return bool|mixed */ private function GetAuthUser() { - $sExtAuthVar = MetaModel::GetConfig()->GetExternalAuthenticationVariable(); // In which variable is the info passed ? - eval('$sAuthUser = isset('.$sExtAuthVar.') ? '.$sExtAuthVar.' : false;'); // Retrieve the value - /** @var string $sAuthUser */ - return $sAuthUser; // Retrieve the value + return MetaModel::GetConfig()->GetExternalAuthenticationVariable(); } } diff --git a/core/config.class.inc.php b/core/config.class.inc.php index 06a83b14d..645fe89f9 100644 --- a/core/config.class.inc.php +++ b/core/config.class.inc.php @@ -75,6 +75,7 @@ define('DEFAULT_EXT_AUTH_VARIABLE', '$_SERVER[\'REMOTE_USER\']'); define('DEFAULT_ENCRYPTION_KEY', '@iT0pEncr1pti0n!'); // We'll use a random generated key later (if possible) define('DEFAULT_ENCRYPTION_LIB', 'Mcrypt'); // We'll define the best encryption available later define('DEFAULT_HASH_ALGO', PASSWORD_DEFAULT); + /** * Config * configuration data (this class cannot not be localized, because it is responsible for loading the dictionaries) @@ -869,6 +870,14 @@ class Config 'source_of_value' => '', 'show_in_conf_sample' => false, ], + 'ext_auth_variable' => [ + 'type' => 'string', + 'description' => 'External authentication expression (allowed: $_SERVER[\'key\'], $_COOKIE[\'key\'], $_REQUEST[\'key\'], getallheaders()[\'Header-Name\'])', + 'default' => '$_SERVER[\'REMOTE_USER\']', + 'value' => '$_SERVER[\'REMOTE_USER\']', + 'source_of_value' => '', + 'show_in_conf_sample' => false, + ], 'login_debug' => [ 'type' => 'bool', 'description' => 'Activate the login FSM debug', @@ -1966,11 +1975,6 @@ class Config */ protected $m_sAllowedLoginTypes; - /** - * @var string Name of the PHP variable in which external authentication information is passed by the web server - */ - protected $m_sExtAuthVariable; - /** * @var string Encryption key used for all attributes of type "encrypted string". Can be set to a random value * unless you want to import a database from another iTop instance, in which case you must use @@ -2043,7 +2047,6 @@ class Config $this->m_bSecureConnectionRequired = DEFAULT_SECURE_CONNECTION_REQUIRED; $this->m_sDefaultLanguage = 'EN US'; $this->m_sAllowedLoginTypes = DEFAULT_ALLOWED_LOGIN_TYPES; - $this->m_sExtAuthVariable = DEFAULT_EXT_AUTH_VARIABLE; $this->m_aCharsets = []; $this->m_bQueryCacheEnabled = DEFAULT_QUERY_CACHE_ENABLED; $this->m_iPasswordHashAlgo = DEFAULT_HASH_ALGO; @@ -2197,7 +2200,6 @@ class Config $this->m_sDefaultLanguage = isset($MySettings['default_language']) ? trim($MySettings['default_language']) : 'EN US'; $this->m_sAllowedLoginTypes = isset($MySettings['allowed_login_types']) ? trim($MySettings['allowed_login_types']) : DEFAULT_ALLOWED_LOGIN_TYPES; - $this->m_sExtAuthVariable = isset($MySettings['ext_auth_variable']) ? trim($MySettings['ext_auth_variable']) : DEFAULT_EXT_AUTH_VARIABLE; $this->m_sEncryptionKey = isset($MySettings['encryption_key']) ? trim($MySettings['encryption_key']) : $this->m_sEncryptionKey; $this->m_sEncryptionLibrary = isset($MySettings['encryption_library']) ? trim($MySettings['encryption_library']) : $this->m_sEncryptionLibrary; $this->m_aCharsets = isset($MySettings['csv_import_charsets']) ? $MySettings['csv_import_charsets'] : []; @@ -2350,9 +2352,73 @@ class Config return explode('|', $this->m_sAllowedLoginTypes); } + /** + * @return bool|mixed + * @since 3.2.3 return the parsed value instead of an unsecured variable name + */ public function GetExternalAuthenticationVariable() { - return $this->m_sExtAuthVariable; + $sExpression = $this->Get('ext_auth_variable'); + $aParsed = $this->ParseExternalAuthVariableExpression($sExpression); + if ($aParsed === null) { + return false; + } + + $sKey = $aParsed['key']; + switch ($aParsed['type']) { + case 'server': + return $_SERVER[$sKey] ?? false; + case 'cookie': + return $_COOKIE[$sKey] ?? false; + case 'request': + return $_REQUEST[$sKey] ?? false; + case 'header': + if (!function_exists('getallheaders')) { + return false; + } + $aHeaders = getallheaders(); + if (!is_array($aHeaders)) { + return false; + } + return $aHeaders[$sKey] ?? false; + } + return false; + } + + /** + * @param $sExpression + * @return array|null + */ + private function ParseExternalAuthVariableExpression($sExpression) + { + // If it's a configuration parameter it's probably already trimmed, but just in case + $sExpression = trim((string) $sExpression); + if ($sExpression === '') { + return null; + } + + // Match $_SERVER/$_COOKIE/$_REQUEST['key'] with optional whitespace and single/double quotes. + if (preg_match('/^\$_(SERVER|COOKIE|REQUEST)\s*\[\s*(["\'])\s*([^"\']+)\2\s*\]\s*$/', $sExpression, $aMatches) === 1) { + $sContext = strtoupper($aMatches[1]); + $sKey = $aMatches[3]; + return [ + 'type' => strtolower($sContext), + 'key' => $sKey, + 'normalized' => '$_'.$sContext.'[\''.$sKey.'\']', + ]; + } + + // Match getallheaders()['Header-Name'] in a case-insensitive way. + if (preg_match('/^getallheaders\(\)\s*\[\s*(["\'])\s*([^"\']+)\1\s*\]\s*$/i', $sExpression, $aMatches) === 1) { + $sKey = $aMatches[2]; + return [ + 'type' => 'header', + 'key' => $sKey, + 'normalized' => 'getallheaders()[\''.$sKey.'\']', + ]; + } + + return null; } public function GetCSVImportCharsets() @@ -2448,7 +2514,7 @@ class Config public function SetExternalAuthenticationVariable($sExtAuthVariable) { - $this->m_sExtAuthVariable = $sExtAuthVariable; + $this->Set('ext_auth_variable', $sExtAuthVariable); } public function SetEncryptionKey($sKey) @@ -2503,7 +2569,6 @@ class Config $aSettings['secure_connection_required'] = $this->m_bSecureConnectionRequired; $aSettings['default_language'] = $this->m_sDefaultLanguage; $aSettings['allowed_login_types'] = $this->m_sAllowedLoginTypes; - $aSettings['ext_auth_variable'] = $this->m_sExtAuthVariable; $aSettings['encryption_key'] = $this->m_sEncryptionKey; $aSettings['encryption_library'] = $this->m_sEncryptionLibrary; $aSettings['csv_import_charsets'] = $this->m_aCharsets; @@ -2608,7 +2673,6 @@ class Config $aOtherValues = [ 'default_language' => $this->m_sDefaultLanguage, 'allowed_login_types' => $this->m_sAllowedLoginTypes, - 'ext_auth_variable' => $this->m_sExtAuthVariable, 'encryption_key' => $this->m_sEncryptionKey, 'encryption_library' => $this->m_sEncryptionLibrary, 'csv_import_charsets' => $this->m_aCharsets, diff --git a/tests/php-unit-tests/unitary-tests/application/LoginExternalTest.php b/tests/php-unit-tests/unitary-tests/application/LoginExternalTest.php new file mode 100644 index 000000000..93f16d2b3 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/application/LoginExternalTest.php @@ -0,0 +1,95 @@ + + */ + +namespace Combodo\iTop\Test\UnitTest\Application; + +use Combodo\iTop\Test\UnitTest\ItopDataTestCase; +use utils; + +class LoginExternalTest extends ItopDataTestCase +{ + private $oConfig; + private $sOriginalExtAuthVariable; + + protected function setUp(): void + { + parent::setUp(); + require_once APPROOT.'application/loginexternal.class.inc.php'; + $this->oConfig = utils::GetConfig(); + $this->sOriginalExtAuthVariable = $this->oConfig->Get('ext_auth_variable'); + } + + protected function tearDown(): void + { + $this->oConfig->SetExternalAuthenticationVariable($this->sOriginalExtAuthVariable); + parent::tearDown(); + } + + private function CallGetAuthUser() + { + $oLoginExternal = new \LoginExternal(); + $oMethod = new \ReflectionMethod(\LoginExternal::class, 'GetAuthUser'); + $oMethod->setAccessible(true); + return $oMethod->invoke($oLoginExternal); + } + + public function testGetAuthUserFromServerVariable() + { + $_SERVER['REMOTE_USER'] = 'alice'; + $this->oConfig->SetExternalAuthenticationVariable('$_SERVER[\'REMOTE_USER\']'); + + $this->assertSame('alice', $this->CallGetAuthUser()); + } + + public function testGetAuthUserFromCookie() + { + $_COOKIE['auth_user'] = 'bob'; + $this->oConfig->SetExternalAuthenticationVariable('$_COOKIE[\'auth_user\']'); + + $this->assertSame('bob', $this->CallGetAuthUser()); + } + + public function testGetAuthUserFromRequest() + { + $_REQUEST['auth_user'] = 'carol'; + $this->oConfig->SetExternalAuthenticationVariable('$_REQUEST[\'auth_user\']'); + + $this->assertSame('carol', $this->CallGetAuthUser()); + } + + public function testInvalidExpressionReturnsFalse() + { + $this->oConfig->SetExternalAuthenticationVariable('$_SERVER[\'HTTP_X_CMD\']) ? print(\'x\') : false; //'); + + $this->assertFalse($this->CallGetAuthUser()); + } + + public function testGetAuthUserFromHeaderWithoutAllowlist() + { + if (!function_exists('getallheaders')) { + $this->markTestSkipped('getallheaders() not available'); + } + $_SERVER['HTTP_X_REMOTE_USER'] = 'CN=header-test'; + $this->oConfig->SetExternalAuthenticationVariable('getallheaders()[\'X-Remote-User\']'); + + $this->assertSame('CN=header-test', $this->CallGetAuthUser()); + } +}