Skip to content

Commit

Permalink
consul: check for acceptable service identity on consul tokens (#15928)
Browse files Browse the repository at this point in the history
When registering a job with a service and 'consul.allow_unauthenticated=false',
we scan the given Consul token for an acceptable policy or role with an
acceptable policy, but did not scan for an acceptable service identity (which
is backed by an acceptable virtual policy). This PR updates our consul token
validation to also accept a matching service identity when registering a service
into Consul.

Fixes #15902
  • Loading branch information
shoenig authored Jan 28, 2023
1 parent 92effde commit ce71d2d
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 4 deletions.
3 changes: 3 additions & 0 deletions .changelog/15928.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
consul: Fixed a bug where acceptable service identity on Consul token was not accepted
```
11 changes: 11 additions & 0 deletions command/agent/consul/acl_testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ const (
ExampleOperatorTokenID3 = "6177d1b9-c0f6-4118-b891-d818a3cb80b1"
ExampleOperatorTokenID4 = "754ae26c-f3cc-e088-d486-9c0d20f5eaea"
ExampleOperatorTokenID5 = "097cbb45-506b-c79c-ec38-82eb0dc0794a"
ExampleOperatorTokenID6 = "6268bd42-6f72-4c90-9c83-90ed6336dcf9"
)

// Example Consul ACL tokens for use in tests that match the policies as the
Expand Down Expand Up @@ -214,6 +215,16 @@ var (
Namespace: "",
}

ExampleOperatorToken6 = &api.ACLToken{
SecretID: ExampleOperatorTokenID6,
AccessorID: "93786935-8856-6e17-0488-c5370a1f044e",
Description: "Operator Token 6",
ServiceIdentities: []*api.ACLServiceIdentity{
{ServiceName: "service1"},
},
Namespace: "",
}

// In Consul namespace "banana"

ExampleOperatorToken10 = &api.ACLToken{
Expand Down
8 changes: 8 additions & 0 deletions nomad/consul_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,14 @@ func (c *consulACLsAPI) canWriteService(namespace, service string, token *api.AC
// treat that like an exact match to preserve backwards compatibility
matches := (namespace == token.Namespace) || (namespace == "" && token.Namespace == "default")

// check each service identity attached to the token -
// the virtual policy for service identities enables service:write
for _, si := range token.ServiceIdentities {
if si.ServiceName == service {
return true, nil
}
}

// check each policy directly attached to the token
for _, policyRef := range token.Policies {
if allowable, err := c.policyAllowsServiceWrite(matches, namespace, service, policyRef.ID); err != nil {
Expand Down
11 changes: 7 additions & 4 deletions nomad/consul_policy_oss_test.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
//go:build !ent
// +build !ent

package nomad

Expand All @@ -10,7 +9,7 @@ import (
"github.com/hashicorp/nomad/ci"
"github.com/hashicorp/nomad/command/agent/consul"
"github.com/hashicorp/nomad/helper/testlog"
"github.com/stretchr/testify/require"
"github.com/shoenig/test/must"
)

func TestConsulACLsAPI_hasSufficientPolicy_oss(t *testing.T) {
Expand All @@ -23,8 +22,8 @@ func TestConsulACLsAPI_hasSufficientPolicy_oss(t *testing.T) {
logger: logger,
}
result, err := cAPI.canWriteService(namespace, task, token)
require.NoError(t, err)
require.Equal(t, exp, result)
must.NoError(t, err)
must.Eq(t, exp, result)
}

// In Nomad OSS, group consul namespace will always be empty string.
Expand All @@ -41,5 +40,9 @@ func TestConsulACLsAPI_hasSufficientPolicy_oss(t *testing.T) {
t.Run("working role only", func(t *testing.T) {
try(t, "", "service1", consul.ExampleOperatorToken4, true)
})

t.Run("working service identity only", func(t *testing.T) {
try(t, "", "service1", consul.ExampleOperatorToken6, true)
})
})
}

0 comments on commit ce71d2d

Please sign in to comment.