compwiz32 / psadhealth Goto Github PK

Hi Everyone,

I'm new to powershell. But found this tool very helpful so i'd though to test it in my lab environment. whenever i ran any built in script inside the module i'm getting below error.

You cannot call a method on a null-valued expression.
At C:\Program Files\WindowsPowerShell\Modules\PSADHealth\0.0.7\PSADHealth.psm1:83 char:9

•     $msg.To.Add("$target")
  

•     ~~~~~~~~~~~~~~~~~~~~~~
  

  • CategoryInfo : InvalidOperation: (:) [], RuntimeException
  • FullyQualifiedErrorId : InvokeMethodOnNull

You cannot call a method on a null-valued expression.
At C:\Program Files\WindowsPowerShell\Modules\PSADHealth\0.0.7\PSADHealth.psm1:83 char:9

•     $msg.To.Add("$target")
  

•     ~~~~~~~~~~~~~~~~~~~~~~
  

  • CategoryInfo : InvalidOperation: (:) [], RuntimeException
  • FullyQualifiedErrorId : InvokeMethodOnNull

Send-MailMessage : A parameter cannot be found that matches parameter name 'ReplyTo'.
At C:\Program Files\WindowsPowerShell\Modules\PSADHealth\0.0.7\PSADHealth.psm1:108 char:22

• Send-MailMessage @mail
  

•                  ~~~~~
  

  • CategoryInfo : InvalidArgument: (:) [Send-MailMessage], ParameterBindingException
  • FullyQualifiedErrorId : NamedParameterNotFound,Microsoft.PowerShell.Commands.SendMailMessage

Instead of just "email", let's be more granular and specific in the config.

Thanks @compwiz32 for this one.

I have been running the monitor in my environment for a while now (with the modifications that I have in my pull request). On problem that I haven't addressed yet is that Test-SRVRecord returns an inordinately large number of false positive records for the PDC SRV record (saying it is missing). The record is not missing and I am unable to make it fail when I am testing. My thought is similar to my solution for the External DNS test with ping. If there is a failed result, just test again and it will likely come back successfully. I haven't had time to test this or try to implement it but I wanted to raise the issue in case anyone else has a better idea.

Scripts using an older method for accessing configuration data need to be updated to call Get-ADConfig and access the $configuration object for the information they require.

To make this something we can put on the Gallery, this should be a module.

New-TaskRegistration did come down in build 0.0.4 as far as I can tell.

The script named Test-ADReplication.ps1 is a flat script. It needs to be changed to a function.

Script Link

Make the sample JSON more "real".

Also converting to json doesn't work with a path. You kinda sorta need Get-Content on the front end. Whoops.

Hi Mike,

By advance, sorry for my english, it's not my mother tongue.

First : one problem identified
Write-Verbose "Last Active Directory backup occurred on $LastBackup! $Result days is less than the alert criteria of $MaxDaysSinceBackup day."
Just locatedf before the If ... else statement ==> useless. The good place is on the else statement.

Suggestion : I've always read "one function, one action".
In this function, 2 actions : Get the ADLastbackupDate (and compare to ref $MaxDaysSinceBackup)
but also Send-MailMessage (New-SlackPost is commented).
I'm thinking that the send-MailMessage should be processed by another function called on condition (i.e. param $mail=$true)

regards
Olivier

The script named Test-ExternalTimeSync.ps1 is a flat script. It needs to be changed to a function.

Script Link

tool check the event log of DC's to find computers that are attempting to authenticate but their computer object has been deleted

Currently, the cmdlet improperly overwrites config file data.

This cleans up the rest of them

If NTDS and SYSVOL is on a secondary drive, we should check for that.

The script named Test-InternalTimeSync.ps1 is a flat script. It needs to be changed to a function.

Script Link

Currently when we use the Verbose parameter, all cmdlets of the PSADHealth module are also importing of the ActiveDirectory module in verbose mode which useless.

Would it be possible to make a quiet import of the ActiveDirectory module while still having a verbose output for the real purpose of the cmdlet?

Instead of throwing an error there should be a check if the service exists first, that way it can be skipped.

Create a function that will monitor a group and report if a membership change is made. Initial thought is that this can be done with compare object by querying the group members at task start and then saving those members to a variable and then running compare-object at regular intervals to check for the changes.

This breaks module import with a lot of red text. No bueno.

The Set- cmdlet is missing from the module on load. Also, fixes need implemented to appropriately store and retrieve info from the config files.

I can't find it anywhere.

Hi Mike,
A long time ago, you wrote the module. Today, I've studied with attention, some of the internal functions.
i.e. Test-AdServices.
I've modified your code to

• Send only one mail if one or more services are stopped for one or more Domain Controller
• Adding some comments with the Write-Verbose cmdlet.

function Test-ADServices
{
    [cmdletBinding()]
    [OutputType([System.Object])]
    Param()

    begin
    {
        Import-Module ActiveDirectory
    }

    process
    {
        Write-Verbose -Message 'Restrieve Domain Controllers List from AD'
        $DClist = (Get-ADGroupMember 'Domain Controllers').name
        Write-Verbose -Message 'List of Services to monitor'
        $Collection = @('ADWS',
            'DHCPServer',
            'DNS',
            'DFS',
            'DFSR',
            'Eventlog',
            'EventSystem',
            'KDC',
            'LanManWorkstation',
            'LanManServer',
            'NetLogon',
            'NTDS',
            'RPCSS',
            'SAMSS',
            'W32Time')
        $Collection

        Write-Verbose -Message 'Retreive Info on each DC'
        forEach ($Server in $DClist)
        {
            Write-Verbose -Message "Retrieve Services Status for $Server"
            Write-Verbose -Message 'EmailBody and Subject initialization'
            $EmailBody = @'

'@
            $Subject = ''
            forEach ($Service in $Collection)
            {
                try
                {
                    $s = Get-Service -Name $Service -Computername $Server -ErrorAction Stop
                    $s
                }
                catch
                {
                    Out-Null
                }


                if ($s.status -eq 'Stopped')
                {
                    $Subject = 'Somme Windows Services for AD are stopped on Domain Controllers'
                    $EmailBody = @"
                                Service named <font color=Red><b>$($s.Displayname)</b></font> is stopped on $Server!
                                Time of Event: <font color=Red><b>"""$((Get-Date))"""</b></font><br/>
                                <br/>
"@
                    Write-Verbose -Message 'Adding EmailBody to previous (if existing)EmailBody'
                    $EmailBody += $EmailBody
                } #End If
            } #End Service Foreach
        } #End DCList Foreach
        If ($Null -ne $Subject)
        {
            <#
            By this way, There is only one single mail send if a service is stopped on one or more Domain Controller
            #>
            Write-Verbose -Message 'One or more Services are stopped. Send Mail'
            Write-Verbose 'Adding a final info into the EmailBody'
            $EmailBody += @'
                                <br/>
                                THIS EMAIL WAS AUTO-GENERATED. PLEASE DO NOT REPLY TO THIS EMAIL.
'@
            $MailParams = @{
                To         = $Configuration.MailTo
                From       = $Configuration.MailFrom
                SmtpServer = $Configuration.SmtpServer
                Subject    = $Subject
                Body       = $EmailBody
                BodyAsHtml = $true
            }
            Send-MailMessage @MailParams
        }
    } #End Process
} #End function

I've verified the code with the cmdlet Invole-ScriptAnalyzer (from PSScriptAnalyzer module), and now there are only 1 warning and 1 Information
RuleName Severity ScriptName Line Message

PSUseSingularNouns Warning Test-ADSer 1 The cmdlet 'Test-ADServices' uses a plural noun. A singular
vices.ps1 noun should be used instead.
PSUseOutputTypeCorrectly Information Test-ADSer 32 The cmdlet 'Test-ADServices' returns an object of type
vices.ps1 'System.Object[]' but this type is not declared in the
OutputType attribute.
For the second advice, it seems simple to resolve it. For the first one, this requires to change the name of the function.

Another generic potential issue : Often, I'm using an AD account from another domain. There is a Trust relationship between the 2 domain, and my account has admin rights on the domain i'm logged on. If i'm using Get-AdDomain, the cmdlet retreive the corresponding info from the domain where my account is, but not the info from the the domain im' currently logged on. The cmdlet has a parameter named -Current (possible values are LocalComputer or CurrentLoggedOnUser), it seems to me that you should add this parameter with value LocalComputer to avoid this issue, or add this parameter as a paramter for your function.

