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

DDLS-434 part 1 of move to limited permissions on state file write #1782

Merged
merged 3 commits into from
Jan 2, 2025

Conversation

jamesrwarren
Copy link
Contributor

@jamesrwarren jamesrwarren commented Dec 30, 2024

Purpose

Multi part ticket to move to limited permissions on state file write

Fixes DDLS-434

Approach

With this PR I just wanted to do some pre work to sure the permissions we were using on our terraform were all set in the right places and that we aren't inheriting secrets incorrectly. I decided to do this first so that any changes we then make to the terraform roles, won't be affected by this bit of work.

What I have mainly done here is:

  • Remove inherit for added security and specified what we're inheriting
  • Moved the aws credentials login to directly prior to the step where it's needed so that any compromised package running in between could not exfiltrate the creds.
  • Explicitly unset the creds after use. As the last step is pretty much always the step that needs the creds, this isn't explicitly needed but I felt that it protects us in case we decide to add more steps later. It also makes the use of these creds much more explicit so we think more about their scope and how they are being used!

Learning

NA

Checklist

  • I have performed a self-review of my own code
  • I have updated documentation (Confluence/ADR/tech debt doc) where relevant
  • I have added tests to prove my work
  • The product team have approved these changes
  • I have checked my work for potential security issues and refered to the OWASP top 10

Frontend

  • I have run an in-browser accessibility test (e.g. WAVE, Lighthouse)
  • There are no deprecated CSS classes noted in the profiler
  • Translations are used and the profiler doesn't identify any missing
  • Any links or buttons added are screen reader friendly and contextually complete
  • If adding GA events, I have updated or checked the existing category or label values

@jamesrwarren jamesrwarren force-pushed the DDLS-434 branch 3 times, most recently from 19308d5 to 760f0e0 Compare December 30, 2024 17:33
@jamesrwarren jamesrwarren force-pushed the DDLS-434 branch 2 times, most recently from 531f801 to ccec8dd Compare December 31, 2024 09:13
@jamesrwarren jamesrwarren changed the title DDLS-434 attempt different state file format DDLS-434 part 1 of move to limited permissions on state file write Jan 2, 2025
@jamesrwarren jamesrwarren marked this pull request as ready for review January 2, 2025 13:14
@jamesrwarren jamesrwarren requested a review from a team as a code owner January 2, 2025 13:14
iqpalm
iqpalm previously approved these changes Jan 2, 2025
@@ -192,3 +192,9 @@ jobs:
docker tag $IMAGE_NAME:latest $ECR_REGISTRY/$ECR_REGISTRY_ALIAS/$IMAGE_NAME:main-$IMAGE_TAG
fi
docker push --all-tags $ECR_REGISTRY/$ECR_REGISTRY_ALIAS/$IMAGE_NAME

- name: Unset AWS variables

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might need an if: always() if it's supported here so that it unsets even when previous jobs fail

@jamesrwarren jamesrwarren merged commit 0d84c0d into main Jan 2, 2025
37 checks passed
@jamesrwarren jamesrwarren deleted the DDLS-434 branch January 2, 2025 15:45
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