Skip to content

Introduce github-mgmt repository #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Introduce github-mgmt repository #99

wants to merge 1 commit into from

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Apr 10, 2025

After me and @winterqt have experimented with the suggestion in #40 of using github-as-code, we're confident that we should deploy for real, so that people can more easily request GitHub org changes by just making PRs.

As this is strictly outside the authority of the org owners, I am hereby formally requesting @NixOS/steering approval to give the org owners authority over the deployment of github-as-code.

Some notes:

  • github-as-code bi-directionally syncs changes, so any imperative changes to the GitHub org will get synced back to the repo automatically, it will not interfere with that. But in addition, merged PRs will get synced to the GitHub org too.
  • Only the org owners will be able to make changes to the repo for a start, but this could be expanded in the future with good file separation and codeowner declarations like the org repo.
  • We'll only use it for team memberships to start out
  • This will pave the way towards Automatic retirement of Nixpkgs committers #91 (which I'd also still like SC approval on)

@infinisil infinisil requested a review from a team April 10, 2025 19:55
@infinisil infinisil requested a review from a team as a code owner April 10, 2025 19:55
@fricklerhandwerk
Copy link
Collaborator

fricklerhandwerk commented Apr 10, 2025

This would also finally open the doors to automatically list team members on nixos.org (ping @avocadoom)!

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Approving conditional on testing against

  • accidental deletions (e.g. can a repo and all its metadata be deleted or what would it take?)
  • a plan B for the possibility that all admins that are managed by this system are locked out

@tomberek
Copy link
Contributor

As another data point: https://github.com/apache/infrastructure-asfyaml

@infinisil
Copy link
Member Author

Thanks for the SC approval! We (org owners) will evaluate this more before committing because there's some security concerns:

  • The github-as-code App needs a lot of permissions and GitHub doesn't allow narrowing it down. This most notably includes:
  • github-as-code regularly runs code from the very same repo that it also pushes bi-directional syncing commits to -> self-modifying code isn't that far off
  • The standard way of upgrading github-as-code is to manually trigger the upgrade action, which syncs the latest changes from upstream and pushes it directly without a PR

We can do:

  • (easy) Reach out to contacts at GitHub to get their thoughts on this (@tomberek is on this)
  • (easy) Test whether admins can be locked out as suggested (I suspect it's definitely possible)
  • (medium) Review upstream for which API calls are used, both initially and continually before upgrading in the future
    • (easy) Make the upgrade action create a PR instead
    • (medium) Fork github-as-code and manually pull in updates
  • (medium) Ensure that we have backups of everything that github-as-code could wipe in theory (could be fairly easy since it itself writes all of the state to an S3 bucket, whose history we can preserve)
  • (medium) Ask github-as-code to onboard trusted members from our side as maintainers
  • (medium) Remove the need for the global Repository Contents permission by separating that permission to another GitHub App scoped to just the github-mgmt repository.
  • (hard) Properly separate code from config by having a separate repository for them. This would be hard to upstream.
  • (hard) Find or build a GitHub-imitating API server that only allows exactly the API calls that we need.
  • (hard) Ask GitHub to add another permission scope for GitHub Apps needed for potentially destructive operations like removing admins and repos.

With some of the above I expect us to have enough confidence in the deployment, but we'll see

@tomberek
Copy link
Contributor

Another data point: https://github.com/eclipse-csi/otterdog

@winterqt
Copy link
Member

Some thoughts:

The Repository Contents permission, which allows pushing commits to any repo (if branch protection allows it), which is used to implement the bi-directional syncing

For this one specifically, I find it a bit odd that we can't restrict it -- realistically, we only need to give the app content access to the management repo, since we aren't using its capability to add files to other repos in the org.

(easy) Test whether admins can be locked out as suggested (I suspect it's definitely possible)

Couldn't we give the app member permissions, but not make it an owner itself, which may allow some security in this sense (that is, it can't remove non-owners)?

  • (medium) Review upstream for which API calls are used, both initially and continually before upgrading in the future
    • (easy) Make the upgrade action create a PR instead
    • (medium) Fork github-as-code and manually pull in updates

I like this.

(medium) Ensure that we have backups of everything that github-as-code could wipe in theory (could be fairly easy since it itself writes all of the state to an S3 bucket, whose history we can preserve)

Doesn't prevent things like issues/wiki/etc from being wiped -- we'd need good backup solutions for those.

(medium) Ask github-as-code to onboard trusted members from our side as maintainers

We'd have to ask, but this may be doable, esp. given we'll probably be its biggest (in terms of members) user.

(medium) Remove the need for the global Repository Contents permission by separating that permission to another GitHub App scoped to just the github-mgmt repository.

Ah, yeah, exactly what I said above. :)

@infinisil
Copy link
Member Author

For this one specifically, I find it a bit odd that we can't restrict it -- realistically, we only need to give the app content access to the management repo, since we aren't using its capability to add files to other repos in the org.
[...]

(medium) Remove the need for the global Repository Contents permission by separating that permission to another GitHub App scoped to just the github-mgmt repository.

Ah, yeah, exactly what I said above. :)

Yeah so we need two GitHub Apps for that, because any single GitHub apps permissions are only restricted to essentially the cross product of repos and permissions, so it's not possible to give it certain permissions for one repo and different permissions for another/all.

Couldn't we give the app member permissions, but not make it an owner itself, which may allow some security in this sense (that is, it can't remove non-owners)?

Permissions of GitHub apps are separate from the user model, the App isn't an owner itself. That said, it's a really interesting idea to make it use the user model instead:

  • Create a dedicated user
  • Give it the team maintainer permission on all teams
  • Give it maintainer permission on all repos, described as "Recommended for project managers who need to manage the repository without access to sensitive or destructive actions"
  • Give it write access to only the github-mgmt repo.
  • Create a personal access token for the user and make github-as-code use it instead of the App

This way there would be no way for the tool to remove admins nor repos! Frankly this sounds like a really good option, probably medium difficulty since it requires some upstream changes.


Separately I'd also like to highlight @arianvp's suggestion here and the associated discussion as a potential alternative security model.

@infinisil
Copy link
Member Author

I've submitted ipdxco/github-as-code#198 to implement the above-mentioned idea, and confirmed that it works for team management!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants