Page MenuHomePhabricator

[terraform] Avoid including Terraform state and secrets in keyserver Docker images
ClosedPublic

Authored by ashoat on Jul 20 2024, 7:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 4, 2:03 PM
Unknown Object (File)
Fri, Nov 1, 10:45 AM
Unknown Object (File)
Fri, Nov 1, 10:45 AM
Unknown Object (File)
Fri, Nov 1, 10:45 AM
Unknown Object (File)
Fri, Nov 1, 10:45 AM
Unknown Object (File)
Fri, Nov 1, 10:44 AM
Unknown Object (File)
Sun, Oct 13, 6:59 AM
Unknown Object (File)
Sep 26 2024, 10:53 PM
Subscribers

Details

Summary

This addresses part 1 of ENG-8869.

Test Plan
  1. Docker build: docker build --build-arg HOST_GID=20 --build-arg HOST_UID=501 --build-arg COMM_JSONCONFIG_secrets_alchemy='{"key":"<secret>"}' --build-arg COMM_JSONCONFIG_secrets_walletconnect='{"key":"<secret>"}' --build-arg COMM_JSONCONFIG_secrets_neynar='{"key":"<secret>"}' --build-arg COMM_JSONCONFIG_secrets_geoip_license='{"key":"<secret>"}' --platform linux/arm64 -f keyserver/Dockerfile -t commapp/keyserver:sometag .
  2. Open the build: docker run -it commapp/keyserver:sometag bash
  3. Search for passwords via cd .. && (grep -R password_string . | grep -v node_modules)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat added inline comments.
.dockerignore
46–49 ↗(On Diff #42589)

Should we consider excluding all *.env, *.env.*, *.tfstate, and *.tfvars files in the entire repo? Seems safer than excluding them piecemeal like this, but I'm not sure if there might be any unintended effects

bartek added inline comments.
.dockerignore
46–49 ↗(On Diff #42589)

A problematic place might be CommTest CI, which builds docker images for each service and then uses Terraform to set up resources on localstack.

This revision is now accepted and ready to land.Jul 22 2024, 1:42 AM
.dockerignore
46–49 ↗(On Diff #42589)

Sounds like that would be easy to detect – I'll try it and we'll see if the CI fails

Try ignoring all *.tfstate and *.tfvars files

Looks like CommTest passes – would love to get another accept from either reviewer before landing, to confirm that this new strategy seems safe

This revision is now accepted and ready to land.Jul 22 2024, 10:57 AM