On Wednesday, October 24, 2012 03:53:01 PM David Malcolm wrote:
> On Mon, 2012-10-08 at 16:23 -0400, Paul Moore wrote:
> > This patch adds Python bindings using Cython, see http://www.cython.org
> > for more information. There are also some minor tweaks to the build
> > macros while we are mucking around in macros.mk; nothing major.
Hi Dave,
Thanks for looking over the patches and sending your thoughts, my comments are
below ...
> Note that by doing this in Cython we (in theory) get both Python 2 and
> Python 3 support. Out of interest, have you tried building this
> against Python 3?
No, I haven't, but that seems like a good thing to do at some point. Although
I'm just getting to the point of being able to muddle through Python 2,
hopefully Python 3 isn't a sea change ...
> [...]
>
> > diff --git a/src/python/seccomp.pyx b/src/python/seccomp.pyx
>
> [...]
>
> > +Example:
> > +
> > + import sys
> > + from seccomp import *
> > +
> > + # create a filter object with a default KILL action
> > + f = SyscallFilter(KILL)
>
> Potentially you could change the example to use keyword arguments like
> this, in the hope that this is more self-documenting:
>
> f = SyscallFilter(def_action=KILL)
Okay, that makes sense.
> > + # add syscall filter rules to allow certain syscalls
> > + f.add_rule(ALLOW, "open")
> > + f.add_rule(ALLOW, "close")
> > + f.add_rule(ALLOW, "read", Arg(0, Arg.EQ, sys.stdin.fileno()))
> > + f.add_rule(ALLOW, "write", Arg(0, Arg.EQ, sys.stdout.fileno()))
> > + f.add_rule(ALLOW, "write", Arg(0, Arg.EQ, sys.stderr.fileno()))
>
> From an API design POV it would be nicer to simply pass in "sys.stdin"
> rather than the fileno, but given the constraint that it's an fd
> (imposed by the underlying interface) what you wrote is probably the
> most readable compromise.
>
> Potentially Arg.__cinit__ could do the conversion for the user, with
> something like this:
>
> if isinstance(datum, file):
> datum = datum.fileno()
>
> That way you could write:
> f.add_rule(ALLOW, "read", Arg(0, Arg.EQ, sys.stdin)))
> directly. I'm not sure if the convenience outweighs the slight element
> of magic.
This is a bit of a slippery slope - I don't want to have to do magic type
conversions for every Python object type in the binding/shim layer - but the
file object seems like it will be a common case so I'll go ahead and add it.
If you, or anyone else, can think of any other standard Python objects that
should be treated the same way please let me know.
> Is there any metadata for the syscalls exposed anywhere? Presumably a
> nicer API might be something like this:
>
> f.add_rule(ALLOW, "read", fildes=sys.stdin)
>
> where the keyword argument "fildes" gets looked up in metadata and
> mapped to argument 0.
I would *love* it if there was syscall metadata somewhere we could look up, it
would make some of the work we've done recently unnecessary, but unfortunately
the answer is no.
> Whilst I'm bikeshedding away, why not just:
>
> f.allow("read", fildes=sys.stdin)
>
> though perhaps that's straying too far from the underlying C API?
Yes, I'd like to try keep the API as consistent as possible across languages.
Although I'll keep my mind open to possibly expanding the Python API to
support something like that in the future.
> [...]
>
> > + def to_c(self):
> > + """ Convert the object into a C structure.
> > +
> > + Description:
> > + Helper function which should only be used internally by
> > + SyscallFilter objects.
> > + """
> > + return self._arg
>
> Perhaps make this a cdef rather than a def? IIRC this will make it only
> accessible from the .pyx file.
Okay.
> Note for posterity: this function exists to make it easier for
> add_rule() et al to invoke libseccomp.seccomp_rule_add, a direct binding
> for the C function seccomp_rule_add(). The latter takes a variable
> number of arguments, and these arguments are structs (as opposed to ptrs
> to structs). Variadic C functions are difficult to write bindings for,
> and passing in structs makes it even harder, hence we need this helper
> function (or, at least, we tried hard to get it work, and this seemed to
> be the only way to make it happen).
>
> It might be worth documenting that the function exists to help deal with
> the variadic functions.
Makes sense.
> > +cdef class SyscallFilter:
> > + """ Python object representing a seccomp syscall filter. """
> > + cdef int _def_action
> > + cdef libseccomp.scmp_filter_ctx _ctx
> > +
> > + def __cinit__(self, int def_action):
> > + """ Initialize the filter state
> > +
> > + Keyword arguments:
> > + def_action - the default filter action
>
> This perhaps should be "defaction" (though I may be overreading PEP8);
> if so, update the example above.
Okay, easy enough.
> BTW, the documentation style seems a bit weird; these are "arguments",
> not "keyword arguments". From my reading of the code they are usable in
> both a positional or a keyword manner.
Okay, changed.
> [...]
>
> > + def add_rule(self, int action, str syscall, *args):
> >
> > + the rule to the best possible match. If you don't want this fule
>
> typo: s/fule/rule/
>
> > + for i, arg in enumerate(args):
> > + c_arg[i] = arg.to_c()
> > + if len(args) == 0:
> > + rc = libseccomp.seccomp_rule_add(self._ctx, action,
> > syscall_num, 0) + elif len(args) == 1:
> > + rc = libseccomp.seccomp_rule_add(self._ctx, action,
> > syscall_num, + len(args),
> > + c_arg[0])
>
> > + elif len(args) == 2:
> [etc] : this code could use a comment to explain why you had to
> hand-unroll for the various possible argument lengths (because
> seccomp_rule_add is variadic and so we have to give Cython extra
> information on what the types of the arguments actually are).
Done.
> > + def add_rule_exactly(self, int action, str syscall, *args):
> [...]
>
> > + if len(args) == 0:
> > + rc = libseccomp.seccomp_rule_add_exact(self._ctx, action,
> > + syscall_num, 0)
>
> Same again here
Yep, also updated.
> > + def export_pfc(self, file):
> > + """ Export the filter in PFC format.
> > +
> > + Keyword arguments:
> > + file - the output file
> > +
> > + Description:
> > + Output the filter in Pseudo Filter Code (PFC) to the given file.
> > + The output is functionally equivalent to the BPF based filter
> > + which is loaded into the Linux Kernel.
> > + """
> > + rc = libseccomp.seccomp_export_pfc(self._ctx, file.fileno())
>
> It seems a shame that is has to have a filedescriptor, but I guess
> that's the fault of the underlying API.
We really want to avoid having the library open the file directly so we either
have to pass in a FILE* or a fd; of the two, the fd is more generic and easier
to deal with from a library perspective.
> > diff --git a/src/python/setup.py b/src/python/setup.py
>
> [...]
>
> > + cmdclass = {'build_ext': build_ext},
> > + ext_modules = [
> > + Extension("seccomp", ["seccomp.pyx"],
> > + extra_objects=["../libseccomp.a"])
>
> Is this statically-linking the library?
Yes, well at least it statically links libseccomp, the Python module remains
dynamically linked with other libraries.
> Shouldn't it be dynamically-linked instead?
I suppose it is a bit of a judgement call. I started with a static link
because when I was first writing this the example I was following used static
linkage. I've kept it up because it makes development a bit easier (I don't
have to worry about collisions with the system libraries) and for that reason
I'm tempted to keep it that way.
I also like that it allows the native library and the Python module to exist
independently if needed.
> (for Fedora we're likely to want to split the python bindings out into a
> separate subpackage, but if it's statically-linked that introduces the risk
> that you could fix libseccomp but have an unfixed copy lurking statically
> linked within the python bindings).
I'm going to update the Fedora package like you describe, the libseccomp.spec
file will contain both the native library package and the Python module
package. Because of this, both will be be built when we push an update and
both will be made available for update simultaneously.
Worrying about an unfixed Python module on the filesystem is the same as
worrying about an unfixed native library; you either update the packages on
your system or you don't. The only gotcha I can see is if there is a Fedora
policy which requires bindings such as these to only used shared libraries;
are you aware of such a policy (I haven't looked into Fedora's packaging
guidelines for Python modules yet)?
> Hope this is helpful
> Dave
Very much so, thank you for taking the time to review the patch!
--
paul moore
security and virtualization @ redhat
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_sfd2d_oct
_______________________________________________
libseccomp-discuss mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libseccomp-discuss
