Re: [PATCH 00/16] Improvements, mainly to user name handling and scp.
On Tue 12/1/2016, at 12:37 am, Michael Witten wrote: > Well, even if I go through the trouble of interfacing with this > other community (OpenSSH), and thereby successfully introduce > these changes 'upstream', it sounds like there's still going to > be a lot of work to get the result merged 'downstream' as > part of the project that I actually [and, yet, only marginally] > care about (Dropbear SSH). > > This seems like it's going to be a waste of my time, and I've > already spent a not-insignificant amount of time on this. > > My instinct is that we should view Dropbear's copies as being > what they, in fact, are: forks with their own history and > purpose. I've added a note at the top of the scp files. Even if changes are being stored in a more structured manner they would be best kept minimal - less things to think about. I've pushed a smaller change that should avoid scp requiring valid usernames. I've committed the newline for fatal(). I don't think the change to make cli-main/runopts use get_user_name() is worthwhile - it's easier to reason about program logic if everything is set up at startup rather than lazily evaluated. If it were a case where performance mattered it might be useful, not in this case. Cheers, Matt
Re: [PATCH 00/16] Improvements, mainly to user name handling and scp.
On Mon, 11 Jan 2016 16:37:09 -, Michael Witten wrote: > Message-ID: <2666123da20b47a589b819f37bd4cc54-mfwit...@gmail.com> > In-Reply-To<20151229150334.gb27...@ucc.gu.uwa.edu.au> > References: > <20151229150334.gb27...@ucc.gu.uwa.edu.au> Woops! I apologize for having sent a malformed email; you'll notice that the header 'In-Reply-To' is missing the colon. Please ignore that email, the one with the following Message-ID: <2666123da20b47a589b819f37bd4cc54-mfwit...@gmail.com> Instead, make replies to the email that has the following Message-ID: Terribly sorry, Michael Witten
Re: [PATCH 00/16] Improvements, mainly to user name handling and scp.
On Tue, 29 Dec 2015 23:03:34 +0800, Matt Johnston wrote: > On Mon, Dec 07, 2015 at 10:42:59PM -, Michael Witten wrote: >> User names have hitherto been handled neither consistently nor well; this >> series alleviates at least some of the issues. >> >> Fear not the long patch series! >> >> Most commits involve a fairly small number of changes; while I could have >> consolidated these changes into fewer commits, I think the series as a whole >> provides a better narration for what's going. >> >> Besides a few small improvements along the way, the main thrust is: >> >> * Removing user-name handling from `scp' (in favor of using the >> handling that is already present in `dropbear'/`dbclient'). >> >> * Lazily looking up the current user's name. >> >> * Removing unused code. >> >> Overall, 7 files were changed, with 37 insertions(+) and 158 deletions(-): >> >> cli-main.c | 2 +- >> cli-runopts.c| 32 ++ >> common-session.c | 1 + >> runopts.h| 2 +- >> scp.c| 125 >> ++- >> scpmisc.c| 31 +- >> scpmisc.h| 2 - >> >> This is the series: >> >> [01] scp: Insert comma into stderr message >> [02] scp: Have `fatal()' append a newline to the message >> [03] scp: only pass `-v' when DEBUG_TRACE is set >> [04] scp: `-l%s' -> `-l %s' >> [05] scp: const/static correctness improvements >> [06] scp: Introduce `get_user_name()' >> [07] scp: Use "unknown" when the user name cannot be retrieved >> [08] scp: style: simplify code by using a tertiary operator >> [09] scp: Use `get_user_name()' more often >> [10] scp: Simplify code now that the user name is never `NULL' >> [11] scp: Remove parsing of `[user@]host' >> [12] scp: Remove unused functions >> [13] scp: Remove `replacearg()' >> [14] runopts: Mark `*cli_runopts.own_user' as `const' >> [15] runopts: There's no reason to make a duplicate of "unknown" >> [16] runopts: Re-introduce the `get_user_name()' function from `scp' >> development >> >> Lastly, for convenience in reviewing the changes, here is the overall patch: >> >> [ ... ] > > Hi Michael, > > I think the general change of these patches makes sense > (avoiding failure when a local user doesn't exist) but it > needs to be more minimal. scp comes straight from OpenSSH > with some small changes for uClinux etc. I've tried to > avoid additional changes since it really needs updating to a > more recent OpenSSH - the more changes, the larger that work > is. Upstreaming it might be an option. > > Cheers, > Matt Well, even if I go through the trouble of interfacing with this other community (OpenSSH), and thereby successfully introduce these changes 'upstream', it sounds like there's still going to be a lot of work to get the result merged 'downstream' as part of the project that I actually [and, yet, only marginally] care about (Dropbear SSH). This seems like it's going to be a waste of my time, and I've already spent a not-insignificant amount of time on this. My instinct is that we should view Dropbear's copies as being what they, in fact, are: forks with their own history and purpose. Yet, if that view is undesirable, then let's get this set straight now and for the rest of time: * Let's identify all the files that are viewed as being merely patched copies of 'upstream' files. * Let's segregate them obviously, and make clear notes (for the benefit of others) that all substantive changes should be made upstream. + Most portably, these files could be refactored into a subdirectory named 'upstream' or 'third-party'. It would be possible to account for DropBear-specific patches either explicitly (by applying them at build-time), or implicitly (by placing them in a specific branch that is merged with the main line of history via a non-fastforward merge commit). + Alternatively, these files could be included in at least one git submodule (or the mercurial equivalent); this submodule could then track the patches explicitly, and provide a focus for discussing Dropbear-specific patches and upstream synchronization. * Let's bring those 'upstream' copies up-to-date. Only after this effort, will it make sense to me to try to introduce my patches upstream in order to have them merged downstream. Sincerely, Michael Witten
Re: [PATCH 00/16] Improvements, mainly to user name handling and scp.
In-Reply-To<20151229150334.gb27...@ucc.gu.uwa.edu.au> References: <20151229150334.gb27...@ucc.gu.uwa.edu.au> On Tue, 29 Dec 2015 23:03:34 +0800, Matt Johnston wrote: > On Mon, Dec 07, 2015 at 10:42:59PM -, Michael Witten wrote: >> User names have hitherto been handled neither consistently nor well; this >> series alleviates at least some of the issues. >> >> Fear not the long patch series! >> >> Most commits involve a fairly small number of changes; while I could have >> consolidated these changes into fewer commits, I think the series as a whole >> provides a better narration for what's going. >> >> Besides a few small improvements along the way, the main thrust is: >> >> * Removing user-name handling from `scp' (in favor of using the >> handling that is already present in `dropbear'/`dbclient'). >> >> * Lazily looking up the current user's name. >> >> * Removing unused code. >> >> Overall, 7 files were changed, with 37 insertions(+) and 158 deletions(-): >> >> cli-main.c | 2 +- >> cli-runopts.c| 32 ++ >> common-session.c | 1 + >> runopts.h| 2 +- >> scp.c| 125 >> ++- >> scpmisc.c| 31 +- >> scpmisc.h| 2 - >> >> This is the series: >> >> [01] scp: Insert comma into stderr message >> [02] scp: Have `fatal()' append a newline to the message >> [03] scp: only pass `-v' when DEBUG_TRACE is set >> [04] scp: `-l%s' -> `-l %s' >> [05] scp: const/static correctness improvements >> [06] scp: Introduce `get_user_name()' >> [07] scp: Use "unknown" when the user name cannot be retrieved >> [08] scp: style: simplify code by using a tertiary operator >> [09] scp: Use `get_user_name()' more often >> [10] scp: Simplify code now that the user name is never `NULL' >> [11] scp: Remove parsing of `[user@]host' >> [12] scp: Remove unused functions >> [13] scp: Remove `replacearg()' >> [14] runopts: Mark `*cli_runopts.own_user' as `const' >> [15] runopts: There's no reason to make a duplicate of "unknown" >> [16] runopts: Re-introduce the `get_user_name()' function from `scp' >> development >> >> Lastly, for convenience in reviewing the changes, here is the overall patch: >> >> [ ... ] > > Hi Michael, > > I think the general change of these patches makes sense > (avoiding failure when a local user doesn't exist) but it > needs to be more minimal. scp comes straight from OpenSSH > with some small changes for uClinux etc. I've tried to > avoid additional changes since it really needs updating to a > more recent OpenSSH - the more changes, the larger that work > is. Upstreaming it might be an option. > > Cheers, > Matt Well, even if I go through the trouble of interfacing with this other community (OpenSSH), and thereby successfully introduce these changes 'upstream', it sounds like there's still going to be a lot of work to get the result merged 'downstream' as part of the project that I actually [and, yet, only marginally] care about (Dropbear SSH). This seems like it's going to be a waste of my time, and I've already spent a not-insignificant amount of time on this. My instinct is that we should view Dropbear's copies as being what they, in fact, are: forks with their own history and purpose. Yet, if that view is undesirable, then let's get this set straight now and for the rest of time: * Let's identify all the files that are viewed as being merely patched copies of 'upstream' files. * Let's segregate them obviously, and make clear notes (for the benefit of others) that all substantive changes should be made upstream. + Most portably, these files could be refactored into a subdirectory named 'upstream' or 'third-party'. It would be possible to account for DropBear-specific patches either explicitly (by applying them at build-time), or implicitly (by placing them in a specific branch that is merged with the main line of history via a non-fastforward merge commit). + Alternatively, these files could be included in at least one git submodule (or the mercurial equivalent); this submodule could then track the patches explicitly, and provide a focus for discussing Dropbear-specific patches and upstream synchronization. * Let's bring those 'upstream' copies up-to-date. Only after this effort, will it make sense to me to try to introduce my patches upstream in order to have them merged downstream. Sincerely, Michael Witten
Re: [PATCH 00/16] Improvements, mainly to user name handling and scp.
Hi Michael, I think the general change of these patches makes sense (avoiding failure when a local user doesn't exist) but it needs to be more minimal. scp comes straight from OpenSSH with some small changes for uClinux etc. I've tried to avoid additional changes since it really needs updating to a more recent OpenSSH - the more changes, the larger that work is. Upstreaming it might be an option. Cheers, Matt On Mon, Dec 07, 2015 at 10:42:59PM -, Michael Witten wrote: > User names have hitherto been handled neither consistently nor well; this > series alleviates at least some of the issues. > > Fear not the long patch series! > > Most commits involve a fairly small number of changes; while I could have > consolidated these changes into fewer commits, I think the series as a whole > provides a better narration for what's going. > > Besides a few small improvements along the way, the main thrust is: > > * Removing user-name handling from `scp' (in favor of using the > handling that is already present in `dropbear'/`dbclient'). > > * Lazily looking up the current user's name. > > * Removing unused code. > > Overall, 7 files were changed, with 37 insertions(+) and 158 deletions(-): > > cli-main.c | 2 +- > cli-runopts.c| 32 ++ > common-session.c | 1 + > runopts.h| 2 +- > scp.c| 125 > ++- > scpmisc.c| 31 +- > scpmisc.h| 2 - > > This is the series: > > [01] scp: Insert comma into stderr message > [02] scp: Have `fatal()' append a newline to the message > [03] scp: only pass `-v' when DEBUG_TRACE is set > [04] scp: `-l%s' -> `-l %s' > [05] scp: const/static correctness improvements > [06] scp: Introduce `get_user_name()' > [07] scp: Use "unknown" when the user name cannot be retrieved > [08] scp: style: simplify code by using a tertiary operator > [09] scp: Use `get_user_name()' more often > [10] scp: Simplify code now that the user name is never `NULL' > [11] scp: Remove parsing of `[user@]host' > [12] scp: Remove unused functions > [13] scp: Remove `replacearg()' > [14] runopts: Mark `*cli_runopts.own_user' as `const' > [15] runopts: There's no reason to make a duplicate of "unknown" > [16] runopts: Re-introduce the `get_user_name()' function from `scp' > development > > Lastly, for convenience in reviewing the changes, here is the overall patch: > > --- a/cli-main.c > +++ b/cli-main.c > @@ -135,7 +135,7 @@ > static void cli_proxy_cmd(int *sock_in, int *sock_out) { > int ret; > > - fill_passwd(cli_opts.own_user); > + fill_passwd(get_user_name()); > > ret = spawn_command(exec_proxy_cmd, cli_opts.proxycmd, > sock_out, sock_in, NULL, NULL); > --- a/cli-runopts.c > +++ b/cli-runopts.c > @@ -36,7 +36,6 @@ > static void printhelp(); > static void parse_hostname(const char* orighostarg); > static void parse_multihop_hostname(const char* orighostarg, const char* > argv0); > -static void fill_own_user(); > #ifdef ENABLE_CLI_PUBKEY_AUTH > static void loadidentityfile(const char* filename, int warnfail); > #endif > @@ -102,6 +101,17 @@ > > } > > +const char *get_user_name() { > + static const char *user_name = NULL; > + > + if (user_name == NULL) { > + struct passwd *pwd = getpwuid(getuid()); > + user_name = pwd ? m_strdup(pwd->pw_name) : "unknown"; > + } > + > + return user_name; > +} > + > void cli_getopts(int argc, char ** argv) { > unsigned int i, j; > char ** next = 0; > @@ -175,8 +185,6 @@ > opts.keepalive_secs = DEFAULT_KEEPALIVE; > opts.idle_timeout_secs = DEFAULT_IDLE_TIMEOUT; > > - fill_own_user(); > - > for (i = 1; i < (unsigned int)argc; i++) { > /* Handle non-flag arguments such as hostname or commands for > the remote host */ > if (argv[i][0] != '-') > @@ -640,7 +648,7 @@ > } > > if (cli_opts.username == NULL) { > - cli_opts.username = m_strdup(cli_opts.own_user); > + cli_opts.username = m_strdup(get_user_name()); > } > > port = strchr(cli_opts.remotehost, '^'); > @@ -695,22 +703,6 @@ > } > #endif > > -static void fill_own_user() { > - uid_t uid; > - struct passwd *pw = NULL; > - > - uid = getuid(); > - > - pw = getpwuid(uid); > - if (pw && pw->pw_name != NULL) { > - cli_opts.own_user = m_strdup(pw->pw_name); > - } else { > - dropbear_log(LOG_INFO, "Warning: failed to identify current > user. Trying anyway."); > - cli_opts.own_user = m_strdup("unknown"); > - } > - > -} > - > #ifdef ENABLE_CLI_ANYTCPFWD > /* Turn a "[listenaddr:]listenport:remoteaddr:remoteport" string into into a > forwarding > * set, and add it to the forwarding list */ > --- a/common-session.c > +++ b/commo
Re: [PATCH 00/16] Improvements, mainly to user name handling and scp.
08.12.2015, 01:48, "Michael Witten" : > User names have hitherto been handled neither consistently nor well; this > series alleviates at least some of the issues. > > Fear not the long patch series! > > Most commits involve a fairly small number of changes; while I could have > consolidated these changes into fewer commits, I think the series as a whole > provides a better narration for what's going. > > Besides a few small improvements along the way, the main thrust is: > > * Removing user-name handling from `scp' (in favor of using the > handling that is already present in `dropbear'/`dbclient'). > > * Lazily looking up the current user's name. > > * Removing unused code. > > Overall, 7 files were changed, with 37 insertions(+) and 158 deletions(-): > > cli-main.c | 2 +- > cli-runopts.c | 32 ++ > common-session.c | 1 + > runopts.h | 2 +- > scp.c | 125 ++- > scpmisc.c | 31 +- > scpmisc.h | 2 - As can be seen from copyright header and git log, scp.c is a copy of corresponding file from OpenSSH (currently 4.3p2) with a few local changes. It might be a better idea to synchronize with upstream than to diverge it more. Just my 2 cents. > > This is the series: > > [01] scp: Insert comma into stderr message > [02] scp: Have `fatal()' append a newline to the message > [03] scp: only pass `-v' when DEBUG_TRACE is set > [04] scp: `-l%s' -> `-l %s' > [05] scp: const/static correctness improvements > [06] scp: Introduce `get_user_name()' > [07] scp: Use "unknown" when the user name cannot be retrieved > [08] scp: style: simplify code by using a tertiary operator > [09] scp: Use `get_user_name()' more often > [10] scp: Simplify code now that the user name is never `NULL' > [11] scp: Remove parsing of `[user@]host' > [12] scp: Remove unused functions > [13] scp: Remove `replacearg()' > [14] runopts: Mark `*cli_runopts.own_user' as `const' > [15] runopts: There's no reason to make a duplicate of "unknown" > [16] runopts: Re-introduce the `get_user_name()' function from `scp' > development > > Lastly, for convenience in reviewing the changes, here is the overall patch: > > --- a/cli-main.c > +++ b/cli-main.c > @@ -135,7 +135,7 @@ > static void cli_proxy_cmd(int *sock_in, int *sock_out) { > int ret; > > - fill_passwd(cli_opts.own_user); > + fill_passwd(get_user_name()); > > ret = spawn_command(exec_proxy_cmd, cli_opts.proxycmd, > sock_out, sock_in, NULL, NULL); > --- a/cli-runopts.c > +++ b/cli-runopts.c > @@ -36,7 +36,6 @@ > static void printhelp(); > static void parse_hostname(const char* orighostarg); > static void parse_multihop_hostname(const char* orighostarg, const char* > argv0); > -static void fill_own_user(); > #ifdef ENABLE_CLI_PUBKEY_AUTH > static void loadidentityfile(const char* filename, int warnfail); > #endif > @@ -102,6 +101,17 @@ > > } > > +const char *get_user_name() { > + static const char *user_name = NULL; > + > + if (user_name == NULL) { > + struct passwd *pwd = getpwuid(getuid()); > + user_name = pwd ? m_strdup(pwd->pw_name) : "unknown"; > + } > + > + return user_name; > +} > + > void cli_getopts(int argc, char ** argv) { > unsigned int i, j; > char ** next = 0; > @@ -175,8 +185,6 @@ > opts.keepalive_secs = DEFAULT_KEEPALIVE; > opts.idle_timeout_secs = DEFAULT_IDLE_TIMEOUT; > > - fill_own_user(); > - > for (i = 1; i < (unsigned int)argc; i++) { > /* Handle non-flag arguments such as hostname or commands > for the remote host */ > if (argv[i][0] != '-') > @@ -640,7 +648,7 @@ > } > > if (cli_opts.username == NULL) { > - cli_opts.username = m_strdup(cli_opts.own_user); > + cli_opts.username = m_strdup(get_user_name()); > } > > port = strchr(cli_opts.remotehost, '^'); > @@ -695,22 +703,6 @@ > } > #endif > > -static void fill_own_user() { > - uid_t uid; > - struct passwd *pw = NULL; > - > - uid = getuid(); > - > - pw = getpwuid(uid); > - if (pw && pw->pw_name != NULL) { > - cli_opts.own_user = m_strdup(pw->pw_name); > - } else { > - dropbear_log(LOG_INFO, "Warning: failed to identify current user. Trying > anyway."); > - cli_opts.own_user = m_strdup("unknown"); > - } > - > -} > - > #ifdef ENABLE_CLI_ANYTCPFWD > /* Turn a "[listenaddr:]listenport:remoteaddr:remoteport" string into into a > forwarding > * set, and add it to the forwarding list */ > --- a/common-session.c > +++ b/common-session.c > @@ -581,6 +581,7 @@ > return ses.authstate.pw_shell; > } > } > + > void fill_passwd(const char* username) { > struct passwd *pw = NULL; > if (ses.authstate.pw_name) > --- a/runopts.h > +++ b/runopts.h > @@ -127,7 +127,6 @@ > char *remotehost; > char *remoteport; > > - char *own_user; > char *username; > > char *cmd; >
[PATCH 00/16] Improvements, mainly to user name handling and scp.
User names have hitherto been handled neither consistently nor well; this series alleviates at least some of the issues. Fear not the long patch series! Most commits involve a fairly small number of changes; while I could have consolidated these changes into fewer commits, I think the series as a whole provides a better narration for what's going. Besides a few small improvements along the way, the main thrust is: * Removing user-name handling from `scp' (in favor of using the handling that is already present in `dropbear'/`dbclient'). * Lazily looking up the current user's name. * Removing unused code. Overall, 7 files were changed, with 37 insertions(+) and 158 deletions(-): cli-main.c | 2 +- cli-runopts.c| 32 ++ common-session.c | 1 + runopts.h| 2 +- scp.c| 125 ++- scpmisc.c| 31 +- scpmisc.h| 2 - This is the series: [01] scp: Insert comma into stderr message [02] scp: Have `fatal()' append a newline to the message [03] scp: only pass `-v' when DEBUG_TRACE is set [04] scp: `-l%s' -> `-l %s' [05] scp: const/static correctness improvements [06] scp: Introduce `get_user_name()' [07] scp: Use "unknown" when the user name cannot be retrieved [08] scp: style: simplify code by using a tertiary operator [09] scp: Use `get_user_name()' more often [10] scp: Simplify code now that the user name is never `NULL' [11] scp: Remove parsing of `[user@]host' [12] scp: Remove unused functions [13] scp: Remove `replacearg()' [14] runopts: Mark `*cli_runopts.own_user' as `const' [15] runopts: There's no reason to make a duplicate of "unknown" [16] runopts: Re-introduce the `get_user_name()' function from `scp' development Lastly, for convenience in reviewing the changes, here is the overall patch: --- a/cli-main.c +++ b/cli-main.c @@ -135,7 +135,7 @@ static void cli_proxy_cmd(int *sock_in, int *sock_out) { int ret; - fill_passwd(cli_opts.own_user); + fill_passwd(get_user_name()); ret = spawn_command(exec_proxy_cmd, cli_opts.proxycmd, sock_out, sock_in, NULL, NULL); --- a/cli-runopts.c +++ b/cli-runopts.c @@ -36,7 +36,6 @@ static void printhelp(); static void parse_hostname(const char* orighostarg); static void parse_multihop_hostname(const char* orighostarg, const char* argv0); -static void fill_own_user(); #ifdef ENABLE_CLI_PUBKEY_AUTH static void loadidentityfile(const char* filename, int warnfail); #endif @@ -102,6 +101,17 @@ } +const char *get_user_name() { + static const char *user_name = NULL; + + if (user_name == NULL) { + struct passwd *pwd = getpwuid(getuid()); + user_name = pwd ? m_strdup(pwd->pw_name) : "unknown"; + } + + return user_name; +} + void cli_getopts(int argc, char ** argv) { unsigned int i, j; char ** next = 0; @@ -175,8 +185,6 @@ opts.keepalive_secs = DEFAULT_KEEPALIVE; opts.idle_timeout_secs = DEFAULT_IDLE_TIMEOUT; - fill_own_user(); - for (i = 1; i < (unsigned int)argc; i++) { /* Handle non-flag arguments such as hostname or commands for the remote host */ if (argv[i][0] != '-') @@ -640,7 +648,7 @@ } if (cli_opts.username == NULL) { - cli_opts.username = m_strdup(cli_opts.own_user); + cli_opts.username = m_strdup(get_user_name()); } port = strchr(cli_opts.remotehost, '^'); @@ -695,22 +703,6 @@ } #endif -static void fill_own_user() { - uid_t uid; - struct passwd *pw = NULL; - - uid = getuid(); - - pw = getpwuid(uid); - if (pw && pw->pw_name != NULL) { - cli_opts.own_user = m_strdup(pw->pw_name); - } else { - dropbear_log(LOG_INFO, "Warning: failed to identify current user. Trying anyway."); - cli_opts.own_user = m_strdup("unknown"); - } - -} - #ifdef ENABLE_CLI_ANYTCPFWD /* Turn a "[listenaddr:]listenport:remoteaddr:remoteport" string into into a forwarding * set, and add it to the forwarding list */ --- a/common-session.c +++ b/common-session.c @@ -581,6 +581,7 @@ return ses.authstate.pw_shell; } } + void fill_passwd(const char* username) { struct passwd *pw = NULL; if (ses.authstate.pw_name) --- a/runopts.h +++ b/runopts.h @@ -127,7 +127,6 @@ char *remotehost; char *remoteport; - char *own_user; char *username; char *cmd; @@ -165,6 +164,7 @@ extern cli_runopts cli_opts; void cli_getopts(int argc, char ** argv); +const char *get_user_name(); #ifdef ENABLE_USER_ALGO_LIST void parse_ciphers_macs(); --- a/scp.c +++ b/scp.c @@ -172,25 +172,21 @@ */ static void -arg_setup(char *host, char *remuser, char *cmd) +ar