You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kada2004 (via GitHub)" <gi...@apache.org> on 2026/07/15 08:30:43 UTC
[PR] Fix passphrase quoting in decrypt_remote_file_to_string for Windows r… [airflow]
kada2004 opened a new pull request, #69908:
URL: https://github.com/apache/airflow/pull/69908
…emotes
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
the fix detects the remote OS using the existing `get_remote_os() helper, already used in tpt_utils.py and applies the correct quoting:
- Windows: double quotes with `'''` escaping
- Unix/Linus: single quotes with `'\''` escaping
closes: #69396
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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
Re: [PR] Fix passphrase quoting in decrypt_remote_file_to_string for Windows [airflow]
Posted by "kada2004 (via GitHub)" <gi...@apache.org>.
kada2004 commented on PR #69908:
URL: https://github.com/apache/airflow/pull/69908#issuecomment-5381000216
> 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 = None` under 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 a `str` in 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](https://github.com/apache/airflow/blob/main/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
> 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 = None` under 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 a `str` in 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](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst).
>
> Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Thank you for catching these issues. Restored shlex.quote on remote path and drop misleading password =None.
--
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
Re: [PR] Fix passphrase quoting in decrypt_remote_file_to_string for Windows [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #69908:
URL: https://github.com/apache/airflow/pull/69908#discussion_r3887199457
##########
providers/teradata/tests/unit/teradata/utils/test_encryption_utils.py:
##########
@@ -91,25 +79,42 @@ def test_decrypt_remote_file_to_string(self):
mock_stdin = MagicMock()
mock_stdout = MagicMock()
mock_stderr = MagicMock()
-
- # Setup mock outputs and exit code
mock_stdout.channel.recv_exit_status.return_value = 0
mock_stdout.read.return_value = b"decrypted output"
mock_stderr.read.return_value = b""
-
ssh_client.exec_command.return_value = (mock_stdin, mock_stdout, mock_stderr)
exit_status, output, err = decrypt_remote_file_to_string(
ssh_client, remote_enc_file, password, bteq_command_str
)
- quoted_password = shell_quote_single(password)
expected_cmd = (
- f"openssl enc -d -aes-256-cbc -salt -pbkdf2 -pass pass:{quoted_password} -in {remote_enc_file} | "
+ f"openssl enc -d -aes-256-cbc -salt -pbkdf2 -pass stdin -in {remote_enc_file} | "
Review Comment:
Non-blocking, but worth closing the loop on: `remote_enc_file` is `/remote/encrypted.enc`, which contains no shell-special characters, so `shlex.quote()` returns it byte-identical. This assertion therefore passes whether or not the source actually applies the quoting — which is exactly how it got dropped last time (from my snippet), silently.
Giving the fixture a path with a space makes the test fail the moment the quoting disappears:
```python
# line 75
remote_enc_file = "/remote dir/encrypted.enc"
# here
f"openssl enc -d -aes-256-cbc -salt -pbkdf2 -pass stdin -in {shlex.quote(remote_enc_file)} | "
```
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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
Re: [PR] Fix passphrase quoting in decrypt_remote_file_to_string for Windows [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #69908:
URL: https://github.com/apache/airflow/pull/69908
--
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
Re: [PR] Fix passphrase quoting in decrypt_remote_file_to_string for Windows [airflow]
Posted by "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
Re: [PR] Fix passphrase quoting in decrypt_remote_file_to_string for Windows [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #69908:
URL: https://github.com/apache/airflow/pull/69908#discussion_r3768469552
##########
providers/teradata/src/airflow/providers/teradata/utils/encryption_utils.py:
##########
@@ -54,25 +48,18 @@ def generate_encrypted_file_with_openssl(file_path: str, password: str, out_file
def decrypt_remote_file_to_string(ssh_client, remote_enc_file, password, bteq_command_str):
- # Run openssl decrypt command on remote machine
- quoted_password = shell_quote_single(password)
-
+ # Use -pass stdin to avoid shell quoting on any OS and keep the passphrase
+ # out of the remote process table where ps could expose it.
decrypt_cmd = (
- f"openssl enc -d -aes-256-cbc -salt -pbkdf2 -pass pass:{quoted_password} -in {shlex.quote(remote_enc_file)} | "
- + bteq_command_str
+ f"openssl enc -d -aes-256-cbc -salt -pbkdf2 -pass stdin -in {remote_enc_file} | " + bteq_command_str
Review Comment:
The path lost its quoting here — main had `-in {shlex.quote(remote_enc_file)}`, and the `shlex` import was removed along with it. A remote path containing a space now breaks the command. (This came from my snippet in the previous review, which dropped it — my mistake.)
```suggestion
decrypt_cmd = (
f"openssl enc -d -aes-256-cbc -salt -pbkdf2 -pass stdin -in {shlex.quote(remote_enc_file)} | "
+ bteq_command_str
)
```
…with `import shlex` restored at the top.
--
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