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

Split up missing and invalid certificate errors #32

Closed
smhg opened this issue Nov 8, 2017 · 5 comments
Closed

Split up missing and invalid certificate errors #32

smhg opened this issue Nov 8, 2017 · 5 comments

Comments

@smhg
Copy link
Contributor

smhg commented Nov 8, 2017

The Cannot get the public key from the certificate. error gets thrown in these circumstances:

  • When openssl_pkey_get_public can't extract the key and returns false.
  • When the certClient can't read the certificate.

The latter happens fairly regularly because of network issues. When using the default certClient (file_get_contents) the $certificate variable will contain false (and a PHP Warning will get logged).
Could this get checked separately from the first circumstance?

A possible issue is a custom certClient which returns something different.
On the other hand, having $certificate === false throw a separate error sounds quite harmless.

I'm happy to send a PR if this sounds good.

@kstich
Copy link
Contributor

kstich commented Nov 16, 2017

Separating the error messages seems like a good idea to me, especially since it's possible the default client can fail to retrieve. Given that openssl_pkey_get_public can take non-strings, would it be better to check for a false-y value instead of explicitly false?

@smhg smhg changed the title Split up missing and invalid public key errors Split up missing and invalid certificate errors Nov 16, 2017
@smhg
Copy link
Contributor Author

smhg commented Nov 16, 2017

That sounds like the right thing to do, yes.
There might however be something out there counting on the fact that their custom certClient can return anything which is then passed on to openssl_get_publickey (don't ask me why that would be a good idea, but it is possible). To not break those edge-cases, an explicit false sounds the safest.
This way you could keep it to a minor (semver) release, no?

@kstich
Copy link
Contributor

kstich commented Nov 16, 2017

After running through a few tests, it looks like other false-y values don't cause any issues for openssl_pkey_get_public. That means a === false should work fine.

@smhg
Copy link
Contributor Author

smhg commented Dec 9, 2017

Hi @kstich! Anything I can help with to get this merged into master?

@smhg
Copy link
Contributor Author

smhg commented Feb 20, 2018

I'm sorry to keep pinging you @kstich, but a release with this change would still be much appreciated.

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

No branches or pull requests

2 participants