[POLICYREP] [RFC/PATCH 0/3] policy package implementation with xar

[POLICYREP] [RFC/PATCH 0/3] policy package implementation with xar

[method]

This uses xar <http://code.google.com/p/xar/> to implement policy packages. This brings in a fair number of dependancies, unfortunately, but provides a useful featureset in exchange. This includes transparent compression of files in the package, signature support and so on. 

Currently it uses the filename to determine the kind of file (eg., file_context file vs. policy module) which is non-ideal, I think it might be better to use xar attributes in the ToC to specify the file but that means we'd have to implement our own packaging functions and could not use the xar command line utility to create packages. Since we don't currently do anything special like that there is no package_write functionality (or set operators for the implimentation). 

I also have concerns about using the module name property as that should be abstract to this code, instead using a xar subdocument could allow us to define the 'name' of the policy as a policy package attribute instead of putting it in the module. This is completely different from how the current code works but I feel like the name should be associated with the policy package rather than the module.

Comments welcome. This is primarilly an RFC to see if this is how we want to handle policy packages, though it should be mergable in its current state if everyone agrees this is the ideal implementation.

FWIW I also looked for more 'lightweight' archival systems and found that no libraries exist for tar, ar or cpio. A quick search of yum only shows one archive library and it is zip format.
-- 

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

[POLICYREP] [RFC/PATCH 1/3] policy package class

[method]

--

--- a/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
+++ b/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
@@ -0,0 +1,38 @@
+/* Author: Joshua Brindle <method@manicmethod.com> */
+
+#ifndef __policy_package_hpp__
+#define __policy_package_hpp__
+
+
+#include <policyrep/policy.hpp>
+
+namespace policyrep
+{
+	struct PolicyPackageImpl;
+	class PolicyPackage
+	{
+	public:
+		PolicyPackage();
+		PolicyPackage(std::string filename);
+		~PolicyPackage();
+
+		const Module get_policy_module() const;
+
+		const std::string get_file_contexts() const;
+		const std::string get_seusers() const;
+		const std::string get_user_extra() const;
+		const std::string get_netfilter_contexts() const;
+
+	protected:
+		void init();
+		int read_package(std::string filename);	
+
+	private:
+		PolicyPackageImpl* impl;
+	};
+
+	typedef boost::shared_ptr<PolicyPackage> PolicyPackagePtr;
+
+} // namespace policyrep
+
+#endif

-- 

--
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: [POLICYREP] [RFC/PATCH 1/3] policy package class

[Karl MacMillan]

On Tue, 2007-07-17 at 11:03 -0400, method@manicmethod.com wrote:
> plain text document attachment (00-policy_package-header.patch)
> --
> 
> --- a/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
> +++ b/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
> @@ -0,0 +1,38 @@
> +/* Author: Joshua Brindle <method@manicmethod.com> */
> +
> +#ifndef __policy_package_hpp__
> +#define __policy_package_hpp__
> +
> +
> +#include <policyrep/policy.hpp>
> +
> +namespace policyrep
> +{
> +	struct PolicyPackageImpl;
> +	class PolicyPackage
> +	{
> +	public:
> +		PolicyPackage();
> +		PolicyPackage(std::string filename);
> +		~PolicyPackage();
> +

Destructors should always be virtual - otherwise subclassing is
difficult.

> +		const Module get_policy_module() const;
> +
> +		const std::string get_file_contexts() const;
> +		const std::string get_seusers() const;
> +		const std::string get_user_extra() const;
> +		const std::string get_netfilter_contexts() const;
> +

why not const std::string&? I also think we should always default to
virtual.

> +	protected:
> +		void init();
> +		int read_package(std::string filename);	
> +
> +	private:
> +		PolicyPackageImpl* impl;

We need to make a decision on whether this is going to be protected or
private and stick with that. I've done protected so far - if you want to
change it please say why and send a patch changing all existing classes.

> +	};
> +
> +	typedef boost::shared_ptr<PolicyPackage> PolicyPackagePtr;
> +

This may not be needed here - it is currently only definitely used for
objects that are part of the ast.

Karl

> +} // namespace policyrep
> +
> +#endif
> 


--
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: [POLICYREP] [RFC/PATCH 1/3] policy package class

[Joshua Brindle]

