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

Implement FTPS implicit FTP over TLS #121

Open
wants to merge 5 commits into
base: master
from

Conversation

6 participants
@catalint
Copy link

catalint commented Aug 9, 2018

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

catalint added some commits Aug 11, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 9, 2018

Coverage Status

Coverage increased (+0.8%) to 82.272% when pulling 2a369df on catalint:feature/ftps into 3f6433f on jlaffaye:master.

@catalint

This comment has been minimized.

Copy link

catalint commented Aug 9, 2018

hi all, fixed all Travis warnings, this is ready to review, nothing special changed just splitted the work I did on #100 , hope it helps

@ncw
Copy link
Contributor

ncw left a comment

(not a project member just an interested party)

I had a look through the code and it looks good to me. Most importantly is is low impact - nothing changes to existing users if they don't call DialImplicitTLS.

The bulk of the changes are setting up the tests which look well done to me.

As noted earlier this only protects the control channel - it would be desirable to protect the data channels too, but the control channel is the most important one.

@jlaffaye

This comment has been minimized.

Copy link
Owner

jlaffaye commented Aug 13, 2018

Hello, thanks for working on this.
I want to rework the Dial* methods so we can pass options without breaking the API each time we add an option. For instance, how do you pass the timeout in your DialImplicitTLS ?

So, long story short, can you unexport the DialImplicitTLS so I can merge your PR then expose it with my new constructor ?

@corny
Copy link
Contributor

corny left a comment

There are many ways of establishing an TLS connection:

  • tls.Dial()
  • tls.DialWithDialer()
  • tls.Client()
  • ...

I would like to see the library completely without TLS and have the DialServer method exported instead. Additional tests with TLS are fine for me.

Please check out my changes: catalint/ftp@feature/ftps...digineo:ftps

@Kegsay

This comment has been minimized.

Copy link

Kegsay commented Sep 17, 2018

Is there a public API you have in mind @jlaffaye ? I'd like to see this merged so if I can help out please let me know the desired API shape.

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