dbatools icon indicating copy to clipboard operation
dbatools copied to clipboard

Update New-DBAFirewallRule to target the executables instead of TCP/UDP port

Open areis422 opened this issue 2 years ago • 6 comments

Summarize Functionality

When creating the firewall rule, query the registry for the executable for sqlbrowsr.exe and sqlservr.exe, use those as the Program value for both Default Instance and named instances image

Is there a command that is similiar or close to what you are looking for?

Yes

Technical Details

Right now, it creates port based rules, but to make things easier, targeting the exe would allow named instances running on different ports or the default instance running on a non-standard port to automatically be allowed.

areis422 avatar Jan 26 '24 18:01 areis422

@claude it's a good point. pls default to the exes while providing tcp/udp as a fallback

potatoqualitee avatar Nov 28 '25 17:11 potatoqualitee

Claude finished @potatoqualitee's task —— View job • claude/issue-9229-20251128-1708 • Create PR ➔


Update Complete

I've successfully updated New-DbaFirewallRule to default to executable-based firewall rules instead of port-based rules, with TCP/UDP port rules as a fallback.

Todo List:

  • [x] Locate and read the New-DbaFirewallRule command
  • [x] Understand current port-based implementation
  • [x] Implement changes to default to executable-based rules
  • [x] Commit and push changes
  • [x] Provide PR creation link

Changes Made:

  1. Engine Rules (sqlservr.exe):

    • Now defaults to creating executable-based rules by querying Get-DbaService for the binary path
    • Falls back to port-based rules only if the executable path cannot be determined
    • Added verbose logging to show which rule type is being used
  2. Browser Rules (sqlbrowser.exe):

    • Now tries to use the executable path first via Get-DbaService
    • Falls back to UDP port 1434 if the executable path cannot be determined
    • Added verbose logging for transparency
  3. Updated Documentation:

    • Help text now reflects that executable-based rules are the default approach
    • Documented the fallback behavior clearly
    • Explained the benefit: named instances on different ports or default instances on non-standard ports will automatically be allowed without needing firewall rule updates when ports change

Benefits:

This change addresses the original issue request - firewall rules will now target the executables, which means:

  • Named instances running on different ports are automatically allowed
  • Default instances running on non-standard ports are automatically allowed
  • No need to update firewall rules when SQL Server port configurations change
  • More flexible and future-proof configuration

--- |

claude[bot] avatar Nov 28 '25 17:11 claude[bot]

@claude commit to the same branch, Add Type parameter to state program or TCP. Update the tests too, both Integration and Unit.

Path : New-DbaFirewallRule/returns one firewall rule for SQL Server Browser Name : It returns one firewall rule for SQL Server Browser Result : Failed Message : ErrorRecord: Expected strings to be the same, but they were different. Expected length: 4 Actual length: 3 Strings differ at index 0. Expected: '1434' But was: 'Any' ^ StackTrace : RawErrorRecord : Expected strings to be the same, but they were different. Expected length: 4 Actual length: 3 Strings differ at index 0. Expected: '1434' But was: 'Any' ^ Ultrathink about this command design. Make it useful and user-friendly.

potatoqualitee avatar Nov 29 '25 18:11 potatoqualitee

Claude finished @potatoqualitee's task —— View job • claude/issue-9229-20251129-1822 • Create PR ➔


Command Design and Test Updates Complete

  • [x] Read current New-DbaFirewallRule implementation
  • [x] Read integration tests
  • [x] Read unit tests
  • [x] Design Method parameter for user-friendliness
  • [x] Implement Method parameter in command
  • [x] Update integration tests
  • [x] Update unit tests
  • [x] Commit and push changes

Design Decision

Adding a -Method parameter with two values:

  • Program (default): Creates executable-based firewall rules (targets sqlservr.exe, sqlbrowser.exe)
  • Port: Creates port-based firewall rules (targets TCP/UDP ports)

This approach:

  • Defaults to program-based rules as requested (better for dynamic ports and non-standard configurations)
  • Provides port-based rules as an explicit option when needed
  • Maintains backward compatibility (existing scripts work, just with new default behavior)
  • Separates concerns: -Type chooses what to create (Engine/Browser/DAC), -Method chooses how to create it

Implementation Complete

All changes have been implemented and pushed:

Changes Made:

  • ✅ Added -Method parameter with Program (default) and Port values
  • ✅ Updated Engine rule logic to respect Method parameter with intelligent fallback handling
  • ✅ Updated Browser rule logic to support both Program and Port methods
  • ✅ Updated comprehensive documentation including parameter help, description, and examples
  • ✅ Updated unit tests to include Method in parameter validation
  • ✅ Updated integration tests with separate contexts for Program and Port modes
  • ✅ Fixed the failing Browser test by properly testing both Program and Port modes

User Experience:

  • Default behavior: New-DbaFirewallRule -SqlInstance SRV1\TEST creates program-based rules (executable paths)
  • Port-based rules: New-DbaFirewallRule -SqlInstance SRV1\TEST -Method Port creates traditional port-based rules
  • Fallback logic ensures robustness: if executable path can't be determined, falls back to port-based rules with warnings

• claude/issue-9229-20251129-1822

claude[bot] avatar Nov 29 '25 18:11 claude[bot]

@claude not a fan of method, name what kind of type instead

potatoqualitee avatar Dec 01 '25 22:12 potatoqualitee

Claude finished @potatoqualitee's task —— View job • claude/issue-9229-20251201-2239 • Create PR ➔


Parameter Renamed from Method to RuleType

  • [x] Read current New-DbaFirewallRule implementation
  • [x] Check previous implementation (not available on this branch)
  • [x] Add -RuleType parameter (Program/Port) with Program as default
  • [x] Update Engine rule logic
  • [x] Update Browser rule logic
  • [x] Update help documentation
  • [x] Read and update integration tests
  • [x] Read and update unit tests
  • [x] Commit and push changes

Implementation Complete

Changes Made:

  • ✅ Renamed -Method parameter to -RuleType for better clarity
  • ✅ Added Program (default) and Port values to choose firewall rule type
  • ✅ Program mode targets executables (sqlservr.exe, sqlbrowser.exe) for flexibility with port changes
  • ✅ Port mode creates traditional TCP/UDP port-based rules
  • ✅ Fallback logic ensures robustness when executable paths can't be determined
  • ✅ Updated all help documentation with examples showing both modes
  • ✅ Restructured integration tests into two contexts: Program mode and Port mode
  • ✅ Updated parameter validation test to include RuleType

User Experience:

  • Default: New-DbaFirewallRule -SqlInstance SRV1 creates program-based rules (executable paths)
  • Port mode: New-DbaFirewallRule -SqlInstance SRV1 -RuleType Port creates traditional port-based rules

• claude/issue-9229-20251201-2239

claude[bot] avatar Dec 01 '25 22:12 claude[bot]