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

… into release/1.15.x (#18344)

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

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

* Add changelog

* Add read-only to docs

* Fix some minor issues.

* Change from ReplaceAll to Sprintf

* Change IsValidPolicy name to return an error instead of bool

* Fix PolicyList test

* Fix other tests

* Apply suggestions from code review



* Fix state store test for policy list.

* Fix naming issues

* Update acl/validation.go



* Update agent/consul/acl_endpoint.go

---------

Co-authored-by: Jeremy Jacobson <jjacobson93@users.noreply.github.com>
Co-authored-by: Paul Glass <pglass@hashicorp.com>
Co-authored-by: Chris Thain <32781396+cthain@users.noreply.github.com>

.changelog/18319.txt

@@ -0,0 +1,6 @@
+```release-note:improvement
+acl: added builtin ACL policy that provides global read-only access (builtin/global-read-only)
+```
+```release-note:improvement
+acl: allow for a single slash character in policy names
+```

acl/acl.go

@@ -9,6 +9,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

@@ -1,16 +1,21 @@
 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}$`)
 )
@@ -37,10 +42,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)
+// ValidatePolicyName returns nil if the provided name can be used as an
+// ACLPolicy Name otherwise a useful error is returned.
+func ValidatePolicyName(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 '%s'", ReservedBuiltinPrefix)
+	}
+
+	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_ValidatePolicyName(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, ValidatePolicyName(tc.name) == nil)
+		})
+	}
+}

agent/acl_endpoint_test.go

@@ -435,8 +435,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

@@ -866,8 +866,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.ValidatePolicyName(policy.Name); err != nil {
+		return err
 	}
 
 	var idMatch *structs.ACLPolicy
@@ -912,13 +912,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 %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)
 			}
 		}
 	}
@@ -996,8 +996,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

@@ -2180,7 +2180,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")
 	}
@@ -2192,47 +2192,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)
+		}
 	}
 }
 
@@ -2268,7 +2271,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")
 	}
@@ -2279,16 +2282,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) {
@@ -2321,6 +2325,7 @@ func TestACLEndpoint_PolicyList(t *testing.T) {
 
 	policies := []string{
 		structs.ACLPolicyGlobalManagementID,
+		structs.ACLPolicyGlobalReadOnlyID,
 		p1.ID,
 		p2.ID,
 	}

agent/consul/auth/token_writer.go

@@ -241,7 +241,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

@@ -38,7 +38,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)": {
@@ -54,7 +54,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

@@ -84,7 +84,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))