From d03f849e496b6ceecb56e5e3c8dbf45d458c196c Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Fri, 22 Oct 2021 04:41:35 -0700 Subject: [PATCH 1/7] acl: Expand ServiceRead logic to look at service-exports for cross-partition --- acl/authorizer.go | 3 +++ acl/authorizer_test.go | 5 +++++ acl/chained_authorizer.go | 6 ++++++ acl/chained_authorizer_test.go | 3 +++ acl/policy_authorizer.go | 4 ++++ acl/static_authorizer.go | 7 +++++++ agent/consul/acl.go | 7 ++++++- 7 files changed, 34 insertions(+), 1 deletion(-) diff --git a/acl/authorizer.go b/acl/authorizer.go index 43d50544bf..427eb1a162 100644 --- a/acl/authorizer.go +++ b/acl/authorizer.go @@ -149,6 +149,9 @@ type Authorizer interface { // service ServiceWrite(string, *AuthorizerContext) EnforcementDecision + // ServiceWriteAny checks for permission to read any service + ServiceWriteAny(*AuthorizerContext) EnforcementDecision + // SessionRead checks for permission to read sessions for a given node. SessionRead(string, *AuthorizerContext) EnforcementDecision diff --git a/acl/authorizer_test.go b/acl/authorizer_test.go index d74029f239..7d32a78bf7 100644 --- a/acl/authorizer_test.go +++ b/acl/authorizer_test.go @@ -185,6 +185,11 @@ func (m *mockAuthorizer) ServiceWrite(segment string, ctx *AuthorizerContext) En return ret.Get(0).(EnforcementDecision) } +func (m *mockAuthorizer) ServiceWriteAny(ctx *AuthorizerContext) EnforcementDecision { + ret := m.Called(ctx) + return ret.Get(0).(EnforcementDecision) +} + // SessionRead checks for permission to read sessions for a given node. func (m *mockAuthorizer) SessionRead(segment string, ctx *AuthorizerContext) EnforcementDecision { ret := m.Called(segment, ctx) diff --git a/acl/chained_authorizer.go b/acl/chained_authorizer.go index 1b3aed4978..33a05f9f1e 100644 --- a/acl/chained_authorizer.go +++ b/acl/chained_authorizer.go @@ -235,6 +235,12 @@ func (c *ChainedAuthorizer) ServiceWrite(name string, entCtx *AuthorizerContext) }) } +func (c *ChainedAuthorizer) ServiceWriteAny(entCtx *AuthorizerContext) EnforcementDecision { + return c.executeChain(func(authz Authorizer) EnforcementDecision { + return authz.ServiceWriteAny(entCtx) + }) +} + // SessionRead checks for permission to read sessions for a given node. func (c *ChainedAuthorizer) SessionRead(node string, entCtx *AuthorizerContext) EnforcementDecision { return c.executeChain(func(authz Authorizer) EnforcementDecision { diff --git a/acl/chained_authorizer_test.go b/acl/chained_authorizer_test.go index 7a1aba2396..ac4880ba09 100644 --- a/acl/chained_authorizer_test.go +++ b/acl/chained_authorizer_test.go @@ -89,6 +89,9 @@ func (authz testAuthorizer) ServiceReadAll(*AuthorizerContext) EnforcementDecisi func (authz testAuthorizer) ServiceWrite(string, *AuthorizerContext) EnforcementDecision { return EnforcementDecision(authz) } +func (authz testAuthorizer) ServiceWriteAny(*AuthorizerContext) EnforcementDecision { + return EnforcementDecision(authz) +} func (authz testAuthorizer) SessionRead(string, *AuthorizerContext) EnforcementDecision { return EnforcementDecision(authz) } diff --git a/acl/policy_authorizer.go b/acl/policy_authorizer.go index 8cce120eb6..0e9496bf78 100644 --- a/acl/policy_authorizer.go +++ b/acl/policy_authorizer.go @@ -767,6 +767,10 @@ func (p *policyAuthorizer) ServiceWrite(name string, _ *AuthorizerContext) Enfor return Default } +func (p *policyAuthorizer) ServiceWriteAny(_ *AuthorizerContext) EnforcementDecision { + return p.anyAllowed(p.serviceRules, AccessWrite) +} + // SessionRead checks for permission to read sessions for a given node. func (p *policyAuthorizer) SessionRead(node string, _ *AuthorizerContext) EnforcementDecision { if rule, ok := getPolicy(node, p.sessionRules); ok { diff --git a/acl/static_authorizer.go b/acl/static_authorizer.go index f257d6b68a..2837b8f0ac 100644 --- a/acl/static_authorizer.go +++ b/acl/static_authorizer.go @@ -219,6 +219,13 @@ func (s *staticAuthorizer) ServiceWrite(string, *AuthorizerContext) EnforcementD return Deny } +func (s *staticAuthorizer) ServiceWriteAny(*AuthorizerContext) EnforcementDecision { + if s.defaultAllow { + return Allow + } + return Deny +} + func (s *staticAuthorizer) SessionRead(string, *AuthorizerContext) EnforcementDecision { if s.defaultAllow { return Allow diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 2bbe961d16..73a181becf 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1090,8 +1090,13 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs // Build the Authorizer var chain []acl.Authorizer + var conf acl.Config + if r.aclConf != nil { + conf = *r.aclConf + } + conf.LocalPartition = identity.EnterpriseMetadata().PartitionOrDefault() - authz, err := policies.Compile(r.cache, r.aclConf) + authz, err := policies.Compile(r.cache, &conf) if err != nil { return nil, nil, err } From 65c91093962b085497d2d7f9f5fd82110df89eed Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Sun, 24 Oct 2021 18:28:46 -0400 Subject: [PATCH 2/7] acl: pass PartitionInfo through ent ACLConfig --- acl/acl.go | 5 +++++ agent/consul/acl.go | 8 +++++++- agent/consul/acl_oss.go | 8 +++++++- agent/consul/client.go | 2 +- agent/consul/server.go | 3 ++- 5 files changed, 22 insertions(+), 4 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index 87295aa005..9538a18e5b 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -14,6 +14,11 @@ type Config struct { EnterpriseConfig } +type PartitionExportInfo interface { + // DownstreamPartitions returns the list of partitions the given service has been exported to. + DownstreamPartitions(service string, ctx *AuthorizerContext) []string +} + // GetWildcardName will retrieve the configured wildcard name or provide a default // in the case that the config is Nil or the wildcard name is unset. func (c *Config) GetWildcardName() string { diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 73a181becf..095ec8eba6 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1094,7 +1094,7 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs if r.aclConf != nil { conf = *r.aclConf } - conf.LocalPartition = identity.EnterpriseMetadata().PartitionOrDefault() + r.setEnterpriseConf(identity, &conf) authz, err := policies.Compile(r.cache, &conf) if err != nil { @@ -1900,3 +1900,9 @@ func filterACL(r *ACLResolver, token string, subj interface{}) error { filterACLWithAuthorizer(r.logger, authorizer, subj) return nil } + +type partitionInfoNoop struct{} + +func (p *partitionInfoNoop) DownstreamPartitions(service string, ctx *acl.AuthorizerContext) []string { + return []string{} +} diff --git a/agent/consul/acl_oss.go b/agent/consul/acl_oss.go index 8b0417b413..7c37023a5d 100644 --- a/agent/consul/acl_oss.go +++ b/agent/consul/acl_oss.go @@ -15,7 +15,11 @@ func (s *Server) replicationEnterpriseMeta() *structs.EnterpriseMeta { return structs.ReplicationEnterpriseMeta() } -func newACLConfig(hclog.Logger) *acl.Config { +func serverPartitionInfo(s *Server) acl.PartitionExportInfo { + return &partitionInfoNoop{} +} + +func newACLConfig(_ acl.PartitionExportInfo, hclog.Logger) *acl.Config { return &acl.Config{ WildcardName: structs.WildcardSpecifier, } @@ -41,3 +45,5 @@ func (_ *ACLResolver) resolveEnterpriseIdentityAndPolicies(_ structs.ACLIdentity func (_ *ACLResolver) resolveLocallyManagedEnterpriseToken(_ string) (structs.ACLIdentity, acl.Authorizer, bool) { return nil, nil, false } + +func (_ *ACLResolver) setEnterpriseConf(identity structs.ACLIdentity, conf *acl.Config) {} diff --git a/agent/consul/client.go b/agent/consul/client.go index a2abc7fb76..031308e19e 100644 --- a/agent/consul/client.go +++ b/agent/consul/client.go @@ -123,7 +123,7 @@ func NewClient(config *Config, deps Deps) (*Client, error) { Logger: c.logger, DisableDuration: aclClientDisabledTTL, CacheConfig: clientACLCacheConfig, - ACLConfig: newACLConfig(c.logger), + ACLConfig: newACLConfig(&partitionInfoNoop{}, c.logger), Tokens: deps.Tokens, } var err error diff --git a/agent/consul/server.go b/agent/consul/server.go index 5069e4d802..969785a23a 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -427,7 +427,8 @@ func NewServer(config *Config, flat Deps) (*Server, error) { // Initialize the stats fetcher that autopilot will use. s.statsFetcher = NewStatsFetcher(logger, s.connPool, s.config.Datacenter) - s.aclConfig = newACLConfig(logger) + partitionInfo := serverPartitionInfo(s) + s.aclConfig = newACLConfig(partitionInfo, logger) aclConfig := ACLResolverConfig{ Config: config.ACLResolverSettings, Delegate: s, From 22bdf279d16bc0a3ecfe519f11d3a47455c77d4b Mon Sep 17 00:00:00 2001 From: freddygv Date: Mon, 25 Oct 2021 20:17:21 -0600 Subject: [PATCH 3/7] Update NodeRead for partition-exports When issuing cross-partition service discovery requests, ACL filtering often checks for NodeRead privileges. This is because the common return type is a CheckServiceNode, which contains node data. --- acl/acl.go | 2 +- agent/consul/acl.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index 9538a18e5b..a59f380446 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -16,7 +16,7 @@ type Config struct { type PartitionExportInfo interface { // DownstreamPartitions returns the list of partitions the given service has been exported to. - DownstreamPartitions(service string, ctx *AuthorizerContext) []string + DownstreamPartitions(service string, anyService bool, ctx *AuthorizerContext) []string } // GetWildcardName will retrieve the configured wildcard name or provide a default diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 095ec8eba6..c5bf1aa96e 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -1903,6 +1903,6 @@ func filterACL(r *ACLResolver, token string, subj interface{}) error { type partitionInfoNoop struct{} -func (p *partitionInfoNoop) DownstreamPartitions(service string, ctx *acl.AuthorizerContext) []string { +func (p *partitionInfoNoop) DownstreamPartitions(service string, anyService bool, ctx *acl.AuthorizerContext) []string { return []string{} } From df7b5af6f089934deee4f538d12393f9d50d7c03 Mon Sep 17 00:00:00 2001 From: freddygv Date: Tue, 26 Oct 2021 12:02:34 -0600 Subject: [PATCH 4/7] Avoid panic on nil partitionAuthorizer config partitionAuthorizer.config can be nil if it wasn't provided on calls to newPartitionAuthorizer outside of the ACLResolver. This usage happens often in tests. This commit: adds a nil check when the config is going to be used, updates non-test usage of NewPolicyAuthorizerWithDefaults to pass a non-nil config, and dettaches setEnterpriseConf from the ACLResolver. --- agent/consul/acl.go | 7 +++++-- agent/consul/acl_oss.go | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/agent/consul/acl.go b/agent/consul/acl.go index c5bf1aa96e..18eae6390f 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -292,7 +292,10 @@ func agentMasterAuthorizer(nodeName string, entMeta *structs.EnterpriseMeta) (ac }, }, } - return acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + + var cfg *acl.Config + setEnterpriseConf(entMeta, cfg) + return acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, cfg) } func NewACLResolver(config *ACLResolverConfig) (*ACLResolver, error) { @@ -1094,7 +1097,7 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs if r.aclConf != nil { conf = *r.aclConf } - r.setEnterpriseConf(identity, &conf) + setEnterpriseConf(identity.EnterpriseMetadata(), &conf) authz, err := policies.Compile(r.cache, &conf) if err != nil { diff --git a/agent/consul/acl_oss.go b/agent/consul/acl_oss.go index 7c37023a5d..5be3b3fb1b 100644 --- a/agent/consul/acl_oss.go +++ b/agent/consul/acl_oss.go @@ -46,4 +46,4 @@ func (_ *ACLResolver) resolveLocallyManagedEnterpriseToken(_ string) (structs.AC return nil, nil, false } -func (_ *ACLResolver) setEnterpriseConf(identity structs.ACLIdentity, conf *acl.Config) {} +func setEnterpriseConf(entMeta *structs.EnterpriseMeta, conf *acl.Config) {} From bf350224a0850e95906aa16dec91b60ca3bda4c4 Mon Sep 17 00:00:00 2001 From: freddygv Date: Tue, 26 Oct 2021 12:40:20 -0600 Subject: [PATCH 5/7] Avoid passing nil config pointer --- agent/consul/acl.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 18eae6390f..6d3414ce3b 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -293,9 +293,9 @@ func agentMasterAuthorizer(nodeName string, entMeta *structs.EnterpriseMeta) (ac }, } - var cfg *acl.Config - setEnterpriseConf(entMeta, cfg) - return acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, cfg) + cfg := acl.Config{} + setEnterpriseConf(entMeta, &cfg) + return acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, &cfg) } func NewACLResolver(config *ACLResolverConfig) (*ACLResolver, error) { From aa931682eafea04e7dfc7984fcc73b3ab483a23d Mon Sep 17 00:00:00 2001 From: freddygv Date: Tue, 26 Oct 2021 12:53:11 -0600 Subject: [PATCH 6/7] Avoid mixing named and unnamed params --- agent/consul/acl_oss.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/consul/acl_oss.go b/agent/consul/acl_oss.go index 5be3b3fb1b..f601f4ce17 100644 --- a/agent/consul/acl_oss.go +++ b/agent/consul/acl_oss.go @@ -19,7 +19,7 @@ func serverPartitionInfo(s *Server) acl.PartitionExportInfo { return &partitionInfoNoop{} } -func newACLConfig(_ acl.PartitionExportInfo, hclog.Logger) *acl.Config { +func newACLConfig(_ acl.PartitionExportInfo, _ hclog.Logger) *acl.Config { return &acl.Config{ WildcardName: structs.WildcardSpecifier, } From 686b883600fa391efeb65b2ca1bb1f5814d2302d Mon Sep 17 00:00:00 2001 From: freddygv Date: Wed, 27 Oct 2021 09:06:34 -0600 Subject: [PATCH 7/7] Add changelog entry --- .changelog/11433.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/11433.txt diff --git a/.changelog/11433.txt b/.changelog/11433.txt new file mode 100644 index 0000000000..dfa6aa4ac9 --- /dev/null +++ b/.changelog/11433.txt @@ -0,0 +1,3 @@ +```release-note:improvement +connect: **(Enterprise only)** updates ServiceRead and NodeRead to account for the partition-exports config entry. +``` \ No newline at end of file