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

Require minimum key size for OpenSSL keys #855

Merged
merged 8 commits into from
Aug 5, 2022

Conversation

Slamdunk
Copy link
Collaborator

@Slamdunk Slamdunk commented Jul 27, 2022

Fixes #854

Few notes to read the PR effectively:

  1. Hmac::minimumBytesLengthForKey has been renamed to Hmac::minimumBitsLengthForKey but it's not a BC-Break since it hasn't been released yet
  2. Like in Require minimum key size for HMAC algorithm #835 (comment) main classes now require a safe minimum key length, Unsafe* classes behave like the previous one and have already been marked as @deprecated
  3. Core classes have been copy-pasted, basically, in order to allow an easier just-delete-deprecated-classes in next major
  4. TestCases have also been copy-pasted except for key length tests
  5. openssl_sign error test ::signShouldRaiseAnExceptionWhenKeyIsInvalid used a short key to tests CannotSignPayload exception raise, but now that's blocked earlier and only UnsafeRsaTest covers those lines of code. In the next major I guess we'll need to just replace the if (! openssl_sign with an assert, maybe?
  6. Doc has been updated too

MT fails because I've been a bit too wary:

 $details = openssl_pkey_get_details($key);
-        assert(array_key_exists('bits', $details) && is_int($details['bits']));
+        assert(array_key_exists('bits', $details) || is_int($details['bits']));

Honestly I don't have enough trust in PHP to delete this.

@lcobucci
Copy link
Owner

Thanks @Slamdunk! I'll review it ASAP (I've been dealing with so many things at work this week 😰)

Copy link
Owner

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, I'll rebase your branch to handle them and we can get it merged.
Thanks for your help!

@lcobucci lcobucci force-pushed the openssl_key_requirements branch from b2f996e to a7bb8dd Compare August 5, 2022 12:36
@lcobucci lcobucci force-pushed the openssl_key_requirements branch from a7bb8dd to 82ad9ce Compare August 5, 2022 13:12
@lcobucci lcobucci merged commit 1d61d63 into lcobucci:4.2.x Aug 5, 2022
@Slamdunk Slamdunk deleted the openssl_key_requirements branch August 5, 2022 13:16
@lcobucci
Copy link
Owner

lcobucci commented Aug 5, 2022

I think we're good for 4.2.0 🎉
I'll prepare the changelog and release it in the evening.

@Slamdunk thanks for all your help!

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.

Require minimum key size for RSA keys
2 participants