From 65ef8548800b4cf8077868490209ca66d7b2e582 Mon Sep 17 00:00:00 2001
From: Seth Hoenig <shoenig@hashicorp.com>
Date: Thu, 27 Aug 2020 11:53:41 -0500
Subject: [PATCH] consul/connect: make use of task kind to determine service
 name in consul token checks

When consul.allow_unauthenticated is set to false, the job_endpoint hook validates
that a `-consul-token` is provided and validates the token against the privileges
inherent to a Consul Service Identity policy for all the Connect enabled services
defined in the job.

Before, the check was assuming the service was of type sidecar-proxy. This fixes the
check to use the type of the task so we can distinguish between the different connect
types.
---
 nomad/job_endpoint.go         | 15 +++++++--------
 nomad/structs/structs.go      | 15 +++++++--------
 nomad/structs/structs_test.go | 29 +++++++++++++++++++++++------
 3 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go
index f57b98036..ba96c688b 100644
--- a/nomad/job_endpoint.go
+++ b/nomad/job_endpoint.go
@@ -252,15 +252,16 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis
 
 	// helper function that checks if the "operator token" supplied with the
 	// job has sufficient ACL permissions for establishing consul connect services
-	checkOperatorToken := func(task string) error {
+	checkOperatorToken := func(kind structs.TaskKind) error {
 		if j.srv.config.ConsulConfig.AllowsUnauthenticated() {
 			// if consul.allow_unauthenticated is enabled (which is the default)
 			// just let the Job through without checking anything.
 			return nil
 		}
-		proxiedTask := strings.TrimPrefix(task, structs.ConnectProxyPrefix+"-")
+
+		service := kind.Value()
 		ctx := context.Background()
-		if err := j.srv.consulACLs.CheckSIPolicy(ctx, proxiedTask, args.Job.ConsulToken); err != nil {
+		if err := j.srv.consulACLs.CheckSIPolicy(ctx, service, args.Job.ConsulToken); err != nil {
 			// not much in the way of exported error types, we could parse
 			// the content, but all errors are going to be failures anyway
 			return errors.Wrap(err, "operator token denied")
@@ -269,11 +270,9 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis
 	}
 
 	// Enforce that the operator has necessary Consul ACL permissions
-	for _, tg := range args.Job.ConnectTasks() {
-		for _, task := range tg {
-			if err := checkOperatorToken(task); err != nil {
-				return err
-			}
+	for _, taskKind := range args.Job.ConnectTasks() {
+		if err := checkOperatorToken(taskKind); err != nil {
+			return err
 		}
 	}
 
diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go
index b2bffd908..493aba30c 100644
--- a/nomad/structs/structs.go
+++ b/nomad/structs/structs.go
@@ -4230,25 +4230,24 @@ func (j *Job) VaultPolicies() map[string]map[string]*Vault {
 	return policies
 }
 
-// Connect tasks returns the set of Consul Connect enabled tasks that will
-// require a Service Identity token, if Consul ACLs are enabled.
+// ConnectTasks returns the set of Consul Connect enabled tasks defined on the
+// job that will require a Service Identity token in the case that Consul ACLs
+// are enabled. The TaskKind.Value is the name of the Consul service.
 //
 // This method is meaningful only after the Job has passed through the job
 // submission Mutator functions.
-//
-// task group -> []task
-func (j *Job) ConnectTasks() map[string][]string {
-	m := make(map[string][]string)
+func (j *Job) ConnectTasks() []TaskKind {
+	var kinds []TaskKind
 	for _, tg := range j.TaskGroups {
 		for _, task := range tg.Tasks {
 			if task.Kind.IsConnectProxy() ||
 				task.Kind.IsConnectNative() ||
 				task.Kind.IsAnyConnectGateway() {
-				m[tg.Name] = append(m[tg.Name], task.Name)
+				kinds = append(kinds, task.Kind)
 			}
 		}
 	}
-	return m
+	return kinds
 }
 
 // ConfigEntries accumulates the Consul Configuration Entries defined in task groups
diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go
index be3bee582..9ea2c8636 100644
--- a/nomad/structs/structs_test.go
+++ b/nomad/structs/structs_test.go
@@ -608,9 +608,6 @@ func TestJob_ConnectTasks(t *testing.T) {
 	t.Parallel()
 	r := require.New(t)
 
-	// todo(shoenig): this will need some updates when we support connect native
-	//  tasks, which will have a different Kind format, probably.
-
 	j0 := &Job{
 		TaskGroups: []*TaskGroup{{
 			Name: "tg1",
@@ -633,15 +630,35 @@ func TestJob_ConnectTasks(t *testing.T) {
 				Name: "connect-proxy-task2",
 				Kind: "connect-proxy:task2",
 			}},
+		}, {
+			Name: "tg3",
+			Tasks: []*Task{{
+				Name: "ingress",
+				Kind: "connect-ingress:ingress",
+			}},
+		}, {
+			Name: "tg4",
+			Tasks: []*Task{{
+				Name: "frontend",
+				Kind: "connect-native:uuid-fe",
+			}, {
+				Name: "generator",
+				Kind: "connect-native:uuid-api",
+			}},
 		}},
 	}
 
 	connectTasks := j0.ConnectTasks()
 
-	exp := map[string][]string{
-		"tg1": {"connect-proxy-task1", "connect-proxy-task3"},
-		"tg2": {"connect-proxy-task2"},
+	exp := []TaskKind{
+		NewTaskKind(ConnectProxyPrefix, "task1"),
+		NewTaskKind(ConnectProxyPrefix, "task3"),
+		NewTaskKind(ConnectProxyPrefix, "task2"),
+		NewTaskKind(ConnectIngressPrefix, "ingress"),
+		NewTaskKind(ConnectNativePrefix, "uuid-fe"),
+		NewTaskKind(ConnectNativePrefix, "uuid-api"),
 	}
+
 	r.Equal(exp, connectTasks)
 }