Ensure ServiceName is populated correctly for agent service checks

Also update some snapshot agent docs

* Enforce correct permissions when registering a check

Previously we had attempted to enforce service:write for a check associated with a service instead of node:write on the agent but due to how we decoded the health check from the request it would never do it properly. This commit fixes that.

* Update website/source/docs/commands/snapshot/agent.html.markdown.erb

Co-Authored-By: mkeeler <mkeeler@users.noreply.github.com>

agent/agent_endpoint.go

@@ -588,6 +588,14 @@ func (s *HTTPServer) AgentRegisterCheck(resp http.ResponseWriter, req *http.Requ
 		return nil, nil
 	}
 
+	if health.ServiceID != "" {
+		// fixup the service name so that vetCheckRegister requires the right ACLs
+		service := s.agent.State.Service(health.ServiceID)
+		if service != nil {
+			health.ServiceName = service.Service
+		}
+	}
+
 	// Get the provided token, if any, and vet against any ACL policies.
 	var token string
 	s.parseToken(req, &token)

agent/agent_endpoint_test.go

@@ -1957,28 +1957,127 @@ func TestAgent_RegisterCheck_BadStatus(t *testing.T) {
 
 func TestAgent_RegisterCheck_ACLDeny(t *testing.T) {
 	t.Parallel()
-	a := NewTestAgent(t, t.Name(), TestACLConfig())
+	a := NewTestAgent(t, t.Name(), TestACLConfigNew())
 	defer a.Shutdown()
 	testrpc.WaitForLeader(t, a.RPC, "dc1")
 
-	args := &structs.CheckDefinition{
+	nodeCheck := &structs.CheckDefinition{
 		Name: "test",
 		TTL:  15 * time.Second,
 	}
 
-	t.Run("no token", func(t *testing.T) {
-		req, _ := http.NewRequest("PUT", "/v1/agent/check/register", jsonReader(args))
-		if _, err := a.srv.AgentRegisterCheck(nil, req); !acl.IsErrPermissionDenied(err) {
-			t.Fatalf("err: %v", err)
+	svc := &structs.ServiceDefinition{
+		ID:   "foo:1234",
+		Name: "foo",
+		Port: 1234,
 	}
+
+	svcCheck := &structs.CheckDefinition{
+		Name:      "test2",
+		ServiceID: "foo:1234",
+		TTL:       15 * time.Second,
+	}
+
+	// ensure the service is ready for registering a check for it.
+	req, _ := http.NewRequest("PUT", "/v1/agent/service/register?token=root", jsonReader(svc))
+	resp := httptest.NewRecorder()
+	_, err := a.srv.AgentRegisterService(resp, req)
+	require.NoError(t, err)
+
+	// create a policy that has write on service foo
+	policyReq := &structs.ACLPolicy{
+		Name:  "write-foo",
+		Rules: `service "foo" { policy = "write"}`,
+	}
+
+	req, _ = http.NewRequest("PUT", "/v1/acl/policy?token=root", jsonReader(policyReq))
+	resp = httptest.NewRecorder()
+	_, err = a.srv.ACLPolicyCreate(resp, req)
+	require.NoError(t, err)
+
+	// create a policy that has write on the node name of the agent
+	policyReq = &structs.ACLPolicy{
+		Name:  "write-node",
+		Rules: fmt.Sprintf(`node "%s" { policy = "write" }`, a.config.NodeName),
+	}
+
+	req, _ = http.NewRequest("PUT", "/v1/acl/policy?token=root", jsonReader(policyReq))
+	resp = httptest.NewRecorder()
+	_, err = a.srv.ACLPolicyCreate(resp, req)
+	require.NoError(t, err)
+
+	// create a token using the write-foo policy
+	tokenReq := &structs.ACLToken{
+		Description: "write-foo",
+		Policies: []structs.ACLTokenPolicyLink{
+			structs.ACLTokenPolicyLink{
+				Name: "write-foo",
+			},
+		},
+	}
+
+	req, _ = http.NewRequest("PUT", "/v1/acl/token?token=root", jsonReader(tokenReq))
+	resp = httptest.NewRecorder()
+	tokInf, err := a.srv.ACLTokenCreate(resp, req)
+	require.NoError(t, err)
+	svcToken, ok := tokInf.(*structs.ACLToken)
+	require.True(t, ok)
+	require.NotNil(t, svcToken)
+
+	// create a token using the write-node policy
+	tokenReq = &structs.ACLToken{
+		Description: "write-node",
+		Policies: []structs.ACLTokenPolicyLink{
+			structs.ACLTokenPolicyLink{
+				Name: "write-node",
+			},
+		},
+	}
+
+	req, _ = http.NewRequest("PUT", "/v1/acl/token?token=root", jsonReader(tokenReq))
+	resp = httptest.NewRecorder()
+	tokInf, err = a.srv.ACLTokenCreate(resp, req)
+	require.NoError(t, err)
+	nodeToken, ok := tokInf.(*structs.ACLToken)
+	require.True(t, ok)
+	require.NotNil(t, nodeToken)
+
+	t.Run("no token - node check", func(t *testing.T) {
+		req, _ := http.NewRequest("PUT", "/v1/agent/check/register", jsonReader(nodeCheck))
+		_, err := a.srv.AgentRegisterCheck(nil, req)
+		require.True(t, acl.IsErrPermissionDenied(err))
 	})
 
-	t.Run("root token", func(t *testing.T) {
-		req, _ := http.NewRequest("PUT", "/v1/agent/check/register?token=root", jsonReader(args))
-		if _, err := a.srv.AgentRegisterCheck(nil, req); err != nil {
-			t.Fatalf("err: %v", err)
-		}
+	t.Run("svc token - node check", func(t *testing.T) {
+		req, _ := http.NewRequest("PUT", "/v1/agent/check/register?token="+svcToken.SecretID, jsonReader(nodeCheck))
+		_, err := a.srv.AgentRegisterCheck(nil, req)
+		require.True(t, acl.IsErrPermissionDenied(err))
 	})
+
+	t.Run("node token - node check", func(t *testing.T) {
+		req, _ := http.NewRequest("PUT", "/v1/agent/check/register?token="+nodeToken.SecretID, jsonReader(nodeCheck))
+		_, err := a.srv.AgentRegisterCheck(nil, req)
+		require.NoError(t, err)
+	})
+
+	t.Run("no token - svc check", func(t *testing.T) {
+		req, _ := http.NewRequest("PUT", "/v1/agent/check/register", jsonReader(svcCheck))
+		_, err := a.srv.AgentRegisterCheck(nil, req)
+		require.True(t, acl.IsErrPermissionDenied(err))
+	})
+
+	t.Run("node token - svc check", func(t *testing.T) {
+		req, _ := http.NewRequest("PUT", "/v1/agent/check/register?token="+nodeToken.SecretID, jsonReader(svcCheck))
+		_, err := a.srv.AgentRegisterCheck(nil, req)
+		require.True(t, acl.IsErrPermissionDenied(err))
+	})
+
+	t.Run("svc token - svc check", func(t *testing.T) {
+		req, _ := http.NewRequest("PUT", "/v1/agent/check/register?token="+svcToken.SecretID, jsonReader(svcCheck))
+		_, err := a.srv.AgentRegisterCheck(nil, req)
+		require.NoError(t, err)
+	})
+
 }
 
 func TestAgent_DeregisterCheck(t *testing.T) {

agent/testagent.go

@@ -398,3 +398,18 @@ func TestACLConfig() string {
 		acl_enforce_version_8 = true
 	`
 }
+
+func TestACLConfigNew() string {
+	return `
+		primary_datacenter = "dc1"
+		acl {
+			enabled = true
+			default_policy = "deny"
+			tokens {
+				master = "root"
+				agent = "root"
+				agent_master = "towel"
+			}
+		}
+	`
+}

website/source/docs/commands/snapshot/agent.html.markdown.erb

@@ -51,8 +51,14 @@ Snapshots can be restored using the
 [`consul snapshot restore`](/docs/commands/snapshot/restore.html) command, or
 the [HTTP API](/api/snapshot.html).
 
-If ACLs are enabled, a management token must be supplied in order to perform
-snapshot operations.
+If ACLs are enabled the following privileges are required:
+
+| Resource  | Segment          | Permission | Explanation |
+| --------- | ---------------- | ---------- | ----------- |
+| `acl`     | N/A              | `write`    | All snapshotting operations require this privilege due to snapshots containing ACL tokens including unredacted secrets. |
+| `key`     | `<lock key>`     | `write`    | The lock key (which defaults to `consul-snapshot/lock`) is used during snapshot agent leader election. |
+| `session` | `<agent name>`   | `write`    | The session used for locking during leader election is created against the agent name of the Consul agent that the Snapshot agent is registering itself with. |
+| `service` | `<service name>` | `write`    | The Snapshot agent registers itself with the local Consul agent and must have write privileges on its service name which is configured with `-service`. |
 
 ## Usage