search

jenkinsci / publish-over-ftp-plugin

https://plugins.jenkins.io/publish-over-ftp/
21 stars 33 forks source link

[JENKINS-37259] Client uses LIST output rather than using MLSD command. #63

Closed jira-importer closed 6 years ago

jira-importer commented 8 years ago

The output of the LIST command is not defined, so parsing the output is error-prone.

The MLSD (and MLST) commands are defined in RFC 3659, provides a "machine readable" directory listing, with a well-defined format. Conformant servers advertise that they support the command (via FEAT), allowing clients to use MLSD command if the server supports it. Apache commons ftp client also supports the mlsd command, albeit via explicit commands.

The suggested improvement is that the publish-over-ftp uses MLSD command on servers that support it.

Originally reported by iepe1eet, imported from: Client uses LIST output rather than using MLSD command.
  • assignee: slide_o_mix
  • status: Resolved
  • priority: Minor
  • resolution: Fixed
  • resolved: 2017-12-21T01:37:12+11:00
  • imported: 2022/01/10
jira-importer commented 6 years ago

slide_o_mix:

I just took this plugin over, can you give me a little more details on where you see this issue?

jira-importer commented 6 years ago

iepe1eet:

Hi Alex,

Sorry, it's a while ago now and I don't remember all the details – looking back, I see this wasn't a very good report of the problem :-/

I remember investigating a problem where, looking into it, I discovered that this plugin was using the LIST parser.  The only place this happens is when doing a recursive delete (deleteTree), so I imagine this report is from a failure to do a recursive delete (the "Clean remote" option).  I have vague memories of some cryptic error message, but unfortunately I didn't record that in this ticket ... sorry.

We are using Jenkins and Publish-over-FTP to build and upload packages of our storage software, which supports the FTP protocol.  The upload targets a production instance of our software, so I imagine the problem stems from our LIST implementation output being slightly different from what Apache-commons FTPClient is expecting.

Given the LIST output format is completely unspecified, and there is a standard approach (MLSD) that Apache-commons FTPClient supports, it would seem reasonable for the Publish-over-FTP plugin to use MLSD if the server supports it, and fall back to LIST for servers that don't.

Cheers,

Paul.

jira-importer commented 6 years ago

slide_o_mix:

Hi Paul,

Thanks for the update, that at least gives me a spot to start looking. I'll see what I can figure out.

Thanks,

Alex

jira-importer commented 6 years ago

slide_o_mix:

Do you have a way of testing this if I add support? I don't have a way myself.

jira-importer commented 6 years ago

scm_issue_link:

Code changed in jenkins
User: Alex Earl
Path:
Jenkinsfile
pom.xml
src/main/java/jenkins/plugins/publish_over_ftp/BapFtpClient.java
src/main/resources/jenkins/plugins/publish_over_ftp/BapFtpPublisherPlugin/help-paramPublish.jelly
src/test/java/jenkins/plugins/publish_over_ftp/BapFtpClientTest.java
http://jenkins-ci.org/commit/publish-over-ftp-plugin/73a92a296e76539a3ecbc255b80eb2bccad0a653
Log:
Fix JENKINS-37259

jira-importer commented 6 years ago

iepe1eet:

I have a test instance of our software specifically for testing client compatibility: prometheus.desy.de.  I can easily create an account for you.  just run the command:

htpasswd -n -m | sed 's/\$/\\\$/g'

replacing with your preferred username.

Send me the output to my email address .

Choose any password you like; however, I'd recommend not reuse any password you care about, as the FTP transfer is unencrypted, so the password will travel plain-text over the internet.

jira-importer commented 6 years ago

slide_o_mix:

It doesn't look like your server shows MLSD feature, only MLST. Should I look for the MLST feature or MLSD feature?

jira-importer commented 6 years ago

iepe1eet:

Have a look at RFC 3659 section 7.8:

When responding to the FEAT command, a server-FTP process that
supports MLST, and MLSD, plus internationalization of pathnames, MUST
indicate that this support exists. It does this by including a MLST
feature line.

The MLST feature implies support for both the MLST and MLSD commands.

The patch should also cares about whether or not the entry is a directory; i.e., the TYPE fact.  Theoretically, a server doesn't have to provide this information — a client should check the FEAT response to discover if it is enabled and use the OPT command to request the information, if available but not enabled.  I would imagine some of this the Apache code already does.

jira-importer commented 6 years ago

slide_o_mix:

Thanks, I missed that part in the RFC. I am using ftpClient.hasFeature("MLST") and then using the ftpClient.mlistDir() method to iterate. 

jira-importer commented 6 years ago

iepe1eet:

Incidentally, a throw-away comment in the FTPClient#listFiles([String)|http://download.oracle.com/javase/1.6.0/docs/api/java/lang/String.html?is-external=true] JavaDoc entry suggests the Apache Commons FTP client automatically chooses between LIST and MLSD.  If so, then perhaps that would be preferrable to testing.for MLST features explicitly.

jira-importer commented 6 years ago

slide_o_mix:

Will be fixed in 1.13