Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Ignore implicit memberships in Requester.getAllTeams #101

Merged
zapu merged 3 commits into from
Jun 20, 2020
Merged

Ignore implicit memberships in Requester.getAllTeams #101

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
913c076
Ignore implicit memberships in Requester.getAllTeams
zapu Jun 16, 2020
2ebc94e
WIP
zapu Jun 19, 2020
60a0e7b
Remove TODO
zapu Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
WIP
  • Loading branch information
@zapu
zapu committed Jun 19, 2020
commit 2ebc94ec236b612fce2118bcdb7eb1a39f8d05d7
9 changes: 1 addition & 8 deletions src/keybaseca/bot/bot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 9 additions & 6 deletions src/keybaseca/sshutils/sshutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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
}
}
Expand Down
13 changes: 1 addition & 12 deletions src/kssh/requester.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/keybase/bot-sshca/src/shared"
"github.com/keybase/go-keybase-chat-bot/kbchat"
"github.com/keybase/go-keybase-chat-bot/kbchat/types/keybase1"
)

type Requester struct {
Expand Down Expand Up @@ -89,17 +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 {
if m.Role != keybase1.TeamRole_NONE {
teams = append(teams, m.FqName)
}
}
return teams, nil
return shared.GetAllTeams(r.api)
}

// Get a signed SSH key from interacting with the CA chatbot
Expand Down
38 changes: 38 additions & 0 deletions src/shared/teams.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
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) {
// TODO: dedup with same method in keybaseca/bot

@mmou mmou Jun 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove this line

@zapu zapu Jun 19, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad. I'm so careless :(

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
}