A last improvement will be to add a self help section in your function.

It's always a pleasure to read your posts on 4SysOp or other sites.

Regards
P.S. : sorry if my english is not perfect, it's not my native tongue. I do my best :-)

The script named Test-SRVRecords.ps1 is a flat script. It needs to be changed to a function.

Script Link

• received suggestions from the community to put examples of script outputs in the module wiki so that people can have a batter understanding of what that module does.

Count of 1 could produce false positives in a busy network. Should increase to something higher, say 10

• block anyone from committing straight to master
• add a required task to increase the increment the module version before a merge to master is allowed. the push to the PowerShell gallery will fail without the increase in module version number

Lots of squigglies in the codebase here.

The script named Get-ADLastBackupDate.ps1 is a flat script. It needs to be changed to a function.

Script Link

The script named Test-ExternalDNSServers.ps1 is a flat script. It needs to be changed to a function.

Script Link

The script named Test-DCsOnline.ps1 is a flat script. It needs to be changed to a function.

Script Link

the parameters need to be updated to use the parameter names from the JSON config file

There is a typo in the services checked

• Lanmanworkstation is test twice but it should be:
  LANMANWORKSTATION
  LANMANSERVER

add a parameter that can be configured to send alerts to Teams.

there is a cmdlet named set-PSADHealthConfig. We should stay consistent with name formats. Since we have a set, the get should match.

It should output it's object

Hi Mike
Perhaps a potential issue : New-ADComputer ... no error check. If user haven't rights to create computer object in AD, this failed. Try ... catch statement should be implemented.

Idem for the Remove-ADComputer : try.... catch statement.

As some other functions, this one should have only task to perform. Send-Mail could be an option (passed by param section) or outside the function.

Regards
Olivier

Hi Mike (it's my - or your- day to check your module :-) ), 3rd post
To get $DCList you use (get-adgroupmember "Domain Controllers").name
Sure, it's correct but not on all cases. I'm currently loggued on a DC on french language.
It's not the name for this group (Contrôleurs de domaine), houch !
May i suggest you use Get-AdDomainController cmdlet and filter on property Name of course ?
Like this ; (Get-ADDomainController -filter *).HostName

Same suggest as the other function : Send-mailmessage optional or passed as param

Another suggest : the if($s.status -eq "Stopped") is located on the foreach service loop. 5 services failed ==> 5 mails ! and this loop is on the foreach server loop. Houah ! 5 services failed on 50 dcs ! 225 mails sended ! Spam, spam, spam alert !

I'm thinking this sould be inproved.

Regards
Olivier

Currently we dot source functions in the psm1, but no exporting happens.

By making the output more generic it will support multiple output plugins without needing to reformat multiple times in the function code.

Hello.

One of our DC's Hostname is set with a capital Character ex. Berlin.

In the _msdcs Zone the system generates two Records one with all lowercase an one with the uppercase Name.

So your Test with No. of DC's -eq No. of SRV Records fails.

Solution could be to normalize the Hostname to lowercase and count distinct records or change to
No. of DC -le No. Srv Records.

Thanks for your tools 👍

• Remove commented out commands from script (they're old and arent relevant anymore)
• add RegisterScheduledJob syntax for remaining functions not already covered in script

The script named Get-DCDiskSpace.ps1 is a flat script. It needs to be changed to a function.

Script Link

The script named Test-ADServices.ps1 is a flat script. It needs to be changed to a function.

Script Link

The function Set-PSADHealthConfig has no working examples. This is probably important for first time users so they can see the syntax of what they need to type to get the cmdlet to run properly.

add two examples:

• one old school PS Code
• one as a splat

i would like to add a way to identify which server sent an alert. the reason for this is that some environments may have multiple methods (tool servers) that could generate alerts. Over time and with staffing changes, the possibility exists that a team may not know where the alerts are being sent from.

A simple one liner text addition to email body would suffice. See example below:

This alert was sent from SERVERNAME at TIME

taking that one step further maybe we could also identity the account used to send the alert

This alert was sent from SERVERNAME at TIME by SVC_ACCT_NAME

the task registration script in non-module scripts is excellent but not easy to find on its own. add relevant documentation to WIKI and README so that users can find that info easier. Also it allows for deeper discussion of topics and concepts than what is feasible in the notes section of a script.