-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[CloudFront] Add exception when signature generation fails #2590
[CloudFront] Add exception when signature generation fails #2590
Conversation
ce21d37
to
7d537d3
Compare
7d537d3
to
7174662
Compare
7174662
to
728ce05
Compare
728ce05
to
202795e
Compare
202795e
to
217d065
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. Can you just please add a new line in between line 122 and 123?
Thanks!
217d065
to
a65c76f
Compare
@yenfryherrerafeliz refactored and rebased code, ready to go when you are ready! |
a65c76f
to
55c8fad
Compare
Hi @DocLM, one last thing. Could you please add a generic error message for when $errorMessages is empty. Just to be safe. $exceptionMessage = implode("\n",$errorMessages);
if (count($errorMessages) == 0) {
$exceptionMessage = "An error has occurred when signing the policy";
}
throw new \RuntimeException($exceptionMessage); Thanks! |
55c8fad
to
4189548
Compare
@yenfryherrerafeliz Fallback error message now available! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @DocLM!
See #2608
Description of changes:
On recent RHEL/RockyLinux/Alma 9 the default system security policies disable SHA-1 in OpenSSL.
This cause
CloudFrontClient
to silently fail signature generation ingetSignedUrl
and generate an URL with empty signature.I've added a check for empty signature and an exception with OpenSSL errors to help to identify the issue.
I'd like to add a test but I'm struggling to find a way to dynamically disable the algorithm during unit tests, any suggestion is welcome.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.