diff --git a/internal/api/config_handlers.go b/internal/api/config_handlers.go index 1bcc2b89b..14ed2a9d7 100644 --- a/internal/api/config_handlers.go +++ b/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 diff --git a/internal/api/config_handlers_setup_script_test.go b/internal/api/config_handlers_setup_script_test.go index 3f61e1472..d3d2d7665 100644 --- a/internal/api/config_handlers_setup_script_test.go +++ b/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)) diff --git a/internal/hostagent/proxmox_setup.go b/internal/hostagent/proxmox_setup.go index deb492cd5..eded8b449 100644 --- a/internal/hostagent/proxmox_setup.go +++ b/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") diff --git a/internal/hostagent/proxmox_setup_test.go b/internal/hostagent/proxmox_setup_test.go index c56e346c0..da47a5339 100644 --- a/internal/hostagent/proxmox_setup_test.go +++ b/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 + 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{} diff --git a/pkg/proxmox/client.go b/pkg/proxmox/client.go index ddb29fbf8..a0fb4be76 100644 --- a/pkg/proxmox/client.go +++ b/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