Re: [PR] Fix passphrase quoting in decrypt_remote_file_to_string for Windows [airflow]

["kada2004 (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org