Subject: Comments on SFTP draft 13

From: Miklos Szeredi <[email-masked]>
Date: Fri, 01 Sep 2006 17:25:19 +0200

First off, I'm new to the list, so apologies for any issues that have
already been discussed.

I have implemented an experimental sftp server that supports versions
6 (as well as 2 and 3) of the SFTP protocol and tries to conform fully
to draft 13 of the specification.  It works on Linux, and the source
code (released under GPL) is available from:

  http://fuse.sourceforge.net/tmp/sftp-server.c

See below for my comments accumulated during the implementation.

Miklos
--------------------------------------------------------------------------

|5.1.  Client Initialization
|
|   The SSH_FXP_INIT packet (from client to server) has the following
|   data:
|
|       uint32 version
|
|   'version' is the version number of the client.  If the client wishes
|   to interoperate with servers that support noncontiguous version
|   numbers it SHOULD send '3', and then use the 'version-select'
|   extension (see below.)

This seems wrong.  If the server doesn't send version-select, but
supports versions > 3, then the negotiation will end up with version
3, even though both the client and server support a greater version.

|5.4.  Supported Features

|   max-read-size
|      This is the maximum read size that the server guarantees to
|      complete.  For example, certain embedded server implementations
|      complete only the first 4K of a read, even if there is additional
|      data to be read from the file.
|
|      If the server specifies a non-zero value for max-read-size, it
|      MUST return the requested number of bytes for reads that are less
|      than or equal to the value, unless it encounters EOF or an ERROR.
|
|      The server MAY use this value to express that it is willing to
|      handle very large read requests, in excess of the standard 34000
|      bytes specified in Section 4.

Shouldn't this read

      ... very large WRITE requests ...

or just

      ... very large requests ...

Also "MAY" here makes client implementation difficult, since the
client has to recover from a failed request for servers which don't
handle large WRITE requests but set 'max-read-size' to a large value.

|6.  File Names

|   A server MAY include the following extension with it's version
|   packet.
|
|       string "filename-charset"
|       string charset-name
|

A pointer to a standard should be included here.  I've found this:

  http://www.iana.org/assignments/character-sets

|8.1.1.3.  flags

|   SSH_FXF_APPEND_DATA
|      Data is always written at the end of the file.  The offset field
|      of SSH_FXP_WRITE requests is ignored.
|
|      Data is not required to be appended atomically.  This means that
|      if multiple writers attempt to append data simultaneously, data
|      from the first may be lost.  However, data MAY be appended
|      atomically.
|
|   SSH_FXF_APPEND_DATA_ATOMIC
|      Data is always written at the end of the file.  The offset field
|      of the SSH_FXP_WRITE requests are ignored.
|
|      Data MUST be written atomically so that there is no chance that
|      multiple appenders can collide and result in data being lost.
|
|      If both append flags are specified, the server SHOULD use atomic
|      append if it is available, but SHOULD use non-atomic appends
|      otherwise.  The server SHOULD NOT fail the request in this case.

What is the relation of SSH_FXF_APPEND_DATA* to ACE4_APPEND_DATA?  As
I see it ACE4_APPEND_DATA is redundant.  It is unlikely that a client
would request APPEND access, while not actually wanting data to be
appended.  I think ACE4_APPEND_DATA should simply be ignored by
servers.

|   The 'attrs' field specifies the initial attributes for the file.
|   Default values MUST be supplied by the server for those attributes
|   that are not specified.  See Section ''File Attributes'' for more
|   information.

It is not clear what should happen if the server doesn't support
setting an attribute atomically with the create.  For example POSIX
create only supports setting the permission bits, and owner/group/
times/size/etc are not settable.  The 'size' attribute semantics with
OPEN are explicitly described in the spec, but others are not.

The most sensible solution I think is to return SSH_FX_OP_UNSUPPORTED
status.  However this could pose a problem for clients, which would
have to learn by trial and error which attributes are supported on
file creation and which are not.  The 'supported-attribute-mask'
doesn't help, if the server supports setting less attributes on file
creation than in a SETATTR operation.

The easiest solution is probably to extend "supported2" extension with
"supported-create-attribute-mask'.  This would apply to OPEN and
MKDIR.

|   O_CREAT
|      flags = SSH_FXF_OPEN_OR_CREATE
|
|   O_TRUNC
|      flags = SSH_FXF_TRUNCATE_EXISTING
|
|   O_TRUNC|O_CREATE
|      flags = SSH_FXF_CREATE_TRUNCATE

should be "O_TRUNC|O_CREAT"

And for completeness sake:

   O_CREAT|O_EXCL
      flags = SSH_FXF_CREATE_NEW

|8.5.  Retrieving File Attributes

|   The flags field specify the attribute flags in which the client has
|   particular interest.  This is a hint to the server.  For example,
|   because retrieving owner / group and acl information can be an
|   expensive operation under some operating systems, the server may
|   choose not to retrieve this information unless the client expresses a
|   specific interest in it.

I think this should be more than a hint: the server SHOULD mask the
attribute flags field with this, before composing the ATTRS data.
This saves bandwidth (both network and CPU) in case the client doesn't
need some of the attributes, even if the attribute itself is not
expensive to compute.

|8.7.  Dealing with Links

|   The SSH_FXP_LINK request creates a link (either hard or symbolic) on
|   the server.
|
|       byte   SSH_FXP_LINK
|       uint32 request-id
|       string new-link-path [UTF-8]
|       string existing-path [UTF-8]

'existing-path' should be 'target-path', as in the description.

|8.9.  Canonicalizing the Server-Side Path Name

|   control-byte
|
|       SSH_FXP_REALPATH_NO_CHECK    0x00000001
|       SSH_FXP_REALPATH_STAT_IF     0x00000002
|       SSH_FXP_REALPATH_STAT_ALWAYS 0x00000003
|
|      This field is optional, and if it is not present in the packet, it
|      is assumed to be SSH_FXP_REALPATH_NO_CHECK.
|
|      If SSH_FXP_REALPATH_NO_CHECK is specified, the server MUST NOT
|      fail the request if the path does not exist, is hidden, or the
|      user does not have access to the path or some component thereof.

It is impossible to canonicalize a non-existent path.  E.g.  "x/../z"
is NOT equivalent to "z" if "x" does not exist (or not a directory,
etc).

It is possible to partially canonicalize a non-existent or erroneous
path, but that doesn't make much sense, and it makes the
implementation difficult.

My implementation does not even try to canonicalize the path for
SSH_FXP_REALPATH_NO_CHECK, and only canonicalizes the path for
SSH_FXP_REALPATH_STAT_IF if stat returns success.

|      In addition, the path MUST NOT resolve symbolic links.  This
|      allows paths to be composed for the SSH_FXP_REMOVE command to
|      remove symbolic links.

This only requires that the last component be not resolved, and the
client can do this by omitting the last component before supplying it
to REALPATH.

|8.9.1.  Best Practice for Dealing with Paths

|   The client SHOULD treat the results of SSH_FXP_REALPATH as a
|   canonical absolute path, even if the path does not appear to be
|   absolute.  A client that uses REALPATH(".", "") and treats the result
|   as absolute, even if there is no leading slash, will continue to
|   function correctly, even when talking to a Windows NT or VMS style
|   system, where absolute paths may not begin with a slash.

This contradicts section 6:

*   File names starting with a slash are "absolute", and are relative to
*   the root of the file system.  Names starting with any other character
*   are relative to the user's default directory (home directory).

|   The client SHOULD also use SSH_FXP_REALPATH call to compose paths so
|   that it does not need to know when a path is absolute or relative.

This is unnecessary if servers _do_ follow the "absolute begins with a
slash" rule.

So I think it should rather be up to the server to always convert any
local path representations into one which follow the rules outlined in
section 6.  E.g.  "c:/x/y/z" could become "/c$/x/y/z".

Hence all the added complexity of REALPATH and it's optional fields
become unnecessary.  In addition path composition normally wouldn't
require a round-trip to the server.

|9.4.  Name Response

|   end-of-list
|      If this field is present and true, there are no more entries to be
|      read.  This field should either be omitted or be true unless the
|      request is SSH_FXP_READDIR.

This last sentence is not clear to me.

Should this field be always specified for READDIR?

Should it be omitted for other requests?

Does omitting it imply a TRUE or a FALSE value?

--------------------------------------------------------------------------
Here are some features that I miss from the current protocol.  These
could be added as extensions, but the core protocol might benefit from
standardizing some of them.

SSH_FXP_READDIR, SSH_FXP_REALPATH:

   SSH_FXP_STAT and friends have a 'flags' field for indicating which
   attributes the client is interested in.  This field should be equally
   applicable to a READDIR request.

Extension to the ATTRS data:

   - numeric owner/group.  The client may want to display numeric or
     named values in directory listings on a request by request basis
     (e.g. depending on the '-n' option of 'ls').

   - device number (st_dev), inode number (st_ino) fields.  The client
     can use this to identify directory entries which are hard links
     to the same file.

   - device type (st_rdev) for device files.

New requests:

MKNOD:

   Create file node (FIFO or other special file types).  Same format
   as MKDIR.  The type field in ATTR is used to determine the file
   type.  Exclude directory creation, but possibly include regular
   file creation, which can be used to create an empty file with a
   single request, instead of having to do OPEN/CLOSE.

STATFS:

   Return file system statistics for the given path.  A good starting
   point would be the fields of POSIX struct statvfs.