feat: validate credentials (#146)

Signed-off-by: Miguel Martinez Trivino <[email protected]>

Author: Miguel Martínez Triviño

cmd/chart.go

@@ -113,12 +113,12 @@ func moveChart(cmd *cobra.Command, args []string) error {
 			Chart:          mover.ChartSpec{},
 			ImageHintsFile: imagePatternsFile,
 			// Use local keychain for authentication
-			Containers: mover.Containers{UseDefaultLocalKeychain: true},
+			ContainersAuth: &mover.ContainersAuth{UseDefaultLocalKeychain: true},
 		},
 		Target: mover.Target{
-			Chart:      mover.ChartSpec{},
-			Rules:      *targetRewriteRules,
-			Containers: mover.Containers{UseDefaultLocalKeychain: true},
+			Chart:          mover.ChartSpec{},
+			Rules:          *targetRewriteRules,
+			ContainersAuth: &mover.ContainersAuth{UseDefaultLocalKeychain: true},
 		},
 	}
 

pkg/mover/auth.go

@@ -3,15 +3,21 @@
 
 package mover
 
-import "github.com/google/go-containerregistry/pkg/authn"
+import (
+	"errors"
+	"fmt"
+	"net/url"
+
+	"github.com/google/go-containerregistry/pkg/authn"
+)
 
 // Resolve implements an authn.KeyChain
 //
 // See https://pkg.go.dev/github.com/google/go-containerregistry/pkg/authn#Keychain
 //
 // Returns a custom credentials authn.Authenticator if the given resource
 // RegistryStr() matches the Repository, otherwise it returns annonymous access
