Skip to content
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

Add GitHub's Safe-Settings app to manage policy as code #24

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

Conversation

paddyroddy
Copy link
Member

This is copying the work across in the @UCL-MIRSG organisation here UCL-MIRSG/.github#141 relating to the deployment of the https://github.com/github/safe-settings app. I recently gave a brief overview of this in the DevOps Hour slides. I have created an app with the appropriate permissions, which will need to be installed organisation wide once this PR is merged.

Safe-Settings has a lot of possible options, so I've gone for as little inoffensive ones as possible. These are currently:

@paddyroddy paddyroddy added enhancement New feature or request safe-settings Related to github/safe-settings deployment labels Jan 6, 2025
@paddyroddy paddyroddy self-assigned this Jan 6, 2025
@@ -0,0 +1,19 @@
# https://github.com/github/safe-settings/blob/main-enterprise/docs/sample-settings/suborg.yml
Copy link
Member Author

Choose a reason for hiding this comment

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

The downside of the app is things are mutually exclusive. If we are to enable this, it would disable any current rulesets. I've left it here for demonstration purposes.

@paddyroddy
Copy link
Member Author

Failing because the app isn't currently installed

@paddyroddy paddyroddy marked this pull request as ready for review January 6, 2025 15:30
@paddyroddy paddyroddy requested review from dpshelio, samcunliffe, jonc125 and a team January 6, 2025 15:31
@jonc125
Copy link
Contributor

jonc125 commented Jan 6, 2025

Given this is an organisation-wide change, I would want feedback and approval from each ARC team before merging, just in case it could break something they rely on.

@paddyroddy
Copy link
Member Author

Fair enough @jonc125. As I said, it has very limited settings enabled. We could just have the autolink one. Knowing what ARC is like, I suspect it won't ever be merged if there's a lot of scepticism - we have successfully used it in MIRSG.

@jonc125
Copy link
Contributor

jonc125 commented Jan 6, 2025

To address scepticism, this PR needs to start with the TL;DR of why it is useful - what is the point and what are the (potential) risks? Then key stakeholders within ARC can take a view on it - we don't need buy-in from everyone.

@t-young31
Copy link
Member

t-young31 commented Jan 6, 2025

The app looks to have a pretty scary amount of privilege. Agree with @jonc125 this needs due care and attention before enabling (i.e. what are the possible privilege escalation routes/are we happy with accepting them)

@samcunliffe
Copy link
Member

samcunliffe commented Jan 7, 2025

The app looks to have a pretty scary amount of privilege. Agree with @jonc125 this needs due care and attention before enabling (i.e. what are the possible privilege escalation routes/are we happy with accepting them)

Hosted by us though, right? ...

I have created an app with the appropriate permissions, which will need to be installed organisation wide once this PR is merged.

And FWIW it's a GitHub-developed tool. Intended to increase uniformity and safety across all repo settings. To help org admin.

I suppose it's expected that a thing configuring how repos are set up, would need admin access to the repos. Is it e.g. the autolinking to MyServices that's going to need +rw on all issues, PRs, projects, thoughts, ideas, and dreams?

I guess the main concern is the org-level admin.

Organization Permissions

  • Administration: Read & Write
  • Custom properties: Admin
  • Members: Read & Write

If I'm playing the evil genus: I need to inject some malicious code into github/safe-settings, make it into a release and we update our bot with the malicious code. The worst thing I can do with these privileges is quite bad: I can create accounts and escalate the privs on one or more repos, run actions and redeploy things. Can I delete a repo? edit: I can delete a repo.

@samcunliffe samcunliffe changed the title Add Safe-Settings app to manage policy as code Add GitHub's Safe-Settings app to manage policy as code Jan 7, 2025
@paddyroddy
Copy link
Member Author

I appreciate the concern regarding permissions, but as @samcunliffe pointed out we are self-hosting and if the app didn't have said permissions then it wouldn't be able to do anything.

I suppose it's expected that a thing configuring how repos are set up, would need admin access to the repos. Is it e.g. the autolinking to MyServices that's going to need +rw on all issues, PRs, projects, thoughts, ideas, and dreams?

