Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update RsaKeysProvider.php to use OpenSSL #446

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aLxSpir
Copy link

@aLxSpir aLxSpir commented Oct 31, 2024

Use OpenSSL instead of phpseclib\Crypt\RSA;

Use OpenSSL instead of phpseclib\Crypt\RSA;
@jokesterfr
Copy link
Contributor

Thank you for your contribution.
We are missing some contextual information here, could you describe in your PR:

1. what problem are you trying to solve?
2. how can anyone reproduce your issue?
3. which version of the module is impacted?
4. how do you analyse the issue and the expected changes?

We are going to update our contribution policy to make these rules easier to implement and understand.

Again, thank you for your contribution.

@jokesterfr
Copy link
Contributor

For the record, it seems phpseclib\Crypt\RSA was used to avoid dependency collision with other modules, as it is a less common package.

However Today we have a php-scoper implementation that prevent this collision to happen, and using a more straightforward implementation is a good idea.

@aLxSpir
Copy link
Author

aLxSpir commented Oct 31, 2024

The main goal was to address a 503 Service Unavailable error that I was encountering, restricting me to connect to the admin panel.

1. What problem are you trying to solve?

The issue involves an integration with the phpseclib library, which is currently being used for RSA encryption in the module. However, due to compatibility or configuration issues, the library may be problematic in the environment where the module runs.
By switching to OpenSSL, which is natively supported in PHP and widely compatible, the goal is to make encryption operations more stable and less reliant on external dependencies.

2. How can anyone reproduce your issue?

To reproduce this issue, one can install the impacted versions of ps_accounts and attempt to run functions requiring RSA key generation or signing (for example, trying to authenticate in the backend with the module being active). If the environment doesn’t have the phpseclib dependency correctly configured, An error 503 would be thrown without any log details.

3. Which version of the module is impacted?

This issue impacts all versions of the module that rely on phpseclib for encryption operations. Specifically, it would affect all versions where phpseclib was implemented without fallbacks for environments where it is not configured or compatible.
Latest version, v7.0.8, is concerned.

4. How do you analyze the issue and the expected changes?

The issue appears to stem from a missing or misconfigured phpseclib library, possibly due to Composer dependency issues or compatibility constraints. Switching to OpenSSL, a native PHP extension, would reduce dependency issues and improve compatibility. The expected changes involve refactoring the module's encryption logic to replace phpseclib's RSA methods with OpenSSL's native functions for key generation, signing, and verification.

Fix PHP-CS issues in RsaKeysProvider.php
return null;
}

openssl_public_encrypt($string, $encrypted, $publicKeyResource);

Check failure

Code scanning / SonarCloud

Encryption algorithms should be used with secure mode and padding scheme High

Use secure mode and padding scheme. See more on SonarCloud
Improvement of security rating and aligning with best practices for RSA encryption by using OAEP padding.
Copy link

sonarcloud bot commented Oct 31, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants