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

[Messenger] Remove TLS related options when not using TLS #41616

Merged

Conversation

odolbeau
Copy link
Contributor

@odolbeau odolbeau commented Jun 8, 2021

Q A
Branch? 5.2
Bug fix? not really
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Remove TLS related options when not using TLS to connect to a broker.

The goal is to be able to use the same configuration for both amqp:// & amqps:// DSN.

Currently, when using a configuration containing a cacert key with a non-TLS DSN will throw a AMQPConnectionException (Socket error: could not connect to host.)

Configuration example:

framework:
    messenger:
        transports:
            async:
              dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
              options:
                cacert: '%kernel.project_dir%/amqp_cacert.pem'

@odolbeau odolbeau requested a review from sroze as a code owner June 8, 2021 11:55
@carsonbot carsonbot added this to the 5.2 milestone Jun 8, 2021
@carsonbot carsonbot changed the title Remove TLS related options when not using TLS [Messenger] Remove TLS related options when not using TLS Jun 8, 2021
@YaFou
Copy link
Contributor

YaFou commented Jun 9, 2021

Can you implement a test, please?

@odolbeau odolbeau force-pushed the improve-messenger-amqp-tls-support branch from 89e2d1d to 37e602d Compare June 9, 2021 13:18
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 as a bugfix for 5.2

@javiereguiluz
Copy link
Member

javiereguiluz commented Jun 10, 2021

I don't know if my question makes sense ... but I'll ask it: if we silently drop the TLS config when using amqp:// instead of amqps:// ... what would happen if someone makes a mistake in their configuration and forgot to add the s of amqps (or removes it unwillingly) ?

Before, this typo would have been caught by the exception ... but now that typo could become a potential security issue?

@odolbeau
Copy link
Contributor Author

odolbeau commented Jun 10, 2021

@javiereguiluz if your broker is configured to accept both TLS & non-TLS connections, you are right, the non-TLS connection will be used even if it's not what you were looking for.
That's why I filled Bugfix: not really: you won't have the error anymore but IMO it improves the DX: you don't have to worry about TLS & non-TLS connections (for prod / dev env typically), both will work with the same configuration.

@nicolas-grekas
Copy link
Member

Thank you @odolbeau.

@nicolas-grekas nicolas-grekas merged commit 9399e2c into symfony:5.2 Jun 17, 2021
@fabpot fabpot mentioned this pull request Jun 17, 2021
@fabpot fabpot mentioned this pull request Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants