#9645 Implement FTP extensions for IPv6#1149
Conversation
These are defined in RFC 2428. The facilities in the RFC for requesting the use of a specific network protocol don't fit very well into Twisted's view of the world, but I've at least arranged for the command arguments to be validated appropriately for the common cases of IPv4 and IPv6. Tests for IPv6 support are only partial, because FTPClient doesn't yet support this (see https://twistedmatrix.com/trac/ticket/9085).
|
There are some remaining lint errors, but they're essentially all due to the justification of "=" characters in constant assignments near the top of the file, and mostly pre-existing except where I made my changes fit the surrounding code. I think it would make more sense to waive these rather than clutter this PR with them, but if reviewers require it then I can do the reformatting. |
When receiving a connection to an IPv6 endpoint using an IPv4-mapped address (possible on some IPv6 stacks), PASV can work but needs to return a properly-encoded IPv4 address. Including the leading "::ffff:" in the response appears to confuse connection tracking in some firewalls.
453d42d to
3787fe8
Compare
glyph
left a comment
There was a problem hiding this comment.
The changes requested are pretty minor; for the most part this is a great change.
Regarding the lint thing: when I'm looking for reviews to do, I tend to hit the ones with green checkmarks first, so I'd probably go ahead and make the style inconsistent to appease the mechanical checks. It's not a dealbreaker, but it'll make it easier and quicker to complete the review. Maybe we should do a lint-ratchet reduction PR to reformat the rest of the surrounding stuff but let's not bundle those together.
adiroiban
left a comment
There was a problem hiding this comment.
I didn't had time to finalize the review...but I am sending my current comments.
src/twisted/protocols/ftp.py
Outdated
| transport addresses. | ||
| """ | ||
| if self.epsvAll: | ||
| return defer.fail(BadCmdSequenceError( |
There was a problem hiding this comment.
I think that is ok to return a failure here...
What I did, is create a standard reponse code like
RESPONSE[EPSV_ALL_ACTIVE] = '503 EPSV ALL was enabled.'
and then just do
if self._epsvAllActive:
return (EPSV_ALL_ACTIVE,)
There was a problem hiding this comment.
I suppose I could have done, but I was following the existing pattern set by ftp_PASV and ftp_PORT, and it didn't really seem worth lifting this up to the top level when each message is only used in one place.
|
I filed #1190 to clean up lint, and will return to this once that's merged. |
It's only used by tests, so moving it saves having to worry about coverage for it.
|
@glyph @adiroiban Sorry for taking nearly two years to get round to dealing with your review comments! I think I've dealt with pretty much everything now, apart from the |
|
No need to apologize — if anything, managing to finally come back after 2 years rather than just dropping it forever is praiseworthy :). |
Another attempt to pacify coverage checks.
adiroiban
left a comment
There was a problem hiding this comment.
Looks good. Thanks.
I think that this can be merged as it's already a big improvement over trunk.
for my project I am using a highly modified version of twisted FTP protocol with support for py2 and Implicit/Explict FTPS, x509 based authentication, and machine listing and a few other extensions . client and server side.
if such functionality is needed in twisted and someone has time to review the changes, let me know and I can look at sending them for review
|
@glyph can we merge this? Do you still think that this needs changes? Thanks! |
for more information, see https://pre-commit.ci
|
In the future, you can always feel free to dismiss my reviews if you've reviewed the points that I raised and they've been addressed. |
Fixes #9645
These are defined in RFC 2428.
The facilities in the RFC for requesting the use of a specific network
protocol don't fit very well into Twisted's view of the world, but I've
at least arranged for the command arguments to be validated
appropriately for the common cases of IPv4 and IPv6.
Tests for IPv6 support are only partial, because FTPClient doesn't yet
support this (see https://twistedmatrix.com/trac/ticket/9085).