Add implicit FTPS support #81

oleksandr-kuzmenko · 2018-10-09T16:08:03Z

SSL support added.

codecov · 2018-10-09T16:41:30Z

Codecov Report

Merging #81 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   94.01%   94.02%   +<.01%     
==========================================
  Files           7        7              
  Lines        1688     1690       +2     
==========================================
+ Hits         1587     1589       +2     
  Misses        101      101

Impacted Files	Coverage Δ
aioftp/server.py	`97.05% <100%> (ø)`	⬆️
aioftp/client.py	`92.52% <100%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95cd305...267db49. Read the comment docs.

asvetlov · 2018-10-09T17:38:19Z

Any chance to add a test?

oleksandr-kuzmenko · 2018-10-09T19:49:49Z

I thought about tests, but I don't know a good way for testing transport without ssl support at the server part. Can you advise me how to implement it?

pohmelie · 2018-10-10T11:34:58Z

@Alxpy, thanks for pr!

This pr is ¼ of what should be implemented for saying «we support ftps». There is two modes: explicit and implicit. And two sides: client and server. You implement implicit client ftps. Also, I'm sceptical about ftps, since it is pretty rare. But, anyway, I see no reasons to not to merge.

@asvetlov
I'm also want some good tests for this, but as mentioned: without server side implementation tests are not possible in meaning even if they will be implemented they will mock server. I'm not sure that this will be good tests.

@Alxpy, is there a chance you will implement implicit server side mode, so we can write tests and have half ftps support?

I will wait a day before merge. Maybe someone have strong point to reject this pr or @Alxpy take care of implicit server side support.

asvetlov · 2018-10-10T11:37:34Z

You are right, mocked tests are useless for this case

oleksandr-kuzmenko · 2018-10-10T12:06:48Z

@pohmelie thanks for the detailed answer!

I will try to implement implicit server side ftps.

rsichnyi · 2018-10-11T01:31:29Z

lgtm (server side is useless, imo)

aioftp/client.py

oleksandr-kuzmenko · 2018-10-11T22:46:45Z

Implicit server side FTPS added.

oleksandr-kuzmenko · 2018-10-11T22:56:43Z

I can't generate certificate which can work with Python 3.7 =(
https://github.com/aio-libs/aioftp/pull/81/files#diff-572606b9b77c96a18b4bf3eefabc6b25R65
https://bugs.python.org/issue34440

webknjaz · 2018-10-12T09:22:57Z

@Alxpy try generating test certs via trustme

oleksandr-kuzmenko · 2018-10-12T12:19:10Z

@webknjaz I tried, trustme certs doesn't work without ssl.match_hostname monkey patching even with Py3.6
Or I did something wrong...

But I can use trustme instead of certs files.

FYI @pohmelie

webknjaz · 2018-10-12T12:27:41Z

Why? Did you set up the hostname correctly?

tests/common.py

webknjaz · 2018-10-12T12:46:27Z

tests/test-throttle.py

-        await client.connect("127.0.0.1", PORT)
-        await client.login()
-        await client.download(
+        _client = aioftp.Client(loop=loop, ssl=client.ssl)


There's no point/value in underscoring bunch of vars.

so how would you like to call a variable?

keep it as is, I didn't realize that it's an "extended boolean"

Oh, that was not about ssl

certificates/Certificate.crt

aioftp/server.py

aioftp/client.py

webknjaz · 2018-10-12T12:49:48Z

@Alxpy

ssl_context = ...
...
...
ca = trustme.CA()
ca.issue_server_cert(u'127.0.0.1')
ca.configure_trust(ssl_context)

oleksandr-kuzmenko · 2018-10-12T13:02:40Z

Yes, I use hostname correctly. And any tests work, but not all.
I got ssl.CertificateError: hostname '::1' doesn't match '127.0.0.1'

webknjaz · 2018-10-12T13:44:52Z

oh, that's easy

webknjaz · 2018-10-12T13:46:05Z

@Alxpy

ssl_context = ...
...
...
ca = trustme.CA()
ca.issue_server_cert(u'127.0.0.1', u'::1', u'localhost')  # list all incarnations of localhost
ca.configure_trust(ssl_context)

Ref: https://trustme.readthedocs.io/en/latest/#trustme.CA.issue_server_cert

oleksandr-kuzmenko · 2018-10-13T22:51:17Z

trustme works fine, thx

pohmelie · 2018-10-13T22:56:47Z

So... @webknjaz @rsichny @asvetlov you think this pr is ready to merge? lgtm. I'm pretty confused, that implicit ftps was so easy to implement.
Hats off to @Alxpy 🎩

webknjaz · 2018-10-13T23:02:07Z

tests/test-throttle.py

-        await client.connect("127.0.0.1", PORT)
-        await client.login()
-        await client.download(
+        _client = aioftp.Client(loop=loop, ssl=client.ssl)


Undo underscoring of client var. It's not related, nor needed.

webknjaz · 2018-10-13T23:06:12Z

@pohmelie FTPS seems to be just plain old FTP on top of TLS, nothing more. So stick SSL between TCP and the app and you're done. Simple and elegant.
SFTP would be a different story, though, as it works on top of SSH.

@Alxpy I insist that wholesale renaming of client to _client in tests is useless and not necessary. It enlarges the diff and brings no value to tests at all.

pohmelie · 2018-10-13T23:11:27Z

@webknjaz
I thought at first about explicit ftps: go to secure connection on unsecure command. That sounds pretty hard. Since there must be a method to go down to unsecure from secure. Of course, implicit ftps was pretty straight, but I did not mind, that it is so easy to implement.

oleksandr-kuzmenko · 2018-10-13T23:17:45Z

I insist ...

@webknjaz you can try rename _client to client and understand the reason for this change.

webknjaz · 2018-10-13T23:19:26Z

@Alxpy so tell me: what is that?

webknjaz · 2018-10-13T23:21:14Z

Oh, I see. You're messing with scopes...

pohmelie · 2018-10-15T14:10:08Z

@Alxpy Thank you for great contribution! 🎉

oleksandr-kuzmenko force-pushed the ssl branch from 9c83950 to 8908633 Compare October 9, 2018 16:14

oleksandr-kuzmenko mentioned this pull request Oct 9, 2018

Secure FTP #37

Open

2 tasks

rsichnyi reviewed Oct 11, 2018

View reviewed changes

aioftp/client.py Show resolved Hide resolved

oleksandr-kuzmenko force-pushed the ssl branch from 8908633 to 7a4d9f6 Compare October 11, 2018 09:48

Add FTPS support for client

a3f13b7

oleksandr-kuzmenko force-pushed the ssl branch from 7a4d9f6 to a3f13b7 Compare October 11, 2018 22:40

oleksandr-kuzmenko changed the title ~~Add FTPS support for client~~ Add implicit FTPS support Oct 11, 2018

oleksandr-kuzmenko force-pushed the ssl branch from 2ac57b2 to e3b5138 Compare October 11, 2018 23:01

webknjaz reviewed Oct 12, 2018

View reviewed changes

tests/common.py Outdated Show resolved Hide resolved

webknjaz reviewed Oct 12, 2018

View reviewed changes

certificates/Certificate.crt Outdated Show resolved Hide resolved

webknjaz reviewed Oct 12, 2018

View reviewed changes

aioftp/server.py Show resolved Hide resolved

webknjaz reviewed Oct 12, 2018

View reviewed changes

aioftp/client.py Show resolved Hide resolved

Add FTPS support for server

267db49

oleksandr-kuzmenko force-pushed the ssl branch from e3b5138 to 267db49 Compare October 13, 2018 22:32

webknjaz reviewed Oct 13, 2018

View reviewed changes

webknjaz approved these changes Oct 13, 2018

View reviewed changes

pohmelie merged commit 09958d1 into aio-libs:master Oct 15, 2018

saulcruz mentioned this pull request Apr 29, 2019

Does aioftp support FTPES (FTP server over explicit TLS/SSL) #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add implicit FTPS support #81

Add implicit FTPS support #81

oleksandr-kuzmenko commented Oct 9, 2018 •

edited

Loading

codecov bot commented Oct 9, 2018 •

edited

Loading

asvetlov commented Oct 9, 2018

oleksandr-kuzmenko commented Oct 9, 2018

pohmelie commented Oct 10, 2018

asvetlov commented Oct 10, 2018

oleksandr-kuzmenko commented Oct 10, 2018

rsichnyi commented Oct 11, 2018 •

edited

Loading

oleksandr-kuzmenko commented Oct 11, 2018

oleksandr-kuzmenko commented Oct 11, 2018

webknjaz commented Oct 12, 2018

oleksandr-kuzmenko commented Oct 12, 2018 •

edited

Loading

webknjaz commented Oct 12, 2018

webknjaz Oct 12, 2018

oleksandr-kuzmenko Oct 12, 2018

webknjaz Oct 12, 2018

webknjaz Oct 13, 2018

webknjaz commented Oct 12, 2018 •

edited

Loading

oleksandr-kuzmenko commented Oct 12, 2018

webknjaz commented Oct 12, 2018

webknjaz commented Oct 12, 2018

oleksandr-kuzmenko commented Oct 13, 2018

pohmelie commented Oct 13, 2018

webknjaz Oct 13, 2018

webknjaz commented Oct 13, 2018

pohmelie commented Oct 13, 2018

oleksandr-kuzmenko commented Oct 13, 2018

webknjaz commented Oct 13, 2018

webknjaz commented Oct 13, 2018

pohmelie commented Oct 15, 2018

Add implicit FTPS support #81

Add implicit FTPS support #81

Conversation

oleksandr-kuzmenko commented Oct 9, 2018 • edited Loading

codecov bot commented Oct 9, 2018 • edited Loading

Codecov Report

asvetlov commented Oct 9, 2018

oleksandr-kuzmenko commented Oct 9, 2018

pohmelie commented Oct 10, 2018

asvetlov commented Oct 10, 2018

oleksandr-kuzmenko commented Oct 10, 2018

rsichnyi commented Oct 11, 2018 • edited Loading

oleksandr-kuzmenko commented Oct 11, 2018

oleksandr-kuzmenko commented Oct 11, 2018

webknjaz commented Oct 12, 2018

oleksandr-kuzmenko commented Oct 12, 2018 • edited Loading

webknjaz commented Oct 12, 2018

webknjaz Oct 12, 2018

Choose a reason for hiding this comment

oleksandr-kuzmenko Oct 12, 2018

Choose a reason for hiding this comment

webknjaz Oct 12, 2018

Choose a reason for hiding this comment

webknjaz Oct 13, 2018

Choose a reason for hiding this comment

webknjaz commented Oct 12, 2018 • edited Loading

oleksandr-kuzmenko commented Oct 12, 2018

webknjaz commented Oct 12, 2018

webknjaz commented Oct 12, 2018

oleksandr-kuzmenko commented Oct 13, 2018

pohmelie commented Oct 13, 2018

webknjaz Oct 13, 2018

Choose a reason for hiding this comment

webknjaz commented Oct 13, 2018

pohmelie commented Oct 13, 2018

oleksandr-kuzmenko commented Oct 13, 2018

webknjaz commented Oct 13, 2018

webknjaz commented Oct 13, 2018

pohmelie commented Oct 15, 2018

oleksandr-kuzmenko commented Oct 9, 2018 •

edited

Loading

codecov bot commented Oct 9, 2018 •

edited

Loading

rsichnyi commented Oct 11, 2018 •

edited

Loading

oleksandr-kuzmenko commented Oct 12, 2018 •

edited

Loading

webknjaz commented Oct 12, 2018 •

edited

Loading