Prevent cleartext credential storage in Git Dag bundles - #64105
Conversation
|
Just to be very clear - this is not a vulnerabilty - this is at most security improvement. |
|
I updated the description and removed the scary RED thingie - as this is not a vulnerability, it might be a nice security improvement though |
Thanks for the guidance, Jarek!😍 |
065f045 to
9470b1a
Compare
|
@rjgoyln Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.
See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
|
The static checks are failing. Running |
Got it, thank you for the suggestion! |
2c8f1d0 to
10e59a4
Compare
|
@rjgoyln Thanks for your earlier reply — there are still five unresolved threads that need your attention. Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.
See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
|
Quick follow-up to the triage comment above — one clarification on the "Unresolved review comments" item: Once you believe a thread has been addressed — whether by pushing a fix, or by replying in-thread with an explanation of why the suggestion doesn't apply — please mark the thread as resolved yourself by clicking the "Resolve conversation" button at the bottom of each thread. Reviewers don't auto-close their own threads, so an addressed-but-unresolved thread reads as "still waiting on the author" and keeps the PR from moving forward. The author doing the resolve-click is the expected convention on this project. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
ephraimbuddy
left a comment
There was a problem hiding this comment.
Thanks for picking this up — moving off URL-embedded credentials to GIT_ASKPASS is the right direction, and the script content (mode 0700, shlex.quote, Username/Password branching, GIT_TERMINAL_PROMPT=0) all looks good.
One blocker before this can merge: the new GIT_ASKPASS / GIT_TERMINAL_PROMPT are set on self.env only, and self.env is just a dict on the hook — it isn't os.environ, so it does not reach the git subprocess for most operations.
The bundle only passes self.hook.env explicitly in one place — the initial clone (_clone_bare_repo_if_required → Repo.clone_from(..., env=self.hook.env)). Everywhere else:
_fetch_bare_repoonly forwardsGIT_SSH_COMMANDviacustom_environment._fetch_submodulescallsself.repo.git.submodule(...)with no env override.refresh()callsself.repo.remotes.origin.fetch(...)with no env override.
Before this PR these paths worked because credentials were baked into repo_url. After this PR the URL is clean, so HTTPS-with-token bundles will fail on refresh / submodule update, and because GIT_TERMINAL_PROMPT is also not propagated, the process may hang waiting for stdin on a worker.
Compare with the existing _passphrase_askpass_env (same file, just above) — it deliberately sets both os.environ and self.env. The simplest fix is to do the same here. Alternatively, teach _fetch_bare_repo, _fetch_submodules, and refresh() in the bundle to forward GIT_ASKPASS / GIT_TERMINAL_PROMPT the way GIT_SSH_COMMAND is forwarded today — but that's more invasive.
Related: the new unit tests only inspect hook.env and the script body; they don't exercise an actual clone/fetch, so CI green here isn't proof that auth works. An end-to-end-ish test that mocks Repo.clone_from / origin.fetch and asserts the env handed to the subprocess contains GIT_ASKPASS would have caught this and would guard against future regressions.
Smaller items inline. Once the propagation issue is fixed and there's a test for it, I think this is close.
Drafted-by: Claude Code (Opus 4.7); reviewed by @ephraimbuddy before posting
|
@rjgoyln — There are 4 unresolved review threads on this PR from @ephraimbuddy. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
The token was interpolated into the askpass script, so it was written to disk for the duration of the git operation — the kind of on-disk credential this branch set out to remove, and CodeQL flags it as cleartext storage. The script now reads the credential and the expected host from its environment, leaving nothing sensitive in the file. Nothing user-supplied reaches the script body any more, so the token no longer needs shell quoting, and a quoted expansion stays literal in a case pattern, which keeps an IPv6 host's brackets from being read as a character class.
The host guard recognises the host inside the prompt git passes rather than parsing a URL, so a change in git's wording would silently stop it answering. Capturing the prompts from the real binary makes a git upgrade fail here, where the cause is obvious, instead of as a bundle that cannot authenticate.
|
I also noticed that using I considered switching to explicitly passing the environment to each Git operation, but that would make this PR more invasive. Since the other askpass helpers have the same pattern, I'm leaning towards addressing this consistently in a separate PR. Would you prefer that I address it here, or would a follow-up PR be better? |
I noticed that too, but I dug a bit deeper into this today. Looking at how Airflow's DAG processor workflow operates, each process runs independently. Because of this, it might not actually affect regular Airflow users. (if I have not miss something :/ ...) The issue would likely only surface if someone wrote a custom Python script running multiple bundles concurrently in threads, something like this: from concurrent.futures import ThreadPoolExecutor
from airflow.providers.git.bundles.git import GitDagBundle
bundle_a = GitDagBundle(
name="team_a",
git_conn_id="team_a_git",
tracking_ref="main",
)
bundle_b = GitDagBundle(
name="team_b",
git_conn_id="team_b_git",
tracking_ref="main",
)
with ThreadPoolExecutor(max_workers=2) as executor:
executor.submit(bundle_a.refresh)
executor.submit(bundle_b.refresh)Since typical Airflow users follow the standard DAG processor flow, the real-world impact is probably minimal. Still, fixing this so it works reliably whether it runs in the same process or across different processes might be a nice improvement. (maybe) These are just my findings and thoughts after looking into it -- take a look when you have time in case I missed anything! That said, I think getting input from the maintainers would probably be the best next step :D |
The askpass script recognised the repository's host inside the prompt git passes, and a submodule url can put that host in its username — `https://github.com'@evil.com/x.git` prompts for `https://github.com'@evil.com`, which the guard read as its own host. Recent git percent-encodes the quote and blocks that particular shape, so the protection rested on an undocumented dependency on the git build in use. Scoping a credential helper to the repository's url moves the decision to git, which matches against the url it parsed rather than a string it formatted for a human. The helper is only invoked for the repository's own host, and receives structured fields instead of a prompt to interpret.
…onfig A connection whose host already carries ``user:password@`` kept it in ``repo_url``, so the url handed to clone — and the one the bundle now writes back to ``origin`` — still put the password in ``<bundle>/bare/config``. That is the file the report is about, reached by a route this branch had not closed. The credential is taken out of the url and served through the same helper as a connection password, so nothing downstream sees it. A url carrying only a username holds no secret and is left as written.
The GitHub App path kept its own askpass script, whose catch-all arm answered any prompt git made with the installation token — the cross-host leak the connection-token path was just fixed for, without even needing a crafted url. It also held the script open while git tried to exec it, so the token never reached git at all. The installation token is a username and a password by the time configure_hook_env sees it, so the helper the connection token already uses serves it unchanged, and the second implementation goes away.
55cb068 to
77fa0a2
Compare
``in caplog`` is a membership check on the structlog capture fixture, which the test plugin only provides from 3.1; before that it falls back to pytest's own LogCaptureFixture and the check raises TypeError, failing the 3.0.6 compat job. The test covers how git and the filesystem behave when the bare repo's config cannot be written, which does not vary by Airflow version, so the main matrix still covers it. This matches the skipif already on the view_url_template test in the same file.
git passes a `credential.helper` value to a shell, so a temporary directory containing a space split the path into two words and no token reached the remote. Marking it as a command with `!` keeps the quoting intact.
git resolves a local source to an absolute path when it clones, so a bundle configured with a relative repo url had that absolute origin replaced with the relative one, which the bare repository then resolves against its own directory and fails to fetch from on every later refresh. Only an origin carrying `user:password@` needs replacing, which is the case the rewrite exists for and never applies to a local path.
|
@rjgoyln Thanks for handling Epharim's comments + the failed tests! |
|
Failed check seems unrelated, merging |
GitHook writes the SSH_ASKPASS helper that unlocks a passphrase-protected private key with NamedTemporaryFile(delete=True) and keeps the handle open for writing while ssh runs. Linux refuses to exec a file that is still open for writing (ETXTBSY, "Text file busy"), so a clone or fetch with a passphrase-protected key fails with "cannot exec ...: Text file busy" and then falls back to prompting for the passphrase. macOS does not enforce this, which hid the bug. Write the helper through a small module-level context manager that closes the file before yielding its path and unlinks it in finally. The added test runs the helper through configure_hook_env and asserts its output; on Linux it fails without the fix with "OSError: [Errno 26] Text file busy". The token path hit the same constraint. apache#64105 fixed it there by replacing GIT_ASKPASS with a credential helper written and closed before git runs, which is why this change is now limited to SSH_ASKPASS. related: apache#73425 related: apache#64105 Co-Authored-By: krzywans <MichalJaroslaw.Krzywanski@tomtom.com> Co-Authored-By: Claude <noreply@anthropic.com>
Summary
GitHookspliced the connection's username and token into the repository URL, and git persisted that URL in the bundle'sbare/config, leaving the token in cleartext on the Dag processor's filesystem where it could be read by Dag author code. Credentials are no longer embedded in the repository URL.Change
originis re-pointed to the credential-free URL when an existing bare repo is openedMatching a credential prompt means matching a string formatted by git for human readability. A submodule URL whose username impersonates the repository host can therefore bypass that check. The credential scope is now matched against the URL parsed by git itself, so a submodule hosted on another host never invokes the helper.
The environment is set on both
os.environandhook.env, because callers currently forward onlyhook.envwhen performing the initial clone. This prevents credentials from being persisted in the bundle, but does not eliminate all exposure: while a git operation is running, the token remains available in the Dag processor's environment.Behavior change
Existing bundles have their
originrewritten to the credential-free URL the first time the bare repo is opened. Token authentication over HTTP(S) now usesGIT_CONFIG_COUNTand requires git 2.31 or newer.closes: #64099
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines