Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-19 Thread Jim Meyering 
Fabio M. Di Nitto wrote:
 @@ -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);
  }

need, maybe not.
But it is generally good to explicitly free otherwise-leaked memory,
even just before exit.

The benefit comes when you use a tool like valgrind to find leaks.
With the explicit free, no leak is reported here.
If you remove the free, valgrind reports the leak
and you or multiple people will have to go investigate it.
Perhaps multiple times.

This becomes especially relevant only once you have
enough of a regression test suite to exercise such code paths.
___
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-19 Thread Joel Becker 
On Tue, May 19, 2009 at 08:45:36AM +0200, Jim Meyering wrote:
 Fabio M. Di Nitto wrote:
  @@ -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);
   }
 
 need, maybe not.
 But it is generally good to explicitly free otherwise-leaked memory,
 even just before exit.
 
 The benefit comes when you use a tool like valgrind to find leaks.
 With the explicit free, no leak is reported here.
 If you remove the free, valgrind reports the leak
 and you or multiple people will have to go investigate it.
 Perhaps multiple times.
 
 This becomes especially relevant only once you have
 enough of a regression test suite to exercise such code paths.

It's even important just to understand what the code is doing.
There's nothing like hunting around for the free of an allocation and
never finding it.
Basically, if you want a particular allocation to be freed by
exit, you mark it such with a comment and never free it, even on a clean
exit.  Otherwise, you try to clean up nicely even in exceptional
circumstances.

Joel

-- 

Lately I've been talking in my sleep.
 Can't imagine what I'd have to say.
 Except my world will be right
 When love comes back my way.

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.bec...@oracle.com
Phone: (650) 506-8127
___
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-19 Thread Fabio M. Di Nitto 
On Tue, 2009-05-19 at 11:07 -0700, Joel Becker wrote:
 On Tue, May 19, 2009 at 08:45:36AM +0200, Jim Meyering wrote:
  Fabio M. Di Nitto wrote:
   @@ -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);
}
  
  need, maybe not.
  But it is generally good to explicitly free otherwise-leaked memory,
  even just before exit.
  
  The benefit comes when you use a tool like valgrind to find leaks.
  With the explicit free, no leak is reported here.
  If you remove the free, valgrind reports the leak
  and you or multiple people will have to go investigate it.
  Perhaps multiple times.
  
  This becomes especially relevant only once you have
  enough of a regression test suite to exercise such code paths.
 
   It's even important just to understand what the code is doing.
 There's nothing like hunting around for the free of an allocation and
 never finding it.
   Basically, if you want a particular allocation to be freed by
 exit, you mark it such with a comment and never free it, even on a clean
 exit.  Otherwise, you try to clean up nicely even in exceptional
 circumstances.

The comment approach sure would be good for corosync as there are plenty
of areas that are not freed on exit. That's also why I didn't really
bother with just one more kind of thing.

Fabio

___
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-19 Thread Joel Becker 
On Tue, May 19, 2009 at 08:38:39PM +0200, Fabio M. Di Nitto wrote:
 The comment approach sure would be good for corosync as there are plenty
 of areas that are not freed on exit. That's also why I didn't really
 bother with just one more kind of thing.

The important point is is it freed on normal exit?  If so, it
should be freed on non-normal exit too.

Joel

-- 

If the human brain were so simple we could understand it, we would
 be so simple that we could not.
- W. A. Clouston

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.bec...@oracle.com
Phone: (650) 506-8127
___
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-15 Thread Jan Friesse 
Fabio,

Fabio M. Di Nitto wrote:
 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..2b4 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.
 

No it's not needed. I can throw it away.

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

More or less, yes. If there will be process cman (or every other) with 
uid or gid foo, it will be able to use corosync. It's only 
ug_config.uid/gid enhancement. See 
https://bugzilla.redhat.com/show_bug.cgi?id=484047.


 
 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.
 
Question similar to this, I asked in some previous mail. Answer was, 
only root has access to that directory - shouldn't be security problem.

  
 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

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-15 Thread Fabio M. Di Nitto 
On Fri, 2009-05-15 at 09:23 +0200, Jan Friesse wrote:
 Fabio,

  +
  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.
 
 More or less, yes. If there will be process cman (or every other) with 
 uid or gid foo, it will be able to use corosync. It's only 
 ug_config.uid/gid enhancement. See 
 https://bugzilla.redhat.com/show_bug.cgi?id=484047.

Ok.

 
 
  
  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.
  
 Question similar to this, I asked in some previous mail. Answer was, 
 only root has access to that directory - shouldn't be security problem.

Ok.

 
   
  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

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-14 Thread Jan Friesse 
Attached is second version of patch.

Read from dir uses better name for function (no security but rather
uidgid) so it's included too. Take it as a version, which will be
back-ported to RHEL 5.

Regards,
  Honza

