godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.61k stars 210 forks source link

CI workflow to notify API Docs automation (gdnative) #1047

Closed Bromeon closed 1 year ago

Bromeon commented 1 year ago

Equivalent of https://github.com/godot-rust/gdext/pull/259.

chitoyuu commented 1 year ago

Are we certain that it's OK for the PAT to be publicly available? Personally I would rather play it safe and provide it as a environment secret instead, even if it's scoped and restricted appropriately. At the very least it can still be exploited to perform DoS on the doc generation if I understand this correctly, I suppose?

Bromeon commented 1 year ago

I had a private token first, however the problem is that workflows triggered by pull requests cannot access repository secrets (for good reasons). So it would only work for pull requests opened by maintainers, or for the master branch itself.

I saw the following options:

  1. Use the pull_request_target instead of pull_request event. This gives PRs access to repository secrets. Needless to say, this is a massive security risk.
  2. Using a fine-grained token which only has access to very specific permissions on very specific repositories.
  3. Instead of triggering workflows from pull requests, have a scheduled job on the website that continuously scans open PRs for updates. While the most secure, it comes with lots of extra latency and state management to detect changes.

This PR would go for option 2. The PAT is a fine-grained token that does not even allow access to the website repo, but only an intermediate repo (dispatch-forwarder). That repo validates the request and forwards it if possible. A DoS attack would either hit our free CI compute limit, or could be countered by disabling GitHub Actions for that repo. It's definitely a risk, but at the same time there can be similar DoS attacks by creating thousands of issues, pull requests or comments on our main repos.

I would definitely not have done this with a Classic PAT, allowing wide-range access to repo permissions. Since the damage is limited to that one repo, I consider it justifiable. In an ideal world, instead of the forwarder repo, we'd have a rate-limited REST API with cheap compute costs but tons of resources. For now, if people think they need to abuse our infrastructure, then we may need to decide we can't have nice things and limit docs to master only.

Bromeon commented 1 year ago

One thing I was also looking into are environment secrets, but I couldn't find a way to restrict a workflow reliably to a certain environment.

The issue was that the environment is specified in the workflow file itself, meaning someone opening a pull request can easily just change that to "escape it". Maybe I was missing some concept here, but it didn't look to me like environments can be used as "security groups" to restrict external access.

GodotRust commented 1 year ago

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdnative/pr-1047

chitoyuu commented 1 year ago

I see. Thanks for the insights! I agree that given the context, this is a justifiable risk to take. This might be the first time I've seen dedicated repos being used for message passing, but it sounds very reasonable and should be effective at limiting damage in case someone found a way to exploit the tokens.

Bromeon commented 1 year ago

bors r+

bors[bot] commented 1 year ago

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here. For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.