rust-lang / infra-team

Coordination repository for the Rust infra team
https://www.rust-lang.org/governance/teams/infra
Apache License 2.0
17 stars 8 forks source link

Authorize G3N Github app #67

Open tmandry opened 1 year ago

tmandry commented 1 year ago

G3 Notifications is a Chrome extension for Google employees to receive notifications when certain items need their attention, like code reviews, in a central place. It has a companion app that allows it to integrate with Github. The app is closed source.

The main benefit of enabling this would be improved responsiveness of Google employees to pull requests that require their review or that they either wrote.

This app would need to be authorized by a rust-lang admin to work with the Rust organization: https://github.com/apps/g3n-github/installations/new/permissions?target_id=5430905

See below for a discussion of the required permissions.

Repositories

The app only requires read access, and only public repositories are being requested. In an earlier Zulip thread @pietroalbini said:

the worry I have is limiting it to only public repos

You can accomplish this by picking the "Only select repositories" option and picking any public repo (say, rust-lang/rust). Other public repos will be automatically included, according to the web page (but you must select at least one repo). EDIT: https://github.com/rust-lang/infra-team/issues/67#issuecomment-1368419939

Permissions

The app requests the following permissions:

Read access to actions, administration, checks, issues, members, metadata, and pull requests

I asked the developer about the administration permission, and got this response:

The administration permission is required as part of the teams API to list the teams associated with your repository. Strangely enough, it seems that finding teams associated with an org is in "members" (GET /orgs/{org}/teams), but finding teams associated with your repository is in "administration" (GET /repos/{owner}/{repo}/teams).

The best documentation I could find matching specific methods to their required permissions was here: https://docs.github.com/en/rest/overview/permissions-required-for-github-apps?apiVersion=2022-11-28#administration

The link above has the set of APIs that are granted with this permission, which are worth looking over. Only read permission is required. I didn't see anything too concerning to me, but I also really wish Github didn't lump all these things together.

Mark-Simulacrum commented 1 year ago

The administration API seems to give access to a bunch of things but I agree that it's not obviously sensitive with just read access.

I am curious - I don't see why the teams with access to a repo are needed for the app? Is there some reason for that access to be necessary?

I think with the current state this is fine, though I would be concerned about future expansion of the APIs there into something sensitive.

chases2 commented 1 year ago

Hello! I helped to write the companion app. We use team lists on a repository to determine team membership, so that way if we get a PR that's assigned to a GitHub team, we can figure out what humans are on that team and notify them. The PR event the app sends doesn't come with that information.

Also note that an app's developers can't just increase the permissions. If we were to change the app to use more permissions, then GitHub requires each user to re-install the app to allow those permissions.

Mark-Simulacrum commented 1 year ago

Also note that an app's developers can't just increase the permissions. If we were to change the app to use more permissions, then GitHub requires each user to re-install the app to allow those permissions.

I think the issue is that if github adds new APIs to administration (or any other category), presumably you get those without further review, right?

tmandry commented 1 year ago

Yeah. We should provide Github with feedback about this structure. At this point I think they should avoid adding new, riskier APIs to administration and create a new permission for it instead.

At the same time, I'm not sure what they would add.. I certainly would not expect to be able to read secrets or anything like that.

Mark-Simulacrum commented 1 year ago

Yeah, I'll try to include it in our next call with GitHub (presuming I remember).

I think there's a bunch of semi-iffy APIs that could look reasonable to extend in a way that's not great (e.g., which specific accounts have branch protection override, to encourage attacks against those, something around security vulnerabilities - where just repository names/existence is leakage of some kind). But I think most of these are relatively innocuous in this case.

I'll bring this up (if I remember!) at infra meeting next week, but I think at this point I'm not inclined to be too protective here, we can probably move ahead.

pietroalbini commented 1 year ago

You can accomplish this by picking the "Only select repositories" option and picking any public repo (say, rust-lang/rust). Other public repos will be automatically included, according to the web page (but you must select at least one repo).

I don't think granting access to a public repository also grants read-only "administration" access to all other public repositories in the org.

I tried installing a test app in one of my personal repositories: I could access contents:read for all public repos in my account with it, but only administration:read for the repo I installed the app on.

Strangely enough, it seems that finding teams associated with an org is in "members" (GET /orgs/{org}/teams), but finding teams associated with your repository is in "administration" (GET /repos/{owner}/{repo}/teams).

We use team lists on a repository to determine team membership, so that way if we get a PR that's assigned to a GitHub team, we can figure out what humans are on that team and notify them. The PR event the app sends doesn't come with that information.

GET /repos/{owner}/{repo}/teams doesn't return team membership, it returns teams that have read or write access granted to a repository. If you care about who is member of a team, the "members" scope should be sufficient.