From 9e5bd5a2dc69523a96bbb59090b9bff9de20b87d Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 12 Jul 2017 14:46:47 -0700 Subject: [PATCH] namespaces, identifiers: split validation After review, there are cases where having common requirements for namespaces and identifiers creates contention between applications. One example is that it is nice to have namespaces comply with domain name requirement, but that does not allow underscores, which are required for certain identifiers. The namespaces validation has been reverted to be in line with RFC 1035. Existing identifiers has been modified to allow simply alpha-numeric identifiers, while limiting adjacent separators. We may follow up tweaks for the identifier charset but this split should remove the hard decisions. Signed-off-by: Stephen J Day --- identifiers/validate.go | 37 +++++++++----------- identifiers/validate_test.go | 8 +++-- linux/runtime.go | 6 ++++ linux/shim/exec.go | 5 +++ linux/shim/init.go | 4 +++ linux/shim/service.go | 6 ++-- metadata/namespaces.go | 3 +- namespaces/context.go | 3 +- namespaces/validate.go | 67 ++++++++++++++++++++++++++++++++++++ namespaces/validate_test.go | 55 +++++++++++++++++++++++++++++ 10 files changed, 162 insertions(+), 32 deletions(-) create mode 100644 namespaces/validate.go create mode 100644 namespaces/validate_test.go diff --git a/identifiers/validate.go b/identifiers/validate.go index b99881ea8..37f9b72ff 100644 --- a/identifiers/validate.go +++ b/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 -// set has been restricted to that defined for domains in RFC 1035, section -// 2.3.1. This will make identifiers safe for use across networks, filesystems -// and other media. +// Identifiers in containerd must be a alphanumeric, allowing limited +// underscores, dashes and dots. // -// 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, identifiers are -// guaranteed to be safely used as filesystem path components. +// While the character set may be expanded in the future, identifiers +// are guaranteed to be safely used as filesystem path components. package identifiers import ( @@ -22,27 +16,28 @@ import ( ) const ( - maxLength = 76 - charclass = `[A-Za-z0-9]+` - label = charclass + `(:?[-]+` + charclass + `)*` + maxLength = 76 + alphanum = `[A-Za-z0-9]+` + separators = `[._-]` ) var ( - // identifierRe validates that a identifier matches valid identifiers. - // - // Rules for domains, defined in RFC 1035, section 2.3.1, are used for - // identifiers. - identifierRe = regexp.MustCompile(reAnchor(label + reGroup("[.]"+reGroup(label)) + "*")) + // identifierRe defines the pattern for valid identifiers. + identifierRe = regexp.MustCompile(reAnchor(alphanum + reGroup(separators+reGroup(alphanum)) + "*")) ) // 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) } diff --git a/identifiers/validate_test.go b/identifiers/validate_test.go index 4f8f9267f..4455ff4dc 100644 --- a/identifiers/validate_test.go +++ b/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), } { diff --git a/linux/runtime.go b/linux/runtime.go index 79d8fc4ee..4a7147e6e 100644 --- a/linux/runtime.go +++ b/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 diff --git a/linux/shim/exec.go b/linux/shim/exec.go index 65135afa3..1d23c6f4b 100644 --- a/linux/shim/exec.go +++ b/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, diff --git a/linux/shim/init.go b/linux/shim/init.go index 87aa11715..a8cc0e9e6 100644 --- a/linux/shim/init.go +++ b/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) diff --git a/linux/shim/service.go b/linux/shim/service.go index 45f364616..b91bfff7e 100644 --- a/linux/shim/service.go +++ b/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 diff --git a/metadata/namespaces.go b/metadata/namespaces.go index 567f2f181..4053733c5 100644 --- a/metadata/namespaces.go +++ b/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 } diff --git a/namespaces/context.go b/namespaces/context.go index e38bda721..6f9faf8a3 100644 --- a/namespaces/context.go +++ b/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") } diff --git a/namespaces/validate.go b/namespaces/validate.go new file mode 100644 index 000000000..ff97a8cc8 --- /dev/null +++ b/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 + `$` +} diff --git a/namespaces/validate_test.go b/namespaces/validate_test.go new file mode 100644 index 000000000..ad48a25a8 --- /dev/null +++ b/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") + } + }) + } +}