-
Notifications
You must be signed in to change notification settings - Fork 10.1k
ssh-based provisioner: Re-enable support for PowerShell #37794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
12660d2 to
5ac1af6
Compare
5ac1af6 to
5be1165
Compare
5be1165 to
24987a4
Compare
| // There is no special handling of the injection attempts below | ||
| // but we include them anyway to demonstrate this | ||
| // and to avoid any regressions due to upstream changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding based on reading the upstream library and also manual testing in a Windows VM is that Windows does not let you break out of the command or at least not in the context of how we execute scp as the shell does not seem to interpret the individual arguments directly. That is why escaping of special characters on Windows does not need to be as rigorous in this particular context.
I'm happy to be corrected here. Either way the PR isn't introducing anything new here - it's only changing how the command name gets treated, all arguments are treated exactly the same as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I don't have much experience with Powershell, so I'd defer to you judgement here.
|
Regarding escaping in general, it seems important to repeat that escaping was initially introduced as a precaution anyway and it does not change anything about the threat model. We only escape the Relatedly, while If this is actually a threat the user is concerned about they could for example apply restrictions to the account that ends up SSHing in and executing the code or ensure that the variable input that can make the script vulnerable is escaped in its own context. Finally, the same kind of threat would apply to server-side provisioning, such as via cloud-config. If user allows passing of untrusted input in the context of |
eastebry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great set of test cases!
SarahFrench
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved after manual testing using the resources in the PR description (thank you!).
From my manual tests I can see that:
- I can use either of the two shells ok
- When I include strange values for the
destinationattribute such asdestination = "C:/Users/azure; echo "FOOBAR; /hello-file.txt"I get an error instead of the file uploaded to C:/Users/azure and then the echo command being executed:
scp: C:/Users/azure; echo "FOOBAR"; : No such file or directory
Context (Why)
This is a further follow-up on #30661 and also to #28626 (shipped in
v1.1.0).PowerShell provisioning did work prior to the linked patch (confirmed with
v1.0.11), though of course suffered from the command injection problem outlined in #28626Further Testing
The functionality can be manually tested (beyond just trusting the unit tests) using the following repo I prepared: https://github.com/radeksimko/tf-azure-windows-ssh
What the PR does
The PR changes escaping for the command name (arg[0]) such that it remains compatible with both cmd.exe and PowerShell, i.e. avoids over-escaping it (see below). It still escapes all arguments, and also adds tests to confirm that is the case - i.e. confirms that we did not re-introduce issues that #28626 aimed to solve.
The initial command is always
scp- so I updated the function name to also reflect this.Before
After
TODO
remote-execinline syntax (still requirespowershell ...)Target Release
1.15.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry