several fixes and features (rfc3659) #19

muirmanders · 2016-09-20T04:01:39Z

Still not sure why why need -k. What are you developing on that doesn't have a CA bundle?

I do have the CA bundle... but from experience I've also ended up working with servers that do not have it or it is outdated (not my choice). If you think it is unsafe I can remove it as it is not really need, I just added it for compatibility.

I think we should remove it.

muirmanders · 2016-09-20T04:12:27Z

I didn't include this because it isn't compatible with the connection pool (would end up with connections in different directories). I considered making it a client-wide setting that gets applied whenever you get a connection (i.e. switches to the desired directory), but that could get complicated, too.

I know(and there is a comment on the commit with this :) ) but it seems it is used as a workaround by ftp clients when LIST or MLSD fails with path argument. I ran into the issue while testing and using CWD first before LIST (without path) worked.

I was considering the following:

renaming all functions that call getIdleConn() to *Ex() and replace them with a wrapper.

the Ex() versions would get the idx of the pconn as a param

the Ex() versions would call getIdleConn() only if idx was -1 otherwise use the idx pconn / fail.

SetWd() returns the pconn idx

some locking might be needed if the user starts several go routines using the same handle

as for me I really need this but I could move it to a separate branch if you do not

What if each connection remembers what dir it is in, and when a new connection is gotten it automatically does the cwd if it isn't in the correct dir (user's desired dir is stored on client object when they set via setwd())?

muirmanders · 2016-09-20T04:16:29Z

Can this commandNotSupported check be moved up directly after the dataStringList() call? It's a little weird having the nested checks if mlsdNotSupported.

right... missed that, I'll commit a fix

muirmanders · 2016-09-20T04:20:30Z

Do you have examples of incomplete MLSD/LIST output? Is it safe to give the user back incomplete results (i.e. maybe we just failed parsing)?

"............" is text utf8, this is a wireshark dump

220 Serv-U FTP Server v10.4 ready...
....
Type=dir;Modify=20160910211543.432;Perm=el; music
Type=dir;Modify=20160917224516.936;Perm=el; ............
....
Type=dir;Modify=20151128035821.241;Perm=el; video
Type=dir;Perm=el; ..........
Type=dir;Modify=20160911053801.535;Perm=el; www
...

"Type=dir;Perm=el; .........." is a directory that is no longer accesible and the server does not send any other information about it but we can still use the received list

muirmanders · 2016-09-20T04:22:03Z

Why did you change these methods to take the pconn? Was it to avoid calling getIdleConn() multiple times?

Mostly because I needed to access pconn.mlsdNotSupported in ReadDir for the connection.
Once I did getIdleConn() in ReadDir() I should also pass it to the underlying functions.

muirmanders · 2016-09-20T04:23:51Z

This error should probably be checked (oversight in existing code).

muirmanders · 2016-09-20T04:48:47Z

Should this be 15:04 for parsing of 24hr clock value without am/pm?

muirmanders · 2016-09-20T04:53:00Z

What server returns this format?

