Direct username + password does not work after change in php-amqplib

[olanko]

Hi,

Direct username + password connection (localhost with no ssl) does not work after this change in php-amqplib: https://github.com/php-amqplib/php-amqplib/commit/995a6d56670998f44c617b152e5186cb82815a12#diff-5e5ce4c996a350f5b6329ef3430992f3

This change somehow forces connection to use ssl even with no ssl defined in config/amqp.php. I could not find a decent solution myself. Does anyone have a fix for this?

BR, -Olli

[stevenklar]

Hi,

Without further investigation my first try would be to set 'ssl' to null in options which should trigger the empty condition. (as long as $ssl_protocol = null matches it should work without ssl)

But maybe we also have a look into our connect() method which directly use AMQPSSLConnection.

-skdo

[olanko]

Hi,

It seems that setting 'ssl_options' => ['ssl' => null] is not enough. I'll try to find the place where ssl is forced and i'll inform you if i find something.

-Olli

[stevenklar]

Hi,

you might be right. We most likely need to add the new constructor parameter otherwise it's always set to ssl. Here is the problem: https://github.com/bschmitt/laravel-amqp/blob/master/src/Request.php#L43

As a test, can you change that directly on your side and add an additional null? So it must look like this afterwards:

        $this->connection = new AMQPSSLConnection(
            $this->getProperty('host'),
            $this->getProperty('port'),
            $this->getProperty('username'),
            $this->getProperty('password'),
            $this->getProperty('vhost'),
            $this->getProperty('ssl_options'),
            $this->getProperty('connect_options'),
            null
        );

If that works we might need to add some more ssl configuration there.

Do you have any idea why they introduced redundant configuration there? Seems like they want ssl as a sane default which would make sense but breaks stuff.

-skdo

[olanko]

Hi,

I just tried to add that extra null as well but it wouldn't help. I think the problem might be somewhere even deeper.

I think they had a problem when someone wanted to use tls and at the same time they introduced this $ssl_protocol parameter defaulting to ssl.

-Olli

[olanko]

Hi,

This piece of code in constructor of AMQPSSLConnection causes that $options['ssl'] is always set and not empty.

        if (!isset($ssl_options['verify_peer'])) {
            $ssl_options['verify_peer'] = true;
        }

This enforces SSL to be used always. I have no quick fix for this :) Maybe it is a problem in their side.

-Olli

[olanko]

Hi,

Adding check for !empty($ssl_options) into AMQPSSLConnection fixes the problem.

        if (!empty($ssl_options) && !isset($ssl_options['verify_peer'])) {
            $ssl_options['verify_peer'] = true;
        }

I'll send this to php-amqplib also a bit later.

Thanks to you! -Olli

[stevenklar]

@olanko Thank you for the clarification.

I would like them to fix this problem prior to us. As we just consume them. What do you think? Is this closed on our side?

This might also be a feature request to use tls instead of ssl which should not be possible without further enhancement.

[olanko]

Yes, I would consider this closed in here. I'm just writing an issue on their side.

-Olli

[stevenklar]

Thanks. Please don't hesitate to link the issue you create here so we can track this further.

[olanko]

Hi,

PR is here: https://github.com/php-amqplib/php-amqplib/pull/674 and issue report here https://github.com/php-amqplib/php-amqplib/issues/672

-Olli

[olanko]

Their comment was simple that why to use AMQPSSLConnection if ssl is not defined. I understand their point and I think I know the reason why it is done this way after all.

I'll fix our problem locally for now and hope you'll find a solution some day :)

All the best, -Olli

[bschmitt]

Thanks for your efforts, I'm gonna prepare a PR later today. The handy thing with the old implementation was that you could define by configuration if you want to use SSL or not.

[bschmitt]

New version 2.0.4 has been published.