driver: add support for credentials helper and static file auth config

This PR adds support for specifying an external credentials helper and/ or an external "auth.json" credentials file. The plugin configuration now has an "auth" block with fields "helper" and "config" (similar to the docker driver). We also now have an "auth_soft_fail" option in Task config for cases where someone has configured the auth block in plugin config, but has a task that is using a public image with no credentials. In that case setting auth_soft_fail is used to ignore the fact that no credentials will be found for the given public image. (This is also how the docker driver works, I didn't come up with this). Unlike the docker driver, the podman driver still does not support specifying a credentials helper _in_ the external "auth.json" credentials file. If there is demand for that use case we can add it, but in the short term it seems like just the plugin's support for specifying a credentials helper could be sufficient. And it's a lot of code to do the other thing.
hashicorp · Jul 5, 2023 · 0a4a995 · 0a4a995
1 parent eebb9f9
commit 0a4a995
Show file tree

Hide file tree

Showing 15 changed files with 721 additions and 55 deletions.
diff --git a/api/image_pull.go b/api/image_pull.go
@@ -20,19 +20,22 @@ func (c *API) ImagePull(ctx context.Context, pullConfig *registry.PullConfig) (s
 
 	var (
 		headers    = make(map[string]string)
-		repository = pullConfig.Repository
+		repository = pullConfig.Image
 		tlsVerify  = pullConfig.TLSVerify
 	)
 
-	auth, err := registry.ResolveRegistryAuthentication(repository, pullConfig)
-	if err != nil {
-		return "", fmt.Errorf("failed to determine authentication for %q", repository)
+	// if the task or driver are configured with an auth block, attempt to find
+	// credentials that are compatible with the given image, and set the appropriate
+	// header if found
+	if pullConfig.AuthAvailable() {
+		auth, err := registry.ResolveRegistryAuthentication(repository, pullConfig)
+		if err != nil {
+			return "", fmt.Errorf("failed to determine authentication for %q: %w", repository, err)
+		}
+		auth.SetHeader(headers)
 	}
-	auth.SetHeader(headers)
-	c.logger.Info("HEADERS", "headers", headers)
 
 	urlPath := fmt.Sprintf("/v1.0.0/libpod/images/pull?reference=%s&tlsVerify=%t", repository, tlsVerify)
-	c.logger.Info("URL PATH", "urlPath", urlPath)
 
 	res, err := c.PostWithHeaders(ctx, urlPath, nil, headers)
 	if err != nil {
@@ -50,17 +53,16 @@ func (c *API) ImagePull(ctx context.Context, pullConfig *registry.PullConfig) (s
 		report ImagePullReport
 		id     string
 	)
+
 	for {
 		decodeErr := dec.Decode(&report)
-		if errors.Is(decodeErr, io.EOF) {
-			break
-		} else if decodeErr != nil {
+		switch {
+		case errors.Is(decodeErr, io.EOF):
+			return id, nil
+		case decodeErr != nil:
 			return "", fmt.Errorf("failed to read image pull response: %w", decodeErr)
-		} else if report.Error != "" {
-			return "", fmt.Errorf("image pull report indicates error: %s", report.Error)
-		} else if report.ID != "" {
+		case report.ID != "":
 			id = report.ID
 		}
 	}
-	return id, nil
 }
diff --git a/api/image_pull_test.go b/api/image_pull_test.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"testing"
 
+	"github.com/hashicorp/nomad-driver-podman/registry"
 	"github.com/shoenig/test/must"
 )
 
@@ -25,7 +26,9 @@ func TestApi_Image_Pull(t *testing.T) {
 	}
 
 	for _, testCase := range testCases {
-		id, err := api.ImagePull(ctx, testCase.Image, ImageAuthConfig{})
+		id, err := api.ImagePull(ctx, &registry.PullConfig{
+			Image: testCase.Image,
+		})
 		if testCase.Exists {
 			must.NoError(t, err)
 			must.NotEq(t, "", id)

diff --git a/config.go b/config.go
@@ -15,6 +15,7 @@ var (
 		// image registry authentication options
 		"auth": hclspec.NewBlock("auth", false, hclspec.NewObject(map[string]*hclspec.Spec{
 			"config": hclspec.NewAttr("config", "string", false),
+			"helper": hclspec.NewAttr("helper", "string", false),
 		})),
 
 		// volume options
@@ -107,9 +108,9 @@ var (
 	})
 )
 
-// AuthConfig is the tasks authentication configuration
+// TaskAuthConfig is the tasks authentication configuration
 // (there is also auth_soft_fail on the top level)
-type AuthConfig struct {
+type TaskAuthConfig struct {
 	Username  string `codec:"username"`
 	Password  string `codec:"password"`
 	TLSVerify bool   `codec:"tls_verify"`
@@ -132,15 +133,21 @@ type VolumeConfig struct {
 	SelinuxLabel string `codec:"selinuxlabel"`
 }
 
+type PluginAuthConfig struct {
+	FileConfig string `codec:"config"`
+	Helper     string `codec:"helper"`
+}
+
 // PluginConfig is the driver configuration set by the SetConfig RPC call
 type PluginConfig struct {
-	Volumes              VolumeConfig `codec:"volumes"`
-	GC                   GCConfig     `codec:"gc"`
-	RecoverStopped       bool         `codec:"recover_stopped"`
-	DisableLogCollection bool         `codec:"disable_log_collection"`
-	SocketPath           string       `codec:"socket_path"`
-	ClientHttpTimeout    string       `codec:"client_http_timeout"`
-	ExtraLabels          []string     `codec:"extra_labels"`
+	Auth                 PluginAuthConfig `codec:"auth"`
+	Volumes              VolumeConfig     `codec:"volumes"`
+	GC                   GCConfig         `codec:"gc"`
+	RecoverStopped       bool             `codec:"recover_stopped"`
+	DisableLogCollection bool             `codec:"disable_log_collection"`
+	SocketPath           string           `codec:"socket_path"`
+	ClientHttpTimeout    string           `codec:"client_http_timeout"`
+	ExtraLabels          []string         `codec:"extra_labels"`
 }
 
 // LogWarnings will emit logs about known problematic configurations
@@ -154,7 +161,7 @@ func (c *PluginConfig) LogWarnings(logger hclog.Logger) {
 type TaskConfig struct {
 	ApparmorProfile   string             `codec:"apparmor_profile"`
 	Args              []string           `codec:"args"`
-	Auth              AuthConfig         `codec:"auth"`
+	Auth              TaskAuthConfig     `codec:"auth"`
 	AuthSoftFail      bool               `codec:"auth_soft_fail"`
 	Ports             []string           `codec:"ports"`
 	Tmpfs             []string           `codec:"tmpfs"`

diff --git a/driver.go b/driver.go
@@ -694,7 +694,14 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *drive
 			return nil, nil, fmt.Errorf("failed to parse image_pull_timeout: %w", parseErr)
 		}
 
-		imageID, createErr := d.createImage(createOpts.Image, &driverConfig.Auth, driverConfig.ForcePull, imagePullTimeout, cfg)
+		imageID, createErr := d.createImage(
+			createOpts.Image,
+			&driverConfig.Auth,
+			driverConfig.AuthSoftFail,
+			driverConfig.ForcePull,
+			imagePullTimeout,
+			cfg,
+		)
 		if createErr != nil {
 			return nil, nil, fmt.Errorf("failed to create image: %s: %w", createOpts.Image, createErr)
 		}
@@ -907,7 +914,14 @@ func sliceMergeUlimit(ulimitsRaw map[string]string) ([]spec.POSIXRlimit, error)
 
 // Creates the requested image if missing from storage
 // returns the 64-byte image ID as an unique image identifier
-func (d *Driver) createImage(image string, auth *AuthConfig, forcePull bool, imagePullTimeout time.Duration, cfg *drivers.TaskConfig) (string, error) {
+func (d *Driver) createImage(
+	image string,
+	auth *TaskAuthConfig,
+	authSoftFail bool,
+	forcePull bool,
+	imagePullTimeout time.Duration,
+	cfg *drivers.TaskConfig,
+) (string, error) {
 	var imageID string
 	imageName := image
 	// If it is a shortname, we should not have to worry
@@ -963,12 +977,15 @@ func (d *Driver) createImage(image string, auth *AuthConfig, forcePull bool, ima
 	})
 
 	pc := &registry.PullConfig{
-		Repository: imageName,
-		TLSVerify:  auth.TLSVerify,
+		Image:     imageName,
+		TLSVerify: auth.TLSVerify,
 		RegistryConfig: &registry.RegistryAuthConfig{
 			Username: auth.Username,
 			Password: auth.Password,
 		},
+		CredentialsFile:   d.config.Auth.FileConfig,
+		CredentialsHelper: d.config.Auth.Helper,
+		AuthSoftFail:      authSoftFail,
 	}
 
 	result, err, _ := d.pullGroup.Do(imageName, func() (interface{}, error) {

diff --git a/driver_test.go b/driver_test.go
@@ -1983,7 +1983,7 @@ func startDestroyInspectImage(t *testing.T, image string, taskName string) {
 		AllocID:   uuid.Generate(),
 		Resources: createBasicResources(),
 	}
-	imageID, err := getPodmanDriver(t, d).createImage(image, &AuthConfig{}, false, 5*time.Minute, task)
+	imageID, err := getPodmanDriver(t, d).createImage(image, &TaskAuthConfig{}, false, false, 5*time.Minute, task)
 	must.NoError(t, err)
 	must.Eq(t, imageID, inspectData.Image)
 }
@@ -2063,7 +2063,7 @@ insecure = true`
 	go func() {
 		// Pull image using our proxy.
 		image := "localhost:5000/quay/busybox:latest"
-		_, err = getPodmanDriver(t, d).createImage(image, &AuthConfig{}, true, 3*time.Second, task)
+		_, err = getPodmanDriver(t, d).createImage(image, &TaskAuthConfig{}, false, true, 3*time.Second, task)
 		resultCh <- err
 	}()
 
@@ -2141,7 +2141,7 @@ func createInspectImage(t *testing.T, image, reference string) {
 		AllocID:   uuid.Generate(),
 		Resources: createBasicResources(),
 	}
-	idTest, err := getPodmanDriver(t, d).createImage(image, &AuthConfig{}, false, 5*time.Minute, task)
+	idTest, err := getPodmanDriver(t, d).createImage(image, &TaskAuthConfig{}, false, false, 5*time.Minute, task)
 	must.NoError(t, err)
 
 	idRef, err := getPodmanDriver(t, d).podman.ImageInspectID(context.Background(), reference)

diff --git a/registry/authentication.go b/registry/authentication.go
@@ -1,21 +1,23 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
 package registry
 
 import (
 	"encoding/base64"
 	"encoding/json"
-	"fmt"
 
 	"github.com/hashicorp/go-hclog"
 )
 
 // PullConfig represents the necessary information needed for an image pull query.
 type PullConfig struct {
-	Repository     string
-	RegistryConfig *RegistryAuthConfig
-	TLSVerify      bool
-
-	// TODO: config file
-	// TODO: creds helper
+	Image             string
+	RegistryConfig    *RegistryAuthConfig
+	TLSVerify         bool
+	CredentialsFile   string
+	CredentialsHelper string
+	AuthSoftFail      bool
 }
 
 // BasicConfig returns the Basic-Auth level of configuration.
@@ -33,16 +35,37 @@ func (pc *PullConfig) BasicConfig() *TaskAuthConfig {
 	}
 }
 
+// AuthAvailable returns true if any of the supported authentication methods are
+// set, indicating we should try to find credentials for the given image by
+// checking each of the options for looking up credentials.
+// - RegistryConfig (from task configuration)
+// - CredentialsFile (from plugin configuration)
+// - CredentialsHelper (from plugin configuration)
+func (pc *PullConfig) AuthAvailable() bool {
+	switch {
+	case !pc.RegistryConfig.Empty():
+		return true
+	case pc.CredentialsFile != "":
+		return true
+	case pc.CredentialsHelper != "":
+		return true
+	default:
+		return false
+	}
+}
+
 // Log the components of PullConfig in a safe way.
 func (pc *PullConfig) Log(logger hclog.Logger) {
 	var (
-		repository = pc.Repository
+		repository = pc.Image
 		tls        bool
-		username   = "<unset>"
-		email      = "<unset>"
-		server     = "<unset>"
+		username   string
+		email      string
+		server     string
 		password   bool
 		token      bool
+		helper     = pc.CredentialsHelper
+		file       = pc.CredentialsFile
 	)
 
 	if r := pc.RegistryConfig; r != nil {
@@ -63,6 +86,8 @@ func (pc *PullConfig) Log(logger hclog.Logger) {
 		"server", server,
 		"tls", tls,
 		"token", token,
+		"helper", helper,
+		"file", file,
 	)
 }
 
@@ -114,8 +139,10 @@ func (c *TaskAuthConfig) IsEmpty() bool {
 // - auth from auth.json file specified by plugin config
 // - auth from a credentials helper specified by plugin config
 func ResolveRegistryAuthentication(repository string, pullConfig *PullConfig) (*RegistryAuthConfig, error) {
-	return firstValidAuth(repository, []AuthBackend{
+	return firstValidAuth(repository, pullConfig.AuthSoftFail, []AuthBackend{
 		authFromTaskConfig(pullConfig.BasicConfig()),
+		authFromFileConfig(pullConfig.CredentialsFile),
+		authFromCredsHelper(pullConfig.CredentialsHelper),
 	})
 }
 
@@ -124,11 +151,20 @@ func ResolveRegistryAuthentication(repository string, pullConfig *PullConfig) (*
 // returned and should be skipped.
 type AuthBackend func(string) (*RegistryAuthConfig, error)
 
-func firstValidAuth(repository string, authBackends []AuthBackend) (*RegistryAuthConfig, error) {
+// firstValidAuth returns the first RegistryAuthConfig associated with repository.
+//
+// If softFail is set, ignore error return values from an authBackend and pretend
+// like they simply do not recognize repository, and continue searching.
+func firstValidAuth(repository string, softFail bool, authBackends []AuthBackend) (*RegistryAuthConfig, error) {
 	for _, backend := range authBackends {
 		auth, err := backend(repository)
-		if auth != nil || err != nil {
-			return auth, err
+		switch {
+		case auth != nil:
+			return auth, nil
+		case err != nil && softFail:
+			continue
+		case err != nil:
+			return nil, err
 		}
 	}
 	return nil, nil
@@ -147,11 +183,3 @@ func authFromTaskConfig(taskAuthConfig *TaskAuthConfig) AuthBackend {
 		}, nil
 	}
 }
-
-func authFromFileConfig(filename string) (*RegistryAuthConfig, error) {
-	return nil, fmt.Errorf("not implemented yet")
-}
-
-func authFromCredsHelper(helper string) (*RegistryAuthConfig, error) {
-	return nil, fmt.Errorf("not implemented yet")
-}
diff --git a/registry/authentication_test.go b/registry/authentication_test.go
@@ -0,0 +1,55 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package registry
+
+import (
+	"testing"
+
+	"github.com/shoenig/test/must"
+)
+
+func TestPullConfig_NoAuth(t *testing.T) {
+	cases := []struct {
+		name string
+		pc   *PullConfig
+		exp  bool
+	}{
+		{
+			name: "task config",
+			pc: &PullConfig{
+				RegistryConfig: &RegistryAuthConfig{
+					Username: "user",
+					Password: "pass",
+				},
+			},
+			exp: true,
+		},
+		{
+			name: "creds helper",
+			pc: &PullConfig{
+				CredentialsHelper: "helper.sh",
+			},
+			exp: true,
+		},
+		{
+			name: "creds file",
+			pc: &PullConfig{
+				CredentialsFile: "auth.json",
+			},
+			exp: true,
+		},
+		{
+			name: "none",
+			pc:   &PullConfig{},
+			exp:  false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := tc.pc.AuthAvailable()
+			must.Eq(t, tc.exp, result)
+		})
+	}
+}