Skip to content

Commit

Permalink
[CC-5719] Add support for builtin global-read-only policy
Browse files Browse the repository at this point in the history
  • Loading branch information
jjacobson93 committed Jul 27, 2023
1 parent b7cdd18 commit 8244dc7
Show file tree
Hide file tree
Showing 14 changed files with 263 additions and 113 deletions.
2 changes: 2 additions & 0 deletions acl/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ const (
AnonymousTokenID = "00000000-0000-0000-0000-000000000002"
AnonymousTokenAlias = "anonymous token"
AnonymousTokenSecret = "anonymous"

ReservedBuiltinPrefix = "builtin/"
)

// Config encapsulates all of the generic configuration parameters used for
Expand Down
16 changes: 14 additions & 2 deletions acl/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,21 @@

package acl

import "regexp"
import (
"regexp"
"strings"
)

const (
ServiceIdentityNameMaxLength = 256
NodeIdentityNameMaxLength = 256
PolicyNameMaxLength = 128
)

var (
validServiceIdentityName = regexp.MustCompile(`^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$`)
validNodeIdentityName = regexp.MustCompile(`^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$`)
validPolicyName = regexp.MustCompile(`^[A-Za-z0-9\-_]{1,128}$`)
validPolicyName = regexp.MustCompile(`^[A-Za-z0-9\-_]+/?[A-Za-z0-9\-_]*$`)
validRoleName = regexp.MustCompile(`^[A-Za-z0-9\-_]{1,256}$`)
validAuthMethodName = regexp.MustCompile(`^[A-Za-z0-9\-_]{1,128}$`)
)
Expand Down Expand Up @@ -43,6 +47,14 @@ func IsValidNodeIdentityName(name string) bool {
// IsValidPolicyName returns true if the provided name can be used as an
// ACLPolicy Name.
func IsValidPolicyName(name string) bool {
if len(name) < 1 || len(name) > PolicyNameMaxLength {
return false
}

if strings.HasPrefix(name, "/") || strings.HasPrefix(name, ReservedBuiltinPrefix) {
return false
}

return validPolicyName.MatchString(name)
}

Expand Down
78 changes: 78 additions & 0 deletions acl/validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package acl

import (
"testing"

"github.com/stretchr/testify/require"
)

func Test_IsValidPolicyName(t *testing.T) {
for _, tc := range []struct {
description string
name string
valid bool
}{
{
description: "valid policy",
name: "this-is-valid",
valid: true,
},
{
description: "empty policy",
name: "",
valid: false,
},
{
description: "with slash",
name: "policy/with-slash",
valid: true,
},
{
description: "leading slash",
name: "/no-leading-slash",
valid: false,
},
{
description: "too many slashes",
name: "too/many/slashes",
valid: false,
},
{
description: "no double-slash",
name: "no//double-slash",
valid: false,
},
{
description: "builtin prefix",
name: "builtin/prefix-cannot-be-used",
valid: false,
},
{
description: "long",
name: "this-policy-name-is-very-very-long-but-it-is-okay-because-it-is-the-max-length-that-we-allow-here-in-a-policy-name-which-is-good",
valid: true,
},
{
description: "too long",
name: "this-is-a-policy-that-has-one-character-too-many-it-is-way-too-long-for-a-policy-we-do-not-want-a-policy-of-this-length-because-1",
valid: false,
},
{
description: "invalid start character",
name: "!foo",
valid: false,
},
{
description: "invalid character",
name: "this%is%bad",
valid: false,
},
} {
t.Run(tc.description, func(t *testing.T) {
require.Equal(t, tc.valid, IsValidPolicyName(tc.name))
})
}
}
12 changes: 6 additions & 6 deletions agent/consul/acl_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,7 @@ func (a *ACL) PolicySet(args *structs.ACLPolicySetRequest, reply *structs.ACLPol
}

if !acl.IsValidPolicyName(policy.Name) {
return fmt.Errorf("Invalid Policy: invalid Name. Only alphanumeric characters, '-' and '_' are allowed")
return fmt.Errorf("Invalid Policy: invalid Name. Only alphanumeric characters, a single '/', '-' and '_' are allowed")
}

var idMatch *structs.ACLPolicy
Expand Down Expand Up @@ -915,13 +915,13 @@ func (a *ACL) PolicySet(args *structs.ACLPolicySetRequest, reply *structs.ACLPol
return fmt.Errorf("Invalid Policy: A policy with name %q already exists", policy.Name)
}

if policy.ID == structs.ACLPolicyGlobalManagementID {
if builtinPolicy, ok := structs.ACLBuiltinPolicies[policy.ID]; ok {
if policy.Datacenters != nil || len(policy.Datacenters) > 0 {
return fmt.Errorf("Changing the Datacenters of the builtin global-management policy is not permitted")
return fmt.Errorf("Changing the Datacenters of the builtin %s policy is not permitted", builtinPolicy.Name)
}

if policy.Rules != idMatch.Rules {
return fmt.Errorf("Changing the Rules for the builtin global-management policy is not permitted")
return fmt.Errorf("Changing the Rules for the builtin %s policy is not permitted", builtinPolicy.Name)
}
}
}
Expand Down Expand Up @@ -999,8 +999,8 @@ func (a *ACL) PolicyDelete(args *structs.ACLPolicyDeleteRequest, reply *string)
return fmt.Errorf("policy does not exist: %w", acl.ErrNotFound)
}

