Merge pull request #2 from hashicorp/fix-identity-external-group-upda…

…tes-1.3 Fix for the issue with not removing users from external groups on log…
hashicorp · Mar 18, 2020 · d6e0c53 · d6e0c53
2 parents 199ef39 + 18485ee
commit d6e0c53
Show file tree

Hide file tree

Showing 4 changed files with 267 additions and 70 deletions.
diff --git 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
@@ -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
@@ -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)
+	}
+}