Closed zadixon closed 5 years ago
@zadixon does the log include secrets?
By the way, there is no need to use terms like "counterproductive" ;-) The purpose of the wrapper is hiding secrets, not hiding logs. If logs includes secrets, ok let's fix that.
On a tangent note though, the deployment platform should be shielded from errors in the solution scripts, which I tried to convey in the design conversation. The wrapper is more of a band-aid and less of a final solution. For instance, the more solutions we onboard the more probable that this can occur, and we can't spend our time manually checking every log file. A better solution would be for the deployment engine not to include logs by default. That's worth a discussion with the ARM team, which might have guidance and experience with this aspect.
I've updated the PR description to better reflect the goal of preventing the logging of secrets. Sorry about the wording, I meant that by catting the output of the .log file, the setup-wrapper.sh script was effectively not doing anything different than if the script hadn't been there at all, which meant that the call itself was counterproductive with regard to the purpose of the script.
I agree that this is a short-term fix and doesn't address the underlying problem. However, I'd also argue that we shouldn't have our scripts be capable of producing logs with secrets to begin with. These secrets get logged because they are echoed to stdout due to the set -ex command that is run at the beginning of setup.sh. When the script fails, all stdout and stderr content is grabbed by ARM. I'm of the opinion that secrets shouldn't be in stdout/stderr to begin with.
At the same time, whether we can reasonably expect third party solutions to not expose secrets in this way is an important consideration, and I agree that we should see if the ARM team has any experience with generically avoiding logging secrets while still allowing us to gather useful logs.
Description and Motivation
Including this call to 'cat' in these scripts goes against the purpose of the setup-wrapper.sh script, which moves error logs potentially containing secrets to the output file in order to prevent those secrets being exposed and captured by the backend of the deployment process.
Change type
Checklist:
This change is