[net] How to implement FTPS extending FTPClient, from a diferente package...

• Key: NET-57
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Jose Juan Montiel
• Affects Version/s: 1.4

Environment

Operating System: All
Platform: All

Description

Hi everybody, I'm Jose from Spain.

I make an implement of FTPS: using
http://sourceforge.net/projects/ufsc implementation (which use a new
class, created by UFSC, org.apache.commons.net.ftp.FtpsClient that
extends org.apache.commons.net.ftp.FTPClient), with some minor
modification to adapt Java 1.3 and solve some fix with PASV transfer
(modification and fix, that i comunicate to the author).

I try to build FtpsClient under diferent packege, then i found that
couldn't do it because, in org.apache.commons.net.ftp.FTP the
variables

BufferedReader _controlInput;
BufferedWriter _controlOutput;

were declare with packege visibility, and FtpsClient use this, to
implement securety
connection to SSLSocket. Something like this:

this._controlInput = new BufferedReader(new
InputStreamReader(socket.getInputStream(), getControlEncoding()));
this._controlOutput = new BufferedWriter(new
OutputStreamWriter(socket.getOutputStream(), getControlEncoding()));

Because of this, FtpsClient, in UFSC, is under org.apache.commons.net.ftp.

Then the solution I adopt, was copy (and minor modify) FTPClient and
FTP from org.apache.commons.net.ftp in my own package, and extends
FtpsClient, from my own FTPClient, to make it in a difetent pakage...

And my suggestion is: It could be possible, for future version, declare
protected, for simplify the extension of api, to implement FTPS, or other future
protocol... in diferent package...?

as well, could by a setter, for this variables, to assing then the socket stream...

Thanks to all.

Comments

[Steve Cohen]

What do you think of this, Daniel? Is there a good reason NOT to make these
data members protected or provide setters/getters for them?

An alternative idea, if Jose is willing, might be for Jose to make local
modifications and then, once he's gotten his FTPSClient working, submit it as a
patch to be added to commons-net.

[Jose Juan Montiel]

Created an attachment (id=17460)
Patch to solve the problem of extension FTP to other protocol...

This is the patch that make the variables, protected, this is the only thing i
need, to extends FTPClient to FTPSClient, in other package...

[Jose Juan Montiel]

