Subject: I-D ACTION:draft-ietf-secsh-filexfer-08.txt

From: [email-masked]
Date: Wed, 06 Apr 2005 15:34:55 -0400

A New Internet-Draft is available from the on-line Internet-Drafts directories.
This draft is a work item of the Secure Shell Working Group of the IETF.

	Title		: SSH File Transfer Protocol
	Author(s)	: J. Galbraith, et al.
	Filename	: draft-ietf-secsh-filexfer-08.txt
	Pages		: 63
	Date		: 2005-4-6
	
The SSH File Transfer Protocol provides secure file transfer
functionality over any reliable data stream.  It is the standard file
transfer protocol for use with the SSH2 protocol.  This document
describes the file transfer protocol and its interface to the SSH2
protocol suite.

A URL for this Internet-Draft is:
http://www.ietf.org/internet-drafts/draft-ietf-secsh-filexfer-08.txt

To remove yourself from the I-D Announcement list, send a message to 
[email-masked] with the word unsubscribe in the body of the message.  
You can also visit https://www1.ietf.org/mailman/listinfo/I-D-announce 
to change your subscription settings.

Internet-Drafts are also available by anonymous FTP. Login with the username
"anonymous" and a password of your e-mail address. After logging in,
type "cd internet-drafts" and then
	"get draft-ietf-secsh-filexfer-08.txt".

A list of Internet-Drafts directories can be found in
http://www.ietf.org/shadow.html 
or ftp://ftp.ietf.org/ietf/1shadow-sites.txt

Internet-Drafts can also be obtained by e-mail.

Send a message to:
	[email-masked].
In the body type:
	"FILE /internet-drafts/draft-ietf-secsh-filexfer-08.txt".
	
NOTE:	The mail server at ietf.org can return the document in
	MIME-encoded form by using the "mpack" utility.  To use this
	feature, insert the command "ENCODING mime" before the "FILE"
	command.  To decode the response(s), you will need "munpack" or
	a MIME-compliant mail reader.  Different MIME-compliant mail readers
	exhibit different behavior, especially when dealing with
	"multipart" MIME messages (i.e. documents which have been split
	up into multiple messages), so check your local documentation on
	how to manipulate these messages.

Below is the data which will enable a MIME compliant mail reader
implementation to automatically retrieve the ASCII version of the
Internet-Draft.

---

From: Joseph Galbraith <[email-masked]>
Date: Thu, 07 Apr 2005 14:16:15 -0600

Comments?

I'm sure the new locking stuff needs work?

If nobody has comments, I'm out of a job :-)

Thanks,

Joseph

---

From: der Mouse <[email-masked]>
Date: Sun, 10 Apr 2005 14:04:28 -0400 (EDT)

> 	Filename	: draft-ietf-secsh-filexfer-08.txt

I finally got around to doing a copy-edit read over this.

As compared to filexfer-07:

-   channel in [I-D.ietf-secsh-architecture].  and that the server has
+   channel in [I-D.ietf-secsh-architecture]. and that the server has

This change is good, but s/\. /, / surely?

At the end of 3.3, I see new text

   Values 210-255 are reserved for use in conjunction with these
   extensions.  The SSH_FXP_EXTENDED packet can be used to negotiate the
   meaning of these reserved types.  It is suggested that the actual
   value to be used also be negotiated, since this will prevent
   collision among multiple uncoordinated extensions.

   The server MUST respond with SSH_FXP_STATUS(SSH_FX_OP_UNSUPPORTED) if
   it receives a packet it does not recognize.  The protocol version
   (Section 4) MUST be incremented if the server is to send new packets
   to the client (because the client has no way to respond indicating
   that the packet isn't recognized.)

Except for the first sentence in each of these paragraphs, they appear
to me to be in conflict: the first is saying "here's how to negotiate
new packet types in this version" and the second is saying "the
protocol version must be changed to send new packet types".

At the beginning of 4.4, I see

   It is often necessary to detect the version of a the server so that

"a the server"?  Surely that needs fixing.

Just after the page break there, there's a missing ):

   so.  (It may also be sent by the client using an EXTENDED request.

In 4.5, supported-open-block-masks and supported-block-masks are
uint64s.  Is there any reason they aren't simply 16-bit fields
(probably uint32 on the wire), with each bit indicating whether a
particular combination is supported?  (There are four BLOCK bits, thus,
16 possible combinations.)  Also, the current spec does not provide any
indication of how many of the 16 four-bit fields are valid in the
uint64; while the sender clearly must pad with multiple copies of some
combination (as in the example given, which pads with multiple copies
of 0b0000), I'd prefer to see this mentioned explicitly too -
especially in the example; the current wording implies that only the
low two nibbles are used in that example.  (Since at least one
combination must be supported for sftp to be useful at all, it's true
that there's no problem finding a combination to pad with.)  It also
occurs to me that you might want to require that 0b0000 be accepted.

There's a blank line missing between supported-block-masks's
description and attrib-extension-count's.

In 4.6, the example given for comma-separated-versions includes a
version not in the list of permitted version numbers.  I'm not sure
whether I think this needs to change or not, but it looks at least a
little odd.  Also, shouldn't "higher than version '3'" at the beginning
of the next paragraph be "at least as high as version '3'" or "no lower
than '3'" or some such?

In the version-select request, the version-from-list is a uint32.
Since version numbers may be other than simple integers (the
comma-separated-versions is defined to include the DNS extensibility
mechanism), shouldn't this be a string?  Otherwise, how is the client
supposed to indicate whether that (say) 8 it's sending is
[email-masked] or [email-masked] or [email-masked] -
and for that matter what should the client do if the server's list
includes totally non-numeric values such as "[email-masked]"?

Also, there is wording such as "higher than" that implies there is a
total order on version "number"s.  It isn't obvious to me that this is
true in the presence of the DNS extensibility mechanism.

In section 5,

   Servers SHOULD interpret a path name component ".."  (Section 11) as

an extra space seems to have got inserted between " and (.  I
conjecture some automated tool mistook ." for an end-of-quoted-sentence
and inserted end-of-sentence space in consequence.

In 6.9,

   comminicute only the bits it knows about without inadvertantly

s/comminicute/communicate/

There's a misplaced "only" that I apparently missed before:

   mechanism is recommended for most purposes; additional flags bits
   should only be defined by an IETF standards action that also
   increments the protocol version number.  The use of such new fields

s/only be defined/be defined only/

Various places in the document speak of SSH_FXF_TEXT.  This appears to
be undefined as to numerical value; the placement of its description in
7.1.1.3, and the lack of a description of the SSH_FXF_ACCESS_TEXT_MODE
flag which appears in the list early in 7.1.1.3, leads me to suspect
that the one is a mistake for the other.  In any case, something needs
to be fixed.

The description of SSH_FXF_TEXT (see the previous paragraph) says that

      When a file is opened with the FXF_TEXT flag, the offset field in
      the read and write functions is ignored.

but it also says

      Clients MUST be prepared to handle SSH_FX_OP_UNSUPPORTED returned
      for read or write operations that are not sequential.

Which is it?  Is the offset ignored, or is the server permitted to
check it and return OP_UNSUPPORTED errors for non-sequential I/O?

SSH_FXF_ACCESS_BLOCK_READ's description begins

      The server MUST guarantee that no other open has taken place which
      blocked handle has been opened with ACE4_READ_DATA access, and

This looks like fragments of two sentences pasted together ("no other
open has taken place which blocked ...??... handle has been opened
with").

SSH_FXF_ACCESS_BLOCK_WRITE's description begins

      The server MUST guarantee that no other lock has been opened with
      ACE4_WRITE_DATA or ACE4_APPEND_DATA access, and that no other

"no other lock has been opened"?  s/lock/handle/, or something else?

SSH_FXF_ACCESS_DELETE_ON_CLOSE appears to be intended for operations
that, in the traditional Unix model, unlink an open file.  But that
operation, while it does not delete the file, *does* delete the *name*
immediately.  It is not clear whether DELETE_ON_CLOSE is permitted to
destroy the name immediately even if it does not destroy the file,
since as far as sftp clients who do not have a file open are concerned,
the file's existence is indistinguishable from its name's existence.

7.2.1, describing the offset argument; 7.2.3's "offset" is similar:

      'offset' is the offset (in bytes) relative to the beginning of the
      file that the read MUST start at. 'offset' is ignored if
      SSH_FXF_TEXT was specified during the open.

The end-of-sentence space after "at." has shrunk.

7.2.1 again, describing "length":

      If the server specified a non-zero 'max-read-size' in it's

s/it's/its/

7.7, third paragraph, typo fix:

   The SSH_FXP_LINK request creates a link (either hare or symbolic) on
   the server.

s/hare/hard/

In 7.7, after describing SSH_FXP_LINK, the text charges right into a
description of SSH_FXP_LOCK, or SSH_FXP_BLOCK - the running text says
_LOCK but the request description says _BLOCK.  There also is no
heading break; I'd expect to see 7.8 begin here.  Also, the first
sentence seems to either have a spurious article or lack a noun:

   The SSH_FXP_LOCK creates a ...

Either s/The // or s/LOCK/& request/, it seems to me.

I also note that LOCK/BLOCK is specified as not working on directories;
I see no particular reason for the protocol to forbid this (though of
course some servers won't support it).  (Of course, it's not that
useful for non-advisory locks, but now that the protocol has advisory
locks, it does have its uses.)

The description is also missing two blank lines between offset and
length, and length and uLockMask.  Also, the studlyCaps style of
uLockMask is rather at odds with the style of most other packet field
names, which would argue for u-lock-mask instead.

SSH_FXP_UNLOCK is similar: it is confused between UNLOCK and UNBLOCK,
it has either a spurious article or a missing noun, it has no heading,
it is specced to fail on directories, and it's missing a blank line
between offset and length.

In 7.8,

   The server MUST take the 'original-path' and apply the 'compose-path'
[page-break]
   as a modification to it. 'compose-path' MAY be relative to 'original-
   path' or may be an absolute path, in which case 'original-path' will
   be discarded.  The 'compose-path' may be zero length.

s/. '/.  '/ in the second line.

I also think that if you're going to use "MAY" there, you should use it
for the other one, the one that's now spelled "may", too.

In 7.8.1, in view of the RFCEDITOR REMOVE BEFORE PUBLISHING paragraphs,
I note that the current REALPATH mechanism does have a downside which
is not described: when composing more than two pieces, multiple server
roundtrips are required.  How much of a pain would it be to make
REALPATH take a more or less arbitrary number of compose-paths?

8.1 seems to be missing a blank line between "language tag" and
"<error-specific data>".  In passing, why does the description have <>
around "error-specific data" when the packet description doesn't?

8.1, describing SSH_FX_OP_UNSUPPORTED, has a stray ) at the end of the
description.

8.1, describing SSH_FX_UNKNOWN_PRINCIPAL, missed changing one
"principle" to "principal" (the fourth-last word in the description).

8.1 says, in SSH_FX_BYTE_RANGE_LOCK_CONFLICT, that "the SFTP protocol
does not support byte-range locks natively" even though
SSH_FXP_{,B}LOCK (see above for the ambiguity) does indeed support
byte-range locks.

9.1.2, describing hash-algorithm-list:

      A comma separated list of hash alogirthms the client is willing to

s/alogirthms/algorithms/

Also in 9.1.2's hash-algorithm-list description, an end-of-sentence
space has shrunk:

      and SHA-512 are decribed in [FIPS-180-2]. crc32 is described in

In 9.3, the absolute-pathname is said to be "suitable for use in
operations such as REALPATH or OPEN" - surely s/OPEN/&DIR/?

/~\ The ASCII				der Mouse
\ / Ribbon Campaign
 X  Against HTML	       [email-masked]
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

---

From: Joseph Galbraith <[email-masked]>
Date: Mon, 18 Apr 2005 23:00:00 -0600

Thanks to der Mouse for the reviews.  I've gone through
and fixed these.  I'm going to cycle the draft again
in a couple of days, so let me know if anything else
needs changing.

(I believe this round introduced one incompatible change.
If you are shipping or getting ready to ship a v6 implementation,
let me know... I think we are getting close to being done...)

> This change is good, but s/\. /, / surely?

Fixed.
> protocol version must be changed to send new packet types".

Not quite.  Notice that the restriction is applied to new
packets being sent to the client, not the other way around.

The client _can_ send new packets.  I have, however, revised
the section to hopefully be more clear:

    The server MUST respond with SSH_FXP_STATUS(SSH_FX_OP_UNSUPPORTED) if
    it receives a packet it does not recognize.

    The use of additional packet types in the non-extension range MUST be
    introduced through IETF consensus.  New packet types to be sent from
    the client to the server MAY be introduced without changing the
    protocol version (Section 4).  Because the client has no way to
    respond to unrecognized packet types, new packet types to be sent
    from the server to the client the client MUST not used unless the
    protocol version is changed or the client has negotiated to received
    them.  This negotiation MAY be explicit, through the use of
    extensions, or MAY be implicit, by the client itself using a packet
    type not defined above.

> "a the server"?  Surely that needs fixing.

Fixed.

> particular combination is supported?

Well, I set out to do it that way originally, but it was easier
to specify this way.  Can you come up with some language that
specs it reasonably clearly with adding a whole bunch of new
identifiers?

I also suspect that it will be easier to use as a series of
bitmasks.

 > (There are four BLOCK bits, thus,
> low two nibbles are used in that example.

I origanally had wording specifying that it should be padded out
with 0b0000, but it got lost somewhere.

I'm still not entirely happy with this whole thing... but nothing
better is coming to mind at the moment.

>  (Since at least one

Right; I've added that 0b0000 MUST be accepted, and that no other
mask other than 0b0000 may occur more than once.

> description and attrib-extension-count's.

Thanks, fixed.

> version not in the list of permitted version numbers.

Fixed.

   I'm not sure
> of the next paragraph be "at least as high as version '3'" or "no lower

Fixed.

> mechanism), shouldn't this be a string?

Yikes!  Thanks, indeed.  It is a string (and the uint32 version
is left over from a prior proposal.)

> Otherwise, how is the client

Pedantically, what you say is true; however, there is order (at
least in time and features set) to the versions specified so
far.

I've changed it to read:

    Typically, the client SHOULD NOT down-grade the protocol version
    using this extension; however, it is not forbidden to do so.  One
    reason a client might do so is to work around a buggy implementation.

Is that better?

> and inserted end-of-sentence space in consequence.

Argh... I can't seem to convince it to stop.  I don't
care enough about this to do manual edits before
publishing... if the RFC editor cares, I'll do a manual
edit before we go RFC :-)

> s/comminicute/communicate/

Thanks.

> s/only be defined/be defined only/

Thanks.

> check it and return OP_UNSUPPORTED errors for non-sequential I/O?

Thanks... I took out the second one-- the offset is ignored.

> "no other lock has been opened"?  s/lock/handle/, or something else?

Thanks, fixed both of them.

> the file's existence is indistinguishable from its name's existence.

Hmmm... in this case, I think I'm inclined to make it undefined
whether the filename is removed immediately or on close.  (In
windows, I believe neither the name nor the file is removed until
close.)

Does anyone think this is a problem?

> The end-of-sentence space after "at." has shrunk.

Thanks.  Fixed to be a little less awkward to (hopefully.)

>       If the server specified a non-zero 'max-read-size' in it's

Thanks.

>    The SSH_FXP_LINK request creates a link (either hare or symbolic) on

Thanks.

> Either s/The // or s/LOCK/& request/, it seems to me.

Fixed.

> locks, it does have its uses.)

Fixed.

> names, which would argue for u-lock-mask instead.

Fixed.

> between offset and length.

Fixed.

> s/. '/.  '/ in the second line.

Argh... xml2rfc is doing this... I'm going to ignore it for now...
maybe eventually, it will get smarter?

> for the other one, the one that's now spelled "may", too.

Done.
> REALPATH take a more or less arbitrary number of compose-paths?

Done.

I switched the order of the control byte and compose path and
let compose path have multiple elements.  Read until EOP.

> around "error-specific data" when the packet description doesn't?

Fixed; and removed <>.

> 8.1, describing SSH_FX_OP_UNSUPPORTED, has a stray ) at the end of the

Thanks.

> "principle" to "principal" (the fourth-last word in the description).

Thanks.

> SSH_FXP_{,B}LOCK (see above for the ambiguity) does indeed support

It's gone now.

>       A comma separated list of hash alogirthms the client is willing to

Fixed.

>       and SHA-512 are decribed in [FIPS-180-2]. crc32 is described in

xml2rfc has a grudge.  I reordered the sentance to make it
do the right thing (TM)

> operations such as REALPATH or OPEN" - surely s/OPEN/&DIR/?

Fixed.

Thanks again for you reviews.

Joseph

---

From: der Mouse <[email-masked]>
Date: Tue, 19 Apr 2005 01:12:54 -0400 (EDT)

> (I believe this round introduced one incompatible change.  [...])

It might help to call out what it is.  I assume you're talking about
the REALPATH change, but it could also be the version-select switch
from uint32 to string.  (Your reply makes it clear *you've* been
thinking of the latter as a string, but that's not what it said.)

>     introduced through IETF consensus.  [...]

Thank you, that helps greatly.  The "in the non-extension range"
semantics were missing from the previous language - or at least,
unclear enough that I read them as missing - and it makes all the
difference.  (The rest of the wording changes are less critical, though
I find the new text significantly better nevertheless.)

> Well, I set out to do it that way originally, but it was easier to
> specify this way.  Can you come up with some language that specs it
> reasonably clearly with adding a whole bunch of new identifiers?

Probably.  Let's see, how about this (using supported-open-block-masks
for the sake of argument):

           uint32 supported-open-block-masks

(actually, I'd prefer uint16, but I don't think we have one - maybe put
both block-masks in a single uint32?)

   supported-open-block-masks
      A 16-bit mask specifying which combinations of blocking flags are
      supported.  Each bit corresponds to one combination of the
      SSH_FXF_ACCESS_BLOCK_READ, SSH_FXF_ACCESS_BLOCK_WRITE,
      SSH_FXF_ACCESS_BLOCK_DELETE, and SSH_FXF_ACCESS_BLOCK_ADVISORY
      bits from Section 7.1.1.3, with a set bit corresponding to a
      supported combination and a clear bit an unsupported combination.
      The index of a bit, bit zero being the least significant bit,
      viewed as a four-bit number, corresponds to a combination of flag
      bits, shifted right so that BLOCK_READ is the least significant
      bit.  The combination `no blocking flags' MUST be supported, so
      the low bit will always be set.

      For example, a server supporting only the classic advisory read
      (shared) and write (exclusive) locks would set the bits
      corresponding to READ+WRITE+ADVISORY, 0b1011, and WRITE+ADVISORY,
      0b1010, plus the always-set bit 0b0000, giving a mask value of
      0b0000110000000001, or 0x0c01; a server supporting no locking at
      all would set only bit zero, giving 0x0001.

      Bits other than the low 16 are reserved for possible future use
      and MUST NOT be set.

The last sentence does not apply if both masks are packed into a single
uint32, of course.

> I also suspect that it will be easier to use as a series of bitmasks.

Well, as a code author, if I had to deal with the masks as specified,
I'd use the 16-bit mask form internally, converting to the spec's form
very late on output and very early on input.  I suspect this says more
about what kind of internal data structures I prefer (and to some
extent what language I'd likely be using) than about anything else....

> I origanally had wording specifying that it should be padded out with
> 0b0000, but it got lost somewhere.

Well, if the current spec is kept, I think it needs to be found. :)

> Is that better?

Somewhat.  There is still the implication that of two versions, one
will be a downgrade compared to the other, which, even if it's true
now, surely will not remain true forever.  However, I think the
implication is loose enough that that language is OK.

I'm still not clear whether totally non-numeric version strings are
acceptable or not.  I see nothing that would forbid, say,
"[email-masked]" from appearing in the list, nor any simple way
to decide whether it would be a downgrade as compared to
"[email-masked]".  But if you're going to allow DNS
extensibility at all, that is, ultimately, unavoidable - it's entirely
possible that of two "private" versions, neither is a feature subset of
the other, regardless of how they're named.

>> It is not clear whether DELETE_ON_CLOSE is permitted to destroy the
>> name immediately even if it does not destroy the file, [...]
> whether the filename is removed immediately or on close.

I think that's probably the route I'd go.

> Thanks again for you reviews.

No problem!

/~\ The ASCII				der Mouse
\ / Ribbon Campaign

---

From: Joseph Galbraith <[email-masked]>
Date: Tue, 19 Apr 2005 06:47:29 -0600

> thinking of the latter as a string, but that's not what it said.)

Sorry... you're right, it was the REALPATH change.

> both block-masks in a single uint32?)

How about we simply define one?

> The last sentence does not apply if both masks are packed into a single

Great, I'll use this then (barring the last sentance.) I think I'll
introduce uint16 in the data-types section... then if you (the
collective you) want to considered it a packed uint32, you can,
but it'll be speced seperately, which is the nicer way to do it.

> the other, regardless of how they're named.

I'll explicitly allow non-numeric values (that was the intent.)

Nico suggested that we could even allow a-la carte implementation
of v6, such as attrib-v6, which would imply whatever version was 
previously in use, but using v6 attributes.  (I'd probably actually
allow "3,attrib-v6" as the response, indicating v3 w/ v6 attributes,
and specifically disallow two 'numeric' items in the response.)

I haven't gone that route... because I'm not convinced it would
convince any more people to migrate to v6 (or even to get the
benifits of just v6 attributes.)

And it would complicate implementations even more and I've got
people complaining as is.

Thanks,

Joseph