220 Microsoft FTP Service
...
STAT
211-Microsoft FTP Service status:
Logged in user: anonymous
TYPE: ASCII; FORM: NONPRINT; STRUcture: FILE; transfer MODE: STREAM
Data connection: none
211 End of status.
SYST
215 Windows_NT`

LIST output looks like this:
02-10-16 12:10PM 8067784 OneDriveSetup.exe
03-03-16 05:52PM
OS
06-20-16 03:43PM 164559 print.pdf

the main issue is the date format so I tried to support the most common formats I have found

muirmanders · 2016-09-20T04:47:41Z

Is ParseUint going to work on sizes with a comma like in your example above?

No, it will fail with err (size will be 0) but I have not found a single server that returns commas yet. Only running "dir" in local "command prompt" does that.

muirmanders · 2016-09-20T04:51:42Z

Is this check needed? Are you certain time strings can never be shorter than the formats (eg. "2006-01-02 2:04" is shorter than "2006-01-02 15:04")?

I needed a way to speed up processing large lists (thousands of files) and avoid false matches.
I have not yet found a server that doesn't pad with zero's but it might be possible.

muirmanders · 2016-09-20T04:27:18Z

Can you add tests for this additional parsing? Maybe some unit tests similar to TestParseMLST() ?

Ok, I will look into it.

muirmanders · 2016-09-20T04:29:34Z

Can you add a test case?

muirmanders · 2016-09-20T04:30:03Z

Can you add a test case?

No sane server should return "." or ".." as type "dir"...
I noticed this as my walk function entered a infinite loop, tcpdump helped identify the problem.

How would you test for this?

Unit test for parseList is fine.

muirmanders · 2016-09-20T04:35:57Z

What case is this handling? If this is for reading the final control response after a failed data transfer, it seems like the transferFromOffset() function should be checking for this error and reading the response (i.e. if prepareDataConn() fails with replyConnectionClosed, then change it so it still does a pconn.readResponse() to read the pending response). Or alternatively, just set pconn.broken = true if we are not confident we know the state of the connection and/or the connection is safe to continue using (next command will open a fresh connection).

This was hard to reproduce as it happened randomly on a high latency server (probably because of some packet loss or timeout...). Looks like a bug in the server as it should not send a 426 code as a preliminary reply. In my tests it happened while walking with ReadDir() .

It could be handled in dataStringList & transferFromOffset if I had more data but this seems like a more generic fix.
I have found no way of checking the socket buffer for data in go in a non-blocking manner (timeout is not a real option as it does not handle sub-second timeouts in my tests). Also does bufio Reader treat the connection break as EOF or returns an error?

I think it is the user of the data connection's responsibility to read the extra control connection response (even in the case of data connection being closed early). I think a sign this doesn't belong here is that it is basically duplicates all of the readResponse() method itself.

muirmanders · 2016-09-20T04:36:35Z

What is the purpose of the CLNT command?

According to the RFCs - it has no real purpose, just telling the server what client you are using.
But as I was testing I found a few servers that would not allow "OPTS UTF8 ON" unless CLNT is issued before...

For example on some version of CGFTP:
OPTS UTF8 ON
500 Send 'CLNT client_type' before enabling UTF8

muirmanders · 2016-09-20T04:06:17Z

Specifying UTF8 when available makes sense, but for my own edification, when is this required? Are there servers that won't send UTF-8 path names without this command?

I had an issue with several servers that treat UTF8 in client commands (like path in LIST / MLSD) as ASCII, thus replying with "not found", although they sent proper UTF-8 paths in replies

Again this is a feature not required in RFCs as UTF8 should be enabled by default if listed in FEAT but needed to work around broken servers (and there are quite a few of them).

muirmanders · 2016-09-20T04:40:56Z

Should this be UNIX.mode or does case not matter?

from RFC 3659:
"Fact names are case-insensitive. Size, size, SIZE, and SiZe are the same fact."

I've made some protocol dumps from a few ftp clients and they seem to use lower case so I kept it as such, though the RFC continues with:
"Further operating system specific keywords could be specified by
using the IANA operating system name as a prefix (examples only):

OS/2.ea -- OS/2 extended attributes MACOS.rf -- MacIntosh resource forks UNIX.mode -- Unix file modes (permissions)"

muirmanders · 2016-09-20T04:36:44Z

Should we check the response message to see if server supports requested options?

I thought of it but it seems like it would just add complexity. If the commands fail the client would still behave as before.

OPTS UTF8 ON is optional and may fail but the server could still use UTF8 according to FEAT reply.

-Original file line number
+Diff line change
@@ Expand Up @@
     ### Tests ###
     How to run tests (windows not supported):
+    * do not use the "root" account, use an unprivileged user with write access to the root goftp directory
     * ```./build_test_server.sh``` from root goftp directory (this downloads and compiles pure-ftpd and proftpd)
     * ```go test``` from the root goftp directory

-Original file line number
+Diff line change
@@ Expand Up / @@ -84,7 +84,7 @@ VpOorURz8ETlfAA= @@
     -----END CERTIFICATE-----
     CERT
-    curl -O http://download.pureftpd.org/pub/pure-ftpd/releases/obsolete/pure-ftpd-1.0.36.tar.gz
+    curl -k -O https://download.pureftpd.org/pub/pure-ftpd/releases/obsolete/pure-ftpd-1.0.36.tar.gz
     tar -xzf pure-ftpd-1.0.36.tar.gz
     cd pure-ftpd-1.0.36
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     		goto Error
     	}
+    	if pconn.hasFeature("CLNT") {
+        		if err = pconn.setClient(); err != nil {
+        			goto Error
+        		}
+    	}
+            if pconn.hasFeature("UTF8") {
+                    if err = pconn.setUnicode(); err != nil {
+    			goto Error
+                    }
+            }
+            if pconn.hasFeature("MLST") {
+                    if err = pconn.setMLST(); err != nil {
+                            goto Error
+                    }
+            }
     	c.mu.Lock()
     	defer c.mu.Unlock()
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -63,6 +63,9 @@ type persistentConn struct { @@
     	// remember EPSV support
     	epsvNotSupported bool
+    	// remember MLSD support
+    	mlsdNotSupported bool
     	// tracks the current type (e.g. ASCII/Image) of connection
     	currentType string
@@ Expand Down Expand Up @@
     			err:       fmt.Errorf("error reading response: %s", err),
     			temporary: true,
     		}
+    	} else if code == replyConnectionClosed {
+    		pconn.controlConn.SetReadDeadline(time.Now().Add(pconn.config.Timeout))
+    		code, msg, err = pconn.reader.ReadResponse(0)
+    		if err != nil {
+    			pconn.broken = true
+    			pconn.debug("error reading second response: %s", err)
+    			err = ftpError{
+    				err:       fmt.Errorf("error reading response: %s", err),
+    				temporary: true,
+    			}
+    		}
     	}
     	return code, msg, err
     }
@@ Expand Down Expand Up / @@ -205,7 +219,7 @@ func (pconn *persistentConn) fetchFeatures() error { @@
     	}
     	for _, line := range strings.Split(msg, "\n") {
-    		if line[0] == ' ' {
+    		if len(line) > 0 && line[0] == ' ' {
     			parts := strings.SplitN(strings.TrimSpace(line), " ", 2)
     			if len(parts) == 1 {
     				pconn.features[strings.ToUpper(parts[0])] = ""
@@ Expand All @@
     	return found && strings.ToUpper(arg) == val
     }
+    func (pconn *persistentConn) setClient() error {
+            code, msg, err := pconn.sendCommand("CLNT GoFTP")
+            if err != nil {
+                    return err
+            }
+            if !positiveCompletionReply(code) {
+                    pconn.debug("server doesn't support CLNT: %d-%s", code, msg)
+                    return nil
+            }
+            return nil
+    }
+    func (pconn *persistentConn) setUnicode() error {
+            code, msg, err := pconn.sendCommand("OPTS UTF8 ON")
+            if err != nil {
+                    return err
+            }
+            if !positiveCompletionReply(code) {
+                    pconn.debug("server doesn't support UTF8: %d-%s", code, msg)
+                    return nil
+            }
+            return nil
+    }
+    func (pconn *persistentConn) setMLST() error {
+            code, msg, err := pconn.sendCommand("OPTS MLST type;size;modify;perm;unix.mode;")
+            if err != nil {
+                    return err
+            }
+            if !positiveCompletionReply(code) {
+                    pconn.debug("server doesn't support UTF8: %d-%s", code, msg)
+                    return nil
+            }
+            return nil
+    }
     func (pconn *persistentConn) logIn() error {
     	if pconn.config.User == "" {
     		return nil
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -112,6 +112,27 @@ func (c *Client) Getwd() (string, error) { @@
     	return dir, nil
     }
+    // Setwd changes the current working directory.
+    func (c *Client) Setwd(path string) (error) {
+    	pconn, err := c.getIdleConn()
+    	if err != nil {
+    		return err
+    	}
+    	defer c.returnConn(pconn)
+    	code, msg, err := pconn.sendCommand("CWD %s", path)
+    	if err != nil {
+    		return err
+    	}
+    	if code != replyFileActionOkay {
+    		return ftpError{code: code, msg: msg}
+    	}
+    	return nil
+    }
     func commandNotSupporterdError(err error) bool {
     	respCode := err.(ftpError).Code()
     	return respCode == replyCommandSyntaxError || respCode == replyCommandNotImplemented
@@ Expand All / @@ -125,16 +146,27 @@ func commandNotSupporterdError(err error) bool { @@
     // be used. You may have to set ServerLocation in your config to get (more)
     // accurate ModTimes in this case.
     func (c *Client) ReadDir(path string) ([]os.FileInfo, error) {
-    	entries, err := c.dataStringList("MLSD %s", path)
+    	pconn, err := c.getIdleConn()
+    	if err != nil {
+    		return nil, err
+    	}
+    	defer c.returnConn(pconn)
+    	var entries []string
+    	if !pconn.mlsdNotSupported {
+    		entries, err = c.dataStringList(pconn, "MLSD %s", path)
+    	}
     	parser := parseMLST
-    	if err != nil {
-    		if !commandNotSupporterdError(err) {
+    	if pconn.mlsdNotSupported || err != nil {
+    		if !pconn.mlsdNotSupported && !commandNotSupporterdError(err) {
     			return nil, err
     		}
+    		pconn.mlsdNotSupported = true
-    		entries, err = c.dataStringList("LIST %s", path)
+    		entries, err = c.dataStringList(pconn, "LIST %s", path)
     		if err != nil {
     			return nil, err
     		}
@@ Expand All @@
     		info, err := parser(entry, true)
     		if err != nil {
     			c.debug("error in ReadDir: %s", err)
-    			return nil, err
+    			if !strings.Contains(err.Error(), "incomplete") {
+    				return nil, err
+    			}
     		}
     		if info == nil {
@@ Expand All @@
     // is a directory. You may have to set ServerLocation in your config to get
     // (more) accurate ModTimes when using "LIST".
     func (c *Client) Stat(path string) (os.FileInfo, error) {
-    	lines, err := c.controlStringList("MLST %s", path)
+    	pconn, err := c.getIdleConn()
+    	if err != nil {
+    		return nil, err
+    	}
+    	defer c.returnConn(pconn)
+    	lines, err := c.controlStringList(pconn, "MLST %s", path)
     	if err != nil {
     		if commandNotSupporterdError(err) {
-    			lines, err = c.dataStringList("LIST %s", path)
+    			lines, err = c.dataStringList(pconn, "LIST %s", path)
     			if err != nil {
     				return nil, err
     			}
@@ Expand Down Expand Up / @@ -202,17 +243,10 @@ func extractDirName(msg string) (string, error) { @@
     	return strings.Replace(msg[openQuote+1:closeQuote], `""`, `"`, -1), nil
     }
