From 4d9cc73ed2c03535fdb925ffff07a79b340f4a34 Mon Sep 17 00:00:00 2001
From: Tim Gross <tgross@hashicorp.com>
Date: Mon, 23 Oct 2023 08:57:02 -0400
Subject: [PATCH] sids_hook: fix check for Consul token derived from WI
 (#18821)

The `sids_hook` serves the legacy Connect workflow, and we want to bypass it
when using workload identities. So the hook checks that there's not already a
Consul token in the alloc hook resources derived from the Workload
Identity. This check was looking for the wrong key. This would cause the hook to
ignore the Consul token we already have and then fail to derive a SI token
unless the Nomad agent has its own token with `acl:write` permission.

Fix the lookup and add tests covering the bypass behavior.
---
 client/allocrunner/taskrunner/sids_hook.go    |  3 +-
 .../allocrunner/taskrunner/sids_hook_test.go  | 42 +++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/client/allocrunner/taskrunner/sids_hook.go b/client/allocrunner/taskrunner/sids_hook.go
index 7cc59ee55..b51c459fe 100644
--- a/client/allocrunner/taskrunner/sids_hook.go
+++ b/client/allocrunner/taskrunner/sids_hook.go
@@ -136,7 +136,8 @@ func (h *sidsHook) Prestart(
 	var cluster string
 	for _, service := range tg.Services {
 		if service.Name == serviceName {
-			serviceIdentityName = service.MakeUniqueIdentityName()
+			serviceIdentityName = fmt.Sprintf("%s_%s",
+				structs.ConsulServiceIdentityNamePrefix, service.MakeUniqueIdentityName())
 			cluster = service.Cluster
 			break
 		}
diff --git a/client/allocrunner/taskrunner/sids_hook_test.go b/client/allocrunner/taskrunner/sids_hook_test.go
index 1111f8314..e617a313d 100644
--- a/client/allocrunner/taskrunner/sids_hook_test.go
+++ b/client/allocrunner/taskrunner/sids_hook_test.go
@@ -11,19 +11,23 @@ package taskrunner
 
 import (
 	"context"
+	"errors"
 	"os"
 	"path/filepath"
 	"testing"
 	"time"
 
 	"github.com/hashicorp/nomad/ci"
+	"github.com/hashicorp/nomad/client/allocdir"
 	"github.com/hashicorp/nomad/client/allocrunner/interfaces"
 	consulapi "github.com/hashicorp/nomad/client/consul"
+	cstructs "github.com/hashicorp/nomad/client/structs"
 	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/helper/testlog"
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
+	"github.com/shoenig/test/must"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sys/unix"
 )
@@ -312,3 +316,41 @@ func TestTaskRunner_DeriveSIToken_UnWritableTokenFile(t *testing.T) {
 	r.NoError(err)
 	r.Empty(token)
 }
+
+// TestSIDSHook_WIBypass exercises the code path where we skip deriving SI
+// tokens if we already have Consul tokens in the alloc hook resources (from WI)
+func TestSIDSHook_WIBypass(t *testing.T) {
+	ci.Parallel(t)
+
+	resources := cstructs.NewAllocHookResources()
+	resources.SetConsulTokens(map[string]map[string]string{
+		"default": {
+			"consul_service_": uuid.Generate(),
+		},
+	})
+
+	alloc := mock.ConnectAlloc()
+	taskName, taskKind := sidecar("web")
+	task := &structs.Task{Name: taskName, Kind: taskKind}
+
+	sidsClient := consulapi.NewMockServiceIdentitiesClient()
+	sidsClient.SetDeriveTokenError(alloc.ID, []string{"web"}, errors.New("should never call"))
+
+	h := newSIDSHook(sidsHookConfig{
+		alloc:              alloc,
+		task:               task,
+		sidsClient:         sidsClient,
+		lifecycle:          nil,
+		logger:             testlog.HCLogger(t),
+		allocHookResources: resources,
+	})
+
+	ctx := context.Background()
+	req := &interfaces.TaskPrestartRequest{
+		Task:    task,
+		TaskDir: &allocdir.TaskDir{SecretsDir: t.TempDir()},
+	}
+	resp := &interfaces.TaskPrestartResponse{}
+	must.NoError(t, h.Prestart(ctx, req, resp))
+	must.True(t, resp.Done)
+}