search

thelastpickle / cassandra-reaper

Automated Repair Awesomeness for Apache Cassandra
http://cassandra-reaper.io/
Apache License 2.0
481 stars 216 forks source link

Security: PoC: Failing tests lead to ssh access via action 'mxschmitt/action-tmate' #1474

Open Hagfjall opened 4 months ago

Hagfjall commented 4 months ago

Problems:

  1. Anyone that will trigger workflow ci for pullrequest can get SSH access to the runner. Reason is that if tests are failing: https://github.com/thelastpickle/cassandra-reaper/blob/c1491e84b559441a8ebe32f74481115e9c0cc11f/.github/workflows/ci.yaml#L69-L72, it will automatically open a ssh server and wait for inbound connection. Whatever secrets are expose to the runner will also be exposed via the ssh-session.
  2. Changing the workflow definition will lead to the PR's definition of the workflow to be run. Can be problematic since the PR author can change and inject arbitrary code/definitions. Being able to extract environments and secrets set on the repository (depending on how they are set)
github-actions[bot] commented 4 months ago

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

adejanovski commented 4 months ago

Whatever secrets are expose to the runner will also be exposed via the ssh-session.

@Hagfjall, this isn't accurate. Anyone external to the organization will not get access to the secrets in these workflows (tmate or not). Also the tmate session is restricted to the ssh key of the user that created the PR in the first place, so tmate sessions from PRs initiated a committer will only be accessible by this specific committer. No external user or even another committer can access the runner through the created session.

Hagfjall commented 4 months ago

@adejanovski glad to hear that your secrets are not accessible for this workflow if it is triggered by someone outside of the org, like me :) That's perfect!

Regarding the tmate, you are partly correct. The ssh session will be configured with a keypair IF, and only if, the actor has ssh key configured in his/hers github account. See https://github.com/mxschmitt/action-tmate/blob/a283f9441d2d96eb62436dc46d7014f5d357ac22/src/index.js#L132-L142 Can be improved by requiring a ssh key like this:

      uses: mxschmitt/action-tmate@v3
      with:
        limit-access-to-actor: true

I just wanted to communicate the security issue, and possible improvement as a proof of concept. OK for me to close this PR, let me know if you want me to close it from my end.

adejanovski commented 4 months ago

Thanks for looking into this @Hagfjall, it's always good to be challenged around security. I think your suggestion of using limit-access-to-actor: true should be implemented for improved security. If you feel like pushing a PR for this, it would be much appreciated. Thanks again!