microsoft / adfsLogTools

Tools for parsing AD FS logs (admin events, audits, and debug logs)
MIT License
22 stars 13 forks source link

Bug(s) found in Enable-AdfsAuditing #25

Closed PaulTown closed 6 years ago

PaulTown commented 6 years ago

I found that it was turning off the Auditing settings. I had them on then ran the Enable-AdfsAuditing and they were turned off. I turned them on again and re-ran the script and they got turned off again.

I looked at the source code and found this code. It looks like to me that this is the code turning them off.
Invoke-Command -Session $Session -ScriptBlock {

    $OSVersion = gwmi win32_operatingsystem
    [int]$BuildNumber = $OSVersion.BuildNumber 

    if ( $BuildNumber -le 7601 )
    {
        Add-PsSnapin Microsoft.Adfs.Powershell -ErrorAction SilentlyContinue
    }else
    {
        Import-Module ADFS -ErrorAction SilentlyContinue
    }

    $SyncProps = Get-ADFSSyncProperties
    if ( $SyncProps.Role -ne 'SecondaryComputer' ) 
    {
        if ( $Enable )
        {
            Set-ADFSProperties -LogLevel  @( "FailureAudits", "SuccessAudits", "Warnings", "Verbose", "Errors", "Information")
            Set-ADFSProperties -AuditLevel Verbose
        }else{
            Set-ADFSProperties -LogLevel  @( "Warnings", "Errors", "Information" )
        }            
    }
} 

It seems they are using the Set-ADFSProperties -LogLevel to set these based on the parameter $Enabled. However $Enabled is a local variable which is not in scope for the script block that was executing trying to use the variable $Enabled. So if it not in scope it is not set and always is false and always just sets "Warnings", "Errors", "Information" which does not include the required "FailureAudits", "SuccessAudits".

I did some testing and was able to add the param declaration to the script block and then pass the local variable $Enabled to the script block using -ArgumentList . I tested and this seemed to do the trick to get the script block to execute as expected.

Invoke-Command -Session $Session -ScriptBlock { param($Enable)

    $OSVersion = gwmi win32_operatingsystem
    [int]$BuildNumber = $OSVersion.BuildNumber 

    if ( $BuildNumber -le 7601 )
    {
        Add-PsSnapin Microsoft.Adfs.Powershell -ErrorAction SilentlyContinue
    }else
    {
        Import-Module ADFS -ErrorAction SilentlyContinue
    }

    $SyncProps = Get-ADFSSyncProperties
    if ( $SyncProps.Role -ne 'SecondaryComputer' ) 
    {
        if ( $Enable )
        {
            Set-ADFSProperties -LogLevel  @( "FailureAudits", "SuccessAudits", "Warnings", "Verbose", "Errors", "Information")
            Set-ADFSProperties -AuditLevel Verbose
        }else{
            Set-ADFSProperties -LogLevel  @( "Warnings", "Errors", "Information" )
        }            
    }
} -ArgumentList $Enable

Also I noticed that the script has defined the functions like Enable-ADFSAuditing twice as well as others. Not sure why this is. I believe only the second definition is actually used since it is redefined the second time.

Thanks, Paul

bongiovimatthew-microsoft commented 6 years ago

@PaulTown - thanks for catching this. You are right on both things, the $Enable flag does need to be passed into that Invoke-Command call, and we also have duplicate functions that should be removed.

Thanks for catching this, I just pulled the correct code to master.