[PATCH 00/16] Improvements, mainly to user name handling and scp.
Konstantin Tokarev
annulen at yandex.ru
Wed Dec 23 18:48:39 AWST 2015
08.12.2015, 01:48, "Michael Witten" <mfwitten at gmail.com>:
> 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;
> @@ -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)
> +arg_setup(const char *user_at_host, const char *cmd)
> {
> - replacearg(&args, 0, "%s", ssh_program);
> - if (remuser != NULL)
> - addargs(&args, "-l%s", remuser);
> - addargs(&args, "%s", host);
> + addargs(&args, "%s", user_at_host);
> addargs(&args, "%s", cmd);
> }
>
> int
> -do_cmd(char *host, char *remuser, char *cmd, int *fdin, int *fdout, int argc)
> +do_cmd(const char *user_at_host, const char *cmd, int *fdin, int *fdout, int argc)
> {
> int pin[2], pout[2], reserved[2];
>
> if (verbose_mode)
> fprintf(stderr,
> - "Executing: program %s host %s, user %s, command %s\n",
> - ssh_program, host,
> - remuser ? remuser : "(unspecified)", cmd);
> + "Executing: program %s, login %s, command %s\n",
> + ssh_program, user_at_host, cmd);
>
> /*
> * Reserve two descriptors so that the real pipes won't get
> @@ -211,7 +207,7 @@
> /* uClinux needs to build the args here before vforking,
> otherwise we do it later on. */
> #ifdef USE_VFORK
> - arg_setup(host, remuser, cmd);
> + arg_setup(user_at_host, cmd);
> #endif
>
> /* Fork a child to execute the command on the remote host using ssh. */
> @@ -231,7 +227,7 @@
> close(pout[1]);
>
> #ifndef USE_VFORK
> - arg_setup(host, remuser, cmd);
> + arg_setup(user_at_host, cmd);
> #endif
>
> execvp(ssh_program, args.list);
> @@ -251,16 +247,10 @@
> xfree(args.list[args.num-1]);
> args.list[args.num-1]=NULL;
> args.num--;
> - /* pop host */
> + /* pop user_at_host */
> xfree(args.list[args.num-1]);
> args.list[args.num-1]=NULL;
> args.num--;
> - /* pop user */
> - if (remuser != NULL) {
> - xfree(args.list[args.num-1]);
> - args.list[args.num-1]=NULL;
> - args.num--;
> - }
> #endif
>
> /* Parent. Close the other side, and return the local side. */
> @@ -282,12 +272,9 @@
> BUF *allocbuf(BUF *, int, int);
> void lostconn(int);
> void nospace(void);
> -int okname(char *);
> void run_err(const char *,...);
> void verifydir(char *);
>
> -struct passwd *pwd;
> -uid_t userid;
> int errs, remin, remout;
> int pflag, iamremote, iamrecursive, targetshouldbedirectory;
>
> @@ -362,7 +349,9 @@
> ssh_program = xstrdup(optarg);
> break;
> case 'v':
> +#ifdef DEBUG_TRACE
> addargs(&args, "-v");
> +#endif
> verbose_mode = 1;
> break;
> case 'q':
> @@ -393,9 +382,6 @@
> argc -= optind;
> argv += optind;
>
> - if ((pwd = getpwuid(userid = getuid())) == NULL)
> - fatal("unknown user %u", (u_int) userid);
> -
> if (!isatty(STDERR_FILENO))
> showprogress = 0;
>
> @@ -459,7 +445,8 @@
> toremote(char *targ, int argc, char **argv)
> {
> int i, len;
> - char *bp, *host, *src, *suser, *thost, *tuser, *arg;
> + const char *targ_user_at_host;
> + char *bp, *src;
> arglist alist;
>
> memset(&alist, '\0', sizeof(alist));
> @@ -469,30 +456,17 @@
> if (*targ == 0)
> targ = ".";
>
> - arg = xstrdup(argv[argc - 1]);
> - if ((thost = strrchr(arg, '@'))) {
> - /* user at host */
> - *thost++ = 0;
> - tuser = arg;
> - if (*tuser == '\0')
> - tuser = NULL;
> - } else {
> - thost = arg;
> - tuser = NULL;
> - }
> -
> - if (tuser != NULL && !okname(tuser)) {
> - xfree(arg);
> - return;
> - }
> + targ_user_at_host = argv[argc - 1];
>
> for (i = 0; i < argc - 1; i++) {
> src = colon(argv[i]);
> if (src) { /* remote to remote */
> freeargs(&alist);
> addargs(&alist, "%s", ssh_program);
> +#ifdef DEBUG_TRACE
> if (verbose_mode)
> addargs(&alist, "-v");
> +#endif
> #if 0
> /* Disabled since dbclient won't understand them
> and scp works fine without them. */
> @@ -504,27 +478,11 @@
> *src++ = 0;
> if (*src == 0)
> src = ".";
> - host = strrchr(argv[i], '@');
> -
> - if (host) {
> - *host++ = 0;
> - host = cleanhostname(host);
> - suser = argv[i];
> - if (*suser == '\0')
> - suser = pwd->pw_name;
> - else if (!okname(suser))
> - continue;
> - addargs(&alist, "-l");
> - addargs(&alist, "%s", suser);
> - } else {
> - host = cleanhostname(argv[i]);
> - }
> - addargs(&alist, "%s", host);
> +
> + addargs(&alist, "%s", argv[i]); /* [user@]host */
> addargs(&alist, "%s", cmd);
> addargs(&alist, "%s", src);
> - addargs(&alist, "%s%s%s:%s",
> - tuser ? tuser : "", tuser ? "@" : "",
> - thost, targ);
> + addargs(&alist, "%s:%s", targ_user_at_host, targ);
> if (do_local_cmd(&alist) != 0)
> errs = 1;
> } else { /* local to remote */
> @@ -532,8 +490,7 @@
> len = strlen(targ) + CMDNEEDS + 20;
> bp = xmalloc(len);
> (void) snprintf(bp, len, "%s -t %s", cmd, targ);
> - host = cleanhostname(thost);
> - if (do_cmd(host, tuser, bp, &remin,
> + if (do_cmd(targ_user_at_host, bp, &remin,
> &remout, argc) < 0)
> exit(1);
> if (response() < 0)
> @@ -549,7 +506,7 @@
> tolocal(int argc, char **argv)
> {
> int i, len;
> - char *bp, *host, *src, *suser;
> + char *bp, *src;
> arglist alist;
>
> memset(&alist, '\0', sizeof(alist));
> @@ -572,20 +529,10 @@
> *src++ = 0;
> if (*src == 0)
> src = ".";
> - if ((host = strrchr(argv[i], '@')) == NULL) {
> - host = argv[i];
> - suser = NULL;
> - } else {
> - *host++ = 0;
> - suser = argv[i];
> - if (*suser == '\0')
> - suser = pwd->pw_name;
> - }
> - host = cleanhostname(host);
> len = strlen(src) + CMDNEEDS + 20;
> bp = xmalloc(len);
> (void) snprintf(bp, len, "%s -f %s", cmd, src);
> - if (do_cmd(host, suser, bp, &remin, &remout, argc) < 0) {
> + if (do_cmd(argv[i], bp, &remin, &remout, argc) < 0) {
> (void) xfree(bp);
> ++errs;
> continue;
> @@ -1190,36 +1137,6 @@
> killchild(0);
> }
>
> -int
> -okname(char *cp0)
> -{
> - int c;
> - char *cp;
> -
> - cp = cp0;
> - do {
> - c = (int)*cp;
> - if (c & 0200)
> - goto bad;
> - if (!isalpha(c) && !isdigit(c)) {
> - switch (c) {
> - case '\'':
> - case '"':
> - case '`':
> - case ' ':
> - case '#':
> - goto bad;
> - default:
> - break;
> - }
> - }
> - } while (*++cp);
> - return (1);
> -
> -bad: fprintf(stderr, "%s: invalid user name\n", cp0);
> - return (0);
> -}
> -
> BUF *
> allocbuf(BUF *bp, int fd, int blksize)
> {
> --- a/scpmisc.c
> +++ b/scpmisc.c
> @@ -104,16 +104,6 @@
> }
>
> char *
> -cleanhostname(char *host)
> -{
> - if (*host == '[' && host[strlen(host) - 1] == ']') {
> - host[strlen(host) - 1] = '\0';
> - return (host + 1);
> - } else
> - return host;
> -}
> -
> -char *
> colon(char *cp)
> {
> int flag = 0;
> @@ -165,26 +155,6 @@
> }
>
> void
> -replacearg(arglist *args, u_int which, char *fmt, ...)
> -{
> - va_list ap;
> - char *cp;
> - int r;
> -
> - va_start(ap, fmt);
> - r = vasprintf(&cp, fmt, ap);
> - va_end(ap);
> - if (r == -1)
> - fatal("replacearg: argument too long");
> -
> - if (which >= args->num)
> - fatal("replacearg: tried to replace invalid arg %d >= %d",
> - which, args->num);
> - xfree(args->list[which]);
> - args->list[which] = cp;
> -}
> -
> -void
> freeargs(arglist *args)
> {
> u_int i;
> @@ -223,6 +193,7 @@
> va_start(args, fmt);
> vfprintf(stderr, fmt, args);
> va_end(args);
> + fputc('\n', stderr);
> exit(255);
> }
>
> --- a/scpmisc.h
> +++ b/scpmisc.h
> @@ -21,7 +21,6 @@
> void unset_nonblock(int);
> void set_nodelay(int);
> int a2port(const char *);
> -char *cleanhostname(char *);
> char *colon(char *);
> long convtime(const char *);
>
> @@ -34,7 +33,6 @@
> int nalloc;
> };
> void addargs(arglist *, char *, ...);
> -void replacearg(arglist *, u_int, char *, ...);
> void freeargs(arglist *);
>
> /* from xmalloc.h */
--
Regards,
Konstantin
More information about the Dropbear
mailing list