Introduce authorize step for external PRs #2493

sentrivana · 2023-11-07T14:37:55Z

[DO NOT MERGE] The external environment has to be set up correctly first and this needs to be looked at by the security team.

Introduce an authorize step to all workflows that require access to secrets (currently just the AWS Lambda test suite). The workflow for fork PRs after this PR will be as follows:

an external contributor submits a PR
all workflows that don't need access to secrets run as before
any workflows that need access to secrets (currently just AWS Lambda):
- will now be run on pull_request_target instead of pull_request, which means they'll run against the base branch of the main repo and will have access to secrets
- will get an extra initial authorize step that checks whether the PR was made from a fork and if so, sets the environment to external
if any PR is running in the external environment, the repo is set up to require explicit approval from a Sentry employee before the workflow can be run
a Sentry employee checks the PR and manually approves the AWS Lambda test suite to run with access to secrets

Background

Our AWS Lambda test suite requires GitHub secrets to run. These are by default not accessible to PRs from forks. GitHub offers a solution for private repos, but for public repos, there is no dedicated solution.

The only way for fork PRs to get access to GH secrets of the original repo is to use pull_request_target. However, if this is used incorrectly it's an obvious security concern (see this and this). There are some options to prevent this, most of them relying on some sort of explicit approval from a maintainer. The two most prominent ones are:

creating a dedicated label that, when applied to a PR, triggers a test run
automatically applying a more restrictive environment to external PRs that requires explicit maintainer approval to run the tests (see https://iterative.ai/blog/testing-external-contributions-using-github-actions-secrets)

The former has the potential for race conditions. Since the label is not tied to a specific commit, a malicious code change pushed at roughly the same time the label was applied, but before the workflow has run, could lead to the workflow running unsafe code.

The latter (which is the solution in this PR) should not allow this since the test run is directly tied to a specific commit and any new code change invalidates any previous approval.

Closes #2487

sentrivana · 2023-11-07T15:31:19Z

@getsentry/security @asottile-sentry Hey folks, would appreciate your thoughts on this worfklow change 🙏🏻

asottile-sentry · 2023-11-07T15:59:40Z

.github/workflows/test-integration-aws_lambda.yml

 jobs:
+  authorize:
+    name: Determine environment
+    environment:
+      ${{ github.event_name == 'pull_request_target' &&
+      github.event.pull_request.head.repo.full_name != github.repository &&
+      'external' || 'internal' }}
+    runs-on: ubuntu-latest
+    steps:
+      - run: true


this is not clear what this does -- I expected a label being required here and I don't see it

as it is written right now it is unsafe as it checks out the pull request and then executes arbitrary code from it

The idea is to use environments instead of labels to control which PRs are allowed to be checked out and executed. The prerequisite is that the repo must be configured to have an external environment such that any environment: external workflows require a review from a Sentry employee before they're allowed to run. The authorize job above sets the environment to external if the PR was made from a fork.

Technically this should be better than using a label, since it revokes any existing workflow approvals automatically on new code changes without us needing to introduce another step to check for code changes and remove the label, and there is no risk of a race condition between granting the approval/label and new code changes being pushed.

More context: iterative/cml#574 (comment)

hmm nice if it works -- we should try this out in a separate repo first with less sensitive credentials

if this is the github feature I think it is we still might not want to do it this way though -- iirc it sends an email to all repository collaborators (read: the entire company) without a way to opt out

I tried this out on https://github.com/sentrivana/playground: here's the ci.yaml, the environment definition:

And here's a fork PR against the repo: sentrivana/playground#2

if this is the github feature I think it is we still might not want to do it this way though -- iirc it sends an email to all repository collaborators (read: the entire company) without a way to opt out

Ugh. Will test this some more to see if that's the case. If it is, will change this to use a label instead.

Thanks for the feedback!

mdtro

We typically use the label approach to achieve this functionality, so this is new territory for us. That said, the approach seems sound. 🙂

I do have some a related questions:

Is the same secret used for both internal and external environments?
- If so, could we separate these accounts and use one for each?
Can we verify that the associated account has the minimum amount of privileges as necessary?

sentrivana · 2023-11-10T12:33:46Z

We typically use the label approach to achieve this functionality, so this is new territory for us. That said, the approach seems sound. 🙂

Thanks for reviewing!

I do have some a related questions:

* Is the same secret used for both `internal` and `external` environments?
  
  * If so, could we separate these accounts and use one for each?

* Can we verify that the associated account has the minimum amount of privileges as necessary?

@antonpirker You're familiar with our AWS Lambda test setup, thoughts on this?

antonpirker · 2023-11-13T13:02:59Z

We have an AWS IAM user with the following permissions to run the tests:

So we definitely can have another user for external user and strip down the permissions (maybe only allow access to resources called "test_*" if this is possible, not sure)

I will create two sets of credentials with minimal permissions each in AWS, then we can move on

antonpirker · 2023-11-13T13:32:12Z

My plan now:

Make branch with only the AWS tests running (for easier testing)
Strip down the permissions as much as possible
make second account (with probably the same permissions, not sure if we need it then)
have the permissions definition in some sort of terraform or similar
maybe more...

antonpirker · 2023-11-14T08:55:40Z

I stripped down the permissions now to the minimum. Only the permissions really needed are set and only usable on resources that are prefixed with "test_" or have the name "python-serverless-sdk-test". (ok, listFunctions can list all functions, but when I tried to constrain this it did not work at all.)

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Allow",
            "Action": "iam:PassRole",
            "Resource": "arn:aws:iam::xxxxxxxxxxxx:role/lambda-ex",
            "Condition": {
                "StringEquals": {
                    "iam:PassedToService": "lambda.amazonaws.com"
                }
            }
        },
        {
            "Sid": "VisualEditor1",
            "Effect": "Allow",
            "Action": [
                "lambda:CreateFunction",
                "iam:GetRole",
                "iam:GetPolicyVersion",
                "iam:GetPolicy",
                "lambda:InvokeFunction",
                "iam:ListAttachedRolePolicies",
                "lambda:GetFunction",
                "lambda:DeleteFunction",
                "iam:ListRolePolicies",
                "iam:GetRolePolicy"
            ],
            "Resource": [
                "arn:aws:iam::xxxxxxxxxxxx:role/lambda-ex",
                "arn:aws:iam::xxxxxxxxxxxx:policy/*",
                "arn:aws:lambda:*:xxxxxxxxxxxx:function:test_*"
            ]
        },
        {
            "Sid": "VisualEditor2",
            "Effect": "Allow",
            "Action": [
                "lambda:ListFunctions",
                "kms:ListAliases"
            ],
            "Resource": "*"
        },
        {
            "Sid": "VisualEditor3",
            "Effect": "Allow",
            "Action": [
                "lambda:GetLayerVersion",
                "lambda:PublishLayerVersion"
            ],
            "Resource": [
                "arn:aws:lambda:*:xxxxxxxxxxxx:layer:python-serverless-sdk-test",
                "arn:aws:lambda:*:xxxxxxxxxxxx:layer:python-serverless-sdk-test:*"
            ]
        }
    ]
}

The tests run by internal and external PRs whould need exactly the same permissions. @mdtro should we still make two accounts?

antonpirker · 2023-11-14T09:07:14Z

For the record, I saved the permissions JSON in the Sentry password manager to the credentials for the user that should have these permissions.

antonpirker · 2023-11-14T09:08:53Z

The PR/branch I used. For faster testing this branch is only running the AWS Lambda tests:
#2503

We can fork this branch with an external user to test external PRs.

antonpirker · 2023-11-14T09:20:58Z

I have now also added a Budget to AWS Lambda of USD 50 per month. If the cost exceed this we will be notified via email. (We will never reach this limit, right now we are always in the free tier usage. Just to limit the damage in case something goes wrong)

sentrivana · 2023-11-28T10:56:42Z

Closing this in favor of #2538

Going with the label approach since we're already using it elsewhere.

Introduce authorize step for external PRs

2a6a93a

sentrivana added the Do not merge label Nov 7, 2023

sentrivana added 3 commits November 7, 2023 15:56

add missing file

29ce844

check out pr commit

32666b9

rename

efa2bf7

sentrivana requested a review from a team November 7, 2023 15:31

sentrivana marked this pull request as ready for review November 7, 2023 15:31

sentrivana marked this pull request as draft November 7, 2023 15:31

asottile-sentry reviewed Nov 7, 2023

View reviewed changes

Merge branch 'master' into ivana/run-aws-lambda-tests-on-forks

3f5d563

mdtro reviewed Nov 8, 2023

View reviewed changes

sentrivana mentioned this pull request Nov 28, 2023

Trigger AWS Lambda tests on label #2538

Merged

sentrivana closed this Nov 28, 2023

sentrivana deleted the ivana/run-aws-lambda-tests-on-forks branch November 30, 2023 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce authorize step for external PRs #2493

Introduce authorize step for external PRs #2493

sentrivana commented Nov 7, 2023 •

edited

sentrivana commented Nov 7, 2023 •

edited

asottile-sentry Nov 7, 2023

sentrivana Nov 8, 2023 •

edited

sentrivana Nov 8, 2023

asottile-sentry Nov 8, 2023

sentrivana Nov 10, 2023

mdtro left a comment

sentrivana commented Nov 10, 2023

antonpirker commented Nov 13, 2023

antonpirker commented Nov 13, 2023 •

edited

antonpirker commented Nov 14, 2023 •

edited by sentrivana

antonpirker commented Nov 14, 2023

antonpirker commented Nov 14, 2023

antonpirker commented Nov 14, 2023

sentrivana commented Nov 28, 2023

Introduce authorize step for external PRs #2493

Introduce authorize step for external PRs #2493

Conversation

sentrivana commented Nov 7, 2023 • edited

Background

sentrivana commented Nov 7, 2023 • edited

asottile-sentry Nov 7, 2023

Choose a reason for hiding this comment

sentrivana Nov 8, 2023 • edited

Choose a reason for hiding this comment

sentrivana Nov 8, 2023

Choose a reason for hiding this comment

asottile-sentry Nov 8, 2023

Choose a reason for hiding this comment

sentrivana Nov 10, 2023

Choose a reason for hiding this comment

mdtro left a comment

Choose a reason for hiding this comment

sentrivana commented Nov 10, 2023

antonpirker commented Nov 13, 2023

antonpirker commented Nov 13, 2023 • edited

antonpirker commented Nov 14, 2023 • edited by sentrivana

antonpirker commented Nov 14, 2023

antonpirker commented Nov 14, 2023

antonpirker commented Nov 14, 2023

sentrivana commented Nov 28, 2023

sentrivana commented Nov 7, 2023 •

edited

sentrivana commented Nov 7, 2023 •

edited

sentrivana Nov 8, 2023 •

edited

antonpirker commented Nov 13, 2023 •

edited

antonpirker commented Nov 14, 2023 •

edited by sentrivana