From de1101ee4cac001845f1aeb2ce844662eaafe81c Mon Sep 17 00:00:00 2001 From: Vishal Nayak Date: Fri, 30 Apr 2021 13:10:47 -0400 Subject: [PATCH 1/3] Use correct mount accessor when refreshing external group memberships --- vault/expiration.go | 2 +- .../external_tests/identity/identity_test.go | 130 +++++++++++++++++- vault/identity_store_util.go | 7 +- vault/request_handling.go | 2 +- 4 files changed, 129 insertions(+), 12 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index bff06d9515c8..ea999f4cf19b 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -1301,7 +1301,7 @@ 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) + validAliases, err := m.core.identityStore.refreshExternalGroupMembershipsByEntityID(ctx, resp.Auth.EntityID, resp.Auth.GroupAliases, req.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 index 0ea5dd8f2326..553fe2edbc81 100644 --- 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 index 1309dc931827..a868fd37469a 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -1884,7 +1884,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 == "" { @@ -1905,11 +1905,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 index fe0fe41177b0..4e8643b919c1 100644 --- 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 } From 944b51184762d69841f4d20f4b17e4df2274e7d0 Mon Sep 17 00:00:00 2001 From: Vishal Nayak Date: Fri, 30 Apr 2021 15:17:41 -0400 Subject: [PATCH 2/3] Add CL --- changelog/11506.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/11506.txt diff --git a/changelog/11506.txt b/changelog/11506.txt new file mode 100644 index 000000000000..15ab6f327705 --- /dev/null +++ b/changelog/11506.txt @@ -0,0 +1,3 @@ +```release-note:bug +identity: Use correct mount accessor when refreshing external group memberships. +``` From b9ab8dc353daaacb7d409b49bbc8f6f82c5b3722 Mon Sep 17 00:00:00 2001 From: Vishal Nayak Date: Fri, 30 Apr 2021 17:00:37 -0400 Subject: [PATCH 3/3] Handle the renew case properly --- vault/expiration.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/vault/expiration.go b/vault/expiration.go index ea999f4cf19b..e8478200eb4b 100644 --- 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, req.MountAccessor) + 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 }