-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Issue #672: Changes in SSL handling breaks bschmitt/laravel-amqp #674
Conversation
I'm not opposed to this change, but why would someone use |
Hi, I understand and kinda agree with you. I don't know the reasoning why laravel-amqp uses this. My guess is that it has been easier to always use AMQPSSLConnection because it has actually worked with or without SSL depending on $ssl_options before this change 0f16fb8. Is that I'll handle this case locally and hope that someone makes these libraries work together again :) BR, |
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.
What about PHP5.5 users who does not submit options(empty $ssl_options) and expect that verify_peer is enabled for secure connection? I think we must still keep compatibility here.
Well, according to this old code we used to check if |
@lukebakken I agree, that change was not 100% backwards compatible. So we either revert it or improve to acceptable point. I think 1st option isn't nice because additional argument $ssl_protocol creates additional confusion about how variables must be passed, imagine matrix:
Is this what everyone would expect? My common sense says that AMQPSSLConnection always must be secure or fail to establish connection. I see laravel guys are adopting library to latest changes, so no big problem here? |
I agree. Anyway, at this point, let's revert to the old behavior and then ensure this is fixed in |
I will take care. |
Hi, Thanks for taking this seriously! Although they fixed my original problem already, you are doing good job. -Olli |
Fix on PR #677 |
Fixed by #677 |
Referring to issue #672
Automatically setting
$ssl_options['verify_peer'] = true
breaksbschmitt/laravel-amqp
which sends empty ssl_options when ssl is not enabled. Setting verify_peer enforcesssl
later inPhpAmqpLib/Wire/IO/StreamIO.php
even if it is not what the user wants.Simply checking if
$ssl_options
is defined fixes this problem.