kada2004 commented on PR #69908:
URL: https://github.com/apache/airflow/pull/69908#issuecomment-5230457669
> 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`:
>
> ```python
> 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
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.
--
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]
