Merge pull request #101 from keybase/zapu/implicit-admin-bug

Fix a bug where kssh and keybaseca would try to fetch KV store data from teams
user is not an explicit member of.

Author: zapu

src/keybaseca/bot/bot.go

@@ -252,14 +252,7 @@ func (b *Bot) DeleteAllClientConfigs() error {
 }
 
 func (b *Bot) getAllTeams() (teams []string, err error) {
-	memberships, err := b.api.ListUserMemberships(b.api.GetUsername())
-	if err != nil {
-		return teams, err
-	}
-	for _, m := range memberships {
-		teams = append(teams, m.FqName)
-	}
-	return teams, nil
+	return shared.GetAllTeams(b.api)
 }
 
 // LogError logs the given error to Keybase chat and to the configured log file. Used so

src/keybaseca/sshutils/sshutils.go

@@ -151,9 +151,10 @@ func SignKey(caKeyLocation, keyID, principals, expiration, publicKey string) (si
 	return string(signatureBytes), nil
 }
 
-// Get the principals that should be placed in the signed certificate.
-// Note that this function is a security boundary since if it was bypassed an
-// attacker would be able to provision SSH keys for environments that they should not have access to.
+// Get the principals that should be placed in the signed certificate. Note
+// that this function is a security boundary since if it was bypassed an
+// attacker would be able to provision SSH keys for environments that they
+// should not have access to.
 func getPrincipals(conf config.Config, sr shared.SignatureRequest) (string, error) {
 	// Start by getting the list of teams the user is in
 	api, err := botwrapper.GetKBChat(conf.GetKeybaseHomeDir(), conf.GetKeybasePaperKey(), conf.GetKeybaseUsername(), conf.GetKeybaseTimeout())
@@ -165,11 +166,13 @@ func getPrincipals(conf config.Config, sr shared.SignatureRequest) (string, erro
 		return "", fmt.Errorf("failed to retrieve the list of teams the user is in: %v", err)
 	}
 
-	// Maps from a team to whether or not the user is in the current team (with writer, admin, or owner permissions)
+	// Maps from a team to whether or not the user is in the current team (with
+	// writer, admin, or owner permissions)
 	teamToMembership := make(map[string]bool)
 	for _, result := range results {
-		if result.Role != 0 {
-			// result.Role == 0 means they are an impicit admin in the team and are not actually a member
+		// Check if the user is actually in the team, and not a restricted bot
+		// or implicit admin.
+		if shared.CanRoleReadTeam(result.Role) {
 			teamToMembership[result.FqName] = true
 		}
 	}

src/kssh/requester.go

@@ -88,15 +88,7 @@ func (r *Requester) LoadConfigForBot(botName string) (Config, error) {
 }
 
 func (r *Requester) getAllTeams() (teams []string, err error) {
-	// TODO: dedup with same method in keybaseca/bot
-	memberships, err := r.api.ListUserMemberships(r.api.GetUsername())
-	if err != nil {
-		return teams, err
-	}
-	for _, m := range memberships {
-		teams = append(teams, m.FqName)
-	}
-	return teams, nil
+	return shared.GetAllTeams(r.api)
 }
 
 // Get a signed SSH key from interacting with the CA chatbot

src/shared/teams.go

@@ -0,0 +1,37 @@
+package shared
+
+import (
+	"github.com/keybase/go-keybase-chat-bot/kbchat"
+	"github.com/keybase/go-keybase-chat-bot/kbchat/types/keybase1"
+)
+
+// CanRoleReadTeam checks if given role can access team data. User has to be an
+// actual team member (not implicit admin) to be able to access data, and
+// cannot be a RESTRICTED BOT.
+func CanRoleReadTeam(role keybase1.TeamRole) bool {
+	switch role {
+	case keybase1.TeamRole_READER,
+		keybase1.TeamRole_WRITER,
+		keybase1.TeamRole_ADMIN,
+		keybase1.TeamRole_OWNER,
+		keybase1.TeamRole_BOT:
+		return true
+	default:
+		return false
+	}
+}
+
+// GetAllTeams makes an API call and returns list of team names readable for
+// current user.
+func GetAllTeams(api *kbchat.API) (teams []string, err error) {
+	memberships, err := api.ListUserMemberships(api.GetUsername())
+	if err != nil {
+		return teams, err
+	}
+	for _, m := range memberships {
+		if CanRoleReadTeam(m.Role) {
+			teams = append(teams, m.FqName)
+		}
+	}
+	return teams, nil
+}