Mike Frysinger
2020-Nov-30 04:57 UTC
[PATCH 1/5] sftp: add missing lsetstat@openssh.com documentation
--- PROTOCOL | 16 ++++++++++++++++ sftp-server.c | 1 + sftp.1 | 27 ++++++++++++++++++--------- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/PROTOCOL b/PROTOCOL index ecdacb9dc75e..abb083af3ea0 100644 --- a/PROTOCOL +++ b/PROTOCOL @@ -465,6 +465,22 @@ respond with a SSH_FXP_STATUS message. This extension is advertised in the SSH_FXP_VERSION hello with version "1". +3.7. sftp: Extension request "lsetstat at openssh.com" + +This request is like the "setstat" command, but sets file attributes on +symlinks. It is implemented as a SSH_FXP_EXTENDED request with the +following format: + + uint32 id + string "lsetstat at openssh.com" + string path + ATTRS attrs + +See the "setstat" command for more details. + +This extension is advertised in the SSH_FXP_VERSION hello with version +"1". + 4. Miscellaneous changes 4.1 Public key format diff --git a/sftp-server.c b/sftp-server.c index f00f39e64f68..ec3274d24cf5 100644 --- a/sftp-server.c +++ b/sftp-server.c @@ -671,6 +671,7 @@ process_init(void) /* fsync extension */ (r = sshbuf_put_cstring(msg, "fsync at openssh.com")) != 0 || (r = sshbuf_put_cstring(msg, "1")) != 0 || /* version */ + /* lsetstat extension */ (r = sshbuf_put_cstring(msg, "lsetstat at openssh.com")) != 0 || (r = sshbuf_put_cstring(msg, "1")) != 0) /* version */ fatal_fr(r, "compose"); diff --git a/sftp.1 b/sftp.1 index 1cfa5ec229e3..e9eec7feffa4 100644 --- a/sftp.1 +++ b/sftp.1 @@ -348,15 +348,18 @@ Change group of file .Ar path to .Ar grp . -If the -.Fl h -flag is specified, then symlinks will not be followed. .Ar path may contain .Xr glob 7 characters and may match multiple files. .Ar grp must be a numeric GID. +.Pp +If the +.Fl h +flag is specified, then symlinks will not be followed. +Note that this is only supported by servers that implement +the "lsetstat at openssh.com" extension. .It Xo Ic chmod .Op Fl h .Ar mode @@ -366,13 +369,16 @@ Change permissions of file .Ar path to .Ar mode . -If the -.Fl h -flag is specified, then symlinks will not be followed. .Ar path may contain .Xr glob 7 characters and may match multiple files. +.Pp +If the +.Fl h +flag is specified, then symlinks will not be followed. +Note that this is only supported by servers that implement +the "lsetstat at openssh.com" extension. .It Xo Ic chown .Op Fl h .Ar own @@ -382,15 +388,18 @@ Change owner of file .Ar path to .Ar own . -If the -.Fl h -flag is specified, then symlinks will not be followed. .Ar path may contain .Xr glob 7 characters and may match multiple files. .Ar own must be a numeric UID. +.Pp +If the +.Fl h +flag is specified, then symlinks will not be followed. +Note that this is only supported by servers that implement +the "lsetstat at openssh.com" extension. .It Xo Ic df .Op Fl hi .Op Ar path -- 2.28.0
Mike Frysinger
2020-Nov-30 04:57 UTC
[PATCH 2/5] sftp-server: implement limits@openssh.com extension
As discussed in bz#2949, this is a simple extension that allows the server to clearly communicate transfer limits it is imposing so the client doesn't have to guess, or force the user to manually tune. This is particularly useful when an attempt to use too large of a value causes the server to abort the connection. --- PROTOCOL | 43 +++++++++++++++++++++++++++++++++++++++++++ sftp-server.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/PROTOCOL b/PROTOCOL index abb083af3ea0..33298613b3b2 100644 --- a/PROTOCOL +++ b/PROTOCOL @@ -481,6 +481,49 @@ See the "setstat" command for more details. This extension is advertised in the SSH_FXP_VERSION hello with version "1". +3.8. sftp: Extension request "limits at openssh.com" + +This request is used to determine various limits the server might impose. +Clients should not attempt to exceed these limits as the server might sever +the connection immediately. + + uint32 id + string "limits at openssh.com" + +The server will respond with a SSH_FXP_EXTENDED_REPLY reply: + + uint32 id + uint64 max-packet-length + uint64 max-read-length + uint64 max-write-length + uint64 max-open-handles + +The 'max-packet-length' applies to the total number of bytes in a +single SFTP packet. Servers SHOULD set this at least to 34000. + +The 'max-read-length' is the largest length in a SSH_FXP_READ packet. +Even if the client requests a larger size, servers will usually respond +with a shorter SSH_FXP_DATA packet. Servers SHOULD set this at least to +32768. + +The 'max-write-length' is the largest length in a SSH_FXP_WRITE packet +the server will accept. Servers SHOULD set this at least to 32768. + +The 'max-open-handles' is the maximum number of active handles that the +server allows (e.g. handles created by SSH_FXP_OPEN and SSH_FXP_OPENDIR +packets). Servers MAY count internal file handles against this limit +(e.g. system logging or stdout/stderr), so clients SHOULD NOT expect to +open this many handles in practice. + +If the server doesn't enforce a specific limit, then the field may be +set to 0. This implies the server relies on the OS to enforce limits +(e.g. available memory or file handles), and such limits might be +dynamic. The client SHOULD take care to not try to exceed reasonable +limits. + +This extension is advertised in the SSH_FXP_VERSION hello with version +"1". + 4. Miscellaneous changes 4.1 Public key format diff --git a/sftp-server.c b/sftp-server.c index ec3274d24cf5..c9500d9900b2 100644 --- a/sftp-server.c +++ b/sftp-server.c @@ -18,6 +18,7 @@ #include "includes.h" #include <sys/types.h> +#include <sys/resource.h> #include <sys/stat.h> #ifdef HAVE_SYS_TIME_H # include <sys/time.h> @@ -53,6 +54,9 @@ char *sftp_realpath(const char *, char *); /* sftp-realpath.c */ +/* Maximum data read that we are willing to accept */ +#define SFTP_MAX_READ_LENGTH (64 * 1024) + /* Our verbosity */ static LogLevel log_level = SYSLOG_LEVEL_ERROR; @@ -110,6 +114,7 @@ static void process_extended_fstatvfs(u_int32_t id); static void process_extended_hardlink(u_int32_t id); static void process_extended_fsync(u_int32_t id); static void process_extended_lsetstat(u_int32_t id); +static void process_extended_limits(u_int32_t id); static void process_extended(u_int32_t id); struct sftp_handler { @@ -152,6 +157,7 @@ static const struct sftp_handler extended_handlers[] = { { "hardlink", "hardlink at openssh.com", 0, process_extended_hardlink, 1 }, { "fsync", "fsync at openssh.com", 0, process_extended_fsync, 1 }, { "lsetstat", "lsetstat at openssh.com", 0, process_extended_lsetstat, 1 }, + { "limits", "limits at openssh.com", 0, process_extended_limits, 1 }, { NULL, NULL, 0, NULL, 0 } }; @@ -641,6 +647,36 @@ send_statvfs(u_int32_t id, struct statvfs *st) sshbuf_free(msg); } +static void +send_limits(u_int32_t id) +{ + struct sshbuf *msg; + int r; + uint64_t nofiles = 0; + +#if defined(HAVE_GETRLIMIT) && defined(RLIMIT_NOFILE) + struct rlimit rlim; + if (getrlimit(RLIMIT_NOFILE, &rlim) != -1) + nofiles = rlim.rlim_cur; +#endif + + if ((msg = sshbuf_new()) == NULL) + fatal_f("sshbuf_new failed"); + if ((r = sshbuf_put_u8(msg, SSH2_FXP_EXTENDED_REPLY)) != 0 || + (r = sshbuf_put_u32(msg, id)) != 0 || + /* max-packet-length */ + (r = sshbuf_put_u64(msg, SFTP_MAX_MSG_LENGTH)) != 0 || + /* max-read-length */ + (r = sshbuf_put_u64(msg, SFTP_MAX_READ_LENGTH)) != 0 || + /* max-write-length */ + (r = sshbuf_put_u64(msg, SFTP_MAX_MSG_LENGTH - 1024)) != 0 || + /* max-open-handles */ + (r = sshbuf_put_u64(msg, nofiles)) != 0) + fatal_fr(r, "compose"); + send_msg(msg); + sshbuf_free(msg); +} + /* parse incoming */ static void @@ -673,6 +709,9 @@ process_init(void) (r = sshbuf_put_cstring(msg, "1")) != 0 || /* version */ /* lsetstat extension */ (r = sshbuf_put_cstring(msg, "lsetstat at openssh.com")) != 0 || + (r = sshbuf_put_cstring(msg, "1")) != 0 || /* version */ + /* limits extension */ + (r = sshbuf_put_cstring(msg, "limits at openssh.com")) != 0 || (r = sshbuf_put_cstring(msg, "1")) != 0) /* version */ fatal_fr(r, "compose"); send_msg(msg); @@ -739,7 +778,7 @@ process_close(u_int32_t id) static void process_read(u_int32_t id) { - u_char buf[64*1024]; + u_char buf[SFTP_MAX_READ_LENGTH]; u_int32_t len; int r, handle, fd, ret, status = SSH2_FX_FAILURE; u_int64_t off; @@ -1437,6 +1476,13 @@ process_extended_lsetstat(u_int32_t id) free(name); } +static void +process_extended_limits(u_int32_t id) +{ + debug("request %u: limits", id); + send_limits(id); +} + static void process_extended(u_int32_t id) { -- 2.28.0
Mike Frysinger
2020-Nov-30 04:57 UTC
[PATCH 3/5] sftp-client: use new limits@openssh.com extension
Now that the server will tell us its limits, we can use those to select a good transfer size. In practice, the default increases from 32KiB to 64KiB when using newer OpenSSH servers. --- sftp-client.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++--- sftp-client.h | 11 ++++++ sftp.c | 7 ++-- 3 files changed, 105 insertions(+), 9 deletions(-) diff --git a/sftp-client.c b/sftp-client.c index d82e31aeeec4..ef3906477bd5 100644 --- a/sftp-client.c +++ b/sftp-client.c @@ -61,6 +61,12 @@ extern volatile sig_atomic_t interrupted; extern int showprogress; +/* Default size of buffer for up/download */ +#define DEFAULT_COPY_BUFLEN 32768 + +/* Default number of concurrent outstanding requests */ +#define DEFAULT_NUM_REQUESTS 64 + /* Minimum amount of data to read at a time */ #define MIN_READ_SIZE 512 @@ -87,6 +93,7 @@ struct sftp_conn { #define SFTP_EXT_HARDLINK 0x00000008 #define SFTP_EXT_FSYNC 0x00000010 #define SFTP_EXT_LSETSTAT 0x00000020 +#define SFTP_EXT_LIMITS 0x00000040 u_int exts; u_int64_t limit_kbps; struct bwlimit bwlimit_in, bwlimit_out; @@ -405,8 +412,10 @@ do_init(int fd_in, int fd_out, u_int transfer_buflen, u_int num_requests, ret->msg_id = 1; ret->fd_in = fd_in; ret->fd_out = fd_out; - ret->transfer_buflen = transfer_buflen; - ret->num_requests = num_requests; + ret->transfer_buflen + transfer_buflen ? transfer_buflen : DEFAULT_COPY_BUFLEN; + ret->num_requests + num_requests ? num_requests : DEFAULT_NUM_REQUESTS; ret->exts = 0; ret->limit_kbps = 0; @@ -418,8 +427,6 @@ do_init(int fd_in, int fd_out, u_int transfer_buflen, u_int num_requests, send_msg(ret, msg); - sshbuf_reset(msg); - get_msg_extended(ret, msg, 1); /* Expecting a VERSION reply */ @@ -471,6 +478,10 @@ do_init(int fd_in, int fd_out, u_int transfer_buflen, u_int num_requests, strcmp((char *)value, "1") == 0) { ret->exts |= SFTP_EXT_LSETSTAT; known = 1; + } else if (strcmp(name, "limits at openssh.com") == 0 && + strcmp((char *)value, "1") == 0) { + ret->exts |= SFTP_EXT_LIMITS; + known = 1; } if (known) { debug2("Server supports extension \"%s\" revision %s", @@ -484,6 +495,33 @@ do_init(int fd_in, int fd_out, u_int transfer_buflen, u_int num_requests, sshbuf_free(msg); + /* Query the server for its limits */ + if (ret->exts & SFTP_EXT_LIMITS) { + struct sftp_limits limits; + if (do_limits(ret, &limits) != 0) + fatal_f("limits failed"); + + /* If the caller did not specify, find a good value */ + if (transfer_buflen == 0) { + /* TODO: We should have sep read & write limits */ + ret->transfer_buflen + MINIMUM(limits.read_length, limits.write_length); + debug("Using smaller of server transfer limits " + "[read:%llu, write:%llu] -> %u", + (unsigned long long)limits.read_length, + (unsigned long long)limits.write_length, + ret->transfer_buflen); + } + + /* Use the server limit to scale down our value only */ + if (num_requests == 0 && limits.open_handles) { + ret->num_requests + MINIMUM(DEFAULT_NUM_REQUESTS, limits.open_handles); + debug("Server handle limit %llu; using %u", + (unsigned long long)limits.open_handles, ret->num_requests); + } + } + /* Some filexfer v.0 servers don't support large packets */ if (ret->version == 0) ret->transfer_buflen = MINIMUM(ret->transfer_buflen, 20480); @@ -505,6 +543,56 @@ sftp_proto_version(struct sftp_conn *conn) return conn->version; } +int +do_limits(struct sftp_conn *conn, struct sftp_limits *limits) +{ + u_int id, msg_id; + u_char type; + struct sshbuf *msg; + int r; + + if ((conn->exts & SFTP_EXT_LIMITS) == 0) { + error("Server does not support limits at openssh.com extension"); + return -1; + } + + if ((msg = sshbuf_new()) == NULL) + fatal_f("sshbuf_new failed"); + + id = conn->msg_id++; + if ((r = sshbuf_put_u8(msg, SSH2_FXP_EXTENDED)) != 0 || + (r = sshbuf_put_u32(msg, id)) != 0 || + (r = sshbuf_put_cstring(msg, "limits at openssh.com")) != 0) + fatal_fr(r, "compose"); + send_msg(conn, msg); + debug3("Sent message limits at openssh.com I:%u", id); + + get_msg(conn, msg); + + if ((r = sshbuf_get_u8(msg, &type)) != 0 || + (r = sshbuf_get_u32(msg, &msg_id)) != 0) + fatal_fr(r, "parse"); + + debug3("Received limits reply T:%u I:%u", type, msg_id); + if (id != msg_id) + fatal("ID mismatch (%u != %u)", msg_id, id); + if (type != SSH2_FXP_EXTENDED_REPLY) { + fatal("Expected SSH2_FXP_EXTENDED_REPLY(%u) packet, got %u", + SSH2_FXP_EXTENDED_REPLY, type); + } + + memset(limits, 0, sizeof(*limits)); + if ((r = sshbuf_get_u64(msg, &limits->packet_length)) != 0 || + (r = sshbuf_get_u64(msg, &limits->read_length)) != 0 || + (r = sshbuf_get_u64(msg, &limits->write_length)) != 0 || + (r = sshbuf_get_u64(msg, &limits->open_handles)) != 0) + fatal_fr(r, "parse limits"); + + sshbuf_free(msg); + + return 0; +} + int do_close(struct sftp_conn *conn, const u_char *handle, u_int handle_len) { diff --git a/sftp-client.h b/sftp-client.h index 63a9b8b13bdc..51ba72a3ab22 100644 --- a/sftp-client.h +++ b/sftp-client.h @@ -53,6 +53,14 @@ struct sftp_statvfs { u_int64_t f_namemax; }; +/* Used for limits response on the wire from the server */ +struct sftp_limits { + u_int64_t packet_length; + u_int64_t read_length; + u_int64_t write_length; + u_int64_t open_handles; +}; + /* * Initialise a SSH filexfer connection. Returns NULL on error or * a pointer to a initialized sftp_conn struct on success. @@ -61,6 +69,9 @@ struct sftp_conn *do_init(int, int, u_int, u_int, u_int64_t); u_int sftp_proto_version(struct sftp_conn *); +/* Query server limits */ +int do_limits(struct sftp_conn *, struct sftp_limits *); + /* Close file referred to by 'handle' */ int do_close(struct sftp_conn *, const u_char *, u_int); diff --git a/sftp.c b/sftp.c index b641be2bc4c7..b63f43b5af48 100644 --- a/sftp.c +++ b/sftp.c @@ -70,9 +70,6 @@ typedef void EditLine; #include "sftp-common.h" #include "sftp-client.h" -#define DEFAULT_COPY_BUFLEN 32768 /* Size of buffer for up/download */ -#define DEFAULT_NUM_REQUESTS 64 /* # concurrent outstanding requests */ - /* File to read commands from */ FILE* infile; @@ -2386,8 +2383,8 @@ main(int argc, char **argv) extern int optind; extern char *optarg; struct sftp_conn *conn; - size_t copy_buffer_len = DEFAULT_COPY_BUFLEN; - size_t num_requests = DEFAULT_NUM_REQUESTS; + size_t copy_buffer_len = 0; + size_t num_requests = 0; long long limit_kbps = 0; /* Ensure that fds 0, 1 and 2 are open or directed to /dev/null */ -- 2.28.0
Mike Frysinger
2020-Nov-30 04:57 UTC
[PATCH 4/5] sftp-client: split download/upload buffer sizes
Now that the server will tell us its limits, we can use those to select good upload & download sizes since they are usually very different. In practice, the default download is still 64KiB (due to the server), but the upload increases from 64KiB to 255KiB when using newer OpenSSH servers. --- sftp-client.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/sftp-client.c b/sftp-client.c index ef3906477bd5..62397ed62503 100644 --- a/sftp-client.c +++ b/sftp-client.c @@ -83,7 +83,8 @@ extern int showprogress; struct sftp_conn { int fd_in; int fd_out; - u_int transfer_buflen; + u_int download_buflen; + u_int upload_buflen; u_int num_requests; u_int version; u_int msg_id; @@ -412,7 +413,7 @@ do_init(int fd_in, int fd_out, u_int transfer_buflen, u_int num_requests, ret->msg_id = 1; ret->fd_in = fd_in; ret->fd_out = fd_out; - ret->transfer_buflen + ret->download_buflen = ret->upload_buflen transfer_buflen ? transfer_buflen : DEFAULT_COPY_BUFLEN; ret->num_requests num_requests ? num_requests : DEFAULT_NUM_REQUESTS; @@ -503,14 +504,10 @@ do_init(int fd_in, int fd_out, u_int transfer_buflen, u_int num_requests, /* If the caller did not specify, find a good value */ if (transfer_buflen == 0) { - /* TODO: We should have sep read & write limits */ - ret->transfer_buflen - MINIMUM(limits.read_length, limits.write_length); - debug("Using smaller of server transfer limits " - "[read:%llu, write:%llu] -> %u", - (unsigned long long)limits.read_length, - (unsigned long long)limits.write_length, - ret->transfer_buflen); + ret->download_buflen = limits.read_length; + ret->upload_buflen = limits.write_length; + debug("Using server download size %u", ret->download_buflen); + debug("Using server upload size %u", ret->upload_buflen); } /* Use the server limit to scale down our value only */ @@ -523,15 +520,17 @@ do_init(int fd_in, int fd_out, u_int transfer_buflen, u_int num_requests, } /* Some filexfer v.0 servers don't support large packets */ - if (ret->version == 0) - ret->transfer_buflen = MINIMUM(ret->transfer_buflen, 20480); + if (ret->version == 0) { + ret->download_buflen = MINIMUM(ret->download_buflen, 20480); + ret->upload_buflen = MINIMUM(ret->upload_buflen, 20480); + } ret->limit_kbps = limit_kbps; if (ret->limit_kbps > 0) { bandwidth_limit_init(&ret->bwlimit_in, ret->limit_kbps, - ret->transfer_buflen); + ret->download_buflen); bandwidth_limit_init(&ret->bwlimit_out, ret->limit_kbps, - ret->transfer_buflen); + ret->upload_buflen); } return ret; @@ -1326,7 +1325,7 @@ do_download(struct sftp_conn *conn, const char *remote_path, else size = 0; - buflen = conn->transfer_buflen; + buflen = conn->download_buflen; if ((msg = sshbuf_new()) == NULL) fatal_f("sshbuf_new failed"); @@ -1791,7 +1790,7 @@ do_upload(struct sftp_conn *conn, const char *local_path, } startid = ackid = id + 1; - data = xmalloc(conn->transfer_buflen); + data = xmalloc(conn->upload_buflen); /* Read from local and write to remote */ offset = progress_counter = (resume ? c->size : 0); @@ -1811,7 +1810,7 @@ do_upload(struct sftp_conn *conn, const char *local_path, if (interrupted || status != SSH2_FX_OK) len = 0; else do - len = read(local_fd, data, conn->transfer_buflen); + len = read(local_fd, data, conn->upload_buflen); while ((len == -1) && (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK)); -- 2.28.0
Mike Frysinger
2020-Nov-30 04:57 UTC
[PATCH 5/5] sftp-server: increase max "read" size to 255 KiB
The server max packet size has been 256 KiB for a long time. Using a read limit of 64 KiB when we already pulled in 256 KiB doesn't make much sense, and is awfully low with modern computing. Set the limit to just below what the server already accepts for its packets. --- sftp-server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sftp-server.c b/sftp-server.c index c9500d9900b2..5e5445ebe73f 100644 --- a/sftp-server.c +++ b/sftp-server.c @@ -55,7 +55,7 @@ char *sftp_realpath(const char *, char *); /* sftp-realpath.c */ /* Maximum data read that we are willing to accept */ -#define SFTP_MAX_READ_LENGTH (64 * 1024) +#define SFTP_MAX_READ_LENGTH (SFTP_MAX_MSG_LENGTH - 1024) /* Our verbosity */ static LogLevel log_level = SYSLOG_LEVEL_ERROR; -- 2.28.0
ping on this patch series ... https://bugzilla.mindrot.org/show_bug.cgi?id=2949 https://lists.mindrot.org/pipermail/openssh-unix-dev/2020-November/038977.html https://lists.mindrot.org/pipermail/openssh-unix-dev/2020-November/038975.html https://lists.mindrot.org/pipermail/openssh-unix-dev/2020-November/038979.html https://lists.mindrot.org/pipermail/openssh-unix-dev/2020-November/038978.html https://lists.mindrot.org/pipermail/openssh-unix-dev/2020-November/038976.html -mike