luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

acl: a role binding rule for a role that does not exist should be ignored (#5778) 

I wrote the docs under this assumption but completely forgot to actually
enforce it.
This commit is contained in:
R.B. Boyer 2019-05-03 14:22:44 -05:00 committed by GitHub
parent 0566fa559f
commit 372bb06c83
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 189 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
agent/consul/acl_authmethod.go
View File

@ -136,9 +136,16 @@ func (s *Server) evaluateRoleBindings(
})
case structs.BindingRuleBindTypeRole:
roleLinks = append(roleLinks, structs.ACLTokenRoleLink{
Name: bindName,
})
_, role, err := s.fsm.State().ACLRoleGetByName(nil, bindName)
if err != nil {
return nil, nil, err
}
if role != nil {
roleLinks = append(roleLinks, structs.ACLTokenRoleLink{
ID: role.ID,
})
}
default:
// skip unknown bind type; don't grant privileges

22
agent/consul/acl_endpoint.go
View File

@ -581,6 +581,14 @@ func (a *ACL) tokenSetInternal(args *structs.ACLTokenSetRequest, reply *structs.
CAS: false,
}
if fromLogin {
// Logins may attempt to link to roles that do not exist. These
// may be persisted, but don't allow tokens to be created that
// have no privileges (i.e. role links that point nowhere).
req.AllowMissingLinks = true
req.ProhibitUnprivileged = true
}
resp, err := a.srv.raftApply(structs.ACLTokenSetRequestType, req)
if err != nil {
return fmt.Errorf("Failed to apply token write request: %v", err)
@ -1992,6 +2000,8 @@ func (a *ACL) Login(args *structs.ACLLoginRequest, reply *structs.ACLToken) erro
return err
}
// We try to prevent the creation of a useless token without taking a trip
// through the state store if we can.
if len(serviceIdentities) == 0 && len(roleLinks) == 0 {
return acl.ErrPermissionDenied
}
@ -2019,7 +2029,17 @@ func (a *ACL) Login(args *structs.ACLLoginRequest, reply *structs.ACLToken) erro
}
// 5. return token information like a TokenCreate would
return a.tokenSetInternal(&createReq, reply, true)
err = a.tokenSetInternal(&createReq, reply, true)
// If we were in a slight race with a role delete operation then we may
// still end up failing to insert an unprivileged token in the state
// machine instead. Return the same error as earlier so it doesn't
// actually matter which one prevents the insertion.
if err != nil && err.Error() == state.ErrTokenHasNoPrivileges.Error() {
return acl.ErrPermissionDenied
}
return err
}
func encodeLoginMeta(meta map[string]string) (string, error) {

114
agent/consul/acl_endpoint_test.go
View File

@ -4524,24 +4524,49 @@ func TestACLEndpoint_Login(t *testing.T) {
)
testauth.InstallSessionToken(
testSessionID,
"fake-db", // 1 rule
"fake-db", // 1 rule (service)
"default", "db", "def456",
)
testauth.InstallSessionToken(
testSessionID,
"fake-monolith", // 1 rule, must exist
"fake-vault", // 1 rule (role)
"default", "vault", "jkl012",
)
testauth.InstallSessionToken(
testSessionID,
"fake-monolith", // 2 rules (one of each)
"default", "monolith", "ghi789",
)
method, err := upsertTestAuthMethod(codec, "root", "dc1", testSessionID)
require.NoError(t, err)
// 'fake-db' rules
ruleDB, err := upsertTestBindingRule(
codec, "root", "dc1", method.Name,
"serviceaccount.namespace==default and serviceaccount.name==db",
structs.BindingRuleBindTypeService,
"method-${serviceaccount.name}",
)
require.NoError(t, err)
// 'fake-vault' rules
_, err = upsertTestBindingRule(
codec, "root", "dc1", method.Name,
"serviceaccount.namespace==default and serviceaccount.name==vault",
structs.BindingRuleBindTypeRole,
"method-${serviceaccount.name}",
)
require.NoError(t, err)
// 'fake-monolith' rules
_, err = upsertTestBindingRule(
codec, "root", "dc1", method.Name,
"serviceaccount.namespace==default and serviceaccount.name==monolith",
structs.BindingRuleBindTypeService,
"method-${serviceaccount.name}",
)
require.NoError(t, err)
_, err = upsertTestBindingRule(
codec, "root", "dc1", method.Name,
"serviceaccount.namespace==default and serviceaccount.name==monolith",
@ -4607,27 +4632,27 @@ func TestACLEndpoint_Login(t *testing.T) {
requireErrorContains(t, acl.Login(&req, &resp), "Permission denied")
})
t.Run("valid method token 1 role binding must exist and does not exist", func(t *testing.T) {
t.Run("valid method token 1 role binding and role does not exist", func(t *testing.T) {
req := structs.ACLLoginRequest{
Auth: &structs.ACLLoginParams{
AuthMethod: method.Name,
BearerToken: "fake-monolith",
BearerToken: "fake-vault",
Meta: map[string]string{"pod": "pod1"},
},
Datacenter: "dc1",
}
resp := structs.ACLToken{}
require.Error(t, acl.Login(&req, &resp))
requireErrorContains(t, acl.Login(&req, &resp), "Permission denied")
})
// create the role so that the bindtype=existing login works
var monolithRoleID string
// create the role so that the bindtype=role login works
var vaultRoleID string
{
arg := structs.ACLRoleSetRequest{
Datacenter: "dc1",
Role: structs.ACLRole{
Name: "method-monolith",
Name: "method-vault",
},
WriteRequest: structs.WriteRequest{Token: "root"},
}
@ -4635,11 +4660,33 @@ func TestACLEndpoint_Login(t *testing.T) {
var out structs.ACLRole
require.NoError(t, acl.RoleSet(&arg, &out))
monolithRoleID = out.ID
vaultRoleID = out.ID
}
s1.purgeAuthMethodValidators()
t.Run("valid bearer token 1 role binding must exist and now exists", func(t *testing.T) {
t.Run("valid method token 1 role binding and role now exists", func(t *testing.T) {
req := structs.ACLLoginRequest{
Auth: &structs.ACLLoginParams{
AuthMethod: method.Name,
BearerToken: "fake-vault",
Meta: map[string]string{"pod": "pod1"},
},
Datacenter: "dc1",
}
resp := structs.ACLToken{}
require.NoError(t, acl.Login(&req, &resp))
require.Equal(t, method.Name, resp.AuthMethod)
require.Equal(t, `token created via login: {"pod":"pod1"}`, resp.Description)
require.True(t, resp.Local)
require.Len(t, resp.ServiceIdentities, 0)
require.Len(t, resp.Roles, 1)
role := resp.Roles[0]
require.Equal(t, vaultRoleID, role.ID)
require.Equal(t, "method-vault", role.Name)
})
t.Run("valid method token 1 service binding 1 role binding and role does not exist", func(t *testing.T) {
req := structs.ACLLoginRequest{
Auth: &structs.ACLLoginParams{
AuthMethod: method.Name,
@ -4655,11 +4702,54 @@ func TestACLEndpoint_Login(t *testing.T) {
require.Equal(t, method.Name, resp.AuthMethod)
require.Equal(t, `token created via login: {"pod":"pod1"}`, resp.Description)
require.True(t, resp.Local)
require.Len(t, resp.ServiceIdentities, 0)
require.Len(t, resp.ServiceIdentities, 1)
require.Len(t, resp.Roles, 0)
svcid := resp.ServiceIdentities[0]
require.Len(t, svcid.Datacenters, 0)
require.Equal(t, "method-monolith", svcid.ServiceName)
})
// create the role so that the bindtype=role login works
var monolithRoleID string
{
arg := structs.ACLRoleSetRequest{
Datacenter: "dc1",
Role: structs.ACLRole{
Name: "method-monolith",
},
WriteRequest: structs.WriteRequest{Token: "root"},
}
var out structs.ACLRole
require.NoError(t, acl.RoleSet(&arg, &out))
monolithRoleID = out.ID
}
t.Run("valid method token 1 service binding 1 role binding and role now exists", func(t *testing.T) {
req := structs.ACLLoginRequest{
Auth: &structs.ACLLoginParams{
AuthMethod: method.Name,
BearerToken: "fake-monolith",
Meta: map[string]string{"pod": "pod1"},
},
Datacenter: "dc1",
}
resp := structs.ACLToken{}
require.NoError(t, acl.Login(&req, &resp))
require.Equal(t, method.Name, resp.AuthMethod)
require.Equal(t, `token created via login: {"pod":"pod1"}`, resp.Description)
require.True(t, resp.Local)
require.Len(t, resp.ServiceIdentities, 1)
require.Len(t, resp.Roles, 1)
role := resp.Roles[0]
require.Equal(t, monolithRoleID, role.ID)
require.Equal(t, "method-monolith", role.Name)
svcid := resp.ServiceIdentities[0]
require.Len(t, svcid.Datacenters, 0)
require.Equal(t, "method-monolith", svcid.ServiceName)
})
t.Run("valid bearer token 1 service binding", func(t *testing.T) {

2
agent/consul/fsm/commands_oss.go
View File

@ -391,7 +391,7 @@ func (c *FSM) applyACLTokenSetOperation(buf []byte, index uint64) interface{} {
defer metrics.MeasureSinceWithLabels([]string{"fsm", "acl", "token"}, time.Now(),
[]metrics.Label{{Name: "op", Value: "upsert"}})
return c.state.ACLTokenBatchSet(index, req.Tokens, req.CAS, req.AllowMissingLinks)
return c.state.ACLTokenBatchSet(index, req.Tokens, req.CAS, req.AllowMissingLinks, req.ProhibitUnprivileged)
}
func (c *FSM) applyACLTokenDeleteOperation(buf []byte, index uint64) interface{} {

46
agent/consul/state/acl.go
View File

@ -528,7 +528,7 @@ func (s *Store) ACLBootstrap(idx, resetIndex uint64, token *structs.ACLToken, le
}
}
if err := s.aclTokenSetTxn(tx, idx, token, false, false, legacy); err != nil {
if err := s.aclTokenSetTxn(tx, idx, token, false, false, false, legacy); err != nil {
return fmt.Errorf("failed inserting bootstrap token: %v", err)
}
if err := indexUpdateMaxTxn(tx, idx, "acl-tokens"); err != nil {
@ -560,26 +560,28 @@ func (s *Store) CanBootstrapACLToken() (bool, uint64, error) {
return false, out.(*IndexEntry).Value, nil
}
func (s *Store) resolveTokenPolicyLinks(tx *memdb.Txn, token *structs.ACLToken, allowMissing bool) error {
func (s *Store) resolveTokenPolicyLinks(tx *memdb.Txn, token *structs.ACLToken, allowMissing bool) (int, error) {
var numValid int
for linkIndex, link := range token.Policies {
if link.ID != "" {
policy, err := s.getPolicyWithTxn(tx, nil, link.ID, "id")
if err != nil {
return err
return 0, err
}
if policy != nil {
// the name doesn't matter here
token.Policies[linkIndex].Name = policy.Name
numValid++
} else if !allowMissing {
return fmt.Errorf("No such policy with ID: %s", link.ID)
return 0, fmt.Errorf("No such policy with ID: %s", link.ID)
}
} else {
return fmt.Errorf("Encountered a Token with policies linked by Name in the state store")
return 0, fmt.Errorf("Encountered a Token with policies linked by Name in the state store")
}
}
return nil
return numValid, nil
}
// fixupTokenPolicyLinks is to be used when retrieving tokens from memdb. The policy links could have gotten
@ -632,26 +634,28 @@ func (s *Store) fixupTokenPolicyLinks(tx *memdb.Txn, original *structs.ACLToken)
return token, nil
}
func (s *Store) resolveTokenRoleLinks(tx *memdb.Txn, token *structs.ACLToken, allowMissing bool) error {
func (s *Store) resolveTokenRoleLinks(tx *memdb.Txn, token *structs.ACLToken, allowMissing bool) (int, error) {
var numValid int
for linkIndex, link := range token.Roles {
if link.ID != "" {
role, err := s.getRoleWithTxn(tx, nil, link.ID, "id")
if err != nil {
return err
return 0, err
}
if role != nil {
// the name doesn't matter here
token.Roles[linkIndex].Name = role.Name
numValid++
} else if !allowMissing {
return fmt.Errorf("No such role with ID: %s", link.ID)
return 0, fmt.Errorf("No such role with ID: %s", link.ID)
}
} else {
return fmt.Errorf("Encountered a Token with roles linked by Name in the state store")
return 0, fmt.Errorf("Encountered a Token with roles linked by Name in the state store")
}
}
return nil
return numValid, nil
}
// fixupTokenRoleLinks is to be used when retrieving tokens from memdb. The role links could have gotten
@ -782,7 +786,7 @@ func (s *Store) ACLTokenSet(idx uint64, token *structs.ACLToken, legacy bool) er
defer tx.Abort()
// Call set on the ACL
if err := s.aclTokenSetTxn(tx, idx, token, false, false, legacy); err != nil {
if err := s.aclTokenSetTxn(tx, idx, token, false, false, false, legacy); err != nil {
return err
}
@ -794,12 +798,12 @@ func (s *Store) ACLTokenSet(idx uint64, token *structs.ACLToken, legacy bool) er
return nil
}
func (s *Store) ACLTokenBatchSet(idx uint64, tokens structs.ACLTokens, cas, allowMissingPolicyAndRoleIDs bool) error {
func (s *Store) ACLTokenBatchSet(idx uint64, tokens structs.ACLTokens, cas, allowMissingPolicyAndRoleIDs, prohibitUnprivileged bool) error {
tx := s.db.Txn(true)
defer tx.Abort()
for _, token := range tokens {
if err := s.aclTokenSetTxn(tx, idx, token, cas, allowMissingPolicyAndRoleIDs, false); err != nil {
if err := s.aclTokenSetTxn(tx, idx, token, cas, allowMissingPolicyAndRoleIDs, prohibitUnprivileged, false); err != nil {
return err
}
}
@ -814,7 +818,7 @@ func (s *Store) ACLTokenBatchSet(idx uint64, tokens structs.ACLTokens, cas, allo
// aclTokenSetTxn is the inner method used to insert an ACL token with the
// proper indexes into the state store.
func (s *Store) aclTokenSetTxn(tx *memdb.Txn, idx uint64, token *structs.ACLToken, cas, allowMissingPolicyAndRoleIDs, legacy bool) error {
func (s *Store) aclTokenSetTxn(tx *memdb.Txn, idx uint64, token *structs.ACLToken, cas, allowMissingPolicyAndRoleIDs, prohibitUnprivileged, legacy bool) error {
// Check that the ID is set
if token.SecretID == "" {
return ErrMissingACLTokenSecret
@ -871,11 +875,13 @@ func (s *Store) aclTokenSetTxn(tx *memdb.Txn, idx uint64, token *structs.ACLToke
token.AccessorID = original.AccessorID
}
if err := s.resolveTokenPolicyLinks(tx, token, allowMissingPolicyAndRoleIDs); err != nil {
var numValidPolicies int
if numValidPolicies, err = s.resolveTokenPolicyLinks(tx, token, allowMissingPolicyAndRoleIDs); err != nil {
return err
}
if err := s.resolveTokenRoleLinks(tx, token, allowMissingPolicyAndRoleIDs); err != nil {
var numValidRoles int
if numValidRoles, err = s.resolveTokenRoleLinks(tx, token, allowMissingPolicyAndRoleIDs); err != nil {
return err
}
@ -894,6 +900,12 @@ func (s *Store) aclTokenSetTxn(tx *memdb.Txn, idx uint64, token *structs.ACLToke
}
}
if prohibitUnprivileged {
if numValidRoles == 0 && numValidPolicies == 0 && len(token.ServiceIdentities) == 0 {
return ErrTokenHasNoPrivileges
}
}
// Set the indexes
if original != nil {
if original.AccessorID != "" && token.AccessorID != original.AccessorID {

34
agent/consul/state/acl_test.go
View File

@ -619,7 +619,7 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(2, tokens, true, false))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, true, false, false))
_, token, err := s.ACLTokenGetByAccessor(nil, tokens[0].AccessorID)
require.NoError(t, err)
@ -640,7 +640,7 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(5, tokens, true, false))
require.NoError(t, s.ACLTokenBatchSet(5, tokens, true, false, false))
updated := structs.ACLTokens{
&structs.ACLToken{
@ -651,7 +651,7 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(6, updated, true, false))
require.NoError(t, s.ACLTokenBatchSet(6, updated, true, false, false))
_, token, err := s.ACLTokenGetByAccessor(nil, tokens[0].AccessorID)
require.NoError(t, err)
@ -670,7 +670,7 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(5, tokens, true, false))
require.NoError(t, s.ACLTokenBatchSet(5, tokens, true, false, false))
updated := structs.ACLTokens{
&structs.ACLToken{
@ -680,7 +680,7 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(6, updated, true, false))
require.NoError(t, s.ACLTokenBatchSet(6, updated, true, false, false))
_, token, err := s.ACLTokenGetByAccessor(nil, tokens[0].AccessorID)
require.NoError(t, err)
@ -703,7 +703,7 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, false))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, false, false))
idx, rtokens, err := s.ACLTokenBatchGet(nil, []string{
"a4f68bd6-3af5-4f56-b764-3c6f20247879",
@ -734,7 +734,7 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, false))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, false, false))
updates := structs.ACLTokens{
&structs.ACLToken{
@ -759,7 +759,7 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(3, updates, false, false))
require.NoError(t, s.ACLTokenBatchSet(3, updates, false, false, false))
idx, rtokens, err := s.ACLTokenBatchGet(nil, []string{
"a4f68bd6-3af5-4f56-b764-3c6f20247879",
@ -806,9 +806,9 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.Error(t, s.ACLTokenBatchSet(2, tokens, false, false))
require.Error(t, s.ACLTokenBatchSet(2, tokens, false, false, false))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, true))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, true, false))
idx, rtokens, err := s.ACLTokenBatchGet(nil, []string{
"a4f68bd6-3af5-4f56-b764-3c6f20247879",
@ -845,9 +845,9 @@ func TestStateStore_ACLTokens_UpsertBatchRead(t *testing.T) {
},
}
require.Error(t, s.ACLTokenBatchSet(2, tokens, false, false))
require.Error(t, s.ACLTokenBatchSet(2, tokens, false, false, false))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, true))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, true, false))
idx, rtokens, err := s.ACLTokenBatchGet(nil, []string{
"a4f68bd6-3af5-4f56-b764-3c6f20247879",
@ -951,7 +951,7 @@ func TestStateStore_ACLTokens_ListUpgradeable(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(7, updates, false, false))
require.NoError(t, s.ACLTokenBatchSet(7, updates, false, false, false))
tokens, _, err = s.ACLTokenListUpgradeable(10)
require.NoError(t, err)
@ -1042,7 +1042,7 @@ func TestStateStore_ACLToken_List(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, false))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, false, false))
type testCase struct {
name string
@ -1563,7 +1563,7 @@ func TestStateStore_ACLToken_Delete(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, false))
require.NoError(t, s.ACLTokenBatchSet(2, tokens, false, false, false))
_, rtoken, err := s.ACLTokenGetByAccessor(nil, "f1093997-b6c7-496d-bfb8-6b1b1895641b")
require.NoError(t, err)
@ -3110,7 +3110,7 @@ func TestStateStore_ACLAuthMethod_Delete_RuleAndTokenCascade(t *testing.T) {
AuthMethod: "test-2",
},
}
require.NoError(t, s.ACLTokenBatchSet(4, tokens, false, false))
require.NoError(t, s.ACLTokenBatchSet(4, tokens, false, false, false))
// Delete one method.
require.NoError(t, s.ACLAuthMethodDeleteByName(4, "test-1"))
@ -3568,7 +3568,7 @@ func TestStateStore_ACLTokens_Snapshot_Restore(t *testing.T) {
},
}
require.NoError(t, s.ACLTokenBatchSet(4, tokens, false, false))
require.NoError(t, s.ACLTokenBatchSet(4, tokens, false, false, false))
// Snapshot the ACLs.
snap := s.Snapshot()

5
agent/consul/state/state_store.go
View File

@ -29,6 +29,11 @@ var (
// token with an empty AccessorID.
ErrMissingACLTokenAccessor = errors.New("Missing ACL Token AccessorID")
// ErrTokenHasNoPrivileges is returned when a token set is called on a
// token with no policies, roles, or service identities and the caller
// requires at least one to be set.
ErrTokenHasNoPrivileges = errors.New("Token has no privileges")
// ErrMissingACLPolicyID is returned when a policy set is called on a
// policy with an empty ID.
ErrMissingACLPolicyID = errors.New("Missing ACL Policy ID")

7
agent/structs/acl.go
View File

@ -1077,9 +1077,10 @@ func (r *ACLTokenBatchGetRequest) RequestDatacenter() string {
// This is particularly useful during token replication and during
// automatic legacy token upgrades.
type ACLTokenBatchSetRequest struct {
Tokens ACLTokens
CAS bool
AllowMissingLinks bool
Tokens ACLTokens
CAS bool
AllowMissingLinks bool
ProhibitUnprivileged bool
}
// ACLTokenBatchDeleteRequest is used only at the Raft layer