-
Notifications
You must be signed in to change notification settings - Fork 60
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
[Bug fix] Prevent exposing API key secret in stdout logging #270
Changes from all commits
5fca7b6
56d69c2
b8a4cc6
42b1c95
0a2b795
2e1f053
50e2c8c
5283491
96b3c9c
fb31e43
c93e7b2
72995ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,46 +153,35 @@ Function Invoke-WithRetries { | |
return $returnvalue | ||
} | ||
|
||
Function Get-MaskedOutput | ||
{ | ||
function Get-MaskedOutput { | ||
[CmdletBinding()] | ||
param($arguments) | ||
|
||
$reg = [System.Text.RegularExpressions.RegEx]::new("--masterkey|--password|--license", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We felt it was prudent to improve the logic around the masking function. Note the edge case clause (the else block). |
||
[System.Text.RegularExpressions.RegexOptions]::IgnoreCase) | ||
$singleAsterixArgs = "--masterkey|--license|--licence|--trust|--password|--remove-trust|--apikey|--pw|--pfx-password|--proxyPassword"; | ||
$connectionStringArgs = "--connectionstring"; | ||
|
||
if(($arguments -match "--masterkey|--password|--license")) | ||
{ | ||
for($x=0;$x -lt $arguments.count; $x++) | ||
{ | ||
if(($arguments[$x] -match "--masterkey|--password|--license|--trust|--remove-trust|--apikey|--password|--pw|--pfx-password|--proxyPassword")) | ||
{ | ||
$arguments[$x+1] = $arguments[$x+1] -replace ".", "*" | ||
} | ||
# Scrub sensitive values | ||
for($x=0; $x -lt $arguments.count; $x++) { | ||
if($arguments[$x] -match $singleAsterixArgs) { | ||
$arguments[$x+1] = "**********" | ||
} elseif($arguments[$x] -match $connectionStringArgs) { | ||
$arguments[$x+1] = $arguments[$x+1] -replace "(password|pwd)=[^;|`"]*", "`$1=********" | ||
} | ||
$out = $arguments | ||
} | ||
elseif(($arguments -match "password|pwd")) | ||
{ | ||
$out = $arguments -replace "(password|pwd)=[^;|`"]*", "`$1=********" | ||
} | ||
else | ||
{ | ||
$out = @("************************") | ||
} | ||
return $out | ||
return $arguments | ||
} | ||
|
||
function Write-VerboseWithMaskedCommand ($cmdArgs) { | ||
$copiedarguments = @() # hack to pass a copy of the array, not a reference | ||
$copiedarguments += $cmdArgs | ||
$maskedarguments = Get-MaskedOutput $copiedarguments | ||
Write-Verbose "Executing command '$octopusServerExePath $($maskedarguments -join ' ')'" | ||
} | ||
|
||
function Invoke-OctopusServerCommand ($cmdArgs) { | ||
# todo: fix this up | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if ((($cmdArgs -match "masterkey|password|license|pwd=").Count -eq 0)) { | ||
Write-Verbose "Executing command '$octopusServerExePath $($cmdArgs -join ' ')'" | ||
} else { | ||
$copiedarguments = @() # hack to pass a copy of the array, not a reference | ||
$copiedarguments += $cmdArgs | ||
$maskedarguments = Get-MaskedOutput $copiedarguments | ||
Write-Verbose "Executing command '$octopusServerExePath $($maskedarguments -join ' ')'" | ||
} | ||
|
||
Write-VerboseWithMaskedCommand($cmdArgs); | ||
|
||
$LASTEXITCODE = 0 | ||
$output = & $octopusServerExePath $cmdArgs 2>&1 | ||
|
||
|
@@ -210,15 +199,9 @@ function Test-TentacleExecutableExists { | |
} | ||
|
||
function Invoke-TentacleCommand ($cmdArgs) { | ||
# todo: fix this up | ||
if ((($cmdArgs -match "masterkey|password|license|pwd=").Count -eq 0)) { | ||
Write-Verbose "Executing command '$tentacleExePath $($cmdArgs -join ' ')'" | ||
} else { | ||
$copiedarguments = @() # hack to pass a copy of the array, not a reference | ||
$copiedarguments += $cmdArgs | ||
$maskedarguments = Get-MaskedOutput $copiedarguments | ||
Write-Verbose "Executing command '$tentacleExePath $($maskedarguments -join ' ')'" | ||
} | ||
|
||
Write-VerboseWithMaskedCommand($cmdArgs); | ||
|
||
$LASTEXITCODE = 0 | ||
$output = & $tentacleExePath $cmdArgs 2>&1 | ||
|
||
|
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.
This is the main fix - this line is redundant and doesn't pass the arguments through the scrubber.