Skip to content

Commit

Permalink
Refactor config checks oss (#12550)
Browse files Browse the repository at this point in the history 
Currently the config_entry.go subsystem delegates authorization decisions via the ConfigEntry interface CanRead and CanWrite code. Unfortunately this returns a true/false value and loses the details of the source.

This is not helpful, especially since it the config subsystem can be more complex to understand, since it covers so many domains.

This refactors CanRead/CanWrite to return a structured error message (PermissionDenied or the like) with more details about the reason for denial.

Part of #12241

Signed-off-by: Mark Anderson <[email protected]>
  • Loading branch information
@markan
markan authored Mar 11, 2022
1 parent b9a5e10 commit 676ea58
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 55 deletions.
18 changes: 10 additions & 8 deletions agent/consul/config_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ func (c *ConfigEntry) Apply(args *structs.ConfigEntryRequest, reply *bool) error
return err
}

if !args.Entry.CanWrite(authz) {
return acl.ErrPermissionDenied // TODO(acl-error-enhancements) Better errors await refactoring of CanWrite above.
if err := args.Entry.CanWrite(authz); err != nil {
return err
}

if args.Op != structs.ConfigEntryUpsert && args.Op != structs.ConfigEntryUpsertCAS {
Expand Down Expand Up @@ -194,8 +194,8 @@ func (c *ConfigEntry) Get(args *structs.ConfigEntryQuery, reply *structs.ConfigE
}
lookupEntry.GetEnterpriseMeta().Merge(&args.EnterpriseMeta)

if !lookupEntry.CanRead(authz) {
return acl.ErrPermissionDenied
if err := lookupEntry.CanRead(authz); err != nil {
return err
}

return c.srv.blockingQuery(
Expand Down Expand Up @@ -254,7 +254,8 @@ func (c *ConfigEntry) List(args *structs.ConfigEntryQuery, reply *structs.Indexe
// Filter the entries returned by ACL permissions.
filteredEntries := make([]structs.ConfigEntry, 0, len(entries))
for _, entry := range entries {
if !entry.CanRead(authz) {
if err := entry.CanRead(authz); err != nil {
// TODO we may wish to extract more details from this error to aid user comprehension
reply.QueryMeta.ResultsFilteredByACLs = true
continue
}
Expand Down Expand Up @@ -335,7 +336,8 @@ func (c *ConfigEntry) ListAll(args *structs.ConfigEntryListAllRequest, reply *st
// Filter the entries returned by ACL permissions or by the provided kinds.
filteredEntries := make([]structs.ConfigEntry, 0, len(entries))
for _, entry := range entries {
if !entry.CanRead(authz) {
if err := entry.CanRead(authz); err != nil {
// TODO we may wish to extract more details from this error to aid user comprehension
reply.QueryMeta.ResultsFilteredByACLs = true
continue
}
Expand Down Expand Up @@ -386,8 +388,8 @@ func (c *ConfigEntry) Delete(args *structs.ConfigEntryRequest, reply *structs.Co
return err
}

if !args.Entry.CanWrite(authz) {
return acl.ErrPermissionDenied
if err := args.Entry.CanWrite(authz); err != nil {
return err
}

// Only delete and delete-cas ops are supported. If the caller erroneously
Expand Down
2 changes: 1 addition & 1 deletion agent/http.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler {
// If the token provided does not have the necessary permissions,
// write a forbidden response
// TODO(partitions): should this be possible in a partition?
// TODO((acl-error-enhancements)): We should return error details somehow here.
// TODO(acl-error-enhancements): We should return error details somehow here.
if authz.OperatorRead(nil) != acl.Allow {
resp.WriteHeader(http.StatusForbidden)
return
Expand Down
20 changes: 10 additions & 10 deletions agent/structs/config_entry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ type ConfigEntry interface {

// CanRead and CanWrite return whether or not the given Authorizer
// has permission to read or write to the config entry, respectively.
CanRead(acl.Authorizer) bool
CanWrite(acl.Authorizer) bool
CanRead(acl.Authorizer) error
CanWrite(acl.Authorizer) error

GetMeta() map[string]string
GetEnterpriseMeta() *EnterpriseMeta
Expand Down Expand Up @@ -183,16 +183,16 @@ func (e *ServiceConfigEntry) Validate() error {
return validationErr
}

func (e *ServiceConfigEntry) CanRead(authz acl.Authorizer) bool {
func (e *ServiceConfigEntry) CanRead(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.ServiceRead(e.Name, &authzContext) == acl.Allow
return authz.ToAllowAuthorizer().ServiceReadAllowed(e.Name, &authzContext)
}

func (e *ServiceConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *ServiceConfigEntry) CanWrite(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.ServiceWrite(e.Name, &authzContext) == acl.Allow
return authz.ToAllowAuthorizer().ServiceWriteAllowed(e.Name, &authzContext)
}

func (e *ServiceConfigEntry) GetRaftIndex() *RaftIndex {
Expand Down Expand Up @@ -306,14 +306,14 @@ func (e *ProxyConfigEntry) Validate() error {
return e.validateEnterpriseMeta()
}

func (e *ProxyConfigEntry) CanRead(authz acl.Authorizer) bool {
return true
func (e *ProxyConfigEntry) CanRead(authz acl.Authorizer) error {
return nil
}

func (e *ProxyConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *ProxyConfigEntry) CanWrite(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.MeshWrite(&authzContext) == acl.Allow
return authz.ToAllowAuthorizer().MeshWriteAllowed(&authzContext)
}

func (e *ProxyConfigEntry) GetRaftIndex() *RaftIndex {
Expand Down
28 changes: 14 additions & 14 deletions agent/structs/config_entry_discoverychain.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,11 +251,11 @@ func isValidHTTPMethod(method string) bool {
}
}

func (e *ServiceRouterConfigEntry) CanRead(authz acl.Authorizer) bool {
func (e *ServiceRouterConfigEntry) CanRead(authz acl.Authorizer) error {
return canReadDiscoveryChain(e, authz)
}

func (e *ServiceRouterConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *ServiceRouterConfigEntry) CanWrite(authz acl.Authorizer) error {
return canWriteDiscoveryChain(e, authz)
}

Expand Down Expand Up @@ -594,11 +594,11 @@ func scaleWeight(v float32) int {
return int(math.Round(float64(v * 100.0)))
}

func (e *ServiceSplitterConfigEntry) CanRead(authz acl.Authorizer) bool {
func (e *ServiceSplitterConfigEntry) CanRead(authz acl.Authorizer) error {
return canReadDiscoveryChain(e, authz)
}

func (e *ServiceSplitterConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *ServiceSplitterConfigEntry) CanWrite(authz acl.Authorizer) error {
return canWriteDiscoveryChain(e, authz)
}

Expand Down Expand Up @@ -1069,11 +1069,11 @@ func (e *ServiceResolverConfigEntry) Validate() error {
return nil
}

func (e *ServiceResolverConfigEntry) CanRead(authz acl.Authorizer) bool {
func (e *ServiceResolverConfigEntry) CanRead(authz acl.Authorizer) error {
return canReadDiscoveryChain(e, authz)
}

func (e *ServiceResolverConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *ServiceResolverConfigEntry) CanWrite(authz acl.Authorizer) error {
return canWriteDiscoveryChain(e, authz)
}

Expand Down Expand Up @@ -1300,22 +1300,22 @@ type discoveryChainConfigEntry interface {
ListRelatedServices() []ServiceID
}

func canReadDiscoveryChain(entry discoveryChainConfigEntry, authz acl.Authorizer) bool {
func canReadDiscoveryChain(entry discoveryChainConfigEntry, authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
entry.GetEnterpriseMeta().FillAuthzContext(&authzContext)
return authz.ServiceRead(entry.GetName(), &authzContext) == acl.Allow
return authz.ToAllowAuthorizer().ServiceReadAllowed(entry.GetName(), &authzContext)
}

func canWriteDiscoveryChain(entry discoveryChainConfigEntry, authz acl.Authorizer) bool {
func canWriteDiscoveryChain(entry discoveryChainConfigEntry, authz acl.Authorizer) error {
entryID := NewServiceID(entry.GetName(), entry.GetEnterpriseMeta())

var authzContext acl.AuthorizerContext
entryID.FillAuthzContext(&authzContext)

name := entry.GetName()

if authz.ServiceWrite(name, &authzContext) != acl.Allow {
return false
if err := authz.ToAllowAuthorizer().ServiceWriteAllowed(name, &authzContext); err != nil {
return err
}

for _, svc := range entry.ListRelatedServices() {
Expand All @@ -1326,11 +1326,11 @@ func canWriteDiscoveryChain(entry discoveryChainConfigEntry, authz acl.Authorize
svc.FillAuthzContext(&authzContext)
// You only need read on related services to redirect traffic flow for
// your own service.
if authz.ServiceRead(svc.ID, &authzContext) != acl.Allow {
return false
if err := authz.ToAllowAuthorizer().ServiceReadAllowed(svc.ID, &authzContext); err != nil {
return err
}
}
return true
return nil
}

// DiscoveryChainRequest is used when requesting the discovery chain for a
Expand Down
8 changes: 4 additions & 4 deletions agent/structs/config_entry_exports.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,16 +136,16 @@ func (e *ExportedServicesConfigEntry) Validate() error {
return nil
}

func (e *ExportedServicesConfigEntry) CanRead(authz acl.Authorizer) bool {
func (e *ExportedServicesConfigEntry) CanRead(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.MeshRead(&authzContext) == acl.Allow
return authz.ToAllowAuthorizer().MeshReadAllowed(&authzContext)
}

func (e *ExportedServicesConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *ExportedServicesConfigEntry) CanWrite(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.MeshWrite(&authzContext) == acl.Allow
return authz.ToAllowAuthorizer().MeshWriteAllowed(&authzContext)
}

func (e *ExportedServicesConfigEntry) GetRaftIndex() *RaftIndex {
Expand Down
16 changes: 8 additions & 8 deletions agent/structs/config_entry_gateways.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,16 +430,16 @@ func (e *IngressGatewayConfigEntry) ListRelatedServices() []ServiceID {
return out
}

func (e *IngressGatewayConfigEntry) CanRead(authz acl.Authorizer) bool {
func (e *IngressGatewayConfigEntry) CanRead(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.ServiceRead(e.Name, &authzContext) == acl.Allow
return authz.ToAllowAuthorizer().ServiceReadAllowed(e.Name, &authzContext)
}

func (e *IngressGatewayConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *IngressGatewayConfigEntry) CanWrite(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.MeshWrite(&authzContext) == acl.Allow
return authz.ToAllowAuthorizer().MeshWriteAllowed(&authzContext)
}

func (e *IngressGatewayConfigEntry) GetRaftIndex() *RaftIndex {
Expand Down Expand Up @@ -572,16 +572,16 @@ func (e *TerminatingGatewayConfigEntry) Validate() error {
return nil
}

func (e *TerminatingGatewayConfigEntry) CanRead(authz acl.Authorizer) bool {
func (e *TerminatingGatewayConfigEntry) CanRead(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.ServiceRead(e.Name, &authzContext) == acl.Allow
return authz.ToAllowAuthorizer().ServiceReadAllowed(e.Name, &authzContext)
}

func (e *TerminatingGatewayConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *TerminatingGatewayConfigEntry) CanWrite(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.MeshWrite(&authzContext) == acl.Allow
return authz.ToAllowAuthorizer().MeshWriteAllowed(&authzContext)
}

func (e *TerminatingGatewayConfigEntry) GetRaftIndex() *RaftIndex {
Expand Down
8 changes: 4 additions & 4 deletions agent/structs/config_entry_intentions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -789,16 +789,16 @@ func (e *ServiceIntentionsConfigEntry) GetEnterpriseMeta() *EnterpriseMeta {
return &e.EnterpriseMeta
}

func (e *ServiceIntentionsConfigEntry) CanRead(authz acl.Authorizer) bool {
func (e *ServiceIntentionsConfigEntry) CanRead(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.IntentionRead(e.GetName(), &authzContext) == acl.Allow
return authz.ToAllowAuthorizer().IntentionReadAllowed(e.GetName(), &authzContext)
}

func (e *ServiceIntentionsConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *ServiceIntentionsConfigEntry) CanWrite(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.IntentionWrite(e.GetName(), &authzContext) == acl.Allow
return authz.ToAllowAuthorizer().IntentionWriteAllowed(e.GetName(), &authzContext)
}

func MigrateIntentions(ixns Intentions) []*ServiceIntentionsConfigEntry {
Expand Down
8 changes: 4 additions & 4 deletions agent/structs/config_entry_mesh.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,14 @@ func (e *MeshConfigEntry) Validate() error {
return e.validateEnterpriseMeta()
}

func (e *MeshConfigEntry) CanRead(authz acl.Authorizer) bool {
return true
func (e *MeshConfigEntry) CanRead(authz acl.Authorizer) error {
return nil
}

func (e *MeshConfigEntry) CanWrite(authz acl.Authorizer) bool {
func (e *MeshConfigEntry) CanWrite(authz acl.Authorizer) error {
var authzContext acl.AuthorizerContext
e.FillAuthzContext(&authzContext)
return authz.MeshWrite(&authzContext) == acl.Allow
return authz.ToAllowAuthorizer().MeshWriteAllowed(&authzContext)
}

func (e *MeshConfigEntry) GetRaftIndex() *RaftIndex {
Expand Down
16 changes: 14 additions & 2 deletions agent/structs/config_entry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,20 @@ func testConfigEntries_ListRelatedServices_AndACLs(t *testing.T, cases []configE
for _, a := range tc.expectACLs {
require.NotEmpty(t, a.name)
t.Run(a.name, func(t *testing.T) {
require.Equal(t, a.canRead, tc.entry.CanRead(a.authorizer), "unexpected CanRead result")
require.Equal(t, a.canWrite, tc.entry.CanWrite(a.authorizer), "unexpected CanWrite result")
canRead := tc.entry.CanRead(a.authorizer)
if a.canRead {
require.Nil(t, canRead)
} else {
require.Error(t, canRead)
require.True(t, acl.IsErrPermissionDenied(canRead))
}
canWrite := tc.entry.CanWrite(a.authorizer)
if a.canWrite {
require.Nil(t, canWrite)
} else {
require.Error(t, canWrite)
require.True(t, acl.IsErrPermissionDenied(canWrite))
}
})
}
}
Expand Down

0 comments on commit 676ea58

Please sign in to comment.