Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions x/api/defaults_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import (
)

func DaemonSocketPath() string {
base := os.Getenv("ProgramData")
base := os.Getenv("LOCALAPPDATA")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH SEVERITY - Breaking change without migration path

This change moves the socket from ProgramData (typically C:\ProgramData) to LOCALAPPDATA (typically C:\Users\<username>\AppData\Local). This breaks backward compatibility:

  • Existing installations will have the socket in the old location
  • After upgrade, the daemon creates a socket in the new location
  • Clients using the old path will fail to connect
  • No automatic cleanup of old sockets
  • No fallback mechanism to check the old location

Impact: Systems will be left in an inconsistent state during upgrades.

Recommendations:

  1. Add migration logic to move/cleanup old socket
  2. Implement fallback to check old location if new one doesn't exist
  3. Document the upgrade path
  4. Consider a configuration option for backwards compatibility

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM SEVERITY - Security model change without justification

This changes the security model from system-wide (ProgramData, requires admin privileges) to user-specific (LOCALAPPDATA, user-writable).

Potential issues:

  1. Multi-user scenarios: Socket now accessible only to specific user, potentially breaking shared daemon usage
  2. Privilege escalation risks: If daemon runs as SYSTEM but uses user's LOCALAPPDATA
  3. Deployment model change: Multiple users might create conflicting daemon instances

Recommendation: Document the intended security model and deployment scenario. If this change is intentional, add comments explaining:

  • Why the move from system-wide to user-specific
  • Expected privilege level for the daemon
  • How multi-user scenarios should be handled

if base == "" {
base = `C:\ProgramData`
base = filepath.Join(os.Getenv("USERPROFILE"), "AppData", "Local")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH SEVERITY - Missing validation for USERPROFILE environment variable

If both LOCALAPPDATA and USERPROFILE environment variables are unset or empty, os.Getenv("USERPROFILE") returns an empty string. This causes filepath.Join("", "AppData", "Local") to produce a relative path "AppData\Local" instead of an absolute path.

This could cause:

  • Socket created in unexpected location (current working directory)
  • Connection failures between daemon and client
  • Security issues if processes have different working directories

The old code had a hardcoded absolute fallback (C:\ProgramData), which prevented this issue.

Recommendation: Add validation for USERPROFILE or use a hardcoded absolute fallback:

if base == "" {
    userProfile := os.Getenv("USERPROFILE")
    if userProfile == "" {
        base = `C:\Users\Default\AppData\Local`  // or another safe default
    } else {
        base = filepath.Join(userProfile, "AppData", "Local")
    }
}

}
return filepath.Join(base, "DockerSecretsEngine", "service", "daemon.sock")
}
Loading