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

feat(terraform)!: access backend behind firewall #261

Closed
wants to merge 20 commits into from

Conversation

sebastianlolv
Copy link
Contributor

Temporarily whiteliet github runner IP to firewall protected key vaults incase you need to create secrets etc. with Terraform.

@sebastianlolv sebastianlolv requested a review from a team as a code owner September 26, 2023 07:46
@hknutsen
Copy link
Member

hknutsen commented Sep 26, 2023

Good stuff 😄

I think this needs to be done for both the terraform-plan and terraform jobs?

Also to clarify, as far as I understand this wouldn't help with managing resources behind a firewall, but it'll allow you to use a backend with behind firewall.

Also remember to pin untrusted Actions to commit, ref.

@sebastianlolv sebastianlolv changed the title feat: temporarily whitelite github runner IP feat: temporarily whitelist github runner IP Sep 26, 2023
Comment on lines 17 to 21
subscription:
description: The subscription to deploy in
type: string
required: true

Copy link
Member

Choose a reason for hiding this comment

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

Is this not just a duplicate of the secret AZURE_SUBSCRIPTION_ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you use the subscription id on --resource-group tag inline command, if so, yes I think we can scrap that then.

Copy link
Contributor Author

@sebastianlolv sebastianlolv Sep 26, 2023

Choose a reason for hiding this comment

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

Can rather rename it to project_name or something similar? Since we need to reference it in the key vault name?

Copy link
Member

Choose a reason for hiding this comment

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

Didn't even notice that we're talking about a Key Vault here, and not a Storage account 😅

How would this work if you're running a Terraform config with multiple or no Key Vaults? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea 😅 I think I might have read this wrong, looks like they whitelist it in the tfstate storage accounts when I look at Radix's version of it. 😅 I can turn it into something like what they have done.

@sebastianlolv
Copy link
Contributor Author

Good stuff 😄

I think this needs to be done for both the terraform-plan and terraform jobs?

Where would you put the Revoke step in the terraform-plan job?

@hknutsen
Copy link
Member

I think it would be a good a idea to add the IP at the start of each job, then remove it again at the end of each job.

If not, then you might leave the firewall disabled for a very long time if the Apply job is not run right after the Plan job 🙂

Comment on lines 80 to 83
container_name: tfstate
key: ${{ inputs.environment }}.${{ inputs.project }}.terraform.tfstate
resource_group_name: ${{ inputs.project_id }}-tfstate
storage_account_name: st${{ inputs.project }}infra${{ inputs.environment }}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could consider updating the terraform-backend.sh script to store the backend config in a file, then pass that to this workflow so that the backend config can be read from it?

Then we could pass that same file to our Terraform config, using partial backend config: https://developer.hashicorp.com/terraform/language/settings/backends/configuration#partial-configuration

Copy link
Member

Choose a reason for hiding this comment

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

Would remove duplication of backend configuration, since with the proposed approach, we'd define backend config in both Terraform and GitHub Actions, if that makes sense 🤷

Copy link
Member

Choose a reason for hiding this comment

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

@hknutsen hknutsen changed the title feat: temporarily whitelist github runner IP feat(terraform): access backend behind firewall Oct 18, 2023
@hknutsen
Copy link
Member

Pushed some WIP code.

@hknutsen
Copy link
Member

Please note that this would require the service principal to be assigned role Contributor (or Storage Account Contributor) at the Storage Account scope.

@hknutsen
Copy link
Member

Linter currently throws error on duplicate code, which was expected. Will look into a more elegant solution.

@hknutsen hknutsen changed the title feat(terraform): access backend behind firewall feat(terraform)!: access backend behind firewall Oct 18, 2023
@hknutsen
Copy link
Member

hknutsen commented Oct 18, 2023

After some testing, I'm starting to think that this might not be the best idea.

  • In order to allow temporary updates to the Storage Account firewall, we must remove the read-only lock. This will allow all Contributors to modify the Storage Account.
  • In order to add temporary network rules to the Storage Account firewall, we must assign role Storage Account Contributor to the service principal, which means that if the SP gets compromised, so does our backend configuration.

Maybe it would be possible to get a dispensation for Storage Accounts used as Terraform backends?

Or maybe wait for this? github/roadmap#614

@sebastianlolv
Copy link
Contributor Author

sebastianlolv commented Oct 18, 2023

We could wait for that feature and try it, it looks like its in the Q4 2023 roadmap and could be released within the next months

@hknutsen hknutsen added the enhancement New feature or request label May 29, 2024
@hknutsen
Copy link
Member

hknutsen commented Nov 7, 2024

Considering this as stale for now.

@hknutsen hknutsen closed this Nov 7, 2024
@hknutsen hknutsen deleted the public-ip-temp-whitelist branch November 7, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants