From 8244dc70e577bb387069b8522ee2304bac1849ff Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Wed, 26 Jul 2023 08:44:31 -0700 Subject: [PATCH 01/13] [CC-5719] Add support for builtin global-read-only policy --- acl/acl.go | 2 + acl/validation.go | 16 ++++- acl/validation_test.go | 78 +++++++++++++++++++++ agent/consul/acl_endpoint.go | 12 ++-- agent/consul/acl_endpoint_test.go | 95 ++++++++++++++------------ agent/consul/auth/token_writer.go | 2 +- agent/consul/auth/token_writer_test.go | 4 +- agent/consul/fsm/snapshot_test.go | 2 +- agent/consul/leader.go | 57 ++++++++-------- agent/consul/leader_test.go | 25 ++++--- agent/consul/state/acl.go | 12 ++-- agent/consul/state/acl_test.go | 13 ++-- agent/structs/acl.go | 57 ++++++++++++++-- agent/structs/acl_oss.go | 1 + 14 files changed, 263 insertions(+), 113 deletions(-) create mode 100644 acl/validation_test.go diff --git a/acl/acl.go b/acl/acl.go index fd6515d2389e..75789dd17498 100644 --- a/acl/acl.go +++ b/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 diff --git a/acl/validation.go b/acl/validation.go index 400465efafbf..3ce307e910c7 100644 --- a/acl/validation.go +++ b/acl/validation.go @@ -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}$`) ) @@ -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) } diff --git a/acl/validation_test.go b/acl/validation_test.go new file mode 100644 index 000000000000..8db7ddfe3556 --- /dev/null +++ b/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)) + }) + } +} diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 5d5ba3cb406f..b57249e71f1d 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -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 @@ -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{ diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index 7e09880ad3d6..20deb56aa4b0 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/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, } diff --git a/agent/consul/auth/token_writer.go b/agent/consul/auth/token_writer.go index ab99eeb37023..857a2e3d1321 100644 --- a/agent/consul/auth/token_writer.go +++ b/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") diff --git a/agent/consul/auth/token_writer_test.go b/agent/consul/auth/token_writer_test.go index e4fbf04db8c4..51a2b3cc45a8 100644 --- a/agent/consul/auth/token_writer_test.go +++ b/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)": { diff --git a/agent/consul/fsm/snapshot_test.go b/agent/consul/fsm/snapshot_test.go index a34e1f3e335c..d95865b92cb3 100644 --- a/agent/consul/fsm/snapshot_test.go +++ b/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)) diff --git a/agent/consul/leader.go b/agent/consul/leader.go index 4bc1908d5fe8..f4b5639bc63c 100644 --- a/agent/consul/leader.go +++ b/agent/consul/leader.go @@ -420,34 +420,11 @@ func (s *Server) initializeACLs(ctx context.Context) error { if s.InPrimaryDatacenter() { s.logger.Info("initializing acls") - // Create/Upgrade the builtin global-management policy - _, policy, err := s.fsm.State().ACLPolicyGetByID(nil, structs.ACLPolicyGlobalManagementID, structs.DefaultEnterpriseMetaInDefaultPartition()) - if err != nil { - return fmt.Errorf("failed to get the builtin global-management policy") - } - if policy == nil || policy.Rules != structs.ACLPolicyGlobalManagement { - newPolicy := structs.ACLPolicy{ - ID: structs.ACLPolicyGlobalManagementID, - Name: "global-management", - Description: "Builtin Policy that grants unlimited access", - Rules: structs.ACLPolicyGlobalManagement, - EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), - } - if policy != nil { - newPolicy.Name = policy.Name - newPolicy.Description = policy.Description - } - - newPolicy.SetHash(true) - - req := structs.ACLPolicyBatchSetRequest{ - Policies: structs.ACLPolicies{&newPolicy}, - } - _, err := s.raftApply(structs.ACLPolicySetRequestType, &req) - if err != nil { - return fmt.Errorf("failed to create global-management policy: %v", err) + // Create/Upgrade the builtin policies + for _, policy := range structs.ACLBuiltinPolicies { + if err := s.initializePolicy(policy); err != nil { + return err } - s.logger.Info("Created ACL 'global-management' policy") } // Check for configured initial management token. @@ -492,6 +469,32 @@ func (s *Server) initializeACLs(ctx context.Context) error { return nil } +func (s *Server) initializePolicy(newPolicy structs.ACLPolicy) error { + _, policy, err := s.fsm.State().ACLPolicyGetByID(nil, newPolicy.ID, structs.DefaultEnterpriseMetaInDefaultPartition()) + if err != nil { + return fmt.Errorf("failed to get the builtin %s policy", newPolicy.Name) + } + if policy == nil || policy.Rules != newPolicy.Rules { + if policy != nil { + newPolicy.Name = policy.Name + newPolicy.Description = policy.Description + } + + newPolicy.EnterpriseMeta = *structs.DefaultEnterpriseMetaInDefaultPartition() + newPolicy.SetHash(true) + + req := structs.ACLPolicyBatchSetRequest{ + Policies: structs.ACLPolicies{&newPolicy}, + } + _, err := s.raftApply(structs.ACLPolicySetRequestType, &req) + if err != nil { + return fmt.Errorf("failed to create %s policy: %v", newPolicy.Name, err) + } + s.logger.Info(fmt.Sprintf("Created ACL '%s' policy", newPolicy.Name)) + } + return nil +} + func (s *Server) initializeManagementToken(name, secretID string) error { state := s.fsm.State() if _, err := uuid.ParseUUID(secretID); err != nil { diff --git a/agent/consul/leader_test.go b/agent/consul/leader_test.go index 48b9876ae76d..8a5a158ce34a 100644 --- a/agent/consul/leader_test.go +++ b/agent/consul/leader_test.go @@ -1307,9 +1307,12 @@ func TestLeader_ACL_Initialization(t *testing.T) { _, s1 := testServerWithConfig(t, conf) testrpc.WaitForTestAgent(t, s1.RPC, "dc1") - _, policy, err := s1.fsm.State().ACLPolicyGetByID(nil, structs.ACLPolicyGlobalManagementID, nil) - require.NoError(t, err) - require.NotNil(t, policy) + // check that the builtin policies were created + for _, builtinPolicy := range structs.ACLBuiltinPolicies { + _, policy, err := s1.fsm.State().ACLPolicyGetByID(nil, builtinPolicy.ID, nil) + require.NoError(t, err) + require.NotNil(t, policy) + } if tt.initialManagement != "" { _, initialManagement, err := s1.fsm.State().ACLTokenGetBySecret(nil, tt.initialManagement, nil) @@ -1439,15 +1442,17 @@ func TestLeader_ACLUpgrade_IsStickyEvenIfSerfTagsRegress(t *testing.T) { waitForLeaderEstablishment(t, s2) waitForNewACLReplication(t, s2, structs.ACLReplicatePolicies, 1, 0, 0) - // Everybody has the management policy. + // Everybody has the builtin policies. retry.Run(t, func(r *retry.R) { - _, policy1, err := s1.fsm.State().ACLPolicyGetByID(nil, structs.ACLPolicyGlobalManagementID, structs.DefaultEnterpriseMetaInDefaultPartition()) - require.NoError(r, err) - require.NotNil(r, policy1) + for _, builtinPolicy := range structs.ACLBuiltinPolicies { + _, policy1, err := s1.fsm.State().ACLPolicyGetByID(nil, builtinPolicy.ID, structs.DefaultEnterpriseMetaInDefaultPartition()) + require.NoError(r, err) + require.NotNil(r, policy1) - _, policy2, err := s2.fsm.State().ACLPolicyGetByID(nil, structs.ACLPolicyGlobalManagementID, structs.DefaultEnterpriseMetaInDefaultPartition()) - require.NoError(r, err) - require.NotNil(r, policy2) + _, policy2, err := s2.fsm.State().ACLPolicyGetByID(nil, builtinPolicy.ID, structs.DefaultEnterpriseMetaInDefaultPartition()) + require.NoError(r, err) + require.NotNil(r, policy2) + } }) // Shutdown s1 and s2. diff --git a/agent/consul/state/acl.go b/agent/consul/state/acl.go index 1774ee879377..6d117c2bb98b 100644 --- a/agent/consul/state/acl.go +++ b/agent/consul/state/acl.go @@ -884,18 +884,18 @@ func aclPolicySetTxn(tx WriteTxn, idx uint64, policy *structs.ACLPolicy) error { } if existing != nil { - if policy.ID == structs.ACLPolicyGlobalManagementID { + if builtinPolicy, ok := structs.ACLBuiltinPolicies[policy.ID]; ok { // Only the name and description are modifiable // Here we specifically check that the rules on the global management policy // are identical to the correct policy rules within the binary. This is opposed // to checking against the current rules to allow us to update the rules during // upgrades. - if policy.Rules != structs.ACLPolicyGlobalManagement { - return fmt.Errorf("Changing the Rules for the builtin global-management policy is not permitted") + if policy.Rules != builtinPolicy.Rules { + return fmt.Errorf("Changing the Rules for the builtin %s policy is not permitted", builtinPolicy.Name) } 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) } } } @@ -1062,8 +1062,8 @@ func aclPolicyDeleteTxn(tx WriteTxn, idx uint64, value string, fn aclPolicyGetFn policy := rawPolicy.(*structs.ACLPolicy) - if policy.ID == structs.ACLPolicyGlobalManagementID { - return fmt.Errorf("Deletion of the builtin global-management policy is not permitted") + if builtinPolicy, ok := structs.ACLBuiltinPolicies[policy.ID]; ok { + return fmt.Errorf("Deletion of the builtin %s policy is not permitted", builtinPolicy.Name) } return aclPolicyDeleteWithPolicy(tx, policy, idx) diff --git a/agent/consul/state/acl_test.go b/agent/consul/state/acl_test.go index 78824315d089..878617540ee7 100644 --- a/agent/consul/state/acl_test.go +++ b/agent/consul/state/acl_test.go @@ -30,12 +30,7 @@ const ( ) func setupGlobalManagement(t *testing.T, s *Store) { - policy := structs.ACLPolicy{ - ID: structs.ACLPolicyGlobalManagementID, - Name: "global-management", - Description: "Builtin Policy that grants unlimited access", - Rules: structs.ACLPolicyGlobalManagement, - } + policy := structs.ACLBuiltinPolicies[structs.ACLPolicyGlobalManagementID] policy.SetHash(true) require.NoError(t, s.ACLPolicySet(1, &policy)) } @@ -1430,7 +1425,7 @@ func TestStateStore_ACLPolicy_SetGet(t *testing.T) { ID: structs.ACLPolicyGlobalManagementID, Name: "global-management", Description: "Global Management", - Rules: structs.ACLPolicyGlobalManagement, + Rules: structs.ACLPolicyGlobalManagementRules, Datacenters: []string{"dc1"}, } @@ -1444,7 +1439,7 @@ func TestStateStore_ACLPolicy_SetGet(t *testing.T) { ID: structs.ACLPolicyGlobalManagementID, Name: "management", Description: "Modified", - Rules: structs.ACLPolicyGlobalManagement, + Rules: structs.ACLPolicyGlobalManagementRules, } require.NoError(t, s.ACLPolicySet(3, &policy)) @@ -1494,7 +1489,7 @@ func TestStateStore_ACLPolicy_SetGet(t *testing.T) { require.NotNil(t, rpolicy) require.Equal(t, "global-management", rpolicy.Name) require.Equal(t, "Builtin Policy that grants unlimited access", rpolicy.Description) - require.Equal(t, structs.ACLPolicyGlobalManagement, rpolicy.Rules) + require.Equal(t, structs.ACLPolicyGlobalManagementRules, rpolicy.Rules) require.Len(t, rpolicy.Datacenters, 0) require.Equal(t, uint64(1), rpolicy.CreateIndex) require.Equal(t, uint64(1), rpolicy.ModifyIndex) diff --git a/agent/structs/acl.go b/agent/structs/acl.go index d239b8822224..66f6f3c07ffa 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -45,8 +45,10 @@ const ( // This policy gives unlimited access to everything. Users // may rename if desired but cannot delete or modify the rules. - ACLPolicyGlobalManagementID = "00000000-0000-0000-0000-000000000001" - ACLPolicyGlobalManagement = ` + ACLPolicyGlobalManagementID = "00000000-0000-0000-0000-000000000001" + ACLPolicyGlobalManagementName = "global-management" + ACLPolicyGlobalManagementDesc = "Builtin Policy that grants unlimited access" + ACLPolicyGlobalManagementRules = ` acl = "write" agent_prefix "" { policy = "write" @@ -75,11 +77,58 @@ session_prefix "" { policy = "write" }` + EnterpriseACLPolicyGlobalManagement - ACLReservedPrefix = "00000000-0000-0000-0000-0000000000" + ACLPolicyGlobalReadOnlyID = "00000000-0000-0000-0000-000000000002" + ACLPolicyGlobalReadOnlyName = "builtin/global-read-only" + ACLPolicyGlobalReadOnlyDesc = "Builtin Policy that grants unlimited read-only access to all components" + ACLPolicyGlobalReadOnlyRules = ` +acl = "read" +agent_prefix "" { + policy = "read" +} +event_prefix "" { + policy = "read" +} +key_prefix "" { + policy = "read" +} +keyring = "read" +node_prefix "" { + policy = "read" +} +operator = "read" +mesh = "read" +peering = "read" +query_prefix "" { + policy = "read" +} +service_prefix "" { + policy = "read" + intentions = "read" +} +session_prefix "" { + policy = "read" +}` + EnterpriseACLPolicyGlobalReadOnly + + ACLReservedIDPrefix = "00000000-0000-0000-0000-0000000000" ) +var ACLBuiltinPolicies = map[string]ACLPolicy{ + ACLPolicyGlobalManagementID: { + ID: ACLPolicyGlobalManagementID, + Name: ACLPolicyGlobalManagementName, + Description: ACLPolicyGlobalManagementDesc, + Rules: ACLPolicyGlobalManagementRules, + }, + ACLPolicyGlobalReadOnlyID: { + ID: ACLPolicyGlobalReadOnlyID, + Name: ACLPolicyGlobalReadOnlyName, + Description: ACLPolicyGlobalReadOnlyDesc, + Rules: ACLPolicyGlobalReadOnlyRules, + }, +} + func ACLIDReserved(id string) bool { - return strings.HasPrefix(id, ACLReservedPrefix) + return strings.HasPrefix(id, ACLReservedIDPrefix) } // ACLBootstrapNotAllowedErr is returned once we know that a bootstrap can no diff --git a/agent/structs/acl_oss.go b/agent/structs/acl_oss.go index 01e252636f86..9cc4e7813ce8 100644 --- a/agent/structs/acl_oss.go +++ b/agent/structs/acl_oss.go @@ -14,6 +14,7 @@ import ( const ( EnterpriseACLPolicyGlobalManagement = "" + EnterpriseACLPolicyGlobalReadOnly = "" // aclPolicyTemplateServiceIdentity is the template used for synthesizing // policies for service identities. From 4a376a6690b06760ba20bf787e0e7c3011f9ecc1 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Fri, 28 Jul 2023 09:27:10 -0700 Subject: [PATCH 02/13] Add changelog --- .changelog/18319.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .changelog/18319.txt diff --git a/.changelog/18319.txt b/.changelog/18319.txt new file mode 100644 index 000000000000..1198f640d18a --- /dev/null +++ b/.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 +``` From 75552a98c33cf3cb306197b3f9f2febb7b448504 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Fri, 28 Jul 2023 10:39:56 -0700 Subject: [PATCH 03/13] Add read-only to docs --- website/content/docs/security/acl/acl-policies.mdx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/website/content/docs/security/acl/acl-policies.mdx b/website/content/docs/security/acl/acl-policies.mdx index f5d005ffbb50..f23b0246d0d0 100644 --- a/website/content/docs/security/acl/acl-policies.mdx +++ b/website/content/docs/security/acl/acl-policies.mdx @@ -393,6 +393,10 @@ New installations of Consul ship with the following built-in policies. The `global-management` policy grants unrestricted privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000001`. You can rename the global management policy, but Consul will prevent you from modifying any other attributes, including the rule set and datacenter scope. +### Global Read-Only + +The `builtin/global-read-only` policy grants unrestricted _read-only_ privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000002`. You can rename the global read-only policy, but Consul will prevent you from modifying any other attributes, including the rule set and datacenter scope. + ### Namespace Management The `namespace-management` policy will be injected into all namespaces you create. The policy will be assigned a randomized UUID and can be managed as a normal, user-defined policy within the namespace. This feature was added in Consul Enterprise 1.7.0. From 209e57afb6950d34dd59965f29b6d6d11f187722 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 12:11:25 -0700 Subject: [PATCH 04/13] Fix some minor issues. --- acl/validation.go | 2 +- agent/consul/state/acl.go | 2 +- agent/consul/state/acl_test.go | 8 ++ agent/structs/acl.go | 110 +++++++----------- .../docs/security/acl/acl-policies.mdx | 4 +- 5 files changed, 56 insertions(+), 70 deletions(-) diff --git a/acl/validation.go b/acl/validation.go index 3ce307e910c7..17019ef8791f 100644 --- a/acl/validation.go +++ b/acl/validation.go @@ -17,7 +17,7 @@ const ( 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\-_]+/?[A-Za-z0-9\-_]*$`) + 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}$`) ) diff --git a/agent/consul/state/acl.go b/agent/consul/state/acl.go index 6d117c2bb98b..22c8a6164e42 100644 --- a/agent/consul/state/acl.go +++ b/agent/consul/state/acl.go @@ -886,7 +886,7 @@ func aclPolicySetTxn(tx WriteTxn, idx uint64, policy *structs.ACLPolicy) error { if existing != nil { if builtinPolicy, ok := structs.ACLBuiltinPolicies[policy.ID]; ok { // Only the name and description are modifiable - // Here we specifically check that the rules on the global management policy + // Here we specifically check that the rules on the builtin policy // are identical to the correct policy rules within the binary. This is opposed // to checking against the current rules to allow us to update the rules during // upgrades. diff --git a/agent/consul/state/acl_test.go b/agent/consul/state/acl_test.go index 878617540ee7..5a67551d9879 100644 --- a/agent/consul/state/acl_test.go +++ b/agent/consul/state/acl_test.go @@ -35,6 +35,12 @@ func setupGlobalManagement(t *testing.T, s *Store) { require.NoError(t, s.ACLPolicySet(1, &policy)) } +func setupBuiltinGlobalReadOnly(t *testing.T, s *Store) { + policy := structs.ACLBuiltinPolicies[structs.ACLPolicyGlobalReadOnlyID] + policy.SetHash(true) + require.NoError(t, s.ACLPolicySet(2, &policy)) +} + func setupAnonymous(t *testing.T, s *Store) { token := structs.ACLToken{ AccessorID: acl.AnonymousTokenID, @@ -48,6 +54,7 @@ func setupAnonymous(t *testing.T, s *Store) { func testACLStateStore(t *testing.T) *Store { s := testStateStore(t) setupGlobalManagement(t, s) + setupBuiltinGlobalReadOnly(t, s) setupAnonymous(t, s) return s } @@ -179,6 +186,7 @@ func TestStateStore_ACLBootstrap(t *testing.T) { s := testStateStore(t) setupGlobalManagement(t, s) + setupBuiltinGlobalReadOnly(t, s) canBootstrap, index, err := s.CanBootstrapACLToken() require.NoError(t, err) diff --git a/agent/structs/acl.go b/agent/structs/acl.go index 66f6f3c07ffa..bfee52bd00ce 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -45,87 +45,65 @@ const ( // This policy gives unlimited access to everything. Users // may rename if desired but cannot delete or modify the rules. - ACLPolicyGlobalManagementID = "00000000-0000-0000-0000-000000000001" - ACLPolicyGlobalManagementName = "global-management" - ACLPolicyGlobalManagementDesc = "Builtin Policy that grants unlimited access" - ACLPolicyGlobalManagementRules = ` -acl = "write" -agent_prefix "" { - policy = "write" -} -event_prefix "" { - policy = "write" -} -key_prefix "" { - policy = "write" -} -keyring = "write" -node_prefix "" { - policy = "write" -} -operator = "write" -mesh = "write" -peering = "write" -query_prefix "" { - policy = "write" -} -service_prefix "" { - policy = "write" - intentions = "write" -} -session_prefix "" { - policy = "write" -}` + EnterpriseACLPolicyGlobalManagement - - ACLPolicyGlobalReadOnlyID = "00000000-0000-0000-0000-000000000002" - ACLPolicyGlobalReadOnlyName = "builtin/global-read-only" - ACLPolicyGlobalReadOnlyDesc = "Builtin Policy that grants unlimited read-only access to all components" - ACLPolicyGlobalReadOnlyRules = ` -acl = "read" + ACLPolicyGlobalManagementID = "00000000-0000-0000-0000-000000000001" + ACLPolicyGlobalManagementName = "global-management" + ACLPolicyGlobalManagementDesc = "Builtin Policy that grants unlimited access" + + ACLPolicyGlobalReadOnlyID = "00000000-0000-0000-0000-000000000002" + ACLPolicyGlobalReadOnlyName = "builtin/global-read-only" + ACLPolicyGlobalReadOnlyDesc = "Builtin Policy that grants unlimited read-only access to all components" + + ACLReservedIDPrefix = "00000000-0000-0000-0000-0000000000" + + aclPolicyGlobalRulesTemplate = ` +acl = "###" agent_prefix "" { - policy = "read" + policy = "###" } event_prefix "" { - policy = "read" + policy = "###" } key_prefix "" { - policy = "read" + policy = "###" } -keyring = "read" +keyring = "###" node_prefix "" { - policy = "read" + policy = "###" } -operator = "read" -mesh = "read" -peering = "read" +operator = "###" +mesh = "###" +peering = "###" query_prefix "" { - policy = "read" + policy = "###" } service_prefix "" { - policy = "read" - intentions = "read" + policy = "###" + intentions = "###" } session_prefix "" { - policy = "read" -}` + EnterpriseACLPolicyGlobalReadOnly - - ACLReservedIDPrefix = "00000000-0000-0000-0000-0000000000" + policy = "###" +}` ) -var ACLBuiltinPolicies = map[string]ACLPolicy{ - ACLPolicyGlobalManagementID: { - ID: ACLPolicyGlobalManagementID, - Name: ACLPolicyGlobalManagementName, - Description: ACLPolicyGlobalManagementDesc, - Rules: ACLPolicyGlobalManagementRules, - }, - ACLPolicyGlobalReadOnlyID: { - ID: ACLPolicyGlobalReadOnlyID, - Name: ACLPolicyGlobalReadOnlyName, - Description: ACLPolicyGlobalReadOnlyDesc, - Rules: ACLPolicyGlobalReadOnlyRules, - }, -} +var ( + ACLPolicyGlobalReadOnlyRules = strings.ReplaceAll(aclPolicyGlobalRulesTemplate, "###", "read") + EnterpriseACLPolicyGlobalReadOnly + ACLPolicyGlobalManagementRules = strings.ReplaceAll(aclPolicyGlobalRulesTemplate, "###", "write") + EnterpriseACLPolicyGlobalManagement + + ACLBuiltinPolicies = map[string]ACLPolicy{ + ACLPolicyGlobalManagementID: { + ID: ACLPolicyGlobalManagementID, + Name: ACLPolicyGlobalManagementName, + Description: ACLPolicyGlobalManagementDesc, + Rules: ACLPolicyGlobalManagementRules, + }, + ACLPolicyGlobalReadOnlyID: { + ID: ACLPolicyGlobalReadOnlyID, + Name: ACLPolicyGlobalReadOnlyName, + Description: ACLPolicyGlobalReadOnlyDesc, + Rules: ACLPolicyGlobalReadOnlyRules, + }, + } +) func ACLIDReserved(id string) bool { return strings.HasPrefix(id, ACLReservedIDPrefix) diff --git a/website/content/docs/security/acl/acl-policies.mdx b/website/content/docs/security/acl/acl-policies.mdx index f23b0246d0d0..e1583f250a29 100644 --- a/website/content/docs/security/acl/acl-policies.mdx +++ b/website/content/docs/security/acl/acl-policies.mdx @@ -391,11 +391,11 @@ New installations of Consul ship with the following built-in policies. ### Global Management -The `global-management` policy grants unrestricted privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000001`. You can rename the global management policy, but Consul will prevent you from modifying any other attributes, including the rule set and datacenter scope. +The `global-management` policy grants unrestricted privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000001`. You can rename the global management policy, but Consul prevents you from modifying any other attributes, including the rule set and datacenter scope. ### Global Read-Only -The `builtin/global-read-only` policy grants unrestricted _read-only_ privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000002`. You can rename the global read-only policy, but Consul will prevent you from modifying any other attributes, including the rule set and datacenter scope. +The `builtin/global-read-only` policy grants unrestricted _read-only_ privileges to any token linked to it. The policy is assigned the reserved ID of `00000000-0000-0000-0000-000000000002`. You can rename the global read-only policy, but Consul prevents you from modifying any other attributes, including the rule set and datacenter scope. ### Namespace Management From aad09467e69436fc330ddd5ca1e0fb0b9fa5392b Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 12:32:35 -0700 Subject: [PATCH 05/13] Change from ReplaceAll to Sprintf --- agent/structs/acl.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/agent/structs/acl.go b/agent/structs/acl.go index bfee52bd00ce..bed8aaca7d6b 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -56,38 +56,38 @@ const ( ACLReservedIDPrefix = "00000000-0000-0000-0000-0000000000" aclPolicyGlobalRulesTemplate = ` -acl = "###" +acl = "%[1]s" agent_prefix "" { - policy = "###" + policy = "%[1]s" } event_prefix "" { - policy = "###" + policy = "%[1]s" } key_prefix "" { - policy = "###" + policy = "%[1]s" } -keyring = "###" +keyring = "%[1]s" node_prefix "" { - policy = "###" + policy = "%[1]s" } -operator = "###" -mesh = "###" -peering = "###" +operator = "%[1]s" +mesh = "%[1]s" +peering = "%[1]s" query_prefix "" { - policy = "###" + policy = "%[1]s" } service_prefix "" { - policy = "###" - intentions = "###" + policy = "%[1]s" + intentions = "%[1]s" } session_prefix "" { - policy = "###" + policy = "%[1]s" }` ) var ( - ACLPolicyGlobalReadOnlyRules = strings.ReplaceAll(aclPolicyGlobalRulesTemplate, "###", "read") + EnterpriseACLPolicyGlobalReadOnly - ACLPolicyGlobalManagementRules = strings.ReplaceAll(aclPolicyGlobalRulesTemplate, "###", "write") + EnterpriseACLPolicyGlobalManagement + ACLPolicyGlobalReadOnlyRules = fmt.Sprintf(aclPolicyGlobalRulesTemplate, "read") + EnterpriseACLPolicyGlobalReadOnly + ACLPolicyGlobalManagementRules = fmt.Sprintf(aclPolicyGlobalRulesTemplate, "write") + EnterpriseACLPolicyGlobalManagement ACLBuiltinPolicies = map[string]ACLPolicy{ ACLPolicyGlobalManagementID: { From 96b08610656b7472db9957026c901f2e84e2d286 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 12:40:42 -0700 Subject: [PATCH 06/13] Change IsValidPolicy name to return an error instead of bool --- acl/validation.go | 16 ++++++++++------ acl/validation_test.go | 2 +- agent/consul/acl_endpoint.go | 4 ++-- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/acl/validation.go b/acl/validation.go index 17019ef8791f..d85b45321da1 100644 --- a/acl/validation.go +++ b/acl/validation.go @@ -4,6 +4,7 @@ package acl import ( + "fmt" "regexp" "strings" ) @@ -44,18 +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 { +// 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 false + 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 false + return fmt.Errorf("Invalid Policy: invalid Name. Names cannot be prefixed with '/' or 'builtin/'") } - return validPolicyName.MatchString(name) + 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 diff --git a/acl/validation_test.go b/acl/validation_test.go index 8db7ddfe3556..6177a8ef4b8d 100644 --- a/acl/validation_test.go +++ b/acl/validation_test.go @@ -72,7 +72,7 @@ func Test_IsValidPolicyName(t *testing.T) { }, } { t.Run(tc.description, func(t *testing.T) { - require.Equal(t, tc.valid, IsValidPolicyName(tc.name)) + require.Equal(t, tc.valid, IsValidPolicyName(tc.name) == nil) }) } } diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index b57249e71f1d..bba2e483e835 100644 --- a/agent/consul/acl_endpoint.go +++ b/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, a single '/', '-' and '_' are allowed") + if err := acl.IsValidPolicyName(policy.Name); err != nil { + return err } var idMatch *structs.ACLPolicy From 495162f1d6b44a2dffbb132ef209bff0bd60070a Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 12:51:04 -0700 Subject: [PATCH 07/13] Fix PolicyList test --- api/acl_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/api/acl_test.go b/api/acl_test.go index 2a7bcbd23f78..f515878290cd 100644 --- a/api/acl_test.go +++ b/api/acl_test.go @@ -190,7 +190,7 @@ func TestAPI_ACLPolicy_List(t *testing.T) { policies, qm, err := acl.PolicyList(nil) require.NoError(t, err) - require.Len(t, policies, 4) + require.Len(t, policies, 5) require.NotEqual(t, 0, qm.LastIndex) require.True(t, qm.KnownLeader) @@ -233,6 +233,11 @@ func TestAPI_ACLPolicy_List(t *testing.T) { policy4, ok := policyMap["00000000-0000-0000-0000-000000000001"] require.True(t, ok) require.NotNil(t, policy4) + + // make sure the 5th policy is the global read-only + policy5, ok := policyMap["00000000-0000-0000-0000-000000000002"] + require.True(t, ok) + require.NotNil(t, policy5) } func prepTokenPolicies(t *testing.T, acl *ACL) (policies []*ACLPolicy) { From d54e3a993904b0170e089cc75848bba560514c85 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 12:54:29 -0700 Subject: [PATCH 08/13] Fix other tests --- agent/acl_endpoint_test.go | 4 ++-- agent/consul/state/acl_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/agent/acl_endpoint_test.go b/agent/acl_endpoint_test.go index 0c948880e036..4a84ef3155fd 100644 --- a/agent/acl_endpoint_test.go +++ b/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 diff --git a/agent/consul/state/acl_test.go b/agent/consul/state/acl_test.go index 5a67551d9879..321b4d7b4c7c 100644 --- a/agent/consul/state/acl_test.go +++ b/agent/consul/state/acl_test.go @@ -1667,7 +1667,7 @@ func TestStateStore_ACLPolicy_List(t *testing.T) { _, policies, err := s.ACLPolicyList(nil, nil) require.NoError(t, err) - require.Len(t, policies, 3) + require.Len(t, policies, 4) policies.Sort() require.Equal(t, structs.ACLPolicyGlobalManagementID, policies[0].ID) require.Equal(t, "global-management", policies[0].Name) From 9878915cee9a3dd165c973ea3d12756bc9f21630 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 13:31:27 -0700 Subject: [PATCH 09/13] Apply suggestions from code review Co-authored-by: Paul Glass --- .changelog/18319.txt | 2 ++ agent/consul/leader.go | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.changelog/18319.txt b/.changelog/18319.txt index 1198f640d18a..bb9c8cdf2c72 100644 --- a/.changelog/18319.txt +++ b/.changelog/18319.txt @@ -1,4 +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 ``` diff --git a/agent/consul/leader.go b/agent/consul/leader.go index f4b5639bc63c..2ca0f7ecf9e7 100644 --- a/agent/consul/leader.go +++ b/agent/consul/leader.go @@ -469,7 +469,11 @@ func (s *Server) initializeACLs(ctx context.Context) error { return nil } -func (s *Server) initializePolicy(newPolicy structs.ACLPolicy) error { +// writeBuiltinACLPolicy writes the given built-in policy to Raft if the policy +// is not found or if the policy rules have been changed. The name and +// description of a built-in policy are user-editable and must be preserved +// during updates. This function must only be called in a primary datacenter. +func (s *Server) writeBuiltinACLPolicy(newPolicy structs.ACLPolicy) error { _, policy, err := s.fsm.State().ACLPolicyGetByID(nil, newPolicy.ID, structs.DefaultEnterpriseMetaInDefaultPartition()) if err != nil { return fmt.Errorf("failed to get the builtin %s policy", newPolicy.Name) From 7b5077def9a789aefd748fb51ce6f9c2f1b0fc8a Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 13:35:46 -0700 Subject: [PATCH 10/13] Fix state store test for policy list. --- agent/consul/state/acl_test.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/agent/consul/state/acl_test.go b/agent/consul/state/acl_test.go index 321b4d7b4c7c..4bf42954dd20 100644 --- a/agent/consul/state/acl_test.go +++ b/agent/consul/state/acl_test.go @@ -1670,28 +1670,36 @@ func TestStateStore_ACLPolicy_List(t *testing.T) { require.Len(t, policies, 4) policies.Sort() require.Equal(t, structs.ACLPolicyGlobalManagementID, policies[0].ID) - require.Equal(t, "global-management", policies[0].Name) - require.Equal(t, "Builtin Policy that grants unlimited access", policies[0].Description) + require.Equal(t, structs.ACLPolicyGlobalManagementName, policies[0].Name) + require.Equal(t, structs.ACLPolicyGlobalManagementDesc, policies[0].Description) require.Empty(t, policies[0].Datacenters) require.NotEqual(t, []byte{}, policies[0].Hash) require.Equal(t, uint64(1), policies[0].CreateIndex) require.Equal(t, uint64(1), policies[0].ModifyIndex) - require.Equal(t, "a2719052-40b3-4a4b-baeb-f3df1831a217", policies[1].ID) - require.Equal(t, "acl-write-dc3", policies[1].Name) - require.Equal(t, "Can manage ACLs in dc3", policies[1].Description) - require.ElementsMatch(t, []string{"dc3"}, policies[1].Datacenters) - require.Nil(t, policies[1].Hash) + require.Equal(t, structs.ACLPolicyGlobalReadOnlyID, policies[1].ID) + require.Equal(t, structs.ACLPolicyGlobalReadOnlyName, policies[1].Name) + require.Equal(t, structs.ACLPolicyGlobalReadOnlyDesc, policies[1].Description) + require.Empty(t, policies[1].Datacenters) + require.NotEqual(t, []byte{}, policies[1].Hash) require.Equal(t, uint64(2), policies[1].CreateIndex) require.Equal(t, uint64(2), policies[1].ModifyIndex) - require.Equal(t, "a4f68bd6-3af5-4f56-b764-3c6f20247879", policies[2].ID) - require.Equal(t, "service-read", policies[2].Name) - require.Equal(t, "", policies[2].Description) - require.Empty(t, policies[2].Datacenters) + require.Equal(t, "a2719052-40b3-4a4b-baeb-f3df1831a217", policies[2].ID) + require.Equal(t, "acl-write-dc3", policies[2].Name) + require.Equal(t, "Can manage ACLs in dc3", policies[2].Description) + require.ElementsMatch(t, []string{"dc3"}, policies[2].Datacenters) require.Nil(t, policies[2].Hash) require.Equal(t, uint64(2), policies[2].CreateIndex) require.Equal(t, uint64(2), policies[2].ModifyIndex) + + require.Equal(t, "a4f68bd6-3af5-4f56-b764-3c6f20247879", policies[3].ID) + require.Equal(t, "service-read", policies[3].Name) + require.Equal(t, "", policies[3].Description) + require.Empty(t, policies[3].Datacenters) + require.Nil(t, policies[3].Hash) + require.Equal(t, uint64(2), policies[3].CreateIndex) + require.Equal(t, uint64(2), policies[3].ModifyIndex) } func TestStateStore_ACLPolicy_Delete(t *testing.T) { From 2a6a531bd5311d14cd8831178bb571140261220c Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 13:37:05 -0700 Subject: [PATCH 11/13] Fix naming issues --- acl/validation.go | 4 ++-- acl/validation_test.go | 4 ++-- agent/consul/acl_endpoint.go | 2 +- agent/consul/leader.go | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/acl/validation.go b/acl/validation.go index d85b45321da1..b63d6ca4dafd 100644 --- a/acl/validation.go +++ b/acl/validation.go @@ -45,9 +45,9 @@ func IsValidNodeIdentityName(name string) bool { return validNodeIdentityName.MatchString(name) } -// IsValidPolicyName returns nil if the provided name can be used as an +// ValidatePolicyName returns nil if the provided name can be used as an // ACLPolicy Name otherwise a useful error is returned. -func IsValidPolicyName(name string) error { +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) } diff --git a/acl/validation_test.go b/acl/validation_test.go index 6177a8ef4b8d..d5d01e0e9054 100644 --- a/acl/validation_test.go +++ b/acl/validation_test.go @@ -9,7 +9,7 @@ import ( "github.com/stretchr/testify/require" ) -func Test_IsValidPolicyName(t *testing.T) { +func Test_ValidatePolicyName(t *testing.T) { for _, tc := range []struct { description string name string @@ -72,7 +72,7 @@ func Test_IsValidPolicyName(t *testing.T) { }, } { t.Run(tc.description, func(t *testing.T) { - require.Equal(t, tc.valid, IsValidPolicyName(tc.name) == nil) + require.Equal(t, tc.valid, ValidatePolicyName(tc.name) == nil) }) } } diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index bba2e483e835..e18e2834bd5c 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -869,7 +869,7 @@ func (a *ACL) PolicySet(args *structs.ACLPolicySetRequest, reply *structs.ACLPol return fmt.Errorf("Invalid Policy: no Name is set") } - if err := acl.IsValidPolicyName(policy.Name); err != nil { + if err := acl.ValidatePolicyName(policy.Name); err != nil { return err } diff --git a/agent/consul/leader.go b/agent/consul/leader.go index 2ca0f7ecf9e7..17408d4ef441 100644 --- a/agent/consul/leader.go +++ b/agent/consul/leader.go @@ -422,7 +422,7 @@ func (s *Server) initializeACLs(ctx context.Context) error { // Create/Upgrade the builtin policies for _, policy := range structs.ACLBuiltinPolicies { - if err := s.initializePolicy(policy); err != nil { + if err := s.writeBuiltinACLPolicy(policy); err != nil { return err } } @@ -470,7 +470,7 @@ func (s *Server) initializeACLs(ctx context.Context) error { } // writeBuiltinACLPolicy writes the given built-in policy to Raft if the policy -// is not found or if the policy rules have been changed. The name and +// is not found or if the policy rules have been changed. The name and // description of a built-in policy are user-editable and must be preserved // during updates. This function must only be called in a primary datacenter. func (s *Server) writeBuiltinACLPolicy(newPolicy structs.ACLPolicy) error { From d63fa5481dc02c6faae7cc2647b4073b3286af1d Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 31 Jul 2023 14:21:08 -0700 Subject: [PATCH 12/13] Update acl/validation.go Co-authored-by: Chris Thain <32781396+cthain@users.noreply.github.com> --- acl/validation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acl/validation.go b/acl/validation.go index b63d6ca4dafd..96119dcc0fba 100644 --- a/acl/validation.go +++ b/acl/validation.go @@ -53,7 +53,7 @@ func ValidatePolicyName(name string) error { } if strings.HasPrefix(name, "/") || strings.HasPrefix(name, ReservedBuiltinPrefix) { - return fmt.Errorf("Invalid Policy: invalid Name. Names cannot be prefixed with '/' or 'builtin/'") + return fmt.Errorf("Invalid Policy: invalid Name. Names cannot be prefixed with '/' or '%s'", ReservedBuiltinPrefix) } if !validPolicyName.MatchString(name) { From 3d099a6ed8ed10b6dc464c466cb1668914db8f08 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Tue, 1 Aug 2023 09:54:16 -0700 Subject: [PATCH 13/13] Update agent/consul/acl_endpoint.go --- agent/consul/acl_endpoint.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index e18e2834bd5c..7d2d1028cc85 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -917,7 +917,7 @@ func (a *ACL) PolicySet(args *structs.ACLPolicySetRequest, reply *structs.ACLPol 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 %s policy is not permitted", builtinPolicy.Name) + return fmt.Errorf("Changing the Datacenters of the %s policy is not permitted", builtinPolicy.Name) } if policy.Rules != idMatch.Rules {