@@ -120,46 +120,44 @@ func ProcessSignatureRequest(conf config.Config, sr shared.SignatureRequest) (re
120120 return shared.SignatureResponse {SignedKey : string (data ), UUID : sr .UUID }, nil
121121}
122122
123- // Get the principals that should be placed in the signed certificate
124- func getPrincipals (conf config.Config , sr shared.SignatureRequest ) (string , error ) {
125- // Iterate through the teams in the config file and use the last portion of the subteam as the principal
126- // if the user is in that subteam
127- var principals []string
128- for _ , team := range conf .GetTeams () {
129- members , err := getMembers (conf , team )
130- if err != nil {
131- return "" , err
132- }
133- for _ , member := range members {
134- if member == sr .Username {
135- principals = append (principals , team )
136- }
137- }
138- }
139- return strings .Join (principals , "," ), nil
140- }
141-
142- // Get the members of the given team. Note that this function is a security boundary since if it was bypassed an
123+ // Get the principals that should be placed in the signed certificate.
124+ // Note that this function is a security boundary since if it was bypassed an
143125// attacker would be able to provision SSH keys for environments that they should not have access to.
144- func getMembers (conf config.Config , team string ) ([]string , error ) {
126+ func getPrincipals (conf config.Config , sr shared.SignatureRequest ) (string , error ) {
127+ // Start by getting the list of teams the user is in
145128 api , err := botwrapper .GetKBChat (conf .GetKeybaseHomeDir (), conf .GetKeybasePaperKey (), conf .GetKeybaseUsername ())
146129 if err != nil {
147- return nil , err
130+ return "" , fmt . Errorf ( "failed to retrieve the list of teams the user is in: %v" , err )
148131 }
149- result , err := api .ListMembersOfTeam ( team )
132+ results , err := api .ListUserMemberships ( sr . Username )
150133 if err != nil {
151- return nil , err
152- }
153- users := []string {}
154- for _ , member := range result .Owners {
155- users = append (users , member .Username )
156- }
157- for _ , member := range result .Admins {
158- users = append (users , member .Username )
134+ return "" , fmt .Errorf ("failed to retrieve the list of teams the user is in: %v" , err )
135+ }
136+
137+ // Maps from a team to whether or not the user is in the current team (with writer, admin, or owner permissions)
138+ teamToMembership := make (map [string ]bool )
139+ for _ , result := range results {
140+ // Sadly result.Role is an integer and this is all we're given. Let's hope no one ever changes this enum out
141+ // from underneath us. Admittedly, the worst that could (should) happen is that someone with minimal permissions
142+ // in a team is given access (eg a reader) which wouldn't lead to a complete compromise since an attacker
143+ // would still have to be added as a reader first.
144+ //
145+ // result.Role == 4 --> owner
146+ // result.Role == 3 --> admin
147+ // result.Role == 2 --> writer
148+ if result .Role == 4 || result .Role == 3 || result .Role == 2 {
149+ teamToMembership [result .TeamName ] = true
150+ }
159151 }
160- for _ , member := range result .Writers {
161- users = append (users , member .Username )
152+
153+ // Iterate through the teams in the config file and use the subteam as the principal
154+ // if the user is in that subteam
155+ var principals []string
156+ for _ , team := range conf .GetTeams () {
157+ result , ok := teamToMembership [team ]
158+ if ok && result {
159+ principals = append (principals , team )
160+ }
162161 }
163- // Read only users are not listed since they shouldn't be issued SSH keys
164- return users , nil
162+ return strings .Join (principals , "," ), nil
165163}
0 commit comments