is there anything i can do to help move this forward ?

Created attachment 3344 [details]
sftp server copy-data extension

Attach patch for commenting.

Comment on attachment 3344 [details]
sftp server copy-data extension

> [...] This avoids needing to
>+download the data before uploading it across the network twice.

Why uploading twice?

[...]
>+	{ "copy-data", "copy-data", 0, process_extended_copy_data, 1 },

This is a protocol violation.  The server is implementing secsh-filexfer-02 which does not specify copy-data.  Vendor extensions such as this need an @[domainname] suffix.

[...]
>+				ret = read(read_fd, buf, len);

ret should be ssize_t not int.

That said I don't think we'd consider this without a client implementation since without that we can't test it.

(In reply to Darren Tucker from comment #3)
[..]
> That said I don't think we'd consider this without a client
> implementation since without that we can't test it.

ignore that comment, I see the followup patch.

Created attachment 3345 [details]
sftp client copy-data extension

Comment on attachment 3345 [details]
sftp client copy-data extension

[...]
>+.It Ic copy Ar oldpath Ar newpath

the man page says "copy" but the code says "cp".  Personally I prefer "cp".

[...]
> +	} else {
>+		mode = 0666;

I'm not sure we should be making world writable files by default.

>+	if (old_handle == NULL) {
>+		sshbuf_free(msg);
>+		return -1;
>+	}
>+
>+	/* Open the new file for writing */
[...]
>+	if (new_handle == NULL) {
>+		sshbuf_free(msg);
>+		return -1;
+	}

I think this leaks old_handle if opening new_handle fails.  It'd probably be cleaner to initialise status and the handles to -1/NULL then do a "goto out" and have all the cleanup in one place.

(In reply to Darren Tucker from comment #3)
[...]
> >+	{ "copy-data", "copy-data", 0, process_extended_copy_data, 1 },
> 
> This is a protocol violation.  The server is implementing
> secsh-filexfer-02 which does not specify copy-data.  Vendor
> extensions such as this need an @[domainname] suffix.

Rereading the draft I think I was wrong about this.

(In reply to Darren Tucker from comment #3)

thx for the reviews.

> > [...] This avoids needing to
> >+download the data before uploading it across the network twice.
> 
> Why uploading twice?

i was referring to network transfers rather than upload specifically.  i'll rephrase to be more clear.

> >+	{ "copy-data", "copy-data", 0, process_extended_copy_data, 1 },
> 
> This is a protocol violation.  The server is implementing
> secsh-filexfer-02 which does not specify copy-data.  Vendor
> extensions such as this need an @[domainname] suffix.

the secsh-filexfer-02 spec [1] says:
> The name should be of the form "name@domain", where the domain is the DNS domain name of the organization defining the extension.
> Additional names that are not of this format may be defined later by the IETF.

since the IETF has an RFC that defines "copy-data" [2], and that's what i implemented, that's why i think it's appropriate to omit the @ vendor suffix.

[1] https://tools.ietf.org/html/draft-ietf-secsh-filexfer-02#section-4
[2] https://tools.ietf.org/html/draft-ietf-secsh-filexfer-extensions-00#section-7

> >+				ret = read(read_fd, buf, len);
> 
> ret should be ssize_t not int.

i'm aware of the API nuance, but i went with "follow existing style".  specifically, process_read() uses int with read(), and i mirrored that.  if that func is an oversight and should also be ssize_t, i'm happy to change.

(In reply to Darren Tucker from comment #6)
> >+.It Ic copy Ar oldpath Ar newpath
> 
> the man page says "copy" but the code says "cp".  Personally I
> prefer "cp".

nice catch.  sftp uses aliases for other commands, so i'll keep "copy" as the documented one, and keep "cp" as a shorter alias.  this keeps with existing "chdir" and "cd" behavior.

> > +	} else {
> >+		mode = 0666;
> 
> I'm not sure we should be making world writable files by default.

it isn't by default exactly ... that mode is used if the server doesn't respond with mode info for the source file which should be fairly unusual.

ignoring that, the server umask should apply.  so if the user's umask is like 0022, i'd expect this to be 0644.

that said, this is also a case of me copying existing style -- this is how do_download() behaves.

i can switch it to 0644 if people really prefer, but it seemed like all things considered, 0666 is correct.

> >+	if (old_handle == NULL) {
> >+		sshbuf_free(msg);
> >+		return -1;
> >+	}
> >+
> >+	/* Open the new file for writing */
> [...]
> >+	if (new_handle == NULL) {
> >+		sshbuf_free(msg);
> >+		return -1;
> +	}
> 
> I think this leaks old_handle if opening new_handle fails.  It'd
> probably be cleaner to initialise status and the handles to -1/NULL
> then do a "goto out" and have all the cleanup in one place.

i'll add an explicit free.  i didn't use goto style since this file doesn't do that for error handling in general.

Comment on attachment 3344 [details]
sftp server copy-data extension

looks good - some minor comments

>diff --git a/PROTOCOL b/PROTOCOL
>index f75c1c0ae5b0..04a392db33be 100644
...
>+static void
>+process_extended_copy_data(u_int32_t id)
...
>+	/* Disallow reading & writing to the same handle */
>+	if (read_handle == write_handle || read_fd < 0 || write_fd < 0) {

I think this should also check that both the read and write handles do not refer to the same path? (use handle_to_name())

>+		status = SSH2_FX_FAILURE;
>+	} else {

nit: prefer "goto out" over nesting if/else

>+		if (lseek(read_fd, read_off, SEEK_SET) < 0) {
>+			status = errno_to_portable(errno);
>+			error("process_extended_copy_data: read_seek failed");

nit: ditto

>+		} else if (!(handle_to_flags(write_handle) & O_APPEND) &&
>+				lseek(write_fd, write_off, SEEK_SET) < 0) {
>+			status = errno_to_portable(errno);
>+			error("process_extended_copy_data: write_seek failed");

nit: prefer __func__ to manual inclusion of function name

>+		} else {
>+			/* Process the request in chunks. */
>+			while (read_len || copy_until_eof) {

nit: prefer explicit comparison against zero (i.e "read_len > 0")

>+
>+				ret = read(read_fd, buf, len);
...
>+				ret = write(write_fd, buf, len);

I think this should use atomicio here to be signal safe.

>+				if ((size_t)ret != len) {
>+					debug2("nothing at all written");
>+					status = SSH2_FX_FAILURE;
>+					break;
>+				}

this block can go away with atomicio

Comment on attachment 3345 [details]
sftp client copy-data extension

This too looks good, minor comments:

>diff --git a/sftp-client.c b/sftp-client.c
>index 4986d6d8d291..cd2844a8585e 100644
>--- a/sftp-client.c
>+++ b/sftp-client.c
...
>+int
>+do_copy(struct sftp_conn *conn, const char *oldpath, const char *newpath)
>+{
...
>+	/* Silently return if the extension is not supported */
>+	if ((conn->exts & SFTP_EXT_COPY_DATA) == 0) {
>+		error("Server does not support copy-data extension");

This is not silent :)

>diff --git a/sftp.1 b/sftp.1
>index 0fd54cae090e..f2eae7f32790 100644
>--- a/sftp.1
>+++ b/sftp.1
...
>+.Ic lchdir , copy , chmod , chown ,

the manpage says the command is "copy", but ...

>diff --git a/sftp.c b/sftp.c
>index 7db86c2d3cf0..3288279172a9 100644
>--- a/sftp.c
>+++ b/sftp.c
...
>+	{ "cp",		I_COPY,		REMOTE	},

... it's implemented as "cp"

Either/both is fine, but it needs to be consistent of course.

Comment on attachment 3344 [details]
sftp server copy-data extension

>+	/* Disallow reading & writing to the same handle */
>+	if (read_handle == write_handle || read_fd < 0 || write_fd < 0) {

Maybe mention here that this also ensures that both handles refer to files rather than directories

thanks for the feedback.  i'll take another pass at this once the limits extension lands since they're touching a lot of the same code and i don't feel like fighting rebase conflicts while review is ongoing.

Created attachment 3494 [details]
sftp server copy-data extension v2

this should address all the feedback provided

Created attachment 3495 [details]
sftp client copy-data extension v2

this should address all the feedback

had to rebase after expand-path@openssh.com was added.  i posted the patches to the mailing list this time:
https://lists.mindrot.org/pipermail/openssh-unix-dev/2021-September/039670.html

it's been merged now for OpenSSH-9.0.  thanks all, very exciting!

Closing bugs from openssh-9.1 release cycle

OpenSSH 9.3 has been released. Close resolved bugs