README.md recommends an insecure default configuration

[ramblingenzyme]

Now that OAuth clients are supported, I think the recommended configuration in the README.md should show an OAuth configuration rather than an API key based one.

I also think it should suggest that you should configure an OAuth client with acl:read scope when running tests, and a second one with acl for applying updates. This prevents an escalation path where the ACLs can be updated from an arbitrary branch by updating the workflow, i.e.

name: Sync Tailscale ACLs

on:
  push:
    branches: [ "main" ]
  pull_request:
    branches: [ "main" ]

jobs:
  acls:
    runs-on: ubuntu-latest

    steps:
      ...
      - name: Deploy ACL
        # By commenting out this `if`, the ACLs get applied when the workflow runs on the PR trigger.
        # if: github.event_name == 'push'
        id: deploy-acl
        uses: tailscale/gitops-acl-action@v1
        with:
          api-key: ${{ secrets.TS_API_KEY }}
          tailnet: ${{ secrets.TS_TAILNET }}
          action: apply
      ...

For reference, this is the workflow we're using, and how we have the secrets configured.

name: Sync Tailscale ACLs

on:
  push:
    branches: [ "main" ]
  pull_request:
    branches: [ "main" ]

jobs:
  apply-acls:
    if: github.event_name == 'push'
    environment: production
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v3

      - name: Fetch version-cache.json
        uses: actions/cache@v3
        with:
          path: ./version-cache.json
          key: version-cache.json-${{ github.run_id }}
          restore-keys: |
            version-cache.json-
      - name: Deploy ACLs
        id: deploy-acls
        # Tailscale has released OAuth support for their action, but haven't cut a new release yet
        uses: tailscale/gitops-acl-action@287fb935799def5f8a2aef4df9b1286f78fc384b
        with:
          oauth-client-id: ${{ secrets.TS_OAUTH_CLIENT_ID }}
          oauth-secret: ${{ secrets.TS_OAUTH_SECRET }}
          tailnet: ${{ secrets.TS_TAILNET }}
          action: apply

  test-acls:
    if: github.event_name == 'pull_request'
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v3

      - name: Fetch version-cache.json
        uses: actions/cache@v3
        with:
          path: ./version-cache.json
          key: version-cache.json-${{ github.run_id }}
          restore-keys: |
            version-cache.json-
      - name: Test ACLs
        id: test-acls
        # Tailscale has released OAuth support for their action, but haven't cut a new release yet
        uses: tailscale/gitops-acl-action@287fb935799def5f8a2aef4df9b1286f78fc384b
        with:
          oauth-client-id: ${{ secrets.TS_OAUTH_CLIENT_ID }}
          oauth-secret: ${{ secrets.TS_OAUTH_SECRET }}
          tailnet: ${{ secrets.TS_TAILNET }}
          action: test

[rowanmoul]

I'm not sure I follow your solution to privilege escalation in PRs.
What is to stop me from making any number of other changes to the yaml in a PR that results in acl changes being applied before merging to main?
I think the solution is to just not allow anyone untrusted to make PRs in the repository in the first place.

Edit: I was not framiliar with "environments". I see there is a way to limit their use to specific branches, which would indeed prevent malicious changes to the yaml from being applied.

[gabeio]

@rowanmoul yes you can limit access to an environment based on a branch:

screenshot

Typically over in the repo settings under Environments > select your environment and you will be able to limit the environment to a branch or branches/tags/etc.

https://github.com/{user_or_org}/{repo}/settings/environments/{environment_id}/edit

[joneskoo]

Wasn't this fixed by #68 @mpminardi ?

[joneskoo]

I see, not completely. The API key was replaced by federation. The remaining problem I believe @ramblingenzyme describes is that where collaborators have write access to repository (can create branches). In such case, they could create an arbitrary workflow and gain control of the ACL write.

This is not specific to this action, but a general GitHub Actions security model property. Still, it would be good to take it into account in the README as it affects users in two ways:

1. In organizations where branch creation is allowed, with the workflow in the README, any user with write access can bypass the branch protection in main by creating a workflow on a branch. (Security concern, depending who has write)
2. In contexts where forks are required to make changes, the OIDC JWT id-token: write is not available to pull-requests, so the ACL test workflow would not work. (Secure, but broken)

Unfortunately a separate federation and client id is required to have different permissions for pull requests. I created #77 to demonstrate what the fix needed would look like. I don't think it should be merged; it makes the example quite complex.

Perhaps a better choice is to acknowledge in the README the threat model that the example assumes only trusted users are given write access to the repository, as branches may modify workflows.