Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws/ci security fix #68

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Conversation

galmasi
Copy link

@galmasi galmasi commented Feb 5, 2024

This fix ensures that

(a) PRs based on forks can be opened on the attestation operator and will be evaluated by the AWS/EC2 framework
(b) no EC2 secrets are given away to the submitted of the PR -- all the PR code gets to see are temporary credentials for a short-lived EC2 virtual machine and kubernetes cluster.

The implementation is as follows:

  • we check out both the PR and target branches
  • All CI setup runs from the target branch (which is safe)
  • Once the EC2 test target exists, all tests are run from the PR branch (to evaluate the PR)
  • At the end of the exercise the CI setup from the target branch ensures that the EC2 resources are freed
  • All temporary credentials that were ceded to the PR branch lose meaning at the end of the test.

…up from target branch (which is safe), run tests from PR branch (to evaluate the PR)

Signed-off-by: George Almasi <[email protected]>
@maugustosilva maugustosilva merged commit 478c3ad into keylime:main Feb 6, 2024
2 checks passed
@mpeters
Copy link
Member

mpeters commented Feb 6, 2024

Just to understand the security context, for untrusted people they would still need to have their PRs approved by a maintainer (after code inspection) first before it runs in CI correct? Otherwise someone's PR could bypass the restrictions by having a loooong running CI job that did some crypto mining right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants