nomad: fixup token policy validation

client/allocrunner/taskrunner/sids_hook.go

@@ -190,30 +190,25 @@ func (h *sidsHook) kill(ctx context.Context, err error) {
 
 // tryDerive loops forever until a token is created, or ctx is done.
 func (h *sidsHook) tryDerive(ctx context.Context, ch chan<- string) {
-	// Derive the SI token using the name of the proxied / native task, not the
-	// name of the literal sidecar task. The virtual ACL policy of the SI token
-	// is oriented this way.
-	siTaskName := h.task.Kind.Value()
-
 	for attempt := 0; backoff(ctx, attempt); attempt++ {
 
-		tokens, err := h.sidsClient.DeriveSITokens(h.alloc, []string{siTaskName})
+		tokens, err := h.sidsClient.DeriveSITokens(h.alloc, []string{h.task.Name})
 
 		switch {
 
 		case err == nil:
 			// nothing broke and we can return the token for the task
-			ch <- tokens[siTaskName]
+			ch <- tokens[h.task.Name]
 			return
 
 		case structs.IsServerSide(err):
 			// the error is known to be a server problem, just die
-			h.logger.Error("failed to derive SI token", "error", err, "task", h.task.Name, "si_task", siTaskName, "server_side", true)
+			h.logger.Error("failed to derive SI token", "error", err, "task", h.task.Name, "server_side", true)
 			h.kill(ctx, errors.Wrap(err, "consul: failed to derive SI token"))
 
 		case !structs.IsRecoverable(err):
 			// the error is known not to be recoverable, just die
-			h.logger.Error("failed to derive SI token", "error", err, "task", h.task.Name, "si_task", siTaskName, "recoverable", false)
+			h.logger.Error("failed to derive SI token", "error", err, "task", h.task.Name, "recoverable", false)
 			h.kill(ctx, errors.Wrap(err, "consul: failed to derive SI token"))
 
 		default:

client/allocrunner/taskrunner/task_runner_test.go

@@ -1192,12 +1192,11 @@ func TestTaskRunner_DeriveSIToken_Retry(t *testing.T) {
 
 	// control when we get a Consul SI token
 	token := "12345678-1234-1234-1234-1234567890"
-	siTaskName := task.Kind.Value()
 	deriveCount := 0
 	deriveFn := func(*structs.Allocation, []string) (map[string]string, error) {
 		if deriveCount > 0 {
 
-			return map[string]string{siTaskName: token}, nil
+			return map[string]string{task.Name: token}, nil
 		}
 		deriveCount++
 		return nil, structs.NewRecoverableError(errors.New("try again later"), true)
@@ -1252,9 +1251,8 @@ func TestTaskRunner_DeriveSIToken_Unrecoverable(t *testing.T) {
 	defer cleanup()
 
 	// SI token derivation suffers a non-retryable error
-	siTaskName := task.Kind.Value()
 	siClient := trConfig.ConsulSI.(*consulapi.MockServiceIdentitiesClient)
-	siClient.SetDeriveTokenError(alloc.ID, []string{siTaskName}, errors.New("non-recoverable"))
+	siClient.SetDeriveTokenError(alloc.ID, []string{task.Name}, errors.New("non-recoverable"))
 
 	tr, err := NewTaskRunner(trConfig)
 	r.NoError(err)

command/agent/consul/acl_testing.go

@@ -35,6 +35,7 @@ func NewMockACLsAPI(l hclog.Logger) *MockACLsAPI {
 	}
 }
 
+// Example Consul policies for use in tests.
 const (
 	ExamplePolicyID1 = "a7c86856-0af5-4ab5-8834-03f4517e5564"
 	ExamplePolicyID2 = "ffa1b66c-967d-4468-8775-c687b5cfc16e"
@@ -47,6 +48,7 @@ func (m *MockACLsAPI) PolicyRead(policyID string, _ *api.QueryOptions) (*api.ACL
 	case ExamplePolicyID1:
 		return &api.ACLPolicy{
 			ID:    ExamplePolicyID1,
+			Name:  "example-policy-1",
 			Rules: `service "service1" { policy = "write" }`,
 		}, nil, nil
 
@@ -69,44 +71,118 @@ service "service2" { policy = "write" }`,
 	}
 }
 
+// Example Consul roles for use in tests.
 const (
-	ExampleOperatorToken1 = "59c219c2-47e4-43f3-bb45-258fd13f59d5"
-	ExampleOperatorToken2 = "868cc216-e123-4c2b-b362-f4d4c087de8e"
-	ExampleOperatorToken3 = "6177d1b9-c0f6-4118-b891-d818a3cb80b1"
+	ExampleRoleID1 = "e569a3a8-7dfb-b024-e492-e790fe3c4183"
+	ExampleRoleID2 = "88c825f4-d0da-1c2b-0c1c-cc9fe84c4468"
+	ExampleRoleID3 = "b19b2058-6205-6dff-d2b0-470f29b8e627"
+)
+
+func (m *MockACLsAPI) RoleRead(roleID string, _ *api.QueryOptions) (*api.ACLRole, *api.QueryMeta, error) {
+	switch roleID {
+	case ExampleRoleID1:
+		return &api.ACLRole{
+			ID:   ExampleRoleID1,
+			Name: "example-role-1",
+			Policies: []*api.ACLRolePolicyLink{{
+				ID:   ExamplePolicyID1,
+				Name: "example-policy-1",
+			}},
+			ServiceIdentities: nil, // would it ever make sense ?
+		}, nil, nil
+	case ExampleRoleID2:
+		return &api.ACLRole{
+			ID:   ExampleRoleID2,
+			Name: "example-role-2",
+			Policies: []*api.ACLRolePolicyLink{{
+				ID:   ExamplePolicyID2,
+				Name: "example-policy-2",
+			}},
+			ServiceIdentities: nil,
+		}, nil, nil
+	case ExampleRoleID3:
+		return &api.ACLRole{
+			ID:                ExampleRoleID3,
+			Name:              "example-role-3",
+			Policies:          nil, // todo
+			ServiceIdentities: nil, // todo
+			ModifyIndex:       0,
+		}, nil, nil
+	default:
+		return nil, nil, nil
+	}
+}
+
+// Example Consul "operator" tokens for use in tests.
+
+const (
+	ExampleOperatorTokenID0 = "de591604-86eb-1e6f-8b44-d4db752921ae"
+	ExampleOperatorTokenID1 = "59c219c2-47e4-43f3-bb45-258fd13f59d5"
+	ExampleOperatorTokenID2 = "868cc216-e123-4c2b-b362-f4d4c087de8e"
+	ExampleOperatorTokenID3 = "6177d1b9-c0f6-4118-b891-d818a3cb80b1"
+	ExampleOperatorTokenID4 = "754ae26c-f3cc-e088-d486-9c0d20f5eaea"
+)
+
+var (
+	ExampleOperatorToken0 = &api.ACLToken{
+		SecretID:    ExampleOperatorTokenID0,
+		AccessorID:  "228865c6-3bf6-6683-df03-06dea2779088 ",
+		Description: "Operator Token 0",
+	}
+
+	ExampleOperatorToken1 = &api.ACLToken{
+		SecretID:    ExampleOperatorTokenID1,
+		AccessorID:  "e341bacd-535e-417c-8f45-f88d7faffcaf",
+		Description: "Operator Token 1",
+		Policies: []*api.ACLTokenPolicyLink{{
+			ID: ExamplePolicyID1,
+		}},
+	}
+
+	ExampleOperatorToken2 = &api.ACLToken{
+		SecretID:    ExampleOperatorTokenID2,
+		AccessorID:  "615b4d77-5164-4ec6-b616-24c0b24ac9cb",
+		Description: "Operator Token 2",
+		Policies: []*api.ACLTokenPolicyLink{{
+			ID: ExamplePolicyID2,
+		}},
+	}
+
+	ExampleOperatorToken3 = &api.ACLToken{
+		SecretID:    ExampleOperatorTokenID3,
+		AccessorID:  "6b7de0d7-15f7-45b4-95eb-fb775bfe3fdc",
+		Description: "Operator Token 3",
+		Policies: []*api.ACLTokenPolicyLink{{
+			ID: ExamplePolicyID3,
+		}},
+	}
+
+	ExampleOperatorToken4 = &api.ACLToken{
+		SecretID:    ExampleOperatorTokenID4,
+		AccessorID:  "7b5fdb1a-71e5-f3d8-2cfe-448d973f327d",
+		Description: "Operator Token 4",
+		Policies:    nil, // no direct policy, only roles
+		Roles: []*api.ACLTokenRoleLink{{
+			ID:   ExampleRoleID1,
+			Name: "example-role-1",
+		}},
+	}
 )
 
 func (m *MockACLsAPI) TokenReadSelf(q *api.QueryOptions) (*api.ACLToken, *api.QueryMeta, error) {
 	switch q.Token {
 
-	case ExampleOperatorToken1:
-		return &api.ACLToken{
-			SecretID:    ExampleOperatorToken1,
-			AccessorID:  "e341bacd-535e-417c-8f45-f88d7faffcaf",
-			Description: "operator token 1",
-			Policies: []*api.ACLTokenPolicyLink{{
-				ID: ExamplePolicyID1,
-			}},
-		}, nil, nil
+	case ExampleOperatorTokenID1:
+		return ExampleOperatorToken1, nil, nil
 
-	case ExampleOperatorToken2:
-		return &api.ACLToken{
-			SecretID:    ExampleOperatorToken2,
-			AccessorID:  "615b4d77-5164-4ec6-b616-24c0b24ac9cb",
-			Description: "operator token 2",
-			Policies: []*api.ACLTokenPolicyLink{{
-				ID: ExamplePolicyID2,
-			}},
-		}, nil, nil
+	case ExampleOperatorTokenID2:
+		return ExampleOperatorToken2, nil, nil
 
-	case ExampleOperatorToken3:
-		return &api.ACLToken{
-			SecretID:    ExampleOperatorToken3,
-			AccessorID:  "6b7de0d7-15f7-45b4-95eb-fb775bfe3fdc",
-			Description: "operator token 3",
-			Policies: []*api.ACLTokenPolicyLink{{
-				ID: ExamplePolicyID3,
-			}},
-		}, nil, nil
+	case ExampleOperatorTokenID3:
+		return ExampleOperatorToken3, nil, nil
+
+	case ExampleOperatorTokenID4:
+		return ExampleOperatorToken4, nil, nil
 
 	default:
 		return nil, nil, errors.New("no such token")

command/agent/consul/client.go

@@ -93,15 +93,14 @@ type AgentAPI interface {
 	UpdateTTL(id, output, status string) error
 }
 
-// ACLsAPI is the consul/api.ACL API used by Nomad Server.
+// ACLsAPI is the consul/api.ACL API subset used by Nomad Server.
 type ACLsAPI interface {
-	// todo: RoleRead (...)
-
 	// We are looking up by [operator token] SecretID, which implies we need
 	// to use this method instead of the normal TokenRead, which can only be
 	// used to lookup tokens by their AccessorID.
 	TokenReadSelf(q *api.QueryOptions) (*api.ACLToken, *api.QueryMeta, error)
 	PolicyRead(policyID string, q *api.QueryOptions) (*api.ACLPolicy, *api.QueryMeta, error)
+	RoleRead(roleID string, q *api.QueryOptions) (*api.ACLRole, *api.QueryMeta, error)
 	TokenCreate(partial *api.ACLToken, q *api.WriteOptions) (*api.ACLToken, *api.WriteMeta, error)
 	TokenDelete(accessorID string, q *api.WriteOptions) (*api.WriteMeta, error)
 	TokenList(q *api.QueryOptions) ([]*api.ACLTokenListEntry, *api.QueryMeta, error)

nomad/consul.go

@@ -9,7 +9,6 @@ import (
 	"github.com/armon/go-metrics"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/go-hclog"
-	"github.com/hashicorp/hcl"
 	"github.com/hashicorp/nomad/command/agent/consul"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/pkg/errors"
@@ -49,36 +48,6 @@ const (
 	ConsulPolicyWrite = "write"
 )
 
-// ConsulServiceRule represents a policy for a service
-type ConsulServiceRule struct {
-	Name   string `hcl:",key"`
-	Policy string
-}
-
-type ConsulPolicy struct {
-	Services        []*ConsulServiceRule `hcl:"service,expand"`
-	ServicePrefixes []*ConsulServiceRule `hcl:"service_prefix,expand"`
-}
-
-func (cp *ConsulPolicy) IsEmpty() bool {
-	if cp == nil {
-		return true
-	}
-	return len(cp.Services) == 0 && len(cp.ServicePrefixes) == 0
-}
-
-func ParseConsulPolicy(s string) (*ConsulPolicy, error) {
-	cp := new(ConsulPolicy)
-	if err := hcl.Decode(cp, s); err != nil {
-		return nil, errors.Wrap(err, "failed to parse ACL policy")
-	}
-	if cp.IsEmpty() {
-		// the only use case for now, may as well validate asap
-		return nil, errors.New("consul policy contains no service rules")
-	}
-	return cp, nil
-}
-
 type ServiceIdentityIndex struct {
 	ClusterID string
 	AllocID   string
@@ -176,72 +145,6 @@ func (c *consulACLsAPI) CheckSIPolicy(_ context.Context, task, secretID string)
 	return nil
 }
 
-func (c *consulACLsAPI) hasSufficientPolicy(task string, token *api.ACLToken) (bool, error) {
-
-	for _, policyRef := range token.Policies {
-		if allowable, err := c.policyAllowsServiceWrite(task, policyRef.ID); err != nil {
-			return false, err
-		} else if allowable {
-			return true, nil
-		}
-	}
-
-	// todo: probably also need to go through each role and check all those
-
-	return false, nil
-}
-
-// policyAllowsServiceWrite
-func (c *consulACLsAPI) policyAllowsServiceWrite(task string, policyID string) (bool, error) {
-	policy, _, err := c.aclClient.PolicyRead(policyID, &api.QueryOptions{
-		AllowStale: false,
-	})
-	if err != nil {
-		return false, err
-	}
-
-	// compare policy to the necessary permission for service write
-	// e.g. service "db" { policy = "write" }
-	// e.g. service_prefix "" { policy == "write" }
-	cp, err := ParseConsulPolicy(policy.Rules)
-	if err != nil {
-		return false, err
-	}
-
-	if c.allowsServiceWrite(task, cp) {
-		return true, nil
-	}
-
-	return false, nil
-}
-
-const (
-	serviceNameWildcard = "*"
-)
-
-func (_ *consulACLsAPI) allowsServiceWrite(task string, cp *ConsulPolicy) bool {
-	for _, service := range cp.Services {
-		name := strings.ToLower(service.Name)
-		policy := strings.ToLower(service.Policy)
-		if policy == ConsulPolicyWrite {
-			if name == task || name == serviceNameWildcard {
-				return true
-			}
-		}
-	}
-
-	for _, servicePrefix := range cp.ServicePrefixes {
-		prefix := strings.ToLower(servicePrefix.Name)
-		policy := strings.ToLower(servicePrefix.Policy)
-		if policy == ConsulPolicyWrite {
-			if strings.HasPrefix(task, prefix) {
-				return true
-			}
-		}
-	}
-	return false
-}
-
 func (c *consulACLsAPI) CreateToken(ctx context.Context, sii ServiceIdentityIndex) (*structs.SIToken, error) {
 	defer metrics.MeasureSince([]string{"nomad", "consul", "create_token"}, time.Now())
 
@@ -254,9 +157,12 @@ func (c *consulACLsAPI) CreateToken(ctx context.Context, sii ServiceIdentityInde
 
 	// todo: rate limiting
 
+	// the token created must be for the service, not the sidecar of the service
+	// https://www.consul.io/docs/acl/acl-system.html#acl-service-identities
+	serviceName := strings.TrimPrefix(sii.TaskName, structs.ConnectProxyPrefix+"-")
 	partial := &api.ACLToken{
 		Description:       sii.Description(),
-		ServiceIdentities: []*api.ACLServiceIdentity{{ServiceName: sii.TaskName}},
+		ServiceIdentities: []*api.ACLServiceIdentity{{ServiceName: serviceName}},
 	}
 
 	token, _, err := c.aclClient.TokenCreate(partial, nil)

nomad/consul_policy.go

@@ -0,0 +1,123 @@
+package nomad
+
+import (
+	"strings"
+
+	"github.com/hashicorp/consul/api"
+	"github.com/hashicorp/hcl"
+	"github.com/pkg/errors"
+)
+
+// ConsulServiceRule represents a policy for a service
+type ConsulServiceRule struct {
+	Name   string `hcl:",key"`
+	Policy string
+}
+
+type ConsulPolicy struct {
+	Services        []*ConsulServiceRule `hcl:"service,expand"`
+	ServicePrefixes []*ConsulServiceRule `hcl:"service_prefix,expand"`
+}
+
+func (cp *ConsulPolicy) IsEmpty() bool {
+	if cp == nil {
+		return true
+	}
+	return len(cp.Services) == 0 && len(cp.ServicePrefixes) == 0
+}
+
+func ParseConsulPolicy(s string) (*ConsulPolicy, error) {
+	cp := new(ConsulPolicy)
+	if err := hcl.Decode(cp, s); err != nil {
+		return nil, errors.Wrap(err, "failed to parse ACL policy")
+	}
+	if cp.IsEmpty() {
+		// the only use case for now, may as well validate asap
+		return nil, errors.New("consul policy contains no service rules")
+	}
+	return cp, nil
+}
+
+func (c *consulACLsAPI) hasSufficientPolicy(task string, token *api.ACLToken) (bool, error) {
+	// check each policy directly attached to the token
+	for _, policyRef := range token.Policies {
+		if allowable, err := c.policyAllowsServiceWrite(task, policyRef.ID); err != nil {
+			return false, err
+		} else if allowable {
+			return true, nil
+		}
+	}
+
+	// check each policy on each role attached to the token
+	for _, roleLink := range token.Roles {
+		role, _, err := c.aclClient.RoleRead(roleLink.ID, &api.QueryOptions{
+			AllowStale: false,
+		})
+		if err != nil {
+			return false, err
+		}
+
+		for _, policyLink := range role.Policies {
+			allowable, err := c.policyAllowsServiceWrite(task, policyLink.ID)
+			if err != nil {
+				return false, err
+			}
+			if allowable {
+				return true, nil
+			}
+		}
+	}
+
+	return false, nil
+}
+
+// policyAllowsServiceWrite
+func (c *consulACLsAPI) policyAllowsServiceWrite(task string, policyID string) (bool, error) {
+	policy, _, err := c.aclClient.PolicyRead(policyID, &api.QueryOptions{
+		AllowStale: false,
+	})
+	if err != nil {
+		return false, err
+	}
+
+	// compare policy to the necessary permission for service write
+	// e.g. service "db" { policy = "write" }
+	// e.g. service_prefix "" { policy == "write" }
+	cp, err := ParseConsulPolicy(policy.Rules)
+	if err != nil {
+		return false, err
+	}
+
+	if c.allowsServiceWrite(task, cp) {
+		return true, nil
+	}
+
+	return false, nil
+}
+
+const (
+	serviceNameWildcard = "*"
+)
+
+func (_ *consulACLsAPI) allowsServiceWrite(task string, cp *ConsulPolicy) bool {
+	for _, service := range cp.Services {
+		name := strings.ToLower(service.Name)
+		policy := strings.ToLower(service.Policy)
+		if policy == ConsulPolicyWrite {
+			if name == task || name == serviceNameWildcard {
+				return true
+			}
+		}
+	}
+
+	for _, servicePrefix := range cp.ServicePrefixes {
+		prefix := strings.ToLower(servicePrefix.Name)
+		policy := strings.ToLower(servicePrefix.Policy)
+		if policy == ConsulPolicyWrite {
+			if strings.HasPrefix(task, prefix) {
+				return true
+			}
+		}
+	}
+	return false
+}

nomad/consul_policy_test.go

@@ -0,0 +1,203 @@
+package nomad
+
+import (
+	"testing"
+
+	"github.com/hashicorp/consul/api"
+	"github.com/hashicorp/nomad/command/agent/consul"
+	"github.com/hashicorp/nomad/helper/testlog"
+	"github.com/stretchr/testify/require"
+)
+
+func TestConsulPolicy_ParseConsulPolicy(t *testing.T) {
+	t.Parallel()
+
+	try := func(t *testing.T, text string, expPolicy *ConsulPolicy, expErr string) {
+		policy, err := ParseConsulPolicy(text)
+		if expErr != "" {
+			require.EqualError(t, err, expErr)
+			require.True(t, policy.IsEmpty())
+		} else {
+			require.NoError(t, err)
+			require.Equal(t, expPolicy, policy)
+		}
+	}
+
+	t.Run("service", func(t *testing.T) {
+		text := `service "web" { policy = "read" }`
+		exp := &ConsulPolicy{
+			Services:        []*ConsulServiceRule{{Name: "web", Policy: "read"}},
+			ServicePrefixes: []*ConsulServiceRule(nil),
+		}
+		try(t, text, exp, "")
+	})
+
+	t.Run("service_prefix", func(t *testing.T) {
+		text := `service_prefix "data" { policy = "write" }`
+		exp := &ConsulPolicy{
+			Services:        []*ConsulServiceRule(nil),
+			ServicePrefixes: []*ConsulServiceRule{{Name: "data", Policy: "write"}},
+		}
+		try(t, text, exp, "")
+	})
+
+	t.Run("empty", func(t *testing.T) {
+		text := ``
+		expErr := "consul policy contains no service rules"
+		try(t, text, nil, expErr)
+	})
+
+	t.Run("malformed", func(t *testing.T) {
+		text := `this is not valid HCL!`
+		expErr := "failed to parse ACL policy: At 1:22: illegal char"
+		try(t, text, nil, expErr)
+	})
+}
+
+func TestConsulPolicy_IsEmpty(t *testing.T) {
+	t.Parallel()
+
+	try := func(t *testing.T, cp *ConsulPolicy, exp bool) {
+		result := cp.IsEmpty()
+		require.Equal(t, exp, result)
+	}
+
+	t.Run("nil", func(t *testing.T) {
+		cp := (*ConsulPolicy)(nil)
+		try(t, cp, true)
+	})
+
+	t.Run("empty slices", func(t *testing.T) {
+		cp := &ConsulPolicy{
+			Services:        []*ConsulServiceRule(nil),
+			ServicePrefixes: []*ConsulServiceRule(nil),
+		}
+		try(t, cp, true)
+	})
+
+	t.Run("services nonempty", func(t *testing.T) {
+		cp := &ConsulPolicy{
+			Services: []*ConsulServiceRule{{Name: "example", Policy: "write"}},
+		}
+		try(t, cp, false)
+	})
+
+	t.Run("service_prefixes nonempty", func(t *testing.T) {
+		cp := &ConsulPolicy{
+			ServicePrefixes: []*ConsulServiceRule{{Name: "pre", Policy: "read"}},
+		}
+		try(t, cp, false)
+	})
+}
+
+func TestConsulACLsAPI_allowsServiceWrite(t *testing.T) {
+	t.Parallel()
+
+	try := func(t *testing.T, task string, cp *ConsulPolicy, exp bool) {
+		cAPI := new(consulACLsAPI)
+		result := cAPI.allowsServiceWrite(task, cp)
+		require.Equal(t, exp, result)
+	}
+
+	makeCP := func(services [][2]string, prefixes [][2]string) *ConsulPolicy {
+		serviceRules := make([]*ConsulServiceRule, 0, len(services))
+		for _, service := range services {
+			serviceRules = append(serviceRules, &ConsulServiceRule{Name: service[0], Policy: service[1]})
+		}
+		prefixRules := make([]*ConsulServiceRule, 0, len(prefixes))
+		for _, prefix := range prefixes {
+			prefixRules = append(prefixRules, &ConsulServiceRule{Name: prefix[0], Policy: prefix[1]})
+		}
+		return &ConsulPolicy{Services: serviceRules, ServicePrefixes: prefixRules}
+	}
+
+	t.Run("matching service policy write", func(t *testing.T) {
+		try(t, "task1", makeCP(
+			[][2]string{{"task1", "write"}},
+			nil,
+		), true)
+	})
+
+	t.Run("matching service policy read", func(t *testing.T) {
+		try(t, "task1", makeCP(
+			[][2]string{{"task1", "read"}},
+			nil,
+		), false)
+	})
+
+	t.Run("wildcard service policy write", func(t *testing.T) {
+		try(t, "task1", makeCP(
+			[][2]string{{"*", "write"}},
+			nil,
+		), true)
+	})
+
+	t.Run("wrong service policy write", func(t *testing.T) {
+		try(t, "other1", makeCP(
+			[][2]string{{"task1", "write"}},
+			nil,
+		), false)
+	})
+
+	t.Run("matching prefix policy write", func(t *testing.T) {
+		try(t, "task-one", makeCP(
+			nil,
+			[][2]string{{"task-", "write"}},
+		), true)
+	})
+
+	t.Run("matching prefix policy read", func(t *testing.T) {
+		try(t, "task-one", makeCP(
+			nil,
+			[][2]string{{"task-", "read"}},
+		), false)
+	})
+
+	t.Run("empty prefix policy write", func(t *testing.T) {
+		try(t, "task-one", makeCP(
+			nil,
+			[][2]string{{"", "write"}},
+		), true)
+	})
+
+	t.Run("late matching service", func(t *testing.T) {
+		try(t, "task1", makeCP(
+			[][2]string{{"task0", "write"}, {"task1", "write"}},
+			nil,
+		), true)
+	})
+
+	t.Run("late matching prefix", func(t *testing.T) {
+		try(t, "task-one", makeCP(
+			nil,
+			[][2]string{{"foo-", "write"}, {"task-", "write"}},
+		), true)
+	})
+}
+
+func TestConsulACLsAPI_hasSufficientPolicy(t *testing.T) {
+	t.Parallel()
+
+	try := func(t *testing.T, task string, token *api.ACLToken, exp bool) {
+		logger := testlog.HCLogger(t)
+		cAPI := &consulACLsAPI{
+			aclClient: consul.NewMockACLsAPI(logger),
+			logger:    logger,
+		}
+		result, err := cAPI.hasSufficientPolicy(task, token)
+		require.NoError(t, err)
+		require.Equal(t, exp, result)
+	}
+
+	t.Run("no useful policy or role", func(t *testing.T) {
+		try(t, "service1", consul.ExampleOperatorToken0, false)
+	})
+
+	t.Run("working policy only", func(t *testing.T) {
+		try(t, "service1", consul.ExampleOperatorToken1, true)
+	})
+
+	t.Run("working role only", func(t *testing.T) {
+		try(t, "service1", consul.ExampleOperatorToken4, true)
+	})
+}

nomad/consul_test.go

@@ -13,87 +13,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestConsulPolicy_ParseConsulPolicy(t *testing.T) {
-	t.Parallel()
-
-	try := func(t *testing.T, text string, expPolicy *ConsulPolicy, expErr string) {
-		policy, err := ParseConsulPolicy(text)
-		if expErr != "" {
-			require.EqualError(t, err, expErr)
-			require.True(t, policy.IsEmpty())
-		} else {
-			require.NoError(t, err)
-			require.Equal(t, expPolicy, policy)
-		}
-	}
-
-	t.Run("service", func(t *testing.T) {
-		text := `service "web" { policy = "read" }`
-		exp := &ConsulPolicy{
-			Services:        []*ConsulServiceRule{{Name: "web", Policy: "read"}},
-			ServicePrefixes: []*ConsulServiceRule(nil),
-		}
-		try(t, text, exp, "")
-	})
-
-	t.Run("service_prefix", func(t *testing.T) {
-		text := `service_prefix "data" { policy = "write" }`
-		exp := &ConsulPolicy{
-			Services:        []*ConsulServiceRule(nil),
-			ServicePrefixes: []*ConsulServiceRule{{Name: "data", Policy: "write"}},
-		}
-		try(t, text, exp, "")
-	})
-
-	t.Run("empty", func(t *testing.T) {
-		text := ``
-		expErr := "consul policy contains no service rules"
-		try(t, text, nil, expErr)
-	})
-
-	t.Run("malformed", func(t *testing.T) {
-		text := `this is not valid HCL!`
-		expErr := "failed to parse ACL policy: At 1:22: illegal char"
-		try(t, text, nil, expErr)
-	})
-}
-
-func TestConsulPolicy_IsEmpty(t *testing.T) {
-	t.Parallel()
-
-	try := func(t *testing.T, cp *ConsulPolicy, exp bool) {
-		result := cp.IsEmpty()
-		require.Equal(t, exp, result)
-	}
-
-	t.Run("nil", func(t *testing.T) {
-		cp := (*ConsulPolicy)(nil)
-		try(t, cp, true)
-	})
-
-	t.Run("empty slices", func(t *testing.T) {
-		cp := &ConsulPolicy{
-			Services:        []*ConsulServiceRule(nil),
-			ServicePrefixes: []*ConsulServiceRule(nil),
-		}
-		try(t, cp, true)
-	})
-
-	t.Run("services nonempty", func(t *testing.T) {
-		cp := &ConsulPolicy{
-			Services: []*ConsulServiceRule{{Name: "example", Policy: "write"}},
-		}
-		try(t, cp, false)
-	})
-
-	t.Run("service_prefixes nonempty", func(t *testing.T) {
-		cp := &ConsulPolicy{
-			ServicePrefixes: []*ConsulServiceRule{{Name: "pre", Policy: "read"}},
-		}
-		try(t, cp, false)
-	})
-}
-
 var _ ConsulACLsAPI = (*consulACLsAPI)(nil)
 var _ ConsulACLsAPI = (*mockConsulACLsAPI)(nil)
 
@@ -229,15 +148,15 @@ func TestConsulACLsAPI_CheckSIPolicy(t *testing.T) {
 	}
 
 	t.Run("operator has service write", func(t *testing.T) {
-		try(t, "service1", consul.ExampleOperatorToken1, "")
+		try(t, "service1", consul.ExampleOperatorTokenID1, "")
 	})
 
 	t.Run("operator has service_prefix write", func(t *testing.T) {
-		try(t, "foo-service1", consul.ExampleOperatorToken2, "")
+		try(t, "foo-service1", consul.ExampleOperatorTokenID2, "")
 	})
 
 	t.Run("operator permissions insufficient", func(t *testing.T) {
-		try(t, "service1", consul.ExampleOperatorToken3,
+		try(t, "service1", consul.ExampleOperatorTokenID3,
 			"permission denied for \"service1\"",
 		)
 	})
@@ -252,88 +171,3 @@ func TestConsulACLsAPI_CheckSIPolicy(t *testing.T) {
 		)
 	})
 }
-
-func TestConsulACLsAPI_allowsServiceWrite(t *testing.T) {
-	t.Parallel()
-
-	try := func(t *testing.T, task string, cp *ConsulPolicy, exp bool) {
-		cAPI := new(consulACLsAPI)
-		result := cAPI.allowsServiceWrite(task, cp)
-		require.Equal(t, exp, result)
-	}
-
-	makeCP := func(services [][2]string, prefixes [][2]string) *ConsulPolicy {
-		serviceRules := make([]*ConsulServiceRule, 0, len(services))
-		for _, service := range services {
-			serviceRules = append(serviceRules, &ConsulServiceRule{Name: service[0], Policy: service[1]})
-		}
-		prefixRules := make([]*ConsulServiceRule, 0, len(prefixes))
-		for _, prefix := range prefixes {
-			prefixRules = append(prefixRules, &ConsulServiceRule{Name: prefix[0], Policy: prefix[1]})
-		}
-		return &ConsulPolicy{Services: serviceRules, ServicePrefixes: prefixRules}
-	}
-
-	t.Run("matching service policy write", func(t *testing.T) {
-		try(t, "task1", makeCP(
-			[][2]string{{"task1", "write"}},
-			nil,
-		), true)
-	})
-
-	t.Run("matching service policy read", func(t *testing.T) {
-		try(t, "task1", makeCP(
-			[][2]string{{"task1", "read"}},
-			nil,
-		), false)
-	})
-
-	t.Run("wildcard service policy write", func(t *testing.T) {
-		try(t, "task1", makeCP(
-			[][2]string{{"*", "write"}},
-			nil,
-		), true)
-	})
-
-	t.Run("wrong service policy write", func(t *testing.T) {
-		try(t, "other1", makeCP(
-			[][2]string{{"task1", "write"}},
-			nil,
-		), false)
-	})
-
-	t.Run("matching prefix policy write", func(t *testing.T) {
-		try(t, "task-one", makeCP(
-			nil,
-			[][2]string{{"task-", "write"}},
-		), true)
-	})
-
-	t.Run("matching prefix policy read", func(t *testing.T) {
-		try(t, "task-one", makeCP(
-			nil,
-			[][2]string{{"task-", "read"}},
-		), false)
-	})
-
-	t.Run("empty prefix policy write", func(t *testing.T) {
-		try(t, "task-one", makeCP(
-			nil,
-			[][2]string{{"", "write"}},
-		), true)
-	})
-
-	t.Run("late matching service", func(t *testing.T) {
-		try(t, "task1", makeCP(
-			[][2]string{{"task0", "write"}, {"task1", "write"}},
-			nil,
-		), true)
-	})
-
-	t.Run("late matching prefix", func(t *testing.T) {
-		try(t, "task-one", makeCP(
-			nil,
-			[][2]string{{"foo-", "write"}, {"task-", "write"}},
-		), true)
-	})
-}

nomad/job_endpoint_test.go

@@ -320,8 +320,8 @@ func TestJobEndpoint_Register_Connect_AllowUnauthenticatedFalse(t *testing.T) {
 	// Each variation of the provided Consul operator token
 	noOpToken := ""
 	unrecognizedOpToken := uuid.Generate()
-	unauthorizedOpToken := consul.ExampleOperatorToken3
-	authorizedOpToken := consul.ExampleOperatorToken1
+	unauthorizedOpToken := consul.ExampleOperatorTokenID3
+	authorizedOpToken := consul.ExampleOperatorTokenID1
 
 	t.Run("no token provided", func(t *testing.T) {
 		request := newRequest(job)

nomad/node_endpoint.go

@@ -1714,6 +1714,8 @@ func (n *Node) DeriveSIToken(args *structs.DeriveSITokenRequest, reply *structs.
 	}
 
 	// make sure each task in args.Tasks is a connect-enabled task
+	// note: the tasks at this point should be the "connect-sidecar-<id>" name
+	//
 	unneeded := tasksNotUsingConnect(tg, args.Tasks)
 	if len(unneeded) > 0 {
 		setError(fmt.Errorf(
@@ -1751,6 +1753,7 @@ func (n *Node) DeriveSIToken(args *structs.DeriveSITokenRequest, reply *structs.
 					if !ok {
 						return nil
 					}
+
 					sii := ServiceIdentityIndex{
 						ClusterID: clusterID,
 						AllocID:   alloc.ID,