Repository: knative-sandbox/kn-plugin-func
Pull request #526 (2021-09-15 12:33:35)

Better credentials handling
# Changes

* If config file does contain credentials but they are incorrect then user will be prompted to enter correct ones.
* If user enters incorrect credentials then the user will be prompted again.

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/526#" title="Author self-approved">matejvasek</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fkn-plugin-func).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/kn-plugin-func/blob/main/OWNERS)~~ [matejvasek]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
matejvasek: relates to #512

Patch:
diff --git a/.github/workflows/knative-style.yaml b/.github/workflows/knative-style.yaml
index 0e417bd3..5f5caaaa 100644
--- a/.github/workflows/knative-style.yaml
+++ b/.github/workflows/knative-style.yaml
@@ -128,12 +128,6 @@ jobs:
         uses: golangci/golangci-lint-action@v2
         with:
           version: v1.42
-          # tar errors extracting to pkg dir
-          # example: https://github.com/knative-sandbox/kn-plugin-func/pull/490/checks?check_run_id=3551662472
-          # issue:   https://github.com/golangci/golangci-lint-action/issues/244
-          # hotfix is to skip the pkg cache, which also requires an extended timeout:
-          skip-pkg-cache: true
-          args: --timeout=5m
 
       - name: misspell
         shell: bash
diff --git a/.golangci.yaml b/.golangci.yaml
index f3616c0f..7b33e61e 100644
--- a/.golangci.yaml
+++ b/.golangci.yaml
@@ -6,6 +6,9 @@
 #
 # 
 #
+run:
+  timeout: 5m
+
 linters:
   enable:
     - unconvert
diff --git a/cmd/deploy.go b/cmd/deploy.go
index 70d59d4d..0a6bdc17 100644
--- a/cmd/deploy.go
+++ b/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) {
-
-	result := docker.Credentials{}
-	credentials, err := config.GetCredentials(nil, registry)
-	if err != nil {
-		return result, errors.Wrap(err, "failed to get credentials")
-	}
-
-	if credentials != (containersTypes.DockerAuthConfig{}) {
-		result.Username, result.Password = credentials.Username, credentials.Password
-		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:",
+func newCredentialsCallback() func(registry string) (docker.Credentials, error) {
+	firstTime := true
+	return func(registry string) (docker.Credentials, error) {
+		var result docker.Credentials
+		var qs = []*survey.Question{
+			{
+				Name: "username",
+				Prompt: &survey.Input{
+					Message: "Username:",
+				},
+				Validate: survey.Required,
 			},
-			Validate: survey.Required,
-		},
-		{
-			Name: "password",
-			Prompt: &survey.Password{
-				Message: "Password:",
+			{
+				Name: "password",
+				Prompt: &survey.Password{
+					Message: "Password:",
+				},
+				Validate: survey.Required,
 			},
-			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
 	}
-	err = survey.Ask(qs, &result)
-
-	return result, err
 }
 
 type deployConfig struct {
diff --git a/docker/credentials_helper.go b/docker/credentials_helper.go
index 7f8c7259..f4bf1467 100644
--- a/docker/credentials_helper.go
+++ b/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 {
diff --git a/docker/pusher.go b/docker/pusher.go
index 76308117..d1e8c248 100644
--- a/docker/pusher.go
+++ b/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()
 


Test patch:
diff --git a/docker/pusher_test.go b/docker/pusher_test.go
index cb399eb9..4c49689b 100644
--- a/docker/pusher_test.go
+++ b/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)
+		}
+	}
+}