The app allows us to set

  • Repository settings - home page, url, visibility, has_issues, has_projects, wikis, etc.
  • Default branch - naming and renaming
  • Topics
  • Custom properties
  • Teams and permissions
  • Collaborators and permissions
  • Issue labels
  • Milestones
  • Branch protections - if the name of the branch is default in the settings, it is applied to the default branch of the repo.
  • Autolinks
  • Repository name validation using regex pattern
  • Rulesets
  • Environments - wait timer, required reviewers, prevent self review, protected branches deployment branch policy, custom deployment branch policy, variables, deployment protection rules

Each permission makes sense when you think of what a given step is doing.

@paddyroddy
Copy link
Member Author

I'm giving a talk on this at the Collaborations Hour on 11th Feb, but slides are ready now https://paddyroddy.github.io/talks/github-safe-settings-policy-as-code

@jonc125
Copy link
Contributor

jonc125 commented Feb 11, 2025

Is it easy to see what repository/org level settings would be changed with the proposed configuration?

@paddyroddy
Copy link
Member Author

There's currently a bug with GHA github/safe-settings#739 which means that PRs don't run in dry-run when they should. Hopefully that should be fixed soon github/safe-settings#738.

I currently have set log level as trace, but can be debug/info.

The PR comment setting should be showing what happens but isn't currently due to above.

@samcunliffe
Copy link
Member

The app looks to have a pretty scary amount of privilege. Agree with @jonc125 this needs due care and attention before enabling (i.e. what are the possible privilege escalation routes/are we happy with accepting them)

Hosted by us though, right? ...

I have created an app with the appropriate permissions, which will need to be installed organisation wide once this PR is merged.

And FWIW it's a GitHub-developed tool. Intended to increase uniformity and safety across all repo settings. To help org admin.

I suppose it's expected that a thing configuring how repos are set up, would need admin access to the repos. Is it e.g. the autolinking to MyServices that's going to need +rw on all issues, PRs, projects, thoughts, ideas, and dreams?

I guess the main concern is the org-level admin.

Organization Permissions

  • Administration: Read & Write
  • Custom properties: Admin
  • Members: Read & Write

If I'm playing the evil genus: I need to inject some malicious code into github/safe-settings, make it into a release and we update our bot with the malicious code. The worst thing I can do with these privileges is quite bad: I can create accounts and escalate the privs on one or more repos, run actions and redeploy things. Can I delete a repo? edit: I can delete a repo.

@t-young31
We've discovered that it's possible to stagger renovate's updates by a minimumReleaseAge. E.g. ask it to wait X months before pulling a new version of safe-settings. Would this help to alleviate this concern? On the other hand, we also want to pull versions if there are any security fixes.

@t-young31
Copy link
Member

Would this help to alleviate this concern? On the other hand, we also want to pull versions if there are any security fixes.

Not hugely – would prefer human-in-the-loop updates, if possible.


I don't really have the time to risk assess so please consider my thoughts as non-blocking. Is it possible to scope the permissions to those required for what it's configured to do, rather than what it asks for? Looping in @brian-maher and @bathomas for their thoughts.

@brian-maher
Copy link

Some thoughts based on a quick read-around I've had (I've not had a chance to look in detail - so happy to be corrected/challenged on these!).

  1. Understanding what this does to each repo before initial roll-out is pretty critical I'd say - it's possible this could unknowingly reduce security (e.g. if a particularly sensitive repo has been manually set to require multiple reviews, etc).
  2. Having an app with those privs is pretty scary - and in a very roundabout way, would mean that anybody with write access to this repo could effectively get org admin (or at the very least, command the app to do org adminy/bad things).
    2.5. I don't have permission to see actual perms (a good thing!) but looking at the repo history it appears there's a shared account that looks like it has write access to this repo (and a quick search of Slack suggests the PW and OTP for that account is in LP). Ergo, access to LP == the ability to do bad things in GitHub.

Whilst that is immediately solvable, it highlights the risks of having an app with org admin permissions hanging around in an org like this.

@paddyroddy
Copy link
Member Author

would mean that anybody with write access to this repo could effectively get org admin

Currently, anyone with access to the LastPass has this ability too

@brian-maher
Copy link

would mean that anybody with write access to this repo could effectively get org admin

Currently, anyone with access to the LastPass has this ability too

Yep, I didn't realise that (which makes me sad!). If the majority of the org already has an easy route to admin, then this should probably be considered more of a convenience thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request safe-settings Related to github/safe-settings deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants