Move the ACL logic into the ConfigEntry interface

agent/consul/config_endpoint.go

@@ -36,8 +36,8 @@ func (c *ConfigEntry) Apply(args *structs.ConfigEntryRequest, reply *struct{}) e
 	if err != nil {
 		return err
 	}
-	if err := verifyConfigWriteACL(rule, args.Entry.GetKind(), args.Entry.GetName()); err != nil {
+	if rule != nil && !args.Entry.VerifyWriteACL(rule) {
-		return err
+		return acl.ErrPermissionDenied
 	}
 
 	args.Op = structs.ConfigEntryUpsert
@@ -63,9 +63,15 @@ func (c *ConfigEntry) Get(args *structs.ConfigEntryQuery, reply *structs.Indexed
 	if err != nil {
 		return err
 	}
-	if err := verifyConfigReadACL(rule, args.Kind, args.Name); err != nil {
+
+	// Create a dummy config entry to check the ACL permissions.
+	lookupEntry, err := structs.MakeConfigEntry(args.Kind, args.Name)
+	if err != nil {
 		return err
 	}
+	if rule != nil && !lookupEntry.VerifyReadACL(rule) {
+		return acl.ErrPermissionDenied
+	}
 
 	return c.srv.blockingQuery(
 		&args.QueryOptions,
@@ -77,6 +83,11 @@ func (c *ConfigEntry) Get(args *structs.ConfigEntryQuery, reply *structs.Indexed
 			}
 
 			reply.Index = index
+			if entry == nil {
+				return nil
+			}
+
+			reply.Kind = args.Kind
 			reply.Entries = []structs.ConfigEntry{entry}
 			return nil
 		})
@@ -106,20 +117,15 @@ func (c *ConfigEntry) List(args *structs.ConfigEntryQuery, reply *structs.Indexe
 			}
 
 			// Filter the entries returned by ACL permissions.
-			// TODO(kyhavlov): should we handle the proxy config differently here since
-			// it's a singleton?
 			filteredEntries := make([]structs.ConfigEntry, 0, len(entries))
 			for _, entry := range entries {
-				if err := verifyConfigReadACL(rule, entry.GetKind(), entry.GetName()); err != nil {
+				if rule != nil && !entry.VerifyReadACL(rule) {
-					if acl.IsErrPermissionDenied(err) {
+					continue
-						continue
-					} else {
-						return err
-					}
 				}
 				filteredEntries = append(filteredEntries, entry)
 			}
 
+			reply.Kind = args.Kind
 			reply.Index = index
 			reply.Entries = filteredEntries
 			return nil
@@ -143,8 +149,8 @@ func (c *ConfigEntry) Delete(args *structs.ConfigEntryRequest, reply *struct{})
 	if err != nil {
 		return err
 	}
-	if err := verifyConfigWriteACL(rule, args.Entry.GetKind(), args.Entry.GetName()); err != nil {
+	if rule != nil && !args.Entry.VerifyWriteACL(rule) {
-		return err
+		return acl.ErrPermissionDenied
 	}
 
 	args.Op = structs.ConfigEntryDelete
@@ -218,49 +224,3 @@ func (c *ConfigEntry) ResolveServiceConfig(args *structs.ServiceConfigRequest, r
 			return nil
 		})
 }
-
-// verifyConfigReadACL checks whether the given ACL authorizer has permission
-// to read the config entry of the given kind/name.
-func verifyConfigReadACL(rule acl.Authorizer, kind, name string) error {
-	if rule == nil {
-		return nil
-	}
-
-	switch kind {
-	case structs.ServiceDefaults:
-		if !rule.ServiceRead(name) {
-			return acl.ErrPermissionDenied
-		}
-	case structs.ProxyDefaults:
-		if !rule.OperatorRead() {
-			return acl.ErrPermissionDenied
-		}
-	default:
-		return fmt.Errorf("unknown config entry type %q", kind)
-	}
-
-	return nil
-}
-
-// verifyConfigWriteACL checks whether the given ACL authorizer has permission
-// to update the config entry of the given kind/name.
-func verifyConfigWriteACL(rule acl.Authorizer, kind, name string) error {
-	if rule == nil {
-		return nil
-	}
-
-	switch kind {
-	case structs.ServiceDefaults:
-		if !rule.ServiceWrite(name, nil) {
-			return acl.ErrPermissionDenied
-		}
-	case structs.ProxyDefaults:
-		if !rule.OperatorWrite() {
-			return acl.ErrPermissionDenied
-		}
-	default:
-		return fmt.Errorf("unknown config entry type %q", kind)
-	}
-
-	return nil
-}

agent/consul/config_endpoint_test.go

@@ -200,7 +200,6 @@ operator = "read"
 		t.Fatalf("err: %v", err)
 	}
 
-	// Create a dummy service in the state store to look up.
 	// Create some dummy service/proxy configs to be looked up.
 	state := s1.fsm.State()
 	require.NoError(state.EnsureConfigEntry(1, &structs.ProxyConfigEntry{
@@ -233,18 +232,6 @@ operator = "read"
 	require.True(ok)
 	require.Equal("foo", serviceConf.Name)
 	require.Equal(structs.ServiceDefaults, serviceConf.Kind)
-
-	// Try to look up the proxy config with no token.
-	args.Kind = structs.ProxyDefaults
-	args.Name = structs.ProxyConfigGlobal
-	args.QueryOptions.Token = ""
-	err = msgpackrpc.CallWithCodec(codec, "ConfigEntry.Get", &args, &out)
-	if !acl.IsErrPermissionDenied(err) {
-		t.Fatalf("err: %v", err)
-	}
-
-	args.QueryOptions.Token = id
-	require.NoError(msgpackrpc.CallWithCodec(codec, "ConfigEntry.Get", &args, &out))
 }
 
 func TestConfigEntry_List(t *testing.T) {
@@ -282,6 +269,7 @@ func TestConfigEntry_List(t *testing.T) {
 	var out structs.IndexedConfigEntries
 	require.NoError(msgpackrpc.CallWithCodec(codec, "ConfigEntry.List", &args, &out))
 
+	expected.Kind = structs.ServiceDefaults
 	expected.QueryMeta = out.QueryMeta
 	require.Equal(expected, out)
 }

agent/structs/config_entry.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/go-msgpack/codec"
 )
 
@@ -25,6 +26,11 @@ type ConfigEntry interface {
 	Normalize() error
 	Validate() error
 
+	// VerifyReadACL and VerifyWriteACL return whether or not the given Authorizer
+	// has permission to read or write to the config entry, respectively.
+	VerifyReadACL(acl.Authorizer) bool
+	VerifyWriteACL(acl.Authorizer) bool
+
 	GetRaftIndex() *RaftIndex
 }
 
@@ -70,6 +76,14 @@ func (e *ServiceConfigEntry) Validate() error {
 	return nil
 }
 
+func (e *ServiceConfigEntry) VerifyReadACL(rule acl.Authorizer) bool {
+	return rule.ServiceRead(e.Name)
+}
+
+func (e *ServiceConfigEntry) VerifyWriteACL(rule acl.Authorizer) bool {
+	return rule.ServiceWrite(e.Name, nil)
+}
+
 func (e *ServiceConfigEntry) GetRaftIndex() *RaftIndex {
 	if e == nil {
 		return &RaftIndex{}
@@ -126,6 +140,14 @@ func (e *ProxyConfigEntry) Validate() error {
 	return nil
 }
 
+func (e *ProxyConfigEntry) VerifyReadACL(rule acl.Authorizer) bool {
+	return true
+}
+
+func (e *ProxyConfigEntry) VerifyWriteACL(rule acl.Authorizer) bool {
+	return rule.OperatorWrite()
+}
+
 func (e *ProxyConfigEntry) GetRaftIndex() *RaftIndex {
 	if e == nil {
 		return &RaftIndex{}
@@ -186,7 +208,7 @@ func (c *ConfigEntryRequest) UnmarshalBinary(data []byte) error {
 	}
 
 	// Then decode the real thing with appropriate kind of ConfigEntry
-	entry, err := makeConfigEntry(kind)
+	entry, err := MakeConfigEntry(kind, "")
 	if err != nil {
 		return err
 	}
@@ -206,12 +228,12 @@ func (c *ConfigEntryRequest) UnmarshalBinary(data []byte) error {
 	return nil
 }
 
-func makeConfigEntry(kind string) (ConfigEntry, error) {
+func MakeConfigEntry(kind, name string) (ConfigEntry, error) {
 	switch kind {
 	case ServiceDefaults:
-		return &ServiceConfigEntry{}, nil
+		return &ServiceConfigEntry{Name: name}, nil
 	case ProxyDefaults:
-		return &ProxyConfigEntry{}, nil
+		return &ProxyConfigEntry{Name: name}, nil
 	default:
 		return nil, fmt.Errorf("invalid config entry kind: %s", kind)
 	}

agent/structs/structs.go

@@ -1143,6 +1143,7 @@ type IndexedNodeDump struct {
 // IndexedConfigEntries has its own encoding logic which differs from
 // ConfigEntryRequest as it has to send a slice of ConfigEntry.
 type IndexedConfigEntries struct {
+	Kind    string
 	Entries []ConfigEntry
 	QueryMeta
 }
@@ -1153,16 +1154,16 @@ func (c *IndexedConfigEntries) MarshalBinary() (data []byte, err error) {
 	bs := make([]byte, 128)
 	enc := codec.NewEncoderBytes(&bs, msgpackHandle)
 
-	// Encode kinds of entries first
+	// Encode length.
 	err = enc.Encode(len(c.Entries))
 	if err != nil {
 		return nil, err
 	}
-	for _, entry := range c.Entries {
+
-		err = enc.Encode(entry.GetKind())
+	// Encode kind.
-		if err != nil {
+	err = enc.Encode(c.Kind)
-			return nil, err
+	if err != nil {
-		}
+		return nil, err
 	}
 
 	// Then actual value using alias trick to avoid infinite recursion
@@ -1179,23 +1180,23 @@ func (c *IndexedConfigEntries) MarshalBinary() (data []byte, err error) {
 }
 
 func (c *IndexedConfigEntries) UnmarshalBinary(data []byte) error {
-	// First decode the number of entries
+	// First decode the number of entries.
 	var numEntries int
 	dec := codec.NewDecoderBytes(data, msgpackHandle)
 	if err := dec.Decode(&numEntries); err != nil {
 		return err
 	}
 
+	// Next decode the kind.
+	var kind string
+	if err := dec.Decode(&kind); err != nil {
+		return err
+	}
+
+	// Then decode the slice of ConfigEntries
 	c.Entries = make([]ConfigEntry, numEntries)
 	for i := 0; i < numEntries; i++ {
-		// First decode the kind prefix
+		entry, err := MakeConfigEntry(kind, "")
-		var kind string
-		if err := dec.Decode(&kind); err != nil {
-			return err
-		}
-
-		// Then decode the real thing with appropriate kind of ConfigEntry
-		entry, err := makeConfigEntry(kind)
 		if err != nil {
 			return err
 		}