[Bug fix] Prevent exposing API key secret in stdout logging #270
Conversation
.github/pull_request_template.md
Outdated
| @@ -0,0 +1,41 @@ | |||
| # Background | |||
There was a problem hiding this comment.
Can this be done as a separate PR... I think there's discussion we want to have around it first.
There was a problem hiding this comment.
Sure, we can leave that to another PR.
There was a problem hiding this comment.
Ah woops - just a heads up on this - I intended to provide a barebones template. Its updated now to that, but I will strip this out into a separate PR momentarily. 👍
paulegradie
force-pushed
the
bug-fix-expose-api-secret
branch
from
fec5730 to
76cb9d1
Compare
paulegradie
force-pushed
the
bug-fix-expose-api-secret
branch
from
29a1ec8 to
50e2c8c
Compare
| @@ -1278,6 +1278,5 @@ function Register-Tentacle { | |||
| $registerArguments += @("--tenanted-deployment-participation", $TenantedDeploymentParticipation) | |||
| } | |||
|
|
|||
| Write-Verbose "Registering with arguments: $registerArguments" | |||
There was a problem hiding this comment.
This is the main fix - this line is redundant and doesn't pass the arguments through the scrubber.
| @@ -158,41 +158,45 @@ Function Get-MaskedOutput | |||
| [CmdletBinding()] | |||
| param($arguments) | |||
|
|
|||
| $reg = [System.Text.RegularExpressions.RegEx]::new("--masterkey|--password|--license", | |||
There was a problem hiding this comment.
We felt it was prudent to improve the logic around the masking function. Note the edge case clause (the else block).
| } | ||
|
|
||
| function Invoke-OctopusServerCommand ($cmdArgs) { | ||
| # todo: fix this up |
There was a problem hiding this comment.
Rewriting the function masker helped make it clear that much of this code was redundant, so it has been removed. Args provided to a call to Write-Verbose (or other logging) should always be passed through the masking function.
README.md
Outdated
| @@ -24,7 +24,7 @@ Version 3.0 of OctopusDSC supports Octopus Deploy 4.x and above with backwards c | |||
| ## Installation | |||
|
|
|||
| ### [PowerShellGet](https://technet.microsoft.com/en-us/library/dn807169.aspx) / PowerShell 5 (recommended) | |||
| 1. Install PowerShellGet from [PowerShell Gallery](https://www.powershellgallery.com/GettingStarted) | |||
There was a problem hiding this comment.
Following the guide, I noticed some of these links no longer work. (the vagrant.io tried to open from within the github domain).
We could consider a little more detail concerning TeamCity env variables.
|
Is there any way to detect if we've been affected by this bug? ...or is it simpler to just change the API key? |
|
You'd have to use the audit log to review changes made via that api key. |
|
@paulegradie Sorry for the late question, but what are the default permissions for where this was logged? Is this a proactive "just in case" fix and the file was e.g. 600 or equiv, or was the file world-readable? Thanks! |
Background
A customer revealed to us that we are exposing an api key secret during a call to
Write-Verbosein one of our configuration scripts forcTentancleAgent. This was due to a redundant bit of logging that was not passed through our secret scrubber functionGet-MaskedOutput. Removing the logging line resolves this problem while leaving the masked logging in tact.Results
Duplicate logging no longer occurs.
How to review this PR
We current provide test coverage on the methods used to mask outputs, however we do not text stdout via assent.
This issue has been reported to the School Security for their ongoing secrets related investigations.