From a4f792641b04da2f5e3243be117448cf017236ed Mon Sep 17 00:00:00 2001
From: Tim Gross <tgross@hashicorp.com>
Date: Fri, 3 Mar 2023 11:41:19 -0500
Subject: [PATCH] service: fix regression in task access to list/read endpoint
 (#16316)

When native service discovery was added, we used the node secret as the auth
token. Once Workload Identity was added in Nomad 1.4.x we needed to use the
claim token for `template` blocks, and so we allowed valid claims to bypass the
ACL policy check to preserve the existing behavior. (Invalid claims are still
rejected, so this didn't widen any security boundary.)

In reworking authentication for 1.5.0, we unintentionally removed this
bypass. For WIs without a policy attached to their job, everything works as
expected because the resulting `acl.ACL` is nil. But once a policy is attached
to the job the `acl.ACL` is no longer nil and this causes permissions errors.

Fix the regression by adding back the bypass for valid claims. In future work,
we should strongly consider getting turning the implicit policies into real
`ACLPolicy` objects (even if not stored in state) so that we don't have these
kind of brittle exceptions to the auth code.
---
 .changelog/16316.txt                        |  3 +++
 nomad/service_registration_endpoint.go      |  6 ++++--
 nomad/service_registration_endpoint_test.go | 18 +++++++++++++++++-
 3 files changed, 24 insertions(+), 3 deletions(-)
 create mode 100644 .changelog/16316.txt

diff --git a/.changelog/16316.txt b/.changelog/16316.txt
new file mode 100644
index 000000000..cd297f5b0
--- /dev/null
+++ b/.changelog/16316.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+service: Fixed a bug where attaching a policy to a job would prevent workload identities for the job from reading the service registration API
+```
diff --git a/nomad/service_registration_endpoint.go b/nomad/service_registration_endpoint.go
index df8014216..23570f26a 100644
--- a/nomad/service_registration_endpoint.go
+++ b/nomad/service_registration_endpoint.go
@@ -217,7 +217,8 @@ func (s *ServiceRegistration) List(
 	if err != nil {
 		return structs.ErrPermissionDenied
 	}
-	if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) {
+	if args.GetIdentity().Claims == nil &&
+		!aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) {
 		return structs.ErrPermissionDenied
 	}
 
@@ -381,7 +382,8 @@ func (s *ServiceRegistration) GetService(
 	if err != nil {
 		return structs.ErrPermissionDenied
 	}
-	if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) {
+	if args.GetIdentity().Claims == nil &&
+		!aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) {
 		return structs.ErrPermissionDenied
 	}
 
diff --git a/nomad/service_registration_endpoint_test.go b/nomad/service_registration_endpoint_test.go
index 9d198e0cb..448e68377 100644
--- a/nomad/service_registration_endpoint_test.go
+++ b/nomad/service_registration_endpoint_test.go
@@ -5,7 +5,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/go-memdb"
-	"github.com/hashicorp/net-rpc-msgpackrpc"
+	msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
 	"github.com/hashicorp/nomad/acl"
 	"github.com/hashicorp/nomad/ci"
 	"github.com/hashicorp/nomad/nomad/mock"
@@ -882,12 +882,28 @@ func TestServiceRegistration_List(t *testing.T) {
 				// Generate an allocation with a signed identity
 				allocs := []*structs.Allocation{mock.Alloc()}
 				job := allocs[0].Job
+				job.Namespace = "platform"
+				allocs[0].Namespace = "platform"
 				require.NoError(t, s.State().UpsertJob(structs.MsgTypeTestSetup, 10, job))
 				s.signAllocIdentities(job, allocs)
 				require.NoError(t, s.State().UpsertAllocs(structs.MsgTypeTestSetup, 15, allocs))
 
 				signedToken := allocs[0].SignedIdentities["web"]
 
+				// Associate an unrelated policy with the identity's job to
+				// ensure it doesn't conflict.
+				policy := &structs.ACLPolicy{
+					Name:  "policy-for-identity",
+					Rules: mock.NodePolicy("read"),
+					JobACL: &structs.JobACL{
+						Namespace: "platform",
+						JobID:     job.ID,
+					},
+				}
+				policy.SetHash()
+				must.NoError(t, s.State().UpsertACLPolicies(structs.MsgTypeTestSetup, 16,
+					[]*structs.ACLPolicy{policy}))
+
 				// Generate and upsert some service registrations.
 				services := mock.ServiceRegistrations()
 				require.NoError(t, s.State().UpsertServiceRegistrations(