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

Replace usage of ADMIN_GITHUB_TOKEN #505

Closed
blink1073 opened this issue Apr 30, 2023 · 12 comments · Fixed by #557
Closed

Replace usage of ADMIN_GITHUB_TOKEN #505

blink1073 opened this issue Apr 30, 2023 · 12 comments · Fixed by #557
Labels
enhancement New feature or request

Comments

@blink1073
Copy link
Contributor

Problem

We should not have to worry about the ADMIN_GITHUB_TOKEN. It is a maintenance burden and an additional security surface area.

Proposed Solution

We should instead create a PR with the commits that were previously pushed directly, and merge it from the Action. This will allow branch protections to remain in place. We can recommend the use of environments with restricted users in the publish action, regardless of whether OIDC tokens are used (#504).

@fcollonval
Copy link
Member

fcollonval commented Jun 19, 2023

I tried using environment and the trusted PyPI publisher action with https://github.com/jupyterlab-contrib/search-replace

For what I saw, the only default restriction you can put on environment is adding mandatory review or delay the execution. But app may be use to perform more complex validation rules.

From my test, I'm wondering if a solution could not be to create an app (like the jupyterlab-probot e.g.) that enforces execution within an environment.

From GitHub.com forum, a way could be to use expressions: https://github.com/orgs/community/discussions/26622. But it seems annoying to maintain a users list.

@blink1073
Copy link
Contributor Author

The workflow itself has to be marked as requiring an environment. I did a full end to end test [here|https://github.com/jupyter/security/issues/63]

@fcollonval
Copy link
Member

Thanks @blink1073

In what I tried I did something similarly to your test by adding mandatory review on the environment. I just found that this add a step to do the release; my laziness kicked in why one more step 😉 But yeah likely better to have to do one more click than having a bad release.

@blink1073
Copy link
Contributor Author

New plan: use the user's GITHUB_TOKEN, and recommend the use of an environment. Technically our check for the github actor's permissions would still work, but environment is still better.

To handle forwardport and silent changelog PRs, we add an option to make the PR comment to toggle the PR.

@blink1073
Copy link
Contributor Author

For release-from-releaser, we will need to keep using ADMIN_GITHUB_TOKEN.
Otherwise, use secrets.GITHUB_TOKEN by default.

Add a new option for post-PR comment, that will default to @jupyterlab-bot, please restart ci.

Update the docs and examples to always use an environment.

Test this against https://github.com/blink1073/test-python-project/ before committing to the process.

First, I need to update jupyterlab-probot to return the default config if no config file is found, because it is returning an empty object:

..."msg":"GitHub request: GET https://api.github.com/repos/blink1073/.github/contents/.github%2Fjupyterlab-probot.yml - 404"}
2023-12-24T02:48:50.916860+00:00 app[web.1]: 
2023-12-24T02:48:50.916862+00:00 app[web.1]: --------------------------------
2023-12-24T02:48:50.916878+00:00 app[web.1]: Handling Issue Comment Created:
2023-12-24T02:48:50.916891+00:00 app[web.1]:     repo: blink1073/test-python-project
2023-12-24T02:48:50.916906+00:00 app[web.1]:     number: 107
2023-12-24T02:48:50.916918+00:00 app[web.1]:     config:
2023-12-24T02:48:50.916953+00:00 app[web.1]: {}
2023-12-24T02:48:50.916967+00:00 app[web.1]: Ignored
2023-12-24T02:48:50.916980+00:00 app[web.1]: Finished handling of issue comment created

@blink1073
Copy link
Contributor Author

blink1073 commented Feb 17, 2024

Okay, I did some testing in my scratch repo, and here's my recommendation:

  • I think we should remove the release-from-releaser capability, since it is a security risk with using such a wide-scoped token. We should also remove support for PYPI_TOKEN, and instead only support id-token. This will require a major version bump.

  • For releasing from repo, there are two options for the access token used in publish-release and finalize-release:

    • Use an org-scoped GitHub App and generate Installation Access Tokens
    • Use Fine-grained access token with admin access that expires (not recommended)

For the GitHub App:

  • Create one per org
    • Add read/write permissions on contents
    • Select "Only on this account"
  • Create a private key and store it in the Jupyter Vault
  • I've already done this for the Jupyter org, and will test it out with one of the repos there.

Then, per repo:

  • Add a ruleset for the current set of branch permissions and allow the Github App to bypass them
  • Update existing branch permissions to only prevent force pushes
  • Add a ruleset for all tags and allow the Github App to bypass them
  • Remove any existing tag protections
  • Create a release environment with APP_ID variable and APP_PRIVATE_KEY secret from the vault
    • Require that administrators approve deployments
  • Run publish-release and finalize-release from the release environment

@blink1073
Copy link
Contributor Author

jupyter/nbconvert#2111

@blink1073
Copy link
Contributor Author

Good to go! https://github.com/jupyter/nbconvert/actions/runs/7959610271

@jtpio
Copy link
Member

jtpio commented Feb 19, 2024

Nice!

Create one per org

So that means for repos hosted under a user account (not part of an organization) the user will have to install a GitHub App for their account (likely to be treated as an "org" by GitHub already)?

@blink1073
Copy link
Contributor Author

Yep, here's mine:

image

@jtpio
Copy link
Member

jtpio commented Feb 19, 2024

Good to go! https://github.com/jupyter/nbconvert/actions/runs/7959610271

Looks like the commit corresponds to the user who run the workflow, which is nice for tracking and knowing who made the release (as previously discussed in #521) 👍

image

@blink1073
Copy link
Contributor Author

Okay, I'll open separate issues to discuss removing release-from-releaser and pypi_token. The PR closing this issue will really just be a documentation update and an update to the example workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants