Use correct mount accessor when refreshing external group memberships (…

…#11506) (#11516) * Use correct mount accessor when refreshing external group memberships * Add CL * Handle the renew case properly
hashicorp · May 3, 2021 · 55a61c7 · 55a61c7
1 parent 011e51e
commit 55a61c7
Show file tree

Hide file tree

Showing 5 changed files with 136 additions and 12 deletions.
diff --git a/changelog/11506.txt b/changelog/11506.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+identity: Use correct mount accessor when refreshing external group memberships.
+```
diff --git a/vault/expiration.go b/vault/expiration.go
@@ -1301,7 +1301,11 @@ func (m *ExpirationManager) RenewToken(ctx context.Context, req *logical.Request
 
 	// Refresh groups
 	if resp.Auth.EntityID != "" && m.core.identityStore != nil {
-		validAliases, err := m.core.identityStore.refreshExternalGroupMembershipsByEntityID(ctx, resp.Auth.EntityID, resp.Auth.GroupAliases)
+		mountAccessor := ""
+		if resp.Auth.Alias != nil {
+			mountAccessor = resp.Auth.Alias.MountAccessor
+		}
+		validAliases, err := m.core.identityStore.refreshExternalGroupMembershipsByEntityID(ctx, resp.Auth.EntityID, resp.Auth.GroupAliases, mountAccessor)
 		if err != nil {
 			return nil, err
 		}

diff --git a/vault/external_tests/identity/identity_test.go b/vault/external_tests/identity/identity_test.go
@@ -4,19 +4,141 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/hashicorp/vault/api"
+	"github.com/hashicorp/vault/sdk/helper/ldaputil"
+	"github.com/hashicorp/vault/sdk/helper/strutil"
+	"github.com/hashicorp/vault/sdk/logical"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/hashicorp/vault/helper/testhelpers/teststorage"
+
 	"github.com/go-ldap/ldap/v3"
 	log "github.com/hashicorp/go-hclog"
-	"github.com/hashicorp/vault/api"
 	ldapcred "github.com/hashicorp/vault/builtin/credential/ldap"
 	"github.com/hashicorp/vault/helper/namespace"
 	ldaphelper "github.com/hashicorp/vault/helper/testhelpers/ldap"
 	vaulthttp "github.com/hashicorp/vault/http"
-	"github.com/hashicorp/vault/sdk/helper/ldaputil"
-	"github.com/hashicorp/vault/sdk/helper/strutil"
-	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/hashicorp/vault/vault"
 )
 
+func TestIdentityStore_ExternalGroupMemberships_DifferentMounts(t *testing.T) {
+	coreConfig := &vault.CoreConfig{
+		CredentialBackends: map[string]logical.Factory{
+			"ldap": ldapcred.Factory,
+		},
+	}
+	conf, opts := teststorage.ClusterSetup(coreConfig, nil, nil)
+	cluster := vault.NewTestCluster(t, conf, opts)
+	cluster.Start()
+	defer cluster.Cleanup()
+
+	core := cluster.Cores[0].Core
+	client := cluster.Cores[0].Client
+	vault.TestWaitActive(t, core)
+
+	// Create a entity
+	secret, err := client.Logical().Write("identity/entity", map[string]interface{}{
+		"name": "testentityname",
+	})
+	require.NoError(t, err)
+	entityID := secret.Data["id"].(string)
+
+	cleanup, config1 := ldaphelper.PrepareTestContainer(t, "latest")
+	defer cleanup()
+
+	cleanup2, config2 := ldaphelper.PrepareTestContainer(t, "latest")
+	defer cleanup2()
+
+	setupFunc := func(path string, cfg *ldaputil.ConfigEntry) string {
+		// Create an external group
+		resp, err := client.Logical().Write("identity/group", map[string]interface{}{
+			"type":     "external",
+			"name":     path + "ldap_admin_staff",
+			"policies": []string{"admin-policy"},
+		})
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		require.NotNil(t, resp.Data)
+		groupID := resp.Data["id"].(string)
+
+		// Enable LDAP mount in Vault
+		err = client.Sys().EnableAuthWithOptions(path, &api.EnableAuthOptions{
+			Type: "ldap",
+		})
+		require.NoError(t, err)
+
+		// Take out its accessor
+		auth, err := client.Sys().ListAuth()
+		require.NoError(t, err)
+		accessor := auth[path+"/"].Accessor
+		require.NotEmpty(t, accessor)
+
+		// Create an external group alias
+		resp, err = client.Logical().Write("identity/group-alias", map[string]interface{}{
+			"name":           "admin_staff",
+			"canonical_id":   groupID,
+			"mount_accessor": accessor,
+		})
+
+		// Create a user in Vault
+		_, err = client.Logical().Write("auth/"+path+"/users/hermes conrad", map[string]interface{}{
+			"password": "hermes",
+		})
+		require.NoError(t, err)
+
+		// Create an entity alias
+		client.Logical().Write("identity/entity-alias", map[string]interface{}{
+			"name":           "hermes conrad",
+			"canonical_id":   entityID,
+			"mount_accessor": accessor,
+		})
+
+		// Configure LDAP auth
+		secret, err = client.Logical().Write("auth/"+path+"/config", map[string]interface{}{
+			"url":       cfg.Url,
+			"userattr":  cfg.UserAttr,
+			"userdn":    cfg.UserDN,
+			"groupdn":   cfg.GroupDN,
+			"groupattr": cfg.GroupAttr,
+			"binddn":    cfg.BindDN,
+			"bindpass":  cfg.BindPassword,
+		})
+		require.NoError(t, err)
+
+		secret, err = client.Logical().Write("auth/"+path+"/login/hermes conrad", map[string]interface{}{
+			"password": "hermes",
+		})
+		require.NoError(t, err)
+
+		policies, err := secret.TokenPolicies()
+		require.NoError(t, err)
+		require.Contains(t, policies, "admin-policy")
+
+		secret, err = client.Logical().Read("identity/group/id/" + groupID)
+		require.NoError(t, err)
+		require.Contains(t, secret.Data["member_entity_ids"], entityID)
+
+		return groupID
+	}
+	groupID1 := setupFunc("ldap", config1)
+	groupID2 := setupFunc("ldap2", config2)
+
+	// Remove hermes conrad from admin_staff group
+	removeLdapGroupMember(t, config1, "admin_staff", "hermes conrad")
+	secret, err = client.Logical().Write("auth/ldap/login/hermes conrad", map[string]interface{}{
+		"password": "hermes",
+	})
+
+	secret, err = client.Logical().Read("identity/group/id/" + groupID1)
+	require.NoError(t, err)
+	require.NotContains(t, secret.Data["member_entity_ids"], entityID)
+
+	secret, err = client.Logical().Read("identity/group/id/" + groupID2)
+	require.NoError(t, err)
+	require.Contains(t, secret.Data["member_entity_ids"], entityID)
+}
+
 func TestIdentityStore_Integ_GroupAliases(t *testing.T) {
 	t.Parallel()
 

diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go
@@ -1886,7 +1886,7 @@ func (i *IdentityStore) MemDBGroupByAliasID(aliasID string, clone bool) (*identi
 	return i.MemDBGroupByAliasIDInTxn(txn, aliasID, clone)
 }
 
-func (i *IdentityStore) refreshExternalGroupMembershipsByEntityID(ctx context.Context, entityID string, groupAliases []*logical.Alias) ([]*logical.Alias, error) {
+func (i *IdentityStore) refreshExternalGroupMembershipsByEntityID(ctx context.Context, entityID string, groupAliases []*logical.Alias, mountAccessor string) ([]*logical.Alias, error) {
 	defer metrics.MeasureSince([]string{"identity", "refresh_external_groups"}, time.Now())
 
 	if entityID == "" {
@@ -1908,11 +1908,6 @@ func (i *IdentityStore) refreshExternalGroupMembershipsByEntityID(ctx context.Co
 			return false, nil, err
 		}
 
-		mountAccessor := ""
-		if len(groupAliases) != 0 {
-			mountAccessor = groupAliases[0].MountAccessor
-		}
-
 		var newGroups []*identity.Group
 		var validAliases []*logical.Alias
 		for _, alias := range groupAliases {

diff --git a/vault/request_handling.go b/vault/request_handling.go
@@ -1203,7 +1203,7 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re
 			}
 
 			auth.EntityID = entity.ID
-			validAliases, err := c.identityStore.refreshExternalGroupMembershipsByEntityID(ctx, auth.EntityID, auth.GroupAliases)
+			validAliases, err := c.identityStore.refreshExternalGroupMembershipsByEntityID(ctx, auth.EntityID, auth.GroupAliases, req.MountAccessor)
 			if err != nil {
 				return nil, nil, err
 			}