-
Notifications
You must be signed in to change notification settings - Fork 116
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
Stevehose#259 checklist bugs and an enhancement #556
Conversation
* added support for 2012R2 MS/DC 2.17/2.18 * added new line on the xccdf * removed tab from processed xml.
* Initial Convert and update IIS 8.5 1.9 * remove n-2 STIGs * removed quotes * added newline to raw xccdf * updated based on PR feedback
…ance STIG - Ver 1, Rel 7 (#544) * fixed and updated SQL Instance STIGs * updated sqlserver composite and removed tabs * updated sqlserver composite. * removed tabs
* updated JRE rule V-66941.a to be a org settings * updated name of processed STIG * update changelog * added space to TS build issue.
…rocessed STIGs (#550) * updated Win2016DC failed converts and added tests * removed V-73517 from MS-1.9 as the rule no longer exist.
…6 V1R1 (#553) * added Office-System2013 STIG support. * reconverted xccdfs, corrected issues in some processed stigs. * added OfficeSystem 2016 V1R1 STIG * updated changelog.md
* Update to fix checklist bugs * Fixed bug in checklist parameter ManualCheckFile * Updated Checklist Pester tests * Updates based upon PR comments * Updated changelog.md
* Added helper function And test to verify module versions * Added tests to assert dependant module versions. * Removed commented code * Removed whitespace
* added support for 2019 MS modified hardcoded parser rule ids to support 2019 MS * added support for 2019 DC STIG; parser update to address failed AD permission rules (ActiveDirectoryAuditRule) which isn't currently implemented * Added ProcessMitigation to WindowsServer composite * regenerated all xccdfs, 6 were corrected/modified * updated changelog.md * appveyor build issue - space insert
…soft/PowerStig into stevehose#259ChecklistBugs
# Conflicts: # CHANGELOG.md # Module/STIG/Functions.Checklist.ps1 # StigData/Processed/SqlServer-2016-Instance-1.3.xml
Codecov Report
@@ Coverage Diff @@
## 4.3.0 #556 +/- ##
=======================================
Coverage 77.83% 77.83%
=======================================
Files 16 16
Lines 203 203
Branches 3 3
=======================================
Hits 158 158
Misses 42 42
Partials 3 3
Continue to review full report at Codecov.
|
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.
Reviewed 11 of 12 files at r1.
Reviewable status: 11 of 12 files reviewed, 27 unresolved discussions (waiting on @bcwilhite, @jcwalker, and @stevehose)
Module/Common/Functions.XccdfXml.ps1, line 38 at r1 (raw file):
) Begin
Begin,Process, End are keywords so they need to be lowercase per the styleguidelines.
Module/Common/Functions.XccdfXml.ps1, line 40 at r1 (raw file):
Begin { $CurrentVerbosePreference = $global:VerbosePreference
$CurrentVerbosePreference should be camel case.
Module/Common/Functions.XccdfXml.ps1, line 50 at r1 (raw file):
{ # Get the raw xccdf xml to pull additional details from the root node. [xml] $msStig = Get-Content -Path $path
$path should be pascal case.
Module/Common/Functions.XccdfXml.ps1, line 58 at r1 (raw file):
# Remove DC and Core settings from the MS xml Write-Information -MessageData "Removing Domain Controller and Core settings from Member Server STIG"
Lets put an empty line between Write-Information and foreach throughout this file.
Module/Common/Functions.XccdfXml.ps1, line 110 at r1 (raw file):
} $FilePath = "$Destination\$(Split-Path -Path $path -Leaf)"
$FilePath should be camel case.
Module/Common/Functions.XccdfXml.ps1, line 125 at r1 (raw file):
<# .SYNOPSIS
Lets put a blank line after each section in the comment based help to make it consistent with the existing code.
Module/Common/Functions.XccdfXml.ps1, line 161 at r1 (raw file):
[int] $stigProcessedCounter = 1 # Global added so that the stig rule can be referenced later
Lets end this sentence with a period to be consistent.
Module/Common/Functions.XccdfXml.ps1, line 183 at r1 (raw file):
foreach ($correction in $StigGroupListChangeLog[$stigRule.Id]) { # If the logfile contains a single * as the OldText, treat it as replacing everything with the newText value
Lets end this sentence with a period.
Module/Common/Functions.XccdfXml.ps1, line 195 at r1 (raw file):
} } $rules = [ConvertFactory]::Rule($stigRule)
Styleguidelines recommend a newline after a closing brace.
Module/Common/Functions.XccdfXml.ps1, line 228 at r1 (raw file):
} } $stigProcessedCounter ++
Styleguidelines recommend a newline after a closing brace.
Module/Common/Functions.XccdfXml.ps1, line 233 at r1 (raw file):
end { $global:stigSettings
Let put a return statement here. I know it's not needed but it will be consistent with C# and the current code.
Module/Common/Functions.XccdfXml.ps1, line 239 at r1 (raw file):
<# .SYNOPSIS Creates the file name to create from the xccdf content
Lets put a blank line after each section in the comment based help to make it consistent with the existing code.
Module/Common/Functions.XccdfXml.ps1, line 278 at r1 (raw file):
else { #$Destination = "$(Split-Path -Path (Split-Path -Path (Split-Path -Path $PSScriptRoot)))\StigData\Processed"
Remove commented code.
Module/Common/Functions.XccdfXml.ps1, line 291 at r1 (raw file):
<# .SYNOPSIS
Lets put a blank line after each section in the comment based help to make it consistent with the existing code.
Module/Common/Functions.XccdfXml.ps1, line 487 at r1 (raw file):
-split "(Release:)(.*?)(Benchmark)" )[2].trim() "$($StigDetails.Benchmark.version).$revision"
lets put a return statement here
Module/STIG/Functions.Checklist.ps1, line 101 at r1 (raw file):
$TargetNode = $DscResult.PSComputerName }
Whitepsace
Module/STIG/Functions.Checklist.ps1, line 217 at r1 (raw file):
$fileList = Get-PowerStigFileList -StigDetails $xccdfBenchmark $processedFileName = $fileList.Settings.FullName [xml]$processed = get-content -Path $processedFileName
Lets Pascal case the Get-Content cmdlet
Module/STIG/Functions.Checklist.ps1, line 219 at r1 (raw file):
[xml]$processed = get-content -Path $processedFileName $vulnerabilities = Get-VulnerabilityList -XccdfBenchmark $xccdfBenchmarkContent
Lets put an empty line above the foreach
Module/STIG/Functions.Checklist.ps1, line 281 at r1 (raw file):
elseif ($PSCmdlet.ParameterSetName -eq 'result') { $manualCheck = $manualCheckData | Where-Object {$_.VulID -eq $VID}
Lets use name parameters, (-FileterScript)
Module/STIG/Functions.Checklist.ps1, line 287 at r1 (raw file):
$findingDetails = $manualCheck.Details $comments = $manualCheck.Comments } else {
else needs to be on it's own line.
Module/STIG/Functions.Checklist.ps1, line 316 at r1 (raw file):
# Test to see if this rule is managed as a duplicate $convertedRule = $processed.SelectSingleNode("//Rule[@id='$vid']")
Lets put a blank line after $convertedRule...etc...
Module/STIG/Functions.Checklist.ps1, line 561 at r1 (raw file):
} } function Get-FindingDetailsString
All the functions need comment based help.
Module/STIG/Functions.Checklist.ps1, line 618 at r1 (raw file):
return 'MACAddress' }
Whitepsace
Module/STIG/Functions.Checklist.ps1, line 626 at r1 (raw file):
return 'IPv4Address' }
Whitepsace
Module/STIG/Functions.Checklist.ps1, line 634 at r1 (raw file):
return 'IPv6Address' }
Whitespace
Module/STIG/Convert/Functions.PowerStigXml.ps1, line 98 at r1 (raw file):
} Get-RegistryRuleExpressions -Path $Path -StigBenchmarkXml $stigBenchmarkXml
Does this function return 2 different objects?
Module/STIG/Convert/Functions.PowerStigXml.ps1, line 130 at r1 (raw file):
) Begin
Begin, Process, End should be lowercase.
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.
Reviewable status: 11 of 12 files reviewed, 27 unresolved discussions (waiting on @bcwilhite, @jcwalker, and @stevehose)
Module/Common/Functions.XccdfXml.ps1, line 38 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Begin,Process, End are keywords so they need to be lowercase per the styleguidelines.
Done.
Module/Common/Functions.XccdfXml.ps1, line 40 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
$CurrentVerbosePreference should be camel case.
Done.
Module/Common/Functions.XccdfXml.ps1, line 50 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
$path should be pascal case.
Done.
Module/Common/Functions.XccdfXml.ps1, line 58 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets put an empty line between Write-Information and foreach throughout this file.
Done.
Module/Common/Functions.XccdfXml.ps1, line 110 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
$FilePath should be camel case.
Done.
Module/Common/Functions.XccdfXml.ps1, line 125 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets put a blank line after each section in the comment based help to make it consistent with the existing code.
Done.
Module/Common/Functions.XccdfXml.ps1, line 183 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets end this sentence with a period.
Done.
Module/Common/Functions.XccdfXml.ps1, line 195 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Styleguidelines recommend a newline after a closing brace.
Done.
Module/Common/Functions.XccdfXml.ps1, line 228 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Styleguidelines recommend a newline after a closing brace.
Done.
Module/Common/Functions.XccdfXml.ps1, line 233 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Let put a return statement here. I know it's not needed but it will be consistent with C# and the current code.
Done.
Module/Common/Functions.XccdfXml.ps1, line 239 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets put a blank line after each section in the comment based help to make it consistent with the existing code.
Done.
Module/Common/Functions.XccdfXml.ps1, line 278 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Remove commented code.
Done.
Module/Common/Functions.XccdfXml.ps1, line 291 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets put a blank line after each section in the comment based help to make it consistent with the existing code.
Done.
Module/Common/Functions.XccdfXml.ps1, line 487 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
lets put a return statement here
Done.
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.
Reviewable status: 10 of 12 files reviewed, 27 unresolved discussions (waiting on @bcwilhite, @jcwalker, and @stevehose)
Module/STIG/Functions.Checklist.ps1, line 101 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Whitepsace
Done.
Module/STIG/Functions.Checklist.ps1, line 217 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets Pascal case the Get-Content cmdlet
Done.
Module/STIG/Functions.Checklist.ps1, line 219 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets put an empty line above the foreach
Done.
Module/STIG/Functions.Checklist.ps1, line 281 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets use name parameters, (-FileterScript)
Done.
Module/STIG/Functions.Checklist.ps1, line 287 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
else needs to be on it's own line.
Done.
Module/STIG/Functions.Checklist.ps1, line 316 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets put a blank line after $convertedRule...etc...
Done.
Module/STIG/Functions.Checklist.ps1, line 561 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
All the functions need comment based help.
Done.
Module/STIG/Functions.Checklist.ps1, line 618 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Whitepsace
Done.
Module/STIG/Functions.Checklist.ps1, line 626 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Whitepsace
Done.
Module/STIG/Functions.Checklist.ps1, line 634 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Whitespace
Done.
Module/STIG/Convert/Functions.PowerStigXml.ps1, line 98 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Does this function return 2 different objects?
No, it doesn't. It does load up some globals, however.
Module/STIG/Convert/Functions.PowerStigXml.ps1, line 130 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Begin, Process, End should be lowercase.
Done.
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.
Reviewable status: 10 of 12 files reviewed, 27 unresolved discussions (waiting on @bcwilhite and @jcwalker)
Module/Common/Functions.XccdfXml.ps1, line 161 at r1 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Lets end this sentence with a period to be consistent.
Done.
Module/STIG/Convert/Functions.PowerStigXml.ps1, line 98 at r1 (raw file):
Previously, stevehose (Steve Hose) wrote…
No, it doesn't. It does load up some globals, however.
OK.
Module/STIG/Functions.Checklist.ps1
Outdated
if ($ManualCheckFile) | ||
{ | ||
if (-not (Test-Path -Path $ManualCheckFile)) | ||
{ | ||
throw "$($ManualCheckFile) is not a valid path to a ManualCheckFile. Provide a full valid path" | ||
} | ||
$manualCheckData = Import-PowerShellDataFile -path $ManualCheckFile | ||
$manualCheckData = Invoke-Expression (Get-Content $manualCheckFile | Out-String) |
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.
Why is Invoke-Expression used here?
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.
Reviewable status: 9 of 12 files reviewed, 28 unresolved discussions (waiting on @bcwilhite and @jcwalker)
Module/STIG/Functions.Checklist.ps1, line 67 at r1 (raw file):
Previously, bcwilhite (Brian Wilhite) wrote…
Why is Invoke-Expression used here?
I tried valiantly to use Import-PowerShellDataFile instead but never could get it to work correctly. After several attempts I bailed and went back to what was there before.
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.
Working.
Reviewable status: 9 of 12 files reviewed, 28 unresolved discussions (waiting on @bcwilhite and @jcwalker)
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.
Reviewed 2 of 12 files at r1, 3 of 5 files at r2.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @jcwalker and @stevehose)
Module/STIG/Functions.Checklist.ps1, line 67 at r1 (raw file):
Previously, stevehose (Steve Hose) wrote…
I tried valiantly to use Import-PowerShellDataFile instead but never could get it to work correctly. After several attempts I bailed and went back to what was there before.
Instead of using Import-PowerShellDataFile, shouldn't what's in between parens work without using Invoke-Expression? i.e.:
$manualCheckData = Get-Content $manualCheckFile | Out-String
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.
Reviewable status: 9 of 10 files reviewed, 28 unresolved discussions (waiting on @bcwilhite and @jcwalker)
Module/STIG/Functions.Checklist.ps1, line 67 at r1 (raw file):
Previously, bcwilhite (Brian Wilhite) wrote…
Instead of using Import-PowerShellDataFile, shouldn't what's in between parens work without using Invoke-Expression? i.e.:
$manualCheckData = Get-Content $manualCheckFile | Out-String
I worked on it and tried a different approach in getting rid of Invoke-Session. I typecast the output and it tested out okay. So its Done.
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.
Reviewable status: 9 of 10 files reviewed, 28 unresolved discussions (waiting on @bcwilhite and @jcwalker)
Module/STIG/Functions.Checklist.ps1, line 67 at r1 (raw file):
Previously, stevehose (Steve Hose) wrote…
I worked on it and tried a different approach in getting rid of Invoke-Session. I typecast the output and it tested out okay. So its Done.
Done.
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.
Reviewed 5 of 12 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @bcwilhite and @jcwalker)
Pull Request (PR) description:
This pull request includes a pile of bug fixes for the New-StigChecklist function:
It also includes the following enhancement
This Pull Request (PR) fixes the following issues:
Task list:
This change is