aws / aws-php-sns-message-validator

Amazon SNS message validation for PHP
Apache License 2.0
210 stars 53 forks source link

Split up missing and invalid certificate errors #32

Closed smhg closed 6 years ago

smhg commented 6 years ago

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

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

smhg commented 6 years ago

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