(In reply to comment #1)
> What do you think of this, Daniel? Is there a good reason NOT to make these
> data members protected or provide setters/getters for them?
>
> An alternative idea, if Jose is willing, might be for Jose to make local
> modifications and then, once he's gotten his FTPSClient working, submit it as a
> patch to be added to commons-net.

I submit the patch.
Thanks for your time and attention.

[Steve Cohen]

Jose, you misunderstood me. I was asking if you would be willing to submit your
FTPSClient as a patch. It would have been a nice extension to commons-net if it
worked out.

But I should have read your submission more carefully. I didn't realize that
UFSC was another open source project. I don't know what the licensing issues
would be, but it's likely to be a nightmare.

So, back to you, Daniel. Do you have any objections to Jose's patch.

[Daniel Savarese]

I made the change, but added documentation.

[Steve Cohen]

(In reply to comment #5)
> I made the change, but added documentation.

I read your documentation but it raises a question. Are you meaning to suggest
that the RIGHT way to do this is to overwrite connectAction()?

[Jose Juan Montiel]

(In reply to comment #6)
> (In reply to comment #5)
> > I made the change, but added documentation.
>
> I read your documentation but it raises a question. Are you meaning to suggest
> that the RIGHT way to do this is to overwrite connectAction()?
>

(In reply to comment #6)
> (In reply to comment #5)
> > I made the change, but added documentation.
>
> I read your documentation but it raises a question. Are you meaning to suggest
> that the RIGHT way to do this is to overwrite connectAction()?
>

Hi, i write to Paul Ferraro, the developer of UFSC and suggest that he could
change its licence from LGPL to APACHE, in this way i could apply the patch,
with my modifications of his code. I'm waiting his anwers.

FTPS its very similar to FTP, the most significant diferences are:
javax.net.ssl.SSLSocket, certificates, and after connect sends AUTH SSL, that
negotiates the secure method connection, and handshake... his implementation,
don't overwrite, only use the variables, to assing them "ssl streams" of the SSl
socket.

I'll tell you, when he responds me.

[Steve Cohen]

(In reply to comment #7)
my modifications of his code. I'm waiting his anwers.
>
> FTPS its very similar to FTP, the most significant diferences are:
> javax.net.ssl.SSLSocket, certificates, and after connect sends AUTH SSL, that
> negotiates the secure method connection, and handshake... his implementation,
> don't overwrite, only use the variables, to assing them "ssl streams" of the SSl
> socket.
>
> I'll tell you, when he responds me.

So if you didn't need to change these variables, all you really needed was a
getter method. Daniel, maybe that would be a better solution? I don't imagine
you're too comfortable with exposing stuff that begins and ends with underscores?

Jose, it would be great if we could bring FTPS into commons-net.

[Daniel Savarese]

(In reply to comment #6)
> I read your documentation but it raises a question. Are you meaning to suggest
> that the RIGHT way to do this is to overwrite connectAction()?

I was only trying to indicate where and how the variable was assigned in the
FTP class so that there would be no surprises to someone using the protected
variables.

[Daniel Savarese]

(In reply to comment #8)
> So if you didn't need to change these variables, all you really needed was a
> getter method. Daniel, maybe that would be a better solution? I don't imagine
> you're too comfortable with exposing stuff that begins and ends with underscores?

Since the underlying SocketClient stream members are already protected,
I couldn't see how making the FTP reader wrappers protected would be
dangerous. Until now, there hadn't been a need to expose that bit outside
of the package. As far as the underscores go, I was just trying to remain
consistent with the variable naming convention originally used in that code.
For better or for worse, protected members were distinguished by leading and
trailing underscores.

At any rate, I don't have any strong feelings about the matter. In general,
if someone using the library needs to specialize its behavior through
inheritance, I figure it's evidence the need should be accommodated as long
as there isn't some other way of meeting the need (e.g., aggregation).

[Jose Juan Montiel]

Created an attachment (id=17483)
Patch that make __fileType protected in FTPClient.

The implements of FTPSClient, need that __fileType be protected, because, need
to overwrite retrieveFile, and in this method, depend on filetype, its recieve
in adecuate way.

[Jose Juan Montiel]

Created an attachment (id=17484)
Implementation of FTPSClient.

Paul Ferraro change the license of his proyecto to Apache License V2.0. Then I
submit the 3 class that implements FTPSClient, and fourth class, ftps, that use
this implementation.

[Steve Cohen]

(In reply to comment #12)
> Created an attachment (id=17484) [edit]
> Implementation of FTPSClient.
>
> Paul Ferraro change the license of his proyecto to Apache License V2.0. Then I
> submit the 3 class that implements FTPSClient, and fourth class, ftps, that use
> this implementation.

Thank you, Jose and Paul. I have a few questions about the submission.

1. You are importing com.sun.net.ssl.X509TrustManagern and
com.sun.net.ssl.SSLContext. Are these allowable imports under the terms of the
Apache License? I don't know the answer, it is a question. We have been
recently hit with licensing questions and I don't want to go through this again
without checking first.

2. Does this import require importing any additional libraries, or is this
class included in a J2SDK library. If so, which version?

3. FTPSClient overrides the retrieveFile() method from FTPClient. I am not an
expert in FTPS but shouldn't similar overrides be provided for storeFile(),
deleteFile()? How about listFiles() and listNames()? Can you point us to a
definition of the FTPS protocol that defines what is required?

4. Assuming all the above are answered, we would like some JUnit tests to be
created for the new classes. The present tests can serve as a model for you.

5. Finally, the files need to have the Apache License.

Please do not take this the wrong way. I am being very demanding and legalistic
here, because I know that the Jakarta project is being closely watched from some
quarters, but I very much want this to go forward if at all possible.

I also think that this is not the place for this discussion to be continued.
Now that we are on to legal as well as coding issues, the discussion should move
back to the commons-dev mailing list, which is where I am moving it.

[Satoshi Ishigami]

I checked his patches and read RFC2228, RFC4217 and some internet web sites.
It would be great if following things are done for the future work of FTPS.

1) implicit mode.
There is the two mode of FTPS to connect it securely. (implicit/explicit)
It is implemented only the explicit mode.

2) It can not specify multiple keystores and/or trustmanagers.
I want to use multiple keystores and/or trustmanagers.

3) The password to access KeyStore can not change.
I think that this lacks security.

4) It can not change the data connection security level(PROT command).

5) It can not change the protected data bufferes(PBSZ command).

6) The X509TrustManager should be made by implements
javax.net.ssl.X509TrustManager. But, X509TrustManager interface is a part
of JSSE, so there are not included in JDK1.3. The JSSE was introduced
since JDK1.4 by default.

(In reply to comment #8)
> (In reply to comment #7)
> my modifications of his code. I'm waiting his anwers.
> >
> > FTPS its very similar to FTP, the most significant diferences are:
> > javax.net.ssl.SSLSocket, certificates, and after connect sends AUTH SSL, that
> > negotiates the secure method connection, and handshake... his implementation,
> > don't overwrite, only use the variables, to assing them "ssl streams" of the SSl
> > socket.
> >
> > I'll tell you, when he responds me.
>
> So if you didn't need to change these variables, all you really needed was a
> getter method. Daniel, maybe that would be a better solution? I don't imagine
> you're too comfortable with exposing stuff that begins and ends with underscores?
>
> Jose, it would be great if we could bring FTPS into commons-net.
>
>

[Jose Juan Montiel]

Created an attachment (id=17488)
Implementation of FTPSClient.

Added the commented modification...

[Jose Juan Montiel]

(In reply to comment #13)
> 1. You are importing com.sun.net.ssl.X509TrustManagern and
> com.sun.net.ssl.SSLContext. Are these allowable imports under the terms of the
> Apache License? I don't know the answer, it is a question. We have been
> recently hit with licensing questions and I don't want to go through this again
> without checking first.

I really don't know about legal, this import its into JSSE
(http://java.sun.com/products/jsse/LICENSE.html), because i want use ftps, with
JDK 1.3, but... there is a correspondent Java 1.4 classes for com.sun.net...

> 2. Does this import require importing any additional libraries, or is this
> class included in a J2SDK library. If so, which version?

This import require Java 1.3 & JSSE.

> 3. FTPSClient overrides the retrieveFile() method from FTPClient. I am not an
> expert in FTPS but shouldn't similar overrides be provided for storeFile(),
> deleteFile()? How about listFiles() and listNames()? Can you point us to a
> definition of the FTPS protocol that defines what is required?

Now, following the paul advice, now only overwrite openDataConnection

> 4. Assuming all the above are answered, we would like some JUnit tests to be
> created for the new classes. The present tests can serve as a model for you.

I attach one junit, to test, with a "local ftps server" of the "junit machine".

> 5. Finally, the files need to have the Apache License.

Add.

> Please do not take this the wrong way. I am being very demanding and legalistic
> here, because I know that the Jakarta project is being closely watched from some
> quarters, but I very much want this to go forward if at all possible.
>
> I also think that this is not the place for this discussion to be continued.
> Now that we are on to legal as well as coding issues, the discussion should move
> back to the commons-dev mailing list, which is where I am moving it.

Don't worry... and thanks for all.

[Jose Juan Montiel]

(In reply to comment #14)
> I checked his patches and read RFC2228, RFC4217 and some internet web sites.
> It would be great if following things are done for the future work of FTPS.
>
> 1) implicit mode.
> There is the two mode of FTPS to connect it securely. (implicit/explicit)
> It is implemented only the explicit mode.

For future work...

> 2) It can not specify multiple keystores and/or trustmanagers.
> I want to use multiple keystores and/or trustmanagers.

It was implemented in code of 2006-01-22 19:14.

> 3) The password to access KeyStore can not change.
> I think that this lacks security.

Implemented now.

> 4) It can not change the data connection security level(PROT command).

Implemented now.

> 5) It can not change the protected data bufferes(PBSZ command).

Implemented now.

> 6) The X509TrustManager should be made by implements
> javax.net.ssl.X509TrustManager. But, X509TrustManager interface is a part
> of JSSE, so there are not included in JDK1.3. The JSSE was introduced
> since JDK1.4 by default.

I need that ftps works with JDK1.3, and dont mind import JSSE...

Thanks for all your comment.

[Steve Cohen]

TO: EVERYONE

PLEASE LET'S NOT HAVE ANY MORE DISCUSSIONS RELATED TO THE POSSIBLE ADDITION OF
FTPS FUNCTIONALITY TO COMMONS-NET AS COMMENTS ON THIS BUG.

This bug is now closed. Comments related to the lively discussion of adding
FTPS to commons-net should be made on the commons-dev mailing list:
[e-mail].