ephraimbuddy commented on code in PR #64105:
URL: https://github.com/apache/airflow/pull/64105#discussion_r4062472723
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -317,6 +322,47 @@ def _clone_bare_repo_if_required(self) -> None:
shutil.rmtree(self.bare_repo_path)
raise
+ def _sync_bare_repo_remote_url(self) -> None:
+ """
+ Re-point the bare repo's origin at the current repo url.
+
+ Called standalone from the ``_initialize`` fast paths that skip
cloning and never
+ reach ``_clone_bare_repo_if_required``, so a bundle that takes one
would otherwise
+ keep a credentialed origin url in ``bare/config`` forever. This is
best-effort: a
+ bundle that can still be served from disk must not fail to initialize
because its
+ bare repo is unreadable.
+ """
+ try:
+ if not self.bare_repo_path.exists():
+ return
+ bare_repo = Repo(self.bare_repo_path)
+ try:
+ self._rewrite_bare_repo_origin(bare_repo)
+ finally:
+ bare_repo.close()
+ except Exception as e:
+ # Deliberately broad: opening the repo raises anything from
``configparser`` on a
+ # truncated config to ``GitError`` on an unsafe remote url, and
the fast paths this
+ # runs ahead of never needed the bare repo at all.
+ self._log.warning(
+ "Could not rewrite the bare repository origin, a credential
may remain in "
+ "cleartext in the bundle's bare/config",
+ bare_repo_path=self.bare_repo_path,
+ exc=e,
+ )
+
+ def _rewrite_bare_repo_origin(self, bare_repo: Repo) -> None:
+ if "origin" not in bare_repo.remotes:
+ return
+ origin = bare_repo.remotes.origin
+ repo_url = str(self.repo_url)
+ # Bundles cloned before credentials moved to a credential helper
embedded ``user:token``
+ # here, so the token sits in cleartext in ``<bundle>/bare/config``
where any Dag author
+ # on the Dag processor can read it. Rewriting origin is what removes
it from those bundles.
+ if origin.url != repo_url:
+ self._log.info("Updating bare repository remote url",
bare_repo_path=self.bare_repo_path)
+ origin.set_url(repo_url)
Review Comment:
For a relative `repo_url` such as `path/to/repo`, Git stores an absolute
origin during cloning. This overwrites it with the relative path, so the
following fetch resolves it inside the bare repository and fails on both
attempts. Please preserve Git’s resolved local origin or normalize local paths
before rewriting, and add a regression test using a relative source path
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -242,73 +247,91 @@ def _ensure_github_app_token(self) -> None:
)
self.user_name, self.auth_token, self.github_app_token_exp =
self._get_github_app_token()
+ def _strip_embedded_credentials(self) -> tuple[str | None, str | None]:
+ """Take any ``user:password@`` out of the repo url and return what it
held."""
+ if not isinstance(self.repo_url, str) or not
self.repo_url.startswith(("http://";, "https://";)):
+ return None, None
+ scheme, separator, rest = self.repo_url.partition("://")
+ authority, slash, path = rest.partition("/")
+ userinfo, at_sign, host = authority.rpartition("@")
+ user, _, password = userinfo.partition(":")
+ # A bare ``user@`` holds no secret, so leave those urls exactly as the
connection wrote
+ # them; anything git clones from a stripped url would lose the
username for nothing.
+ if not at_sign or not password:
+ return None, None
+ self.repo_url = f"{scheme}{separator}{host}{slash}{path}"
+ return unquote(user) or None, unquote(password)
+
+ def _extract_credential_scope(self) -> str:
+ """Return the ``<scheme>://<host>[:port]`` git matches a credential
config against."""
+ scheme, _, rest = str(self.repo_url).partition("://")
+ host = rest.partition("/")[0].rpartition("@")[2]
+ return f"{scheme}://{host}" if host else ""
+
@contextlib.contextmanager
- def _github_app_askpass_env(self) -> Generator[None]:
- if not self.auth_token:
+ def _token_credential_env(self) -> Generator[None]:
+ """Hand the token to git through a credential helper scoped to the
repository's host."""
+ # Credential helpers only serve http(s); an SSH connection that
happens to carry a
+ # password would gain nothing from one.
+ if not self.auth_token or not
str(self.repo_url).startswith(("http://";, "https://";)):
yield
return
- token = shlex.quote(self.auth_token)
- with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=True)
as askpass_script:
- askpass_script.write(
- "#!/bin/sh\n"
- 'case "$1" in\n'
- " *Username*) echo x-access-token;;\n"
- f" *Password*) echo {token};;\n"
- f" *) echo {token};;\n"
- "esac\n"
- )
- askpass_script.flush()
- os.chmod(askpass_script.name, stat.S_IRWXU)
+ scope = self._extract_credential_scope()
+ if not scope:
+ yield
+ return
+
+ with tempfile.TemporaryDirectory() as helper_dir:
+ helper_path = os.path.join(helper_dir, "credential-helper.sh")
+ # git matches the configured scope against the url it parsed, then
hands the helper
+ # structured fields on stdin. A submodule elsewhere never reaches
this helper, and no
+ # part of the decision depends on the wording of a human-readable
prompt.
+ # Written and closed before git runs: Linux refuses to exec a file
that is still
+ # open for writing, which git surfaces as "cannot exec: Text file
busy".
+ with open(helper_path, "w") as helper_script:
+ helper_script.write(
+ r"""#!/bin/sh
+cat > /dev/null
+[ "$1" = get ] || exit 0
+printf 'username=%s\npassword=%s\n' "$AIRFLOW_GIT_USER" "$AIRFLOW_GIT_TOKEN"
+"""
+ )
+ os.chmod(helper_path, stat.S_IRWXU)
- old_askpass = os.environ.get("GIT_ASKPASS")
- old_lc_all = os.environ.get("LC_ALL")
- old_terminal_prompt = os.environ.get("GIT_TERMINAL_PROMPT")
+ # Append to any GIT_CONFIG_* the deployment already exports rather
than replacing it.
+ try:
+ index = int(os.environ.get("GIT_CONFIG_COUNT", "0"))
+ except ValueError:
+ index = 0
+ # System/global config loads before env-supplied config, so a
deployment-wide
+ # `credential.helper` would otherwise answer `get` first and our
token would never
+ # be used. git also invokes every helper on `approve`, so a
`store` helper would
+ # persist it to ~/.git-credentials. Reset the scope to empty first
(git help credentials).
+ values = {
+ "GIT_CONFIG_COUNT": str(index + 2),
+ f"GIT_CONFIG_KEY_{index}": f"credential.{scope}.helper",
+ f"GIT_CONFIG_VALUE_{index}": "",
+ f"GIT_CONFIG_KEY_{index + 1}": f"credential.{scope}.helper",
+ f"GIT_CONFIG_VALUE_{index + 1}": helper_path,
Review Comment:
Git interprets `credential.helper` through a shell, so this unquoted path
breaks token authentication when `TMPDIR` contains spaces. Please use `!`
followed by `shlex.quote(helper_path)` and cover a temporary directory
containing spaces; quoting without `!` makes Git treat it as a named helper
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
