Skip to content

Conversation

Copy link

Copilot AI commented Sep 2, 2025

This PR reverts the changes introduced in PR #377 by removing the powershell package dependency and restoring the original logic for admin permission granting in pkg/fs/fs_windows.go.

Changes Made

  • Removed powershell import: Eliminated the dependency on "github.com/microsoft/moc-pkg/pkg/powershell"
  • Simplified admin group resolution: Replaced PowerShell-based SID-to-name translation with direct SID usage in icacls commands
  • Maintained security: Preserved all existing security features including command injection protection

Technical Details

The previous implementation used PowerShell to translate the well-known Administrators SID (S-1-5-32-544) to a localized group name:

// Old approach - PowerShell execution
getBuiltInAdminGroupName := `function Get-BuiltInAdminName {
    param()
    $obj = New-Object System.Security.Principal.SecurityIdentifier("S-1-5-32-544")
    $name = ($obj.Translate([System.Security.Principal.NTAccount])).Value
    "$name"
}`
builtInAdminGroupName, err := powershell.ExecutePowershell(getBuiltInAdminGroupName, `Get-BuiltInAdminName`)

The new implementation uses the SID directly with icacls, which is the standard and more reliable approach:

// New approach - Direct SID usage
adminGroupPermissions := "*S-1-5-32-544:(OI)(CI)(F)"
cmd = exec.Command("icacls", path, "/grant", adminGroupPermissions)

Benefits

  1. Removes external dependency: No longer requires the moc-pkg/powershell package
  2. Improved reliability: Direct SID usage is more stable than PowerShell translation
  3. Better performance: Eliminates PowerShell process spawning overhead
  4. Simplified maintenance: Fewer moving parts and dependencies to manage
  5. Cross-locale compatibility: SIDs work consistently across different Windows language versions

Testing

  • ✅ All existing tests pass
  • ✅ Code builds successfully without the powershell dependency
  • ✅ Command injection protection remains intact
  • ✅ No remaining powershell references in codebase

The implementation maintains the same security model while being more robust and efficient.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • w3proxy.netscape.com
    • Triggering command: /tmp/go-build20981999/b391/validations.test -test.testlogfile=/tmp/go-build20981999/b391/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

This pull request was created as a result of the following prompt from Copilot chat.

Revert the changes introduced in pull request #377 in the microsoft/moc repository. This includes removing the use of the powershell package and restoring the original logic for admin permission granting in pkg/fs/fs_windows.go.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Revert PR #377: Restore original admin permission logic in fs_windows.go Revert PR #377: Remove powershell dependency and restore direct SID-based admin permissions Sep 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants