Backport of WI: ensure Consul hook and WID manager interpolate servic…

…es into release/1.7.x (#20363) Services can have some of their string fields interpolated. The new Workload Identity flow doesn't interpolate the services before requesting signed identities or using those identities to get Consul tokens. Add support for interpolation to the WID manager and the Consul tokens hook by providing both with a taskenv builder. Add an "interpolate workload" field to the WI handle to allow passing the original workload name to the server so the server can find the correct service to sign. This changeset also makes two related test improvements: * Remove the mock WID manager, which was only used in the Consul hook tests and isn't necessary so long as we provide the real WID manager with the mock signer and never call `Run` on it. It wasn't feasible to exercise the correct behavior without this refactor, as the mocks were bypassing the new code. * Fixed swapped expect-vs-actual assertions on the `consul_hook` tests. Fixes: #20025 Co-authored-by: Tim Gross <[email protected]>
hashicorp · Apr 11, 2024 · 951251a · 951251a
1 parent 91e93ec
commit 951251a
Show file tree

Hide file tree

Showing 18 changed files with 127 additions and 101 deletions.
diff --git a/.changelog/20344.txt b/.changelog/20344.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+consul: Fixed a bug where services with interpolation would not get correctly signed Workload Identities
+```
diff --git a/client/allocrunner/alloc_runner.go b/client/allocrunner/alloc_runner.go
@@ -31,6 +31,7 @@ import (
 	"github.com/hashicorp/nomad/client/serviceregistration/wrapper"
 	cstate "github.com/hashicorp/nomad/client/state"
 	cstructs "github.com/hashicorp/nomad/client/structs"
+	"github.com/hashicorp/nomad/client/taskenv"
 	"github.com/hashicorp/nomad/client/vaultclient"
 	"github.com/hashicorp/nomad/client/widmgr"
 	"github.com/hashicorp/nomad/helper/pointer"
@@ -272,8 +273,17 @@ func NewAllocRunner(config *config.AllocRunnerConfig) (interfaces.AllocRunner, e
 	ar.shutdownDelayCtx = shutdownDelayCtx
 	ar.shutdownDelayCancelFn = shutdownDelayCancel
 
+	// Create a *taskenv.Builder for the allocation so the WID manager can
+	// interpolate services with the allocation and tasks as needed
+	envBuilder := taskenv.NewBuilder(
+		config.ClientConfig.Node,
+		ar.Alloc(),
+		nil,
+		config.ClientConfig.Region,
+	).SetAllocDir(ar.allocDir.AllocDir)
+
 	// initialize the workload identity manager
-	widmgr := widmgr.NewWIDMgr(ar.widsigner, alloc, ar.stateDB, ar.logger)
+	widmgr := widmgr.NewWIDMgr(ar.widsigner, alloc, ar.stateDB, ar.logger, envBuilder)
 	ar.widmgr = widmgr
 
 	// Initialize the runners hooks.

diff --git a/client/allocrunner/alloc_runner_hooks.go b/client/allocrunner/alloc_runner_hooks.go
@@ -110,7 +110,7 @@ func (ar *allocRunner) initRunnerHooks(config *clientconfig.Config) error {
 			SetAllocDir(ar.allocDir.AllocDir)
 	}
 
-	// Create a taskenv.TaskEnv which is used for read only purposes by the
+	// Create a *taskenv.TaskEnv which is used for read only purposes by the
 	// newNetworkHook and newChecksHook.
 	builtTaskEnv := newEnvBuilder().Build()
 
@@ -128,6 +128,7 @@ func (ar *allocRunner) initRunnerHooks(config *clientconfig.Config) error {
 			consulConfigs:           ar.clientConfig.GetConsulConfigs(hookLogger),
 			consulClientConstructor: consul.NewConsulClient,
 			hookResources:           ar.hookResources,
+			envBuilder:              newEnvBuilder,
 			logger:                  hookLogger,
 		}),
 		newUpstreamAllocsHook(hookLogger, ar.prevAllocWatcher),

diff --git a/client/allocrunner/consul_hook.go b/client/allocrunner/consul_hook.go
@@ -12,6 +12,7 @@ import (
 	"github.com/hashicorp/nomad/client/allocdir"
 	"github.com/hashicorp/nomad/client/consul"
 	cstructs "github.com/hashicorp/nomad/client/structs"
+	"github.com/hashicorp/nomad/client/taskenv"
 	"github.com/hashicorp/nomad/client/widmgr"
 	"github.com/hashicorp/nomad/nomad/structs"
 	structsc "github.com/hashicorp/nomad/nomad/structs/config"
@@ -24,6 +25,7 @@ type consulHook struct {
 	consulConfigs           map[string]*structsc.ConsulConfig
 	consulClientConstructor func(*structsc.ConsulConfig, log.Logger) (consul.Client, error)
 	hookResources           *cstructs.AllocHookResources
+	envBuilder              *taskenv.Builder
 
 	logger log.Logger
 }
@@ -42,6 +44,9 @@ type consulHookConfig struct {
 	// hookResources is used for storing and retrieving Consul tokens
 	hookResources *cstructs.AllocHookResources
 
+	// envBuilder is used to interpolate services
+	envBuilder func() *taskenv.Builder
+
 	logger log.Logger
 }
 
@@ -53,6 +58,7 @@ func newConsulHook(cfg consulHookConfig) *consulHook {
 		consulConfigs:           cfg.consulConfigs,
 		consulClientConstructor: cfg.consulClientConstructor,
 		hookResources:           cfg.hookResources,
+		envBuilder:              cfg.envBuilder(),
 	}
 	h.logger = cfg.logger.Named(h.Name())
 	return h
@@ -81,11 +87,12 @@ func (h *consulHook) Prerun() error {
 	}
 
 	var mErr *multierror.Error
-	if err := h.prepareConsulTokensForServices(tg.Services, tg, tokens); err != nil {
+	if err := h.prepareConsulTokensForServices(tg.Services, tg, tokens, h.envBuilder.Build()); err != nil {
 		mErr = multierror.Append(mErr, err)
 	}
 	for _, task := range tg.Tasks {
-		if err := h.prepareConsulTokensForServices(task.Services, tg, tokens); err != nil {
+		h.envBuilder.UpdateTask(h.alloc, task)
+		if err := h.prepareConsulTokensForServices(task.Services, tg, tokens, h.envBuilder.Build()); err != nil {
 			mErr = multierror.Append(mErr, err)
 		}
 		if err := h.prepareConsulTokensForTask(task, tg, tokens); err != nil {
@@ -155,7 +162,7 @@ func (h *consulHook) prepareConsulTokensForTask(task *structs.Task, tg *structs.
 	return nil
 }
 
-func (h *consulHook) prepareConsulTokensForServices(services []*structs.Service, tg *structs.TaskGroup, tokens map[string]map[string]*consulapi.ACLToken) error {
+func (h *consulHook) prepareConsulTokensForServices(services []*structs.Service, tg *structs.TaskGroup, tokens map[string]map[string]*consulapi.ACLToken, env *taskenv.TaskEnv) error {
 	if len(services) == 0 {
 		return nil
 	}
@@ -174,8 +181,8 @@ func (h *consulHook) prepareConsulTokensForServices(services []*structs.Service,
 		}
 
 		// Find signed identity workload.
-		identity := *service.IdentityHandle()
-		jwt, err := h.widmgr.Get(identity)
+		handle := *service.IdentityHandle(env.ReplaceEnv)
+		jwt, err := h.widmgr.Get(handle)
 		if err != nil {
 			mErr = multierror.Append(mErr, fmt.Errorf(
 				"error getting signed identity for service %s: %v",
@@ -189,7 +196,7 @@ func (h *consulHook) prepareConsulTokensForServices(services []*structs.Service,
 			JWT:            jwt.JWT,
 			AuthMethodName: consulConfig.ServiceIdentityAuthMethod,
 			Meta: map[string]string{
-				"requested_by": fmt.Sprintf("nomad_service_%s", identity.WorkloadIdentifier),
+				"requested_by": fmt.Sprintf("nomad_service_%s", handle.InterpolatedWorkloadIdentifier),
 			},
 		}
 		token, err := h.getConsulToken(clusterName, req)

diff --git a/client/allocrunner/consul_hook_test.go b/client/allocrunner/consul_hook_test.go
@@ -13,7 +13,9 @@ import (
 	"github.com/hashicorp/nomad/ci"
 	"github.com/hashicorp/nomad/client/allocrunner/interfaces"
 	"github.com/hashicorp/nomad/client/consul"
+	cstate "github.com/hashicorp/nomad/client/state"
 	cstructs "github.com/hashicorp/nomad/client/structs"
+	"github.com/hashicorp/nomad/client/taskenv"
 	"github.com/hashicorp/nomad/client/widmgr"
 	"github.com/hashicorp/nomad/helper/testlog"
 	"github.com/hashicorp/nomad/nomad/mock"
@@ -41,45 +43,30 @@ func consulHookTestHarness(t *testing.T) *consulHook {
 			Provider: structs.ServiceProviderConsul,
 			Identity: &structs.WorkloadIdentity{Name: "consul-service_webservice", Audience: []string{"consul.io"}},
 			Cluster:  "default",
-			Name:     "webservice",
-			TaskName: "web",
+			Name:     "${NOMAD_TASK_NAME}service",
+			TaskName: "web", // note: this doesn't interpolate
 		},
 	}
 
-	identitiesToSign := []*structs.WorkloadIdentity{}
-	identitiesToSign = append(identitiesToSign, task.Identities...)
-	for _, service := range task.Services {
-		identitiesToSign = append(identitiesToSign, service.Identity)
-	}
+	// setup mock signer but don't sign identities, as we're going to want them
+	// interpolated by the WIDMgr
+	mockSigner := widmgr.NewMockWIDSigner(nil)
+	db := cstate.NewMemDB(logger)
 
-	// setup mock signer and sign the identities
-	mockSigner := widmgr.NewMockWIDSigner(identitiesToSign)
-	signedIDs, err := mockSigner.SignIdentities(1, []*structs.WorkloadIdentityRequest{
-		{
-			AllocID: alloc.ID,
-			WIHandle: structs.WIHandle{
-				WorkloadIdentifier: task.Name,
-				IdentityName:       task.Identities[0].Name,
-			},
-		},
-		{
-			AllocID: alloc.ID,
-			WIHandle: structs.WIHandle{
-				WorkloadIdentifier: task.Services[0].Name,
-				IdentityName:       task.Services[0].Identity.Name,
-				WorkloadType:       structs.WorkloadTypeService,
-			},
-		},
-	})
-	must.NoError(t, err)
+	// the WIDMgr env builder never has the task available
+	envBuilder := taskenv.NewBuilder(mock.Node(), alloc, nil, "global")
 
-	mockWIDMgr := widmgr.NewMockWIDMgr(signedIDs)
+	mockWIDMgr := widmgr.NewWIDMgr(mockSigner, alloc, db, logger, envBuilder)
+	mockWIDMgr.SignForTesting()
 
 	consulConfigs := map[string]*structsc.ConsulConfig{
 		"default": structsc.DefaultConsulConfig(),
 	}
 
 	hookResources := cstructs.NewAllocHookResources()
+	envBuilderFn := func() *taskenv.Builder {
+		return taskenv.NewBuilder(mock.Node(), alloc, task, "global")
+	}
 
 	consulHookCfg := consulHookConfig{
 		alloc:                   alloc,
@@ -88,6 +75,7 @@ func consulHookTestHarness(t *testing.T) *consulHook {
 		consulConfigs:           consulConfigs,
 		consulClientConstructor: consul.NewMockConsulClient,
 		hookResources:           hookResources,
+		envBuilder:              envBuilderFn,
 		logger:                  logger,
 	}
 	return newConsulHook(consulHookCfg)
@@ -103,29 +91,25 @@ func Test_consulHook_prepareConsulTokensForTask(t *testing.T) {
 	ti := *task.IdentityHandle(wid)
 	jwt, err := hook.widmgr.Get(ti)
 	must.NoError(t, err)
-
 	hashJWT := md5.Sum([]byte(jwt.JWT))
 
 	tests := []struct {
 		name           string
 		task           *structs.Task
-		tokens         map[string]map[string]*consulapi.ACLToken
 		wantErr        bool
 		errMsg         string
 		expectedTokens map[string]map[string]*consulapi.ACLToken
 	}{
 		{
 			name:           "empty task",
 			task:           nil,
-			tokens:         map[string]map[string]*consulapi.ACLToken{},
 			wantErr:        true,
 			errMsg:         "no task specified",
 			expectedTokens: map[string]map[string]*consulapi.ACLToken{},
 		},
 		{
 			name:    "task with signed identity",
 			task:    task,
-			tokens:  map[string]map[string]*consulapi.ACLToken{},
 			wantErr: false,
 			errMsg:  "",
 			expectedTokens: map[string]map[string]*consulapi.ACLToken{
@@ -144,21 +128,21 @@ func Test_consulHook_prepareConsulTokensForTask(t *testing.T) {
 					{Name: structs.ConsulTaskIdentityNamePrefix + "_default"}},
 				Name: "foo",
 			},
-			tokens:         map[string]map[string]*consulapi.ACLToken{},
 			wantErr:        true,
 			errMsg:         "unable to find token for workload",
 			expectedTokens: map[string]map[string]*consulapi.ACLToken{},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := hook.prepareConsulTokensForTask(tt.task, nil, tt.tokens)
+			tokens := map[string]map[string]*consulapi.ACLToken{}
+			err := hook.prepareConsulTokensForTask(tt.task, nil, tokens)
 			if tt.wantErr {
 				must.Error(t, err)
 				must.ErrorContains(t, err, tt.errMsg)
 			} else {
 				must.NoError(t, err)
-				must.Eq(t, tt.tokens, tt.expectedTokens)
+				must.Eq(t, tt.expectedTokens, tokens)
 			}
 		})
 	}
@@ -170,37 +154,35 @@ func Test_consulHook_prepareConsulTokensForServices(t *testing.T) {
 	hook := consulHookTestHarness(t)
 	task := hook.alloc.LookupTask("web")
 	services := task.Services
-
+	env := hook.envBuilder.Build()
 	hashedJWT := make(map[string]string)
+
 	for _, s := range services {
-		widHandle := *s.IdentityHandle()
+		widHandle := *s.IdentityHandle(env.ReplaceEnv)
 		jwt, err := hook.widmgr.Get(widHandle)
 		must.NoError(t, err)
 
 		hash := md5.Sum([]byte(jwt.JWT))
-		hashedJWT[s.Name] = hex.EncodeToString(hash[:])
+		hashedJWT[widHandle.InterpolatedWorkloadIdentifier] = hex.EncodeToString(hash[:])
 	}
 
 	tests := []struct {
 		name           string
 		services       []*structs.Service
-		tokens         map[string]map[string]*consulapi.ACLToken
 		wantErr        bool
 		errMsg         string
 		expectedTokens map[string]map[string]*consulapi.ACLToken
 	}{
 		{
 			name:           "empty services",
 			services:       nil,
-			tokens:         map[string]map[string]*consulapi.ACLToken{},
 			wantErr:        false,
 			errMsg:         "",
 			expectedTokens: map[string]map[string]*consulapi.ACLToken{},
 		},
 		{
 			name:     "services with signed identity",
 			services: services,
-			tokens:   map[string]map[string]*consulapi.ACLToken{},
 			wantErr:  false,
 			errMsg:   "",
 			expectedTokens: map[string]map[string]*consulapi.ACLToken{
@@ -224,21 +206,21 @@ func Test_consulHook_prepareConsulTokensForServices(t *testing.T) {
 					TaskName: "web",
 				},
 			},
-			tokens:         map[string]map[string]*consulapi.ACLToken{},
 			wantErr:        true,
 			errMsg:         "unable to find token for workload",
 			expectedTokens: map[string]map[string]*consulapi.ACLToken{},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := hook.prepareConsulTokensForServices(tt.services, nil, tt.tokens)
+			tokens := map[string]map[string]*consulapi.ACLToken{}
+			err := hook.prepareConsulTokensForServices(tt.services, nil, tokens, env)
 			if tt.wantErr {
 				must.Error(t, err)
 				must.ErrorContains(t, err, tt.errMsg)
 			} else {
 				must.NoError(t, err)
-				must.Eq(t, tt.tokens, tt.expectedTokens)
+				must.Eq(t, tt.expectedTokens, tokens)
 			}
 		})
 	}

diff --git a/client/allocrunner/identity_hook_test.go b/client/allocrunner/identity_hook_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/hashicorp/nomad/ci"
 	"github.com/hashicorp/nomad/client/allocrunner/interfaces"
 	cstate "github.com/hashicorp/nomad/client/state"
+	"github.com/hashicorp/nomad/client/taskenv"
 	"github.com/hashicorp/nomad/client/widmgr"
 	"github.com/hashicorp/nomad/helper/testlog"
 	"github.com/hashicorp/nomad/nomad/mock"
@@ -47,9 +48,12 @@ func TestIdentityHook_Prerun(t *testing.T) {
 	logger := testlog.HCLogger(t)
 	db := cstate.NewMemDB(logger)
 
+	// the WIDMgr env builder never has the task available
+	envBuilder := taskenv.NewBuilder(mock.Node(), alloc, nil, "global")
+
 	// setup mock signer and WIDMgr
 	mockSigner := widmgr.NewMockWIDSigner(task.Identities)
-	mockWIDMgr := widmgr.NewWIDMgr(mockSigner, alloc, db, logger)
+	mockWIDMgr := widmgr.NewWIDMgr(mockSigner, alloc, db, logger, envBuilder)
 	allocrunner.widmgr = mockWIDMgr
 	allocrunner.widsigner = mockSigner
 

diff --git a/client/allocrunner/taskrunner/identity_hook_test.go b/client/allocrunner/taskrunner/identity_hook_test.go
@@ -81,7 +81,9 @@ func TestIdentityHook_RenewAll(t *testing.T) {
 	logger := testlog.HCLogger(t)
 	db := cstate.NewMemDB(logger)
 	mockSigner := widmgr.NewMockWIDSigner(task.Identities)
-	mockWIDMgr := widmgr.NewWIDMgr(mockSigner, alloc, db, logger)
+	envBuilder := taskenv.NewBuilder(mock.Node(), alloc, nil, "global")
+
+	mockWIDMgr := widmgr.NewWIDMgr(mockSigner, alloc, db, logger, envBuilder)
 	mockWIDMgr.SetMinWait(time.Second) // fast renewals, because the default is 10s
 	mockLifecycle := trtesting.NewMockTaskHooks()
 
@@ -188,7 +190,8 @@ func TestIdentityHook_RenewOne(t *testing.T) {
 	logger := testlog.HCLogger(t)
 	db := cstate.NewMemDB(logger)
 	mockSigner := widmgr.NewMockWIDSigner(task.Identities)
-	mockWIDMgr := widmgr.NewWIDMgr(mockSigner, alloc, db, logger)
+	envBuilder := taskenv.NewBuilder(mock.Node(), alloc, nil, "global")
+	mockWIDMgr := widmgr.NewWIDMgr(mockSigner, alloc, db, logger, envBuilder)
 	mockWIDMgr.SetMinWait(time.Second) // fast renewals, because the default is 10s
 
 	h := &identityHook{

diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go
@@ -33,6 +33,7 @@ import (
 	"github.com/hashicorp/nomad/client/serviceregistration/wrapper"
 	cstate "github.com/hashicorp/nomad/client/state"
 	cstructs "github.com/hashicorp/nomad/client/structs"
+	"github.com/hashicorp/nomad/client/taskenv"
 	"github.com/hashicorp/nomad/helper"
 	structsc "github.com/hashicorp/nomad/nomad/structs/config"
 
@@ -136,6 +137,9 @@ func testTaskRunnerConfig(t *testing.T, alloc *structs.Allocation, taskName stri
 	if vault != nil {
 		vaultFunc = func(_ string) (vaultclient.VaultClient, error) { return vault, nil }
 	}
+	// the envBuilder for the WIDMgr never has access to the task, so don't
+	// include it here
+	envBuilder := taskenv.NewBuilder(mock.Node(), alloc, nil, "global")
 
 	conf := &Config{
 		Alloc:                 alloc,
@@ -157,7 +161,7 @@ func testTaskRunnerConfig(t *testing.T, alloc *structs.Allocation, taskName stri
 		ServiceRegWrapper:     wrapperMock,
 		Getter:                getter.TestSandbox(t),
 		Wranglers:             proclib.MockWranglers(t),
-		WIDMgr:                widmgr.NewWIDMgr(widsigner, alloc, db, logger),
+		WIDMgr:                widmgr.NewWIDMgr(widsigner, alloc, db, logger, envBuilder),
 		AllocHookResources:    cstructs.NewAllocHookResources(),
 	}