secrets: refactor template providers to hold secrets in memory (#26506)

client/allocrunner/taskrunner/secrets/nomad_provider.go

@@ -6,11 +6,9 @@ package secrets
 import (
 	"errors"
 	"fmt"
-	"os"
 	"path/filepath"
 	"strings"
 
-	"github.com/hashicorp/go-envparse"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/mapstructure"
 )
@@ -69,25 +67,6 @@ func (n *NomadProvider) BuildTemplate() *structs.Template {
 	}
 }
 
-func (n *NomadProvider) Parse() (map[string]string, error) {
-	r, err := os.OpenRoot(n.secretDir)
-	if err != nil {
-		return nil, fmt.Errorf("error opening task secrets directory: %v", err)
-	}
-	defer r.Close()
-
-	f, err := r.Open(n.tmplFile)
-	if err != nil {
-		return nil, fmt.Errorf("error opening env template: %v", err)
-	}
-	defer func() {
-		f.Close()
-		r.Remove(n.tmplFile)
-	}()
-
-	return envparse.Parse(f)
-}
-
 // validateNomadInputs ensures none of the user provided inputs contain delimiters
 // that could be used to inject other CT functions.
 func validateNomadInputs(conf *nomadProviderConfig, path string) error {

client/allocrunner/taskrunner/secrets/nomad_provider_test.go

@@ -4,8 +4,6 @@
 package secrets
 
 import (
-	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/hashicorp/nomad/nomad/structs"
@@ -82,23 +80,3 @@ func TestNomadProvider_BuildTemplate(t *testing.T) {
 		must.Error(t, err)
 	})
 }
-
-func TestNomadProvider_Parse(t *testing.T) {
-	testDir := t.TempDir()
-	tmplFile := "foo"
-	tmplPath := filepath.Join(testDir, tmplFile)
-
-	data := "foo=bar"
-	err := os.WriteFile(tmplPath, []byte(data), 0777)
-	must.NoError(t, err)
-
-	p, err := NewNomadProvider(&structs.Secret{}, testDir, tmplFile, "default")
-	must.NoError(t, err)
-
-	vars, err := p.Parse()
-	must.NoError(t, err)
-	must.Eq(t, vars, map[string]string{"foo": "bar"})
-
-	_, err = os.Stat(tmplPath)
-	must.ErrorContains(t, err, "no such file")
-}

client/allocrunner/taskrunner/secrets/plugin_provider.go

@@ -5,7 +5,6 @@ package secrets
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	"github.com/hashicorp/nomad/client/commonplugins"
@@ -15,9 +14,6 @@ type ExternalPluginProvider struct {
 	// plugin is the commonplugin to be executed by this secret
 	plugin commonplugins.SecretsPlugin
 
-	// response is the plugin response saved after Fetch is called
-	response *commonplugins.SecretResponse
-
 	// name of the plugin and also the executable
 	name string
 
@@ -38,26 +34,17 @@ func NewExternalPluginProvider(plugin commonplugins.SecretsPlugin, name string,
 	}
 }
 
