I worry that deciding whether or not a commit is “non-trivial” can be gate keeping. This could have significant adverse impact by putting additional barriers up for new contributors especially ones who are less confident or less experienced.
I’m not convinced that putting contribution requirements as a barrier to gaining commit access actually helps from a supply chain security perspective. A sophisticated attacker wouldn’t have much trouble creating a GitHub account, fixing some “beginner” bugs and requesting commit access. Sure, it’s a bit of work, but for a sophisticated attacker that doesn’t seem like a stretch. It certainly seems to me that fixing 10 beginner bugs is easier than hacking someone’s 2FA (although I’m not much of a hacker).
If we put code contribution requirements up as a barrier, I think it blocks contributions from other sources. Specifically, it is extremely useful for my team’s project manager to have access to the LLVM issues for triaging and other project managerial things.
I’m generally in favor of culling commit access lists (and code owners), but I also think we need to find the right line to calibrate on. I’d feel more comfortable with your current proposal if we made the timeframe 18-months. Some countries have up to one year of parental leave, and I think we should make sure that a process we adopt for culling committees respects that.
One of the big challenges I see with supply chain security is the identities of contributors. GitHub allows a great degree of anonymity, which basically makes one of the bullets on Red Hat’s page a really hard problem:
Know your suppliers and who you do business with, starting with your tier-one suppliers. Conduct risk assessments to evaluate each supplier’s cybersecurity posture and public policies on vulnerabilities.
If I can create a GitHub account tied to an anonymous email account with no verified identity and get LLVM commit access for that account, we really have no idea who we’re trusting.
This probably speaks to a requirement for some sort of identity verification requirement.
Technically, if they don’t have commit access, they can’t merge a PR for someone else: did you mean “author a PR”? (you spelled it “have a pull request merged into the project” later in the message).
Otherwise: what about “A user who previously had commit access just needs to ask to be reinstated”, this seems in line with “Users can ask to keep their commit access even if they are under the limit”?
I worry that deciding whether or not a commit is “non-trivial” can be gate keeping. This could have significant adverse impact by putting additional barriers up for new contributors especially ones who are less confident or less experienced.
This is definitely something we should think about. I don’t think this is a significant regression to the current situation though. I trust Chris to make proper decisions here, but it is ultimately up to his discretion. This proposal might help slightly as it makes the process for obtaining commit access publicly auditable (through Github issues), but it is ultimately still up to human discretion. Implementing some of the ideas within this RFC do help enable other efforts that could help within this area like allowing multiple people to grant access (which has its own merits and demerits) and perhaps other efforts that could help improve the situation.
I’m not convinced that putting contribution requirements as a barrier to gaining commit access actually helps from a supply chain security perspective. A sophisticated attacker wouldn’t have much trouble creating a GitHub account, fixing some “beginner” bugs and requesting commit access. Sure, it’s a bit of work, but for a sophisticated attacker that doesn’t seem like a stretch. It certainly seems to me that fixing 10 beginner bugs is easier than hacking someone’s 2FA (although I’m not much of a hacker).
A sophisticated attacker could definitely gain access that way and potentially also through other methods. I think the most likely scenarios that this proposal helps with are attackers being opportunistic when they find a Github token/account credentials with access to a project like LLVM or large sets of credentials being sold and then combed over to gain access to supply-chain critical projects like LLVM. Maybe those are common attack vectors, maybe not. However, this proposal does reduce the possible attack surface area, which is something we should strive to do when it doesn’t significantly impact the communities ability to get work done.
If we put code contribution requirements up as a barrier, I think it blocks contributions from other sources. Specifically, it is extremely useful for my team’s project manager to have access to the LLVM issues for triaging and other project managerial things.
Specifically for issue management and repository access that doesn’t necessarily require commit access, I agree there will be a gap there created by this proposal. I think we need to investigate further if the triage role is able to cover these use cases and set up a system for people to be able to request triage access to perform these sorts of duties.
As has been mentioned several times previously and I think there is a consensus on the idea if not the actual implementation of it, but we do want to count non-commit related activites like reviews where having and maintaining commit access would be useful.
I’m generally in favor of culling commit access lists (and code owners), but I also think we need to find the right line to calibrate on. I’d feel more comfortable with your current proposal if we made the timeframe 18-months. Some countries have up to one year of parental leave, and I think we should make sure that a process we adopt for culling committees respects that.
This is a good point. I think making it easy for users to regain their commit access might be better than a hard number. There will definitely be situations where the hard number ends up being a barrier, but having a process to regain commit access would help mitigate this problem somewhat I believe.
If I can create a GitHub account tied to an anonymous email account with no verified identity and get LLVM commit access for that account, we really have no idea who we’re trusting.
This is a good point. However, there will still be some compromise when implementing a decision like this. There are quite a few online communities where many are not comfortable sharing their real life identities for one reason or another and there can often be a strong overlap between these communities and low-level programming folks, including the LLVM community. I know this was brought up with Linux’s developer certificate of origin, but I haven’t looked too much into the experiences over there. It’s a policy that does decrease attack surface, but also does come with compromise (as I guess most things do).
This goes back to @hansw2000 proposal to ask. Proactively asking and blocking in case of no response, but quickly reinstating if the user was away or missed the email.
For the continuous blocking on random metrics, genuine contributors can get lost (long breaks, career change, health issues, etc) but still get back where they left once they get back on track.
At the very least, this would clean the unused / forgotten accounts, which from a security point of view has lower chance of spotting leaks / hacks.
It does nothing to fix the bad actor case, but we already have that problem for sufficiently crafty actors.
As someone who objected to the original, I thought I should just note that the latest proposal (with the tweak to allow reinstatement simply by asking) satisfies my concerns.
What about using a dedicated website (or the GitHub Discussion area) to issue/revoke commit access instead of GitHub issues? As there are too many issues, using it for non-project purposes makes exploring issues much harder.
What if we had some general guidelines as to what qualifies as a “non-trivial” contribution? Also, if there is a dedicated group of approvers, could we have, say, 2 people from that group decide whether the contributions are sufficient? This should make the decision more objective.
My small concern with counting number of commits/pull requests is that it incentivizes breaking up contributions in smaller pieces than logically necessary. I don’t think this is a big issue, but I wanted to bring it up.
Thank you for raising this discussion, this is an important thing to circle back on and discuss.
Some quick points of agreement: 1) supply chain security is important, 2) access control is important, 3) me granting commit access is legacy and I’d be happy for that to change. That said, as others have raised, we want an open and inclusive community, and need to find a balance here. The current policies are based around the idea that we have code review and a principle of “it’s almost impossible to do irreversible harm to a git repository”.
As to my personal opinion about the proposal, I don’t think it actually achieves the goals. Defining an arbitrary bar for “10” reviews doesn’t achieve anything, whether the patches are trivial or not: If someone serious wants to attach LLVM, they can provide 10 patches. This proposal might make us feel good by reducing risk from a PhD student, but I don’t think that is where actual supply chain risks would present themselves from.
Furthermore, your proposal doesn’t address the most motivating-to-me issues identified by your first post:
The rationale for proposing this change is that commit access on GitHub comes with a lot of other privileges, including but not limited to:
Uploading releases to the release page.
Viewing repository secrets.
Running and modifying GitHub Actions workflows.
Publishing packages, including container images.
Instead of excluding a thousand committers and making it more onerous to get commit access, have we considered directly attacking these problems? For example, why not move the releases to a different ACL? Furthermore, we do allow (and I think it is important to allow) direct pushes to the repository for “break glass” sorts of events, but we could escalate visibility for cases when this is used of this to reduce the risk by bad actors.
I think bringing things back to “lean on distributed review” and “prevent harm through ‘as little access as necessary for commits’ ACL” is a better direction to pursue here. Your bulleted points above seem like important things to fix for a variety of reasons anyway,
If we ever come into a position of asking people to use PRs for everything, even NFC patches (which also implies we have decent pre-commit CI that doesn’t require one wait for hours), we can require commits that doesn’t have PR associated with them to have something along the lines of [Skip Review] in the title. This makes such occasions auditable.
Apologies if this is a bit of a late and railing wall-of-text, but I’d like to
weigh in here somewhat. My central thesis here is that we’re optimising for the
wrong thing; we should optimize for (and enforce) readable and usable
well-curated history rather than lower the barrier to entry for new
contributors.
I was very pleased to learn that when LLVM migrated from self-hosted git to
github, direct pushes were still allowed. However, this potential change of
policy troubles me for the future of a clean and readable history given previous
experiences of letting github “handle it”.
I’ve been involved in a number of projects that moved from “inaccessible”
platforms to github or similar and subsequently try and use such platforms’
admin tooling to solve social problems.
It’s been my experience that the git history gets worse when you
force people through the funnel of github’s UI and prevent them from using git
directly. I’d like to tender that
a) although many of these policy tools appear nice on paper, they do infact
destroy the good habits of those experienced with git, and b) In the long-run,
the ability to understand project archaeology is much more important than not
needing to tell your users they need to curate their patchsets properly. By
enforcing a very limiting technical restriction on how project history works,
you make the git history much less useful for tracking down bugs or making good
design decisions later.
Here are some things that absolutely ruin good intentions:
Enforced squash and merge:
Sometimes a patchset includes multiple commits that belong together. They may
have been curated that way. Enforcing that patches are squashed by the github
tooling destroys the good behaviour of leaving a clean archeaological trail,
and has a number of other serious potential side effects:
The github PR description becomes the squashed commit message. Often this
does not track what has happened across multiple commits, or even follow
what’s happened during review, where semantics might have changed.
It also messes with authorship here. I’ve seen cases where someone has
submitted patchsets that involved cherry-picked piece of groundwork by
another author, followed by their own feature or improvement on top of it.
They’re legally different authors, but the enforced squash-and-merge policy
means that all content was authored by a single author. There may be legal
implications for this.
It forces maintainers to spend more time than necessary in the github UI,
which is nowhere near as fast, featureful, or powerful as the real git.
github’s review model doesn’t track review comments across rebases. This
means reviewers lose track of “resolved comments” across a rebase. Phab didn’t have this
problem (and other tools like gitlab don’t either). This is tangentially
related to the present discussion of the restriction of direct pushes as
I’ve see that reviewers get upset when a person rebases their patch and try
and set policy to prevent this happening. We’ve always had a linear history,
and that’s historically meant people rebasing their work manually on
top-of-tree before pushing to main.
That’s not a difficult requirement, and trains users to think before they
act.
The “squash-and-merge” button removes the user from decision making, and
prevents those with good intentions from creating really nicely formatted
and well-structured changes.
Preventing direct pushes
Assuming users also have the ability to press “merge” in the github UI, all
this really does is cause accessibility issues. As Chris says, I might be able
to force-overwrite the history by pushing directly, but all that’s going to
happen is I get booted out of the project and someone pushes the real history
again. The outcome is not much different to me creating a PR that deletes all
files, and then pressing “merge” - someone will revert it 5 minutes later and
boot me out of the community. All that happens when you prevent people from
using sharp tools, is they use blunt ones instead. Blunt tools are a
liability.
Maintainers can no longer check push others’ patches with local edits.
git is not github
While it’s inevitable that some github-specific language ends up in the
history, I find it very frustrating when looking through a git log and seeing
only github usernames / github merge request IDs / fake github-generated email
address rather than real names and
git SHAs that I can use directly with git show etc. Not everyone wants to
(or can) switch to their browser every two minutes when running git log to
track down a pernicious bug. However, relying on the github machinery to
handle curating history automatically makes us more dependent on it. This was
one of the big deficiencies of having phabricator review IDs in the history
(until very recently): we now either have to keep phabricator alive forever,
or accept that context will be lost. If you approach with the mindset that you
must curate your history for future reading, then you will put more context
useful for future developers into the real history of LLVM.
@clattner What is the current criteria you use to determine if someone is eligible for commit access?
You are right, there is no criteria we can come up with that will prevent someone motivated enough from obtaining commit access and doing something malicious. The reason to have some kind of threshold like this is that it helps to lower the number of users who have write access. So, for example, someone who pushes two patches and then disappears doesn’t end up having commit access forever.
As I’ve said before, I think we are overrating the value of having commit access. When we were using SVN before Phabricator, having commit access was essential, when we started using Phabricator having commit access was still important, but less so. Now that we are using GitHub, I just don’t think it’s necessary for infrequent contributors to have commit access, and I think we should update our policies to reflect this.
When it comes to security, you are only as strong as the weakest link. It’s much easier to find a weak link in a group of 1500 than a group of 500. Especially if those 500 are all active and paying attention to security policies we implement as a project.
We don’t have a lot of flexibility here, we are pretty much stuck with what GitHub gives us. We could move the release somewhere else as @jroelofs suggested, but that still won’t prevent someone from uploading binaries to our repo’s release page.
It seems like there is some disagreement about whether reducing the number of LLVM contributors improves software supply side security without creating extra barriers to contribution. I don’t think we’re going to reach agreement on that point, so I think perhaps we should focus instead on what we want our committer criteria to be, since @clattner being the sole approver doesn’t work indefinitely.
I’m hearing that the 10 commit threshold is arbitrary. Have we considered instead doing some kind of nomination and seconding process? That would distribute the work of approving and vetting contributors.
Chromium’s committer critieria requires 10 patches, one nominator, and two seconding committers to gain committer status:
Two other committers need to second your nomination. 5 working days (U.S.) after the nomination, or 2 working days (U.S.) after the last message in the discussion, whichever is later, you’re a committer. If anyone objects or wants more information, the committers discuss and usually come to a consensus. If issues can’t be resolved, there’s a vote among current committers.
Should we drop the patch number requirement, and instead say that the nominee should have a substantial body of contributions, as determined by the nominators? Regarding voting, I’m not aware of that process ever being used, there’s usually consensus.
I still support the principle of revoking credentials that aren’t being used. Allowing commit access to expire shouldn’t mean that someone is ejected from the community. We should be inclusive and allow former committers to participate in decision making process. I think of it as more of password or SSH key rotation than a change in personal status. At least, what I’m envisioning.
+1 to various comments on the thread which suggest that whatever we do for current committers, we are liberal with Triage access going forward. In my opinion, with GitHub (which is not git), Triage access provides a good “I’m part of the project” experience and comes with virtually no (permanent) risk. I expect that there are many people who could be happy being high functioning project contributors with this level of access. Let’s make that trivial to get.
In relation to mitigations for supply chain artifacts specifically:
GitHub’s security model around much of these things leaves a lot to be desired. On some other community projects, we’ve discussed isolating supply-chain artifacts (releases, binaries, actions with write access, secrets, packages) to satellite repositories that have stricter permissions. Since much of the GH security perimeter is bound to a repository, it is something of a game of cat and mouse to have a generously open community repo that is simultaneously being used to orchestrate the building and distribution of potentially damaging artifacts.
I’m not a security expert, but I’ve navigated enough of GH’s settings pages to not believe that a project of this size will be able to define the ACLs in such a way (and have that hold over time in a way that any mere humans can understand) so that we can have it both ways.
If someone has been putting in patches, they must have reviewers/committers for those patches, which would be a reasonable pool of people to nominate/second.
A timeout that moves you from commit access to triage access seems like a reasonable policy. If you previously had commit access, and lost it due to a timeout, you should be able to get it back just for the asking.
Practically speaking, this is more or less how I’ve always done. Good work leading to nomination (as an email to Chris). I have no idea how many emails Chris get without nomination, just based on commits, but I’d support nomination as the strongest part of the equation.
What we want to avoid with the number of commits is “conflict of interest”, where people have 1 commit and are already nominated because they have a strong network (their company, colleagues, etc).
Arguably, this can happen at any number of commits (the network can approve a bunch of patches in succession), but it becomes more and more clear that this is not a good behaviour.
Do we envision dropping those, too, on (perhaps longer) inactivity?
That GitHub fails to distinguish commit rights from dealing with release binaries is not an immutable rule of physics. It’s a failing on the part of Microsoft to provide reasonable developer tooling.
I would suggest we complain at GitHub in preference to kicking 2/3 of our developers off the commit list.
Not wanting to make this a dichotomy, it isn’t, but asking Github for new features is like waiting hell to freeze over. Sure, with climate change that can actually happen.
Hell freezing, not Github listening.
There have been other proposals to split that, which are more realistic.
People here seem to have more opinions about that than I do. I’d fix the barn door by granting triage access more liberally this year: that will reduce pressure on commit access growth. Both probably need another evaluation a year from now, and I’d worry about triage pruning then.
