phpsaml icon indicating copy to clipboard operation
phpsaml copied to clipboard

GLPIv10 support?

Open DanielWeeber opened this issue 1 year ago • 18 comments

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

DanielWeeber avatar Apr 29 '23 19:04 DanielWeeber

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

DonutsNL avatar Apr 29 '23 20:04 DonutsNL

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

its not fully tested yet.

DonutsNL avatar Apr 29 '23 20:04 DonutsNL

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

laszlokertesz avatar May 03 '23 11:05 laszlokertesz

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.

DonutsNL avatar May 03 '23 11:05 DonutsNL

@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 avatar May 26 '23 11:05 AldarisPale

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()

AldarisPale avatar May 26 '23 12:05 AldarisPale

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)

DonutsNL avatar May 28 '23 08:05 DonutsNL

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.

AldarisPale avatar May 29 '23 10:05 AldarisPale

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.

DonutsNL avatar May 29 '23 10:05 DonutsNL

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);

AldarisPale avatar May 29 '23 11:05 AldarisPale

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.

DonutsNL avatar May 29 '23 14:05 DonutsNL

@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) {

AldarisPale avatar May 30 '23 10:05 AldarisPale

@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

DonutsNL avatar May 30 '23 12:05 DonutsNL

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

AldarisPale avatar May 30 '23 12:05 AldarisPale

@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

AldarisPale avatar May 30 '23 13:05 AldarisPale

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 new config class will try to decode the provided certificate and report the decoded certificate's CN and Validity date back. If it wasnt able to do so it will report this message. This is either because the idp certificate string is not yet configured, your PHP installation does not have the SSL module enabled or the provided string isnt correct (and thus could not be decoded).

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

  • The config class did not find a SP certificate. Without this certificate the strict config and additional security options will actually break the configuration. The config should also give an warning if you try to enable strict mode without the SP certificate being present. Im not sure I allready implemented it so that it will not allow you to enable strict without this certificate present.

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

  • The config class now checks what version is published by @derricksmith and will report any delta's. You are running bleeding-edge and this version is not yet merged and published by @derricksmith.

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

DonutsNL avatar May 30 '23 13:05 DonutsNL

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

AldarisPale avatar May 30 '23 13:05 AldarisPale

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.

DonutsNL avatar May 30 '23 14:05 DonutsNL