derricksmith / phpsaml

GLPI Plugin - SAML integration using the Onelogin SAML Library
MIT License
32 stars 24 forks source link

GLPIv10 support? #132

Open DanielWeeber opened 1 year ago

DanielWeeber commented 1 year ago

This does not seem to support GLPIv10. Any chance we can get an update?

DonutsNL commented 1 year ago

Im currently developing against the latest glpi version and created a pull request to merge the latest changes in.

DonutsNL commented 1 year ago

If you cant wait: https://github.com/DonutsNL/phpsaml

its not fully tested yet.

laszlokertesz commented 1 year ago

It is actually working for version 1.20 if you put everything in the database manually.

DonutsNL commented 1 year ago

No manual insertion should be required, you should be able to install it and configure it using just the plugins config page. In my development environment (running the latest version of GLPI) it is functioning properly against Azure AD - SAML enterprise App.

AldarisPale commented 1 year ago

@DonutsNL can confirm that on GLPI 10.0.7 + latest master from https://github.com/DonutsNL/phpsaml results in empty page for /plugins/phpsaml/front/config.php

Login does seem to work though even with modified https://github.com/derricksmith/phpsaml/issues/120#issuecomment-1525542541 applied on top of https://github.com/DonutsNL/phpsaml (the file has changed to plugins/phpsaml/lib/php-saml/src/Saml2/Utils.php)

Edit: https://glpi-project.org/glpi-9-5-x-will-be-discontinued/ is coming sooner than desired

AldarisPale commented 1 year ago

The error message is: glpiphplog.CRITICAL: *** Uncaught Exception TypeError: Argument 1 passed to PluginPhpsamlConfig::requested_authn_context() must be of the type string, null given, called in /var/www/plugins/phpsaml/inc/config.class.php on line 162 in /var/www/plugins/phpsaml/inc/config.class.php at line 889 Backtrace : plugins/phpsaml/inc/config.class.php:162 PluginPhpsamlConfig->requested_authn_context() plugins/phpsaml/front/config.php:53 PluginPhpsamlConfig->showForm() public/index.php:73 require()

DonutsNL commented 1 year ago

I added type safety but was not yet able to test all possible conditions. As a work arround remove the type in de method listed. I.e requested_authn_context(string $var) to requested_authn_context($var)

AldarisPale commented 1 year ago

Thanks, @DonutsNL . When I change row 889 at https://github.com/DonutsNL/phpsaml/blob/56a15f3aa923588d27f70c4cd6d00729f0a7b533/inc/config.class.php#L817 from protected function saml_idp_single_logout_service(string $cValue) : void to protected function saml_idp_single_logout_service($cValue) : void then /plugins/phpsaml/front/config.php does open, but with errors:

Phpsaml expected 21 configuration items but got 19 items instead No valid Ipd certificate details provided or available The optional Service Provider Certificate is not configured, we strongly recommend that you do and enable strict mode You are sing version 1.2.1 which is also the latest version

If I enable debug mode in GLPI, the additonal notice appears: PHP Notice (8): Undefined variable: pCert in .../plugins/phpsaml/inc/config.class.php at line 409.

DonutsNL commented 1 year ago

Thats strange. It seems the db schema was not updated or the update was not called. Ill dive into that. See update.php what schema updates are required and run them manually.

AldarisPale commented 1 year ago

Thanks, @DonutsNL , for looking into it. Also noticed a typo at https://github.com/DonutsNL/phpsaml/blob/56a15f3aa923588d27f70c4cd6d00729f0a7b533/install/update.class.php#L301 Instead of Toolbox::logInFile("phpsaml", "INFO -- PHPSAML upgraded to 1.2.1" . "\n", true); it should probably be Toolbox::logInFile("phpsaml", "INFO -- PHPSAML upgraded to 1.2.2" . "\n", true);

DonutsNL commented 1 year ago

Yea i prob did not update all versions yet and that also causes the update.php to not function. I was going to add a rerun option if less than expected items where found in the database.

AldarisPale commented 1 year ago

@DonutsNL one more addition regarding glpi 10 - this time about fusioninventory -> glpi inventory / glpi agent migration.

Here's a patch for a couple-days-old phpsaml version, does not directly apply anymore but it should be easy to fix. It should handle all combinations of fusioninventory agent/ glpi agent and fusioninventory plugin / glpiinventory plugin.

--- a/plugins/phpsaml/inc/phpsaml.class.php
+++ b/plugins/phpsaml/inc/phpsaml.class.php
@@ -182,13 +182,35 @@
                 $cfgObj             = new PluginPhpsamlConfig();
                 $config                 = $cfgObj->getConfig();

-               // Return false for fusioninventory agents
+               // Return false for fusioninventory agent using 
fusioninventory endpoint
                 if ((strpos($_SERVER['HTTP_USER_AGENT'], 
'FusionInventory-Agent_') !== false) &&
                         (strpos($_SERVER['REQUEST_URI'], 
'/plugins/fusioninventory/'))) {

                         return false;
                 }

+               // Return false for fusioninventory agent using 
glpiinventory endpoint
+               if ((strpos($_SERVER['HTTP_USER_AGENT'], 
'FusionInventory-Agent_') !== false) &&
+                       (strpos($_SERVER['REQUEST_URI'], 
'/plugins/glpiinventory/'))) {
+
+                       return false;
+               }
+
+               // Return false for glpi inventory agent using 
fusioninventory endpoint
+               if ((strpos($_SERVER['HTTP_USER_AGENT'], 'GLPI-Agent_') 
!== false) &&
+                       (strpos($_SERVER['REQUEST_URI'], 
'/plugins/fusioninventory/'))) {
+
+                       return false;
+               }
+
+               // Return false for glpi inventory agent using 
glpiinventory endpoint
+               if ((strpos($_SERVER['HTTP_USER_AGENT'], 'GLPI-Agent_') 
!== false) &&
+                       (strpos($_SERVER['REQUEST_URI'], 
'/plugins/glpiinventory/'))) {
+
+                       return false;
+               }
+
+
                 // Return true for files in Excludes
                 foreach (self::EXCLUDED as $value) {
                         if ((PHP_SAPI === 'cli') || 
strpos($_SERVER['REQUEST_URI'], $value) !== false) {
DonutsNL commented 1 year ago

@AldarisPale Would it be to big a hussle to create a new issue for this problem (if it doesnt allready exist) including a clear description of the issue (being fixed) by the code snippet?

This will help me and @derricksmith out keeping a good overview of issues and things.

Thx

AldarisPale commented 1 year ago

@DonutsNL separate issue created: https://github.com/derricksmith/phpsaml/issues/134

AldarisPale commented 1 year ago

@DonutsNL about https://github.com/derricksmith/phpsaml/issues/132#issuecomment-1566962011 - when I downloaded bleeding edge from https://github.com/DonutsNL/phpsaml an upgrade was offered and plugin config page does not result in empty page anymore. Thanks!

The current messages are: No valid Ipd certificate details provided or available The optional Service Provider Certificate is not configured, we strongly recommend that you do and enable strict mode A different version of Phpsaml is marked as latest. Version 1.2.1 was found in the repository, you are running 1.2.2

DonutsNL commented 1 year ago

Thanks for checking and reporting back at me. This helps me greatly 👍

The messages shown are informational mostly and should allow you to check the sanity of the config a little better. To elaborate a bit more:

No valid Ipd certificate details provided or available

The optional Service Provider Certificate is not configured, we strongly recommend that you do and enable strict mode

A different version of Phpsaml is marked as latest. Version 1.2.1 was found in the repository, you are running 1.2.2

I hope you like these additions, suggestions for additional validations are welcome ;-)

AldarisPale commented 1 year ago

Thanks, @DonutsNL I'm debugging the Ipd problem (there's a typo in here btw - it should be IdP). What's strange is that when I copy certificate from "Service Provider Certificate" field and paste it into text file and then run openssl x509 -in certificate.crt -text -noout, everything is valid. Also, it works in the practical world too.

So not clear why it is complaining, cannot see other error messages either. openssl php extension is installed

DonutsNL commented 1 year ago

you might be hitting a GLPI filtering issue I experienced whenever the first and initial update didnt go right and values are captured by the GLPI _POST handler. GLPI then replaces all line brakes with secure entities (to litteral \r\n) effectivly breaking the certificate. Breaking it because (if i remember correcly) X509 certificates only allows \n for a line breaks in base64 encoded certificates. I tried to correct the filtering issue post filtering with mixed results, i was considering the alternative capturing the $post in the acs.php before glpi would have a chance to filter stuff out. This might cause CSRF compliancy issues (as the CSRF field also being passed by the form). I was also considering an alternative like uploading the certificate file and storing it as CLOB or BLOB in the config field.