Skip to content

Commit

Permalink
consul: correctly check consul acl token namespace when using consul oss
Browse files Browse the repository at this point in the history
This PR fixes the Nomad Object Namespace <-> Consul ACL Token relationship
check when using Consul OSS (or Consul ENT without namespace support).

Nomad v1.1.0 introduced a regression where Nomad would fail the validation
when submitting Connect jobs and allow_unauthenticated set to true, with
Consul OSS - because it would do the namespace check against the Consul ACL
token assuming the "default" namespace, which does not work because Consul OSS
does not have namespaces.

Instead of making the bad assumption, expand the namespace check to handle
each special case explicitly.

Fixes #10718
  • Loading branch information
shoenig committed Jun 8, 2021
1 parent 5f968ba commit 09c9a17
Show file tree
Hide file tree
Showing 9 changed files with 681 additions and 353 deletions.
65 changes: 51 additions & 14 deletions command/agent/consul/acl_testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ const (
)

// Example Consul ACL tokens for use in tests that match the policies as the
// tokens above, but these belong to the "banana' Consul namespace.
// tokens above, but these belong to the "banana" Consul namespace.
const (
ExampleOperatorTokenID10 = "ddfe688f-655f-e8dd-1db5-5650eed00aeb"
ExampleOperatorTokenID11 = "46d09394-598c-1e55-b7fd-64cd2f409707"
Expand All @@ -141,14 +141,21 @@ const (
ExampleOperatorTokenID15 = "e9db1754-c075-d0fc-0a7e-de1e9e7bff98"
)

// Example Consul ACL tokens for use in tests that match the policies as the
// tokens above, but these belong to the "default" Consul namespace.
const (
ExampleOperatorTokenID20 = "937b3287-557c-5af8-beb0-d62191988719"
ExampleOperatorTokenID21 = "067fd927-abfb-d98f-b693-bb05dccea565"
)

var (
// In Consul namespace "default"
// In no Consul namespace (OSS, ENT w/o Namespaces)

ExampleOperatorToken0 = &api.ACLToken{
SecretID: ExampleOperatorTokenID0,
AccessorID: "228865c6-3bf6-6683-df03-06dea2779088 ",
Description: "Operator Token 0",
Namespace: "default",
Namespace: "",
}

ExampleOperatorToken1 = &api.ACLToken{
Expand All @@ -158,7 +165,7 @@ var (
Policies: []*api.ACLTokenPolicyLink{{
ID: ExamplePolicyID1,
}},
Namespace: "default",
Namespace: "",
}

ExampleOperatorToken2 = &api.ACLToken{
Expand All @@ -168,7 +175,7 @@ var (
Policies: []*api.ACLTokenPolicyLink{{
ID: ExamplePolicyID2,
}},
Namespace: "default",
Namespace: "",
}

ExampleOperatorToken3 = &api.ACLToken{
Expand All @@ -178,7 +185,7 @@ var (
Policies: []*api.ACLTokenPolicyLink{{
ID: ExamplePolicyID3,
}},
Namespace: "default",
Namespace: "",
}

ExampleOperatorToken4 = &api.ACLToken{
Expand All @@ -190,7 +197,7 @@ var (
ID: ExampleRoleID1,
Name: "example-role-1",
}},
Namespace: "default",
Namespace: "",
}

ExampleOperatorToken5 = &api.ACLToken{
Expand All @@ -200,20 +207,20 @@ var (
Policies: []*api.ACLTokenPolicyLink{{
ID: ExamplePolicyID4,
}},
Namespace: "default",
Namespace: "",
}

// In Consul namespace "banana"

ExampleOperatorToken10 = &api.ACLToken{
SecretID: ExampleOperatorTokenID0,
SecretID: ExampleOperatorTokenID10,
AccessorID: "76a2c3b5-5d64-9089-f701-660eec2d3554",
Description: "Operator Token 0",
Namespace: "banana",
}

ExampleOperatorToken11 = &api.ACLToken{
SecretID: ExampleOperatorTokenID1,
SecretID: ExampleOperatorTokenID11,
AccessorID: "40f2a36a-0a65-1972-106c-b2e5dd46d6e8",
Description: "Operator Token 1",
Policies: []*api.ACLTokenPolicyLink{{
Expand All @@ -223,7 +230,7 @@ var (
}

ExampleOperatorToken12 = &api.ACLToken{
SecretID: ExampleOperatorTokenID2,
SecretID: ExampleOperatorTokenID12,
AccessorID: "894f2c5c-b285-71bf-4acb-6344cecf71f3",
Description: "Operator Token 2",
Policies: []*api.ACLTokenPolicyLink{{
Expand All @@ -233,7 +240,7 @@ var (
}

ExampleOperatorToken13 = &api.ACLToken{
SecretID: ExampleOperatorTokenID3,
SecretID: ExampleOperatorTokenID13,
AccessorID: "2a81ec0b-692e-845e-f5b8-c33c05e5af22",
Description: "Operator Token 3",
Policies: []*api.ACLTokenPolicyLink{{
Expand All @@ -243,7 +250,7 @@ var (
}

ExampleOperatorToken14 = &api.ACLToken{
SecretID: ExampleOperatorTokenID4,
SecretID: ExampleOperatorTokenID14,
AccessorID: "4273f1cc-5626-7a77-dc65-1f24af035ed5d",
Description: "Operator Token 4",
Policies: nil, // no direct policy, only roles
Expand All @@ -255,14 +262,35 @@ var (
}

ExampleOperatorToken15 = &api.ACLToken{
SecretID: ExampleOperatorTokenID5,
SecretID: ExampleOperatorTokenID15,
AccessorID: "5b78e186-87d8-c1ad-966f-f5fa87b05c9a",
Description: "Operator Token 5",
Policies: []*api.ACLTokenPolicyLink{{
ID: ExamplePolicyID4,
}},
Namespace: "banana",
}

// In Consul namespace "default"

ExampleOperatorToken20 = &api.ACLToken{
SecretID: ExampleOperatorTokenID20,
AccessorID: "228865c6-3bf6-6683-df03-06dea2779088",
Description: "Operator Token 0",
// Should still be able to register jobs where no namespace was set
Namespace: "default",
}

ExampleOperatorToken21 = &api.ACLToken{
SecretID: ExampleOperatorTokenID21,
AccessorID: "54d01af9-5036-31d3-296b-b15b941d7aa2",
Description: "Operator Token 1",
Policies: []*api.ACLTokenPolicyLink{{
ID: ExamplePolicyID1,
}},
// Should still be able to register jobs where no namespace was set
Namespace: "default",
}
)

func (m *MockACLsAPI) TokenReadSelf(q *api.QueryOptions) (*api.ACLToken, *api.QueryMeta, error) {
Expand All @@ -283,6 +311,9 @@ func (m *MockACLsAPI) TokenReadSelf(q *api.QueryOptions) (*api.ACLToken, *api.Qu
case ExampleOperatorTokenID5:
return ExampleOperatorToken5, nil, nil

case ExampleOperatorTokenID10:
return ExampleOperatorToken10, nil, nil

case ExampleOperatorTokenID11:
return ExampleOperatorToken11, nil, nil

Expand All @@ -298,6 +329,12 @@ func (m *MockACLsAPI) TokenReadSelf(q *api.QueryOptions) (*api.ACLToken, *api.Qu
case ExampleOperatorTokenID15:
return ExampleOperatorToken15, nil, nil

case ExampleOperatorTokenID20:
return ExampleOperatorToken20, nil, nil

case ExampleOperatorTokenID21:
return ExampleOperatorToken21, nil, nil

default:
return nil, nil, errors.New("no such token")
}
Expand Down
7 changes: 0 additions & 7 deletions nomad/consul.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,6 @@ func (c *consulACLsAPI) CheckPermissions(ctx context.Context, namespace string,
return nil
}

// If namespace is not declared on nomad jobs, assume default consul namespace
// when comparing with the consul ACL token. This maintains backwards compatibility
// with existing connect jobs, which may already be authorized with Consul tokens.
if namespace == "" {
namespace = "default"
}

// lookup the token from consul
token, readErr := c.readToken(ctx, secretID)
if readErr != nil {
Expand Down
123 changes: 123 additions & 0 deletions nomad/consul_oss_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
//+build !ent

package nomad

import (
"context"
"errors"
"testing"

"github.com/hashicorp/nomad/command/agent/consul"
"github.com/hashicorp/nomad/helper/testlog"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/stretchr/testify/require"
)

func TestConsulACLsAPI_CheckPermissions_oss(t *testing.T) {

// In Nomad OSS, CheckPermissions will only receive "" as input for the
// namespace parameter - as the ConsulUsage map from namespace to usages will
// always contain one key - the empty string.

t.Parallel()

try := func(t *testing.T, namespace string, usage *structs.ConsulUsage, secretID string, exp error) {
logger := testlog.HCLogger(t)
aclAPI := consul.NewMockACLsAPI(logger)
cAPI := NewConsulACLsAPI(aclAPI, logger, nil)

err := cAPI.CheckPermissions(context.Background(), namespace, usage, secretID)
if exp == nil {
require.NoError(t, err)
} else {
require.Equal(t, exp.Error(), err.Error())
}
}

t.Run("check-permissions kv read", func(t *testing.T) {
t.Run("uses kv has permission", func(t *testing.T) {
u := &structs.ConsulUsage{KV: true}
try(t, "", u, consul.ExampleOperatorTokenID5, nil)
})

t.Run("uses kv without permission", func(t *testing.T) {
u := &structs.ConsulUsage{KV: true}
try(t, "", u, consul.ExampleOperatorTokenID1, errors.New("insufficient Consul ACL permissions to use template"))
})

t.Run("uses kv no token", func(t *testing.T) {
u := &structs.ConsulUsage{KV: true}
try(t, "", u, "", errors.New("missing consul token"))
})

t.Run("uses kv nonsense token", func(t *testing.T) {
u := &structs.ConsulUsage{KV: true}
try(t, "", u, "47d33e22-720a-7fe6-7d7f-418bf844a0be", errors.New("unable to read consul token: no such token"))
})

t.Run("no kv no token", func(t *testing.T) {
u := &structs.ConsulUsage{KV: false}
try(t, "", u, "", nil)
})
})

t.Run("check-permissions service write", func(t *testing.T) {
usage := &structs.ConsulUsage{Services: []string{"service1"}}

t.Run("operator has service write", func(t *testing.T) {
try(t, "", usage, consul.ExampleOperatorTokenID1, nil)
})

t.Run("operator has service_prefix write", func(t *testing.T) {
u := &structs.ConsulUsage{Services: []string{"foo-service1"}}
try(t, "", u, consul.ExampleOperatorTokenID2, nil)
})

t.Run("operator has service_prefix write wrong prefix", func(t *testing.T) {
u := &structs.ConsulUsage{Services: []string{"bar-service1"}}
try(t, "", u, consul.ExampleOperatorTokenID2, errors.New(`insufficient Consul ACL permissions to write service "bar-service1"`))
})

t.Run("operator permissions insufficient", func(t *testing.T) {
try(t, "", usage, consul.ExampleOperatorTokenID3, errors.New(`insufficient Consul ACL permissions to write service "service1"`))
})

t.Run("operator provided no token", func(t *testing.T) {
try(t, "", usage, "", errors.New("missing consul token"))
})

t.Run("operator provided nonsense token", func(t *testing.T) {
try(t, "", usage, "f1682bde-1e71-90b1-9204-85d35467ba61", errors.New("unable to read consul token: no such token"))
})
})

t.Run("check-permissions connect service identity write", func(t *testing.T) {
usage := &structs.ConsulUsage{Kinds: []structs.TaskKind{structs.NewTaskKind(structs.ConnectProxyPrefix, "service1")}}

t.Run("operator has service write", func(t *testing.T) {
try(t, "", usage, consul.ExampleOperatorTokenID1, nil)
})

t.Run("operator has service_prefix write", func(t *testing.T) {
u := &structs.ConsulUsage{Kinds: []structs.TaskKind{structs.NewTaskKind(structs.ConnectProxyPrefix, "foo-service1")}}
try(t, "", u, consul.ExampleOperatorTokenID2, nil)
})

t.Run("operator has service_prefix write wrong prefix", func(t *testing.T) {
u := &structs.ConsulUsage{Kinds: []structs.TaskKind{structs.NewTaskKind(structs.ConnectProxyPrefix, "bar-service1")}}
try(t, "", u, consul.ExampleOperatorTokenID2, errors.New(`insufficient Consul ACL permissions to write Connect service "bar-service1"`))
})

t.Run("operator permissions insufficient", func(t *testing.T) {
try(t, "", usage, consul.ExampleOperatorTokenID3, errors.New(`insufficient Consul ACL permissions to write Connect service "service1"`))
})

t.Run("operator provided no token", func(t *testing.T) {
try(t, "", usage, "", errors.New("missing consul token"))
})

t.Run("operator provided nonsense token", func(t *testing.T) {
try(t, "", usage, "f1682bde-1e71-90b1-9204-85d35467ba61", errors.New("unable to read consul token: no such token"))
})
})
}
46 changes: 39 additions & 7 deletions nomad/consul_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,42 @@ func (c *consulACLsAPI) isManagementToken(token *api.ACLToken) bool {
return false
}

// namespaceCheck is used to verify the namespace of the object matches the
// namespace of the ACL token provided.
// namespaceCheck is used to fail the request if the namespace of the object does
// not match the namespace of the ACL token provided.
//
// exception: iff token is in the default namespace, it may contain policies
// *exception*: if token is in the default namespace, it may contain policies
// that extend into other namespaces using namespace_prefix, which must bypass
// this early check and validate in the service/keystore helpers
//
// *exception*: if token is not in a namespace, consul namespaces are not enabled
// and there is nothing to validate
//
// If the namespaces match, whether the token is allowed to perform an operation
// is checked later.
func namespaceCheck(namespace string, token *api.ACLToken) error {
if token.Namespace != "default" && token.Namespace != namespace {

switch {
case namespace == token.Namespace:
// ACLs enabled, namespaces are the same
return nil

case token.Namespace == "default":
// ACLs enabled, must defer to per-object checking, since the token could
// have namespace or namespace_prefix blocks with extended policies that
// allow an operation. Using namespace or namespace_prefix blocks is only
// applicable to tokens in the "default" namespace.
//
// https://www.consul.io/docs/security/acl/acl-rules#namespace-rules
return nil

case namespace == "" && token.Namespace != "default":
// ACLs enabled with non-default token, but namespace on job not set, so
// provide a more informative error message.
return errors.Errorf("consul ACL token requires using namespace %q", token.Namespace)

default:
return errors.Errorf("consul ACL token cannot use namespace %q", namespace)
}
return nil
}

func (c *consulACLsAPI) canReadKeystore(namespace string, token *api.ACLToken) (bool, error) {
Expand All @@ -87,7 +112,10 @@ func (c *consulACLsAPI) canReadKeystore(namespace string, token *api.ACLToken) (
}

// determines whether a top-level ACL policy will be applicable
matches := namespace == token.Namespace
//
// if the namespace is not set in the job and the token is in the default namespace,
// treat that like an exact match to preserve backwards compatibility
matches := (namespace == token.Namespace) || (namespace == "" && token.Namespace == "default")

// check each policy directly attached to the token
for _, policyRef := range token.Policies {
Expand Down Expand Up @@ -127,7 +155,10 @@ func (c *consulACLsAPI) canWriteService(namespace, service string, token *api.AC
}

// determines whether a top-level ACL policy will be applicable
matches := namespace == token.Namespace
//
// if the namespace is not set in the job and the token is in the default namespace,
// treat that like an exact match to preserve backwards compatibility
matches := (namespace == token.Namespace) || (namespace == "" && token.Namespace == "default")

// check each policy directly attached to the token
for _, policyRef := range token.Policies {
Expand Down Expand Up @@ -179,6 +210,7 @@ func (c *consulACLsAPI) policyAllowsServiceWrite(matches bool, namespace, servic
if cp.allowsServiceWrite(matches, namespace, service) {
return true, nil
}

return false, nil
}

Expand Down
Loading

0 comments on commit 09c9a17

Please sign in to comment.