Karl MacMillan wrote:
> On Tue, 2007-07-17 at 11:03 -0400, method@manicmethod.com wrote:
>   
>> plain text document attachment (00-policy_package-header.patch)
>> --
>>
>> --- a/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
>> +++ b/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
>> @@ -0,0 +1,38 @@
>> +/* Author: Joshua Brindle <method@manicmethod.com> */
>> +
>> +#ifndef __policy_package_hpp__
>> +#define __policy_package_hpp__
>> +
>> +
>> +#include <policyrep/policy.hpp>
>> +
>> +namespace policyrep
>> +{
>> +	struct PolicyPackageImpl;
>> +	class PolicyPackage
>> +	{
>> +	public:
>> +		PolicyPackage();
>> +		PolicyPackage(std::string filename);
>> +		~PolicyPackage();
>> +
>>     
>
> Destructors should always be virtual - otherwise subclassing is
> difficult.
>   

I don't think there is any reason to ever subclass a policy package. Its 
different from the ast components.
>   
>> +		const Module get_policy_module() const;
>> +
>> +		const std::string get_file_contexts() const;
>> +		const std::string get_seusers() const;
>> +		const std::string get_user_extra() const;
>> +		const std::string get_netfilter_contexts() const;
>> +
>>     
>
> why not const std::string&? I also think we should always default to
> virtual.
>
>   
because this won't be subclassed I don't think these should be virtual.

>> +	protected:
>> +		void init();
>> +		int read_package(std::string filename);	
>> +
>> +	private:
>> +		PolicyPackageImpl* impl;
>>     
>
> We need to make a decision on whether this is going to be protected or
> private and stick with that. I've done protected so far - if you want to
> change it please say why and send a patch changing all existing classes.
>
>   

Same argument. If this isn't subclassed it shouldn't be protected, other 
classes are different that way.
>> +	};
>> +
>> +	typedef boost::shared_ptr<PolicyPackage> PolicyPackagePtr;
>> +
>>     
>
> This may not be needed here - it is currently only definitely used for
> objects that are part of the ast.
>
>   

I like it, it will enable users to have several pointers to the package 
and have them dealt with correctly. Do you disagree?


--
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: [POLICYREP] [RFC/PATCH 1/3] policy package class

[Karl MacMillan]

On Tue, 2007-07-17 at 12:23 -0400, Joshua Brindle wrote:
> Karl MacMillan wrote:
> > On Tue, 2007-07-17 at 11:03 -0400, method@manicmethod.com wrote:
> >   
> >> plain text document attachment (00-policy_package-header.patch)
> >> --
> >>
> >> --- a/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
> >> +++ b/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
> >> @@ -0,0 +1,38 @@
> >> +/* Author: Joshua Brindle <method@manicmethod.com> */
> >> +
> >> +#ifndef __policy_package_hpp__
> >> +#define __policy_package_hpp__
> >> +
> >> +
> >> +#include <policyrep/policy.hpp>
> >> +
> >> +namespace policyrep
> >> +{
> >> +	struct PolicyPackageImpl;
> >> +	class PolicyPackage
> >> +	{
> >> +	public:
> >> +		PolicyPackage();
> >> +		PolicyPackage(std::string filename);
> >> +		~PolicyPackage();
> >> +
> >>     
> >
> > Destructors should always be virtual - otherwise subclassing is
> > difficult.
> >   
> 
> I don't think there is any reason to ever subclass a policy package. Its 
> different from the ast components.

Except that if the destructor is not virtual you cannot effectively
subclass without breaking binary compatibility. Also - in this case
there is no real downside to virtual by default, so why not do it?

> >   
> >> +		const Module get_policy_module() const;
> >> +
> >> +		const std::string get_file_contexts() const;
> >> +		const std::string get_seusers() const;
> >> +		const std::string get_user_extra() const;
> >> +		const std::string get_netfilter_contexts() const;
> >> +
> >>     
> >
> > why not const std::string&? I also think we should always default to
> > virtual.
> >
> >   
> because this won't be subclassed I don't think these should be virtual.
> 

See above. What about the return type, though?

> >> +	protected:
> >> +		void init();
> >> +		int read_package(std::string filename);	
> >> +
> >> +	private:
> >> +		PolicyPackageImpl* impl;
> >>     
> >
> > We need to make a decision on whether this is going to be protected or
> > private and stick with that. I've done protected so far - if you want to
> > change it please say why and send a patch changing all existing classes.
> >
> >   
> 
> Same argument. If this isn't subclassed it shouldn't be protected, other 
> classes are different that way.
> >> +	};
> >> +
> >> +	typedef boost::shared_ptr<PolicyPackage> PolicyPackagePtr;
> >> +
> >>     
> >
> > This may not be needed here - it is currently only definitely used for
> > objects that are part of the ast.
> >
> >   
> 
> I like it, it will enable users to have several pointers to the package 
> and have them dealt with correctly. Do you disagree?
> 

No - I was just noting that it wasn't strictly necessary. If you find it
useful I'm fine with it.

Karl


--
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: [POLICYREP] [RFC/PATCH 1/3] policy package class

[Joshua Brindle]

Karl MacMillan wrote:
> On Tue, 2007-07-17 at 12:23 -0400, Joshua Brindle wrote:
>   
>> Karl MacMillan wrote:
>>     
>>> On Tue, 2007-07-17 at 11:03 -0400, method@manicmethod.com wrote:
>>>   
>>>       
>>>> plain text document attachment (00-policy_package-header.patch)
>>>> --
>>>>
>>>> --- a/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
>>>> +++ b/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
>>>> @@ -0,0 +1,38 @@
>>>> +/* Author: Joshua Brindle <method@manicmethod.com> */
>>>> +
>>>> +#ifndef __policy_package_hpp__
>>>> +#define __policy_package_hpp__
>>>> +
>>>> +
>>>> +#include <policyrep/policy.hpp>
>>>> +
>>>> +namespace policyrep
>>>> +{
>>>> +	struct PolicyPackageImpl;
>>>> +	class PolicyPackage
>>>> +	{
>>>> +	public:
>>>> +		PolicyPackage();
>>>> +		PolicyPackage(std::string filename);
>>>> +		~PolicyPackage();
>>>> +
>>>>     
>>>>         
>>> Destructors should always be virtual - otherwise subclassing is
>>> difficult.
>>>   
>>>       
>> I don't think there is any reason to ever subclass a policy package. Its 
>> different from the ast components.
>>     
>
> Except that if the destructor is not virtual you cannot effectively
> subclass without breaking binary compatibility. Also - in this case
> there is no real downside to virtual by default, so why not do it?
>
>   

Fine.

>>>   
>>>       
>>>> +		const Module get_policy_module() const;
>>>> +
>>>> +		const std::string get_file_contexts() const;
>>>> +		const std::string get_seusers() const;
>>>> +		const std::string get_user_extra() const;
>>>> +		const std::string get_netfilter_contexts() const;
>>>> +
>>>>     
>>>>         
>>> why not const std::string&? I also think we should always default to
>>> virtual.
>>>
>>>   
>>>       
>> because this won't be subclassed I don't think these should be virtual.
>>
>>     
>
> See above. What about the return type, though?
>
>   

I just don't see it as advantageous, since functions aren't part of the 
mangled class struct the ABI won't change if they are added later as 
virtuals (assuming there is no other virtuals below that will change 
alignment and everything else is private). Also, if we determine that, 
e.g. seusers have no business in modules we can remove the functions 
without destroying the rest of the function alignments which will only 
affect the API not the ABI, for this reason I'd prefer to avoid virtuals 
unless there is a real possibility of the class being subclassed.

On the std::string&, I've seen arguments against making std::strings 
pointers because then you don't need to deal with initialization and the 
string data itself is on the heap anyway, whats the advantage to making 
these pointers?
>>>> +	protected:
>>>> +		void init();
>>>> +		int read_package(std::string filename);	
>>>> +
>>>> +	private:
>>>> +		PolicyPackageImpl* impl;
>>>>     
>>>>         
>>> We need to make a decision on whether this is going to be protected or
>>> private and stick with that. I've done protected so far - if you want to
>>> change it please say why and send a patch changing all existing classes.
>>>
>>>   
>>>       
>> Same argument. If this isn't subclassed it shouldn't be protected, other 
>> classes are different that way.
>>     



--
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: [POLICYREP] [RFC/PATCH 1/3] policy package class

[Karl MacMillan]

On Tue, 2007-07-17 at 16:33 -0400, Joshua Brindle wrote:
> Karl MacMillan wrote:
> > On Tue, 2007-07-17 at 12:23 -0400, Joshua Brindle wrote:
> >   
> >> Karl MacMillan wrote:
> >>     
> >>> On Tue, 2007-07-17 at 11:03 -0400, method@manicmethod.com wrote:
> >>>   
> >>>       
> >>>> plain text document attachment (00-policy_package-header.patch)
> >>>> --
> >>>>
> >>>> --- a/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
> >>>> +++ b/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
> >>>> @@ -0,0 +1,38 @@
> >>>> +/* Author: Joshua Brindle <method@manicmethod.com> */
> >>>> +
> >>>> +#ifndef __policy_package_hpp__
> >>>> +#define __policy_package_hpp__
> >>>> +
> >>>> +
> >>>> +#include <policyrep/policy.hpp>
> >>>> +
> >>>> +namespace policyrep
> >>>> +{
> >>>> +	struct PolicyPackageImpl;
> >>>> +	class PolicyPackage
> >>>> +	{
> >>>> +	public:
> >>>> +		PolicyPackage();
> >>>> +		PolicyPackage(std::string filename);
> >>>> +		~PolicyPackage();
> >>>> +
> >>>>     
> >>>>         
> >>> Destructors should always be virtual - otherwise subclassing is
> >>> difficult.
> >>>   
> >>>       
> >> I don't think there is any reason to ever subclass a policy package. Its 
> >> different from the ast components.
> >>     
> >
> > Except that if the destructor is not virtual you cannot effectively
> > subclass without breaking binary compatibility. Also - in this case
> > there is no real downside to virtual by default, so why not do it?
> >
> >   
> 
> Fine.
> 
> >>>   
> >>>       
> >>>> +		const Module get_policy_module() const;
> >>>> +
> >>>> +		const std::string get_file_contexts() const;
> >>>> +		const std::string get_seusers() const;
> >>>> +		const std::string get_user_extra() const;
> >>>> +		const std::string get_netfilter_contexts() const;
> >>>> +
> >>>>     
> >>>>         
> >>> why not const std::string&? I also think we should always default to
> >>> virtual.
> >>>
> >>>   
> >>>       
> >> because this won't be subclassed I don't think these should be virtual.
> >>
> >>     
> >
> > See above. What about the return type, though?
> >
> >   
> 
> I just don't see it as advantageous, since functions aren't part of the 
> mangled class struct the ABI won't change if they are added later as 
> virtuals (assuming there is no other virtuals below that will change 
> alignment and everything else is private).

Not certain what you mean, but you can't convert a function to virtual
later and retain ABI.

>  Also, if we determine that, 
> e.g. seusers have no business in modules we can remove the functions 
> without destroying the rest of the function alignments which will only 
> affect the API not the ABI,

Err - of course it will effect the ABI. The runtime behavior will be
_vastly_ different. It is, I think, better to break ABI in these cases
so that you don't get undefined runtime behavior.

>  for this reason I'd prefer to avoid virtuals 
> unless there is a real possibility of the class being subclassed.
> 

But there is at least some chance - what if I want to create a different
kind of package? Or a fake package that is just some files on disk? The
whole point is that a class that the potential of subclassing adds real
flexibility to the code.

> On the std::string&, I've seen arguments against making std::strings 
> pointers because then you don't need to deal with initialization and the 
> string data itself is on the heap anyway, whats the advantage to making 
> these pointers?

Umm - I'm suggesting that you should return references not pointers. It
avoids copies and allows the caller to change the strings.

Karl


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

[POLICYREP] [RFC/PATCH 2/3] policy package implementation

[method]

--

--- a/libpolicyrep/src/policy_package.cpp	(revision 0)
+++ b/libpolicyrep/src/policy_package.cpp	(revision 0)
@@ -0,0 +1,166 @@
+/*
+ * Author : Joshua Brindle <jbrindle@tresys.com>
+ *
+ * Copyright (C) 2007 Tresys Technology, llc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+
+extern "C" {
+#include <xar/xar.h>
+#include <string.h>
+}
+
+#include <policyrep/policy.hpp>
+#include <policyrep/policy_package.hpp>
+
+#include <sstream>
+#include <iostream>
+
+namespace policyrep
+{
+
+	struct PolicyPackageImpl {
+		Module policy_module;
+		std::string file_contexts;
+		std::string seusers;
+		std::string user_extra;
+		std::string netfilter_contexts;
+	};
+
+	void PolicyPackage::init() {
+		impl = new PolicyPackageImpl;
+	}
+
+	int PolicyPackage::read_package(std::string filename) {
+		xar_t x;
+		xar_file_t f;
+		xar_iter_t i;
+
+		i = xar_iter_new();
+		if (i == NULL) {
+			throw "Unable to allocate iterator";
+		}
+
+		x = xar_open(filename.c_str(), READ);
+		if (x == NULL) {
+			throw "Unable to open policy package";
+		}
+
+		for (f = xar_file_first(x, i); f; f = xar_file_next(i)) {
+			size_t sz;
+			char *fbuf;
+			const char *filename;			
+			int32_t ret;
+			
+			ret = xar_extract_tobuffersz(x, f, &fbuf, &sz);
+			if (ret) {
+				// This can happen if the file is 0 bytes 
+				// or is a symlink, directory, etc. We might want 
+				// to put code here to check those cases and bail 
+				// but for now we just ignore them and continue.
+				continue;
+			}
+
+
+			// Currently we use the name of the file to decide what kind of file it is
+			// I am fairly uncomfortable with this but it allows us to use xar to create
+			// policy packages instead of having our own write_package function that 
+			// assigns attributes to files instead of using names. 
+			ret = xar_prop_get(f, "name", &filename);
+			if (ret) {
+				xar_close(x);
+				throw "Error getting name property of file";
+			}
+			if (strstr(filename, ".mod")) {
+				// found module, do stuff
+			} else if (strstr(filename, ".file_contexts")) {
+				if (!impl->file_contexts.empty()) {
+					// Found more than one file containing in .file_contexts,
+					// don't know how to handle that so bail for now
+					xar_close(x);
+					throw "Multiple file_contexts files in policy package";
+				}
+				impl->file_contexts = std::string(fbuf);
+			} else if (strstr(filename, ".seusers")) {
+				if (!impl->seusers.empty()) {
+					// Found more than one file containing in .seusers,
+					// don't know how to handle that so bail for now
+					xar_close(x);
+					throw "Multiple seusers files in policy package";
+				}
+				impl->seusers = std::string(fbuf);
+			} else if (strstr(filename, ".user_extra")) {
+				if (!impl->user_extra.empty()) {
+					// Found more than one file containing in .user_extra,
+					// don't know how to handle that so bail for now
+					xar_close(x);
+					throw "Multiple user_extra files in policy package";
+				}
+				impl->user_extra = std::string(fbuf);
+			} else if (strstr(filename, ".netfilter_contexts")) {
+				if (!impl->netfilter_contexts.empty()) {
+					// Found more than one file containing in .netfilter_contexts,
+					// don't know how to handle that so bail for now
+					xar_close(x);
+					throw "Multiple netfilter_contexts files in policy package";
+				}
+				impl->netfilter_contexts = std::string(fbuf);
+			} else {
+				// unrecognized file, just skip it
+				continue;
+			}
+
+		}
+
+		xar_close(x);
+
+		return 0;
+	}
+
+	PolicyPackage::PolicyPackage() {
+		init();
+	}
+
+	PolicyPackage::PolicyPackage(std::string filename) {
+		init();
+		read_package(filename);
+	}
+
+	PolicyPackage::~PolicyPackage() { delete impl; }
+
+	const Module PolicyPackage::get_policy_module() const {
+		return impl->policy_module;
+	}
+
+	const std::string PolicyPackage::get_file_contexts() const {
+		return impl->file_contexts;
+	}
+
+	const std::string PolicyPackage::get_seusers() const {
+		return impl->seusers;
+	}
+
+	const std::string PolicyPackage::get_user_extra() const {
+		return impl->user_extra;
+	}
+
+	const std::string PolicyPackage::get_netfilter_contexts() const {
+		return impl->netfilter_contexts;
+	}
+
+
+} // namespace policyrep

-- 

--
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: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Karl MacMillan]

On Tue, 2007-07-17 at 11:03 -0400, method@manicmethod.com wrote:
> plain text document attachment (01-policy_package.diff)
> --
> 
> --- a/libpolicyrep/src/policy_package.cpp	(revision 0)
> +++ b/libpolicyrep/src/policy_package.cpp	(revision 0)
> @@ -0,0 +1,166 @@
> +/*
> + * Author : Joshua Brindle <jbrindle@tresys.com>
> + *
> + * Copyright (C) 2007 Tresys Technology, llc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +
> +extern "C" {
> +#include <xar/xar.h>
> +#include <string.h>
> +}
> +
> +#include <policyrep/policy.hpp>
> +#include <policyrep/policy_package.hpp>
> +
> +#include <sstream>
> +#include <iostream>
> +
> +namespace policyrep
> +{
> +
> +	struct PolicyPackageImpl {
> +		Module policy_module;

Are you sure you want this by value?

> +		std::string file_contexts;
> +		std::string seusers;
> +		std::string user_extra;
> +		std::string netfilter_contexts;
> +	};
> +
> +	void PolicyPackage::init() {
> +		impl = new PolicyPackageImpl;
> +	}
> +

I prefer to order the .cpp file in the same order functions are declared
in the class. I think it makes it easier to find the function you want.

> +	int PolicyPackage::read_package(std::string filename) {
> +		xar_t x;
> +		xar_file_t f;
> +		xar_iter_t i;
> +
> +		i = xar_iter_new();
> +		if (i == NULL) {
> +			throw "Unable to allocate iterator";

Ick - no string exceptions please. Either use an exception in stdexcept
or define a new one (std::bad_alloc is appropriate here).

> +		}
> +
> +		x = xar_open(filename.c_str(), READ);
> +		if (x == NULL) {
> +			throw "Unable to open policy package";
> +		}
> +
> +		for (f = xar_file_first(x, i); f; f = xar_file_next(i)) {
> +			size_t sz;
> +			char *fbuf;
> +			const char *filename;			
> +			int32_t ret;
> +			
> +			ret = xar_extract_tobuffersz(x, f, &fbuf, &sz);
> +			if (ret) {
> +				// This can happen if the file is 0 bytes 
> +				// or is a symlink, directory, etc. We might want 
> +				// to put code here to check those cases and bail 
> +				// but for now we just ignore them and continue.
> +				continue;
> +			}
> +
> +
> +			// Currently we use the name of the file to decide what kind of file it is
> +			// I am fairly uncomfortable with this but it allows us to use xar to create
> +			// policy packages instead of having our own write_package function that 
> +			// assigns attributes to files instead of using names. 

I don't like this either - what is the alternative?

> +			ret = xar_prop_get(f, "name", &filename);
> +			if (ret) {
> +				xar_close(x);
> +				throw "Error getting name property of file";

Please define an exception.

> +			}
> +			if (strstr(filename, ".mod")) {
> +				// found module, do stuff

Err - "do stuff"?

> +			} else if (strstr(filename, ".file_contexts")) {
> +				if (!impl->file_contexts.empty()) {
> +					// Found more than one file containing in .file_contexts,
> +					// don't know how to handle that so bail for now
> +					xar_close(x);
> +					throw "Multiple file_contexts files in policy package";
> +				}
> +				impl->file_contexts = std::string(fbuf);
> +			} else if (strstr(filename, ".seusers")) {
> +				if (!impl->seusers.empty()) {
> +					// Found more than one file containing in .seusers,
> +					// don't know how to handle that so bail for now
> +					xar_close(x);
> +					throw "Multiple seusers files in policy package";
> +				}
> +				impl->seusers = std::string(fbuf);

As we discussed privately - these copies are not great. Perhaps just use
raw char*.

> +			} else if (strstr(filename, ".user_extra")) {
> +				if (!impl->user_extra.empty()) {
> +					// Found more than one file containing in .user_extra,
> +					// don't know how to handle that so bail for now
> +					xar_close(x);
> +					throw "Multiple user_extra files in policy package";
> +				}
> +				impl->user_extra = std::string(fbuf);
> +			} else if (strstr(filename, ".netfilter_contexts")) {
> +				if (!impl->netfilter_contexts.empty()) {
> +					// Found more than one file containing in .netfilter_contexts,
> +					// don't know how to handle that so bail for now
> +					xar_close(x);
> +					throw "Multiple netfilter_contexts files in policy package";
> +				}
> +				impl->netfilter_contexts = std::string(fbuf);
> +			} else {
> +				// unrecognized file, just skip it
> +				continue;
> +			}
> +
> +		}
> +
> +		xar_close(x);
> +
> +		return 0;
> +	}
> +
> +	PolicyPackage::PolicyPackage() {
> +		init();
> +	}
> +
> +	PolicyPackage::PolicyPackage(std::string filename) {
> +		init();
> +		read_package(filename);
> +	}
> +
> +	PolicyPackage::~PolicyPackage() { delete impl; }
> +
> +	const Module PolicyPackage::get_policy_module() const {
> +		return impl->policy_module;
> +	}

This should definitely not be by value. It should also likely not be
const - this is how the user will retrieve the module after reading to
use, correct? Then it shouldn't be const (they might want to much with
it), should be a reference, and shouldn't be const for the class (as
allowing users to change the module changes the object).

> +
> +	const std::string PolicyPackage::get_file_contexts() const {
> +		return impl->file_contexts;
> +	}
> +
> +	const std::string PolicyPackage::get_seusers() const {
> +		return impl->seusers;
> +	}
> +
> +	const std::string PolicyPackage::get_user_extra() const {
> +		return impl->user_extra;
> +	}
> +
> +	const std::string PolicyPackage::get_netfilter_contexts() const {
> +		return impl->netfilter_contexts;
> +	}
> +

Ditto.

> +
> +} // namespace policyrep
> 


--
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: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Joshua Brindle]

Karl MacMillan wrote:
>
>> +
>> +namespace policyrep
>> +{
>> +
>> +	struct PolicyPackageImpl {
>> +		Module policy_module;
>>     
>
> Are you sure you want this by value?
>
>   
oops, yes, fixed.

>> +		std::string file_contexts;
>> +		std::string seusers;
>> +		std::string user_extra;
>> +		std::string netfilter_contexts;
>> +	};
>> +
>> +	void PolicyPackage::init() {
>> +		impl = new PolicyPackageImpl;
>> +	}
>> +
>>     
>
> I prefer to order the .cpp file in the same order functions are declared
> in the class. I think it makes it easier to find the function you want.
>
>   

I reordered them, I was still use to having to order based on usage 
(eg., read_package first since its called from one of the constructors.. 
silly C habits :\)

>> +	int PolicyPackage::read_package(std::string filename) {
>> +		xar_t x;
>> +		xar_file_t f;
>> +		xar_iter_t i;
>> +
>> +		i = xar_iter_new();
>> +		if (i == NULL) {
>> +			throw "Unable to allocate iterator";
>>     
>
> Ick - no string exceptions please. Either use an exception in stdexcept
> or define a new one (std::bad_alloc is appropriate here).
>
>   
fixed.

>> +		}
>> +
>> +		x = xar_open(filename.c_str(), READ);
>> +		if (x == NULL) {
>> +			throw "Unable to open policy package";
>> +		}
>> +
>> +		for (f = xar_file_first(x, i); f; f = xar_file_next(i)) {
>> +			size_t sz;
>> +			char *fbuf;
>> +			const char *filename;			
>> +			int32_t ret;
>> +			
>> +			ret = xar_extract_tobuffersz(x, f, &fbuf, &sz);
>> +			if (ret) {
>> +				// This can happen if the file is 0 bytes 
>> +				// or is a symlink, directory, etc. We might want 
>> +				// to put code here to check those cases and bail 
>> +				// but for now we just ignore them and continue.
>> +				continue;
>> +			}
>> +
>> +
>> +			// Currently we use the name of the file to decide what kind of file it is
>> +			// I am fairly uncomfortable with this but it allows us to use xar to create
>> +			// policy packages instead of having our own write_package function that 
>> +			// assigns attributes to files instead of using names. 
>>     
>
> I don't like this either - what is the alternative?
>
>   

Ok, xar has an XML ToC for the file. It also provides an API to set 
arbitrary attributes for each file, so I can do something like 
selinuxfiletype = netfilter_contexts. This means that we would not be 
able to use the command line xar to create packages (I was trying to 
make this as simple as possible). If we move to using attributes we'd 
have to have a custom packager again (ala semodule_package).

A custom packager has advantages, we can basically ensure that the 
package is sane (doesn't have directory trees or multiple 
<something>.netfilter_contexts files, etc. We can also add new 
properties such as the policy name to the xar XML subdocument.

>> +			ret = xar_prop_get(f, "name", &filename);
>> +			if (ret) {
>> +				xar_close(x);
>> +				throw "Error getting name property of file";
>>     
>
> Please define an exception.
>
>   
Why?

>> +			}
>> +			if (strstr(filename, ".mod")) {
>> +				// found module, do stuff
>>     
>
> Err - "do stuff"?
>
>   
There is no module_read function right now so thats just a placeholder.

>> +			} else if (strstr(filename, ".file_contexts")) {
>> +				if (!impl->file_contexts.empty()) {
>> +					// Found more than one file containing in .file_contexts,
>> +					// don't know how to handle that so bail for now
>> +					xar_close(x);
>> +					throw "Multiple file_contexts files in policy package";
>> +				}
>> +				impl->file_contexts = std::string(fbuf);
>> +			} else if (strstr(filename, ".seusers")) {
>> +				if (!impl->seusers.empty()) {
>> +					// Found more than one file containing in .seusers,
>> +					// don't know how to handle that so bail for now
>> +					xar_close(x);
>> +					throw "Multiple seusers files in policy package";
>> +				}
>> +				impl->seusers = std::string(fbuf);
>>     
>
> As we discussed privately - these copies are not great. Perhaps just use
> raw char*.
>
>   

Ok, I don't know if this code is ever going to do anything with those 
and was trying to keep it in a format that would be more accessible if 
we needed to. I can change to char * though and see if anything develops 
that makes std::string make more sense.

>> +			} else if (strstr(filename, ".user_extra")) {
>> +				if (!impl->user_extra.empty()) {
>> +					// Found more than one file containing in .user_extra,
>> +					// don't know how to handle that so bail for now
>> +					xar_close(x);
>> +					throw "Multiple user_extra files in policy package";
>> +				}
>> +				impl->user_extra = std::string(fbuf);
>> +			} else if (strstr(filename, ".netfilter_contexts")) {
>> +				if (!impl->netfilter_contexts.empty()) {
>> +					// Found more than one file containing in .netfilter_contexts,
>> +					// don't know how to handle that so bail for now
>> +					xar_close(x);
>> +					throw "Multiple netfilter_contexts files in policy package";
>> +				}
>> +				impl->netfilter_contexts = std::string(fbuf);
>> +			} else {
>> +				// unrecognized file, just skip it
>> +				continue;
>> +			}
>> +
>> +		}
>> +
>> +		xar_close(x);
>> +
>> +		return 0;
>> +	}
>> +
>> +	PolicyPackage::PolicyPackage() {
>> +		init();
>> +	}
>> +
>> +	PolicyPackage::PolicyPackage(std::string filename) {
>> +		init();
>> +		read_package(filename);
>> +	}
>> +
>> +	PolicyPackage::~PolicyPackage() { delete impl; }
>> +
>> +	const Module PolicyPackage::get_policy_module() const {
>> +		return impl->policy_module;
>> +	}
>>     
>
> This should definitely not be by value. It should also likely not be
> const - this is how the user will retrieve the module after reading to
> use, correct? Then it shouldn't be const (they might want to much with
> it), should be a reference, and shouldn't be const for the class (as
> allowing users to change the module changes the object).
>
>   
Fair enough. I was basically thinking users would read a module and make 
their own copies (eg., copying the data into a merged copy or something) 
but that isn't really how policyrep works so I'll change all these to be 
non-const.

>> +
>> +	const std::string PolicyPackage::get_file_contexts() const {
>> +		return impl->file_contexts;
>> +	}
>> +
>> +	const std::string PolicyPackage::get_seusers() const {
>> +		return impl->seusers;
>> +	}
>> +
>> +	const std::string PolicyPackage::get_user_extra() const {
>> +		return impl->user_extra;
>> +	}
>> +
>> +	const std::string PolicyPackage::get_netfilter_contexts() const {
>> +		return impl->netfilter_contexts;
>> +	}
>> +
>>     
>
> Ditto.
>   


--
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: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Karl MacMillan]

On Tue, 2007-07-17 at 12:40 -0400, Joshua Brindle wrote:
> Karl MacMillan wrote:
[...]
> >> +			// Currently we use the name of the file to decide what kind of file it is
> >> +			// I am fairly uncomfortable with this but it allows us to use xar to create
> >> +			// policy packages instead of having our own write_package function that 
> >> +			// assigns attributes to files instead of using names. 
> >>     
> >
> > I don't like this either - what is the alternative?
> >
> >   
> 
> Ok, xar has an XML ToC for the file. It also provides an API to set 
> arbitrary attributes for each file, so I can do something like 
> selinuxfiletype = netfilter_contexts. This means that we would not be 
> able to use the command line xar to create packages (I was trying to 
> make this as simple as possible). If we move to using attributes we'd 
> have to have a custom packager again (ala semodule_package).
> 

It would be simpler though, correct? And what about an unpackager?

> A custom packager has advantages, we can basically ensure that the 
> package is sane (doesn't have directory trees or multiple 
> <something>.netfilter_contexts files, etc. We can also add new 
> properties such as the policy name to the xar XML subdocument.
> 

That sounds better than relying on file extension to me.

> >> +			ret = xar_prop_get(f, "name", &filename);
> >> +			if (ret) {
> >> +				xar_close(x);
> >> +				throw "Error getting name property of file";
> >>     
> >
> > Please define an exception.
> >
> >   
> Why?
> 

Because none of the standard exception types seemed to fit and string
exceptions are not a good idea.

> >> +			}
> >> +			if (strstr(filename, ".mod")) {
> >> +				// found module, do stuff
> >>     
> >
> > Err - "do stuff"?
> >
> >   
> There is no module_read function right now so thats just a placeholder.
> 

There is if the module is source :)

> >> +			} else if (strstr(filename, ".file_contexts")) {
> >> +				if (!impl->file_contexts.empty()) {
> >> +					// Found more than one file containing in .file_contexts,
> >> +					// don't know how to handle that so bail for now
> >> +					xar_close(x);
> >> +					throw "Multiple file_contexts files in policy package";
> >> +				}
> >> +				impl->file_contexts = std::string(fbuf);
> >> +			} else if (strstr(filename, ".seusers")) {
> >> +				if (!impl->seusers.empty()) {
> >> +					// Found more than one file containing in .seusers,
> >> +					// don't know how to handle that so bail for now
> >> +					xar_close(x);
> >> +					throw "Multiple seusers files in policy package";
> >> +				}
> >> +				impl->seusers = std::string(fbuf);
> >>     
> >
> > As we discussed privately - these copies are not great. Perhaps just use
> > raw char*.
> >
> >   
> 
> Ok, I don't know if this code is ever going to do anything with those 
> and was trying to keep it in a format that would be more accessible if 
> we needed to. I can change to char * though and see if anything develops 
> that makes std::string make more sense.
> 
> >> +			} else if (strstr(filename, ".user_extra")) {
> >> +				if (!impl->user_extra.empty()) {
> >> +					// Found more than one file containing in .user_extra,
> >> +					// don't know how to handle that so bail for now
> >> +					xar_close(x);
> >> +					throw "Multiple user_extra files in policy package";
> >> +				}
> >> +				impl->user_extra = std::string(fbuf);
> >> +			} else if (strstr(filename, ".netfilter_contexts")) {
> >> +				if (!impl->netfilter_contexts.empty()) {
> >> +					// Found more than one file containing in .netfilter_contexts,
> >> +					// don't know how to handle that so bail for now
> >> +					xar_close(x);
> >> +					throw "Multiple netfilter_contexts files in policy package";
> >> +				}
> >> +				impl->netfilter_contexts = std::string(fbuf);
> >> +			} else {
> >> +				// unrecognized file, just skip it
> >> +				continue;
> >> +			}
> >> +
> >> +		}
> >> +
> >> +		xar_close(x);
> >> +
> >> +		return 0;
> >> +	}
> >> +
> >> +	PolicyPackage::PolicyPackage() {
> >> +		init();
> >> +	}
> >> +
> >> +	PolicyPackage::PolicyPackage(std::string filename) {
> >> +		init();
> >> +		read_package(filename);
> >> +	}
> >> +
> >> +	PolicyPackage::~PolicyPackage() { delete impl; }
> >> +
> >> +	const Module PolicyPackage::get_policy_module() const {
> >> +		return impl->policy_module;
> >> +	}
> >>     
> >
> > This should definitely not be by value. It should also likely not be
> > const - this is how the user will retrieve the module after reading to
> > use, correct? Then it shouldn't be const (they might want to much with
> > it), should be a reference, and shouldn't be const for the class (as
> > allowing users to change the module changes the object).
> >
> >   
> Fair enough. I was basically thinking users would read a module and make 
> their own copies (eg., copying the data into a merged copy or something) 
> but that isn't really how policyrep works so I'll change all these to be 
> non-const.
> 

One other note - if the caller is expected to take ownership for the
lifetime of the object (which seems possible), then it should return a
pointer or a shared_ptr.

Karl



--
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: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Joshua Brindle]

[-- Attachment #1: Type: text/html, Size: 7822 bytes --]

Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Joshua Brindle]

Karl MacMillan wrote:
> On Tue, 2007-07-17 at 12:40 -0400, Joshua Brindle wrote:
>   
>> Karl MacMillan wrote:
>>     
> [...]
>   
>>>> +			// Currently we use the name of the file to decide what kind of file it is
>>>> +			// I am fairly uncomfortable with this but it allows us to use xar to create
>>>> +			// policy packages instead of having our own write_package function that 
>>>> +			// assigns attributes to files instead of using names. 
>>>>     
>>>>         
>>> I don't like this either - what is the alternative?
>>>
>>>   
>>>       
>> Ok, xar has an XML ToC for the file. It also provides an API to set 
>> arbitrary attributes for each file, so I can do something like 
>> selinuxfiletype = netfilter_contexts. This means that we would not be 
>> able to use the command line xar to create packages (I was trying to 
>> make this as simple as possible). If we move to using attributes we'd 
>> have to have a custom packager again (ala semodule_package).
>>
>>     
>
> It would be simpler though, correct? And what about an unpackager?
>   

Simpler in what way? This first patch I was trying to keep it dead 
simple by not even requiring a custom packager and instead just using 
xar CLI. If we want to use more sophisticated things in xar (like 
per-file attributes) we'd need something custom. Even if we just make a 
subdocument (which is archive-wide in scope) and add some selinux bits 
to it like policy name that can still be done with the CLI.

xar -xf foo.xar will unpackage it
>   
>> A custom packager has advantages, we can basically ensure that the 
>> package is sane (doesn't have directory trees or multiple 
>> <something>.netfilter_contexts files, etc. We can also add new 
>> properties such as the policy name to the xar XML subdocument.
>>
>>     
>
> That sounds better than relying on file extension to me.
>
>   

agreed.

>>>> +			ret = xar_prop_get(f, "name", &filename);
>>>> +			if (ret) {
>>>> +				xar_close(x);
>>>> +				throw "Error getting name property of file";
>>>>     
>>>>         
>>> Please define an exception.
>>>
>>>   
>>>       
>> Why?
>>
>>     
>
> Because none of the standard exception types seemed to fit and string
> exceptions are not a good idea.
>
>   

So I can do both to get a specific error out instead of one that could 
mean any of the files were duplicated, for example.

>>>> +			}
>>>> +			if (strstr(filename, ".mod")) {
>>>> +				// found module, do stuff
>>>>     
>>>>         
>>> Err - "do stuff"?
>>>
>>>   
>>>       
>> There is no module_read function right now so thats just a placeholder.
>>
>>     
>
> There is if the module is source :)
>
>   

nak.
>>>> +			} else if (strstr(filename, ".file_contexts")) {
>>>> +				if (!impl->file_contexts.empty()) {
>>>> +					// Found more than one file containing in .file_contexts,
>>>> +					// don't know how to handle that so bail for now
>>>> +					xar_close(x);
>>>> +					throw "Multiple file_contexts files in policy package";
>>>> +				}
>>>> +				impl->file_contexts = std::string(fbuf);
>>>> +			} else if (strstr(filename, ".seusers")) {
>>>> +				if (!impl->seusers.empty()) {
>>>> +					// Found more than one file containing in .seusers,
>>>> +					// don't know how to handle that so bail for now
>>>> +					xar_close(x);
>>>> +					throw "Multiple seusers files in policy package";
>>>> +				}
>>>> +				impl->seusers = std::string(fbuf);
>>>>     
>>>>         
>>> As we discussed privately - these copies are not great. Perhaps just use
>>> raw char*.
>>>
>>>   
>>>       
>> Ok, I don't know if this code is ever going to do anything with those 
>> and was trying to keep it in a format that would be more accessible if 
>> we needed to. I can change to char * though and see if anything develops 
>> that makes std::string make more sense.
>>
>>     
>>>> +			} else if (strstr(filename, ".user_extra")) {
>>>> +				if (!impl->user_extra.empty()) {
>>>> +					// Found more than one file containing in .user_extra,
>>>> +					// don't know how to handle that so bail for now
>>>> +					xar_close(x);
>>>> +					throw "Multiple user_extra files in policy package";
>>>> +				}
>>>> +				impl->user_extra = std::string(fbuf);
>>>> +			} else if (strstr(filename, ".netfilter_contexts")) {
>>>> +				if (!impl->netfilter_contexts.empty()) {
>>>> +					// Found more than one file containing in .netfilter_contexts,
>>>> +					// don't know how to handle that so bail for now
>>>> +					xar_close(x);
>>>> +					throw "Multiple netfilter_contexts files in policy package";
>>>> +				}
>>>> +				impl->netfilter_contexts = std::string(fbuf);
>>>> +			} else {
>>>> +				// unrecognized file, just skip it
>>>> +				continue;
>>>> +			}
>>>> +
>>>> +		}
>>>> +
>>>> +		xar_close(x);
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	PolicyPackage::PolicyPackage() {
>>>> +		init();
>>>> +	}
>>>> +
>>>> +	PolicyPackage::PolicyPackage(std::string filename) {
>>>> +		init();
>>>> +		read_package(filename);
>>>> +	}
>>>> +
>>>> +	PolicyPackage::~PolicyPackage() { delete impl; }
>>>> +
>>>> +	const Module PolicyPackage::get_policy_module() const {
>>>> +		return impl->policy_module;
>>>> +	}
>>>>     
>>>>         
>>> This should definitely not be by value. It should also likely not be
>>> const - this is how the user will retrieve the module after reading to
>>> use, correct? Then it shouldn't be const (they might want to much with
>>> it), should be a reference, and shouldn't be const for the class (as
>>> allowing users to change the module changes the object).
>>>
>>>   
>>>       
>> Fair enough. I was basically thinking users would read a module and make 
>> their own copies (eg., copying the data into a merged copy or something) 
>> but that isn't really how policyrep works so I'll change all these to be 
>> non-const.
>>
>>     
>
> One other note - if the caller is expected to take ownership for the
> lifetime of the object (which seems possible), then it should return a
> pointer or a shared_ptr.
>
>   

ok?


--
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: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Karl MacMillan]

On Tue, 2007-07-17 at 16:48 -0400, Joshua Brindle wrote:
> Karl MacMillan wrote:
> > On Tue, 2007-07-17 at 12:40 -0400, Joshua Brindle wrote:
> >   
> >> Karl MacMillan wrote:
> >>     
> > [...]
> >   
> >>>> +			// Currently we use the name of the file to decide what kind of file it is
> >>>> +			// I am fairly uncomfortable with this but it allows us to use xar to create
> >>>> +			// policy packages instead of having our own write_package function that 
> >>>> +			// assigns attributes to files instead of using names. 
> >>>>     
> >>>>         
> >>> I don't like this either - what is the alternative?
> >>>
> >>>   
> >>>       
> >> Ok, xar has an XML ToC for the file. It also provides an API to set 
> >> arbitrary attributes for each file, so I can do something like 
> >> selinuxfiletype = netfilter_contexts. This means that we would not be 
> >> able to use the command line xar to create packages (I was trying to 
> >> make this as simple as possible). If we move to using attributes we'd 
> >> have to have a custom packager again (ala semodule_package).
> >>
> >>     
> >
> > It would be simpler though, correct? And what about an unpackager?
> >   
> 
> Simpler in what way?

Simpler than our current semodule_package and all of the library support
behind it.

>  This first patch I was trying to keep it dead 
> simple by not even requiring a custom packager and instead just using 
> xar CLI. If we want to use more sophisticated things in xar (like 
> per-file attributes) we'd need something custom. Even if we just make a 
> subdocument (which is archive-wide in scope) and add some selinux bits 
> to it like policy name that can still be done with the CLI.
> 
> xar -xf foo.xar will unpackage it

Ok - then I vote for a custom packaging tool. If nothing else it can
retain some similarity to the current tool syntax.

[..]

> >>>
> >>>   
> >>>       
> >> There is no module_read function right now so thats just a placeholder.
> >>
> >>     
> >
> > There is if the module is source :)
> >
> >   
> 
> nak.

Why? Last time we discussed it you wanted support for storing additional
information but the only examples that you had were original filename
and line number. This is already supported (it has to be for the M4
processing). So what else? If nothing else, why have an additional
format?

Karl


--
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: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Joshua Brindle]

Karl MacMillan wrote:
>
>>  This first patch I was trying to keep it dead 
>> simple by not even requiring a custom packager and instead just using 
>> xar CLI. If we want to use more sophisticated things in xar (like 
>> per-file attributes) we'd need something custom. Even if we just make a 
>> subdocument (which is archive-wide in scope) and add some selinux bits 
>> to it like policy name that can still be done with the CLI.
>>
>> xar -xf foo.xar will unpackage it
>>     
>
> Ok - then I vote for a custom packaging tool. If nothing else it can
> retain some similarity to the current tool syntax.
>
>   
Ok, I'll go ahead and do that for the next version of the patches. We 
are basically taking a standard archive format and one-offing it, 
fortunately the format supports that kind of thing so we can continue 
using all their library infrastructure, which is a huge win over what we 
are doing now.

> [..]
>
>   
>>>>>   
>>>>>       
>>>>>           
>>>> There is no module_read function right now so thats just a placeholder.
>>>>
>>>>     
>>>>         
>>> There is if the module is source :)
>>>
>>>   
>>>       
>> nak.
>>     
>
> Why? Last time we discussed it you wanted support for storing additional
> information but the only examples that you had were original filename
> and line number. This is already supported (it has to be for the M4
> processing). So what else? If nothing else, why have an additional
> format?
>   

It doesn't seem reasonable to essentially reproduce the m4 preprocessing 
statements just to keep a single parser. just look at a preprocessed 
module and see how nasty it looks:

#line 156537 "tmp/all_interfaces.conf"
       
#line 1 "policy/modules/services/sendmail.te"

       
#line 2
      
#line 2
                module sendmail 1.3.1;
#line 2

#line 2
                require {
#line 2
                        role system_r;
#line 2


That isn't exactly what I had in mind when I said we'd want to store 
extra info. We'd also have to add parser support for that mess to the 
source parser, non-ideal I think when we can path the data in something 
easily readable (by machine) without loads of junk that isn't necessary.


--
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: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Karl MacMillan]

On Tue, 2007-07-17 at 17:01 -0400, Joshua Brindle wrote:
> Karl MacMillan wrote:

[...]

> 
> It doesn't seem reasonable to essentially reproduce the m4 preprocessing 
> statements just to keep a single parser. just look at a preprocessed 
> module and see how nasty it looks:
> 
> #line 156537 "tmp/all_interfaces.conf"
>        
> #line 1 "policy/modules/services/sendmail.te"
> 
>        
> #line 2
>       
> #line 2
>                 module sendmail 1.3.1;
> #line 2
> 
> #line 2
>                 require {
> #line 2
>                         role system_r;
> #line 2
> 

1) so what?
2) it doesn't have to be that ugly - the repeated line number thing is
an artifact of M4.

> 
> That isn't exactly what I had in mind when I said we'd want to store 
> extra info. We'd also have to add parser support for that mess to the 
> source parser, non-ideal I think when we can path the data in something 
> easily readable (by machine) without loads of junk that isn't necessary.
> 

But the lexer already supports these - it's done. I just can't see
inventing an entirely new file format just because you think this is
"ugly". It works, it's there now, and we can make it prettier.

Karl


--
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: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Christopher J. PeBenito]

On Tue, 2007-07-17 at 17:11 -0400, Karl MacMillan wrote:
> On Tue, 2007-07-17 at 17:01 -0400, Joshua Brindle wrote:
> > Karl MacMillan wrote:
> 
> [...]
> 
> > 
> > It doesn't seem reasonable to essentially reproduce the m4 preprocessing 
> > statements just to keep a single parser. just look at a preprocessed 
> > module and see how nasty it looks:
> > 
> > #line 156537 "tmp/all_interfaces.conf"
> >        
> > #line 1 "policy/modules/services/sendmail.te"
> > 
> >        
> > #line 2
> >       
> > #line 2
> >                 module sendmail 1.3.1;
> > #line 2
> > 
> > #line 2
> >                 require {
> > #line 2
> >                         role system_r;
> > #line 2
> > 
> 
> 1) so what?
> 2) it doesn't have to be that ugly - the repeated line number thing is
> an artifact of M4.

The line number and file name comments are optional in M4 and only
turned on (via M4's -s option) since the current compiler can leverage
them for the file name and line number in error messages.

-- 
Chris PeBenito
Tresys Technology, LLC
(410) 290-1411 x150


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

[POLICYREP] [RFC/PATCH 3/3] policy package tests

[method]

--

--- a/libpolicyrep/tests/libpolicyrep-test.cpp	(revision 2495)
+++ b/libpolicyrep/tests/libpolicyrep-test.cpp	(working copy)
@@ -20,6 +20,7 @@
 
 #include <policyrep/policy.hpp>
 #include <policyrep/parse.hpp>
+#include <policyrep/policy_package.hpp>
 
 #include <sstream>
 #include <iostream>
@@ -57,6 +58,27 @@
 
 	parsed_mod->append_children(mod->children().begin(), mod->children().end());
 
+	// policy package teddsts
+	std::cout << "============ Reading policy package ===========" << std::endl;
+
+	try {
+		PolicyPackagePtr polpackage(new PolicyPackage("policy_package/test.xar"));
+		const std::string fc = polpackage->get_file_contexts();
+		if (fc.empty())
+			std::cout << "Error, file_contexts file not in package" << std::endl;
+		const std::string su = polpackage->get_seusers();
+		if (su.empty())
+			std::cout << "Error, seusers file not in package" << std::endl;
+		const std::string ue = polpackage->get_user_extra();
+		if (ue.empty())
+			std::cout << "Error, user_extra file not in package" << std::endl;
+		const std::string nc = polpackage->get_netfilter_contexts();
+		if (nc.empty())
+			std::cout << "Error, netfilter_contexts file not in package" << std::endl;
+	} catch (const char *e) {
+		std::cout << "Exception thrown: " << e << std::endl;
+	}
+
 }
 
 int main(int argc, char **argv)
--- a/libpolicyrep/tests/Makefile	(revision 2495)
+++ b/libpolicyrep/tests/Makefile	(working copy)
@@ -12,7 +12,7 @@
 all: $(EXE)
 
 $(EXE): $(objs) $(LIBPOLICYREP)
-	g++ $(CFLAGS) $(objs) $(LIBPOLICYREP) -lboost_serialization -o $@
+	g++ $(CFLAGS) $(objs) $(LIBPOLICYREP) -lboost_serialization -lxar -o $@
 
 %.o:  %.cpp
 	g++ $(CFLAGS) -fPIC -c -o $@ $<

-- 

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