Hi Jan,
I have few comments...
On Thu, 2009-05-14 at 11:49 +0200, Jan Friesse wrote:
>
>
>
>
>
> differences
> between files
> attachment
> (corosync-support-for-uidgid-try2.patch)
>
> diff --git a/trunk/exec/main.c b/trunk/exec/main.c
> index db22e96..2b41111 100644
> --- a/trunk/exec/main.c
> +++ b/trunk/exec/main.c
> @@ -138,6 +138,18 @@ static void sigusr2_handler (int num)
> }
> }
>
> +static void corosync_remove_uidgid_list (void) {
> + struct list_head *iter;
> +
> + for (iter = ug_config.uidgid_list.next; iter !=
> &ug_config.uidgid_list; ) {
> + struct uidgid_item *ugi = list_entry (iter, struct
> uidgid_item, list);
> + iter = iter->next;
> +
> + list_del (&ugi->list);
> + free (ugi);
> + }
> +}
> +
> /*
> * TODO this function needs some love
> */
> @@ -150,6 +162,10 @@ void corosync_request_shutdown (void)
> poll_stop (0);
> totempg_finalize ();
> coroipcs_ipc_exit ();
> +
> + /*Remove uidgid_list*/
> + corosync_remove_uidgid_list ();
Is there really a need to free this list on exit? we are about to abort
anyway and the kernel will take care of that.
> +
> corosync_exit_error (AIS_DONE_EXIT);
> }
>
> @@ -482,12 +498,18 @@ static coroipcs_handler_fn_lvalue
> corosync_handler_fn_get (unsigned int service,
>
> static int corosync_security_valid (int euid, int egid)
> {
> + struct list_head *iter;
> if (euid == 0 || egid == 0) {
> return (1);
> }
> - if (euid == ug_config.uid || egid == ug_config.gid) {
> - return (1);
> +
> + for (iter = ug_config.uidgid_list.next; iter !=
> &ug_config.uidgid_list; iter = iter->next) {
> + struct uidgid_item *ugi = list_entry (iter, struct
> uidgid_item, list);
> +
> + if (euid == ugi->uid || egid == ugi->gid)
> + return (1);
> }
> +
> return (0);
> }
It's entirely possible that I misunderstood but aren't we looking here
to allow a certain process name to access IPC via certain uid/gid?
Let say that we drop a file in /etc/corosync/uidgid.d/cman with uid/gid
foo:foo, it means to me that a process called cman (or identified as
such) is allowed to access corosync IPC with uid/gid foo:foo.
With this check I can see the possibility that a user X can get access
to IPC because somebody else did drop a file in there... not really sure
that's what you want.
>
> diff --git a/trunk/exec/mainconfig.c b/trunk/exec/mainconfig.c
> index 79e01cd..eb0563c 100644
> --- a/trunk/exec/mainconfig.c
> +++ b/trunk/exec/mainconfig.c
> @@ -671,6 +671,54 @@ static void add_logsys_config_notification(
>
> }
>
> +static int corosync_main_config_read_uidgid (
> + struct objdb_iface_ver0 *objdb,
> + const char **error_string,
> + struct ug_config *ug_config)
> +{
> + hdb_handle_t object_find_handle;
> + hdb_handle_t object_service_handle;
> + char *value;
> + int uid, gid;
> + struct uidgid_item *ugi;
> +
> + list_init (&ug_config->uidgid_list);
> +
> + objdb->object_find_create (
> + OBJECT_PARENT_HANDLE,
> + "uidgid",
> + strlen ("uidgid"),
> + &object_find_handle);
> +
> + while (objdb->object_find_next (
> + object_find_handle,
> + &object_service_handle) == 0) {
> + uid = -1;
> + gid = -1;
> +
> + if (!objdb_get_string (objdb,object_service_handle,
> "uid", &value)) {
> + uid = uid_determine(value);
> + }
> +
> + if (!objdb_get_string (objdb,object_service_handle,
> "gid", &value)) {
> + gid = gid_determine(value);
> + }
> +
> + if (uid > -1 || gid > -1) {
> + ugi = malloc (sizeof (*ugi));
> + if (ugi == NULL) {
> + _corosync_out_of_memory_error();
> + }
> + ugi->uid = uid;
> + ugi->gid = gid;
> + list_add (&ugi->list,
> &ug_config->uidgid_list);
> + }
> + }
> + objdb->object_find_destroy (object_find_handle);
> +
> + return 0;
> +}
> +
For the sake of tracking, it would be a good idea to have a record of
what service/file did open for a certain uid/gid set.
So perhaps add an objdb entry with the name of the file and record the
values both as strings and numeric (easier to read in debugging).
It adds a bit of complexity to code but imho it's worth the pain.
For example:
/etc/corosync/uidgid.d/foo
contains:
uidgid {
uid: root
gid: whatever
}
it will automatically translate to (into the objdb):
uidgid {
uid: 0
gid: $random_number
service: foo
file: /etc/corosync/uidgid.d/foo
uid_requested: root (uid_r or whatever keyname)
gid_requested: whatever ^^ same as above
}
a configfile could also contain more than one set:
cat /etc/corosync/uidgid.d/many
uidgid {
service: foo
uid: root
gid: root
}
uidgid {
service: cman
uid: ais
gid: ais
}
that would translate to this in the objdb:
uidgid {
uid: 0
gid: 0
service: foo
file: /etc/corosync/uidgid.d/many
uid_requested: root (uid_r or whatever keyname)
gid_requested: root ^^ same as above
}
uidgid {
uid: $random_number
gid: $random_number
service: cman
file: /etc/corosync/uidgid.d/many
uid_requested: ais (uid_r or whatever keyname)
gid_requested: ais ^^ same as above
}
> diff --git a/trunk/exec/mainconfig.h b/trunk/exec/mainconfig.h
> index c9ab7ea..26135ad 100644
> --- a/trunk/exec/mainconfig.h
> +++ b/trunk/exec/mainconfig.h
> @@ -37,6 +37,7 @@
>
> #include <corosync/engine/objdb.h>
> #include <corosync/engine/logsys.h>
> +#include <corosync/list.h>
>
> /*
> * All service handlers in the AIS
> @@ -49,14 +50,29 @@ struct dynamic_service {
> };
> #define MAX_DYNAMIC_SERVICES 128
>
> +/*
> + * Structure describing cached uidgid item
> + */
> +struct uidgid_item {
> + struct list_head list;
> + int uid;
> + int gid;
> +};
> +
> struct ug_config {
> /*
> * user/group to run as
> */
> int uid;
> int gid;
> +
> + /*
> + * Allowed users/group to connect. This is of type uidgid
> item.
> + */
> + struct list_head uidgid_list;
> };
Do we need to have 2 structs? at this point one should be plenty?
struct uidgid {
struct list_head list;
int uid;
int gid;
}
and init the first one to match corosync default conf.
The loader should make sure to filter the config entries to load only
uidgid entries (maybe it's already there and I haven't noticed).
Fabio
_______________________________________________
Openais mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/openais
