Re: [PATCH 00/16] Improvements, mainly to user name handling and scp.

[Michael Witten]

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.

[Michael Witten]

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.

[Michael Witten]

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.

[Matt Johnston]

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
> +++ 

Re: [PATCH 00/16] Improvements, mainly to user name handling and scp.

[Konstantin Tokarev]

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;
>
>