Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CC-5719] Add support for builtin global-read-only policy #18319

Merged
merged 13 commits into from
Aug 1, 2023
4 changes: 4 additions & 0 deletions .changelog/18319.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
```release-note:improvement
acl: added builtin ACL policy that provides global read-only access (builtin/global-read-only)
acl: allow for a single slash character in policy names
```
jjacobson93 marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions acl/acl.go
Original file line number Diff line number Diff line change
@@ -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
28 changes: 22 additions & 6 deletions acl/validation.go
Original file line number Diff line number Diff line change
@@ -3,17 +3,22 @@

package acl

import "regexp"
import (
"fmt"
"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}$`)
)
@@ -40,10 +45,21 @@ func IsValidNodeIdentityName(name string) bool {
return validNodeIdentityName.MatchString(name)
}

// IsValidPolicyName returns true if the provided name can be used as an
// ACLPolicy Name.
func IsValidPolicyName(name string) bool {
return validPolicyName.MatchString(name)
// IsValidPolicyName returns nil if the provided name can be used as an
// ACLPolicy Name otherwise a useful error is returned.
func IsValidPolicyName(name string) error {
jjacobson93 marked this conversation as resolved.
Show resolved Hide resolved
if len(name) < 1 || len(name) > PolicyNameMaxLength {
return fmt.Errorf("Invalid Policy: invalid Name. Length must be greater than 0 and less than %d", PolicyNameMaxLength)
}

if strings.HasPrefix(name, "/") || strings.HasPrefix(name, ReservedBuiltinPrefix) {
return fmt.Errorf("Invalid Policy: invalid Name. Names cannot be prefixed with '/' or 'builtin/'")
jjacobson93 marked this conversation as resolved.
Show resolved Hide resolved
}

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

// IsValidRoleName returns true if the provided name can be used as an
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) {
jjacobson93 marked this conversation as resolved.
Show resolved Hide resolved
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) == nil)
})
}
}
4 changes: 2 additions & 2 deletions agent/acl_endpoint_test.go
Original file line number Diff line number Diff line change
@@ -438,8 +438,8 @@ func TestACL_HTTP(t *testing.T) {
policies, ok := raw.(structs.ACLPolicyListStubs)
require.True(t, ok)

// 2 we just created + global management
require.Len(t, policies, 3)
// 2 we just created + builtin policies
require.Len(t, policies, 2+len(structs.ACLBuiltinPolicies))

for policyID, expected := range policyMap {
found := false
14 changes: 7 additions & 7 deletions agent/consul/acl_endpoint.go
Original file line number Diff line number Diff line change
@@ -869,8 +869,8 @@ func (a *ACL) PolicySet(args *structs.ACLPolicySetRequest, reply *structs.ACLPol
return fmt.Errorf("Invalid Policy: no Name is set")
}

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

var idMatch *structs.ACLPolicy
@@ -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)
jjacobson93 marked this conversation as resolved.
Show resolved Hide resolved
}

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)
}
}
}
@@ -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{
95 changes: 50 additions & 45 deletions agent/consul/acl_endpoint_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
@@ -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)
}
}
}

@@ -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")
}
@@ -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) {
@@ -2324,6 +2328,7 @@ func TestACLEndpoint_PolicyList(t *testing.T) {

policies := []string{
structs.ACLPolicyGlobalManagementID,
structs.ACLPolicyGlobalReadOnlyID,
p1.ID,
p2.ID,
}
2 changes: 1 addition & 1 deletion agent/consul/auth/token_writer.go
Original file line number Diff line number Diff line change
@@ -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")
4 changes: 2 additions & 2 deletions agent/consul/auth/token_writer_test.go
Original file line number Diff line number Diff line change
@@ -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)": {
@@ -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)": {
2 changes: 1 addition & 1 deletion agent/consul/fsm/snapshot_test.go
Original file line number Diff line number Diff line change
@@ -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))
Loading