feat: read security advisories from private GitHub repo (WT-1285)#17227
feat: read security advisories from private GitHub repo (WT-1285)#17227stevejalim wants to merge 1 commit into
Conversation
|
FYI @simon-friedberger once this merges, it'll be rolled out with the next Bedrock release - usually the same day. Monday is a likely target, depending on when it gets reviewed |
64b3d88 to
4088a41
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Bedrock’s security advisories ingestion to pull directly from the private GitHub advisories repo, adding PAT-based authentication support to GitRepo and plumbing a new settings value through the update command.
Changes:
- Add optional HTTP Basic auth for git clone/fetch via git’s
GIT_CONFIG_*env-var protocol (token stays out of argv/URLs/config). - Switch
MOFO_SECURITY_ADVISORIES_REPOdefault to the private repo and introduceMOFO_SECURITY_ADVISORIES_AUTH. - Expand git test coverage around auth parsing, env handling, and token non-leakage in argv.
Note: This review was performed following the repository’s custom Copilot instructions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
bedrock/utils/tests/test_git.py |
Adds tests for auth parsing/env creation and verifies tokens don’t land in argv/URLs. |
bedrock/utils/git.py |
Extends GitRepo to support authenticated clone/fetch via env-scoped git config and env merging. |
bedrock/settings/base.py |
Switches default advisories repo to private, adds MOFO_SECURITY_ADVISORIES_AUTH, and expands Sentry sensitive key list. |
bedrock/security/management/commands/update_security_advisories.py |
Passes the new auth setting into GitRepo for advisories updates. |
| username, token = self._split_auth() | ||
| basic = base64.b64encode(f"{username}:{token}".encode()).decode("ascii") | ||
| yield { | ||
| "GIT_CONFIG_COUNT": "1", | ||
| "GIT_CONFIG_KEY_0": "http.extraheader", | ||
| "GIT_CONFIG_VALUE_0": f"AUTHORIZATION: Basic {basic}", | ||
| "GIT_TERMINAL_PROMPT": "0", | ||
| } |
| MOFO_SECURITY_ADVISORIES_REPO = config( | ||
| "MOFO_SECURITY_ADVISORIES_REPO", | ||
| default="https://github.com/mozilla/foundation-security-advisories.git", | ||
| default="https://github.com/mozilla/foundation-security-advisories-private.git", | ||
| ) | ||
| MOFO_SECURITY_ADVISORIES_BRANCH = config("MOFO_SECURITY_ADVISORIES_BRANCH", default="master") | ||
| # "<github username>:<github token>" (or just "<github token>") used to authenticate | ||
| # git clone/fetch against the advisories repo when it is private. Empty = no auth. | ||
| MOFO_SECURITY_ADVISORIES_AUTH = config("MOFO_SECURITY_ADVISORIES_AUTH", default="") |
4088a41 to
aaf36d2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17227 +/- ##
==========================================
+ Coverage 82.76% 82.86% +0.10%
==========================================
Files 182 182
Lines 9751 9788 +37
==========================================
+ Hits 8070 8111 +41
+ Misses 1681 1677 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
janriokrause
left a comment
There was a problem hiding this comment.
PAT can still leak via Sentry locals. _auth_env() puts base64(username:token) in GIT_CONFIG_VALUE_0, and git() carries it through env/kwargs. On clone/fetch failure, Sentry may capture those locals. Current redaction names may not catch the encoded credential. What do you think?
| if not ADVISORIES_AUTH and settings.DEV: | ||
| # Local / dev environments typically don't have the PAT configured, | ||
| # so skip rather than failing scripts like bin/run-db-update.sh. | ||
| # In non-DEV (prod / staging) we fall through to the existing flow, | ||
| # which will fail loudly and alert Sentry if the secret is missing. |
e19027e to
9e1114b
Compare
Mozilla's security advisories are authored in a private GitHub repo and then synced out to a public mirror. Bedrock has historically read from the mirror. Keeping the two repos in sync has been a recurring operational pain point, so we're retiring the mirror and having Bedrock read directly from the private repo. That means Bedrock now needs authenticated access to GitHub for this data flow for the first time. GitRepo gains an optional auth kwarg accepting a "<user>:<token>" string (matching the existing FLUENT_REPO_AUTH convention). When set, an internal context manager supplies HTTP Basic auth via git's GIT_CONFIG_COUNT / GIT_CONFIG_KEY_0 / GIT_CONFIG_VALUE_0 env-var protocol, which sets a host-scoped http.<scheme>://<host>/.extraheader for the duration of the subprocess. Scoping by host means the Authorization header won't follow incidental redirects or LFS fetches against unrelated hosts during the clone or fetch; falls back to the unscoped global key for non-HTTPS remotes. The token never enters argv, the remote URL, the local .git/config, or the GitRepoState DB row. That keeps it out of any CalledProcessError payload and out of the Sentry event produced by the @alert_sentry_on_exception decorator on the management command. This is why we chose extraheader over the simpler token-in-URL pattern used elsewhere in the codebase. bedrock/settings/base.py adds a new MOFO_SECURITY_ADVISORIES_AUTH setting (env-driven, empty by default), flips the default MOFO_SECURITY_ADVISORIES_REPO to the private repo URL, and gains "token", "auth", "pat", and "git_config_value" entries on the Sentry desensitization blocklist. The first three protect against future variable-name drift; "git_config_value" specifically catches GIT_CONFIG_VALUE_0 (and any future _1/_2/...) since those keys don't substring-match the default blocklist -- without it, an exception in GitRepo.git would let the base64-encoded credential survive into the Sentry payload via the captured env / kwargs locals. A targeted test in test_sentry.py exercises that path with a realistic AUTHORIZATION: Basic <base64> payload. .env-dist gains a commented entry documenting the new var. update_security_advisories passes the auth setting through to GitRepo and skips entirely if MOFO_SECURITY_ADVISORIES_AUTH is missing -- there is no useful sync work to do without a PAT against the private repo. Prints a short "Skipping..." line so bin/run-db-update.sh logs make the reason visible. bin/run-db-update.sh moves the update_security_advisories invocation inside the AUTH=true block so the docker image build (which has no secrets) doesn't even attempt to run it. Deployment note: MOFO_SECURITY_ADVISORIES_AUTH must be present in the deployment secret store. Without it, the cron run skips the security-advisories step (printed to stdout, no Sentry alert). Existing SecurityAdvisory rows persist through any outage, so the public-facing pages keep serving while the secret is being put in place.
9e1114b to
7e98bb1
Compare
One-line summary
This changeset switches Bedrock to read security advisories directly from the private GitHub repo, with PAT-based authentication plumbed through GitRepo.
Significant changes and points to review
Mozilla's security advisories are authored in a private GitHub repo that's been mirrored out to a public one (
mozilla/foundation-security-advisories). Bedrock has read from the mirror. Keeping the two repos in sync has been a recurring operational pain point, so the mirror is being retired and Bedrock is moving to reading the private repo directly.That means Bedrock needs authenticated GitHub access for this data flow for the first time.
Worth a careful review is how the token reaches the git subprocess: we use git's
GIT_CONFIG_*env-var protocol to sethttp.extraheaderfor the duration of the subprocess. The token lives only in the subprocess environment;argv, the remote URL, the local.git/config, and theGitRepoStateDB row all stay clean. When reviewing, pay particular attention toGitRepo._auth_env,GitRepo.cloneandGitRepo.pull.bedrock/settings/base.pyflips the defaultMOFO_SECURITY_ADVISORIES_REPOto the private repo and adds the env-drivenMOFO_SECURITY_ADVISORIES_AUTHsetting. A small belt-and-braces tweak also adds"token","auth", and"pat"to the Sentry sensitive-value blocklist alongside the existingwith_default_keysdefaults.Deployment ordering note:
MOFO_SECURITY_ADVISORIES_AUTHhas been set in Dev, Stage, Test and Prod, but not in Demos. ExistingSecurityAdvisoryrows persist through any auth-failure window, so the demos will not start showing fresh advisories, but Dev and above will.Issue / Bugzilla link
JIRA: https://mozilla-hub.atlassian.net/browse/WT-1285
Testing
I've tested this locally with the relevant auth token, so CI passing here and a human review is enough