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

.changelog/18319.txt

@@ -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
+```

acl/acl.go

@@ -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

acl/validation.go

@@ -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 {
+	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/'")
+	}
+
+	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

acl/validation_test.go

@@ -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) == nil)
+		})
+	}
+}

agent/acl_endpoint_test.go

@@ -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

agent/consul/acl_endpoint.go

@@ -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)
 			}
 
 			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{

agent/consul/acl_endpoint_test.go

@@ -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,
 	}

agent/consul/auth/token_writer.go

@@ -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")

agent/consul/auth/token_writer_test.go

@@ -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)": {

agent/consul/fsm/snapshot_test.go

@@ -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))