Succeeded: Change GetValue to GetValues for Providers to allow batch retrieval

design/proposal-zeroization.md

@@ -13,7 +13,7 @@ Ideally, 2 would happen in a centralized fashion to avoid the need for repeated
 It is worth noting the journey of Secrets in Secretless.
 
 ```
-Resolver.#Resolve -> Provider.#GetValue |-> Handler -> Listener -> Target Backend
+Resolver.#Resolve -> Provider.#GetValues |-> Handler -> Listener -> Target Backend
                                         |
 					|-> EventNotifier.#ResolveSecret
 ```
@@ -87,5 +87,5 @@ The recommendation per Secret usage session is as follows:
 ## Further consideration
 
 1. Audit `Listener -> Target Backend`
-1. Audit `Provider.#GetValue`
+1. Audit `Provider.#GetValues`
 1. Reconsider `EventNotifier.#ResolveSecret`, why do we need this ?

go.sum

@@ -29,6 +29,7 @@ github.com/containerd/containerd v1.3.2 h1:ForxmXkA6tPIvffbrDAcPUIB32QgXkt2XFj+F
 github.com/containerd/containerd v1.3.2/go.mod h1:bC6axHOhabU15QhwfG7w5PipXdVtMXFTttgp+kVtyUA=
 github.com/cyberark/conjur-api-go v0.5.2 h1:8ntk07YNRz5bBwjNXkDEAPR70Yr+J2MN8NGlkhaMC3k=
 github.com/cyberark/conjur-api-go v0.5.2/go.mod h1:hwaReWirzgKor+JtH6vbwZaASDXulvd0SzGCloC5uOc=
+github.com/cyberark/conjur-authn-k8s-client v0.19.0 h1:zHjdKyZ8bu4cRyV3iYMh1/XfIVtTugoiU/CflnboP9Q=
 github.com/cyberark/conjur-authn-k8s-client v0.19.0/go.mod h1:qacUJXCppU1Rg/C+br9B1jBitTq4yG04oc4a+cfI200=
 github.com/cyberark/secretless-broker v1.4.1-0.20191211191712-251c5ec034af/go.mod h1:+GueI3WCJL5gDYaYa38ZokAR8ceEyCVet7MkuZyjf80=
 github.com/cyberark/summon v0.7.0 h1:eBjyNzpJEeFb7BFcOgidM6S/UbEsN7LGSFeQE7szPsk=

internal/plugin/resolver.go

@@ -3,6 +3,7 @@ package plugin
 import (
 	"fmt"
 	"log"
+	"sort"
 	"strings"
 	"sync"
 
@@ -70,7 +71,60 @@ func (resolver *Resolver) Provider(name string) (provider plugin_v1.Provider, er
 	return
 }
 
-// Resolve accepts an list of Providers and a list of Credentials and
+func (resolver *Resolver) resolveForProvider(
+	providerID string,
+	credentials []*config_v2.Credential,
+) (plugin_v1.Provider, map[string]plugin_v1.ProviderResponse, string) {
+	provider, err := resolver.Provider(providerID)
+	if err != nil {
+		resolver.LogFatalf("ERROR: Provider '%s' could not be used! %v", providerID, err)
+	}
+
+	// Create secretIds slice and credentialBySecretId map
+	secretIds := make([]string, len(credentials))
+	for idx, cred := range credentials {
+		secretIds[idx] = cred.Get
+	}
+
+	// Resolves all credentials for current provider
+	var hasErrors bool
+	var providerErrStrings []string
+	providerResponses, err := provider.GetValues(secretIds...)
+	if err != nil {
+		hasErrors = true
+		providerErrStrings = append(
+			providerErrStrings,
+			err.Error(),
+		)
+	}
+
+	// Collect errors from provider responses
+	for _, providerResponse := range providerResponses {
+		if providerResponse.Error != nil {
+			hasErrors = true
+			providerErrStrings = append(
+				providerErrStrings,
+				providerResponse.Error.Error(),
+			)
+			continue
+		}
+	}
+	if hasErrors {
+		// Sort the error strings to provide deterministic output
+		sort.Strings(providerErrStrings)
+
+		errInfo := fmt.Sprintf(
+			"ERROR: Resolving credentials from provider '%s' failed: %v",
+			provider.GetName(),
+			strings.Join(providerErrStrings, ", "),
+		)
+		return nil, nil, errInfo
+	}
+
+	return provider, providerResponses, ""
+}
+
+// Resolve accepts n list of Providers and a list of Credentials and
 // attempts to obtain the value of each Credential from the appropriate Provider.
 func (resolver *Resolver) Resolve(credentials []*config_v2.Credential) (map[string][]byte, error) {
 	if len(credentials) == 0 {
@@ -81,37 +135,61 @@ func (resolver *Resolver) Resolve(credentials []*config_v2.Credential) (map[stri
 	errorStrings := make([]string, 0, len(credentials))
 
 	var err error
-	for _, credential := range credentials {
-		var provider plugin_v1.Provider
-		var value []byte
 
-		if provider, err = resolver.Provider(credential.From); err != nil {
-			resolver.LogFatalf("ERROR: Provider '%s' could not be used! %v", credential.From, err)
-		}
+	// Group credentials by provider
+	credentialsByProvider := groupCredentialsByProvider(credentials)
 
-		// This provider cannot resolve the named credential
-		if value, err = provider.GetValue(credential.Get); err != nil {
-			errInfo := fmt.Sprintf("ERROR: Resolving credential '%s' from provider '%s' failed: %v",
-				credential.Get,
-				credential.From,
-				err)
-			log.Println(errInfo)
+	// Resolve credentials by provider
+	for providerID, credentialsForProvider := range credentialsByProvider {
+		provider, providerResponses, errStr := resolver.resolveForProvider(
+			providerID,
+			credentialsForProvider,
+		)
+		if errStr != "" {
+			log.Println(errStr)
 
-			errorStrings = append(errorStrings, errInfo)
+			errorStrings = append(errorStrings, errStr)
 			continue
 		}
 
-		result[credential.Name] = value
+		// Set provider responses on result map before returning
+		for _, credential := range credentialsForProvider {
+			credentialName := credential.Name
+			secretValue := providerResponses[credential.Get].Value
+
+			result[credentialName] = secretValue
 
-		if resolver.EventNotifier != nil {
-			resolver.EventNotifier.ResolveCredential(provider, credential.Name, value)
+			if resolver.EventNotifier != nil {
+				resolver.EventNotifier.ResolveCredential(
+					provider,
+					credentialName,
+					secretValue,
+				)
+			}
 		}
 	}
 
 	err = nil
 	if len(errorStrings) > 0 {
+		// Sort the error strings to provide deterministic output
+		sort.Strings(errorStrings)
+
 		err = fmt.Errorf(strings.Join(errorStrings, "\n"))
 	}
 
 	return result, err
 }
+
+func groupCredentialsByProvider(
+	credentials []*config_v2.Credential,
+) map[string][]*config_v2.Credential {
+	credentialsByProvider := make(map[string][]*config_v2.Credential)
+	for _, credential := range credentials {
+		credentialsByProvider[credential.From] = append(
+			credentialsByProvider[credential.From],
+			credential,
+		)
+	}
+
+	return credentialsByProvider
+}

internal/plugin/resolver_test.go

@@ -2,6 +2,7 @@ package plugin
 
 import (
 	"fmt"
+	"os"
 	"testing"
 
 	. "github.com/smartystreets/goconvey/convey"
@@ -25,82 +26,60 @@ func newInstance() plugin_v1.Resolver {
 
 func Test_Resolver(t *testing.T) {
 	Convey("Resolve", t, func() {
-		Convey("Can resolve credentials", func() {
-			resolver := newInstance()
-
-			credentials := make([]*config_v2.Credential, 1, 1)
-			credentials[0] = &config_v2.Credential{
-				Name: "foo",
-				From: "literal",
-				Get:  "bar",
-			}
-
-			values, err := resolver.Resolve(credentials)
-			So(err, ShouldBeNil)
-			So(len(values), ShouldEqual, 1)
-		})
-
 		Convey("Exits if credential resolution array is empty", func() {
 			resolver := newInstance()
-
 			credentials := make([]*config_v2.Credential, 0)
-
-			resolveVarFunc := func() {
-				resolver.Resolve(credentials)
-			}
+			resolveVarFunc := func() { resolver.Resolve(credentials) }
 
 			So(resolveVarFunc, ShouldPanic)
 			So(len(fatalErrors), ShouldEqual, 1)
 		})
 
-		Convey("Exits if provider cannot be found", func() {
+		Convey("Exits if even single provider cannot be found", func() {
 			resolver := newInstance()
-
-			credentials := make([]*config_v2.Credential, 1, 1)
-			credentials[0] = &config_v2.Credential{
-				Name: "foo",
-				From: "nope-not-found",
-				Get:  "bar",
+			credentials := []*config_v2.Credential{
+				{Name: "foo", From: "env", Get: "bar"},
+				{Name: "foo", From: "nope-not-found", Get: "bar"},
+				{Name: "baz", From: "also-not-found", Get: "bar"},
 			}
+			resolveVarFunc := func() { resolver.Resolve(credentials) }
 
-			resolveVarFunc := func() {
-				resolver.Resolve(credentials)
-			}
 			So(resolveVarFunc, ShouldPanic)
 			So(len(fatalErrors), ShouldEqual, 1)
 		})
 
-		Convey("Exits if credential can't be resolved", func() {
+		Convey("Returns an error if credential can't be resolved", func() {
 			resolver := newInstance()
-
-			credentials := make([]*config_v2.Credential, 1, 1)
-			credentials[0] = &config_v2.Credential{
-				Name: "foo",
-				From: "env",
-				Get:  "something-not-in-env",
+			credentials := []*config_v2.Credential{
+				{Name: "path", From: "env", Get: "PATH"},
+				{Name: "foo", From: "env", Get: "something-not-in-env"},
+				{Name: "bar", From: "env", Get: "something-also-not-in-env"},
+				{Name: "baz", From: "file", Get: "something-not-on-file"},
 			}
-
 			credentialValues, err := resolver.Resolve(credentials)
+
 			So(len(credentialValues), ShouldEqual, 0)
 			So(err, ShouldNotBeNil)
-			errorMsg := "ERROR: Resolving credential 'something-not-in-env' from provider 'env' failed: env cannot find environment variable 'something-not-in-env'"
-			So(err.Error(), ShouldEqual, errorMsg)
-
+			So(err.Error(), ShouldEqual,
+				"ERROR: Resolving credentials from provider 'env' failed: "+
+					"env cannot find environment variable 'something-also-not-in-env', "+
+					"env cannot find environment variable 'something-not-in-env'\n"+
+					"ERROR: Resolving credentials from provider 'file' failed: "+
+					"open something-not-on-file: no such file or directory")
 		})
 
-		Convey("Can resolve credential2", func() {
+		Convey("Can resolve credential", func() {
 			resolver := newInstance()
-
-			credentials := make([]*config_v2.Credential, 1, 1)
-			credentials[0] = &config_v2.Credential{
-				Name: "foo",
-				From: "literal",
-				Get:  "bar",
+			credentials := []*config_v2.Credential{
+				{Name: "foo", From: "env", Get: "PATH"},
+				{Name: "bar", From: "literal", Get: "bar"},
 			}
-
 			values, err := resolver.Resolve(credentials)
+
 			So(err, ShouldBeNil)
-			So(len(values), ShouldEqual, 1)
+			So(len(values), ShouldEqual, 2)
+			So(string(values["foo"]), ShouldEqual, os.Getenv("PATH"))
+			So(string(values["bar"]), ShouldEqual, "bar")
 		})
 	})
 }

internal/plugin/v1/provider.go

@@ -7,11 +7,47 @@ type ProviderOptions struct {
 	Name string
 }
 
+// ProviderResponse is the response from the provider for a given secret request
+type ProviderResponse struct {
+	Value []byte
+	Error error
+}
+
 // Provider is the interface used to obtain values from a secret vault backend.
 type Provider interface {
 	// GetName returns the name that the Provider was instantiated with
 	GetName() string
 
+	// GetValues takes in variable ids and returns their resolved values
+	GetValues(ids ...string) (map[string]ProviderResponse, error)
+}
+
+type singleValueProvider interface {
 	// GetValue takes in an id of a variable and returns its resolved value
 	GetValue(id string) ([]byte, error)
 }
+
+// GetValues takes in variable ids and returns their resolved values by making sequential
+// getValueCallArgs to a singleValueProvider.
+// This is a convenience function since most providers with batch retrieval capabilities
+// will have need the exact same code. Note: most internal providers simply use this
+// function in their implementation of the Provider interface's GetValues method.
+func GetValues(
+	p singleValueProvider,
+	ids ...string,
+) (map[string]ProviderResponse, error) {
+	responses := map[string]ProviderResponse{}
+
+	for _, id := range ids {
+		if _, ok := responses[id]; ok {
+			continue
+		}
+
+		pr := ProviderResponse{}
+		pr.Value, pr.Error = p.GetValue(id)
+
+		responses[id] = pr
+	}
+
+	return responses, nil
+}

internal/plugin/v1/provider_mock.go

@@ -0,0 +1,57 @@
+package v1
+
+import (
+	"errors"
+	"strings"
+)
+
+// MockProvider conforms to, and allows testing of, both the singleValueProvider and
+// Provider interfaces
+type MockProvider struct {
+	GetValueCallArgs []string // keeps track of args for each call to getValue
+}
+
+// GetValue returns
+// 0. If [id] has prefix 'err_', returns (nil, errors.New(id + "_value"))
+// 1. Otherwise, returns ([]byte(id + "_value"), nil)
+func (p *MockProvider) GetValue(id string) ([]byte, error) {
+	p.GetValueCallArgs = append(p.GetValueCallArgs, id)
+
+	if strings.HasPrefix(id, "err_") {
+		return nil, errors.New(id + "_value")
+	}
+	return []byte(id + "_value"), nil
+}
+
+// GetValues sequentially get values for unique ids by calling GetValue
+//
+// If there exists any id with the prefix 'global_err_', the function will return
+// (nil, errors.New(id + "_value"))
+func (p *MockProvider) GetValues(ids ...string) (
+	map[string]ProviderResponse,
+	error,
+) {
+	responses := map[string]ProviderResponse{}
+
+	for _, id := range ids {
+		if _, ok := responses[id]; ok {
+			continue
+		}
+
+		if strings.HasPrefix(id, "global_err_") {
+			return nil, errors.New(id + "_value")
+		}
+
+		pr := ProviderResponse{}
+		pr.Value, pr.Error = p.GetValue(id)
+
+		responses[id] = pr
+	}
+
+	return responses, nil
+}
+
+// GetName simply returns "mock-provider"
+func (p *MockProvider) GetName() string {
+	return "mock-provider"
+}