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

chore: Fixing non-propagated custom GH token in forked PRs #134

Merged
merged 2 commits into from
Dec 24, 2024

Conversation

heyvister1
Copy link
Contributor

@heyvister1 heyvister1 commented Nov 18, 2024

Since custom GH secrets are not propagated for forked repos, we need a way to have GH token to generate docs repo PRs under network-operator repo.
Solution:

  • Use fine grained Github Personal Access Token (PAT), for nvidia-ci-cd user , with limited network-operator-docs permissions
  • There is a need to add GH_DOCS_TOKEN PAT token for nvidia-ci-cd user

@heyvister1 heyvister1 force-pushed the fix-non-propagated-gh-token branch from 6c986c3 to c911071 Compare November 18, 2024 10:28
@heyvister1 heyvister1 changed the title Fixing non-propagated custom GH token in forken PRs Fixing non-propagated custom GH token in forked PRs Nov 18, 2024
@heyvister1 heyvister1 changed the title Fixing non-propagated custom GH token in forked PRs chore: Fixing non-propagated custom GH token in forked PRs Nov 18, 2024
@heyvister1 heyvister1 force-pushed the fix-non-propagated-gh-token branch from c911071 to 859e91e Compare November 18, 2024 10:30
@killianmuldoon
Copy link
Contributor

What happens if some one creates a PR to this action with a script to exfiltrate the token before the check runs?

@heyvister1
Copy link
Contributor Author

What happens if some one creates a PR to this action with a script to exfiltrate the token before the check runs?

He needs to gain GH_TOKEN first, but he should be blocked on the first step which checks his membership

Comment on lines 38 to 42
response=$(curl -H "Authorization: Bearer $GH_TOKEN" \
-H "Accept: application/vnd.github+json" \
"https://api.github.com/orgs/Mellanox/teams/cloud-orchestration/members/$ACTOR")

if [[ $(echo "$response" | jq -r '.message') == "Not Found" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the GH API return an appropriate status code (404?) if the actor is not part of the organization?
If so, this could be shortened to if curl -f ... ; then?

@killianmuldoon
Copy link
Contributor

He needs to gain GH_TOKEN first, but he should be blocked on the first step which checks his membership

What if I create an MR which changes that first step?

@heyvister1
Copy link
Contributor Author

heyvister1 commented Nov 19, 2024

He needs to gain GH_TOKEN first, but he should be blocked on the first step which checks his membership

What if I create an MR which changes that first step?

So this is not only related to my PR change, if anyone can create its own workflow file which will be engaged in PR then we're in a bad shape.
For current status, docs repo can be triggered manually by maintainers. So non-privileged collaborators cannot engage it.
The following workflow is triggered by network-operator PRs which is executing this workflow from main branch only. So others PRs theoretically should not affects our workflow security posture.

@maze88
Copy link
Collaborator

maze88 commented Nov 24, 2024

He needs to gain GH_TOKEN first, but he should be blocked on the first step which checks his membership

What if I create an MR which changes that first step?

The current trigger configuration doesn't trigger on PRs, so shouldn't this not be a problem?

And either way - we can configure the repository (Settings -> Actions -> General -> Approval for running fork pull request workflows from contributors to Require approval for all external contributors.

runs-on: ubuntu-latest
env:
GH_TOKEN: ${{ secrets.GH_TOKEN_NVIDIA_CI_CD }}
GH_TOKEN: ${{ secrets.GH_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it GITHUB_TOKEN ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to make sure the GITHUB_TOKEN used in network-operator is sufficient.

@@ -27,10 +31,24 @@ jobs:
PR_NUMBER: ${{ github.event.number }}
PR_TITLE_PREFIX: "task: update documentation for"
steps:
- name: Check if PR actor is part of team
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think this is needed.

this workflow is only triggered by other workflows not PRs.
the other workflow needs to have the needed ppermissions in order to trigger workflow in this repo.
currently only members of Mellanox org can trigger workflows for this project (i just modfied this in setting)

A new step has been added to verify that actor is a member of
Mellanox/cloud-orchestration team
@heyvister1 heyvister1 force-pushed the fix-non-propagated-gh-token branch from 859e91e to 1f0bd13 Compare December 22, 2024 13:21
runs-on: ubuntu-latest
env:
GH_TOKEN: ${{ secrets.GH_TOKEN_NVIDIA_CI_CD }}
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we using this env var in the job ?

Copy link
Collaborator

@maze88 maze88 Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrianchiris see my comment from the original line 33 in this file:

# token must be explicitly set here for push to work in following step

@heyvister1 , although you renamed the secret variable (in line 33) - perhaps it's worth preserving the comment (unless you tested that after your change the token no longer needs to be explicitly set?).

@heyvister1 heyvister1 force-pushed the fix-non-propagated-gh-token branch from 1f0bd13 to 7145c22 Compare December 23, 2024 19:35
@@ -65,7 +65,8 @@ jobs:
run: |
git config user.name nvidia-ci-cd
git config user.email [email protected]
gh repo fork --remote --default-branch-only
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to fork docs repo, since there is already an existing fork for nvidia-ci-cd user

@maze88 maze88 merged commit da07c79 into Mellanox:main Dec 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants