codecov / engineering-team

This is a general repo to use with GH Projects
1 stars 1 forks source link

Fix GH rate limit issues experienced by contributors forking open source repos that have Codecov Installed #1574

Open rohan-at-sentry opened 5 months ago

rohan-at-sentry commented 5 months ago

The issue is manifested in "tokenless" flow that was introduced to support v4. It would appear we're performing checks to verify provenance which doesn't afford that much protection.

ALso, explore making token requirement for open source repos optional to alleviate challenges to set up Codecov. However, this needs some level of privilege so a bad actor cannot simply make it a requirement.

rohan-at-sentry commented 5 months ago

more reading (internal to codecov ) https://www.notion.so/sentry/Tokenless-v3-proposal-54e07b5f7793410692e7954fc8bc23c4

rohan-at-sentry commented 5 months ago

Path forward

Step 1

Solving the GitHub rate limit issue

Tokenless authentication performs two GitHub API requests:

These checks don’t afford much protection. Neither matters for private repos because tokens are required unconditionally. For public repos, the first can be satisfied by scraping CI history or just opening a PR and the second doesn’t really prevent anything in particular.

Both checks can be removed if we rework the logic of tokenless/token auth:

Under this scheme, branches from forks (forkname:*) are not authenticated and can be spoofed, but branches from the upstream repo (main) are protected.

We need to implement this logic on both upload endpoints to solve the problem. These three bullet points should each have their own unit test.

Step 2

Making tokens optional without undermining their security

By default, tokens should not be required for public repos so that users who relied on the authentication of the old upload endpoint are able to upgrade their GH Action and onboard to the CLI. Some public repos will want to require tokens for their repos, and we should build a way for them to opt themselves into that requirement.

Enabling/disabling token requirements needs to be a privileged operation so that a bad actor can’t just disable it to troll. This means it can’t be done with a CLI switch or in a repo’s codecov.yml. Instead, we should build the ability in Gazebo for an admin to enable/disable a token requirement.

Phase 1

Phase 2

rohan-at-sentry commented 5 months ago

Expected Upload experience from Forks as a result of Step 1

matt-codecov commented 5 months ago

a couple implementation details for Step 1 that i am dumping here:

if this is put on anyone else's plate, happy to help them ramp up / write up more organized technical steps

matt-codecov commented 4 months ago

https://github.com/codecov/worker/blob/02be4f56149316f6be42fdbfa79d162dd32dd4d3/services/repository.py#L253-L258 leaving a code pointer here, this logic in worker will do forkname:branchname munging. i don't know if it will double-apply the munging to branches that had their branchname munged by the CLI already

rohan-at-sentry commented 4 months ago

folks at https://github.com/codecov/feedback/issues/358 are facing issues with forks being rate limited