Security implications of a public repository

[oocube]

Oolite (and OoliteProject in general) hosts a public repository. Anyone can clone it, perform changes and bring them back in form of Pull Requests. This per se allows the public to contribute to the project.

This on the other hand includes a security risk. There are multiple attack vectors (which I do not want to list right here at this moment).
What is our posture to such risk? What countermeasures are we happy with?

[LW-archlinux]

Set up a structure that ensures multiple people have to agree on changes .

A possible implementation using a 3 tier setup

Dev roles : Junior , Senior, Veteran
PR classification : minor, major, critical

Upgrading to a higher classification should be doable for all dev tiers.
Downgrading major to minor would require senior or higher. Downgrading from critical would require veterans.

Merging would require multiple reviews, a minor change could mandate review by 1 juniors + 1 senior/vet .
major change could require 1 senior + 1 vet.

[oocube]

This would work to decide whether we want to merge or not. By that time something has already happened...

We need to understand each and every code line. We would need to ensure Oolite still runs with that change. Essentially we would build and run the project. I'd like to have the build and test-run to be automated, even before a human being looks at it. But that is the point: When executing the test, this new, untrusted code already executes. Even more, a PR can change our workflows and run havoc there, in our beloved project.

So how can we run a workflow and build/test the code with minimal security risk?

[oocube]

Here are two resources that even reference more regarding the topic:

• https://blog.gitguardian.com/github-actions-security-cheat-sheet/
• https://github.blog/security/supply-chain-security/four-tips-to-keep-your-github-actions-workflows-secure/

My worst concern to all the patterns of what you should do or not in the workflow is that the workflow files are part of the repository and can be changed by anyone using a PR. Yet they get executed in our project scope and using our secrets. I know that is possible since I created, modified and tested the workflow in a side branch, then ran PRs before finally merging onto the main branch.

Google Gemini, when asked "In a public Github repository, how can I prevent others to tamper the workflow files in pull requests? " responded this:

To protect your GitHub Actions workflow files from being tampered with or maliciously executed via pull requests in a public repository, you need a multi-layered approach. Because anyone can fork a public repo and submit a PR, you must prevent both the merging of tampered files and the unauthorized execution of those files.

Here is the best-practice setup to secure your workflows.

1. Require Approval to Run Workflows (Crucial for Public Repos)

Even if you block a PR from merging, a bad actor can submit a PR that alters a workflow file to mine cryptocurrency or steal secrets, and GitHub Actions might automatically run it. You must configure your repo to require approval before running workflows from outside contributors.

Go to your repository Settings.

On the left sidebar, click Actions, then General.

Scroll down to Fork pull request workflows from outside collaborators.

Select Require approval for all outside collaborators (or at minimum, "Require approval for first-time contributors").

Click Save.

Note: This ensures that a maintainer must manually click "Approve and run" before any code from a fork triggers your Actions.
2. Use a CODEOWNERS File

You can designate specific users or teams as the strict owners of the .github/workflows/ directory.

Create a file named CODEOWNERS in the .github/ directory (or the root) of your repository.

Add the following line to assign ownership of the workflows directory to specific maintainers (replace @username with your GitHub handle or team name):
Plaintext

# Require specific reviewers for any changes to GitHub Actions workflows
/.github/workflows/ @username

3. Enforce Branch Protection Rules

The CODEOWNERS file does nothing on its own; you must enforce it using Branch Protection rules so that no one can bypass the required review.

Go to your repository Settings.

On the left sidebar, click Branches.

Click Add branch protection rule (or edit an existing one for your default branch, e.g., main).

Under Protect matching branches, check the following:

    Require a pull request before merging

    Require approvals (set to at least 1)

    Require review from Code Owners (This is the critical step that activates the CODEOWNERS file).

Click Create or Save changes.

4. Optional: Create a "Lockdown" Workflow

