[systemd-devel] [PATCH 2/2 v3] socket: introduce SELinuxContextFromNet option

[Michal Sekletar]

On Tue, Aug 26, 2014 at 08:54:01PM +0200, Lennart Poettering wrote:
> On Mon, 25.08.14 10:02, Michal Sekletar (msekleta at redhat.com) wrote:
> 
> > +int label_get_our_label(char **label) {
> > +        int r = 0;
> > +        char *l = NULL;
> > +
> > +#ifdef HAVE_SELINUX
> > +        r = getcon(&l);
> > +        if (r < 0)
> > +                return r;
> > +
> > +        *label = l;
> > +#endif
> > +
> > +        return r;
> > +}
> 
> Hmm, we shouldn't return success if selinux support is turned off, and
> we don't write anything to *label... This really should return -ENOTSUP
> or so, i figure....

Are you suggesting that we should change other functions in label.c as well?
AFAICT all non-void functions from that module implement same pattern now.

> 
> >  
> > diff --git a/src/shared/label.h b/src/shared/label.h
> > index 7294820..0df03f7 100644
> > --- a/src/shared/label.h
> > +++ b/src/shared/label.h
> > @@ -25,6 +25,13 @@
> >  #include <stdbool.h>
> >  #include <sys/socket.h>
> >  
> > +#include "util.h"
> > +
> > +#ifdef HAVE_SELINUX
> > +#include <selinux/selinux.h>
> > +#include <selinux/context.h>
> > +#endif
> > +
> >  int label_init(const char *prefix);
> >  void label_finish(void);
> >  
> > @@ -39,6 +46,8 @@ void label_context_clear(void);
> >  void label_free(const char *label);
> >  
> >  int label_get_create_label_from_exe(const char *exe, char **label);
> > +int label_get_our_label(char **label);
> > +int label_get_child_mls_label(int socket_fd, const char *exec, char **label);
> >  
> >  int label_mkdir(const char *path, mode_t mode);
> >  
> > @@ -49,3 +58,11 @@ int label_apply(const char *path, const char *label);
> >  int label_write_one_line_file_atomic(const char *fn, const char *line);
> >  int label_write_env_file(const char *fname, char **l);
> >  int label_fopen_temporary(const char *path, FILE **_f, char **_temp_path);
> > +
> > +#ifdef HAVE_SELINUX
> > +DEFINE_TRIVIAL_CLEANUP_FUNC(security_context_t, freecon);
> > +DEFINE_TRIVIAL_CLEANUP_FUNC(context_t, context_free);
> > +
> > +#define _cleanup_security_context_free_ _cleanup_(freeconp)
> > +#define _cleanup_context_free_ _cleanup_(context_freep)
> > +#endif
> 
> Hmm, wouldn't it suffice to have the latter four lines simply in
> label.c, not in the header files? I'd prefer if we didn't have to
> include the selinux headers from the header file...

Quick check revealed that we are using for example security_context_t type in
following modules other than label.c: 

src/core/selinux-access.c
src/core/selinux-setup.c
src/journal/journald-server.c
src/journal/journald-stream.c /* now cleanup macros can't be of any use here */
src/nspawn/nspawn.c /* here as well */

Honestly I don't really care either way. Given that use cases in above modules
for new macros are really minimal (1-2 per module) we can just put those four
lines to C file make use of macros there and leave the rest as is.

> 
> Otherwise looks great!

Thanks, again, for the review. It is much appreciated.

> 
> Lennart

Michal
> 
> -- 
> Lennart Poettering, Red Hat