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

glob method && Implement FTPS implicit FTP over TLS #100

Open
wants to merge 2 commits into
base: master
from

Conversation

6 participants
@catalint
Copy link

catalint commented Aug 11, 2017

glob method

ported Glob method from filepath - https://golang.org/pkg/path/filepath/#Glob and also present in sftp client http://godoc.org/github.com/pkg/sftp#Client.Glob

Implement FTPS implicit FTP over TLS

This will allow the package to also be able to connect to FTPS on servers that have implicit tls enabled
added new method func DialImplicitTLS(addr string, config *tls.Config) (*ServerConn, error)
added new TLS server and ran tests over TLS also, all pass

increase test coverage

catalint added some commits Aug 11, 2017

@catalint catalint force-pushed the catalint:feature/glob branch from f5734bc to c64af70 Aug 11, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage increased (+2.3%) to 83.935% when pulling f5734bc on catalint:feature/glob into 769512c on jlaffaye:master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage increased (+2.3%) to 83.935% when pulling f5734bc on catalint:feature/glob into 769512c on jlaffaye:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage increased (+2.3%) to 83.935% when pulling f5734bc on catalint:feature/glob into 769512c on jlaffaye:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage increased (+2.3%) to 83.935% when pulling c64af70 on catalint:feature/glob into 769512c on jlaffaye:master.

@catalint

This comment has been minimized.

Copy link

catalint commented Aug 11, 2017

travis failed because one of the tests error-ed with

Bad response status from coveralls: 422
{"message":"Couldn't find a repository matching this job.","error":true}

this pull request is good for you to look at
thanks

@corny

This comment has been minimized.

Copy link
Contributor

corny commented Aug 11, 2017

Please do two separate pull requests - one for each feature.

@catalint

This comment has been minimized.

Copy link

catalint commented Aug 12, 2017

The commits are different, do you really need 2 pull requests?

@corny

This comment has been minimized.

Copy link
Contributor

corny commented Aug 12, 2017

Yes please. That makes it easier for reviewing. I don't think your Glob-function can be merged in the current state. What are the license requirements for copying code from the Go standard library?

@tomtitchener
Copy link

tomtitchener left a comment

Minor nit-pick about ports, 21 vs. 990


func getConnection() (*ServerConn, error) {
if isTLSServer() {
return DialImplicitTLS("localhost:21", &tls.Config{InsecureSkipVerify: true})

This comment has been minimized.

@tomtitchener

tomtitchener Aug 14, 2017

Why is that port 21 and not 990?

@ncw

This comment has been minimized.

Copy link
Contributor

ncw commented Jul 15, 2018

Is this going to be merged?

I have rclone users asking for FTP with SSL/TLS support and as far as I can see it is all here.

Perhaps the maintainers could pull the PR locally, drop the glob part which seems to be controversial, fixup and merge the TLS feature?

Or alternatively I'll do it in a new PR if you want?

@corny

This comment has been minimized.

Copy link
Contributor

corny commented Jul 28, 2018

I checked out your commits and I read the specification of implicit TLS for FTP. Your changes secure the control channel but not the data channel. Are there seriously people still preffering FTP/S to SCP and SFTP?

I don't think it worths the effort:
https://tools.ietf.org/html/rfc4217#section-9

@ncw

This comment has been minimized.

Copy link
Contributor

ncw commented Jul 30, 2018

@corny
Thanks for looking into this.

Your changes secure the control channel but not the data channel.

I believe it is possible to secure the data channel too. Securing the control channel at least secures the credentials which are otherwise sent in the clear! I agree that securing the data channel too would be extremely desireable.

Are there seriously people still preffering FTP/S to SCP and SFTP?

Not by choice I imagine, but there are an awful lot of legacy systems out there!

@garymoon

This comment has been minimized.

Copy link

garymoon commented Aug 8, 2018

As an example of not-by-choice, Azure (also not by choice) supports FTP access to their "Azure Websites" feature. It (horrifyingly) supports plain FTP, but also supports FTPS/FTPES which I would much prefer to use.

Thanks so much for working on support for this everyone!

P.S. I found this thread via the rclone forums, it would be awesome to have FTPS/FTPES support in there. Really cool of you to be following up personally also @ncw.

@catalint

This comment has been minimized.

Copy link

catalint commented Aug 9, 2018

as this pull request was generating some comments in my mail, it reminded me that I have some unfinished work here

I've created #121 for ftps changes only

hope it helps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment