Clean Code is Secure Code
This is an article I have had scheduled for a while. It is a topic that should be self-evident, but in my experience, this isn’t always the case. And based on a recent story in the news, it is clear not every IT professional is paying attention. I don’t want to read about your company in the news, so I thought I should address this topic here.
I’ll be helping someone troubleshoot a PowerShell scripting problem, so naturally, I ask to look at the code. When I get the response, “Give me a moment to clean it up.” alarm bells begin going off. Almost always, this is because they want to remove organizational-related information. I’ll be blunt. If I ask you to look at your code, and you can’t show it to me, barring the use of proprietary technology, you are doing it wrong. In my experience, such code contains hard-coded data such as passwords or server names.
That was the story in the news. Uber suffered a serious security breach. A hacker penetrated Uber’s network and discovered a directory of PowerShell scripts. One of the scripts contained clear text credentials to their internal PAM product. After that, it was game over. Don’t let this happen to you.
Data is Not Code
The data you need to run your PowerShell command is separate from the code. You should not include items such as credentials, server names, or domain names in your code. This is not an exhaustive list. Your code should be so sterile that anyone could view it and not learn anything about your environment. It should also be so sterile that anyone could potentially run it in their environment.
I’m only focused on actual code. I recognize that code comments may contain organizational-specific information. But even then, I would encourage you to be as generic as possible.
Here is the code for a deliberately bad PowerShell script file.
Param($OUName)
$Domain = "Company.pri"
$DC = "dom1"
$Admin ="company\artd"
$ss = ConvertTo-SecureString -String "P@ssw(*)d" -Force -AsPlainText
$cred = [pscredential]::new($admin,$ss)
$ReportPath = "\\srv1\public\$OUName-Report.html"
$OU = Get-ADOrganizationalUnit -filter "Name -eq '$OUName'" -Server $dc -Credential $cred
#code to report on $OU
#generate html report and save to $ReportPath
If this were your code, you would need to sanitize it before sharing it with me. It most likely runs without error in the environment where it was created, and anyone could run it. This is an example of “quick and dirty” scripting that is far from secure. Sadly, many PowerShell projects begin this way.
Parameters
Another major drawback to my demonstration script is that I need to edit it when the password changes or if I want to connect to another domain controller. It is why we use parameters. If you are defining a static variable in your script, you need to ask yourself if it could be a parameter. If there is even the slightest chance someone might want to run the code with a different value, make it a parameter. You can always define a default parameter value to be used 95% of the time.
A slightly more experienced scripter might write this version of the script.
#requires -module ActiveDirectory
Param(
[string]$OUName,
[string]$DomainController,
[string]$Destination = "\\srv1\public\",
[string]$Username = "company\artd",
[string]$Password,
[string]$Domain = (Get-ADDomain).DNSRoot
)
$Domain = (Get-ADDomain).DNSRoot
$ss = ConvertTo-SecureString -String $Passthru -Force -AsPlainText
$cred = [pscredential]::new($admin, $ss)
$ReportPath = Join-Path -Path $Destination -childpath "$OUName-Report.html"
$OU = Get-ADOrganizationalUnit -Filter "Name -eq '$OUName'" -Server $DomainController -Credential $cred
#code to report on $OU
#generate html report and save to $ReportPath
Note that $ReportPath
is now statically defined based on parameter values. This is perfectly acceptable. In this version, the main body of the script doesn’t expose any sensitive information and is generic, but it is far from secure or following best scripting practices.
Credentials
The major issue is the use of credentials. If you create this file in VSCode, you’ll get a warning about the Password parameter.
If you need to use a credential object, then define an appropriate parameter.
#requires -module ActiveDirectory
Param(
[string]$OUName,
[string]$DomainController,
[string]$Destination = "\\srv1\public\",
[pscredential]$Credential,
[string]$Domain = (Get-ADDomain).DNSRoot
)
$ReportPath = Join-Path -Path $Destination -childpath "$OUName-Report.html"
$OU = Get-ADOrganizationalUnit -Filter "Name -eq '$OUName'" -Server $DomainController -Credential $credential
#code to report on $OU
#generate html report and save to $ReportPath
When the user runs the script, if they specify a username for -Credential, they will get the password prompt from Get-Credential
.
Setting Defaults
When creating PowerShell commands, you want your code to be easy to use with minimal friction. One way is to set default parameter values. You can also make a parameter mandatory to force the user to provide a value. But remember that you can’t define a default value and make the parameter mandatory. Here is a function version of my demonstration code.
#requires -module ActiveDirectory
Function Get-OUReport {
[cmdletbinding()]
Param (
[Parameter(Position = 0, Mandatory, HelpMessage = "Enter an OU name like FooBar")]
[ValidateNotNullorEmpty()]
[string]$OUName,
[alias("DC")]
[ValidateNotNullOrEmpty()]
[string]$DomainController,
[ValidateNotNullOrEmpty()]
[string]$Destination = "\\srv1\public\",
[Parameter(Mandatory)]
[alias("Admin","RunAs")]
[ValidateNotNullOrEmpty()]
[pscredential]$Credential,
[ValidateNotNullOrEmpty()]
[string]$Domain = (Get-ADDomain).DNSRoot
)
Begin {
Write-Verbose "[$((Get-Date).TimeofDay) BEGIN ] Starting $($myinvocation.mycommand)"
$ReportPath = Join-Path -Path $Destination -childpath "$OUName-Report.html"
} #begin
Process {
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Processing $OUName in $Domain "
$OU = Get-ADOrganizationalUnit -Filter "Name -eq '$OUName'" -Server $DomainController -Credential $credential
#code to report on $OU
#generate html report and save to $ReportPath
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Saving report to $ReportPath"
} #process
End {
Write-Verbose "[$((Get-Date).TimeofDay) END ] Ending $($myinvocation.mycommand)"
} #end
} #close Get-OUReport
This is getting better. I also added parameter aliases to realign the code with the original script. But I still have potentially sensitive information exposed, and I’ve removed some default values I started with.
PSDefaultParameterValues
One way to handle this is to leverage $PSDefaultParameterValues
. This is a built-in special hashtable. You can assign default values for any PowerShell command parameter.
$PSDefaultParameterValues.Add("Get-OUReport:Credential","company\artd")
$PSDefaultParameterValues.Add("Get-OUReport:Destination","\\srv1\public")
$PSDefaultParameterValues.Add("Get-OUReport:domain","company.pri")
$PSDefaultParameterValues.Add("Get-OUReport:dc","dom1")
How you populate $PSDefaultParameterValues
is entirely up to you. One typical technique is to use your PowerShell profile script. Here is a revised function designed to be used with $PSDefaultParameterValues
.
Function Get-OUReport {
[cmdletbinding()]
Param (
[Parameter(Position = 0, Mandatory, HelpMessage = "Enter an OU name like FooBar")]
[ValidateNotNullorEmpty()]
[string]$OUName,
[alias("dc")]
[ValidateNotNullOrEmpty()]
[string]$DomainController,
[ValidateNotNullOrEmpty()]
[string]$Destination,
[Parameter(Mandatory)]
[alias("Admin","RunAs")]
[ValidateNotNullOrEmpty()]
[pscredential]$Credential,
[ValidateNotNullOrEmpty()]
[string]$Domain,
[ValidateSet('html','json','xml')]
[string]$Format = "html"
)
Begin {
Write-Verbose "[$((Get-Date).TimeofDay) BEGIN ] Starting $($myinvocation.mycommand)"
$ReportPath = Join-Path -Path $Destination -childpath "$OUName-Report.$Format"
} #begin
Process {
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Processing $OUName in $Domain "
$OU = Get-ADOrganizationalUnit -Filter "Name -eq '$OUName'" -Server $DomainController -Credential $credential
#code to report on $OU
#generate html report and save to $ReportPath
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Saving report to $ReportPath"
} #process
End {
Write-Verbose "[$((Get-Date).TimeofDay) END ] Ending $($myinvocation.mycommand)"
} #end
} #close Get-OUReport
I added a Format
parameter to demonstrate non-sensitive information.
You don’t have to do any special like splatting. Run the command, and PowerShell will use the values automatically.
Because the default credential was only a user name, PowerShell prompts me for the password. The OUName parameter is mandatory, so PowerShell prompts for that as well since there is no default, and I neglected to specify it.
I could make this even easier by storing the credential.
$PSDefaultParameterValues.Add("Get-OUReport:Credential",(Get-Credential "company\artd"))
SecretManagement
Another option to securely separate data from code is to use a secrets management solution, like the SecretManagement module from Microsoft. I’m not going to cover the module in detail today. With SecretManagement, I can safely store critical information and import it into my PowerShell session when needed.
In my vault, I have the credential for the Company domain.
PS C:\> Get-Secret -Name company -Vault jhVault
UserName Password
-------- --------
company\artd System.Security.SecureString
I’ll add the domain controller as a secure string.
set-secret -Name dc -Vault jhvault -Secret dom1
I suppose I could use this as a default parameter value.
$Credential = (Get-Secret -name company)
You can define a default vault, so you don’t have to specify the vault name. But I’m going to stick with PSDefaultParameterValues
for now.
$PSDefaultParameterValues.Clear()
$PSDefaultParameterValues.Add("Get-OUReport:Credential",(Get-Secret -Name company ))
$PSDefaultParameterValues.Add("Get-OUReport:dc",(Get-Secret -Name dc -AsPlainText))
$PSDefaultParameterValues.Add("Get-OUReport:Destination","\\srv1\public")
$PSDefaultParameterValues.Add("Get-OUReport:domain","company.pri")
Configuration Data
Using PSDefaultParameterValues
may not be a simple solution if other people will be running your code. If you deploy your command via module, you could add code in the psm1 file to populate the user’s $PSDefaultParameterValues
setting.
Another approach is to use configuration data. You can store the data you need for the function in a separate file. This file will have sensitive information in plaintext, but it is separate from the code, and you don’t have to share it. But you need to protect it. Configuration data is a hashtable stored in a .psd1 file.
#oureport.psd1
@{
Domain = "company.pri"
Destination = "\\srv1\public"
DomainController = "dom1"
}
Here’s the revised function to use it.
Function Get-OUReport {
[cmdletbinding()]
Param (
[Parameter(Position = 0, Mandatory, HelpMessage = "Enter an OU name like FooBar")]
[ValidateNotNullorEmpty()]
[string]$OUName,
[ValidateScript({Test-Path $_})]
[string]$ConfigurationData = ".\oureport.psd1",
[Parameter(Mandatory)]
[alias("Admin","RunAs")]
[ValidateNotNullOrEmpty()]
[pscredential]$Credential,
[ValidateSet('html','json','xml')]
[string]$Format = "html"
)
Begin {
Write-Verbose "[$((Get-Date).TimeofDay) BEGIN ] Starting $($myinvocation.mycommand)"
Write-Verbose "[$((Get-Date).TimeofDay) BEGIN ] Importing configuration data from $(Convert-Path $ConfigurationData)"
$config = Import-PowerShellDataFile -Path $ConfigurationData
$ReportPath = Join-Path -Path $config.Destination -childpath "$OUName-Report.$Format"
} #begin
Process {
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Processing $OUName in $($Config.Domain) "
$OU = Get-ADOrganizationalUnit -Filter "Name -eq '$OUName'" -Server $config.DomainController -Credential $credential
#code to report on $OU
#generate html report and save to $ReportPath
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Saving report to $ReportPath"
} #process
End {
Write-Verbose "[$((Get-Date).TimeofDay) END ] Ending $($myinvocation.mycommand)"
} #end
} #close Get-OUReport
This approach has a few limitations, especially if you want to store a credential. The psd1 file cannot contain any code. All of the values must be static. And it can be challenging to specify the path. You could combine this with PSDefaultParameter values.
$PSDefaultParameterValues.Add("Get-OUReport:Credential",(Get-Secret -Name company))
$PSDefaultParameterValues.Add("Get-OUReport:ConfigurationData","D:\OneDrive\substack\separate-data-from-code\oureport.psd1")
One last approach would be to pass the configuration data hashtable and not merely the path to the file.
Function Get-OUReport {
[cmdletbinding()]
Param (
[Parameter(Position = 0, Mandatory, HelpMessage = "Enter an OU name like FooBar")]
[ValidateNotNullorEmpty()]
[string]$OUName,
[parameter(Mandatory,HelpMessage="Enter the configuration data hashtable")]
[ValidateNotNullOrEmpty()]
[hashtable]$ConfigurationData,
[ValidateSet('html','json','xml')]
[string]$Format = "html"
)
Begin {
Write-Verbose "[$((Get-Date).TimeofDay) BEGIN ] Starting $($myinvocation.mycommand)"
$ReportPath = Join-Path -Path $configurationdata.Destination -childpath "$OUName-Report.$Format"
} #begin
Process {
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Processing $OUName in $($ConfigurationData.Domain)"
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Authenticating to $($configurationdata.domaincontroller) as $($configurationdata.credential.username)"
$OU = Get-ADOrganizationalUnit -Filter "Name -eq '$OUName'" -Server $configurationdata.DomainController -Credential $configurationdata.credential
#code to report on $OU
#generate html report and save to $ReportPath
Write-Verbose "[$((Get-Date).TimeofDay) PROCESS] Saving report to $ReportPath"
} #process
End {
Write-Verbose "[$((Get-Date).TimeofDay) END ] Ending $($myinvocation.mycommand)"
} #end
} #close Get-OUReport
This allows me to define the configuration data separately and include the credential.
$PSDefaultParameterValues.Clear()
$config = Import-PowerShellDataFile D:\OneDrive\substack\separate-data-from-code\oureport.psd1
$config.add("Credential",(Get-Secret -Name company ))
$PSDefaultParameterValues.Add("Get-OUReport:ConfigurationData",$config)
I trust you recognize there are many ways to combine these techniques.
Summary
Separating data from code should be obvious, but it is very easy to hardcode values when under the gun. Unfortunately, I think many of these scripts hang around and are never adequately revised. Using VS Code with its integrated PSScriptAnalyzer rules will help you catch some problems. You might also look at the PSSecretScanner module I mentioned a few months ago. This module will scan your files looking for known secrets like API keys and plaintext passwords.
I have a related topic to cover next time. I’m sure you’ll have questions or comments, so please feel free to leave them on Substack or email behind@jdhitsolutions.com.