RFC: New criteria for commit access

I grant access to literally anyone who asks who is able to follow the instructions in the developer policy: effectively, they provide a github user id and some justification. Many people who ask don’t do this the first time around btw.

I don’t attempt to validate the justification for a variety of reasons, including that I’m not willing to set arbitrary bars based on “my judgement” as suggested upthread.

I do think that having an advocate/nominator/sponsor sort of thing would be a good addition to our process, but I’m not going to check to see the sponsor has commit access already :slight_smile:

can’t we just move our release binaries to a different github repo/org?

As I mentioned in my ^^ post, I don’t see how this matters at all. Why is the person who pushes two patches and disappears a supply chain risk? You’re not addressing the observed problems, and you are introducing new ones. In practicality, I don’t think we can achieve security by reducing access from 1500 people to 500 because we can’t vet 500 people either, particularly as they change affiliations and motivations over the years.

This is my objection to the proposal.

-Chris

11 Likes

Unused and forgotten credentials can pose a risk beyond simply whether the intended account owner is a trustworthy person.

IMHO, it makes sense to periodically revoke access which is unneeded or unused, independent of any of the other questions. That is: if we can revoke access for an account and have a high probability that the account owner won’t even notice, then why not do that. (E.g. if someone has done ZERO actions that required commit access in the last year, we can predict with high likelihood they don’t need it next year either.)

2 Likes

While this is true, I think if we keep our barriers for commit low to keep the community open to new contributors it will often be easier to just get commit access yourself than it will be to find someone’s forgotten credentials.

GitHub has already migrated basically every organization to requiring 2FA, and many of their API keys have short expirations by design. So the risk here would be a forgotten SSH key or legacy API token… I think getting commit access is a lower bar.

What this tells me is that to improve our supply chain security we should focus in different areas. Moving the releases is one thing we could do. We could also focus on improving our OpenSSF scorecard. Culling old contributors is not required to make progress on either of those areas.

4 Likes

I think that contributors are an asset instead of a burden to the community. It’s slightly strange to consider contributors as a risk to the community personally.

9 Likes

Again, this is not a dichotomy. It’s not because we have a more strict rule on who gets write access that we’re an “unwelcoming community”. It’s not because I can’t get commit access legally that I’ll “find someone else’s credentials”.

No one will “hack” someone else’s account to commit genuine code to LLVM, but it is not uncommon for hackers to introduce hidden code to harmless projects.

Revoking unused rights (partially or completely) is a fundamental part of risk mitigation. Open source projects are not immune to being targeted by rogue actors.

Totally agree. This is way more important than anything we’ve been discussing in this thread so far. It is, as I understand, the key motivation behind, but the direction we went was a red herring.

Not true. One of the points in the scorecard you pointed out is the “principle of least privilege”. Removing write access to all repos and make it granular, moving people as triage, separate releases from code all work towards that goal and are “more important” than removing old accounts, yes.

However, old accounts do have a higher risk of being misused by bad actors. Those emails may not exist anymore, the domain was hijacked by hackers after expiration, the authors don’t look at the messages anymore. All of those are very easy avenues into hijacking someone’s Github account.

Usually, when accounts are hijacked, they don’t do anything, just keep it dormant, until when they need it. If something happens later on, the author does not correlate the events.

This is much more likely to work on older unused accounts than people still working on Github repos. There was an incident where someone reported a leaked key. This is from someone who is monitoring emails and validating them. But we don’t know if the leaked key got used (unlikely, due to - I assume - early detection).

However, there could be any number of leaked keys from a number of old accounts, or previous domains hijacked and their accounts already in the hands of hackers and we don’t know.

Now, I don’t want to use falacies myself. This is not an argument for “slippery slope”, it’s not a “false dichotomy”, and I’m not advocating that we cull everyone because of it.

I’m just saying: removing old accounts “is one of the required steps” in cleaning up and strengthening our security. Just not the most effective one right now.

I still think that @hansw2000 original point to email people would work best for the forgotten accounts than any policy we have today, and will allow us to focus on the real problems, as exposed by @beanz score card point.

No one is doing that, as I explain above. It’s just the higher risk of silent hijacking.

3 Likes

Thanks for the additional feedback. It seems like there is not consensus on creating requirements for obtaining commit access, so I’m going to drop that part of the proposal. I’m keeping the rest of the proposal the same as v2, except that I’m dropping the 1 commit requirement to regain commit access so that people can re-apply for commit access at any time.