if policy.ID == structs.ACLPolicyGlobalManagementID {
return fmt.Errorf("Delete operation not permitted on the builtin global-management policy")
if builtinPolicy, ok := structs.ACLBuiltinPolicies[policy.ID]; ok {
return fmt.Errorf("Delete operation not permitted on the builtin %s policy", builtinPolicy.Name)
}

req := structs.ACLPolicyBatchDeleteRequest{
Expand Down
95 changes: 50 additions & 45 deletions agent/consul/acl_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2183,7 +2183,7 @@ func TestACLEndpoint_PolicySet_CustomID(t *testing.T) {
require.Error(t, err)
}

func TestACLEndpoint_PolicySet_globalManagement(t *testing.T) {
func TestACLEndpoint_PolicySet_builtins(t *testing.T) {
if testing.Short() {
t.Skip("too slow for testing.Short")
}
Expand All @@ -2195,47 +2195,50 @@ func TestACLEndpoint_PolicySet_globalManagement(t *testing.T) {

aclEp := ACL{srv: srv}

// Can't change the rules
{
req := structs.ACLPolicySetRequest{
Datacenter: "dc1",
Policy: structs.ACLPolicy{
ID: structs.ACLPolicyGlobalManagementID,
Name: "foobar", // This is required to get past validation
Rules: "service \"\" { policy = \"write\" }",
},
WriteRequest: structs.WriteRequest{Token: TestDefaultInitialManagementToken},
}
resp := structs.ACLPolicy{}
for _, builtinPolicy := range structs.ACLBuiltinPolicies {
name := fmt.Sprintf("foobar-%s", builtinPolicy.Name) // This is required to get past validation

err := aclEp.PolicySet(&req, &resp)
require.EqualError(t, err, "Changing the Rules for the builtin global-management policy is not permitted")
}
// Can't change the rules
{
req := structs.ACLPolicySetRequest{
Datacenter: "dc1",
Policy: structs.ACLPolicy{
ID: builtinPolicy.ID,
Name: name,
Rules: "service \"\" { policy = \"write\" }",
},
WriteRequest: structs.WriteRequest{Token: TestDefaultInitialManagementToken},
}
resp := structs.ACLPolicy{}

// Can rename it
{
req := structs.ACLPolicySetRequest{
Datacenter: "dc1",
Policy: structs.ACLPolicy{
ID: structs.ACLPolicyGlobalManagementID,
Name: "foobar",
Rules: structs.ACLPolicyGlobalManagement,
},
WriteRequest: structs.WriteRequest{Token: TestDefaultInitialManagementToken},
err := aclEp.PolicySet(&req, &resp)
require.EqualError(t, err, fmt.Sprintf("Changing the Rules for the builtin %s policy is not permitted", builtinPolicy.Name))
}
resp := structs.ACLPolicy{}

err := aclEp.PolicySet(&req, &resp)
require.NoError(t, err)
// Can rename it
{
req := structs.ACLPolicySetRequest{
Datacenter: "dc1",
Policy: structs.ACLPolicy{
ID: builtinPolicy.ID,
Name: name,
Rules: builtinPolicy.Rules,
},
WriteRequest: structs.WriteRequest{Token: TestDefaultInitialManagementToken},
}
resp := structs.ACLPolicy{}

// Get the policy again
policyResp, err := retrieveTestPolicy(codec, TestDefaultInitialManagementToken, "dc1", structs.ACLPolicyGlobalManagementID)
require.NoError(t, err)
policy := policyResp.Policy
err := aclEp.PolicySet(&req, &resp)
require.NoError(t, err)

require.Equal(t, policy.ID, structs.ACLPolicyGlobalManagementID)
require.Equal(t, policy.Name, "foobar")
// Get the policy again
policyResp, err := retrieveTestPolicy(codec, TestDefaultInitialManagementToken, "dc1", builtinPolicy.ID)
require.NoError(t, err)
policy := policyResp.Policy

require.Equal(t, policy.ID, builtinPolicy.ID)
require.Equal(t, policy.Name, name)
}
}
}

Expand Down Expand Up @@ -2271,7 +2274,7 @@ func TestACLEndpoint_PolicyDelete(t *testing.T) {
require.Nil(t, tokenResp.Policy)
}

func TestACLEndpoint_PolicyDelete_globalManagement(t *testing.T) {
func TestACLEndpoint_PolicyDelete_builtins(t *testing.T) {
if testing.Short() {
t.Skip("too slow for testing.Short")
}
Expand All @@ -2282,16 +2285,17 @@ func TestACLEndpoint_PolicyDelete_globalManagement(t *testing.T) {
waitForLeaderEstablishment(t, srv)
aclEp := ACL{srv: srv}

req := structs.ACLPolicyDeleteRequest{
Datacenter: "dc1",
PolicyID: structs.ACLPolicyGlobalManagementID,
WriteRequest: structs.WriteRequest{Token: TestDefaultInitialManagementToken},
}
var resp string

err := aclEp.PolicyDelete(&req, &resp)
for _, builtinPolicy := range structs.ACLBuiltinPolicies {
req := structs.ACLPolicyDeleteRequest{
Datacenter: "dc1",
PolicyID: builtinPolicy.ID,
WriteRequest: structs.WriteRequest{Token: TestDefaultInitialManagementToken},
}
var resp string

require.EqualError(t, err, "Delete operation not permitted on the builtin global-management policy")
err := aclEp.PolicyDelete(&req, &resp)
require.EqualError(t, err, fmt.Sprintf("Delete operation not permitted on the builtin %s policy", builtinPolicy.Name))
}
}

func TestACLEndpoint_PolicyList(t *testing.T) {
Expand Down Expand Up @@ -2324,6 +2328,7 @@ func TestACLEndpoint_PolicyList(t *testing.T) {

policies := []string{
structs.ACLPolicyGlobalManagementID,
structs.ACLPolicyGlobalReadOnlyID,
p1.ID,
p2.ID,
}
Expand Down
2 changes: 1 addition & 1 deletion agent/consul/auth/token_writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func (w *TokenWriter) Delete(secretID string, fromLogout bool) error {

func validateTokenID(id string) error {
if structs.ACLIDReserved(id) {
return fmt.Errorf("UUIDs with the prefix %q are reserved", structs.ACLReservedPrefix)
return fmt.Errorf("UUIDs with the prefix %q are reserved", structs.ACLReservedIDPrefix)
}
if _, err := uuid.ParseUUID(id); err != nil {
return errors.New("not a valid UUID")
Expand Down
4 changes: 2 additions & 2 deletions agent/consul/auth/token_writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestTokenWriter_Create_Validation(t *testing.T) {
errorContains: "not a valid UUID",
},
"AccessorID is reserved": {
token: structs.ACLToken{AccessorID: structs.ACLReservedPrefix + generateID(t)},
token: structs.ACLToken{AccessorID: structs.ACLReservedIDPrefix + generateID(t)},
errorContains: "reserved",
},
"AccessorID already in use (as AccessorID)": {
Expand All @@ -57,7 +57,7 @@ func TestTokenWriter_Create_Validation(t *testing.T) {
errorContains: "not a valid UUID",
},
"SecretID is reserved": {
token: structs.ACLToken{SecretID: structs.ACLReservedPrefix + generateID(t)},
token: structs.ACLToken{SecretID: structs.ACLReservedIDPrefix + generateID(t)},
errorContains: "reserved",
},
"SecretID already in use (as AccessorID)": {
Expand Down
2 changes: 1 addition & 1 deletion agent/consul/fsm/snapshot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
ID: structs.ACLPolicyGlobalManagementID,
Name: "global-management",
Description: "Builtin Policy that grants unlimited access",
Rules: structs.ACLPolicyGlobalManagement,
Rules: structs.ACLPolicyGlobalManagementRules,
}
policy.SetHash(true)
require.NoError(t, fsm.state.ACLPolicySet(1, policy))
Expand Down
Loading

0 comments on commit 8244dc7

Please sign in to comment.