Better credentials handling (#526)

Signed-off-by: Matej Vasek <mvasek@redhat.com>

cmd/deploy.go

@@ -1,15 +1,11 @@
 package cmd
 
 import (
-	"context"
 	"fmt"
 
 	"github.com/AlecAivazis/survey/v2"
 	"github.com/AlecAivazis/survey/v2/terminal"
-	"github.com/containers/image/v5/pkg/docker/config"
-	containersTypes "github.com/containers/image/v5/types"
 	"github.com/ory/viper"
-	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"knative.dev/client/pkg/util"
 
@@ -29,7 +25,7 @@ func newDeployClient(cfg deployConfig) (*fn.Client, error) {
 
 	builder := buildpacks.NewBuilder()
 
-	pusher, err := docker.NewPusher(docker.WithCredentialsProvider(credentialsProvider),
+	pusher, err := docker.NewPusher(docker.WithCredentialsProvider(docker.NewCredentialsProvider(newCredentialsCallback(), nil)),
 		docker.WithProgressListener(listener))
 	if err != nil {
 		return nil, err
@@ -186,45 +182,38 @@ func runDeploy(cmd *cobra.Command, _ []string, clientFn deployClientFn) (err err
 	// (for example kubectl usually uses ~/.kube/config)
 }
 
-func credentialsProvider(ctx context.Context, registry string) (docker.Credentials, error) {
+func newCredentialsCallback() func(registry string) (docker.Credentials, error) {
-
+	firstTime := true
-	result := docker.Credentials{}
+	return func(registry string) (docker.Credentials, error) {
-	credentials, err := config.GetCredentials(nil, registry)
+		var result docker.Credentials
-	if err != nil {
+		var qs = []*survey.Question{
-		return result, errors.Wrap(err, "failed to get credentials")
+			{
-	}
+				Name: "username",
-
+				Prompt: &survey.Input{
-	if credentials != (containersTypes.DockerAuthConfig{}) {
+					Message: "Username:",
-		result.Username, result.Password = credentials.Username, credentials.Password
+				},
+				Validate: survey.Required,
+			},
+			{
+				Name: "password",
+				Prompt: &survey.Password{
+					Message: "Password:",
+				},
+				Validate: survey.Required,
+			},
+		}
+		if firstTime {
+			firstTime = false
+			fmt.Printf("Please provide credentials for image registry (%s).\n", registry)
+		} else {
+			fmt.Println("Incorrect credentials, please try again.")
+		}
+		err := survey.Ask(qs, &result)
+		if err != nil {
+			return docker.Credentials{}, err
+		}
 		return result, nil
 	}
-
-	credentials, _ = docker.GetCredentialsFromCredsStore(registry)
-	if credentials != (containersTypes.DockerAuthConfig{}) {
-		result.Username, result.Password = credentials.Username, credentials.Password
-		return result, nil
-	}
-
-	fmt.Println("Please provide credentials for image registry.")
-	var qs = []*survey.Question{
-		{
-			Name: "username",
-			Prompt: &survey.Input{
-				Message: "Username:",
-			},
-			Validate: survey.Required,
-		},
-		{
-			Name: "password",
-			Prompt: &survey.Password{
-				Message: "Password:",
-			},
-			Validate: survey.Required,
-		},
-	}
-	err = survey.Ask(qs, &result)
-
-	return result, err
 }
 
 type deployConfig struct {

docker/credentials_helper.go

@@ -2,6 +2,7 @@ package docker
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/url"
 	"os"
@@ -12,6 +13,8 @@ import (
 	"github.com/docker/docker-credential-helpers/client"
 )
 
+var ErrCredentialsNotFound = errors.New("credentials not found")
+
 func GetCredentialsFromCredsStore(registry string) (types.DockerAuthConfig, error) {
 	result := types.DockerAuthConfig{}
 
@@ -20,10 +23,13 @@ func GetCredentialsFromCredsStore(registry string) (types.DockerAuthConfig, erro
 		return result, fmt.Errorf("failed to determine home directory: %w", err)
 	}
 
-	confFilePath := filepath.Join(dirname, ".docker/config.json")
+	confFilePath := filepath.Join(dirname, ".docker", "config.json")
 
 	f, err := os.Open(confFilePath)
 	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return types.DockerAuthConfig{}, ErrCredentialsNotFound
+		}
 		return result, fmt.Errorf("failed to open docker config file: %w", err)
 	}
 	defer f.Close()
@@ -63,7 +69,7 @@ func GetCredentialsFromCredsStore(registry string) (types.DockerAuthConfig, erro
 		}
 	}
 
-	return result, fmt.Errorf("credentials cannot be found: %w", os.ErrNotExist)
+	return result, fmt.Errorf("failed to get credentials from helper specified in ~/.docker/config.json: %w", ErrCredentialsNotFound)
 }
 
 func to2ndLevelDomain(rawurl string) string {

docker/pusher.go

@@ -5,15 +5,20 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
 	"regexp"
 	"strings"
 
+	"github.com/docker/docker/errdefs"
+
+	"github.com/containers/image/v5/pkg/docker/config"
+	containersTypes "github.com/containers/image/v5/types"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/client"
-	"github.com/pkg/errors"
+	dockerClient "github.com/docker/docker/client"
 
 	fn "knative.dev/kn-plugin-func"
 )
@@ -27,6 +32,103 @@ type Credentials struct {
 
 type CredentialsProvider func(ctx context.Context, registry string) (Credentials, error)
 
+type CredentialsCallback func(registry string) (Credentials, error)
+
+var ErrUnauthorized = errors.New("bad credentials")
+
+// VerifyCredentialsCallback checks if credentials are accepted by the registry.
+// If credentials are incorrect this callback shall return ErrUnauthorized.
+type VerifyCredentialsCallback func(ctx context.Context, username, password, registry string) error
+
+func CheckAuth(ctx context.Context, username, password, registry string) error {
+	cli, err := dockerClient.NewClientWithOpts(dockerClient.FromEnv, dockerClient.WithAPIVersionNegotiation())
+	if err != nil {
+		return err
+	}
+
+	_, err = cli.RegistryLogin(ctx, types.AuthConfig{Username: username, Password: password, ServerAddress: registry})
+	if err != nil && strings.Contains(err.Error(), "401 Unauthorized") {
+		return ErrUnauthorized
+	}
+
+	// podman hack until https://github.com/containers/podman/pull/11595 is merged
+	// podman returns 400 (instead of 500) and body in unexpected shape
+	if errdefs.IsInvalidParameter(err) {
+		return ErrUnauthorized
+	}
+
+	return err
+}
+
+// NewCredentialsProvider returns new CredentialsProvider that tires to read credentials from `~/.docker/config.json`.
+// If reading credentials from the config fails caller provided callback will be invoked to obtain credentials.
+// The callback will typically prompt user to enter password to stdin.
+// To verify that password is correct verifyCredentials param may be used.
+// If verifyCredentials == nil then CheckAuth will be used.
+func NewCredentialsProvider(credentialsCallback CredentialsCallback, verifyCredentials VerifyCredentialsCallback) CredentialsProvider {
+	if verifyCredentials == nil {
+		verifyCredentials = CheckAuth
+	}
+	return func(ctx context.Context, registry string) (Credentials, error) {
+
+		result := Credentials{}
+		credentials, err := config.GetCredentials(nil, registry)
+		if err != nil {
+			return result, fmt.Errorf("failed to get credentials: %w", err)
+		}
+
+		if credentials != (containersTypes.DockerAuthConfig{}) {
+			result.Username, result.Password = credentials.Username, credentials.Password
+
+			err = verifyCredentials(ctx, result.Username, result.Password, registry)
+			if err == nil {
+				return result, nil
+			} else {
+				if !errors.Is(err, ErrUnauthorized) {
+					return Credentials{}, err
+				}
+			}
+		}
+
+		credentials, err = GetCredentialsFromCredsStore(registry)
+		if err != nil && !errors.Is(err, ErrCredentialsNotFound) {
+			return Credentials{}, err
+		}
+
+		if credentials != (containersTypes.DockerAuthConfig{}) {
+			result.Username, result.Password = credentials.Username, credentials.Password
+
+			err = verifyCredentials(ctx, result.Username, result.Password, registry)
+			if err == nil {
+				return result, nil
+			} else {
+				if !errors.Is(err, ErrUnauthorized) {
+					return Credentials{}, err
+				}
+			}
+		}
+
+		for {
+			result, err = credentialsCallback(registry)
+			if err != nil {
+				return Credentials{}, err
+			}
+
+			err = verifyCredentials(ctx, result.Username, result.Password, registry)
+			if err == nil {
+				// TODO maybe save the credentials
+				// but where? ~/.docker/conf.json or our own config file?
+				return result, nil
+			} else {
+				if errors.Is(err, ErrUnauthorized) {
+					continue
+				}
+				return Credentials{}, err
+			}
+		}
+	}
+}
+
 // Pusher of images from local to remote registry.
 type Pusher struct {
 	// Verbose logging.
@@ -78,7 +180,7 @@ func getRegistry(image_url string) (string, error) {
 	case len(parts) >= 3:
 		registry = parts[0]
 	default:
-		return "", errors.Errorf("failed to parse image name: %q", image_url)
+		return "", fmt.Errorf("failed to parse image name: %q", image_url)
 	}
 
 	return registry, nil
@@ -98,13 +200,13 @@ func (n *Pusher) Push(ctx context.Context, f fn.Function) (digest string, err er
 
 	cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
 	if err != nil {
-		return "", errors.Wrap(err, "failed to create docker api client")
+		return "", fmt.Errorf("failed to create docker api client: %w", err)
 	}
 
 	n.progressListener.Stopping()
 	credentials, err := n.credentialsProvider(ctx, registry)
 	if err != nil {
-		return "", errors.Wrap(err, "failed to get credentials")
+		return "", fmt.Errorf("failed to get credentials: %w", err)
 	}
 	n.progressListener.Increment("Pushing function image to the registry")
 
@@ -117,7 +219,7 @@ func (n *Pusher) Push(ctx context.Context, f fn.Function) (digest string, err er
 
 	r, err := cli.ImagePush(ctx, f.Image, opts)
 	if err != nil {
-		return "", errors.Wrap(err, "failed to push the image")
+		return "", fmt.Errorf("failed to push the image: %w", err)
 	}
 	defer r.Close()
 

docker/pusher_test.go

@@ -1,6 +1,10 @@
 package docker
 
 import (
+	"context"
+	"fmt"
+	"os"
+	"runtime"
 	"testing"
 )
 
@@ -55,3 +59,61 @@ func Test_getRegistry(t *testing.T) {
 		})
 	}
 }
+
+func Test_NewCredentialsProvider(t *testing.T) {
+	// TODO add tests where also reading from config is utilized.
+	defer withCleanHome(t)()
+
+	ctx := context.Background()
+
+	firstInvocation := true
+	pwdCbk := func(registry string) (Credentials, error) {
+		if registry != "docker.io" {
+			return Credentials{}, fmt.Errorf("unexpected registry: %s", registry)
+		}
+		if firstInvocation {
+			firstInvocation = false
+			return Credentials{"testUser", "badPwd"}, nil
+		}
+		return Credentials{"testUser", "goodPwd"}, nil
+	}
+
+	verifyCbk := func(ctx context.Context, username, password, registry string) error {
+		if username == "testUser" && password == "goodPwd" && registry == "docker.io" {
+			return nil
+		}
+		return ErrUnauthorized
+	}
+
+	credentialProvider := NewCredentialsProvider(pwdCbk, verifyCbk)
+
+	creds, err := credentialProvider(ctx, "docker.io")
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+		return
+	}
+
+	expectedCredentials := Credentials{Username: "testUser", Password: "goodPwd"}
+	if creds != expectedCredentials {
+		t.Errorf("credentialProvider() = %v, want %v", creds, expectedCredentials)
+	}
+}
+
+func withCleanHome(t *testing.T) func() {
+	t.Helper()
+	homeName := "HOME"
+	if runtime.GOOS == "windows" {
+		homeName = "USERPROFILE"
+	}
+	tmpDir := t.TempDir()
+	oldHome, hadHome := os.LookupEnv(homeName)
+	os.Setenv(homeName, tmpDir)
+
+	return func() {
+		if hadHome {
+			os.Setenv(homeName, oldHome)
+		} else {
+			os.Unsetenv(homeName)
+		}
+	}
+}