If you want to be extremely strict and automatically fail any PR that attempts to touch workflow files (so you don't even have to look at it), you can create a workflow that checks the PR files and fails the status check if .github/workflows/ is modified.

Here is an example using a popular third-party action (tj-actions/changed-files):
YAML

name: Prevent Workflow Tampering

on:
pull_request:
paths:
- '.github/workflows/**'

jobs:
block-workflow-changes:
runs-on: ubuntu-latest
steps:
- name: Fail the PR
run: |
echo "::error::Changes to .github/workflows/ are not allowed from external pull requests."
exit 1

(If you do this, make sure to add block-workflow-changes as a Required Status Check in your Branch Protection rules).
Summary

By combining Run Approvals (to stop malicious code from executing) with CODEOWNERS + Branch Protection (to stop malicious code from merging), your public repository's CI/CD pipeline will be securely locked down.

I could investigate more what to do about this but it won't help much if I cannot modify the security settings on the repository or the project.

[phkb]

I could investigate more what to do about this but it won't help much if I cannot modify the security settings on the repository or the project.

What level of access do you need? Is it a specific role you need to have?

[oocube]

I could investigate more what to do about this but it won't help much if I cannot modify the security settings on the repository or the project.

What level of access do you need? Is it a specific role you need to have?

I just added some settings on the OoliteStarter repository. My privileges there are sufficient.

[mcarans]

As I mentioned before, here are some thoughts on security (slightly updated):

It should only be maintainers who have write access to the oolite repo. Not all devs also need to be maintainers. Making any new dev a maintainer leaves open the possibility of branches or even the whole repo being deleted!

It is sensible for security reasons that people are not given immediate write access to the oolite repo when they start developing so we should expect that there will be PRs coming from forks like this old open one for example: #426

Currently, all new devs do not have write access to the oolite repo so cannot create branches in it. I only received write access when it was required to install readthedocs. I was developing for months without any write access so forking and PRs was the only option.

As for visibility of PRs from forks, as long as there is a PR then there is visibility to the work done in a fork provided the user doesn't delete their branch or fork.

I think if we make branches for every small fix directly on the main Oolite repo it will be hard to find the major branches and also it will make cloning the repo slower and slower - it already seems quite slow compared to other repos.

I think branches in the repo should be reserved for releases and joint efforts on a major feature. For example, things like a 1.93-maintenance branch or develop branch for the former or an SDL3 branch for the latter (since multiple devs could work on it).

[oocube]

It should only be maintainers who have write access to the oolite repo. Not all devs also need to be maintainers. Making any new dev a maintainer leaves open the possibility of branches or even the whole repo being deleted!

It is sensible for security reasons that people are not given immediate write access to the oolite repo when they start developing so we should expect that there will be PRs coming from forks like this old open one for example: #426

Currently, all new devs do not have write access to the oolite repo so cannot create branches in it. I only received write access when it was required to install readthedocs. I was developing for months without any write access so forking and PRs was the only option.

As for visibility of PRs from forks, as long as there is a PR then there is visibility to the work done in a fork provided the user doesn't delete their branch or fork.

There is no intent to change this behaviour.

Oolite is open source. That means everybody can download or read the code. On top of that, do we want the community to contribute or do we expect all the work to be done by just a couple of developers? Would the community run pull requests against our repository? That is some kind of write access. How would we handle such contributions? Each of them is potentially dangerous.

Have a look at this video what may happen, and that even came from some trusted developer.
https://www.youtube.com/watch?v=MjKxgBi8Jio

I think if we make branches for every small fix directly on the main Oolite repo it will be hard to find the major branches and also it will make cloning the repo slower and slower - it already seems quite slow compared to other repos.

I think branches in the repo should be reserved for releases and joint efforts on a major feature. For example, things like a 1.93-maintenance branch or develop branch for the former or an SDL3 branch for the latter (since multiple devs could work on it).

This is a completely different matter. I like your idea of having release branches. But at the same time I like the idea of having feature branches. While you may have seen me creating many branches, how many of them are left in the repository?
Git was built with all this in mind, so there is no abuse.

If the repository is loading slow, we could reduce it's size. Let's move all static resources that hardly ever change to a separate repository. ;-)

[mcarans]

But at the same time I like the idea of having feature branches. While you may have seen me creating many branches, how many of them are left in the repository? Git was built with all this in mind, so there is no abuse.

It depends what you mean by feature. I would see SDL3 as a feature, but this little bug fix does not warrant creating another branch in the oolite repo. I would define "feature" as a branch that is likely to be long lived + a large piece of work. Aside from finding it harder to find branches if every small change is a feature, there is also the large number of GH Actions workflow runs that are generated by working directly on the Oolite repo. During iterative development in a fork, all the runs happen in the fork's GitHub and only when the PR is made do they start appearing in Oolite GitHub.

If the repository is loading slow, we could reduce it's size. Let's move all static resources that hardly ever change to a separate repository. ;-)

:-) Technically correct, but putting them in another repo doesn't save overall build time as that repo must be cloned for the build to work. What might work is having an artifact of static resources generated by another repo that can be put in a central location (so only downloaded once) such as ~/.local/share/oolite (like the Mozilla library) and used by multiple checkouts of the oolite repo (I usually have 2 or 3) but that would need to be set up for both Windows and Linux (unlike the Mozilla library). There's an argument though that these resources are core Oolite unlike the Mozilla library so should just be in the main repo.

[oocube]

I added a CODEOWNERS file in #637. This is a half-step to prevent unauthorized changes to the workflow files. It still requires a change on the repository settings.

[oocube]

Changed the test workflow to only run on internal (trusted) code. Sounds odd...

[mcarans]

Changed the test workflow to only run on internal (trusted) code. Sounds odd...

Will the tests run on PRs?

[oocube]

Changed the test workflow to only run on internal (trusted) code. Sounds odd...

Will the tests run on PRs?

Confirmed.
https://github.com/OoliteProject/oolite/actions/runs/25973546451

Edit: Ah, maybe I was not clear.
The test workflow always executes and builds the project. But for external PRs it will skip executing Oolite to take a screenshot. So you might want to compare the behaviour to this run:
https://github.com/OoliteProject/oolite/actions/runs/25973979649

[mcarans]

for external PRs it will skip executing Oolite to take a screenshot

We need to be able to run the tests with screenshots to know that external PRs haven't broken anything before merging. If we get to review whether or not workflow execution starts for external PRs, would that be sufficient (as we can check for malicious source code changes then)?

[oocube]

I wrote...

We need to understand each and every code line. We would need to ensure Oolite still runs with that change. Essentially we would build and run the project. I'd like to have the build and test-run to be automated, even before a human being looks at it. But that is the point: When executing the test, this new, untrusted code already executes. Even more, a PR can change our workflows and run havoc there, in our beloved project.

So how can we run a workflow and build/test the code with minimal security risk?

mcarans wrote...

for external PRs it will skip executing Oolite to take a screenshot

We need to be able to run the tests with screenshots to know that external PRs haven't broken anything before merging. If we get to review whether or not workflow execution starts for external PRs, would that be sufficient (as we can check for malicious source code changes then)?

I think that is a good choice. It would mean we first look at the code to understand, then run the automatic tests. They still enhance our roundtrip but the risk is vastly reduced.

Who can implement that? Approvals are not just a change in the workflow file (which btw still is not secured against changes in PRs). They also require changes on the repository settings.