-func (p *ExternalPluginProvider) Fetch(ctx context.Context) error {
+func (p *ExternalPluginProvider) Fetch(ctx context.Context) (map[string]string, error) {
 	resp, err := p.plugin.Fetch(ctx, p.path)
 	if err != nil {
-		return fmt.Errorf("failed to fetch secret from plugin %s: %w", p.name, err)
+		return nil, fmt.Errorf("failed to fetch secret from plugin %s: %w", p.name, err)
 	}
 	if resp.Error != nil {
-		return fmt.Errorf("error returned from secret plugin %s: %s", p.name, *resp.Error)
+		return nil, fmt.Errorf("error returned from secret plugin %s: %s", p.name, *resp.Error)
 	}
 
-	p.response = resp
-	return nil
-}
-
-func (p *ExternalPluginProvider) Parse() (map[string]string, error) {
-	if p.response == nil {
-		return nil, errors.New("no plugin response for provider to parse")
-	}
-
-	formatted := map[string]string{}
-	for k, v := range p.response.Result {
+	formatted := make(map[string]string, len(resp.Result))
+	for k, v := range resp.Result {
 		formatted[fmt.Sprintf("secret.%s.%s", p.name, k)] = v
 	}
 

client/allocrunner/taskrunner/secrets/plugin_provider_test.go

@@ -44,8 +44,9 @@ func TestExternalPluginProvider_Fetch(t *testing.T) {
 
 		testProvider := NewExternalPluginProvider(mockSecretPlugin, "test", "test")
 
-		err := testProvider.Fetch(context.Background())
+		vars, err := testProvider.Fetch(t.Context())
 		must.ErrorContains(t, err, "something bad")
+		must.Nil(t, vars)
 	})
 
 	t.Run("errors if fetch response contains error", func(t *testing.T) {
@@ -58,35 +59,28 @@ func TestExternalPluginProvider_Fetch(t *testing.T) {
 
 		testProvider := NewExternalPluginProvider(mockSecretPlugin, "test", "test")
 
-		err := testProvider.Fetch(context.Background())
+		vars, err := testProvider.Fetch(t.Context())
 		must.ErrorContains(t, err, "error returned from secret plugin")
+		must.Nil(t, vars)
 	})
-}
 
-func TestExternalPluginProvider_Parse(t *testing.T) {
 	t.Run("formats response correctly", func(t *testing.T) {
-		testProvider := NewExternalPluginProvider(nil, "test", "test")
-		testProvider.response = &commonplugins.SecretResponse{
+		mockSecretPlugin := new(MockSecretPlugin)
+		mockSecretPlugin.On("Fetch", mock.Anything).Return(&commonplugins.SecretResponse{
 			Result: map[string]string{
 				"testkey": "testvalue",
 			},
-		}
+			Error: nil,
+		}, nil)
 
-		result, err := testProvider.Parse()
+		testProvider := NewExternalPluginProvider(mockSecretPlugin, "test", "test")
+
+		result, err := testProvider.Fetch(t.Context())
 		must.NoError(t, err)
 
 		exp := map[string]string{
 			"secret.test.testkey": "testvalue",
 		}
 		must.Eq(t, exp, result)
-
-	})
-	t.Run("errors if response is nil", func(t *testing.T) {
-		testProvider := NewExternalPluginProvider(nil, "test", "test")
-		testProvider.response = nil
-
-		result, err := testProvider.Parse()
-		must.Error(t, err)
-		must.Nil(t, result)
 	})
 }

client/allocrunner/taskrunner/secrets/vault_provider.go

@@ -6,11 +6,9 @@ package secrets
 import (
 	"errors"
 	"fmt"
-	"os"
 	"path/filepath"
 	"strings"
 
-	"github.com/hashicorp/go-envparse"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/mapstructure"
 )
@@ -78,22 +76,3 @@ func (v *VaultProvider) BuildTemplate() *structs.Template {
 		Once:         true,
 	}
 }
-
-func (v *VaultProvider) Parse() (map[string]string, error) {
-	r, err := os.OpenRoot(v.secretDir)
-	if err != nil {
-		return nil, fmt.Errorf("error opening task secrets directory: %v", err)
-	}
-	defer r.Close()
-
-	f, err := r.Open(v.tmplFile)
-	if err != nil {
-		return nil, fmt.Errorf("error opening env template: %v", err)
-	}
-	defer func() {
-		f.Close()
-		r.Remove(v.tmplFile)
-	}()
-
-	return envparse.Parse(f)
-}

client/allocrunner/taskrunner/secrets/vault_provider_test.go

@@ -4,8 +4,6 @@
 package secrets
 
 import (
-	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/hashicorp/nomad/nomad/structs"
@@ -89,24 +87,3 @@ func TestVaultProvider_BuildTemplate(t *testing.T) {
 		must.Error(t, err)
 	})
 }
-
-func TestVaultProvider_Parse(t *testing.T) {
-	testDir := t.TempDir()
-
-	tmplFile := "foo"
-	tmplPath := filepath.Join(testDir, tmplFile)
-
-	data := "foo=bar"
-	err := os.WriteFile(tmplPath, []byte(data), 0777)
-	must.NoError(t, err)
-
-	p, err := NewVaultProvider(&structs.Secret{}, testDir, tmplFile)
-	must.NoError(t, err)
-
-	vars, err := p.Parse()
-	must.NoError(t, err)
-	must.Eq(t, vars, map[string]string{"foo": "bar"})
-
-	_, err = os.Stat(tmplPath)
-	must.ErrorContains(t, err, "no such file")
-}

client/allocrunner/taskrunner/secrets_hook.go

@@ -4,9 +4,13 @@
 package taskrunner
 
 import (
+	"bytes"
 	"context"
 	"fmt"
+	"sync"
 
+	"github.com/hashicorp/consul-template/renderer"
+	"github.com/hashicorp/go-envparse"
 	log "github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/nomad/client/allocrunner/interfaces"
@@ -19,22 +23,12 @@ import (
 	"github.com/hashicorp/nomad/nomad/structs"
 )
 
-// SecretProvider currently only supports Vault and Nomad which use CT. Future
-// work can modify this interface to include custom providers using a plugin
-// interface.
-type SecretProvider interface {
-	// Parse allows each provider implementation to parse its "response" object.
-	Parse() (map[string]string, error)
-}
-
 type TemplateProvider interface {
-	SecretProvider
 	BuildTemplate() *structs.Template
 }
 
 type PluginProvider interface {
-	SecretProvider
-	Fetch(context.Context) error
+	Fetch(context.Context) (map[string]string, error)
 }
 
 type secretsHookConfig struct {
@@ -97,27 +91,21 @@ func (h *secretsHook) Name() string {
 }
 
 func (h *secretsHook) Prestart(ctx context.Context, req *interfaces.TaskPrestartRequest, resp *interfaces.TaskPrestartResponse) error {
-	templates := []*structs.Template{}
-
-	providers, err := h.buildSecretProviders(req.TaskDir.SecretsDir)
+	tmplProvider, pluginProvider, err := h.buildSecretProviders(req.TaskDir.SecretsDir)
 	if err != nil {
 		return err
 	}
 
-	for _, p := range providers {
-		switch v := p.(type) {
-		case TemplateProvider:
-			templates = append(templates, v.BuildTemplate())
-		case PluginProvider:
-			if err := v.Fetch(ctx); err != nil {
-				return err
-			}
-		}
+	templates := []*structs.Template{}
+	for _, p := range tmplProvider {
+		templates = append(templates, p.BuildTemplate())
 	}
 
 	vaultCluster := req.Task.GetVaultClusterName()
 	vaultConfig := h.clientConfig.GetVaultConfigs(h.logger)[vaultCluster]
 
+	mu := &sync.Mutex{}
+	contents := []byte{}
 	unblock := make(chan struct{})
 	tm, err := template.NewTaskTemplateManager(&template.TaskTemplateManagerConfig{
 		UnblockCh:            unblock,
@@ -135,12 +123,25 @@ func (h *secretsHook) Prestart(ctx context.Context, req *interfaces.TaskPrestart
 		NomadToken:           req.NomadToken,
 		TaskID:               req.Alloc.ID + "-" + req.Task.Name,
 		Logger:               h.logger,
+
+		// This RenderFunc is used to keep any secret data from being written to disk.
+		RenderFunc: func(ri *renderer.RenderInput) (*renderer.RenderResult, error) {
+			// This RenderFunc is called by a single goroutine synchronously, but we
+			// lock the append in the event this behavior changes without us knowing.
+			mu.Lock()
+			defer mu.Unlock()
+			contents = append(contents, ri.Contents...)
+			return &renderer.RenderResult{
+				DidRender:   true,
+				WouldRender: true,
+				Contents:    ri.Contents,
+			}, nil
+		},
 	})
 	if err != nil {
 		return err
 	}
 
-	// Run the template manager to render templates.
 	go tm.Run()
 
 	// Safeguard against the template manager continuing to run.
@@ -152,9 +153,16 @@ func (h *secretsHook) Prestart(ctx context.Context, req *interfaces.TaskPrestart
 	case <-unblock:
 	}
 
-	// parse and copy variables to envBuilder secrets
-	for _, p := range providers {
-		vars, err := p.Parse()
+	// Set secrets from templates
+	m, err := envparse.Parse(bytes.NewBuffer(contents))
+	if err != nil {
+		return err
+	}
+	h.envBuilder.SetSecrets(m)
+
+	// Set secrets from plugin providers
+	for _, p := range pluginProvider {
+		vars, err := p.Fetch(ctx)
 		if err != nil {
 			return err
 		}
@@ -165,10 +173,10 @@ func (h *secretsHook) Prestart(ctx context.Context, req *interfaces.TaskPrestart
 	return nil
 }
 
-func (h *secretsHook) buildSecretProviders(secretDir string) ([]SecretProvider, error) {
+func (h *secretsHook) buildSecretProviders(secretDir string) ([]TemplateProvider, []PluginProvider, error) {
 	// Any configuration errors will be found when calling the secret providers constructor,
 	// so use a multierror to collect all errors and return them to the user at the same time.
-	providers, mErr := []SecretProvider{}, new(multierror.Error)
+	tmplProvider, pluginProvider, mErr := []TemplateProvider{}, []PluginProvider{}, new(multierror.Error)
 
 	for idx, s := range h.secrets {
 		if s == nil {
@@ -181,13 +189,13 @@ func (h *secretsHook) buildSecretProviders(secretDir string) ([]SecretProvider,
 			if p, err := secrets.NewNomadProvider(s, secretDir, tmplFile, h.nomadNamespace); err != nil {
 				multierror.Append(mErr, err)
 			} else {
-				providers = append(providers, p)
+				tmplProvider = append(tmplProvider, p)
 			}
 		case "vault":
 			if p, err := secrets.NewVaultProvider(s, secretDir, tmplFile); err != nil {
 				multierror.Append(mErr, err)
 			} else {
-				providers = append(providers, p)
+				tmplProvider = append(tmplProvider, p)
 			}
 		default:
 			plug, err := commonplugins.NewExternalSecretsPlugin(h.clientConfig.CommonPluginDir, s.Provider)
@@ -195,9 +203,9 @@ func (h *secretsHook) buildSecretProviders(secretDir string) ([]SecretProvider,
 				multierror.Append(mErr, err)
 				continue
 			}
-			providers = append(providers, secrets.NewExternalPluginProvider(plug, s.Name, s.Path))
+			pluginProvider = append(pluginProvider, secrets.NewExternalPluginProvider(plug, s.Name, s.Path))
 		}
 	}
 
-	return providers, mErr.ErrorOrNil()
+	return tmplProvider, pluginProvider, mErr.ErrorOrNil()
 }

client/allocrunner/taskrunner/secrets_hook_test.go

@@ -30,7 +30,7 @@ import (
 func TestSecretsHook_Prestart_Nomad(t *testing.T) {
 	ci.Parallel(t)
 
-	t.Run("nomad provider successfully renders valid secret", func(t *testing.T) {
+	t.Run("nomad provider successfully renders valid secrets", func(t *testing.T) {
 		secretsResp := `
 		{
 		  "CreateIndex": 812,
@@ -83,6 +83,14 @@ func TestSecretsHook_Prestart_Nomad(t *testing.T) {
 					"namespace": "default",
 				},
 			},
+			{
+				Name:     "test_secret1",
+				Provider: "nomad",
+				Path:     "testnomadvar1",
+				Config: map[string]any{
+					"namespace": "default",
+				},
+			},
 		})
 
 		req := &interfaces.TaskPrestartRequest{
@@ -98,8 +106,10 @@ func TestSecretsHook_Prestart_Nomad(t *testing.T) {
 		must.NoError(t, err)
 
 		expected := map[string]string{
-			"secret.test_secret.key1": "value1",
-			"secret.test_secret.key2": "value2",
+			"secret.test_secret.key1":  "value1",
+			"secret.test_secret.key2":  "value2",
+			"secret.test_secret1.key1": "value1",
+			"secret.test_secret1.key2": "value2",
 		}
 		must.Eq(t, expected, taskEnv.Build().TaskSecrets)
 	})
@@ -277,6 +287,14 @@ func TestSecretsHook_Prestart_Vault(t *testing.T) {
 				"engine": "kv_v2",
 			},
 		},
+		{
+			Name:     "test_secret1",
+			Provider: "vault",
+			Path:     "/test/path1",
+			Config: map[string]any{
+				"engine": "kv_v2",
+			},
+		},
 	})
 
 	// Start template hook with a timeout context to ensure it exists.
@@ -293,7 +311,8 @@ func TestSecretsHook_Prestart_Vault(t *testing.T) {
 	must.NoError(t, err)
 
 	exp := map[string]string{
-		"secret.test_secret.secret": "secret",
+		"secret.test_secret.secret":  "secret",
+		"secret.test_secret1.secret": "secret",
 	}
 
 	must.Eq(t, exp, taskEnv.Build().TaskSecrets)

client/allocrunner/taskrunner/template/template.go

@@ -18,6 +18,7 @@ import (
 
 	ctconf "github.com/hashicorp/consul-template/config"
 	"github.com/hashicorp/consul-template/manager"
+	"github.com/hashicorp/consul-template/renderer"
 	"github.com/hashicorp/consul-template/signals"
 	envparse "github.com/hashicorp/go-envparse"
 	"github.com/hashicorp/go-hclog"
@@ -132,6 +133,11 @@ type TaskTemplateManagerConfig struct {
 	TaskID string
 
 	Logger hclog.Logger
+
+	// RenderFunc allows custom rendering of templated data, and overrides the
+	// Nomad custom RenderFunc used for sandboxing. This is currently used by
+	// the secrets block to hold all templated data in memory.
+	RenderFunc renderer.Renderer
 }
 
 // Validate validates the configuration.
@@ -980,7 +986,11 @@ func newRunnerConfig(config *TaskTemplateManagerConfig,
 	sandboxEnabled := isSandboxEnabled(config)
 	sandboxDir := filepath.Dir(config.TaskDir) // alloc working directory
 	conf.ReaderFunc = ReaderFn(config.TaskID, sandboxDir, sandboxEnabled)
-	conf.RendererFunc = RenderFn(config.TaskID, sandboxDir, sandboxEnabled)
+	if config.RenderFunc != nil {
+		conf.RendererFunc = config.RenderFunc
+	} else {
+		conf.RendererFunc = RenderFn(config.TaskID, sandboxDir, sandboxEnabled)
+	}
 	conf.Finalize()
 	return conf, nil
 }