[SEMANAGE] Reorganize sandbox_expand

[SEMANAGE] Reorganize sandbox_expand

[Ivan Gyurdiev]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

Happy New Year to everybody...
My new year's resolution is to spent less time writing patches, and more 
time having fun..
Like all such resolutions, it's probably wishful thinking.

==========
This patch splits semanage_sandbox_install into 3 functions - one that 
expands, one that merges local stuff, and one that writes.

This is more flexible, because I can skip the parts I don't like. For 
example I can skip link/expand/write if modules_modified=0, and I just 
want to validate a seuser change (needs a few more changes to do that). 
I can keep the policydb around for a while if I want to - maybe for some 
kind of caching scheme w/ commit numbers. I can choose to grab a 
different policydb - maybe we have it cached, or maybe no modules were 
changed, and we can reuse an existing expanded policy. The point is - do 
one thing per function. The policydb is valuable and should not be 
discarded so easily.

By the way, I don't understand the separation between semanage_store.c 
and direct_api.c anymore.
Expanding things, applying local changes, and writing the policy don't 
seem related to the store IMHO - they work on things stored in the 
store... but so does all the rest of libsemanage. On the other hand, the 
name system (which I still don't use, because it's not flexible enough), 
and locking seem more related to the store.

[-- Attachment #2: libsemanage13.reorganize_sandbox_expand.diff --]
[-- Type: text/x-patch, Size: 6183 bytes --]

diff -Naurp --exclude-from excludes old/libsemanage/src/direct_api.c new/libsemanage/src/direct_api.c
--- old/libsemanage/src/direct_api.c	2005-12-26 19:12:49.000000000 -0500
+++ new/libsemanage/src/direct_api.c	2006-01-01 06:07:12.000000000 -0500
@@ -333,6 +333,7 @@ static int semanage_direct_commit(semana
 	const char *linked_filename = NULL, *fc_filename = NULL;
 	sepol_module_package_t *base = NULL;
 	int retval = -1, num_modfiles = 0, i;
+	sepol_policydb_t* out = NULL;
 
 	/* Check if anything was changed */
 	int modified = sh->modules_modified;
@@ -376,8 +377,14 @@ static int semanage_direct_commit(semana
 			goto cleanup;
 		}
 
-		/* Expand the resulting policy */
-		if (semanage_expand_sandbox(sh, base) < 0)
+		/* Expand the resulting policy, apply local changes, and write it out  */
+		if (semanage_expand_sandbox(sh, base, &out) < 0)
+			goto cleanup;
+
+		if (semanage_apply_local_changes(sh, out) < 0)
+			goto cleanup;
+		
+		if (semanage_write_policydb(sh, out) < 0)
 			goto cleanup;
 
 		/* Verify policy */
@@ -397,6 +404,7 @@ static int semanage_direct_commit(semana
 	}
 	free(mod_filenames);
 	sepol_module_package_free(base);
+	sepol_policydb_free(out);
 	semanage_release_trans_lock(sh);
 
 	/* regardless if the commit was successful or not, remove the
diff -Naurp --exclude-from excludes old/libsemanage/src/semanage_store.c new/libsemanage/src/semanage_store.c
--- old/libsemanage/src/semanage_store.c	2005-12-12 11:37:35.000000000 -0500
+++ new/libsemanage/src/semanage_store.c	2006-01-01 06:09:16.000000000 -0500
@@ -1353,33 +1353,49 @@ int semanage_link_sandbox(semanage_handl
 	return retval;
 }
 
-/* Expands the policy contained within *base and write the final
- * policy to the sandbox's kernel policy.  Returns 0 on success, -1 on
- * error.
+/* 
+ * Expands the policy contained within *base 
  */
-int semanage_expand_sandbox(semanage_handle_t *sh, sepol_module_package_t *base) {
-	struct sepol_policydb *out;
-	int retval = -1;
-	const char *kernel_filename = NULL;
-	struct sepol_policy_file *pf = NULL;
+int semanage_expand_sandbox(
+	semanage_handle_t *sh, 
+	sepol_module_package_t *base,
+	sepol_policydb_t** policydb) {
+
+	struct sepol_policydb *out = NULL;
 	int policyvers = sh->conf->policyvers;
 	int expand_check = sh->conf->expand_check ? sh->modules_modified : 0;
-	FILE *outfile = NULL;
 
-	if (sepol_policydb_create(&out)) {
-		return -1;
-	}
+	if (sepol_policydb_create(&out))
+		goto err;
+
 	if (sepol_expand_module(sh->sepolh, 
-				sepol_module_package_get_policy(base), out, 0, expand_check)
-				== -1) {
+			sepol_module_package_get_policy(base), out, 0, expand_check)
+			== -1) {
 		ERR(sh, "Expand module failed");
-		goto cleanup;
+		goto err;
 	}
 	if (sepol_policydb_set_vers(out, policyvers)) {
  		ERR(sh, "Unknown/Invalid policy version %d.", policyvers);
-		goto cleanup;
+		goto err;
 	}
 
+	*policydb = out;
+	return STATUS_SUCCESS;
+
+	err:
+	sepol_policydb_free(out);
+	return STATUS_ERR;
+}
+
+/** 
+ * Applies local changes to the policy
+ */
+int semanage_apply_local_changes(
+	semanage_handle_t *sh,
+	sepol_policydb_t* out) {
+
+	int retval;
+
 	dbase_policydb_attach(semanage_user_dbase_policy(sh)->dbase, out);
 	dbase_policydb_attach(semanage_port_dbase_policy(sh)->dbase, out);
 	dbase_policydb_attach(semanage_iface_dbase_policy(sh)->dbase, out);
@@ -1392,8 +1408,20 @@ int semanage_expand_sandbox(semanage_han
         dbase_policydb_detach(semanage_iface_dbase_policy(sh)->dbase);
         dbase_policydb_detach(semanage_bool_dbase_policy(sh)->dbase);
 
-	if (retval < 0) 
-		goto cleanup;
+	return retval;
+}
+
+/**
+ * Writes the final policy to the sandbox (kernel)
+ */
+int semanage_write_policydb(
+	semanage_handle_t *sh,
+	sepol_policydb_t* out) {
+
+	int retval = STATUS_ERR;
+	const char *kernel_filename = NULL;
+	struct sepol_policy_file *pf = NULL;
+	FILE *outfile = NULL;
 
 	if ((kernel_filename = semanage_path(SEMANAGE_TMP, SEMANAGE_KERNEL)) == NULL) {
 		goto cleanup;
@@ -1412,13 +1440,12 @@ int semanage_expand_sandbox(semanage_han
  		ERR(sh, "Error while writing kernel policy to %s.", kernel_filename);
 		goto cleanup;
 	}
-	retval = 0;
+	retval = STATUS_SUCCESS;
 
  cleanup:
 	if (outfile != NULL) {
 		fclose(outfile);
 	}
-	sepol_policydb_free(out);
 	sepol_policy_file_free(pf);
 	return retval;
 }
diff -Naurp --exclude-from excludes old/libsemanage/src/semanage_store.h new/libsemanage/src/semanage_store.h
--- old/libsemanage/src/semanage_store.h	2005-11-04 15:37:49.000000000 -0500
+++ new/libsemanage/src/semanage_store.h	2006-01-01 06:02:41.000000000 -0500
@@ -61,8 +61,12 @@ int semanage_create_store(semanage_handl
 int semanage_remove_directory(const char *path);
 
 int semanage_make_sandbox(semanage_handle_t *sh);
-int semanage_get_modules_names(semanage_handle_t *sh,
-			       char ***filenames, int *len);
+
+int semanage_get_modules_names(
+	semanage_handle_t *sh,
+	char ***filenames, 
+	int *len);
+
 int semanage_install_sandbox(semanage_handle_t *sh);
 
 /* lock file routines */
@@ -72,13 +76,30 @@ void semanage_release_trans_lock(semanag
 void semanage_release_active_lock(semanage_handle_t *sh);
 int semanage_get_commit_number(semanage_handle_t *sh);
 
+int semanage_link_sandbox(
+	semanage_handle_t *sh, 
+	sepol_module_package_t **base);
+
+int semanage_expand_sandbox(
+	semanage_handle_t *sh, 
+	sepol_module_package_t *base,
+	sepol_policydb_t** policydb);
+
+int semanage_apply_local_changes(
+	semanage_handle_t *sh,
+	sepol_policydb_t* policydb);
+
+int semanage_write_policydb(
+	semanage_handle_t *sh,
+	sepol_policydb_t* policydb);
 
-int semanage_link_sandbox(semanage_handle_t *sh, sepol_module_package_t **base);
-int semanage_expand_sandbox(semanage_handle_t *sh, sepol_module_package_t *base);
 int semanage_install_sandbox(semanage_handle_t *sh);
 
-int semanage_verify_modules(semanage_handle_t *sh,
-			    char **module_filenames, int num_modules);
+int semanage_verify_modules(
+	semanage_handle_t *sh,
+	char **module_filenames, 
+	int num_modules);
+
 int semanage_verify_linked(semanage_handle_t *sh);
 int semanage_verify_kernel(semanage_handle_t *sh);
 int semanage_split_fc(semanage_handle_t *sh);

Re: [SEMANAGE] Reorganize sandbox_expand

[Joshua Brindle]

Ivan Gyurdiev wrote:
> Happy New Year to everybody...
> My new year's resolution is to spent less time writing patches, and more 
> time having fun..
> Like all such resolutions, it's probably wishful thinking.
> 
shocker :)

> ==========
> This patch splits semanage_sandbox_install into 3 functions - one that 
> expands, one that merges local stuff, and one that writes.
> 
> This is more flexible, because I can skip the parts I don't like. For 
> example I can skip link/expand/write if modules_modified=0, and I just 
> want to validate a seuser change (needs a few more changes to do that). 
> I can keep the policydb around for a while if I want to - maybe for some 
> kind of caching scheme w/ commit numbers. I can choose to grab a 
> different policydb - maybe we have it cached, or maybe no modules were 
> changed, and we can reuse an existing expanded policy. The point is - do 
> one thing per function. The policydb is valuable and should not be 
> discarded so easily.
> 
I think this is fine. Personally I don't split things out until I need 
to, else the code becomes much harder to debug, but this seems like a 
good reason to do so.

It looks like the tmp store is going to be written back to active even 
if nothing is written, is this intentional? (it looks like that is also 
the case with the optional rebuild patch, as it only makes a policydb 
rebuild optional). In that case the commit number is probably going to 
be incremented.

> By the way, I don't understand the separation between semanage_store.c 
> and direct_api.c anymore.
> Expanding things, applying local changes, and writing the policy don't 
> seem related to the store IMHO - they work on things stored in the 
> store... but so does all the rest of libsemanage. On the other hand, the 
> name system (which I still don't use, because it's not flexible enough), 
> and locking seem more related to the store.
direct_api is a user of the store. everything in libsemanage does not 
necessarilly use the store, when we add policy server api it will not 
use the store at all. direct_api necessarilly depends on the store (all 
of its data is stored there), this is not wrong.

What name system are you refering to?


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [SEMANAGE] Reorganize sandbox_expand

[Ivan Gyurdiev]

>
> It looks like the tmp store is going to be written back to active even 
> if nothing is written, is this intentional? (it looks like that is 
> also the case with the optional rebuild patch, as it only makes a 
> policydb rebuild optional). In that case the commit number is probably 
> going to be incremented.
That's because policy_components.c is a misnomer, and those functions 
contain things that aren't really policy components, like active 
booleans, file_contexts, and seusers. Specifically, file_contexts and 
seusers get written to disk (in 
policy_components.c:semanage_commit_components()), so yes, an install is 
needed that will increment the commit number.

I probably need to move things around and re-organize them to allow 
future optimizations.
> direct_api is a user of the store. everything in libsemanage does not 
> necessarilly use the store, when we add policy server api it will not 
> use the store at all. 
That's what concerns me with *database.c* depending on semanage_store.c. 
The other thread was talking about *database_file.c*, which is something 
different. The way I see the server working is as another backend of 
database.c, so it still goes through that code. Database.c is supposed 
to be backend-independent, generic entrypoint for dbase calls.

...but, it uses store locking, and store commit numbers, and relies on 
those functions, which will probably need to change for the server. The 
server api wouldn't need locking (because locking should be done done 
server-side, not client-side). It would need commit numbers, but we'd 
have to get those in some other kind of way, not by calling the store - 
will need to probably put the function that gets commit numbers in a 
table and initialize it differently for pserver vs direct.

> direct_api necessarilly depends on the store (all of its data is 
> stored there), this is not wrong.
Yes, but I didn't understand what's the criterion for splitting things 
stylistically between direct_api and semanage_store. I'd put 
expand_sandbox and things like that into direct_api.c

> What name system are you refering to?
The thing with semanage_path() and the prefixes...


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.