Steven Dake wrote:
 rename security as an objdb key to uidgid.
 
 The uid || gid should be valid, not requiring an and operation.
 
 On Wed, 2009-05-13 at 18:21 +0200, Jan Friesse wrote:
 Attached is first version of support for multiple security items (uid-gid).

 First question what I have. I'm currently testing uid and gid as a pair,
 so user process must have gid and uid (not only uid or gid). Is that
 correct, or you will rather see something, what will check uid OR gid?
 (From my point of view, both solution are acceptable and both have some
 pros/cons, so I think, there should be major consensus)

 or operation
 
 Second question. Items are cached, but in list. Steve talked something
 about, that this is fast path, so isn't list some performance killer? If
 yes, I think we can use:
 - hash table (red black tree/...) in case 1. question will be answered,
 that we should check uid and gid as a pair
 - bit-array of uid and gid, if 1. question will be answered uid OR gid

 A list should be ok for now.
 
 Third question. I'm not sure, if I should implement some reloading stuff
 or not. Because in current implementation, ug_config.uid/gid are never
 reloaded, and only logstuff is reloaded.

 
 followup patch imo
 
 Fourth think. From my point of view. ug_config.uid/gid no longer make
 sense to be used for IPC authentifications (becase this patch should be
 full and better replacement), so second patch (corosync-remove-...)
 removes this.

 
 ok
 
 And last think. Can please somebody with native English language update
 manual pages? Of course I can do that, but ... I'm not sure that my
 Czechlish is understandable to anybody different then me, and you, as my
 colleagues ;)

 
 right up there with root canals.  The man pages need love, and we will
 get to them eventually.
 
 Regards
 -steve
 
 Regards,
   Honza
 ___
 Openais mailing list
 Openais@lists.linux-foundation.org
 https://lists.linux-foundation.org/mailman/listinfo/openais
 

diff --git a/trunk/exec/main.c b/trunk/exec/main.c
index db22e96..2b4 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 ();
+
 	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);
 }
 
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;
+}
+
 int corosync_main_config_read (
 	struct objdb_iface_ver0 *objdb,
 	const char **error_string,
@@ -719,6 +767,8 @@ int

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-14 Thread Fabio M. Di Nitto 
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..2b4 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

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-14 Thread Steven Dake 
We decided on /etc/corosync as the dir, not /etc/ais for the uidgid.d
directory.  Other then that looks good

regards
-steve

On Thu, 2009-05-14 at 11:49 +0200, Jan Friesse wrote:
 Attached is second version of patch.
 
 Read from dir uses better name for function (no security but rather
 uidgid) so it's included too. Take it as a version, which will be
 back-ported to RHEL 5.
 
 Regards,
   Honza
 
 Steven Dake wrote:
  rename security as an objdb key to uidgid.
  
  The uid || gid should be valid, not requiring an and operation.
  
  On Wed, 2009-05-13 at 18:21 +0200, Jan Friesse wrote:
  Attached is first version of support for multiple security items (uid-gid).
 
  First question what I have. I'm currently testing uid and gid as a pair,
  so user process must have gid and uid (not only uid or gid). Is that
  correct, or you will rather see something, what will check uid OR gid?
  (From my point of view, both solution are acceptable and both have some
  pros/cons, so I think, there should be major consensus)
 
  or operation
  
  Second question. Items are cached, but in list. Steve talked something
  about, that this is fast path, so isn't list some performance killer? If
  yes, I think we can use:
  - hash table (red black tree/...) in case 1. question will be answered,
  that we should check uid and gid as a pair
  - bit-array of uid and gid, if 1. question will be answered uid OR gid
 
  A list should be ok for now.
  
  Third question. I'm not sure, if I should implement some reloading stuff
  or not. Because in current implementation, ug_config.uid/gid are never
  reloaded, and only logstuff is reloaded.
 
  
  followup patch imo
  
  Fourth think. From my point of view. ug_config.uid/gid no longer make
  sense to be used for IPC authentifications (becase this patch should be
  full and better replacement), so second patch (corosync-remove-...)
  removes this.
 
  
  ok
  
  And last think. Can please somebody with native English language update
  manual pages? Of course I can do that, but ... I'm not sure that my
  Czechlish is understandable to anybody different then me, and you, as my
  colleagues ;)
 
  
  right up there with root canals.  The man pages need love, and we will
  get to them eventually.
  
  Regards
  -steve
  
  Regards,
Honza
  ___
  Openais mailing list
  Openais@lists.linux-foundation.org
  https://lists.linux-foundation.org/mailman/listinfo/openais
  
 

___
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais

[Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-13 Thread Jan Friesse 
Attached is first version of support for multiple security items (uid-gid).

First question what I have. I'm currently testing uid and gid as a pair,
so user process must have gid and uid (not only uid or gid). Is that
correct, or you will rather see something, what will check uid OR gid?
(From my point of view, both solution are acceptable and both have some
pros/cons, so I think, there should be major consensus)

Second question. Items are cached, but in list. Steve talked something
about, that this is fast path, so isn't list some performance killer? If
yes, I think we can use:
- hash table (red black tree/...) in case 1. question will be answered,
that we should check uid and gid as a pair
- bit-array of uid and gid, if 1. question will be answered uid OR gid

Third question. I'm not sure, if I should implement some reloading stuff
or not. Because in current implementation, ug_config.uid/gid are never
reloaded, and only logstuff is reloaded.

Fourth think. From my point of view. ug_config.uid/gid no longer make
sense to be used for IPC authentifications (becase this patch should be
full and better replacement), so second patch (corosync-remove-...)
removes this.

And last think. Can please somebody with native English language update
manual pages? Of course I can do that, but ... I'm not sure that my
Czechlish is understandable to anybody different then me, and you, as my
colleagues ;)

Regards,
  Honza
diff --git a/trunk/exec/main.c b/trunk/exec/main.c
index db22e96..8ebf8ad 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_security_list (void) {
+	struct list_head *iter;
+
+	for (iter = ug_config.security_list.next; iter != ug_config.security_list; ) {
+		struct security_item *si = list_entry (iter, struct security_item, list);
+		iter = iter-next;
+
+		list_del (si-list);
+		free (si);
+	}
+}
+
 /*
  * TODO this function needs some love
  */
@@ -150,6 +162,10 @@ void corosync_request_shutdown (void)
 	poll_stop (0);
 	totempg_finalize ();
 	coroipcs_ipc_exit ();
+
+	/*Remove security_list*/
+	corosync_remove_security_list ();
+
 	corosync_exit_error (AIS_DONE_EXIT);
 }
 
@@ -482,12 +498,21 @@ 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.security_list.next; iter != ug_config.security_list; iter = iter-next) {
+		struct security_item *si = list_entry (iter, struct security_item, list);
+
+		if (euid == si-uid  egid == si-gid)
+			return (1);
+	}
+
 	return (0);
 }
 
diff --git a/trunk/exec/mainconfig.c b/trunk/exec/mainconfig.c
index 79e01cd..e20fffc 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_security (
+	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 security_item *si;
+
+	list_init (ug_config-security_list);
+
+	objdb-object_find_create (
+		OBJECT_PARENT_HANDLE,
+		security,
+		strlen (security),
+		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) {
+			si = malloc (sizeof (*si));
+			if (si == NULL) {
+_corosync_out_of_memory_error();
+			}
+			si-uid = uid;
+			si-gid = gid;
+			list_add (si-list, ug_config-security_list);
+		}
+	}
+	objdb-object_find_destroy (object_find_handle);
+
+	return 0;
+}
+
 int corosync_main_config_read (
 	struct objdb_iface_ver0 *objdb,
 	const char **error_string,
@@ -719,6 +767,8 @@ int corosync_main_config_read (
 		ug_config-gid = gid_determine(ais);
 	}
 
+	corosync_main_config_read_security (objdb, error_string, ug_config);
+
 	add_logsys_config_notification(objdb);
 
 	return 0;
diff --git a/trunk/exec/mainconfig.h b/trunk/exec/mainconfig.h
index c9ab7ea..f3b8cd8 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 security item
+ */
+struct security_item {
+	struct list_head list;
+	int uid;
+	int gid;
+};
+
 struct ug_config {
 	/*
 	 * user/group to run as
 	 */
 	int uid;

Re: [Openais] [corosync trunk] [patch] Initial support for security (uid-gid)

2009-05-13 Thread Steven Dake 
rename security as an objdb key to uidgid.

The uid || gid should be valid, not requiring an and operation.

On Wed, 2009-05-13 at 18:21 +0200, Jan Friesse wrote:
 Attached is first version of support for multiple security items (uid-gid).
 
 First question what I have. I'm currently testing uid and gid as a pair,
 so user process must have gid and uid (not only uid or gid). Is that
 correct, or you will rather see something, what will check uid OR gid?
 (From my point of view, both solution are acceptable and both have some
 pros/cons, so I think, there should be major consensus)
 
or operation

 Second question. Items are cached, but in list. Steve talked something
 about, that this is fast path, so isn't list some performance killer? If
 yes, I think we can use:
 - hash table (red black tree/...) in case 1. question will be answered,
 that we should check uid and gid as a pair
 - bit-array of uid and gid, if 1. question will be answered uid OR gid
 
A list should be ok for now.

 Third question. I'm not sure, if I should implement some reloading stuff
 or not. Because in current implementation, ug_config.uid/gid are never
 reloaded, and only logstuff is reloaded.
 

followup patch imo

 Fourth think. From my point of view. ug_config.uid/gid no longer make
 sense to be used for IPC authentifications (becase this patch should be
 full and better replacement), so second patch (corosync-remove-...)
 removes this.
 

ok

 And last think. Can please somebody with native English language update
 manual pages? Of course I can do that, but ... I'm not sure that my
 Czechlish is understandable to anybody different then me, and you, as my
 colleagues ;)
 

right up there with root canals.  The man pages need love, and we will
get to them eventually.

Regards
-steve

 Regards,
   Honza
 ___
 Openais mailing list
 Openais@lists.linux-foundation.org
 https://lists.linux-foundation.org/mailman/listinfo/openais

___
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais