Skip to content

[Amqp] Add amqps support #38007

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 16, 2020
Merged

[Amqp] Add amqps support #38007

merged 1 commit into from
Sep 16, 2020

Conversation

vasilvestre
Copy link

@vasilvestre vasilvestre commented Aug 31, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #37002
License MIT
Doc PR symfony/symfony-docs#14250

Cases

Case 1 (simple):
I want to set up my app with a locally installed amqp broker supporting SSL. No auth is needed, no vhost, only SSL certificate file located at /etc/ssl/cafile.
'amqps://'

Case 2 (complex):
I use a special port, special file locations for SSL. I pass it through php.ini or configuration.

Questions

  1. Actually AmqpTransportFactory use supports function to see if 'amqp://' is contained in dsn string and accept cacert argument to use SSL
    How are we supposed to cover AMQPS's case in your opinion?

cacert argument is settable through php.ini and code.

Observations

I think this PR should aim at:

  • Having correct defaults ssl values in case of 'amqps://' eg : case 1
  • Being able to pass cacert file path and verify argument. eg : case 2
  • Test: but test what?

EDIT

As discuted with @nicolas-grekas, we should check that cacert is existing in php.ini or is passed in configuration and throw an exception to give a nice feedback about what's wrong. Php amqp lib isn't actually precise about why it fails when you forget it.

Steps

  • Ensure AMQPS can be used out of the box without DSN.
  • If not, try to modify vendor directly to fix it.
  • Modify vendor to make AMQPS works with DSN.
  • Check cacert to throw a better exception.
  • Add test related to last point.
  • Pass CI

@vasilvestre vasilvestre requested a review from sroze as a code owner August 31, 2020 12:53
@nicolas-grekas nicolas-grekas changed the title bug #37002 [Amqp] Add amqps support [Amqp] Add amqps support Aug 31, 2020
@nicolas-grekas nicolas-grekas added this to the next milestone Aug 31, 2020
@nicolas-grekas
Copy link
Member

Can you make it work currently using only php.ini settings?

@vasilvestre
Copy link
Author

vasilvestre commented Aug 31, 2020 •
edited
Loading

Can you make it work currently using only php.ini settings?

I do not really understand, wdym? Connect to amqp with ssl ?
If yes, it do actually work. The PR just allow amqps string (aws actually give you an amqps string only)

@nicolas-grekas
Copy link
Member

In php-amqp/php-amqp#205, it looks like setting the amqp.cacert ini setting is enought to use SSL, isn't it?

@vasilvestre
Copy link
Author

vasilvestre commented Sep 1, 2020 •
edited
Loading

In php-amqp/php-amqp#205, it looks like setting the amqp.cacert ini setting is enought to use SSL, isn't it?

It is :

fabpot
fabpot requested changes Sep 10, 2020
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Cosmetic review.

@vasilvestre
Copy link
Author

Cosmetic review.

I actually have added a new exception, I'm not sure it is useful and require to have a new PR merged first. The SSLException class, what do you think about it? Should I use a RuntimeException, a LogicException, or the new one?

@vasilvestre vasilvestre requested a review from fabpot September 11, 2020 12:01
stof
stof reviewed Sep 11, 2020
View reviewed changes
weaverryan
weaverryan reviewed Sep 15, 2020
View reviewed changes
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Minor comments - looks good to me, though I have zero experience with this!

I've seen some confusion around people using amqps:// and it not working, which is partially why I suggest changing the message to also notify people that amqp:// IS a valid option. For CloudAMQP, I think they are now showing amqps:// by default, which is confusing some people when it doesn't work (especially beginners).

fabpot
fabpot requested changes Sep 16, 2020
View reviewed changes
@vasilvestre vasilvestre requested a review from fabpot September 16, 2020 07:22
fabpot
fabpot requested changes Sep 16, 2020
View reviewed changes
@vasilvestre vasilvestre requested a review from fabpot September 16, 2020 07:59
@fabpot
Copy link
Member

fabpot commented Sep 16, 2020

@vasilvestre Can you check the tests please? They seem to be broken by the changes introduced by this PR.

@vasilvestre
Copy link
Author

vasilvestre commented Sep 16, 2020 •
edited
Loading

@vasilvestre Can you check the tests please? They seem to be broken by the changes introduced by this PR.

Yes I'm actualling trying to search how we should handle ini parameters in tests, these are my own test, I think in phpunit.xml but the parameters should depend of system running tests, it could break on windows..

@vasilvestre
Copy link
Author

@fabpot it will pass this time :)

@fabpot
Copy link
Member

fabpot commented Sep 16, 2020

Thank you @vasilvestre.

@fabpot fabpot merged commit 83910df into symfony:master Sep 16, 2020
@fabpot fabpot mentioned this pull request Sep 16, 2020
Closed
@vasilvestre vasilvestre deleted the fix/amqps branch September 16, 2020 13:52
@vasilvestre
Copy link
Author

@fabpot should I create a documentation PR?

@weaverryan
Copy link
Member

That would be awesome :). A docs issue has already been created

@vasilvestre
Copy link
Author

here we are : symfony/symfony-docs#14250

@stof
Copy link
Member

stof commented Sep 16, 2020

To make this easier, should we automatically provide the cacert option to AMPQ if not configured, based on the CA file used by everything else in PHP (basically emulating what is suggested in php-amqp/php-amqp#311 as a better API) ? The amqps scheme is enough for us to be sure that the encrypted connection is being requested.
Of course, an explicit cacert config for AMQP should still be allowed in case of needing a weird CA file.

@vasilvestre
Copy link
Author

If this seem fine to you, I would love working with queuing systems in future so I would love to work on this. :)

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@ajcerqueti ajcerqueti mentioned this pull request Sep 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan left review comments

@stof stof stof left review comments

@OskarStark OskarStark OskarStark left review comments

@andrew-demb andrew-demb andrew-demb left review comments

@fabpot fabpot fabpot approved these changes

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Feature Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

Add (amqp) ssl capacity to messenger DSN
8 participants
@vasilvestre @nicolas-grekas @fabpot @weaverryan @stof @OskarStark @andrew-demb @carsonbot