-
Notifications
You must be signed in to change notification settings - Fork 258
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
ssl support #156
ssl support #156
Conversation
Add ssl context parameter to get ability connect mysql using ssl
Codecov Report
@@ Coverage Diff @@
## master #156 +/- ##
==========================================
- Coverage 92.01% 91.03% -0.99%
==========================================
Files 12 12
Lines 1804 1829 +25
Branches 254 258 +4
==========================================
+ Hits 1660 1665 +5
- Misses 89 107 +18
- Partials 55 57 +2
Continue to review full report at Codecov.
|
Contribution in this area very appreciated! We need to figure out how to test SSL on CI, do we need specially configured MySQL? |
aiomysql/connection.py
Outdated
self.write_packet(packet) | ||
# upgrade connection to ssl | ||
# close recent reader and write and keep socket connected | ||
sock = self._writer._transport._sock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sock = self._writer.transport.get_extra_info('socket', default=None)
is beter, since _transport._sock
private attributes
self._writer._transport._call_connection_lost = \ | ||
types.MethodType( | ||
_call_connection_lost, self._writer._transport) | ||
self._writer._transport._force_close(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just self._writer._transport.close()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just close transport asyncio close socket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go it, but not sure we need wot close transport...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to pass ssl context in https://github.com/aio-libs/aiomysql/pull/156/files#diff-b4987acea49665e9f985c0da9f5604f4R487 without upgrading socket ourself and let asyncio to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be less robust but code simpler
aiomysql/connection.py
Outdated
# close recent reader and write and keep socket connected | ||
sock = self._writer._transport._sock | ||
# patch asyncion | ||
self._writer._transport._call_connection_lost = \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you clarify why we need this line? _call_connection_los
is private... and may not work for other loop
implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do this to replace asyncio method behavior, to not close socket after close transport
example of connection
|
aiomysql/connection.py
Outdated
self._writer._transport._force_close(None) | ||
self._reader, self._writer = yield from \ | ||
asyncio.open_connection(sock=sock, ssl=self._sslcontext, | ||
server_hostname=self._host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loop=self._loop
should be passed explicitly.
I merged aiomysql 0.0.12 with this PR (resolved the conflicts in connection.py which is here- https://github.com/terrisgit/aiomysql/blob/master/aiomysql/connection.py) and everything is working great. However, #225 also supports AWS db auth tokens which also require SSL. Therefore, I think this PR should be denied unless the SSL implementation is better than #225 's in which case the two should be reconciled. |
see #280 |
Thanks a lot for contribution, but other PR was finished first. |
Added ssl support for connection
using external parameter ssl context to wrap connection
steps: