From bf846c0c0edc99f0783fda05808a82b111aaee46 Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 7 May 2026 23:07:53 +0200 Subject: [PATCH 1/2] =?UTF-8?q?refactor:=20address=20audit=20Bundle=20D=20?= =?UTF-8?q?=E2=80=94=20code-org=20quick=20wins=20(F016,=20F028,=20F049)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes audit findings F016, F028, F049. Filed F024, F032, F048 as #164 + #165 (deferred — diff shape was riskier than the rest of the bundle and the in-place CR-CLI catch saved us from doing them blind). F016 — Dedupe `ActionSigner` interface. The api package + control package both declared identical local interfaces (the SDK has the concrete `verify.ActionSigner` struct, but tests need an interface for the NoOp double). Move the canonical interface to `internal/ca/signer.go` (where the NewActionSigner constructor already lives), import from both consumers via `ca.ActionSigner`. Sites updated: api/action_handler.go, api/system_actions.go, api/service.go, control/inbox_worker.go. F028 — Delete reimplemented `indexByte` in handler/agent.go. The comment said "tiny stdlib-free helper so this file doesn't need the strings import for one call" — but the test sibling already imports strings, and the standalone reimplementation only saved one line. Use `strings.IndexByte` directly. F049 — Move `scheduleToMap` / `scheduleFromJSON` from action_handler.go to a new `internal/actionparams/schedule.go`. The two helpers are used by action_set_handler.go, definition_handler.go, internal_handler.go, and action_handler.go itself — keeping them in action_handler.go forced sibling handlers to import an action_handler-internal symbol. The actionparams package already owns adjacent serialization (params marshalling), so schedule belongs there too. CR-CLI catch on the move: scheduleFromJSON used to silently return nil on json.Unmarshal failure. The audit comment said "Empty object means no schedule configured" — but the same code path also caught corrupted JSONB or schema drift and returned nil. Now logs a structured slog.Warn when the bytes are non-empty (real corruption signal) but stays silent for the truly-empty no-schedule case. --- internal/actionparams/schedule.go | 72 ++++++++++++++++++++++++++++++ internal/api/action_set_handler.go | 7 +-- internal/api/definition_handler.go | 7 +-- internal/api/internal_handler.go | 6 +-- internal/api/service.go | 2 +- internal/api/system_actions.go | 5 ++- internal/ca/signer.go | 18 ++++++++ internal/control/inbox_worker.go | 22 ++++----- internal/handler/agent.go | 14 +----- 9 files changed, 115 insertions(+), 38 deletions(-) create mode 100644 internal/actionparams/schedule.go diff --git a/internal/actionparams/schedule.go b/internal/actionparams/schedule.go new file mode 100644 index 00000000..2439fff1 --- /dev/null +++ b/internal/actionparams/schedule.go @@ -0,0 +1,72 @@ +package actionparams + +import ( + "encoding/json" + "log/slog" + + pm "github.com/manchtools/power-manage/sdk/gen/go/pm/v1" +) + +// ScheduleToMap converts an ActionSchedule proto into the map shape +// used for JSONB event payloads. Empty-valued fields are omitted so +// the projector's ON CONFLICT / COALESCE behaviour treats them as +// "preserve" rather than "set to zero". +// +// Used by the action / action_set / definition handlers when emitting +// XxxCreated / XxxUpdated events. Centralised here so the serialised +// shape stays identical across handlers — drift would surface as +// silently-different schedule semantics depending on which entity +// the schedule was attached to. +func ScheduleToMap(s *pm.ActionSchedule) map[string]any { + m := map[string]any{} + if s == nil { + return m + } + if s.Cron != "" { + m["cron"] = s.Cron + } + if s.IntervalHours > 0 { + m["interval_hours"] = s.IntervalHours + } + if s.RunOnAssign { + m["run_on_assign"] = true + } + if s.SkipIfUnchanged { + m["skip_if_unchanged"] = true + } + return m +} + +// ScheduleFromJSON deserialises a schedule JSONB column back into an +// ActionSchedule proto. Returns nil for malformed JSON or for the +// empty-object case (so callers can leave the proto field unset +// instead of carrying an all-zeros placeholder). +func ScheduleFromJSON(data []byte) *pm.ActionSchedule { + var raw struct { + Cron string `json:"cron"` + IntervalHours int32 `json:"interval_hours"` + RunOnAssign bool `json:"run_on_assign"` + SkipIfUnchanged bool `json:"skip_if_unchanged"` + } + if err := json.Unmarshal(data, &raw); err != nil { + // Truly-empty input is a normal "no schedule configured" + // signal — stay silent. Non-empty bytes that fail to parse + // indicate event-store corruption or a schema drift the + // projector should surface, so emit a structured log so + // operators can grep for it. + if len(data) > 0 { + slog.Warn("actionparams: schedule JSON malformed; treating as no schedule", + "bytes", len(data), "error", err) + } + return nil + } + if raw.Cron == "" && raw.IntervalHours == 0 && !raw.RunOnAssign && !raw.SkipIfUnchanged { + return nil + } + return &pm.ActionSchedule{ + Cron: raw.Cron, + IntervalHours: raw.IntervalHours, + RunOnAssign: raw.RunOnAssign, + SkipIfUnchanged: raw.SkipIfUnchanged, + } +} diff --git a/internal/api/action_set_handler.go b/internal/api/action_set_handler.go index db4199e8..6e11b5a4 100644 --- a/internal/api/action_set_handler.go +++ b/internal/api/action_set_handler.go @@ -9,6 +9,7 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" pm "github.com/manchtools/power-manage/sdk/gen/go/pm/v1" + "github.com/manchtools/power-manage/server/internal/actionparams" "github.com/manchtools/power-manage/server/internal/store" db "github.com/manchtools/power-manage/server/internal/store/generated" ) @@ -50,7 +51,7 @@ func (h *ActionSetHandler) CreateActionSet(ctx context.Context, req *connect.Req // non-empty so a zero-value request can't bypass the projection's // default-schedule fallback by emitting an empty `{}` blob. if req.Msg.Schedule != nil { - if schedule := scheduleToMap(req.Msg.Schedule); len(schedule) > 0 { + if schedule := actionparams.ScheduleToMap(req.Msg.Schedule); len(schedule) > 0 { data["schedule"] = schedule } } @@ -191,7 +192,7 @@ func (h *ActionSetHandler) UpdateActionSetSchedule(ctx context.Context, req *con data := map[string]any{} if req.Msg.Schedule != nil { - if schedule := scheduleToMap(req.Msg.Schedule); len(schedule) > 0 { + if schedule := actionparams.ScheduleToMap(req.Msg.Schedule); len(schedule) > 0 { data["schedule"] = schedule } } @@ -415,7 +416,7 @@ func (h *ActionSetHandler) actionSetToProto(s db.ActionSetsProjection) *pm.Actio Description: s.Description, MemberCount: s.MemberCount, CreatedBy: s.CreatedBy, - Schedule: scheduleFromJSON(s.Schedule), + Schedule: actionparams.ScheduleFromJSON(s.Schedule), } if s.CreatedAt != nil { diff --git a/internal/api/definition_handler.go b/internal/api/definition_handler.go index 80317e02..f59090b2 100644 --- a/internal/api/definition_handler.go +++ b/internal/api/definition_handler.go @@ -9,6 +9,7 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" pm "github.com/manchtools/power-manage/sdk/gen/go/pm/v1" + "github.com/manchtools/power-manage/server/internal/actionparams" "github.com/manchtools/power-manage/server/internal/store" db "github.com/manchtools/power-manage/server/internal/store/generated" ) @@ -49,7 +50,7 @@ func (h *DefinitionHandler) CreateDefinition(ctx context.Context, req *connect.R // payload defensively — see action_set_handler.go.CreateActionSet // for the rationale (default-schedule fallback in the projector). if req.Msg.Schedule != nil { - if schedule := scheduleToMap(req.Msg.Schedule); len(schedule) > 0 { + if schedule := actionparams.ScheduleToMap(req.Msg.Schedule); len(schedule) > 0 { data["schedule"] = schedule } } @@ -189,7 +190,7 @@ func (h *DefinitionHandler) UpdateDefinitionSchedule(ctx context.Context, req *c data := map[string]any{} if req.Msg.Schedule != nil { - if schedule := scheduleToMap(req.Msg.Schedule); len(schedule) > 0 { + if schedule := actionparams.ScheduleToMap(req.Msg.Schedule); len(schedule) > 0 { data["schedule"] = schedule } } @@ -413,7 +414,7 @@ func (h *DefinitionHandler) definitionToProto(d db.DefinitionsProjection) *pm.De Description: d.Description, MemberCount: d.MemberCount, CreatedBy: d.CreatedBy, - Schedule: scheduleFromJSON(d.Schedule), + Schedule: actionparams.ScheduleFromJSON(d.Schedule), } if d.CreatedAt != nil { diff --git a/internal/api/internal_handler.go b/internal/api/internal_handler.go index 96abe67b..8ef47c42 100644 --- a/internal/api/internal_handler.go +++ b/internal/api/internal_handler.go @@ -177,7 +177,7 @@ func (h *InternalHandler) ProxySyncActions(ctx context.Context, req *connect.Req } groups = append(groups, &pm.ActionGroup{ SourceLabel: g.SourceLabel, - Schedule: scheduleFromJSON(g.Schedule), + Schedule: actionparams.ScheduleFromJSON(g.Schedule), Actions: groupActions, }) } @@ -232,7 +232,7 @@ func dbActionToWireAction(a db.ActionsProjection) *pm.Action { actionparams.PopulateAction(action, a.ActionType, a.Params) } if len(a.Schedule) > 0 { - action.Schedule = scheduleFromJSON(a.Schedule) + action.Schedule = actionparams.ScheduleFromJSON(a.Schedule) } return action } @@ -518,7 +518,7 @@ func dbResolvedActionToWireAction(a db.ListResolvedActionsForDeviceRow) *pm.Acti } if len(a.Schedule) > 0 { - action.Schedule = scheduleFromJSON(a.Schedule) + action.Schedule = actionparams.ScheduleFromJSON(a.Schedule) } return action diff --git a/internal/api/service.go b/internal/api/service.go index 22d94eb2..a1d281c6 100644 --- a/internal/api/service.go +++ b/internal/api/service.go @@ -55,7 +55,7 @@ type ControlServiceConfig struct { } // NewControlService creates a new control service. -func NewControlService(st *store.Store, jwtManager *auth.JWTManager, signer ActionSigner, certAuth *ca.CA, gatewayURL string, logger *slog.Logger, enc *crypto.Encryptor, cfg ControlServiceConfig) *ControlService { +func NewControlService(st *store.Store, jwtManager *auth.JWTManager, signer ca.ActionSigner, certAuth *ca.CA, gatewayURL string, logger *slog.Logger, enc *crypto.Encryptor, cfg ControlServiceConfig) *ControlService { actionHandler := NewActionHandler(st, logger.With("component", "action_handler"), signer) systemActions := NewSystemActionManager(st, signer, logger) settingsHandler := NewSettingsHandler(st, logger, systemActions) diff --git a/internal/api/system_actions.go b/internal/api/system_actions.go index 70169f95..f4647549 100644 --- a/internal/api/system_actions.go +++ b/internal/api/system_actions.go @@ -10,6 +10,7 @@ import ( pm "github.com/manchtools/power-manage/sdk/gen/go/pm/v1" "github.com/manchtools/power-manage/server/internal/actionparams" + "github.com/manchtools/power-manage/server/internal/ca" "github.com/manchtools/power-manage/server/internal/store" db "github.com/manchtools/power-manage/server/internal/store/generated" ) @@ -35,12 +36,12 @@ import ( // land on every device for permission holders" property. type SystemActionManager struct { store *store.Store - signer ActionSigner + signer ca.ActionSigner logger *slog.Logger } // NewSystemActionManager creates a new system action manager. -func NewSystemActionManager(st *store.Store, signer ActionSigner, logger *slog.Logger) *SystemActionManager { +func NewSystemActionManager(st *store.Store, signer ca.ActionSigner, logger *slog.Logger) *SystemActionManager { return &SystemActionManager{ store: st, signer: signer, diff --git a/internal/ca/signer.go b/internal/ca/signer.go index 7ac1ecff..375dab27 100644 --- a/internal/ca/signer.go +++ b/internal/ca/signer.go @@ -2,6 +2,24 @@ package ca import "github.com/manchtools/power-manage/sdk/go/verify" +// ActionSigner is the minimal contract dispatch / inbox handlers need +// to sign action payloads so agents can verify authenticity. Defined +// as an interface (vs the SDK's concrete *verify.ActionSigner) so +// tests can substitute a NoOp implementation without dragging in the +// real CA. +// +// A nil ActionSigner disables signing globally — this is intentional +// for development bootstraps that haven't yet provisioned a CA, and +// is checked at every dispatch site so production handlers fail +// loudly rather than silently produce unsigned actions. +// +// Previously declared independently in internal/api/action_handler.go +// and internal/control/inbox_worker.go; consolidated here so the two +// can't drift. +type ActionSigner interface { + Sign(actionID string, actionType int32, paramsJSON []byte) ([]byte, error) +} + // NewActionSigner creates a new action signer using the CA's private key. func NewActionSigner(ca *CA) *verify.ActionSigner { return verify.NewActionSigner(ca.Signer()) diff --git a/internal/control/inbox_worker.go b/internal/control/inbox_worker.go index ecedd43f..433262df 100644 --- a/internal/control/inbox_worker.go +++ b/internal/control/inbox_worker.go @@ -15,34 +15,28 @@ import ( "google.golang.org/protobuf/encoding/protojson" pm "github.com/manchtools/power-manage/sdk/gen/go/pm/v1" + "github.com/manchtools/power-manage/server/internal/ca" "github.com/manchtools/power-manage/server/internal/store" db "github.com/manchtools/power-manage/server/internal/store/generated" "github.com/manchtools/power-manage/server/internal/taskqueue" ) -// ActionSigner signs action payloads so agents can verify authenticity. -// The Sign method returns the signature bytes for the given action ID, -// type, and canonical params JSON. -// -// A nil ActionSigner disables signing globally (development only). -// Note that dispatchPendingActions requires a non-nil signer and will -// skip dispatch for any execution that references an action when the -// signer is nil, since the agent rejects unsigned payloads. -type ActionSigner interface { - Sign(actionID string, actionType int32, paramsJSON []byte) ([]byte, error) -} - // InboxWorker processes tasks from the control:inbox queue. // It replaces the PostgreSQL LISTEN-based Handler for gateway → control messages. +// +// signer follows the ca.ActionSigner contract: a nil signer disables +// signing (dev mode); dispatchPendingActions skips any execution +// referencing an action when signer is nil, since agents reject +// unsigned payloads. type InboxWorker struct { store *store.Store aqClient *taskqueue.Client - signer ActionSigner + signer ca.ActionSigner logger *slog.Logger } // NewInboxWorker creates a new inbox worker. -func NewInboxWorker(st *store.Store, aqClient *taskqueue.Client, signer ActionSigner, logger *slog.Logger) *InboxWorker { +func NewInboxWorker(st *store.Store, aqClient *taskqueue.Client, signer ca.ActionSigner, logger *slog.Logger) *InboxWorker { return &InboxWorker{ store: st, aqClient: aqClient, diff --git a/internal/handler/agent.go b/internal/handler/agent.go index 23c65871..926fbebd 100644 --- a/internal/handler/agent.go +++ b/internal/handler/agent.go @@ -8,6 +8,7 @@ import ( "fmt" "log/slog" "net/http" + "strings" "time" "connectrpc.com/connect" @@ -226,7 +227,7 @@ func BootstrapRedirectMiddleware(next http.Handler, bootstrapHost, assignedHost // just the hostname — the agent may include the port, // the bootstrap config typically doesn't. reqHost := r.Host - if i := indexByte(reqHost, ':'); i >= 0 { + if i := strings.IndexByte(reqHost, ':'); i >= 0 { reqHost = reqHost[:i] } if reqHost != bootstrapHost { @@ -243,17 +244,6 @@ func BootstrapRedirectMiddleware(next http.Handler, bootstrapHost, assignedHost }) } -// indexByte is a tiny stdlib-free helper so this file doesn't need -// the strings import for one call. -func indexByte(s string, c byte) int { - for i := 0; i < len(s); i++ { - if s[i] == c { - return i - } - } - return -1 -} - // Stream handles the bidirectional stream between agent and server. func (h *AgentHandler) Stream(ctx context.Context, stream *connect.BidiStream[pm.AgentMessage, pm.ServerMessage]) (err error) { // Recover from panics to prevent server crashes From 15d62e1dd45174658f77e4632be9e30b49e9ec54 Mon Sep 17 00:00:00 2001 From: PaulDotterer Date: Thu, 7 May 2026 23:33:33 +0200 Subject: [PATCH 2/2] fix: bracketed-IPv6-aware host parse in BootstrapRedirectMiddleware (CR PR #166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR caught that strings.IndexByte(reqHost, ':') breaks on bracketed IPv6 authorities like [2001:db8::1]:443 — the truncation lands at the first internal colon and produces reqHost == "[", which never matches bootstrapHost so the redirect silently misfires. Switch to net.SplitHostPort, which handles bracketed IPv6, plain IPv4, and unbracketed hostnames uniformly. Falls back to the raw r.Host when there is no port (SplitHostPort returns an error in that case), keeping unbracketed inputs working unchanged. --- internal/handler/agent.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/handler/agent.go b/internal/handler/agent.go index 926fbebd..c018b06d 100644 --- a/internal/handler/agent.go +++ b/internal/handler/agent.go @@ -7,8 +7,8 @@ import ( "errors" "fmt" "log/slog" + "net" "net/http" - "strings" "time" "connectrpc.com/connect" @@ -226,9 +226,16 @@ func BootstrapRedirectMiddleware(next http.Handler, bootstrapHost, assignedHost // :authority pseudo-header). Strip any port so we compare // just the hostname — the agent may include the port, // the bootstrap config typically doesn't. + // + // net.SplitHostPort handles bracketed IPv6 authorities + // ([2001:db8::1]:443) correctly; falling back to the raw + // r.Host when there is no port keeps unbracketed IPv4 / + // hostname inputs working unchanged. Audit / CR catch: + // strings.IndexByte(':') broke IPv6 by truncating at the + // first internal colon and producing reqHost == "[". reqHost := r.Host - if i := strings.IndexByte(reqHost, ':'); i >= 0 { - reqHost = reqHost[:i] + if h, _, err := net.SplitHostPort(reqHost); err == nil { + reqHost = h } if reqHost != bootstrapHost { next.ServeHTTP(w, r)