[WIP] feat(security): add checksum validation infrastructure #106
Conversation
…oads - remove harden-runner from 9 workflows (audit-only, no security value) - add tool-checksums.json manifest for SHA256 tracking - add Get-ToolStaleness and Test-ShellDownloadSecurity functions - add Get-VerifiedDownload.ps1 helper for verified downloads - add npm-audit job to pr-validation.yml 🔒 - Generated by Copilot
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned Files
|
There was a problem hiding this comment.
Pull request overview
This PR introduces checksum validation infrastructure for tool downloads and removes the audit-only harden-runner action from workflows. While the direction is sound, several implementation issues prevent the new infrastructure from being functional.
Key concerns:
Get-ToolStalenessfunction is defined but never calledTest-ShellDownloadSecurityfunction is defined but not integrated into the scanning logicGet-VerifiedDownload.ps1has compatibility issues (tar.gz extraction not supported)
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/security/tool-checksums.json | New manifest for tracking tool versions with SHA256 checksums; schema reference is generic rather than project-specific |
| scripts/security/Test-SHAStaleness.ps1 | Adds Get-ToolStaleness function for querying GitHub Releases API, but function is never invoked; version comparison uses string equality instead of semantic versioning |
| scripts/security/Test-DependencyPinning.ps1 | Adds shell-downloads ecosystem and Test-ShellDownloadSecurity function, but not integrated into main scanning flow; regex pattern has potential false positive issues |
| scripts/lib/Get-VerifiedDownload.ps1 | New helper for verified downloads; has critical issues with tar.gz extraction support, exit handling, and ExtractPath parameter documentation |
| scripts/security/Update-ActionSHAPinning.ps1 | Removes harden-runner SHA mapping, consistent with workflow removals |
| .github/workflows/*.yml (9 files) | Cleanly removes harden-runner steps from all validation workflows |
| .github/workflows/pr-validation.yml | Adds npm-audit job following established patterns |
| .github/workflows/README.md | Updates documentation to remove harden-runner references |
| @@ -0,0 +1,14 @@ | |||
| { | |||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | |||
There was a problem hiding this comment.
The $schema field references a generic JSON Schema draft but should point to an actual schema file that defines the structure of this manifest. Consider creating a JSON Schema file (e.g., tool-checksums.schema.json) that validates the structure of this manifest, similar to how the codebase uses schemas for frontmatter validation (as seen in scripts/linting/schemas/).
| "$schema": "https://json-schema.org/draft/2020-12/schema", | |
| "$schema": "../linting/schemas/tool-checksums.schema.json", |
| $violations = @() | ||
|
|
||
| # Pattern to match curl/wget download commands | ||
| $downloadPattern = '(curl|wget)\s+.*https?://[^\s]+' |
There was a problem hiding this comment.
The download pattern '(curl|wget)\s+.*https?://[^\s]+' will match lines with curl or wget commands but may produce false positives. It doesn't account for:
- Commented-out commands (lines starting with
#) - Commands in strings or heredocs
curl/wgetappearing in URLs or variable names
Consider adding a check to exclude commented lines (e.g., if ($line -match '^\s*#')) and potentially using more precise regex patterns.
| $latestRelease = Invoke-RestMethod -Uri $uri -Headers $headers -Method Get | ||
| $latestVersion = $latestRelease.tag_name -replace '^v', '' | ||
|
|
||
| $isStale = $latestVersion -ne $tool.version |
There was a problem hiding this comment.
The version comparison $latestVersion -ne $tool.version uses simple string equality, which will incorrectly identify versions as stale. For example, "8.18.2" and "8.18.10" would be considered different using string comparison, but semantic version comparison would properly identify 8.18.10 as newer. Consider using semantic version comparison (e.g., [System.Version] casting) or a semver library to properly determine if an update is available.
| $isStale = $latestVersion -ne $tool.version | |
| try { | |
| $currentVerObj = [System.Version]$tool.version | |
| $latestVerObj = [System.Version]$latestVersion | |
| $isStale = $currentVerObj -lt $latestVerObj | |
| } | |
| catch { | |
| # Fallback to string comparison if version parsing fails | |
| $isStale = $latestVersion -ne $tool.version | |
| } |
| if ($Extract -and $ExtractPath) { | ||
| Write-Host "Extracting to: $ExtractPath" | ||
| if (-not (Test-Path $ExtractPath)) { | ||
| New-Item -ItemType Directory -Path $ExtractPath -Force | Out-Null | ||
| } | ||
| Expand-Archive -Path $tempFile -DestinationPath $ExtractPath -Force | ||
| } |
There was a problem hiding this comment.
The condition if ($Extract -and $ExtractPath) means that extraction will silently fail if -Extract is specified but -ExtractPath is not provided. Consider making ExtractPath mandatory when Extract is used, or provide a better error message, or default to extracting in the same directory as OutputPath.
| FilePatterns = @('*.sh') | ||
| SearchPaths = @('.devcontainer/scripts', 'scripts') |
There was a problem hiding this comment.
The SearchPaths property is specific to shell-downloads but is not used by any other ecosystem in $DependencyPatterns. This inconsistency suggests incomplete implementation. If this is a custom property for the ValidationFunc approach, it should be documented or the design should be revised to match the pattern used by other ecosystems (FilePatterns + VersionPatterns).
| FilePatterns = @('*.sh') | |
| SearchPaths = @('.devcontainer/scripts', 'scripts') | |
| FilePatterns = @('**/.devcontainer/scripts/*.sh', '**/scripts/*.sh') |
| Write-Error "Checksum verification failed!`nExpected: $ExpectedSHA256`nActual: $actualHash" | ||
| exit 1 | ||
| } | ||
|
|
There was a problem hiding this comment.
The script uses exit 1 which will terminate the entire PowerShell session when run as a script. This is problematic if the script is dot-sourced or used as a function. Consider using throw instead to allow proper error handling by the caller, or use return with a proper error status if this is intended to be a standalone script.
| Write-Error "Checksum verification failed!`nExpected: $ExpectedSHA256`nActual: $actualHash" | |
| exit 1 | |
| } | |
| throw "Checksum verification failed!`nExpected: $ExpectedSHA256`nActual: $actualHash" | |
| } |
| Expand-Archive -Path $tempFile -DestinationPath $ExtractPath -Force | ||
| } |
There was a problem hiding this comment.
Expand-Archive is only available for .zip files in PowerShell. This will fail for .tar.gz files (like the gitleaks download mentioned in tool-checksums.json). Consider using platform-appropriate extraction commands (e.g., tar on Linux/macOS) or checking the file extension and choosing the appropriate extraction method.
| Expand-Archive -Path $tempFile -DestinationPath $ExtractPath -Force | |
| } | |
| $fileExt = [System.IO.Path]::GetExtension($tempFile).ToLowerInvariant() | |
| $fileName = [System.IO.Path]::GetFileName($tempFile).ToLowerInvariant() | |
| if ($fileExt -eq ".zip") { | |
| Expand-Archive -Path $tempFile -DestinationPath $ExtractPath -Force | |
| } | |
| elseif ($fileName -match "\.tar\.gz$" -or $fileExt -eq ".tgz") { | |
| # Use tar for .tar.gz and .tgz | |
| Write-Host "Extracting tar.gz/tgz archive using tar" | |
| $tarCmd = "tar" | |
| $tarArgs = "xf `"$tempFile`" -C `"$ExtractPath`"" | |
| $process = Start-Process -FilePath $tarCmd -ArgumentList $tarArgs -NoNewWindow -Wait -PassThru | |
| if ($process.ExitCode -ne 0) { | |
| Write-Error "Extraction failed for tar.gz/tgz archive." | |
| exit 1 | |
| } | |
| } | |
| elseif ($fileExt -eq ".tar") { | |
| # Use tar for .tar | |
| Write-Host "Extracting tar archive using tar" | |
| $tarCmd = "tar" | |
| $tarArgs = "xf `"$tempFile`" -C `"$ExtractPath`"" | |
| $process = Start-Process -FilePath $tarCmd -ArgumentList $tarArgs -NoNewWindow -Wait -PassThru | |
| if ($process.ExitCode -ne 0) { | |
| Write-Error "Extraction failed for tar archive." | |
| exit 1 | |
| } | |
| } | |
| else { | |
| Write-Error "Unsupported archive format: $fileExt. Only .zip, .tar.gz, .tgz, and .tar are supported." | |
| exit 1 | |
| } |
| 'shell-downloads' = @{ | ||
| FilePatterns = @('*.sh') | ||
| SearchPaths = @('.devcontainer/scripts', 'scripts') | ||
| ValidationFunc = 'Test-ShellDownloadSecurity' | ||
| Description = 'Shell script downloads must include checksum verification' | ||
| } |
There was a problem hiding this comment.
The shell-downloads ecosystem is defined with a ValidationFunc property, but this function is never invoked in the main execution flow. The main script only calls Get-DependencyViolation which uses VersionPatterns to scan files. Since shell-downloads doesn't define VersionPatterns, this ecosystem will not be checked. Either integrate Test-ShellDownloadSecurity into the scanning logic or remove this unused configuration.
| 'shell-downloads' = @{ | |
| FilePatterns = @('*.sh') | |
| SearchPaths = @('.devcontainer/scripts', 'scripts') | |
| ValidationFunc = 'Test-ShellDownloadSecurity' | |
| Description = 'Shell script downloads must include checksum verification' | |
| } |
| function Get-ToolStaleness { | ||
| <# | ||
| .SYNOPSIS | ||
| Checks tool versions against their latest GitHub releases. | ||
| .DESCRIPTION | ||
| Reads the tool-checksums.json manifest and queries the GitHub Releases API | ||
| to detect when tracked tools have newer versions available. | ||
| .PARAMETER ManifestPath | ||
| Path to the tool-checksums.json manifest file. | ||
| .PARAMETER GitHubToken | ||
| GitHub API token for authenticated requests (higher rate limits). | ||
| #> | ||
| [CmdletBinding()] | ||
| param( | ||
| [Parameter()] | ||
| [string]$ManifestPath = (Join-Path $PSScriptRoot "tool-checksums.json"), | ||
|
|
||
| [Parameter()] | ||
| [string]$GitHubToken = $env:GITHUB_TOKEN | ||
| ) | ||
|
|
||
| if (-not (Test-Path $ManifestPath)) { | ||
| Write-Warning "Tool manifest not found: $ManifestPath" | ||
| return @() | ||
| } | ||
|
|
||
| $manifest = Get-Content $ManifestPath -Raw | ConvertFrom-Json | ||
| $results = @() | ||
|
|
||
| $headers = @{ | ||
| 'Accept' = 'application/vnd.github+json' | ||
| 'X-GitHub-Api-Version' = '2022-11-28' | ||
| } | ||
| if ($GitHubToken) { | ||
| $headers['Authorization'] = "Bearer $GitHubToken" | ||
| } | ||
|
|
||
| foreach ($tool in $manifest.tools) { | ||
| try { | ||
| $uri = "https://api.github.com/repos/$($tool.repo)/releases/latest" | ||
| $latestRelease = Invoke-RestMethod -Uri $uri -Headers $headers -Method Get | ||
| $latestVersion = $latestRelease.tag_name -replace '^v', '' | ||
|
|
||
| $isStale = $latestVersion -ne $tool.version | ||
|
|
||
| $results += [PSCustomObject]@{ | ||
| Tool = $tool.name | ||
| Repository = $tool.repo | ||
| CurrentVersion = $tool.version | ||
| LatestVersion = $latestVersion | ||
| IsStale = $isStale | ||
| CurrentSHA256 = $tool.sha256 | ||
| Notes = $tool.notes | ||
| } | ||
| } | ||
| catch { | ||
| Write-Warning "Failed to check $($tool.name): $_" | ||
| } | ||
| } | ||
|
|
||
| return $results | ||
| } |
There was a problem hiding this comment.
The Get-ToolStaleness function is defined but never called in the script's main execution block. This means tool staleness checking from tool-checksums.json won't actually run. Either add a call to this function in the main execution block or remove it if it's intended for future use.
|
The issues (PR comments) in this will be resolved after PR #100 merges. |
Pull Request
Description
Introduces comprehensive checksum validation infrastructure for tool downloads and removes the audit-only harden-runner action from all workflows. This establishes a foundation for verifying the integrity of external binary downloads in CI/CD pipelines and development containers.
feat(security): Added
tool-checksums.jsonmanifest for tracking tool versions with SHA256 checksumsfeat(security): Added
Get-ToolStalenessfunction toTest-SHAStaleness.ps1feat(security): Added
shell-downloadsecosystem toTest-DependencyPinning.ps1Test-ShellDownloadSecurityfunction scans shell scripts for curl/wget downloadsfeat(lib): Added
Get-VerifiedDownload.ps1helper scriptfeat(ci): Added npm-audit job to
pr-validation.ymlnpm audit --audit-level=moderateon pull requestsrefactor(workflows): Removed harden-runner from 9 workflow files
dependency-pinning-scan.yml,frontmatter-validation.yml,link-lang-check.ymlmarkdown-link-check.yml,markdown-lint.yml,ps-script-analyzer.ymlsha-staleness-check.yml,spell-check.yml,table-format.ymldocs(workflows): Updated
README.mdto remove harden-runner referenceschore(security): Removed harden-runner SHA mapping from
Update-ActionSHAPinning.ps1Related Issue(s)
Fixes #105
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
Other:
.ps1,.sh,.py)Testing
npm run lint:ps(PSScriptAnalyzer) - all scripts passnpm run lint:md- all markdown passesTest-ShellDownloadSecuritycorrectly detects downloads with and without checksum verificationChecklist
Required Checks
Required Automated Checks
The following validation commands must pass before merging:
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run lint:md-linksnpm run lint:psSecurity Considerations
Additional Notes
Test-ShellDownloadSecurityfunction validates that curl/wget downloads have checksum verification, which PR chore(devcontainer): enhance gitleaks installation with checksum verification #100'son-create.shalready implements correctly🔒 - Generated by Copilot