-    func (c *Client) controlStringList(f string, args ...interface{}) ([]string, error) {
-    	pconn, err := c.getIdleConn()
-    	if err != nil {
-    		return nil, err
-    	}
-    	defer c.returnConn(pconn)
+    func (c *Client) controlStringList(pconn *persistentConn, f string, args ...interface{}) ([]string, error) {
     	cmd := fmt.Sprintf(f, args...)
-    	code, msg, err := pconn.sendCommand(cmd)
+    	code, msg, _ := pconn.sendCommand(cmd)
     	if !positiveCompletionReply(code) {
     		pconn.debug("unexpected response to %s: %d-%s", cmd, code, msg)
@@ Expand All @@
     	return strings.Split(msg, "\n"), nil
     }
-    func (c *Client) dataStringList(f string, args ...interface{}) ([]string, error) {
-    	pconn, err := c.getIdleConn()
-    	if err != nil {
-    		return nil, err
-    	}
-    	defer c.returnConn(pconn)
+    func (c *Client) dataStringList(pconn *persistentConn, f string, args ...interface{}) ([]string, error) {
     	dcGetter, err := pconn.prepareDataConn()
     	if err != nil {
     		return nil, err
@@ Expand Down Expand Up / @@ -321,6 +348,66 @@ func (f *ftpFile) Sys() interface{} { @@
     	return f.raw
     }
+    var dirRegex = regexp.MustCompile(`^\s*(\S+\s+\S+\s{0,1}\S{2})\s+(\S+)\s+(.*)`)
+    var dirTimeFormats = []string{
+            "01-02-06  03:04",
+            "01-02-06  03:04PM",
+            "2006-01-02  15:04",
+    }
+    // 08/03/2016  17:13    <DIR>          directory-name
+    // 08/07/2016  17:20             1,135 file-name
+    // or
+    // 02-10-16  12:10PM           1067784 file.exe
+    func parseDirLIST(entry string, loc *time.Location, skipSelfParent bool) (os.FileInfo, error) {
+            matches := dirRegex.FindStringSubmatch(entry)
+            if len(matches) == 0 {
+                    return nil, ftpError{err: fmt.Errorf(`failed parsing LIST entry: %s`, entry)}
+            }
+            if skipSelfParent && (matches[3] == "." || matches[3] == "..") {
+                    return nil, nil
+            }
+            var err error
+            var size uint64
+            var mode os.FileMode = 0400
+            if strings.Contains(matches[2], "DIR") {
+                    mode |= os.ModeDir
+            } else {
+                size, err = strconv.ParseUint(matches[2], 10, 64)
+                if err != nil {
+                        return nil, ftpError{err: fmt.Errorf(`failed parsing LIST entry's size: %s (%s)`, err, entry)}
+                }
+            }
+            var mtime time.Time
+            for _, format := range dirTimeFormats {
+                    if len(entry) >= len(format) {
+                            mtime, err = time.ParseInLocation(format, matches[1], loc)
+                            if err == nil {
+                                    break
+                            }
+                    }
+            }
+            if err != nil {
+                    return nil, ftpError{err: fmt.Errorf(`failed parsing LIST entry's mtime: %s (%s)`, err, entry)}
+            }
+            info := &ftpFile{
+                    name:  filepath.Base(matches[3]),
+                    mode:  mode,
+                    mtime: mtime,
+                    raw:   entry,
+                    size:  int64(size),
+            }
+            return info, nil
+    }
     var lsRegex = regexp.MustCompile(`^\s*(\S)(\S{3})(\S{3})(\S{3})(?:\s+\S+){3}\s+(\d+)\s+(\w+\s+\d+)\s+([\d:]+)\s+(.+)$`)
     // total 404456
@@ Expand All @@
     	matches := lsRegex.FindStringSubmatch(entry)
     	if len(matches) == 0 {
-    		return nil, ftpError{err: fmt.Errorf(`failed parsing LIST entry: %s`, entry)}
+    		info, err := parseDirLIST(entry, loc, skipSelfParent)
+    		return info, err
     	}
     	if skipSelfParent && (matches[8] == "." || matches[8] == "..") {
@@ Expand Down Expand Up @@
     	parseError := ftpError{err: fmt.Errorf(`failed parsing MLST entry: %s`, entry)}
     	incompleteError := ftpError{err: fmt.Errorf(`MLST entry incomplete: %s`, entry)}
-    	parts := strings.Split(entry, "; ")
+    	parts := strings.SplitN(entry, "; ", 2)
     	if len(parts) != 2 {
     		return nil, parseError
     	}
@@ Expand All @@
     		return nil, incompleteError
     	}
-    	if skipSelfParent && (typ == "cdir" || typ == "pdir" || typ == "." || typ == "..") {
+    	if skipSelfParent && (typ == "cdir" || typ == "pdir" || typ == "." || typ == ".."|| parts[1] == "." || parts[1] == "..") {
     		return nil, nil
     	}
@@ Expand Down @@

several fixes and features (rfc3659) #19

Are you sure you want to change the base?

Uh oh!

several fixes and features (rfc3659) #19

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvicentiu Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muirmanders Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvicentiu Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muirmanders Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvicentiu Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvicentiu Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvicentiu Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muirmanders Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvicentiu Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading