From a35727c80bd2a26613aae21db00628045cb9be24 Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Thu, 20 Jul 2017 10:01:23 +0200 Subject: [PATCH] *: implement additional pull registries Signed-off-by: Antonio Murdaca --- cmd/crio/config.go | 5 ++ cmd/crio/main.go | 7 ++ docs/crio.8.md | 19 ++++++ docs/crio.conf.5.md | 20 +++++- libkpod/config.go | 2 + libkpod/container_server.go | 2 +- pkg/storage/image.go | 56 +++++++++++++++- pkg/storage/image_regexp.go | 125 ++++++++++++++++++++++++++++++++++++ server/container_create.go | 10 +++ server/image_pull.go | 95 ++++++++++++++++----------- server/image_remove.go | 25 +++++++- server/image_status.go | 12 ++++ test/helpers.bash | 2 +- 13 files changed, 337 insertions(+), 43 deletions(-) create mode 100644 pkg/storage/image_regexp.go diff --git a/cmd/crio/config.go b/cmd/crio/config.go index e886aba3..2bd22fc2 100644 --- a/cmd/crio/config.go +++ b/cmd/crio/config.go @@ -135,6 +135,11 @@ image_volumes = "{{ .ImageVolumes }}" insecure_registries = [ {{ range $opt := .InsecureRegistries }}{{ printf "\t%q,\n" $opt }}{{ end }}] +# registries is used to specify a comma separated list of registries to be used +# when pulling an unqualified image (e.g. fedora:rawhide). +registries = [ +{{ range $opt := .Registries }}{{ printf "\t%q,\n" $opt }}{{ end }}] + # The "crio.network" table contains settings pertaining to the # management of CNI plugins. [crio.network] diff --git a/cmd/crio/main.go b/cmd/crio/main.go index d91e9308..40fb8608 100644 --- a/cmd/crio/main.go +++ b/cmd/crio/main.go @@ -81,6 +81,9 @@ func mergeConfig(config *server.Config, ctx *cli.Context) error { if ctx.GlobalIsSet("insecure-registry") { config.InsecureRegistries = ctx.GlobalStringSlice("insecure-registry") } + if ctx.GlobalIsSet("registry") { + config.Registries = ctx.GlobalStringSlice("registry") + } if ctx.GlobalIsSet("default-transport") { config.DefaultTransport = ctx.GlobalString("default-transport") } @@ -227,6 +230,10 @@ func main() { Name: "insecure-registry", Usage: "whether to disable TLS verification for the given registry", }, + cli.StringSliceFlag{ + Name: "registry", + Usage: "registry to be prepended when pulling unqualified images, can be specified multiple times", + }, cli.StringFlag{ Name: "default-transport", Usage: "default transport", diff --git a/docs/crio.8.md b/docs/crio.8.md index 521dba2a..21a2e2dd 100644 --- a/docs/crio.8.md +++ b/docs/crio.8.md @@ -16,11 +16,13 @@ crio - OCI Kubernetes Container Runtime daemon [**--debug**] [**--default-transport**=[*value*]] [**--help**|**-h**] +[**--insecure-registry**=[*value*]] [**--listen**=[*value*]] [**--log**=[*value*]] [**--log-format value**] [**--pause-command**=[*value*]] [**--pause-image**=[*value*]] +[**--registry**=[*value*]] [**--root**=[*value*]] [**--runroot**=[*value*]] [**--runtime**=[*value*]] @@ -73,6 +75,20 @@ set the CPU profile file path **--help, -h** Print usage statement +**--insecure-registry=** + Enable insecure registry communication, i.e., enable un-encrypted + and/or untrusted communication. + + List of insecure registries can contain an element with CIDR notation + to specify a whole subnet. Insecure registries accept HTTP and/or + accept HTTPS with certificates from unknown CAs. + + Enabling --insecure-registry is useful when running a local registry. + However, because its use creates security vulnerabilities it should + ONLY be enabled for testing purposes. For increased security, users + should add their CA to their system's list of trusted CAs instead of + using --insecure-registry. + **--image-volumes**="" Image volume handling ('mkdir' or 'ignore') (default: "mkdir") @@ -97,6 +113,9 @@ set the CPU profile file path **--root**="" CRIO root dir (default: "/var/lib/containers/storage") +**--registry**="" + Registry host which will be prepended to unqualified images, can be specified multiple times + **--runroot**="" CRIO state dir (default: "/var/run/containers/storage") diff --git a/docs/crio.conf.5.md b/docs/crio.conf.5.md index 418b0d8e..9c1896b4 100644 --- a/docs/crio.conf.5.md +++ b/docs/crio.conf.5.md @@ -77,15 +77,33 @@ The `crio` table supports the following options: **default_transport** A prefix to prepend to image names that can't be pulled as-is (default: "docker://") -**--image_volumes**="" +**image_volumes**="" Image volume handling ('mkdir' or 'ignore') (default: "mkdir") +**insecure_registries**="" + Enable insecure registry communication, i.e., enable un-encrypted + and/or untrusted communication. + + List of insecure registries can contain an element with CIDR notation + to specify a whole subnet. Insecure registries accept HTTP and/or + accept HTTPS with certificates from unknown CAs. + + Enabling --insecure-registry is useful when running a local registry. + However, because its use creates security vulnerabilities it should + ONLY be enabled for testing purposes. For increased security, users + should add their CA to their system's list of trusted CAs instead of + using --insecure-registry. + **pause_command**="" Path to the pause executable in the pause image (default: "/pause") **pause_image**="" Image which contains the pause executable (default: "kubernetes/pause") +**registries**="" + Comma separated list of registries that will be prepended when pulling + unqualified images + ## CRIO.NETWORK TABLE **network_dir**="" diff --git a/libkpod/config.go b/libkpod/config.go index 6390d76a..a6b276d3 100644 --- a/libkpod/config.go +++ b/libkpod/config.go @@ -160,6 +160,8 @@ type ImageConfig struct { InsecureRegistries []string `toml:"insecure_registries"` // ImageVolumes controls how volumes specified in image config are handled ImageVolumes ImageVolumesType `toml:"image_volumes"` + // Registries holds a list of registries used to pull unqualified images + Registries []string `toml:"registries"` } // NetworkConfig represents the "crio.network" TOML config table diff --git a/libkpod/container_server.go b/libkpod/container_server.go index 749e8d99..c2c76807 100644 --- a/libkpod/container_server.go +++ b/libkpod/container_server.go @@ -103,7 +103,7 @@ func New(config *Config) (*ContainerServer, error) { return nil, err } - imageService, err := storage.GetImageService(store, config.DefaultTransport, config.InsecureRegistries) + imageService, err := storage.GetImageService(store, config.DefaultTransport, config.InsecureRegistries, config.Registries) if err != nil { return nil, err } diff --git a/pkg/storage/image.go b/pkg/storage/image.go index 8dec3660..5a52d81c 100644 --- a/pkg/storage/image.go +++ b/pkg/storage/image.go @@ -1,7 +1,10 @@ package storage import ( + "errors" "net" + "path/filepath" + "strings" "github.com/containers/image/copy" "github.com/containers/image/docker/reference" @@ -31,6 +34,7 @@ type imageService struct { defaultTransport string insecureRegistryCIDRs []*net.IPNet indexConfigs map[string]*indexInfo + registries []string } // ImageServer wraps up various CRI-related activities into a reusable @@ -50,6 +54,9 @@ type ImageServer interface { GetStore() storage.Store // CanPull preliminary checks whether we're allowed to pull an image CanPull(imageName string, options *copy.Options) (bool, error) + // ResolveNames takes an image reference and if it's unqualified (w/o hostname), + // it uses crio's default registries to qualify it. + ResolveNames(imageName string) ([]string, error) } func (svc *imageService) ListImages(filter string) ([]ImageResult, error) { @@ -271,11 +278,47 @@ func (svc *imageService) isSecureIndex(indexName string) bool { return true } +func isValidHostname(hostname string) bool { + return hostname != "" && !strings.Contains(hostname, "/") && + (strings.Contains(hostname, ".") || + strings.Contains(hostname, ":") || hostname == "localhost") +} + +func (svc *imageService) ResolveNames(imageName string) ([]string, error) { + r, err := reference.ParseNormalizedNamed(imageName) + if err != nil { + return nil, err + } + domain, rest := splitDomain(r.Name()) + if len(domain) != 0 && isValidHostname(domain) { + // this means the image is already fully qualified + return []string{imageName}, nil + } + // we got an unqualified image here, we can't go ahead w/o registries configured + // properly. + if len(svc.registries) == 0 { + return nil, errors.New("no registries configured while trying to pull an unqualified image") + } + // this means we got an image in the form of "busybox" + // we need to use additional registries... + // normalize the unqualified image to be domain/repo/image... + images := []string{} + for _, r := range svc.registries { + path := rest + if !isValidHostname(domain) { + // This is the case where we have an image like "runcom/busybox" + path = imageName + } + images = append(images, filepath.Join(r, path)) + } + return images, nil +} + // GetImageService returns an ImageServer that uses the passed-in store, and // which will prepend the passed-in defaultTransport value to an image name if // a name that's passed to its PullImage() method can't be resolved to an image // in the store and can't be resolved to a source on its own. -func GetImageService(store storage.Store, defaultTransport string, insecureRegistries []string) (ImageServer, error) { +func GetImageService(store storage.Store, defaultTransport string, insecureRegistries []string, registries []string) (ImageServer, error) { if store == nil { var err error store, err = storage.GetStore(storage.DefaultStoreOptions) @@ -284,11 +327,22 @@ func GetImageService(store storage.Store, defaultTransport string, insecureRegis } } + seenRegistries := make(map[string]bool, len(registries)) + cleanRegistries := []string{} + for _, r := range registries { + if seenRegistries[r] { + continue + } + cleanRegistries = append(cleanRegistries, r) + seenRegistries[r] = true + } + is := &imageService{ store: store, defaultTransport: defaultTransport, indexConfigs: make(map[string]*indexInfo, 0), insecureRegistryCIDRs: make([]*net.IPNet, 0), + registries: cleanRegistries, } insecureRegistries = append(insecureRegistries, "127.0.0.0/8") diff --git a/pkg/storage/image_regexp.go b/pkg/storage/image_regexp.go new file mode 100644 index 00000000..96de6488 --- /dev/null +++ b/pkg/storage/image_regexp.go @@ -0,0 +1,125 @@ +package storage + +// This is a fork of docker/distribution code to be used when manipulating image +// references. +// DO NOT EDIT THIS FILE. + +import "regexp" + +var ( + // alphaNumericRegexp defines the alpha numeric atom, typically a + // component of names. This only allows lower case characters and digits. + alphaNumericRegexp = match(`[a-z0-9]+`) + + // separatorRegexp defines the separators allowed to be embedded in name + // components. This allow one period, one or two underscore and multiple + // dashes. + separatorRegexp = match(`(?:[._]|__|[-]*)`) + + // nameComponentRegexp restricts registry path component names to start + // with at least one letter or number, with following parts able to be + // separated by one period, one or two underscore and multiple dashes. + nameComponentRegexp = expression( + alphaNumericRegexp, + optional(repeated(separatorRegexp, alphaNumericRegexp))) + + // domainComponentRegexp restricts the registry domain component of a + // repository name to start with a component as defined by domainRegexp + // and followed by an optional port. + domainComponentRegexp = match(`(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])`) + + // domainRegexp defines the structure of potential domain components + // that may be part of image names. This is purposely a subset of what is + // allowed by DNS to ensure backwards compatibility with Docker image + // names. + domainRegexp = expression( + domainComponentRegexp, + optional(repeated(literal(`.`), domainComponentRegexp)), + optional(literal(`:`), match(`[0-9]+`))) + + // NameRegexp is the format for the name component of references. The + // regexp has capturing groups for the domain and name part omitting + // the separating forward slash from either. + NameRegexp = expression( + optional(domainRegexp, literal(`/`)), + nameComponentRegexp, + optional(repeated(literal(`/`), nameComponentRegexp))) + + // anchoredNameRegexp is used to parse a name value, capturing the + // domain and trailing components. + anchoredNameRegexp = anchored( + optional(capture(domainRegexp), literal(`/`)), + capture(nameComponentRegexp, + optional(repeated(literal(`/`), nameComponentRegexp)))) + + // IdentifierRegexp is the format for string identifier used as a + // content addressable identifier using sha256. These identifiers + // are like digests without the algorithm, since sha256 is used. + IdentifierRegexp = match(`([a-f0-9]{64})`) + + // ShortIdentifierRegexp is the format used to represent a prefix + // of an identifier. A prefix may be used to match a sha256 identifier + // within a list of trusted identifiers. + ShortIdentifierRegexp = match(`([a-f0-9]{6,64})`) +) + +// match compiles the string to a regular expression. +var match = regexp.MustCompile + +// literal compiles s into a literal regular expression, escaping any regexp +// reserved characters. +func literal(s string) *regexp.Regexp { + re := match(regexp.QuoteMeta(s)) + + if _, complete := re.LiteralPrefix(); !complete { + panic("must be a literal") + } + + return re +} + +func splitDomain(name string) (string, string) { + match := anchoredNameRegexp.FindStringSubmatch(name) + if len(match) != 3 { + return "", name + } + return match[1], match[2] +} + +// expression defines a full expression, where each regular expression must +// follow the previous. +func expression(res ...*regexp.Regexp) *regexp.Regexp { + var s string + for _, re := range res { + s += re.String() + } + + return match(s) +} + +// optional wraps the expression in a non-capturing group and makes the +// production optional. +func optional(res ...*regexp.Regexp) *regexp.Regexp { + return match(group(expression(res...)).String() + `?`) +} + +// repeated wraps the regexp in a non-capturing group to get one or more +// matches. +func repeated(res ...*regexp.Regexp) *regexp.Regexp { + return match(group(expression(res...)).String() + `+`) +} + +// group wraps the regexp in a non-capturing group. +func group(res ...*regexp.Regexp) *regexp.Regexp { + return match(`(?:` + expression(res...).String() + `)`) +} + +// capture wraps the expression in a capturing group. +func capture(res ...*regexp.Regexp) *regexp.Regexp { + return match(`(` + expression(res...).String() + `)`) +} + +// anchored anchors the regular expression by adding start and end delimiters. +func anchored(res ...*regexp.Regexp) *regexp.Regexp { + return match(`^` + expression(res...).String() + `$`) +} diff --git a/server/container_create.go b/server/container_create.go index 2e6f500a..bf88789b 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -553,6 +553,16 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string, if image == "" { return nil, fmt.Errorf("CreateContainerRequest.ContainerConfig.Image.Image is empty") } + images, err := s.StorageImageServer().ResolveNames(image) + if err != nil { + // This means we got an image ID + if strings.Contains(err.Error(), "cannot specify 64-byte hexadecimal strings") { + images = append(images, image) + } else { + return nil, err + } + } + image = images[0] // bind mount the pod shm specgen.AddBindMount(sb.ShmPath(), "/dev/shm", []string{"rw"}) diff --git a/server/image_pull.go b/server/image_pull.go index 298b5b24..32fd01b2 100644 --- a/server/image_pull.go +++ b/server/image_pull.go @@ -14,7 +14,6 @@ import ( // PullImage pulls a image with authentication config. func (s *Server) PullImage(ctx context.Context, req *pb.PullImageRequest) (*pb.PullImageResponse, error) { logrus.Debugf("PullImageRequest: %+v", req) - // TODO(runcom?): deal with AuthConfig in req.GetAuth() // TODO: what else do we need here? (Signatures when the story isn't just pulling from docker://) image := "" img := req.GetImage() @@ -23,51 +22,71 @@ func (s *Server) PullImage(ctx context.Context, req *pb.PullImageRequest) (*pb.P } var ( - username string - password string + images []string + pulled string + err error ) - if req.GetAuth() != nil { - username = req.GetAuth().Username - password = req.GetAuth().Password - if req.GetAuth().Auth != "" { - var err error - username, password, err = decodeDockerAuth(req.GetAuth().Auth) - if err != nil { - return nil, err - } - } - } - options := ©.Options{ - SourceCtx: &types.SystemContext{}, - } - // a not empty username should be sufficient to decide whether to send auth - // or not I guess - if username != "" { - options.SourceCtx = &types.SystemContext{ - DockerAuthConfig: &types.DockerAuthConfig{ - Username: username, - Password: password, - }, - } - } - - canPull, err := s.StorageImageServer().CanPull(image, options) - if err != nil && !canPull { + images, err = s.StorageImageServer().ResolveNames(image) + if err != nil { return nil, err } + for _, img := range images { + var ( + username string + password string + ) + if req.GetAuth() != nil { + username = req.GetAuth().Username + password = req.GetAuth().Password + if req.GetAuth().Auth != "" { + username, password, err = decodeDockerAuth(req.GetAuth().Auth) + if err != nil { + logrus.Debugf("error decoding authentication for image %s: %v", img, err) + continue + } + } + } + options := ©.Options{ + SourceCtx: &types.SystemContext{}, + } + // Specifiying a username indicates the user intends to send authentication to the registry. + if username != "" { + options.SourceCtx = &types.SystemContext{ + DockerAuthConfig: &types.DockerAuthConfig{ + Username: username, + Password: password, + }, + } + } - // let's be smart, docker doesn't repull if image already exists. - if _, err := s.StorageImageServer().ImageStatus(s.ImageContext(), image); err == nil { - return &pb.PullImageResponse{ - ImageRef: image, - }, nil + var canPull bool + canPull, err = s.StorageImageServer().CanPull(img, options) + if err != nil && !canPull { + logrus.Debugf("error checking image %s: %v", img, err) + continue + } + + // let's be smart, docker doesn't repull if image already exists. + _, err = s.StorageImageServer().ImageStatus(s.ImageContext(), img) + if err == nil { + logrus.Debugf("image %s already in store, skipping pull", img) + pulled = img + break + } + + _, err = s.StorageImageServer().PullImage(s.ImageContext(), img, options) + if err != nil { + logrus.Debugf("error pulling image %s: %v", img, err) + continue + } + pulled = img + break } - - if _, err := s.StorageImageServer().PullImage(s.ImageContext(), image, options); err != nil { + if pulled == "" && err != nil { return nil, err } resp := &pb.PullImageResponse{ - ImageRef: image, + ImageRef: pulled, } logrus.Debugf("PullImageResponse: %+v", resp) return resp, nil diff --git a/server/image_remove.go b/server/image_remove.go index cafb5a73..29d296e4 100644 --- a/server/image_remove.go +++ b/server/image_remove.go @@ -2,6 +2,7 @@ package server import ( "fmt" + "strings" "github.com/Sirupsen/logrus" "golang.org/x/net/context" @@ -19,8 +20,30 @@ func (s *Server) RemoveImage(ctx context.Context, req *pb.RemoveImageRequest) (* if image == "" { return nil, fmt.Errorf("no image specified") } - err := s.StorageImageServer().RemoveImage(s.ImageContext(), image) + var ( + images []string + err error + deleted bool + ) + images, err = s.StorageImageServer().ResolveNames(image) if err != nil { + // This means we got an image ID + if strings.Contains(err.Error(), "cannot specify 64-byte hexadecimal strings") { + images = append(images, image) + } else { + return nil, err + } + } + for _, img := range images { + err = s.StorageImageServer().RemoveImage(s.ImageContext(), img) + if err != nil { + logrus.Debugf("error deleting image %s: %v", img, err) + continue + } + deleted = true + break + } + if !deleted && err != nil { return nil, err } resp := &pb.RemoveImageResponse{} diff --git a/server/image_status.go b/server/image_status.go index eb0a1226..44068151 100644 --- a/server/image_status.go +++ b/server/image_status.go @@ -2,6 +2,7 @@ package server import ( "fmt" + "strings" "github.com/Sirupsen/logrus" "github.com/containers/storage" @@ -20,6 +21,17 @@ func (s *Server) ImageStatus(ctx context.Context, req *pb.ImageStatusRequest) (* if image == "" { return nil, fmt.Errorf("no image specified") } + images, err := s.StorageImageServer().ResolveNames(image) + if err != nil { + // This means we got an image ID + if strings.Contains(err.Error(), "cannot specify 64-byte hexadecimal strings") { + images = append(images, image) + } else { + return nil, err + } + } + // match just the first registry as that's what kube meant + image = images[0] status, err := s.StorageImageServer().ImageStatus(s.ImageContext(), image) if err != nil { if err == storage.ErrImageUnknown { diff --git a/test/helpers.bash b/test/helpers.bash index 80c6f844..108c8d12 100644 --- a/test/helpers.bash +++ b/test/helpers.bash @@ -215,7 +215,7 @@ function start_crio() { "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=mrunalp/image-volume-test --import-from=dir:"$ARTIFACTS_PATH"/image-volume-test-image --add-name=docker.io/library/mrunalp/image-volume-test --signature-policy="$INTEGRATION_ROOT"/policy.json "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=busybox:latest --import-from=dir:"$ARTIFACTS_PATH"/busybox-image --add-name=docker.io/library/busybox:latest --signature-policy="$INTEGRATION_ROOT"/policy.json "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTS --runroot "$TESTDIR/crio-run" --image-name=runcom/stderr-test:latest --import-from=dir:"$ARTIFACTS_PATH"/stderr-test --add-name=docker.io/runcom/stderr-test:latest --signature-policy="$INTEGRATION_ROOT"/policy.json - "$CRIO_BINARY" --conmon "$CONMON_BINARY" --listen "$CRIO_SOCKET" --cgroup-manager "$CGROUP_MANAGER" --runtime "$RUNTIME_BINARY" --root "$TESTDIR/crio" --runroot "$TESTDIR/crio-run" $STORAGE_OPTS --seccomp-profile "$seccomp" --apparmor-profile "$apparmor" --cni-config-dir "$CRIO_CNI_CONFIG" --signature-policy "$INTEGRATION_ROOT"/policy.json --image-volumes "$IMAGE_VOLUMES" --pids-limit "$PIDS_LIMIT" --config /dev/null config >$CRIO_CONFIG + "$CRIO_BINARY" --conmon "$CONMON_BINARY" --listen "$CRIO_SOCKET" --cgroup-manager "$CGROUP_MANAGER" --registry "docker.io" --runtime "$RUNTIME_BINARY" --root "$TESTDIR/crio" --runroot "$TESTDIR/crio-run" $STORAGE_OPTS --seccomp-profile "$seccomp" --apparmor-profile "$apparmor" --cni-config-dir "$CRIO_CNI_CONFIG" --signature-policy "$INTEGRATION_ROOT"/policy.json --image-volumes "$IMAGE_VOLUMES" --pids-limit "$PIDS_LIMIT" --config /dev/null config >$CRIO_CONFIG # Prepare the CNI configuration files, we're running with non host networking by default if [[ -n "$4" ]]; then