fix(proxmox): avoid 403 on apt update checks

internal/api/config_handlers.go

@@ -4304,19 +4304,21 @@ elif [ "$HAS_VM_GUEST_AGENT_AUDIT" = true ]; then
     EXTRA_PRIVS+=("VM.GuestAgent.Audit")
 fi
 
-if [ ${#EXTRA_PRIVS[@]} -gt 0 ]; then
-    PRIV_STRING="${EXTRA_PRIVS[*]}"
-    pveum role delete PulseMonitor 2>/dev/null || true
-    if pveum role add PulseMonitor -privs "$PRIV_STRING" 2>/dev/null; then
-        pveum aclmod / -user pulse-monitor@pam -role PulseMonitor
-        echo "  • Applied privileges: $PRIV_STRING"
-    else
-        echo "  • Failed to create PulseMonitor role with: $PRIV_STRING"
-        echo "    Assign these privileges manually if Pulse reports permission errors."
-    fi
-else
-    echo "  • No additional privileges detected. Pulse may show limited VM metrics."
-fi
+	if [ ${#EXTRA_PRIVS[@]} -gt 0 ]; then
+	    # Join as comma-separated list (pveum expects comma-separated privilege names).
+	    PRIV_STRING="$(IFS=,; echo "${EXTRA_PRIVS[*]}")"
+
+	    # Prefer modify (non-destructive) in case PulseMonitor already exists.
+	    if pveum role modify PulseMonitor -privs "$PRIV_STRING" 2>/dev/null || pveum role add PulseMonitor -privs "$PRIV_STRING" 2>/dev/null; then
+	        pveum aclmod / -user pulse-monitor@pam -role PulseMonitor
+	        echo "  • Applied privileges: $PRIV_STRING"
+	    else
+	        echo "  • Failed to configure PulseMonitor role with: $PRIV_STRING"
+	        echo "    Assign these privileges manually if Pulse reports permission errors."
+	    fi
+	else
+	    echo "  • No additional privileges detected. Pulse may show limited VM metrics."
+	fi
 
 attempt_auto_registration
 

internal/api/config_handlers_setup_script_test.go

@@ -107,6 +107,39 @@ func TestPVESetupScriptArgumentAlignment(t *testing.T) {
 	}
 }
 
+func TestPVESetupScript_ConfiguresPulseMonitorRoleSafely(t *testing.T) {
+	tempDir := t.TempDir()
+	cfg := &config.Config{
+		DataPath:   tempDir,
+		ConfigPath: tempDir,
+	}
+
+	handlers := newTestConfigHandlers(t, cfg)
+
+	req := httptest.NewRequest(http.MethodGet,
+		"/api/setup-script?type=pve&host=http://SENTINEL_HOST:8006&pulse_url=http://SENTINEL_URL:7656&auth_token=deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef", nil)
+	rr := httptest.NewRecorder()
+
+	handlers.HandleSetupScript(rr, req)
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("expected 200 OK, got %d (%s)", rr.Code, rr.Body.String())
+	}
+
+	script := rr.Body.String()
+
+	// Privileges must be comma-separated for pveum, and role updates should be non-destructive.
+	wantSnippets := []string{
+		`PRIV_STRING="$(IFS=,; echo "${EXTRA_PRIVS[*]}")"`,
+		`pveum role modify PulseMonitor -privs "$PRIV_STRING" 2>/dev/null || pveum role add PulseMonitor -privs "$PRIV_STRING" 2>/dev/null`,
+	}
+	for _, snippet := range wantSnippets {
+		if !containsString(script, snippet) {
+			t.Fatalf("expected generated script to contain:\n%s\n\nGot (first 500 chars):\n%s", snippet, truncate(script, 500))
+		}
+	}
+}
+
 func containsString(s, substr string) bool {
 	return len(s) >= len(substr) && (s == substr || len(s) > len(substr) &&
 		(findSubstring(s, substr) >= 0))

internal/hostagent/proxmox_setup.go

@@ -44,6 +44,72 @@ const (
 	proxmoxComment = "Pulse monitoring service"
 )
 
+const proxmoxMonitorRole = "PulseMonitor"
+
+func privProbeRoleName(priv string) string {
+	// Keep role name deterministic (helps tests) and valid for pveum.
+	// Replace characters that are likely to cause issues in role names.
+	safe := strings.NewReplacer(".", "_", ":", "_", "/", "_", " ", "_", ",", "_").Replace(priv)
+	return "PulseTmpPrivCheck_" + safe
+}
+
+func (p *ProxmoxSetup) probePVEPrivilege(ctx context.Context, privilege string) bool {
+	roleName := privProbeRoleName(privilege)
+
+	// If privilege doesn't exist on this PVE version, pveum will fail with a non-zero exit code.
+	if _, err := p.collector.CommandCombinedOutput(ctx, "pveum", "role", "add", roleName, "-privs", privilege); err != nil {
+		return false
+	}
+	_, _ = p.collector.CommandCombinedOutput(ctx, "pveum", "role", "delete", roleName)
+	return true
+}
+
+func (p *ProxmoxSetup) configurePVEPermissions(ctx context.Context) {
+	// Baseline: read-only access.
+	if _, err := p.collector.CommandCombinedOutput(ctx, "pveum", "aclmod", "/", "-user", proxmoxUserPVE, "-role", "PVEAuditor"); err != nil {
+		p.logger.Warn().Err(err).Msg("Failed to add PVEAuditor role (may already exist)")
+	}
+
+	// Extra privileges are optional, but enable additional features:
+	// - Sys.Audit: required for pending apt updates + some cluster/ceph visibility
+	// - VM.Monitor (PVE 8) or VM.GuestAgent.Audit (PVE 9+): guest agent data
+	// - Datastore.Audit: improved storage visibility
+	var extraPrivs []string
+
+	if p.probePVEPrivilege(ctx, "Sys.Audit") {
+		extraPrivs = append(extraPrivs, "Sys.Audit")
+	}
+
+	if p.probePVEPrivilege(ctx, "VM.Monitor") {
+		extraPrivs = append(extraPrivs, "VM.Monitor")
+	} else if p.probePVEPrivilege(ctx, "VM.GuestAgent.Audit") {
+		extraPrivs = append(extraPrivs, "VM.GuestAgent.Audit")
+	}
+
+	if p.probePVEPrivilege(ctx, "Datastore.Audit") {
+		extraPrivs = append(extraPrivs, "Datastore.Audit")
+	}
+
+	if len(extraPrivs) > 0 {
+		privString := strings.Join(extraPrivs, ",")
+
+		// Prefer modify (non-destructive) in case the role already exists.
+		if _, err := p.collector.CommandCombinedOutput(ctx, "pveum", "role", "modify", proxmoxMonitorRole, "-privs", privString); err != nil {
+			if _, addErr := p.collector.CommandCombinedOutput(ctx, "pveum", "role", "add", proxmoxMonitorRole, "-privs", privString); addErr != nil {
+				p.logger.Warn().Err(addErr).Str("privs", privString).Msg("Failed to configure PulseMonitor role")
+				return
+			}
+		}
+
+		if _, err := p.collector.CommandCombinedOutput(ctx, "pveum", "aclmod", "/", "-user", proxmoxUserPVE, "-role", proxmoxMonitorRole); err != nil {
+			p.logger.Warn().Err(err).Msg("Failed to apply PulseMonitor role")
+		}
+	}
+
+	// Add PVEDatastoreAdmin on /storage for backup visibility (issue #1139)
+	_, _ = p.collector.CommandCombinedOutput(ctx, "pveum", "aclmod", "/storage", "-user", proxmoxUserPVE, "-role", "PVEDatastoreAdmin")
+}
+
 var (
 	stateFilePath = "/var/lib/pulse-agent/proxmox-registered" // Legacy, kept for backward compat
 	stateFileDir  = "/var/lib/pulse-agent"
@@ -250,17 +316,8 @@ func (p *ProxmoxSetup) setupPVEToken(ctx context.Context, tokenName string) (str
 	// Create user (ignore error if already exists)
 	_, _ = p.collector.CommandCombinedOutput(ctx, "pveum", "user", "add", proxmoxUserPVE, "--comment", proxmoxComment)
 
-	// Add PVEAuditor role
-	if _, err := p.collector.CommandCombinedOutput(ctx, "pveum", "aclmod", "/", "-user", proxmoxUserPVE, "-role", "PVEAuditor"); err != nil {
-		p.logger.Warn().Err(err).Msg("Failed to add PVEAuditor role (may already exist)")
-	}
-
-	// Try to create PulseMonitor role with additional privileges
-	_, _ = p.collector.CommandCombinedOutput(ctx, "pveum", "role", "add", "PulseMonitor", "-privs", "Sys.Audit,VM.Monitor,Datastore.Audit")
-	_, _ = p.collector.CommandCombinedOutput(ctx, "pveum", "aclmod", "/", "-user", proxmoxUserPVE, "-role", "PulseMonitor")
-
-	// Add PVEDatastoreAdmin on /storage for backup visibility (issue #1139)
-	_, _ = p.collector.CommandCombinedOutput(ctx, "pveum", "aclmod", "/storage", "-user", proxmoxUserPVE, "-role", "PVEDatastoreAdmin")
+	// Apply baseline + optional enhanced permissions (Sys.Audit, guest agent access).
+	p.configurePVEPermissions(ctx)
 
 	// Create token with privilege separation disabled
 	output, err := p.collector.CommandCombinedOutput(ctx, "pveum", "user", "token", "add", proxmoxUserPVE, tokenName, "--privsep", "0")

internal/hostagent/proxmox_setup_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -233,6 +234,67 @@ func TestProxmoxSetup_RunForType(t *testing.T) {
 	})
 }
 
+func TestProxmoxSetup_PVEPrivilegeProbe_FallsBackToGuestAgentAudit(t *testing.T) {
+	mc := &mockCollector{}
+	var pulseMonitorPrivs string
+
+	mc.commandCombinedOutputFn = func(ctx context.Context, name string, arg ...string) (string, error) {
+		if name != "pveum" {
+			return "", nil
+		}
+
+		// Temp role privilege probe: pveum role add PulseTmpPrivCheck_* -privs <priv>
+		if len(arg) >= 5 && arg[0] == "role" && arg[1] == "add" && strings.HasPrefix(arg[2], "PulseTmpPrivCheck_") {
+			for i := 0; i < len(arg)-1; i++ {
+				if arg[i] == "-privs" {
+					priv := arg[i+1]
+					if priv == "VM.Monitor" {
+						return "", fmt.Errorf("unknown privilege")
+					}
+					return "", nil
+				}
+			}
+		}
+
+		// Capture configured privileges for PulseMonitor role.
+		if len(arg) >= 5 && arg[0] == "role" && (arg[1] == "modify" || arg[1] == "add") && arg[2] == proxmoxMonitorRole {
+			for i := 0; i < len(arg)-1; i++ {
+				if arg[i] == "-privs" {
+					pulseMonitorPrivs = arg[i+1]
+				}
+			}
+			return "", nil
+		}
+
+		// Token creation path expects a value row.
+		if len(arg) > 2 && arg[0] == "user" && arg[1] == "token" && arg[2] == "add" {
+			return "│ value │ my-token │", nil
+		}
+
+		return "", nil
+	}
+
+	p := &ProxmoxSetup{
+		logger:    zerolog.Nop(),
+		collector: mc,
+	}
+
+	tokenID, tokenValue, err := p.setupPVEToken(context.Background(), "pulse-test")
+	if err != nil {
+		t.Fatalf("setupPVEToken returned error: %v", err)
+	}
+	if tokenID == "" || tokenValue == "" {
+		t.Fatalf("expected tokenID and tokenValue to be set, got tokenID=%q tokenValue=%q", tokenID, tokenValue)
+	}
+
+	if !strings.Contains(pulseMonitorPrivs, "VM.GuestAgent.Audit") {
+		t.Fatalf("expected PulseMonitor privileges to include VM.GuestAgent.Audit, got %q", pulseMonitorPrivs)
+	}
+	if strings.Contains(pulseMonitorPrivs, "VM.Monitor") {
+		t.Fatalf("did not expect PulseMonitor privileges to include VM.Monitor when it is unavailable, got %q", pulseMonitorPrivs)
+	}
+}
+
 func TestProxmoxSetup_RunAll(t *testing.T) {
 	mc := &mockCollector{}
 

pkg/proxmox/client.go

@@ -414,13 +414,22 @@ func (c *Client) request(ctx context.Context, method, path string, data url.Valu
 
 		// Log auth issues for debugging (595 is Proxmox "no ticket" error)
 		if resp.StatusCode == 595 || resp.StatusCode == 401 || resp.StatusCode == 403 {
-			log.Warn().
+			// Some endpoints are optional and may return 403 if the token is intentionally
+			// scoped read-only. Avoid warning-level log spam for those.
+			event := log.Warn()
+			msg := "Proxmox authentication error"
+			if resp.StatusCode == 403 && strings.Contains(req.URL.Path, "/apt/update") {
+				event = log.Debug()
+				msg = "Proxmox permission error (optional endpoint)"
+			}
+
+			event.
 				Str("url", req.URL.String()).
 				Int("status", resp.StatusCode).
 				Bool("hasToken", c.config.TokenName != "").
 				Bool("hasPassword", c.config.Password != "").
 				Str("tokenName", c.config.TokenName).
-				Msg("Proxmox authentication error")
+				Msg(msg)
 		}
 
 		// Wrap with appropriate error type