Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@odolbeau
Copy link
Contributor

@odolbeauodolbeau commentedJun 8, 2021•
edited
Loading

QA
Branch?5.2
Bug fix?not really
New feature?no
Deprecations?no
Tickets-
LicenseMIT
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 bothamqp:// &amqps:// DSN.

Currently, when using a configuration containing acacert key with a non-TLS DSN will throw aAMQPConnectionException (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'

@odolbeauodolbeau requested a review fromsroze as acode ownerJune 8, 2021 11:55
@carsonbotcarsonbot added this to the5.2 milestoneJun 8, 2021
@carsonbotcarsonbot changed the titleRemove TLS related options when not using TLS[Messenger] Remove TLS related options when not using TLSJun 8, 2021
@YaFou
Copy link
Contributor

Can you implement a test, please?

@odolbeauodolbeauforce-pushed theimprove-messenger-amqp-tls-support branch from89e2d1d to37e602dCompareJune 9, 2021 13:18
Copy link
Member

@chalasrchalasr 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 commentedJun 10, 2021•
edited
Loading

I don't know if my question makes sense ... but I'll ask it: if we silently drop the TLS config when usingamqp:// instead ofamqps:// ... what would happen if someone makes a mistake in their configuration and forgot to add thes ofamqps (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
ContributorAuthor

odolbeau commentedJun 10, 2021•
edited
Loading

@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 filledBugfix: 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-grekasnicolas-grekas merged commit9399e2c intosymfony:5.2Jun 17, 2021
@fabpotfabpot mentioned this pull requestJun 17, 2021
@fabpotfabpot mentioned this pull requestJun 30, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

7 participants

@odolbeau@YaFou@javiereguiluz@nicolas-grekas@chalasr@derrabus@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp