Skip to content

Allow ssl.SSLContext instance to be supplied to connect and from_url … #142

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

Closed
wants to merge 6 commits into from

Conversation

pwistrand
Copy link
Contributor

Its impossible to use client side certificates with a TLS termination proxy to connect to rabbitmq without having control over the ssl.SSLContext. This change is a simple one that makes the from_url and connect methods behaviour like the loop.create_connection() they ultimately call. This will also make using self signed certificates since they don't have to be installed in an OS dependant manner but instead can just be referenced by the path in the supplied context. See #91

@skewty
Copy link
Contributor

skewty commented Feb 18, 2018

What happened with this? Is this issue actually still open? I see the pull request and all checks have passed with a few unit tests. Is something missing?

@dzen
Copy link
Contributor

dzen commented Jan 4, 2019

Hi, I'm very sorry for this late answer, but we need some more changes:
as the ssl can be both a bool or a SSLContext, this must be clarifed and I don't like having multiple type for a single argument.

@pwistrand
Copy link
Contributor Author

Hi @dzen , is it just the method documentation that needs to be changed or supporting either a boolean or SSLContext in the ssl keyword argument? If the later then I tend to agree with you but I have taken the same approach as the standard library (see https://docs.python.org/3/library/asyncio-eventloop.html#opening-network-connections.

@dzen
Copy link
Contributor

dzen commented Jan 8, 2019

Ping @RemiCardona for any opinion on this

@RemiCardona
Copy link
Contributor

If anything, this PR doesn't go far enough. We should just get rid of the few lines where we try to create a context ourselves and set options on it. We should just have a single ssl argument and simply forward it to loop.create_connection() and point to the relevant CPython documentation on how to use it.

@pwistrand
Copy link
Contributor Author

@dzen @RemiCardona removing bool support in the ssl parameter is going to break backwards compatibility which IMO is a big call and seems a step away from "batteries included". In the PR I tried to come up with what I felt was the least intrusive compromise but I'm happy to rework whatever way you decided.

@dzen
Copy link
Contributor

dzen commented Jan 11, 2019

@pwistrand : IMHO since we're still not in 1.0 version the API may change, and it already did.

Feel free to contribute as suggested :)

@notmeta
Copy link
Contributor

notmeta commented Jan 22, 2019

Happy to help work on this as it's something I need too.

I agree with what @dzen said, it's only at 0.12 so breaking changes may occur if they're important enough.

@pwistrand
Copy link
Contributor Author

@notmeta I'm overloaded at the moment so if you want to go ahead and make the changes go for it.

@notmeta
Copy link
Contributor

notmeta commented Jan 22, 2019

@pwistrand mind giving me access to your forked repo? cheers

@pwistrand
Copy link
Contributor Author

@notmeta invite sent

ssl_context.check_hostname = False
ssl_context.verify_mode = ssl_module.CERT_NONE
create_connection_kwargs['ssl'] = ssl_context
ssl.check_hostname = False
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to remove the verify_ssl parameter, since the context can be configured in the parent code

Copy link
Contributor

@notmeta notmeta Jan 24, 2019

Choose a reason for hiding this comment

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

I agree - have removed 🙂

@dzen
Copy link
Contributor

dzen commented Jan 24, 2019

To be honest, the PR would be more readable if you rebase it from master

pwistrand and others added 6 commits January 24, 2019 12:11

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…mirroring loop.create_connection()

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@notmeta
Copy link
Contributor

notmeta commented Jan 24, 2019

@dzen

To be honest, the PR would be more readable if you rebase it from master

Done, I've cleaned it up - sorry about that, must have messed something up whilst merging originally

@dzen
Copy link
Contributor

dzen commented Jan 24, 2019

We will merge soon :)

@dzen
Copy link
Contributor

dzen commented Jan 29, 2019

Rebased & Merged

@adamhooper
Copy link

This is a killer feature. I'd love to use it in a library I maintain, channels_rabbitmq. Could we please have a new release? (Or if releasing is hard, could you please share an idea of when the next release will happen?)

@dzen
Copy link
Contributor

dzen commented Feb 25, 2019

Hello @adamhooper,

we've done a lots of changes on master last two past month.
Did you tried your https://github.com/CJWorkbench/channels_rabbitmq with aioamqp master ?
Did you find any bugs or so ?

adamhooper added a commit to CJWorkbench/channels_rabbitmq that referenced this pull request May 6, 2019

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@adamhooper
Copy link

@dzen At long last I've tested channel_rabbitmq with aioamqp master. The unit tests all pass, and it's given me immense pleasure to migrate our test suite to SSL. Is there anything else I can do to help with a release?

@notmeta
Copy link
Contributor

notmeta commented May 7, 2019

Agreed, a release would be much appreciated; it's been a while.
Means I can remove the monkey patching from my code to add proper SSL support 😄

@dzen
Copy link
Contributor

dzen commented May 9, 2019

Just released aioamqp 0.13.0.

Feel free to report new incorrect behaviour and such !

Thanks for your patience 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants