[lxc-devel] [PATCH] Validate container name during creation

[Robert Vogelgesang]

Hi Serge,

On Thu, Feb 05, 2015 at 05:40:54PM +0000, Serge Hallyn wrote:
> Quoting Robert Vogelgesang (vogel at users.sourceforge.net):
> > Hello,
> > 
> > On Thu, Feb 05, 2015 at 05:50:39PM +0200, Joel Nider wrote:
> > > The name used to identify the container on the host is also used as the 
> > > host
> > > name of the container itself. Therefore, the name should be restricted to 
> > > a
> > > legal Linux hostname, which is specified in RFC 1123
> 
> Thanks for the patch, Joel.  One other comment below.
> 
> > no, this should not be the job of the lxc create API, IMHO.
> > It's the template script that re-uses the container name as the
> > container's hostname, so it should be the template's job to ensure
> > the name conforms to the rules.
> > 
> > I don't mind checking the value of the configuration item lxc.utsname,
> > which actually defines the container's hostname, according to the
> > RFC 1123 rules, but please don't do this for the container's name.
> 
> So you're suggesting using the same validate_hostname() function
> at src/lxc/confile.c:config_utsname() ?

well, if you want to ensure that the hostname conforms to the standards,
then config_utsname() would be a better place to enforce this.

But PLEASE do the check in such a way that FQDNs get accepted as well.
Rejecting a FQDN for lxc.utsname will lead to trouble for some Linux
distro's, I'm sure.

	Robert

> 
> That should still end up being caught at lxc-create since we re-read
> the config file at the end of lxcapi_create().
> 
> > 	Robert
> > 
> > > (http://tools.ietf.org/html/rfc1123#page-13). Basically it says the host 
> > > name
> > > is composed of up to 63 alphanumeric ASCII characters (case insensitive) 
> > > as
> > > well as '-'.
> > > 
> > > See this thread for more details:
> > > https://lists.linuxcontainers.org/pipermail/lxc-devel/2014-December/011007.html
> > > 
> > > Signed-off-by: Joel Nider <joeln at il.ibm.com>
> > > ---
> > >  src/lxc/lxccontainer.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > index e02ee93..7cba771 100644
> > > --- a/src/lxc/lxccontainer.c
> > > +++ b/src/lxc/lxccontainer.c
> > > @@ -36,6 +36,7 @@
> > >  #include <stdint.h>
> > >  #include <grp.h>
> > >  #include <sys/syscall.h>
> > > +#include <ctype.h>
> > > 
> > >  #include <lxc/lxccontainer.h>
> > >  #include <lxc/version.h>
> > > @@ -66,7 +67,7 @@
> > >  #endif
> > > 
> > >  #define MAX_BUFFER 4096
> > > -
> > > +#define MAX_LENGTH_HOSTNAME 63
> > >  #define NOT_SUPPORTED_ERROR "the requested function %s is not currently 
> > > supported with unprivileged containers"
> > > 
> > >  /* Define faccessat() if missing from the C library */
> > > @@ -190,6 +191,37 @@ static void remove_partial(struct lxc_container *c, 
> > > int fd)
> > >                 SYSERROR("Error unlink partial file %s", path);
> > >  }
> > > 
> > > +/* Ensure requested hostname follows RFC 1123
> > > + * In our case, that means simple host name (not FQDN)
> > > + * characters in the set {[A-Z], [0-9], '-'} (no '.')
> > > + * maximum length of 63 characters
> > > + */
> > > +static int validate_hostname(struct lxc_container *c)
> > > +{
> > > +   char *a;
> > > +   int count = 0;
> > > +
> > > +   if (!c)
> > > +      return MAX_LENGTH_HOSTNAME;
> > > +
> > > +   a = c->name;
> > > +   while (*a) {
> > > +      count++;
> > > +      if (count > MAX_LENGTH_HOSTNAME)
> > > +         return MAX_LENGTH_HOSTNAME;
> > > +
> > > +      if (!(isalnum(*a) | (*a == '-')))
> 
> Would prefer to see this as
> 
> 	if (!valid_hostname_char(*a))
> 		return count;
> 
> with valid_hostname_char(const char a) {
> 	if (isalnum(a))
> 		return true;
> 	if (a == '-')
> 		return true;
> 	return false;
> }
> 
> above.
> 
> > > +         return count;
> > > +
> > > +     a++;
> > > +   }
> > > +
> > > +   if (count == 0)
> > > +      return MAX_LENGTH_HOSTNAME;
> > > +
> > > +   return 0;
> > > +}
> > > +
> > >  /* LOCKING
> > >   * 1. container_mem_lock(c) protects the struct lxc_container from 
> > > multiple threads.
> > >   * 2. container_disk_lock(c) protects the on-disk container data - in 
> > > particular the
> > > @@ -1235,6 +1267,7 @@ static bool lxcapi_create(struct lxc_container *c, 
> > > const char *t,
> > >         pid_t pid;
> > >         char *tpath = NULL;
> > >         int partial_fd;
> > > +   int err;
> > > 
> > >         if (!c)
> > >                 return false;
> > > @@ -1247,6 +1280,13 @@ static bool lxcapi_create(struct lxc_container *c, 
> > > const char *t,
> > >                 }
> > >         }
> > > 
> > > +   /* validate the container name */
> > > +   err = validate_hostname(c);
> > > +   if (err) {
> > > +      ERROR("Invalid hostname: %s (character %i)", c->name, err);
> > > +      goto out;
> > > +   }
> > > +
> > >         /*
> > >          * If a template is passed in, and the rootfs already is defined 
> > > in
> > >          * the container config and exists, then * caller is trying to 
> > > create
> > > --
> > > 1.9.1
> > > 
> > > 
> > > 
> > > Joel Nider
> > > Virtualization Research
> > > IBM Research and Development
> > > Haifa Research Lab
> > > 
> > > Phone: 972-4-829-6326 | Mobile: 972-54-3155635
> > > E-mail: JOELN at il.ibm.com
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > lxc-devel mailing list
> > > lxc-devel at lists.linuxcontainers.org
> > > http://lists.linuxcontainers.org/listinfo/lxc-devel
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel