PowerShell Code Cleanup



  • I wrote a PowerShell script to run as a task every hour, or as part of another task. I wrote it to work, and it works great and gets the job done. But looking at it, it seems like there's a much cleaner and future-proof way to do it.

    In addition to getting this done, I also want to take the opportunity to learn better PowerShell methods where I can as I go, which is the reason for this post.

    Imagine if I had to add like 20 or more Firewall rules to this script (which could be possible in the near future)... it will become messy, and the last line of the script may or may not work with so many "-and" statements.

    The script:

    $FWRule1 = Get-NetFireWallRule -DisplayName "!Allow Sync Fileserv 445 "
    $FWRule2 = Get-NetFireWallRule -DisplayName "!Allow Outbound UDP svchost.exe "
    $FWRule3 = Get-NetFireWallRule -DisplayName "!Allow Outbound TCP TeamViewer.exe "
    $FWRule4 = Get-NetFireWallRule -DisplayName "!Allow Outbound 5938 UDP TeamViewer.exe Local Port "
    if ($FWRule1 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Sync Fileserv 445 " -Enabled True -Direction Inbound -Profile Private -LocalPort 445 -RemotePort ANY -RemoteAddress 192.168.4.7 -Protocol TCP -Action Allow -Description "Allows file sync from Fileserv via SMB 445. (FWRule1)"}
    if ($FWRule2 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Outbound UDP svchost.exe " -Enabled True -Direction Outbound -Profile ANY -Protocol UDP -Program "C:\WINDOWS\system32\svchost.exe" -Action Allow -Description "Allows Outbound UDP svchost.exe. (FWRule2)"}
    if ($FWRule3 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Outbound TCP TeamViewer.exe " -Enabled True -Direction Outbound -Profile ANY -Protocol TCP -Program "C:\Program Files (x86)\TeamViewer\TeamViewer.exe" -Action Allow -Description "Allows Outbound TCP TeamViewer.exe. (FWRule3)"}
    if ($FWRule4 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Outbound 5938 UDP TeamViewer.exe Local Port " -Enabled True -Direction Outbound -Profile ANY -LocalPort 5938 -RemotePort ANY -Protocol UDP -Program "C:\Program Files (x86)\TeamViewer\TeamViewer.exe" -Action Allow -Description "Allows Outbound TeamViewer.exe communication via 5938 UDP. (FWRule4)"}
    Remove-Variable FWRule1
    Remove-Variable FWRule2
    Remove-Variable FWRule3
    Remove-Variable FWRule4
    Get-NetFirewallRule | Where-Object {($_.DisplayName -ne "!Allow Sync Fileserv 445 ") -and ($_.DisplayName -ne "!Allow Outbound UDP svchost.exe ") -and ($_.DisplayName -ne "!Allow Outbound TCP TeamViewer.exe ") -and ($_.DisplayName -ne "!Allow Outbound 5938 UDP TeamViewer.exe Local Port ")} | Remove-NetFirewallRule
    
    


  • You can fix the ugilness of long single lines by using the backtick (`). That makes the code more readable to some folks.

    Be consistent with your ordering as well, and I don't think you'd have a problem.



  • @Tim_G never used PS seriousltùy, but as acoder I would you suggest to make your FW rules instances of a class and put them into an array. then for-looping them

    this will fix the growing number of rules.

    about last line... don't get it. it was better to say: remove ALL firewall rules FIRST and THEN ADD only those required?

    in linux a simple ip-tables script always start wiping everything and then adding a deny/allow on basic traffic chains, like in/out/forward. then you add specific rules.

    just my 2 cents



  • @matteo-nunziati said in PowerShell Code Cleanup:

    about last line... don't get it. it was better to say: remove ALL firewall rules FIRST and THEN ADD only those required?

    I thought about that too, but then if someone/something is connected, wiping the firewall rules first will disconnect the current user or process. I don't want that to happen. So I set it to wipe everything that shouldn't be there.



  • I ended up changing the script a little bit because it seems like TeamViewer made a change, and I also wanted to allow LAN connections to connect to TeamViewer in addition from the internet... but I at least made it a little bit easier to manage the -and statements:

    # $FWRule1 = Get-NetFireWallRule -DisplayName "!Allow Sync Fileserv 445 "
    $FWRule2 = Get-NetFireWallRule -DisplayName "!Allow Outbound UDP svchost.exe "
    $FWRule3 = Get-NetFireWallRule -DisplayName "!Allow Outbound TCP TeamViewer_Service.exe "
    $FWRule4 = Get-NetFireWallRule -DisplayName "!Allow Outbound 5938 UDP TeamViewer_Service.exe Local Port "
    $FWRule5 = Get-NetFireWallRule -DisplayName "!Allow Inbound TCP LAN TeamViewer_Service.exe "
    $FWRule6 = Get-NetFireWallRule -DisplayName "!Allow Inbound UDP LAN TeamViewer_Service.exe "
    # if ($FWRule1 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Sync Fileserv 445 " -Enabled True -Direction Inbound -Profile Private -LocalPort 445 -RemotePort ANY -RemoteAddress 192.168.4.7 -Protocol TCP -Action Allow -Description "Allows file sync from Fileserv via SMB 445. (FWRule1)"}
    if ($FWRule2 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Outbound UDP svchost.exe " -Enabled True -Direction Outbound -Profile ANY -Protocol UDP -Program "C:\WINDOWS\system32\svchost.exe" -Action Allow -Description "Allows Outbound UDP svchost.exe. (FWRule2)"}
    if ($FWRule3 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Outbound TCP TeamViewer_Service.exe " -Enabled True -Direction Outbound -Profile ANY -Protocol TCP -Program "%ProgramFiles% (x86)\TeamViewer\TeamViewer_Service.exe" -Action Allow -Description "Allows Outbound TCP TeamViewer_Service.exe. (FWRule3)"}
    if ($FWRule4 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Outbound 5938 UDP TeamViewer_Service.exe Local Port " -Enabled True -Direction Outbound -Profile ANY -LocalPort 5938 -RemotePort ANY -Protocol UDP -Program "%ProgramFiles% (x86)\TeamViewer\TeamViewer_Service.exe" -Action Allow -Description "Allows Outbound TeamViewer_Service.exe communication via 5938 UDP. (FWRule4)"}
    if ($FWRule5 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Inbound TCP LAN TeamViewer_Service.exe " -Enabled True -Direction Inbound -Profile Private -Protocol TCP -Program "%ProgramFiles% (x86)\TeamViewer\TeamViewer_Service.exe" -Action Allow -Description "Allows Inbound TCP LAN TeamViewer_Service.exe. (FWRule5)"}
    if ($FWRule6 -eq $null) {New-NetFirewallRule -DisplayName "!Allow Inbound UDP LAN TeamViewer_Service.exe " -Enabled True -Direction Inbound -Profile Private -Protocol UDP -Program "%ProgramFiles% (x86)\TeamViewer\TeamViewer_Service.exe" -Action Allow -Description "Allows Inbound UDP LAN TeamViewer_Service.exe. (FWRule6)"}
    Get-NetFirewallRule | Where-Object {
        ($_.DisplayName -ne "!Allow Outbound UDP svchost.exe ") -and
        ($_.DisplayName -ne "!Allow Outbound TCP TeamViewer_Service.exe ") -and
        ($_.DisplayName -ne "!Allow Outbound 5938 UDP TeamViewer_Service.exe Local Port ") -and
        ($_.DisplayName -ne "!Allow Inbound TCP LAN TeamViewer_Service.exe ") -and
        ($_.DisplayName -ne "!Allow Inbound UDP LAN TeamViewer_Service.exe ")
    } | Remove-NetFirewallRule
    # Remove-Variable FWRule1
    Remove-Variable FWRule2
    Remove-Variable FWRule3
    Remove-Variable FWRule4
    Remove-Variable FWRule5
    Remove-Variable FWRule6
    
    


  • @matteo-nunziati said in PowerShell Code Cleanup:

    @Tim_G never used PS seriousltùy, but as acoder I would you suggest to make your FW rules instances of a class and put them into an array. then for-looping them
    this will fix the growing number of rules.

    Thanks, I will look into this.


Log in to reply
 

Looks like your connection to MangoLassi was lost, please wait while we try to reconnect.