You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Mladen Turk <mt...@apache.org> on 2004/08/27 10:30:17 UTC
[PATCH] set process attribute uid (WAS: [PATCH] WIN32 CreateProcessAsUser)
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.
The win version now makes sure that the calling tread does
not remain under impersonated user if something goes wrong.
Regards,
MT.
Re: [PATCH] set process attribute uid (WAS: [PATCH] WIN32 CreateProcessAsUser)
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, 27 Aug 2004 13:00:07 +0200, Mladen Turk <mt...@apache.org> wrote:
> Jeff Trawick wrote:
>
> >>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
> >
>
> That's the only way AFAICT to enable the 'RunAs'.
> The calling thread is switching to a different account only to call
> the single API function and then reverts. Other threads are not
> affected, even if this is a main thread.
okay, just me being ignorant; sorry
> >>+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?
> >
>
> OK. Any suggestons for the name of the second param?
> Perhaps grp_or_pwd?
I was thinking along the lines of a third parameter instead of
overloading the second parameter. Any comments from the crowd on that
choice?
Re: [PATCH] set process attribute uid (WAS: [PATCH] WIN32 CreateProcessAsUser)
Posted by Mladen Turk <mt...@apache.org>.
Jeff Trawick wrote:
>>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 ;)
>
OK, I'll test on (have only FreeBSD) unix and then enable.
>
>>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
>
That's the only way AFAICT to enable the 'RunAs'.
The calling thread is switching to a different account only to call
the single API function and then reverts. Other threads are not
affected, even if this is a main thread.
The actual code comes from jakarta-commons/daemon's procrun, that
I wrote and it drives the Tomcat as a service, so It's quite tested :).
>>+ */
>>+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?
>
OK. Any suggestons for the name of the second param?
Perhaps grp_or_pwd?
>
>>--- 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
>
OK. Seems reasonable.
Regards,
MT.
Re: [PATCH] set process attribute uid (WAS: [PATCH] WIN32 CreateProcessAsUser)
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, 27 Aug 2004 10:30:17 +0200, Mladen Turk <mt...@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