Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Message-ID: <cc67648e0408270327723289d4@mail.gmail.com>
Date: Fri, 27 Aug 2004 06:27:58 -0400
From: Jeff Trawick <trawick@gmail.com>
Reply-To: Jeff Trawick <trawick@gmail.com>
To: dev@apr.apache.org
Subject: Re: [PATCH] set process attribute uid (WAS: [PATCH] WIN32
 CreateProcessAsUser)
In-Reply-To: <412EF119.8090503@apache.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
References: <412EF119.8090503@apache.org>

On Fri, 27 Aug 2004 10:30:17 +0200, Mladen Turk <mturk@apache.org> wrote:
> Hi,
> 
> Discussing with wrowe, I've changed the patch a bit.
> The apr_proc_attr_user_set is now visible on all platforms,
> and on most of them it is ENOTIMPL.
> The unix version uses apr_uid_get to obtain the uid and gid,
> but the actual code is noop for now.

either return ENOTIMPL and don't bother putting in dead, untested
code, or activate the code and hope for the best; the latter is how
many things begin working ;)

> The win version now makes sure that the calling tread does
> not remain under impersonated user if something goes wrong.

it seems very uncool for apr_procattr_FOO_set to modify any
characteristics of the calling thread/process...  is that happening on
Win32?  there's quite a bit of interesting logic in
apr_procattr_user_set for Win32

> Index: include/apr_thread_proc.h
> +/**
> + * Set the username and password used for creating process.
> + * @param attr The procattr we care about.
> + * @param username Username for creating process.
> + * @param passwor Password for username if required
                             typo here (missing "d" at end of password)

> + */
> +APR_DECLARE(apr_status_t) apr_procattr_user_set(apr_procattr_t *attr,
> +                                                const char *username,
> +                                                const char *password);

maybe this should allow specifying a group too, in case on Unix you
don't want to require that the identity change to the default group of
the specified user?

> --- test/testproc.c     14 May 2004 14:43:22 -0000      1.46
> +++ test/testproc.c     27 Aug 2004 08:18:04 -0000
> @@ -21,6 +21,11 @@
>  #include "testutil.h"
> 
>  #define TESTSTR "This is a test"
> +/* You will need to create an account with
> + * 'Replace a process level token' priviledge set.
> + */
> +#define USERNAME "test"
> +#define PASSWORD "test"

shouldn't this be an optional aspect of the test?  folks will want to
be able to run the test suite without adding new accounts

> Index: threadproc/unix/proc.c
> ===================================================================
> RCS file: /home/cvspublic/apr/threadproc/unix/proc.c,v
> retrieving revision 1.75
> diff -u -r1.75 proc.c
> --- threadproc/unix/proc.c      20 Jul 2004 03:31:57 -0000      1.75
> +++ threadproc/unix/proc.c      27 Aug 2004 08:10:39 -0000
> @@ -181,6 +181,23 @@
>      return APR_SUCCESS;
>  }
> 
> +APR_DECLARE(apr_status_t) apr_procattr_user_set(apr_procattr_t *attr,
> +                                                const char *username,
> +                                                const char *password)
> +{
> +    apr_status_t rv;
> +
> +    attr->userid  = apr_palloc(attr->pool, sizeof(apr_uid_t));
> +    attr->groupid = apr_palloc(attr->pool, sizeof(apr_gid_t));
> +
> +    if ((rv = apr_uid_get(attr->userid, attr->groupid,
> +                          username, attr->pool)) != APR_SUCCESS) {
> +        attr->userid  = NULL;
> +        attr->groupid = NULL;
> +    }
> +    return rv;
> +}
> +
>  APR_DECLARE(apr_status_t) apr_proc_fork(apr_proc_t *proc, apr_pool_t *pool)
>  {
>      int pid;
> @@ -325,7 +342,17 @@
>      else if (new->pid == 0) {
>          int status;
>          /* child process */
> -
> +#if 0
> +        /* XXX: Not sure if this is correct */
> +        if (attr->userid) {
> +            if (setuid(*(attr->userid)))
> +                exit(-1);   /* Unable to set the user id */
               this should call any registered errfn here to report
the problem prior to exit()
               
                     if (attr->errfn) {
                         attr->errfn(pool, errno, "setuid() failed");
                     }

> +        }
> +        if (attr->groupid) {
> +            if (sethid(*(attr->groupid)))
                       typo (should be setgid)

               also this should call any registered errfn here to
report the problem