Saturday, July 22, 2017

Static Code Analysis for ARM Templates?

In a previous post I discussed the fact that ARM templates have an interesting flow when it comes to dealing with passwords.  The recommendation was that the ARM templates should be reviewed on each check-in.  This got me thinking that maybe we need a static code analysis tool for ARM templates.  The goal of this post is to discuss this in more detail.

For a long time now, code analysis tools (particularly static ones) have been in use for many managed and un-managed languages.  It is so popular that code analysis is built-in to Visual Studio 2017.  In fact, security guidance generally recommends the use of these tools.  Taking this further, many development shops use this type of analysis for not only enforcing code quality but also enforcing style guides, etc.  As ARM templates are "infrastructure-as-code" it only makes sense to try and extend these processes to those artifacts as well.

I think there are two avenues that static analysis could assist with ARM templates.  Firstly, it could identify common security concerns that may be found in these templates.  The first one that comes to mind is how passwords are handled.  As my previous post discussed, you can pass in passwords via securestring, or you can use keyvault references.  The better method is to use keyvault, and so we could write a test that ensures no parameters of type "securestring" exists.  This is just one example.

The second use for this would be around style.  One example is the use of description attributes on parameter elements.  You could issue a warning (for example) if a parameter was missing this attribute.  This could help enforcing good design guides at scale.

I know that Microsoft has been working on expanding Azure Resource Policies, and while a step in the right direction, they focus more on what can be created  in Azure (and what properties can/should/must be set) than how a template is written.  I do see a need where rules are potentially enforced in Azure (via ARP) and also, potentially, enforced in a build-step.  The build-step aspect would provide rapid feedback to the developers of policy violations (usability, security, or otherwise).

Ultimately, coming up with a full set of tests is outside the scope of this blog post.  I'm really looking for some feedback from the community on the viability of something like this.  For completeness, here is some example code that could be used to write tests.


#
# Script.ps1
#

param(
 [Parameter(Mandatory=$true)]
 [string]$filePath
)

class ARMParameter{
 [string]$name
 [string]$type
 [string]$metadata
 ARMParameter([string]$name,[string]$type,[string]$metadata){
  $this.name = $name
  $this.type = $type
  $this.metadata = $metadata
 }
 [string] ToString(){
  return "{0}-{1}" -f $this.name,$this.type
 }
}

class ARMResource{
}

Class ARMTemplate{
 hidden [PSCustomObject]$json
 hidden [ARMParameter[]]$parameters

 hidden [Void] ParseParameters(){
  Write-Verbose "Entering ParseParameters"
  $parsedParameters = @()
  $parametersRaw = $this.json.parameters
  foreach ($parameterRaw in $parametersRaw.psobject.Properties){
   Write-Verbose "parsing $parameterRaw"
   $name = $parameterRaw.Name
   $type = ""
   $metadata = ""
   foreach ($parameterRawContent in $parameterRaw.value.psobject.Properties){
    if ($parameterRawContent.Name -eq "type"){
     $type = $parameterRawContent.value
    }

    if ($parameterRawContent.Name -eq "metadata"){
     $metadata = $parameterRawContent.value.description
    }
   }
   $parsedParameters += [ARMParameter]::new($name,$type,$metadata)
  }
  $this.parameters = $parsedParameters
 }

 ARMTemplate([string]$filePath){
  Write-Verbose "Creating ARMTemplate Object"
  $this.json = ConvertFrom-Json -InputObject ((Get-Content -Path $filePath) -Join "`n")
  $this.ParseParameters()
 }

 [PsCustomObject[]] GetParametersByType([string]$type){
  return $this.parameters | ? {$_.type -eq $type}
 }

 [PsCustomObject[]] GetParameters(){
  return $this.parameters
 }
}


if (!(Test-Path $filePath)){
 Write-Error ("$filePath does not exist")
 exit
}
$armTemplate = [ARMTemplate]::new($filePath)

Write-Host "Test 1: Parameters with securestring" -ForegroundColor Cyan
$parameterWithTypeSecureString = $armTemplate.GetParametersByType("securestring")
if ($parameterWithTypeSecureString.length -eq 0){
 Write-Host "PASSED: No parameters with securestring type found" -ForegroundColor Green
} else {
 Write-Host "FAILED: Review Required: Paramters found with securestring type" -ForegroundColor Red
 Write-Host $parameterWithTypeSecureString
@"
Notes
===========
 Since these securestring parameters are passed in via the template, a reviewer should ensure that the pipeline for deployment
 handles the password in a secure manner.  An example would be a build server that retrieves the password from a secret store.
 Pipelines that give the developer access to the password for deployment purposes, or that store the credentials in plain-text
 should be avoided.
"@  
 
}


Write-Host "Test 2: All parameters should have metadata flag" -ForegroundColor Cyan
$parameters = $armTemplate.GetParameters()
$parametersWithNoMetadata = $parameters.Where({[string]::IsNullOrEmpty($_.metadata)})
if ($parametersWithNoMetadata.length -gt 0){
 Write-Host "Failed: Review Required: Metadata description missing from parameter fields" -ForegroundColor Red
 Write-Host "The following properties should have a metadata description added"
 Write-Host $parametersWithNoMetadata
@"
Notes
===========
 All parameters should contain a metadata tag with a description tag that defines the purpose of the property and where it 
 is used in the template.
"@  
} else {
 Write-Host "Passed" -ForegroundColor Green
}


So what are some thoughts around this concept?  Is is worth creating something like this where a team could create "standards" both from a usability perspective and a security perspective, and have it enforced (likely as part of a build pipeline)?  Does one of these exist in the marketplace and I am simply unaware?