[security] Fix exposed push tokens through gh workflow_run

[mishig25]

Pretty much implements https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

tldr:

1. one workflow (without access to repo secrets) builds docs
2. another workflow (with access to repo secrets through wokflow_run) uploads/pushes the docs to the hub

todos (in order):

• revert 7935dc2 (done in faac33f)
• merge this PR
• add github secret to hf organization and/or individual repos if necessary (added org level secret and messaged maintainers individually when necessary)
• update doc-build github actions of all repositories that are using doc-builder (ex: [doc-build] Use new github workflows transformers#24013)

[sgugger]

I am very uneducated in secuitry issues but what prevents the malicious user to rewrite those new workflows and get access to the secret token?

[mishig25]

from: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

a new trigger workflow_run was introduced to enable scenarios that require building the untrusted code and also need write permissions to update the PR with e.g. code coverage results or other test results. To do this in a secure manner, the untrusted code must be handled via the pull_request trigger so that it is isolated in an unprivileged environment. The workflow processing the PR should then store any results like code coverage or failed/passed tests in artifacts and exit. The following workflow then starts on workflow_run where it is granted write permission to the target repository and access to repository secrets, so that it can download the artifacts and make any necessary modifications to the repository or interact with third party services that require repository secrets (e.g. API tokens).

if i understand it correctly, the excerpt above is saying that a malicious user might modify new workflows trying to get access to the secret token, BUT github will only run version of those workflows (specifically, workflows that are triggerred by workflow_run event) that are on main branch of a repository. Therefore, as long as we don't merge a PR that modifies those workflows, malicious user can't access those secrets

[coyotte508]

Looks mostly good to me.

I think that using GH webhooks to a HF space may be better in the long run, that way no need to add the secret to each repo, which is really annoying (especially since we're going to rotate them all).

The only concern is that currently someone malicious could overwrite the docs of another PR from the same repo.

Also, we can remove the delete_doc_comment and delete_pr_documentation workflows probably: PR docs are automatically deleted after 30 days anyway.

[coyotte508]

I am very uneducated in secuitry issues but what prevents the malicious user to rewrite those new workflows and get access to the secret token?

Secrets are never passed to workflows run from forks.

The workflow that uploads the docs here: https://github.com/huggingface/accelerate-wip/blob/main/.github/workflows/upload_pr_documentation.yml

It's only run from the main branch, and https://github.com/huggingface/doc-builder/pull/379/files#diff-f05826b801b9407ec985196b30fc45b111e09b5d98ee8670333493868e2b8dad it only downloads & reuploads an artifact

[mishig25]

that way no need to add the secret to each repo, which is really annoying (especially since we're going to rotate them all).

I think you can create organization-level secret. Which should solve this issue, no ?

PR docs are automatically deleted after 30 days anyway.

I didn't know it. Could you point me to a resource for conformation? If so, indeed delete_pr_documentation workflow is unnecessary

[coyotte508]

I think you can create organization-level secret. Which should solve this issue, no ?

It would help a lot yes :) A few repos (transformers.js, ...) would still need but it's a lot better

PR docs are automatically deleted after 30 days anyway.

I didn't know it. Could you point me to a resource for conformation? If so, indeed delete_pr_documentation workflow is unnecessary

#353

[sgugger]

Thanks for the explanation!

Definitely worth a try (as long as it's tested with PRs opened from forks, which is the main issue we are trying to solve :-) ).

[LysandreJik]

If you all think it's safe, then this sounds good to me!