Merge pull request #1170 from stevvooe/identifier-validation

namespaces, identifiers: split validation

identifiers/validate.go

@@ -1,17 +1,11 @@
-// Package identifiers provides common validation for identifiers, keys and ids
+// Package identifiers provides common validation for identifiers and keys
 // across containerd.
 //
-// To allow such identifiers to be used across various contexts safely, the character
+// Identifiers in containerd must be a alphanumeric, allowing limited
-// set has been restricted to that defined for domains in RFC 1035, section
+// underscores, dashes and dots.
-// 2.3.1. This will make identifiers safe for use across networks, filesystems
-// and other media.
 //
-// The identifier specification departs from RFC 1035 in that it allows
+// While the character set may be expanded in the future, identifiers
-// "labels" to start with number and only enforces a total length restriction
+// are guaranteed to be safely used as filesystem path components.
-// of 76 characters.
-//
-// While the character set may be expanded in the future, identifiers are
-// guaranteed to be safely used as filesystem path components.
 package identifiers
 
 import (
@@ -23,26 +17,27 @@ import (
 
 const (
 	maxLength  = 76
-	charclass = `[A-Za-z0-9]+`
+	alphanum   = `[A-Za-z0-9]+`
-	label     = charclass + `(:?[-]+` + charclass + `)*`
+	separators = `[._-]`
 )
 
 var (
-	// identifierRe validates that a identifier matches valid identifiers.
+	// identifierRe defines the pattern for valid identifiers.
-	//
+	identifierRe = regexp.MustCompile(reAnchor(alphanum + reGroup(separators+reGroup(alphanum)) + "*"))
-	// Rules for domains, defined in RFC 1035, section 2.3.1, are used for
-	// identifiers.
-	identifierRe = regexp.MustCompile(reAnchor(label + reGroup("[.]"+reGroup(label)) + "*"))
 )
 
 // Validate return nil if the string s is a valid identifier.
 //
-// identifiers must be valid domain identifiers according to RFC 1035, section 2.3.1.  To
+// identifiers must be valid domain names according to RFC 1035, section 2.3.1.  To
 // enforce case insensitvity, all characters must be lower case.
 //
 // In general, identifiers that pass this validation, should be safe for use as
-// a domain identifier or filesystem path component.
+// a domain names or filesystem path component.
 func Validate(s string) error {
+	if len(s) == 0 {
+		return errors.Wrapf(errdefs.ErrInvalidArgument, "identifier must not be empty")
+	}
+
 	if len(s) > maxLength {
 		return errors.Wrapf(errdefs.ErrInvalidArgument, "identifier %q greater than maximum length (%d characters)", s, maxLength)
 	}

identifiers/validate_test.go

@@ -13,13 +13,12 @@ func TestValidIdentifiers(t *testing.T) {
 		"Default",
 		t.Name(),
 		"default-default",
-		"default--default",
 		"containerd.io",
 		"foo.boo",
 		"swarmkit.docker.io",
-		"zn--e9.org", // or something like it!
 		"0912341234",
 		"task.0.0123456789",
+		"underscores_are_allowed",
 		strings.Repeat("a", maxLength),
 	} {
 		t.Run(input, func(t *testing.T) {
@@ -32,6 +31,7 @@ func TestValidIdentifiers(t *testing.T) {
 
 func TestInvalidIdentifiers(t *testing.T) {
 	for _, input := range []string{
+		"",
 		".foo..foo",
 		"foo/foo",
 		"foo/..",
@@ -39,7 +39,9 @@ func TestInvalidIdentifiers(t *testing.T) {
 		"foo.-boo",
 		"-foo.boo",
 		"foo.boo-",
-		"foo_foo.boo_underscores", // boo-urns?
+		"but__only_tasteful_underscores",
+		"zn--e9.org", // or something like it!
+		"default--default",
 		strings.Repeat("a", maxLength+1),
 	} {
 

linux/runtime.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/boltdb/bolt"
 	"github.com/containerd/containerd/api/types"
+	"github.com/containerd/containerd/identifiers"
 	client "github.com/containerd/containerd/linux/shim"
 	shim "github.com/containerd/containerd/linux/shim/v1"
 	"github.com/containerd/containerd/log"
@@ -127,6 +128,11 @@ func (r *Runtime) Create(ctx context.Context, id string, opts runtime.CreateOpts
 	if err != nil {
 		return nil, err
 	}
+
+	if err := identifiers.Validate(id); err != nil {
+		return nil, errors.Wrapf(err, "invalid task id")
+	}
+
 	bundle, err := newBundle(filepath.Join(r.root, namespace), namespace, id, opts.Spec.Value)
 	if err != nil {
 		return nil, err

linux/shim/exec.go

@@ -16,6 +16,7 @@ import (
 	"golang.org/x/sys/unix"
 
 	"github.com/containerd/console"
+	"github.com/containerd/containerd/identifiers"
 	shimapi "github.com/containerd/containerd/linux/shim/v1"
 	"github.com/containerd/fifo"
 	runc "github.com/containerd/go-runc"
@@ -40,6 +41,10 @@ type execProcess struct {
 }
 
 func newExecProcess(context context.Context, path string, r *shimapi.ExecProcessRequest, parent *initProcess, id string) (process, error) {
+	if err := identifiers.Validate(id); err != nil {
+		return nil, errors.Wrapf(err, "invalid exec id")
+	}
+
 	e := &execProcess{
 		id:     id,
 		parent: parent,

linux/shim/init.go

@@ -17,6 +17,7 @@ import (
 	"golang.org/x/sys/unix"
 
 	"github.com/containerd/console"
+	"github.com/containerd/containerd/identifiers"
 	"github.com/containerd/containerd/linux/runcopts"
 	shimapi "github.com/containerd/containerd/linux/shim/v1"
 	"github.com/containerd/containerd/log"
@@ -52,6 +53,9 @@ type initProcess struct {
 }
 
 func newInitProcess(context context.Context, path, namespace string, r *shimapi.CreateTaskRequest) (*initProcess, error) {
+	if err := identifiers.Validate(r.ID); err != nil {
+		return nil, errors.Wrapf(err, "invalid task id")
+	}
 	var options runcopts.CreateOptions
 	if r.Options != nil {
 		v, err := typeurl.UnmarshalAny(r.Options)

linux/shim/service.go

@@ -14,6 +14,7 @@ import (
 	"github.com/containerd/console"
 	events "github.com/containerd/containerd/api/services/events/v1"
 	"github.com/containerd/containerd/api/types/task"
+	"github.com/containerd/containerd/errdefs"
 	evt "github.com/containerd/containerd/events"
 	shimapi "github.com/containerd/containerd/linux/shim/v1"
 	"github.com/containerd/containerd/log"
@@ -77,12 +78,9 @@ type Service struct {
 }
 
 func (s *Service) Create(ctx context.Context, r *shimapi.CreateTaskRequest) (*shimapi.CreateTaskResponse, error) {
-	if r.ID == "" {
-		return nil, grpc.Errorf(codes.InvalidArgument, "task id cannot be empty")
-	}
 	process, err := newInitProcess(ctx, s.path, s.namespace, r)
 	if err != nil {
-		return nil, err
+		return nil, errdefs.ToGRPC(err)
 	}
 	s.mu.Lock()
 	// save the main task id and bundle to the shim for additional requests

metadata/namespaces.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/boltdb/bolt"
 	"github.com/containerd/containerd/errdefs"
-	"github.com/containerd/containerd/identifiers"
 	"github.com/containerd/containerd/namespaces"
 	"github.com/pkg/errors"
 )
@@ -24,7 +23,7 @@ func (s *namespaceStore) Create(ctx context.Context, namespace string, labels ma
 		return err
 	}
 
-	if err := identifiers.Validate(namespace); err != nil {
+	if err := namespaces.Validate(namespace); err != nil {
 		return err
 	}
 

namespaces/context.go

@@ -4,7 +4,6 @@ import (
 	"os"
 
 	"github.com/containerd/containerd/errdefs"
-	"github.com/containerd/containerd/identifiers"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 )
@@ -54,7 +53,7 @@ func NamespaceRequired(ctx context.Context) (string, error) {
 		return "", errors.Wrapf(errdefs.ErrFailedPrecondition, "namespace is required")
 	}
 
-	if err := identifiers.Validate(namespace); err != nil {
+	if err := Validate(namespace); err != nil {
 		return "", errors.Wrap(err, "namespace validation")
 	}
 

namespaces/validate.go

@@ -0,0 +1,67 @@
+// Package namespaces provides tools for working with namespaces across
+// containerd.
+//
+// Namespaces collect resources such as containers and images, into a unique
+// identifier space. This means that two applications can use the same
+// identifiers and not conflict while using containerd.
+//
+// This package can be used to ensure that client and server functions
+// correctly store the namespace on the context.
+package namespaces
+
+import (
+	"regexp"
+
+	"github.com/containerd/containerd/errdefs"
+	"github.com/pkg/errors"
+)
+
+const (
+	maxLength = 76
+	alpha     = `[A-Za-z]`
+	alphanum  = `[A-Za-z0-9]+`
+	label     = alpha + alphanum + `(:?[-]+` + alpha + alphanum + `)*`
+)
+
+var (
+	// namespaceRe validates that a namespace matches valid identifiers.
+	//
+	// Rules for domains, defined in RFC 1035, section 2.3.1, are used for
+	// namespaces.
+	namespaceRe = regexp.MustCompile(reAnchor(label + reGroup("[.]"+reGroup(label)) + "*"))
+)
+
+// Validate returns nil if the string s is a valid namespace.
+//
+// To allow such namespace identifiers to be used across various contexts
+// safely, the character set has been restricted to that defined for domains in
+// RFC 1035, section 2.3.1. This will make namespace identifiers safe for use
+// across networks, filesystems and other media.
+//
+// The identifier specification departs from RFC 1035 in that it allows
+// "labels" to start with number and only enforces a total length restriction
+// of 76 characters.
+//
+// While the character set may be expanded in the future, namespace identifiers
+// are guaranteed to be safely used as filesystem path components.
+//
+// For the most part, this doesn't need to be called directly when using the
+// context-oriented functions.
+func Validate(s string) error {
+	if len(s) > maxLength {
+		return errors.Wrapf(errdefs.ErrInvalidArgument, "namespace %q greater than maximum length (%d characters)", s, maxLength)
+	}
+
+	if !namespaceRe.MatchString(s) {
+		return errors.Wrapf(errdefs.ErrInvalidArgument, "namespace %q must match %v", s, namespaceRe)
+	}
+	return nil
+}
+
+func reGroup(s string) string {
+	return `(?:` + s + `)`
+}
+
+func reAnchor(s string) string {
+	return `^` + s + `$`
+}

namespaces/validate_test.go

@@ -0,0 +1,55 @@
+package namespaces
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/containerd/containerd/errdefs"
+)
+
+func TestValidNamespaces(t *testing.T) {
+	for _, input := range []string{
+		"default",
+		"Default",
+		t.Name(),
+		"default-default",
+		"default--default",
+		"containerd.io",
+		"foo.boo",
+		"swarmkit.docker.io",
+		"zn--e9.org", // or something like it!
+		strings.Repeat("a", maxLength),
+	} {
+		t.Run(input, func(t *testing.T) {
+			if err := Validate(input); err != nil {
+				t.Fatalf("unexpected error: %v != nil", err)
+			}
+		})
+	}
+}
+
+func TestInvalidNamespaces(t *testing.T) {
+	for _, input := range []string{
+		".foo..foo",
+		"foo/foo",
+		"foo/..",
+		"foo..foo",
+		"foo.-boo",
+		"foo.-boo.bar",
+		"-foo.boo",
+		"foo.boo-",
+		"foo_foo.boo_underscores", // boo-urns?
+		"0912341234",
+		"task.0.0123456789",
+		strings.Repeat("a", maxLength+1),
+	} {
+
+		t.Run(input, func(t *testing.T) {
+			if err := Validate(input); err == nil {
+				t.Fatal("expected invalid error")
+			} else if !errdefs.IsInvalidArgument(err) {
+				t.Fatal("error should be an invalid identifier error")
+			}
+		})
+	}
+}