Requirements For Retaining Commit Access

In order to retain commit access, a user must have a total of 5 repository ‘activities’ in the last 12 months. These activities can either:

  • Pushing a commit.
  • Merging a pull request (either their own or someone else’s).
  • Approving a PR.

Revoking Commit Access

We will have automation that files a github issue every month which tags any users that don’t meet the threshold for retaining commit access. If a user does not respond to the ticket within a week, their repository role will be changed from Write to Triage. This will remove their commit access but allow them to keep privileges to do other things like:

  • Apply/dismiss labels
  • Close, reopen, and assign all issues and pull requests
  • Apply milestones
  • Mark duplicate issues and pull requests
  • Request pull request reviews
  • Hide anyone’s comments

Re-Applying For Commit Access

A user with the Triage role may re-apply for commit access at any time by following the process for obtaining commit access outlined in the developer policy.

Other Changes

We will add automation to remind reviewers to merge a PR when they approve one from an author without commit access.

2 Likes

If we go forward with that kind of policy, I’d make two alterations, both aimed at maintaining some flex for mistakes:

  • Change the notification period from one week to one month for downgrade to triage.
  • Have a fast path reinstatement process by reopening the issue and requesting reinstatement. Maybe put a time limit on it of being able to do this for six months.

I’ve definitely gone more than a week before while missing or purposefully ignoring notifications from some projects, and I’d like to not have to worry about one more source of urgency in my life from a volunteer project.

Further, I manage a number of open source projects and handling the influx of admin/access issues is always a problem. Administrative mistakes are common and having a low overhead way to correct them (ie. Not email Chris and “apply”) helps everyone and leads with respect to the busy people who help keep this stuff going.

I’m neutral on the plan, but if we do it would like to see these tweaks to soften it.

5 Likes

I think also if we publish this that we include a preface that includes some of the context in the discussion between beanz and rengolin upthread. It is critical that we show respect to our committers and the security situation is not universally understood:


LLVM has a policy of downgrading write access to the repository for accounts with long term inactivity. This is done because inactive accounts with high levels of access tend to be at increased risk of compromise and this is one tactic that the project employs to guard itself from malicious actors. Here is how this works:

In other words, make sure committers know this isn’t about them but about making them and the project less of a target for bad actors.

4 Likes

I’m in favor of the 5 activity account maintenance requirement, to give some positive feedback. I like @stellaraccident 's wording suggestions.

I would like to revisit the topic of requirements to gain commit access, but I respect your desire to limit scope for this proposal.

1 Like

Personally, I dislike policies such as this, as I do think it makes the community less welcoming. Since it seems likely that some version will move ahead though, I’ve offered some feedback below to hopefully make it a bit more palatable.

Could we perhaps broaden the scope of the last item (“approving a PR”) to include something like “interacting with a PR by commenting or setting approval status”?

Many people add important and insightful comments when reviewing PRs, even if they do not actually mark a final status (they may leave that to another reviewer, for example). I would think we should still count such work as “activity” for this purpose.

Additionally, “approving a PR” could be strictly interpreted to mean only positive approvals count. I would think setting any approval status (approved, needs changes, general comment) are all useful activities which should be counted.

5 Likes

I also wonder if this maybe assumes a certain model of contributor.

@mgorny (who previously had a job involving LLVM, but not at the moment) and I both maintain LLVM in Gentoo and the amount we need to commit really depends on luck / external factors. Some months it’s very quiet, and then we need to do a bunch of build system or test fixes.

It’d be a shame to have to keep asking for commit access to be restored when we’re long-term regular contributors with a stake in LLVM.

1 Like

One additional point of data to consider from the recent security report: 66 - llvm - LLVM special issue tracker - Monorail

A Github token with push access to repositories was accidentally pushed within a container to DockerHub. I think it’s reasonable to assume that people are automatically running secrets scanners on at least some significant portion of containers that get pushed there. It’s not possible to tell (at least from the outside) whether or not this person was active in the project at the time or if this policy would’ve helped in that specific case, and the token wasn’t exploited (as far as we know) and was caught quickly. However, someone who scrapes DockerHub + other places where a token might end up could reasonably end up with a token that has commit access to a supply-chain critical project and do some damage. I believe that LLVM’s situation with so many people with write access does increase the risk that this happens to us, but ultimately it’s a tradeoff as other people in the thread have already commented on.

Do you have a sense of how long you’ve gone without doing any of the actions described here? An entire year without doing any of those things seems not unreasonable - and if the threshold for re-requesting access is low, it’s at most a once a year thing. That doesn’t seem like a huge inconvenience? (a process that maybe takes less time than rebuilding from a year old checkout/getting things sorted to be able to commit again after that kind of delay)

You and @mgorny both meet the requirements to retain access, so you current level of contributions is fine. The worst case scenario (which seems unlikely) would be you need to ask for commit access once per year or have other people commit patches for you.

I think this is fine.

I like the idea, but in practice I don’t think this will necessary be faster than just emailing Chris. I wonder if this could be solved by having a better process in general for requesting commit access (e.g. filling issues) instead of just emailing Chris.

1 Like

Ok. Let’s at least remove the “apply” verbiage whatever we do. It struck me as off putting to suggest “back to square one”.

For the record, to compare if other compilers use GitHub releases to distribute binaries.

Language Included with releases
Rust Releases · rust-lang/rust · GitHub Source code archives
Go Releases · golang/go · GitHub Nothing
Swift Releases · apple/swift · GitHub Source code archives
.NET Releases · dotnet/roslyn · GitHub Releases · dotnet/runtime · GitHub Source code archives

The list isn’t representative, just included a bunch of popular projects. But the results are quite consistent. Interesting to know why has LLVM decided to use GitHub releases for distribution purposes (I believe that security risks weren’t known at that time).

I seem to have missed this thread entirely. However, I would like to express my entire support for @tstellar ’s proposal. I would even be OK with a stricter approach than what’s proposed in v3, but I’ll settle for that if that’s what we get consensus on.

Personally, I don’t buy arguments of « being less friendly to new contributors ». We’re on Github, there’s virtually no barrier to contributing even without commit access. No other open source project I know of has such a large number of committers — this is only a relic of how it had to work with Phabricator. I also suspect only a few maintainers would have commit access if we were setting this up from scratch now, and everything would work just fine.

I do agree that counting contributions is tricky, in particular we want to make sure that people who participate in code-adjacent activities can continue doing so effectively.

I view the liberal distribution of commit access as a consequence of a manual process that didn’t scale well and a requirement of our past infrastructure (Phabricator). Now that we have better, IMO it would be irresponsible not to lock things down a bit more. LLVM is at the root of almost all software stacks and it’s mind blowing that literally anyone can get write access to that just by asking.

Just my .02

2 Likes

Thanks for the additional feedback. Below is version four of my proposal. I’ve added ‘commenting on a PR’ as one of the activities that counts towards retaining commit access, extended the notification period from one week to one month, and added some better language explaining why we are doing this.

Requirements For Retaining Commit Access

In order to retain commit access, a user must have a total of 5 repository ‘activities’ in the last 12 months. These activities can either:

  • Pushing a commit.
  • Merging a pull request (either their own or someone else’s).
  • Commenting on a PR.

Implementation

We will have automation that files a github issue every month which tags any users that don’t meet the threshold for retaining commit access. If a user does not respond to the ticket within a month, their repository role will be changed from Write to Triage. This will remove their commit access but allow them to keep privileges to do other things like:

  • Apply/dismiss labels
  • Close, reopen, and assign all issues and pull requests
  • Apply milestones
  • Mark duplicate issues and pull requests
  • Request pull request reviews
  • Hide anyone’s comments

The issue will contain a friendly explanation of why we are doing this:

Regaining Commit Access

A user with the Triage role does not need any justification or have to meet any criteria to regain commit access. To restore their commit access, they just need to follow the process for outlined in the developer policy .

Other Changes

We will add automation to remind reviewers to merge a PR when they approve one from an author without commit access.

Impact

As of Feb 1, 2024 there were 1798 users with commit access to the llvm-project. If this proposal were to be adopted, 731 of those users would be switched from the write role to the triage role.

5 Likes

Explicit +1 to the most recent policy draft from me.

One minor point. For the first deployment of this, I’d encourage you to send out the notifications more than a month in advance. I’m not suggesting that for the ongoing part, but I see no reason not to give folks extra heads up the first time.