-
Notifications
You must be signed in to change notification settings - Fork 6
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
202408 proposal for reducing filecoin-project org ownership #61
Conversation
This is covered in #47
The following access changes will be introduced as a result of applying the plan: Access Changes
|
This was a cut/paste mistake before.
Before merge, verify that all the following plans are correct. They will be applied as-is after the merge. Terraform plansfilecoin-project
|
Even before I get the ability to see https://github.com/organizations/filecoin-project/settings/audit-log, I'm still marking this as ready for review to give more time for feedback and comments. I'm not expecting to have gotten this all right on try one, so the quicker I can get feedback the better for being able to close this out. |
Per #47 , I have proposed a moderators and security-managers team and included that in the PR. The PR description has been updated to textually describe who has been affected. |
…filecoin-project/github-mgmt into biglep/adjust-org-admins-owners
I have org owner access and was able to check the audit log. This hasn't lead me to make any suggested changes based on the original recommendation. How data was collectedBelow is the dump from https://github.com/organizations/filecoin-project/settings/audit-log?q=created%3A%3E2024-02-25+action%3Aaccount.*+action%3Abilling.*+action%3Abusiness.*+action%3Aenterprise.*+action%3Aintegration_installation.create+action%3Aorg.*+action%3Apayment_method.*+action%3Ateam.*+-actor%3Afilecoin-project-mgmt-read-write%5Bbot%5D+-actor%3Acustom-gh-runners-by-ipdx-co-filoz%5Bbot%5D using filter I reduced the columns that are displayed and sorted using Data dump
|
Is there any way to make this into a two-key system? I.e.:
That way we have no single point of failure? |
@@ -1,2 +1,6 @@ | |||
# The ipdx team is responsible for GitHub Management maintenance (at least through 2024) | |||
* @filecoin-project/ipdx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But do they need immediate write access to this repo? IMO, any changes should go through our team (I'm mostly concerned about them already being a massive target).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, that only comes in handy when we have to resolve some issue with the project setup, but we can deal with that on a case-by-case basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But do they need immediate write access to this repo?
To be clear, this isn't giving them permission (see discussion about the role of codeowners in thie repo here).
The whole of this file is affectively saying that ipdx is needed for any changes made outside https://github.com/filecoin-project/github-mgmt/blob/master/github/filecoin-project.yml . I think that is good. If anyone is making github-mgmt setup changes (which happens in the directories outside of filecoin-project/github-mgmt/github
, then they should be engaged.)
(No further action planned by here, but feedback/ideas welcome.)
I'm mostly concerned about them already being a massive target
Fair. To be clear on this, the ipdx team as a whole doesn't have write access to this repo or any other I don't think. @galargh individually is an org owner/admin and part of github-mgmt-stewards (according to this proposed PR), so I agree he's a big target (as are the other couple of owners - which is why I'm trying to limit that set of people).
Potential action: remove @galargh from being a org owner or github-mgmt steward and replace him with someone else. I didn't opt for that in this proposal/PR because I think the risk of his account being compromised is outweighed by if crap-hits-the-fan that we have someone observant, skilled, and knowledgable with github tools take action. I'm happy to change course though.
It would be the easiest to just require 2 approvals - it's as easy as changing a setting in branch protection rules. We can also require reviews from CODEOWNERS. |
I'm good with my personal permissions being reduced, but I think it still makes sense for someone from the Foundation to be an owner? I'll defer to @relotnek , but we should probably also include Baldy in the security-managers. |
I'll work on responding to the feedback in approximately an hour. @galargh: what do we need to do so that branch protection rules show up in github-mgmt? If we make any adjustments to number of approvals, I'd like to ideally have a log/review of that. |
can we add @relotnek and @parthshah1 to this discussion? |
@Stebalien: responding to comments above...
I know you weren't asking about this, but to be clear, the reason we have a CODEOWNERS in this repo is so those folks get added to all PRs in the repo as required reviewers. This serves two purposes:
(I didn't realize we didn't have "Require review from Code Owners" enabled for this repo. I have enabled it manually through the UI since this isn't showing up in github-mgmt currently. I put a screenshot below of the branch protection rule UI for
The current CODEOWNERS has two teams (github-mgmt-stewards and ipdx) which both have multiple people behind them. Are you worried we only have two teams, or that we don't have enough people behind those teams?
There unfortunately isn't an out-of-the-box way to require 2 approvals by default, but only 1 if the author is a CODEOWNER. There are various threads about this online, but I think https://github.com/orgs/community/discussions/84831 most sucinctly describes the issue. It also outlines potential steps one could take to build more smarts with GitHub Actions.
Given the limitation above, I'm weary of requiring two approvals until we create the smarter approval/enforcement flow. Another option would be to create a new team: github-mgmt-reviewers . This group could have non-write/push access and thus we'd be more comfortable adding more people to it (e.g., more folks from FF, FilOz, ChainSafe, SEAD) and then increase the number of approvers to two. Merging would still be restricted to the github-mgmt-stewards group, who is expected to have good judgement and be versed in the tool. I'm happy to create (and action) backlog items for the ideas above. I'm thinking not to block this PR on them though since this isn't a one-way door, and we can also observe in practice how important these extra/smarter approval levels are. (Devil's advocate: wait-and-see may not be smart as you may have already had a major event/compromise.) |
Ok, I'm personally not hard-opposed, but what value add do you see here vs. the few people that are now proposed? With the proposal here, we've got a few people who will see notifications and can bypass this whole github-mgmt system if necessary. As part of github-mgmt stewards, you can also escalate your permissions to an owner if needed. Anyways, if you still think its good for FF to have direct owner representation, let me know. I assume I should just add you back.
Sure. I don't know their login though. Do you want to add a github suggestion so I can commit it (or put it here)? (I won't plan on blocking the merge for this though - you/they/we can always do a followup with it.)
Sure. This PR is public. They can comment at will, and they were @mentioned in the original PR description, so should have been receiving notifications. |
It looks like their ignored resources as defined in https://github.com/filecoin-project/github-mgmt/blob/master/terraform/resources_override.tf . I think we should do a separate PR after this with any additional resources we want to synchronize as this will bring in a lot of changes. From this PR, two things I wish had been accessible via github-mgmt were repo branch protection rules and team visibility. |
2024-08-27 status:
|
The resource is |
Made Magik owner Extended BigLep owner's term Added willscott as github-mgmt-steward
…filecoin-project/github-mgmt into biglep/adjust-org-admins-owners
…itch anyway given he is org owner
Created github-mgmt approvers
2024-08-28 update: I have pushed some commits that have done the following:
The rationale for these people is in the PR. The main item that isn't fully addressed here is increasing the reviewer/approval required approvers to 2 approvers (ideally with smart branch protection to only require 1 reviewer if the author is also a CODEOWNER). This topic was covered more in comment above. I have a backlog item to get the "smart" branch protection in place before increasing the number of approvers: #65 In terms of next steps:
I believe I have updated the issue description accordingly as well. |
Yeah, that's right @anorth . Thanks for reviewing. I also pushed some additional changes per #61 (comment) |
I'm okay being a github-mgmt reviewer for this org |
At a minimum we should have Baldy for operational management / billing related items, and I think it would be good to note who else is performing those duties within the org |
IIUC this integration is more about org repo perm management, less about billing like admin operations and so on. If you can provide a description of baldly like bigleps did in his PR, we may evaluate it then. |
Further changes can be proposed via new PRs |
This got merged while I was out. No objections from me. It does mean that I didn't get to #65 beforehand. I have that on my personal backlog. At this point, I'm going to wait and see how much we wish we had this over the next month before spending time on it. Please flag if you think we need it sooner. I'll now turn to the followup tasks in #47 |
Message to those affected
@arden-sead
@dr-bizz
@filecoin-helper
@jbenet
@jmac-sead
@laurentsenta
@mishmosh
@momack2
@protocolin
@raulk
@Stebalien
You've been @mentioned because your filecoin-project github "org ownership" permissions are proposed for removal as part of #47 unless you respond back with your use-case for having these broad permissions by 2024-08-28. You will still be a member of the filecoin-project github org, retain your existing direct github repo permissions or team permissions, and be able to make permission requests through https://github.com/filecoin-project/github-mgmt.
The current plan is to merge this change on Wednesday,
2024-08-282024-09-04, if there are no blocking comments/feedback.That said, this isn't a one-way door. If we got this wrong or you don't see the notification until after-the-fact, a new PR can be created to fix permissions.
Thanks and let me know if you have any questions or concerns.
Who is affected and how
Below is the textual summary of the changes. I'll keep it updated, if/as the code updates as well.
Retained org ownership (rationale is documented code):
Removed org ownership but are part of "github-mgmt Stewards" which effectively has the ability escalate to org ownership permissions (rationale is documented code):
Removed org ownership because have not been active in org administration the last 6 months[*]:
Removed org ownership because are part of sead, but haven't been active in administering the org and still have representation with masterwayne-admin[*]:
Removed org ownership event hough have been active in org administration because have representation with other FilOz team members:
Added org ownership given experience and diversifying representation:
Added to new github-mgmt-approvers team:
Added to the newly created security-managers team:
(This team will be given the security manager" role after being created as tracked in #47)
Added to the newly created moderators team:
(This team will be given the moderator role after being created as tracked in #47)
FAQ
Why are we removing so many owners, especially of longtime/foundational project members?
This is in no way meant to downplay the contribution of those affected who have been significant builders in shaping the project. Beyond the reasons listed in #47 , I'm suggesting remove folks because I view having these superpowers as a burden. Many of those removed haven't had filecoin-project github org activity in the last 6 months, and thus I assume they're not getting utility from carrying the burden. In addition, their accounts would be a prime account for attackers to go after, and this change reduces the blast radius in the unfortunate-I-never-hope-it-happens event that their accounts do get attacked.
My general mindset: Github org ownership permissions are very powerful, and they enable someone to do things in the UI without review that can have significant consequences. Reducing the ownership set to a small set of people helps ensure:
Why not remove all org owners?
I considered a larger extreme of effectively removing all org owners and then just relying on github-mgmt stewards to self-promote themselves to "org owner" when they need this (leaving a comment for why they need this and what conditions need to be met to be removed from the owner role). This seems like the safest thing to reduce the chance that org owners accidentally use their permissions in unintended ways (and reduces some of the potential blas radius if their account gets hacked - there would at least be a PR to github-mgmt that would show the escalation of permissions which may give someone the chance to react/respond.)
I sided against advocating for this because there are likely notifications that come through to owners that it would be risky to not have any human receiving.
Feedback welcome though if anyone thinks we should tighten further.
Why aren't more teams/groups represented on the "github-mgmt stewards" team?
FilOz and IDPX are well represented on that team (see code). The Filecoin Foundation now has representation. Many of those who lost org owner permissions have representation via "org owners" or the "githb-mgmt Stewards" team. More teams are not being proactively added to the list yet because:
A new "github-mgmt approvers" team was also created that has a wider pool of people who can approve (but not merge) PRs.
Reviewer's Checklist
Additional notes
[*] Audit log dump from the last 6 months was done here.