Support x509certMulti #2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for handling multiple X.509 certificates from an Identity Provider (IdP), which is necessary for scenarios like certificate key rollover. The implementation detects whether the IdP provides multiple certificates and stores them accordingly.
Changes:
- Modified certificate retrieval to handle both single and multiple certificate formats
- Added storage mechanism for multiple certificates using JSON format
- Updated PHP testing matrix to include PHP 8.5
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Commands/GenerateKeys.php | Implements logic to detect and store multiple certificates in JSON format, or single certificate in PEM format |
| config/php-saml-toolkit.php | Dynamically loads multiple certificates from JSON file if it exists |
| .github/workflows/phpunit.yml | Adds PHP 8.5 to the test matrix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'x509certMulti' => (file_exists($cert_path.'/idp_cert_multi.json')) | ||
| ? json_decode(file_get_contents($cert_path.'/idp_cert_multi.json'), true) | ||
| : null, |
There was a problem hiding this comment.
The json_decode call lacks error handling. If the JSON file is corrupted or contains invalid JSON, this will silently fail and return null, which could be confused with the intentional null for missing files. Consider validating the JSON decode result or handling JSON_ERROR_NONE to distinguish between missing files and malformed JSON.
Addresses the optional possibility of multiple certs from an IdP (for instance, supporting key rollover).
This happened on the Weill Apollo test site and this was the solution.