fix(docker): stop shipping the Firebase credential inside the image - #42
fix(docker): stop shipping the Firebase credential inside the image#42AndreaDiazCorreia wants to merge 1 commit into
Conversation
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47b77e0e9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if grep -qx "${secret}" <<< "${configured_secret_names}"; then | ||
| credential_present=true | ||
| break |
There was a problem hiding this comment.
Require an inline credential for standard Fly deploys
When an existing Fly app has only the previously required FIREBASE_SERVICE_ACCOUNT_PATH secret, this check still marks the credential as present and proceeds. However, the updated Dockerfile no longer copies the referenced file, and fly.toml defines no file or volume mount, so the server starts with FCM disabled and silently drops all FCM pushes. The Fly wrapper should require FIREBASE_SERVICE_ACCOUNT_JSON, unless it can verify that an external file mount has actually been provisioned.
Useful? React with 👍 / 👎.
Dockerfilecopiedsecrets/into a layer, so the Firebase service-account private key shipped with the image.docker saveanddocker historyreach it without ever running the container, and image access is governed by registry membership rather than by anything this repo controls. That set changed recently when the app moved to a shared Fly organisation, which is what moved this up the list.The credential now arrives at runtime
Two forms, exactly one required:
FIREBASE_SERVICE_ACCOUNT_JSONFIREBASE_SERVICE_ACCOUNT_PATHThe inline form leads for a specific reason rather than taste: the container now runs as UID 10001, and a file the platform mounts carries ownership and mode this project does not control. Fly's docs do not specify either for
[[files]], and the failure mode of guessing wrong is silent — FCM starts disabled and every push is dropped. An environment variable is readable by the process whatever its UID, and it is howSERVER_PRIVATE_KEYalready travels.load_service_accounttakes both values as arguments instead of reading the environment itself, so precedence is tested without mutating process-wide state. An empty inline value is treated as absent rather than as a parse failure, since a half-set variable should fall back to a working file rather than break.A second exposure, not in the issue
There was no
.dockerignore. The whole build context —secrets/,.env,target/,.git/— was sent to the daemon on every build, and Fly builds on a remote builder by default, so that content left the machine each deploy. Removing theCOPYcloses the image;.dockerignorecloses the context. The Dockerfile only needsCargo.toml,Cargo.lock,src/andconfig/.Making the failure loud
Removing the
COPYmakes "no credential" a likely deployment slip rather than a corner case, and the existing behaviour was to log awarn!and carry on — an instance that accepts registrations and delivers nothing, with one line of scrollback as the only symptom. Two changes:main.rslogs aterror!and names both variables. The server still starts, because a Nostr listener and an HTTP API without push beat no server at all.deploy-fly.shrefuses to deploy when neither credential secret exists. ItsREQUIRED_SECRETSlist previously demandedFIREBASE_SERVICE_ACCOUNT_PATH, which would have blocked every deploy using the inline form.Container hardening
useradd --uidalone picks the GID from the system range, soUSER 10001:10001would have named a group absent from/etc/group. Caught while re-reading the file, not by a build.WORKDIR /appwithdata/owned by the runtime user, so the UnifiedPush endpoint store still works if that backend is ever enabled.HEALTHCHECKagainst/api/health, plus[[http_service.checks]]infly.toml. These are not redundant: Fly ignores Docker health checks and runs its own, andfly statuscurrently reports no checks at all.curlis added solely for the Docker form; the tradeoff is called out in a comment so it can be rejected.Also fixed
docker-compose.ymlwas already broken for FCM before this change: it bind-mounted the credential but never setFIREBASE_SERVICE_ACCOUNT_PATH, so the service account resolved toNoneevery time. It also forcedUNIFIEDPUSH_ENABLED=true. Both corrected, with the UID 10001 readability requirement documented next to the mount.Verification
56 tests pass (7 new, covering precedence, the empty-value fallback, and every failure path),
cargo fmt --checkandcargo clippy --all-targetsclean,deploy-fly.shpassesbash -n,fly.tomlparses.The image itself is unverified. Docker was not available in the environment this was written in, and for a Dockerfile change that is a real gap. Worth running before merge:
Merge order
Touches
src/push/fcm.rs, as does #41. The production change is a small isolated hunk, but both PRs append a test module to the end of that file, so a conflict there is certain — mechanical to resolve by concatenating. Merge #41 first.Set
FIREBASE_SERVICE_ACCOUNT_JSONon Fly before deploying an image built from this Dockerfile.deploy-fly.shnow blocks the deploy if it is missing, but the ordering is secret first, deploy second.Closes #15