Repository navigation
Fix passphrase quoting in decrypt_remote_file_to_string for Windows - #69908
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Thanks — you've identified a real problem. Unix-style '\'' quoting is simply wrong against cmd.exe, so a Windows remote would break on any passphrase containing a quote today.
But I don't think the "" escaping is sufficient, and the reason is concrete rather than hypothetical. generate_random_password draws from string.punctuation:
!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~
which includes ! " % & ( ) < > ^ | — all cmd.exe metacharacters. Doubling " doesn't neutralise them: %VAR% expands inside double quotes, & and | still separate commands, and ^ is cmd's own escape character. So a randomly generated passphrase will fairly often contain something this doesn't cover, and the failure will look like a mysterious decrypt error rather than a quoting bug. There's also the question of whether the Windows remote shell is cmd.exe at all — OpenSSH on Windows is often configured with PowerShell as the default, which needs different escaping again.
The stronger fix is to stop putting the passphrase on the command line. openssl enc supports -pass stdin, and you already hold stdin from exec_command:
decrypt_cmd = f"openssl enc -d -aes-256-cbc -salt -pbkdf2 -pass stdin -in {remote_enc_file} | " + bteq_command_str
stdin, stdout, stderr = ssh_client.exec_command(decrypt_cmd)
stdin.write(password + "\n")
stdin.flush()
stdin.channel.shutdown_write()That solves both problems at once: no shell quoting to get right on either OS, and — importantly — the passphrase no longer appears in the remote process table where any user running ps can read it. That exposure exists today with -pass pass: and is arguably the bigger issue of the two.
If -pass stdin isn't viable for some reason, the fallback would need genuine cmd.exe escaping (caret-escaping the metacharacters, and confirming the target shell), not just quote doubling.
Separately: the branch now conflicts with main and needs a rebase.
One small thing either way — removing shell_quote_single and inlining the Unix branch loses a named helper for no gain. The module already imports shlex, so shlex.quote() covers the Unix side exactly.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
148d4d3 to
8e3388c
Compare
Thanks for the review:). I have updated the implementation to use -pass stdin for both functions as suggested. Instead of quoting the passphrase on the command line, the passphrase is now passed via stdin in both generate_encrypted_file_with_openssl (using subprocess.run(... input=...)) and decrypt_remote_file_to_string (using stdin.write() over SSH). This avoids all shell quoting issues on any OS and keeps the passphrase out of the process table entirely. The tests have been updated to verify the new approach, including two security-focused tests that assert the passphrase never appears on the command line. |
83abd53 to
771362e
Compare
potiuk
left a comment
There was a problem hiding this comment.
This is exactly the right shape now — thanks for taking the bigger change rather than patching the quoting. The passphrase never reaches the remote command line, so there is nothing left to escape on either OS, and it is no longer visible to anyone running ps on the remote host. The test with s3cr3t&rm -rf ~ is a good one to have: it fails loudly if the passphrase ever creeps back onto the command string.
One thing to restore before this lands, and one small note.
shlex.quote() on the remote path was dropped (encryption_utils.py:54)
Before this PR the command built the input path as -in {shlex.quote(remote_enc_file)}; it is now interpolated raw, and the shlex import went with it. A remote path containing a space (a temp directory under a user profile with a space in it, for example) will break the command, and it puts an unquoted value back into a shell string for no benefit. This came from the snippet in my previous review, which dropped the quoting — my mistake, not something you should have caught. Re-adding import shlex and wrapping the path restores the old behaviour.
Smaller observations
encryption_utils.py:61—password = Noneunder the comment "Clear password to prevent lingering sensitive data". Rebinding the local name does not clear anything: the caller still holds the same string object, and CPython gives no way to zero astrin place. This predates your change, but it now sits inside the block you rewrote, and the comment promises a guarantee the code cannot make. Dropping both lines would leave the function honest — the real protection is the one you just added, which is keeping the value off the command line.
This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The findings
below are observations, not blockers; an Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Thank you for catching this. Restored shlex.quote on remote path and drop misleading password = None
Thank you for catching these issues. Restored shlex.quote on remote path and drop misleading password =None. |
potiuk
left a comment
There was a problem hiding this comment.
Both points from the last round are done, and the shape of the fix is right — the passphrase never touches the remote command line now, so there is nothing to escape on either OS and nothing for ps to expose on the remote host. Net deletion of 8 lines for that is a good trade.
Dropping shell_quote_single along with its two tests is correct now that nothing quotes a passphrase, and stripping the narrating comments moves the module toward the "comment sparingly" rule rather than away from it.
One non-blocking note inline about the test fixture — it is the same class of gap that let the shlex.quote() regression through in the first place.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
…pache#69908) * Fix passphrase quoting in decrypt_remote_file_to_string for Windows remotes * Restored shlex.quote on remote path and drop misleading password = None --------- Co-authored-by: kada2004 <dankalenga3@gmail,com>
…emotes
the fix detects the remote OS using the existing `get_remote_os() helper, already used in tpt_utils.py and applies the correct quoting:
'''escaping'\''escapingcloses: Quoting of passphrase won't work for windows #69396
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.