-func (repo ContainerRepository) Resolve(resource authn.Resource) (authn.Authenticator, error) {
+func (repo *OCICredentials) Resolve(resource authn.Resource) (authn.Authenticator, error) {
 	if repo.Server == resource.RegistryStr() {
 		return repo, nil
 	}
@@ -25,16 +31,41 @@ func (repo ContainerRepository) Resolve(resource authn.Resource) (authn.Authenti
 // See https://pkg.go.dev/github.com/google/go-containerregistry/pkg/authn#Authenticator
 //
 // Returns an authn.AuthConfig with a user / password pair to be used for authentication
-func (repo ContainerRepository) Authorization() (*authn.AuthConfig, error) {
+func (repo *OCICredentials) Authorization() (*authn.AuthConfig, error) {
 	return &authn.AuthConfig{Username: repo.Username, Password: repo.Password}, nil
 }
 
-// Define a container images keychain based on the settings provided via the containers struct
-// If useDefaultKeychain is set, use config/docker.json otherwise load the provided creds (if any)
-func getContainersKeychain(c Containers) authn.Keychain {
+// Define a container registry keychain based on the settings provided in containers Auth
+// If useDefaultKeychain is set, use config/docker.json otherwise it will load the provided credentials (if any)
+func getContainersKeychain(c *ContainersAuth) (authn.Keychain, error) {
+	// No credentials provided
+	if !c.UseDefaultLocalKeychain && c.Credentials == nil {
+		return nil, errors.New("either local keychain or explicit credentials are required")
+	}
+
+	if c.UseDefaultLocalKeychain && c.Credentials != nil {
+		return nil, errors.New("you can use either local keychain or explicit credentials not both")
+	}
+
 	if c.UseDefaultLocalKeychain {
-		return authn.DefaultKeychain
+		return authn.DefaultKeychain, nil
+	}
+
+	return validateOCICredentials(c.Credentials)
+}
+
+// validate if the provided OCI credentials are valid
+// They include a username, password and a valid (RFC 3986 URI authority) serverName
+func validateOCICredentials(c *OCICredentials) (authn.Keychain, error) {
+	if c.Username == "" || c.Password == "" || c.Server == "" {
+		return nil, errors.New("OCI credentials require an username, password and a server name")
+	}
+
+	// See https://github.com/google/go-containerregistry/blob/main/pkg/name/registry.go#L97
+	// Valid RFC 3986 URI authority
+	if uri, err := url.Parse("//" + c.Server); err != nil || uri.Host != c.Server {
+		return nil, fmt.Errorf("credentials server name %q must not contain a scheme (\"http://\") nor a path. Example of valid registry names are \"myregistry.io\" or \"myregistry.io:9999\"", c.Server)
 	}
 
-	return c.ContainerRepository
+	return c, nil
 }

pkg/mover/auth_test.go

@@ -0,0 +1,84 @@
+// Copyright 2022 VMware, Inc.
+// SPDX-License-Identifier: BSD-2-Clause
+
+package mover
+
+import (
+	"testing"
+
+	"github.com/google/go-containerregistry/pkg/authn"
+)
+
+func TestGetContainersKeychain(t *testing.T) {
+	explicitCreds := &OCICredentials{Username: "user", Password: "pass", Server: "server.io"}
+
+	tests := []struct {
+		name      string
+		auth      *ContainersAuth
+		want      authn.Keychain
+		wantError bool // if the function should return an error
+	}{
+		// Valid triplets
+		{name: "neither local nor provided credentials", auth: &ContainersAuth{}, wantError: true},
+		{name: "both local and provided credentials",
+			auth: &ContainersAuth{
+				UseDefaultLocalKeychain: true,
+				Credentials:             explicitCreds},
+			wantError: true},
+		{name: "localKeychain", auth: &ContainersAuth{UseDefaultLocalKeychain: true}, want: authn.DefaultKeychain},
+		{
+			name: "valid explicit creds",
+			auth: &ContainersAuth{
+				Credentials: explicitCreds,
+			},
+			want: explicitCreds,
+		},
+		{name: "invalid provided credentials",
+			auth: &ContainersAuth{
+				Credentials: &OCICredentials{Username: "user", Password: "pass", Server: "https://server.io"}},
+			wantError: true},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got, gotError := getContainersKeychain(test.auth)
+			if gotError == nil == test.wantError {
+				t.Errorf("expected error %t, got error %q. auth=%v", test.wantError, gotError, test.auth)
+			}
+
+			if got != test.want {
+				t.Errorf("got=%v, want=%v", got, test.want)
+			}
+		})
+	}
+}
+
+func TestValidateOCICredentials(t *testing.T) {
+	var tests = []struct {
+		username  string
+		password  string
+		server    string
+		wantError bool // if the function should return an error
+	}{
+		// Valid triplets
+		{"username", "password", "server.io", false},
+		{"username", "password", "server.io:9999", false},
+
+		// Missing one of the three items
+		{"", "password", "server.io", true},
+		{"username", "", "server.io", true},
+		{"username", "password", "", true},
+
+		// Invalid serverName
+		{"username", "password", "http://server.io", true},
+		{"username", "password", "//server.io", true},
+		{"username", "password", "server.io/baz", true},
+	}
+
+	for _, test := range tests {
+		_, gotError := validateOCICredentials(&OCICredentials{test.server, test.username, test.password})
+		if gotError == nil == test.wantError {
+			t.Errorf("expected error %t, got error %q. username=%q, pass=%q, server=%q", test.wantError, gotError, test.username, test.password, test.server)
+		}
+	}
+}

pkg/mover/chart.go

@@ -11,6 +11,8 @@ import (
 	"path/filepath"
 	"regexp"
 
+	"github.com/google/go-containerregistry/pkg/authn"
+
 	"github.com/avast/retry-go"
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
@@ -94,15 +96,15 @@ type LocalChart struct {
 // the hints file and container images
 type IntermediateBundle LocalChart
 
-// ContainerRepository defines a private repo name and credentials
-type ContainerRepository struct {
+// OCICredentials defines a private repo name and credentials
+type OCICredentials struct {
 	Server             string
 	Username, Password string
 }
 
-// Containers is the section for private repository definition
-type Containers struct {
-	ContainerRepository
+// ContainersAuth is the section for private repository credentials definition
+type ContainersAuth struct {
+	Credentials *OCICredentials
 	// Use local keychain in the system (config/docker.json)
 	// This is useful to offer a CLI experience similar to docker
 	UseDefaultLocalKeychain bool
@@ -118,14 +120,14 @@ type ChartSpec struct {
 type Source struct {
 	Chart          ChartSpec
 	ImageHintsFile string
-	Containers     Containers
+	ContainersAuth *ContainersAuth
 }
 
 // Target of the chart move
 type Target struct {
-	Chart      ChartSpec
-	Rules      RewriteRules
-	Containers Containers
+	Chart          ChartSpec
+	Rules          RewriteRules
+	ContainersAuth *ContainersAuth
 }
 
 // ChartMoveRequest defines a chart move
@@ -155,10 +157,12 @@ type ChartMover struct {
 // imagePatters and rules.
 func NewChartMover(req *ChartMoveRequest, opts ...Option) (*ChartMover, error) {
 	cm := &ChartMover{
-		logger:                  defaultLogger{},
-		retries:                 DefaultRetries,
-		sourceContainerRegistry: internal.NewContainerRegistryClient(getContainersKeychain(req.Source.Containers)),
-		targetContainerRegistry: internal.NewContainerRegistryClient(getContainersKeychain(req.Target.Containers)),
+		logger:  defaultLogger{},
+		retries: DefaultRetries,
+	}
+
+	if err := initializeContainersAuth(req, cm); err != nil {
+		return nil, err
 	}
 
 	if err := validateTarget(&req.Target); err != nil {
@@ -727,3 +731,31 @@ func notNilData(data []byte, err error) ([]byte, error) {
 	}
 	return data, err
 }
+
+// Initialize the ChartMover OCI credentials based on the provided request
+func initializeContainersAuth(req *ChartMoveRequest, cm *ChartMover) error {
+	var err error
+	if cm.sourceContainerRegistry, err = newContainerRegistryClient(req.Source.ContainersAuth); err != nil {
+		return err
+	}
+
+	if cm.targetContainerRegistry, err = newContainerRegistryClient(req.Target.ContainersAuth); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// Return a private registry keychain or one for anonymous access
+func newContainerRegistryClient(auth *ContainersAuth) (*internal.ContainerRegistryClient, error) {
+	var keychain authn.Keychain
+	var err error
+
+	if auth != nil {
+		if keychain, err = getContainersKeychain(auth); err != nil {
+			return nil, err
+		}
+	}
+
+	return internal.NewContainerRegistryClient(keychain), nil
+}

pkg/mover/registry_test.go

@@ -78,8 +78,8 @@ func NewMoveRequest(chartPath, hints, target, targetRegistry, targetPrefix strin
 	}
 
 	if useLocalKeychain {
-		req.Source.Containers = mover.Containers{UseDefaultLocalKeychain: true}
-		req.Target.Containers = mover.Containers{UseDefaultLocalKeychain: true}
+		req.Source.ContainersAuth = &mover.ContainersAuth{UseDefaultLocalKeychain: true}
+		req.Target.ContainersAuth = &mover.ContainersAuth{UseDefaultLocalKeychain: true}
 	}
 
 	return req
@@ -92,7 +92,7 @@ func NewSaveRequest(chartPath, hints, bundle string) *mover.ChartMoveRequest {
 			Chart: mover.ChartSpec{Local: &mover.LocalChart{Path: chartPath}},
 			// path to file containing rules such as // {{.image.registry}}:{{.image.tag}}
 			ImageHintsFile: hints,
-			Containers:     mover.Containers{UseDefaultLocalKeychain: true},
+			ContainersAuth: &mover.ContainersAuth{UseDefaultLocalKeychain: true},
 		},
 		Target: mover.Target{
 			Chart: mover.ChartSpec{IntermediateBundle: &mover.IntermediateBundle{Path: bundle}},
@@ -107,8 +107,8 @@ func NewLoadRequest(bundle, target, targetRegistry, targetPrefix string) *mover.
 			Chart: mover.ChartSpec{IntermediateBundle: &mover.IntermediateBundle{Path: bundle}},
 		},
 		Target: mover.Target{
-			Chart:      mover.ChartSpec{Local: &mover.LocalChart{Path: target}},
-			Containers: mover.Containers{UseDefaultLocalKeychain: true},
+			Chart:          mover.ChartSpec{Local: &mover.LocalChart{Path: target}},
+			ContainersAuth: &mover.ContainersAuth{UseDefaultLocalKeychain: true},
 			// Where to push and how to rewrite the found images
 			// i.e docker.io/bitnami/mariadb => myregistry.com/myteam/mariadb
 			Rules: mover.RewriteRules{
@@ -119,8 +119,8 @@ func NewLoadRequest(bundle, target, targetRegistry, targetPrefix string) *mover.
 	}
 }
 
-func repo(domain, user, passwd string) mover.ContainerRepository {
-	return mover.ContainerRepository{
+func repo(domain, user, passwd string) *mover.OCICredentials {
+	return &mover.OCICredentials{
 		Server:   domain,
 		Username: user,
 		Password: passwd,
@@ -166,7 +166,9 @@ func TestRegistryCustomCredentials(t *testing.T) {
 	prepareDockerCA(t, params.certFile)
 	dockerLogout(t, params.domain)
 	req := NewMoveRequest(TestChart, Hints, Target, params.domain, Prefix, false)
-	req.Target.Containers.ContainerRepository = repo(params.domain, params.user, params.passwd)
+	req.Target.ContainersAuth = &mover.ContainersAuth{
+		Credentials: repo(params.domain, params.user, params.passwd),
+	}
 	got := relok8s(t, req)
 	var want error
 	if got != want {
@@ -193,7 +195,9 @@ func TestRegistryBadCustomCredentials(t *testing.T) {
 	prepareDockerCA(t, params.certFile)
 	dockerLogout(t, params.domain)
 	req := NewMoveRequest(TestChart, Hints, Target, params.domain, Prefix, false)
-	req.Target.Containers.ContainerRepository = repo(params.domain, BadUser, BadPasswd)
+	req.Target.ContainersAuth = &mover.ContainersAuth{
+		Credentials: repo(params.domain, BadUser, BadPasswd),
+	}
 	got := relok8s(t, req)
 	// retry.Error is incompatible with errors package, it cannot be unwrapped
 	_, ok := got.(retry.Error)