From dfe7bccc6c0daf7a1a584a301bf218fac3f90f49 Mon Sep 17 00:00:00 2001 From: ncabatoff Date: Mon, 23 Mar 2020 18:00:26 -0400 Subject: [PATCH] Make sure if a user gets removed from all groups in the external system, Vault updates itself accordingly. This is CVE-2020-10660. (#8606) --- helper/testhelpers/ldap/ldaphelper.go | 2 +- vault/expiration.go | 4 +- .../external_tests/identity/identity_test.go | 321 ++++++++++++++---- vault/request_handling.go | 10 +- 4 files changed, 267 insertions(+), 70 deletions(-) diff --git a/helper/testhelpers/ldap/ldaphelper.go b/helper/testhelpers/ldap/ldaphelper.go index 5571565caa1d..d7ef2281f11e 100644 --- a/helper/testhelpers/ldap/ldaphelper.go +++ b/helper/testhelpers/ldap/ldaphelper.go @@ -47,7 +47,7 @@ func PrepareTestContainer(t *testing.T, version string) (cleanup func(), cfg *ld cfg.BindDN = "cn=admin,dc=planetexpress,dc=com" cfg.BindPassword = "GoodNewsEveryone" cfg.GroupDN = "ou=people,dc=planetexpress,dc=com" - cfg.GroupAttr = "memberOf" + cfg.GroupAttr = "cn" cfg.RequestTimeout = 60 conn, err := client.DialLDAP(cfg) if err != nil { diff --git a/vault/expiration.go b/vault/expiration.go index 303b9058d551..ec0246abaeaf 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -1004,9 +1004,7 @@ func (m *ExpirationManager) RenewToken(ctx context.Context, req *logical.Request resp.Auth.ClientToken = te.ID // Refresh groups - if resp.Auth.EntityID != "" && - resp.Auth.GroupAliases != nil && - m.core.identityStore != nil { + if resp.Auth.EntityID != "" && m.core.identityStore != nil { validAliases, err := m.core.identityStore.refreshExternalGroupMembershipsByEntityID(ctx, resp.Auth.EntityID, resp.Auth.GroupAliases) 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 8ff561c1fd08..ed2897e73c2e 100644 --- a/vault/external_tests/identity/identity_test.go +++ b/vault/external_tests/identity/identity_test.go @@ -1,6 +1,7 @@ package identity import ( + "fmt" "testing" "github.com/go-ldap/ldap/v3" @@ -11,11 +12,15 @@ import ( 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/logging" + "github.com/hashicorp/vault/sdk/helper/strutil" "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/vault" ) func TestIdentityStore_Integ_GroupAliases(t *testing.T) { + t.Parallel() + var err error coreConfig := &vault.CoreConfig{ DisableMlock: true, @@ -189,60 +194,13 @@ func TestIdentityStore_Integ_GroupAliases(t *testing.T) { // Re-read the admin_staff, ship_crew and devops group. This entity ID should have // been added to admin_staff but not ship_crew. - assertMember := func(groupName, groupID string, expectFound bool) { - secret, err = client.Logical().Read("identity/group/id/" + groupID) - if err != nil { - t.Fatal(err) - } - groupMap := secret.Data - found := false - for _, entityIDRaw := range groupMap["member_entity_ids"].([]interface{}) { - if entityIDRaw.(string) == entityID { - found = true - } - } - if found != expectFound { - negation := "" - if !expectFound { - negation = "not " - } - t.Fatalf("expected entity ID %q to %sbe part of %q group", entityID, negation, groupName) - } - } - - assertMember("ship_crew", shipCrewGroupID, false) - assertMember("admin_staff", adminStaffGroupID, true) - assertMember("devops", devopsGroupID, true) - assertMember("engineer", devopsGroupID, true) + assertMember(t, client, entityID, "ship_crew", shipCrewGroupID, false) + assertMember(t, client, entityID, "admin_staff", adminStaffGroupID, true) + assertMember(t, client, entityID, "devops", devopsGroupID, true) + assertMember(t, client, entityID, "engineer", devopsGroupID, true) // Now add Hermes to ship_crew - { - logger := log.New(nil) - ldapClient := ldaputil.Client{LDAP: ldaputil.NewLDAP(), Logger: logger} - // LDAP server won't accept changes unless we connect with TLS. This - // isn't the default config returned by PrepareTestContainer because - // the Vault LDAP backend won't work with it, even with InsecureTLS, - // because the ServerName should be planetexpress.com and not localhost. - conn, err := ldapClient.DialLDAP(cfg) - if err != nil { - t.Fatal(err) - } - defer conn.Close() - - err = conn.Bind(cfg.BindDN, cfg.BindPassword) - if err != nil { - t.Fatal(err) - } - - hermesDn := "cn=Hermes Conrad,ou=people,dc=planetexpress,dc=com" - shipCrewDn := "cn=ship_crew,ou=people,dc=planetexpress,dc=com" - ldapreq := ldap.ModifyRequest{DN: shipCrewDn} - ldapreq.Add("member", []string{hermesDn}) - err = conn.Modify(&ldapreq) - if err != nil { - t.Fatal(err) - } - } + addLdapGroupMember(t, cfg, "ship_crew", "hermes conrad") // Re-login with LDAP secret, err = client.Logical().Write("auth/ldap/login/hermes conrad", map[string]interface{}{ @@ -253,10 +211,10 @@ func TestIdentityStore_Integ_GroupAliases(t *testing.T) { } // Hermes should now be in ship_crew external group - assertMember("ship_crew", shipCrewGroupID, true) - assertMember("admin_staff", adminStaffGroupID, true) - assertMember("devops", devopsGroupID, true) - assertMember("engineer", devopsGroupID, true) + assertMember(t, client, entityID, "ship_crew", shipCrewGroupID, true) + assertMember(t, client, entityID, "admin_staff", adminStaffGroupID, true) + assertMember(t, client, entityID, "devops", devopsGroupID, true) + assertMember(t, client, entityID, "engineer", devopsGroupID, true) identityStore := cores[0].IdentityStore() @@ -330,10 +288,10 @@ func TestIdentityStore_Integ_GroupAliases(t *testing.T) { t.Fatal(err) } - assertMember("ship_crew", shipCrewGroupID, true) - assertMember("admin_staff", adminStaffGroupID, true) - assertMember("devops", devopsGroupID, true) - assertMember("engineer", devopsGroupID, true) + assertMember(t, client, entityID, "ship_crew", shipCrewGroupID, true) + assertMember(t, client, entityID, "admin_staff", adminStaffGroupID, true) + assertMember(t, client, entityID, "devops", devopsGroupID, true) + assertMember(t, client, entityID, "engineer", devopsGroupID, true) // Remove user hermes conrad from the devops group in LDAP backend secret, err = client.Logical().Write("auth/ldap/users/hermes conrad", map[string]interface{}{ @@ -359,3 +317,246 @@ func TestIdentityStore_Integ_GroupAliases(t *testing.T) { t.Fatalf("failed to remove entity ID from the group") } } + +func TestIdentityStore_Integ_RemoveFromExternalGroup(t *testing.T) { + t.Parallel() + var err error + coreConfig := &vault.CoreConfig{ + DisableMlock: true, + DisableCache: true, + Logger: log.NewNullLogger(), + CredentialBackends: map[string]logical.Factory{ + "ldap": ldapcred.Factory, + }, + } + + cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + Logger: logging.NewVaultLogger(log.Debug), + }) + + cluster.Start() + defer cluster.Cleanup() + + cores := cluster.Cores + client := cores[0].Client + + err = client.Sys().EnableAuthWithOptions("ldap", &api.EnableAuthOptions{ + Type: "ldap", + }) + if err != nil { + t.Fatal(err) + } + + auth, err := client.Sys().ListAuth() + if err != nil { + t.Fatal(err) + } + + accessor := auth["ldap/"].Accessor + + adminPolicy := "admin_policy" + secret, err := client.Logical().Write("identity/group", map[string]interface{}{ + "type": "external", + "name": "ldap_admin_staff", + "policies": []string{adminPolicy}, + }) + if err != nil { + t.Fatal(err) + } + adminStaffGroupID := secret.Data["id"].(string) + adminGroupName := "admin_staff" + + secret, err = client.Logical().Write("identity/group-alias", map[string]interface{}{ + "name": adminGroupName, + "canonical_id": adminStaffGroupID, + "mount_accessor": accessor, + }) + if err != nil { + t.Fatal(err) + } + + secret, err = client.Logical().Read("identity/group/id/" + adminStaffGroupID) + if err != nil { + t.Fatal(err) + } + aliasMap := secret.Data["alias"].(map[string]interface{}) + if aliasMap["canonical_id"] != adminStaffGroupID || + aliasMap["name"] != adminGroupName || + aliasMap["mount_accessor"] != accessor { + t.Fatalf("bad: group alias: %#v\n", aliasMap) + } + + cleanup, cfg := ldaphelper.PrepareTestContainer(t, "latest") + defer cleanup() + + // Configure LDAP auth + secret, err = client.Logical().Write("auth/ldap/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, + }) + if err != nil { + t.Fatal(err) + } + + // Create a local user in LDAP + secret, err = client.Logical().Write("auth/ldap/users/hermes conrad", map[string]interface{}{ + "policies": "default", + }) + if err != nil { + t.Fatal(err) + } + + // Login with LDAP and create a token + secret, err = client.Logical().Write("auth/ldap/login/hermes conrad", map[string]interface{}{ + "password": "hermes", + }) + if err != nil { + t.Fatal(err) + } + token := secret.Auth.ClientToken + tokenPolicies, err := secret.TokenPolicies() + if err != nil { + t.Fatal(err) + } + if !strutil.StrListContains(tokenPolicies, adminPolicy) { + t.Fatalf("expected token policies to contain %s, got: %v", adminPolicy, tokenPolicies) + } + + // Lookup the token to get the entity ID + secret, err = client.Auth().Token().Lookup(token) + if err != nil { + t.Fatal(err) + } + entityID := secret.Data["entity_id"].(string) + + assertMember(t, client, entityID, adminGroupName, adminStaffGroupID, true) + + // Now remove Hermes from admin_staff + removeLdapGroupMember(t, cfg, adminGroupName, "hermes conrad") + + // Re-login with LDAP + secret, err = client.Logical().Write("auth/ldap/login/hermes conrad", map[string]interface{}{ + "password": "hermes", + }) + if err != nil { + t.Fatal(err) + } + + // Hermes should now be out of admin_staff group + assertMember(t, client, entityID, adminGroupName, adminStaffGroupID, false) + tokenPolicies, err = secret.TokenPolicies() + if err != nil { + t.Fatal(err) + } + if strutil.StrListContains(tokenPolicies, adminPolicy) { + t.Fatalf("expected token policies to not contain %s, got: %v", adminPolicy, tokenPolicies) + } + + // Add Hermes back to admin_staff + addLdapGroupMember(t, cfg, adminGroupName, "hermes conrad") + + // Re-login with LDAP + secret, err = client.Logical().Write("auth/ldap/login/hermes conrad", map[string]interface{}{ + "password": "hermes", + }) + if err != nil { + t.Fatal(err) + } + + // Hermes should now be back in admin_staff group + assertMember(t, client, entityID, adminGroupName, adminStaffGroupID, true) + tokenPolicies, err = secret.TokenPolicies() + if err != nil { + t.Fatal(err) + } + if !strutil.StrListContains(tokenPolicies, adminPolicy) { + t.Fatalf("expected token policies to contain %s, got: %v", adminPolicy, tokenPolicies) + } + + // Remove Hermes from admin_staff once again + removeLdapGroupMember(t, cfg, adminGroupName, "hermes conrad") + + oldToken := client.Token() + client.SetToken(secret.Auth.ClientToken) + secret, err = client.Auth().Token().RenewSelf(1) + if err != nil { + t.Fatal(err) + } + client.SetToken(oldToken) + assertMember(t, client, entityID, adminGroupName, adminStaffGroupID, false) + tokenPolicies, err = secret.TokenPolicies() + if err != nil { + t.Fatal(err) + } + if strutil.StrListContains(tokenPolicies, adminPolicy) { + t.Fatalf("expected token policies to not contain %s, got: %v", adminPolicy, tokenPolicies) + } +} + +func assertMember(t *testing.T, client *api.Client, entityID, groupName, groupID string, expectFound bool) { + t.Helper() + secret, err := client.Logical().Read("identity/group/id/" + groupID) + if err != nil { + t.Fatal(err) + } + groupMap := secret.Data + found := false + for _, entityIDRaw := range groupMap["member_entity_ids"].([]interface{}) { + if entityIDRaw.(string) == entityID { + found = true + } + } + if found != expectFound { + negation := "" + if !expectFound { + negation = "not " + } + t.Fatalf("expected entity ID %q to %sbe part of %q group", entityID, negation, groupName) + } +} + +func removeLdapGroupMember(t *testing.T, cfg *ldaputil.ConfigEntry, groupCN, userCN string) { + userDN := fmt.Sprintf("cn=%s,ou=people,dc=planetexpress,dc=com", userCN) + groupDN := fmt.Sprintf("cn=%s,ou=people,dc=planetexpress,dc=com", groupCN) + ldapreq := ldap.ModifyRequest{DN: groupDN} + ldapreq.Delete("member", []string{userDN}) + addRemoveLdapGroupMember(t, cfg, userCN, &ldapreq) +} + +func addLdapGroupMember(t *testing.T, cfg *ldaputil.ConfigEntry, groupCN, userCN string) { + userDN := fmt.Sprintf("cn=%s,ou=people,dc=planetexpress,dc=com", userCN) + groupDN := fmt.Sprintf("cn=%s,ou=people,dc=planetexpress,dc=com", groupCN) + ldapreq := ldap.ModifyRequest{DN: groupDN} + ldapreq.Add("member", []string{userDN}) + addRemoveLdapGroupMember(t, cfg, userCN, &ldapreq) +} + +func addRemoveLdapGroupMember(t *testing.T, cfg *ldaputil.ConfigEntry, userCN string, req *ldap.ModifyRequest) { + logger := log.New(nil) + ldapClient := ldaputil.Client{LDAP: ldaputil.NewLDAP(), Logger: logger} + // LDAP server won't accept changes unless we connect with TLS. This + // isn't the default config returned by PrepareTestContainer because + // the Vault LDAP backend won't work with it, even with InsecureTLS, + // because the ServerName should be planetexpress.com and not localhost. + conn, err := ldapClient.DialLDAP(cfg) + if err != nil { + t.Fatal(err) + } + defer conn.Close() + + err = conn.Bind(cfg.BindDN, cfg.BindPassword) + if err != nil { + t.Fatal(err) + } + + err = conn.Modify(req) + if err != nil { + t.Fatal(err) + } +} diff --git a/vault/request_handling.go b/vault/request_handling.go index aeedd18b1230..b6090fa965fe 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -1077,13 +1077,11 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re } auth.EntityID = entity.ID - if auth.GroupAliases != nil { - validAliases, err := c.identityStore.refreshExternalGroupMembershipsByEntityID(ctx, auth.EntityID, auth.GroupAliases) - if err != nil { - return nil, nil, err - } - auth.GroupAliases = validAliases + validAliases, err := c.identityStore.refreshExternalGroupMembershipsByEntityID(ctx, auth.EntityID, auth.GroupAliases) + if err != nil { + return nil, nil, err } + auth.GroupAliases = validAliases } // Determine the source of the login