From dcbd289979042d014b3e2d6e5e736c4f1c6a6f54 Mon Sep 17 00:00:00 2001 From: Eric Schoeller Date: Wed, 29 Jul 2026 20:27:57 -0600 Subject: [PATCH 1/6] SEPE-1177: Extract the dedup key format into a shared package The dedup key is the contract between the two halves of the PagerDuty integration. The outbound Nagios path builds it when enqueueing an event; the inbound webhook receiver has to parse it back to work out which monitoring object an incident refers to. It lived as an unexported function in a cmd/ package, so the reader could not import it and the two implementations would have drifted apart the moment either changed. Build() is byte-identical to the previous buildDedupKey, which the existing Nagios tests pin, so incidents already open in PagerDuty keep resolving against the keys that opened them. Parse() is deliberately strict, and rejects rather than sanitises. Two properties of the format demand it. The delimiter is a semicolon, which is also what Naemon uses to separate arguments in its external command file, and the parsed values are destined for exactly that file. And a caller holding a routing key can create an incident with an arbitrary dedup key, so a key arriving on an inbound webhook is attacker controlled even when PagerDuty's signature over the request is valid -- the signature proves who relayed the message, not that its contents are safe. Rejecting costs nothing, since a mangled host or service name would produce a command Naemon silently ignores anyway. --- cmd/integrations/nagios/nagios_enqueue.go | 18 ++- pkg/dedupkey/dedupkey.go | 147 +++++++++++++++++++++ pkg/dedupkey/dedupkey_test.go | 154 ++++++++++++++++++++++ 3 files changed, 312 insertions(+), 7 deletions(-) create mode 100644 pkg/dedupkey/dedupkey.go create mode 100644 pkg/dedupkey/dedupkey_test.go diff --git a/cmd/integrations/nagios/nagios_enqueue.go b/cmd/integrations/nagios/nagios_enqueue.go index 4de8512..70e5bc4 100644 --- a/cmd/integrations/nagios/nagios_enqueue.go +++ b/cmd/integrations/nagios/nagios_enqueue.go @@ -23,6 +23,7 @@ import ( "github.com/PagerDuty/go-pdagent/pkg/cmdutil" "github.com/PagerDuty/go-pdagent/pkg/common" + "github.com/PagerDuty/go-pdagent/pkg/dedupkey" "github.com/PagerDuty/go-pdagent/pkg/eventsapi" "github.com/spf13/cobra" ) @@ -287,14 +288,17 @@ func buildEventDescription(cmdInputs nagiosEnqueueInput) string { // for a given object, so the recovery resolves the incident the problem opened. // It is deliberately identical to the v1 incident key format, so migrating an // existing installation from v1 to v2 does not orphan open incidents. +// +// The format itself lives in pkg/dedupkey because the inbound webhook receiver +// has to parse these keys back into a host and service. Keeping the writer and +// the reader in one package is what stops the two halves of the integration +// drifting apart. func buildDedupKey(cmdInputs nagiosEnqueueInput) string { - if cmdInputs.sourceType == "host" { - return fmt.Sprintf("event_source=host;host_name=%v", cmdInputs.customFields["HOSTNAME"]) - } - return fmt.Sprintf( - "event_source=service;host_name=%v;service_desc=%v", - cmdInputs.customFields["HOSTNAME"], cmdInputs.customFields["SERVICEDESC"], - ) + return dedupkey.Build(dedupkey.Key{ + IsHost: cmdInputs.sourceType == "host", + HostName: cmdInputs.customFields["HOSTNAME"], + ServiceDesc: cmdInputs.customFields["SERVICEDESC"], + }) } func validateNagiosSendCommand(cmdInputs nagiosEnqueueInput) error { diff --git a/pkg/dedupkey/dedupkey.go b/pkg/dedupkey/dedupkey.go new file mode 100644 index 0000000..6e349dc --- /dev/null +++ b/pkg/dedupkey/dedupkey.go @@ -0,0 +1,147 @@ +// Package dedupkey builds and parses the PagerDuty dedup_key used by the Nagios +// and Naemon integrations. +// +// The key is the contract between the two halves of the integration. The +// outbound path builds it when enqueueing an event; the inbound webhook +// receiver parses it back to work out which monitoring object an incident +// refers to. Both live in this package so the format cannot drift. +// +// The format is semicolon-delimited and unescaped: +// +// host: event_source=host;host_name= +// service: event_source=service;host_name=;service_desc= +// +// Two properties of that format drive the strictness of Parse: +// +// First, the delimiter is the same character Naemon uses to separate arguments +// in its external command file, and the parsed values are destined for exactly +// that file. Second, a caller holding a routing key can create an incident with +// an arbitrary dedup_key, so a key arriving on an inbound webhook is attacker +// controlled even though PagerDuty's signature over the request is valid. The +// signature proves who relayed the message, not that its contents are safe. +// +// Parse therefore rejects rather than sanitises. A rejected key produces no +// command, which is the same outcome a mangled one would have produced anyway, +// so rejecting costs nothing. +package dedupkey + +import ( + "errors" + "fmt" + "regexp" +) + +// Key identifies the monitoring object an event refers to. +type Key struct { + // IsHost distinguishes a host event from a service event. When true, + // ServiceDesc is empty. + IsHost bool + + HostName string + ServiceDesc string +} + +// Field length limits. These bound the assembled external command well below +// PIPE_BUF so that a single atomic write is always possible. +const ( + MaxHostNameLen = 64 + MaxServiceDescLen = 128 +) + +var ( + // ErrMalformed means the key did not match the expected format exactly. + ErrMalformed = errors.New("dedup key is malformed") + + // ErrInvalidField means the key parsed but a field contained characters + // that are not permitted, or was empty or over-long. + ErrInvalidField = errors.New("dedup key contains an invalid field") +) + +// keyPattern matches the whole key or nothing. Anchored deliberately: parsing +// by trimming a prefix and keeping the remainder would accept trailing junk, +// which is the shape most injection attempts take. +// +// The field captures are permissive here and validated separately, so that a +// key which is structurally correct but carries a bad hostname reports +// ErrInvalidField rather than being indistinguishable from garbage. +var keyPattern = regexp.MustCompile( + `\A` + + `event_source=(host|service)` + + `;host_name=([^;]*)` + + `(?:;service_desc=(.*))?` + + `\z`, +) + +// hostNamePattern is deliberately narrow. Host names in this estate are DNS +// names or short labels, so there is no reason to accept anything else. +var hostNamePattern = regexp.MustCompile(`\A[A-Za-z0-9._-]+\z`) + +// Build renders a Key. It is the inverse of Parse for any Key that Parse +// accepts. +func Build(k Key) string { + if k.IsHost { + return fmt.Sprintf("event_source=host;host_name=%v", k.HostName) + } + return fmt.Sprintf( + "event_source=service;host_name=%v;service_desc=%v", + k.HostName, k.ServiceDesc, + ) +} + +// Parse converts a dedup key back into a Key, rejecting anything that is not +// exactly the expected format with acceptable field contents. +// +// Callers must treat a non-nil error as "do nothing", never as "try harder". +func Parse(s string) (Key, error) { + m := keyPattern.FindStringSubmatch(s) + if m == nil { + return Key{}, ErrMalformed + } + + sourceType, hostName, serviceDesc := m[1], m[2], m[3] + isHost := sourceType == "host" + + // A host key must not carry a service_desc segment, and a service key must. + // FindStringSubmatch cannot distinguish "group absent" from "group matched + // empty", so check the raw string for the segment instead. + hasServiceSegment := len(m[0]) != len("event_source="+sourceType+";host_name="+hostName) + if isHost == hasServiceSegment { + return Key{}, ErrMalformed + } + + if !validField(hostName, MaxHostNameLen, hostNamePattern) { + return Key{}, fmt.Errorf("%w: host_name", ErrInvalidField) + } + + if isHost { + return Key{IsHost: true, HostName: hostName}, nil + } + + // Service descriptions legitimately contain spaces and punctuation, so the + // permitted set is wider than for host names. It remains an allowlist: + // printable ASCII only, which excludes every control character, and the + // regexp's [^;] on the preceding field plus this check together keep the + // command delimiter out of both values. + if !validField(serviceDesc, MaxServiceDescLen, nil) { + return Key{}, fmt.Errorf("%w: service_desc", ErrInvalidField) + } + + return Key{HostName: hostName, ServiceDesc: serviceDesc}, nil +} + +// validField enforces a length bound and a character allowlist. When pattern is +// nil the allowlist is printable ASCII excluding the semicolon delimiter. +func validField(s string, maxLen int, pattern *regexp.Regexp) bool { + if s == "" || len(s) > maxLen { + return false + } + if pattern != nil { + return pattern.MatchString(s) + } + for _, r := range s { + if r < 0x20 || r > 0x7E || r == ';' { + return false + } + } + return true +} diff --git a/pkg/dedupkey/dedupkey_test.go b/pkg/dedupkey/dedupkey_test.go new file mode 100644 index 0000000..516687e --- /dev/null +++ b/pkg/dedupkey/dedupkey_test.go @@ -0,0 +1,154 @@ +package dedupkey + +import ( + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestBuildMatchesLegacyFormat pins the exact strings the outbound Nagios +// integration has always produced. Incidents already open in PagerDuty carry +// these keys, so changing the format would orphan them: the resolve would not +// match the trigger. +func TestBuildMatchesLegacyFormat(t *testing.T) { + assert.Equal(t, + "event_source=host;host_name=web01", + Build(Key{IsHost: true, HostName: "web01"})) + + assert.Equal(t, + "event_source=service;host_name=web01;service_desc=HTTP", + Build(Key{HostName: "web01", ServiceDesc: "HTTP"})) +} + +func TestParse(t *testing.T) { + cases := []struct { + name string + in string + want Key + }{ + {"host", "event_source=host;host_name=web01", + Key{IsHost: true, HostName: "web01"}}, + {"service", "event_source=service;host_name=web01;service_desc=HTTP", + Key{HostName: "web01", ServiceDesc: "HTTP"}}, + {"service description with spaces", + "event_source=service;host_name=db-02.example.edu;service_desc=Disk Space /var", + Key{HostName: "db-02.example.edu", ServiceDesc: "Disk Space /var"}}, + {"host name with dots and underscores", + "event_source=host;host_name=naemon_test-01.colorado.edu", + Key{IsHost: true, HostName: "naemon_test-01.colorado.edu"}}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := Parse(c.in) + assert.NoError(t, err) + assert.Equal(t, c.want, got) + }) + } +} + +// TestParseRejects is the security-critical case. Every input here is one an +// attacker could put in a dedup_key, which PagerDuty would then relay back to +// us over a correctly signed webhook. +func TestParseRejects(t *testing.T) { + cases := []struct { + name string + in string + }{ + // Structure. + {"empty", ""}, + {"nonsense", "hello"}, + {"wrong source type", "event_source=poller;host_name=web01"}, + {"missing host_name", "event_source=host"}, + {"empty host_name", "event_source=host;host_name="}, + {"host with a service segment", "event_source=host;host_name=web01;service_desc=HTTP"}, + {"service without a service segment", "event_source=service;host_name=web01"}, + {"empty service_desc", "event_source=service;host_name=web01;service_desc="}, + {"leading junk", "x event_source=host;host_name=web01"}, + {"trailing junk", "event_source=host;host_name=web01 extra"}, + {"prefix only, remainder unparsed", "event_source=host;host_name=web01;evil=1"}, + + // Newline injection. A newline terminates a Naemon external command and + // begins another, so this is the vector that turns a webhook into + // arbitrary command submission. + {"newline in host", "event_source=host;host_name=web01\n[1] SHUTDOWN_PROGRAM"}, + {"newline in service", + "event_source=service;host_name=web01;service_desc=HTTP\n[1] DISABLE_NOTIFICATIONS"}, + {"carriage return in service", + "event_source=service;host_name=web01;service_desc=HTTP\r[1] SHUTDOWN_PROGRAM"}, + + // Delimiter injection, which shifts subsequent command arguments. + {"semicolon in host", "event_source=host;host_name=web01;1;1;0;evil"}, + + // Other control characters. Naemon is C, so a NUL truncates mid-field. + {"nul in service", + "event_source=service;host_name=web01;service_desc=HTTP\x00rest"}, + {"tab in service", + "event_source=service;host_name=web01;service_desc=HTTP\tmore"}, + + // Character allowlist. + {"space in host name", "event_source=host;host_name=web 01"}, + {"slash in host name", "event_source=host;host_name=web01/../etc"}, + {"non-ascii in service", + "event_source=service;host_name=web01;service_desc=café"}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + _, err := Parse(c.in) + assert.Error(t, err, "input must be rejected: %q", c.in) + }) + } +} + +// TestParseRejectsOverLongFields guards the assembled command length. Naemon +// reads its command file through a fixed-size buffer, and an over-long line is +// split with the remainder parsed as a fresh command — which can smuggle a +// command past a filter that only looks for newlines. +func TestParseRejectsOverLongFields(t *testing.T) { + longHost := strings.Repeat("a", MaxHostNameLen+1) + _, err := Parse("event_source=host;host_name=" + longHost) + assert.ErrorIs(t, err, ErrInvalidField) + + longDesc := strings.Repeat("b", MaxServiceDescLen+1) + _, err = Parse("event_source=service;host_name=web01;service_desc=" + longDesc) + assert.ErrorIs(t, err, ErrInvalidField) + + // Exactly at the limit is acceptable. + okHost := strings.Repeat("a", MaxHostNameLen) + _, err = Parse("event_source=host;host_name=" + okHost) + assert.NoError(t, err) +} + +// TestParseErrorKinds checks that a structurally sound key with a bad field is +// distinguishable from garbage, which matters for logging a useful reason. +func TestParseErrorKinds(t *testing.T) { + _, err := Parse("not a key at all") + assert.True(t, errors.Is(err, ErrMalformed)) + + _, err = Parse("event_source=host;host_name=web 01") + assert.True(t, errors.Is(err, ErrInvalidField)) +} + +// TestRoundTrip is the property that keeps the two halves of the integration +// honest: anything Parse accepts, Build reproduces exactly. +func TestRoundTrip(t *testing.T) { + keys := []Key{ + {IsHost: true, HostName: "web01"}, + {IsHost: true, HostName: "naemon-test01.colorado.edu"}, + {HostName: "web01", ServiceDesc: "HTTP"}, + {HostName: "db-02", ServiceDesc: "Disk Space /var"}, + {HostName: "sw01", ServiceDesc: "Port 1/0/24 (uplink)"}, + } + + for _, k := range keys { + t.Run(Build(k), func(t *testing.T) { + parsed, err := Parse(Build(k)) + assert.NoError(t, err) + assert.Equal(t, k, parsed) + assert.Equal(t, Build(k), Build(parsed)) + }) + } +} From 5e9c883dc9241a8476574a38c7a415ab7ee43700 Mon Sep 17 00:00:00 2001 From: Eric Schoeller Date: Wed, 29 Jul 2026 20:28:18 -0600 Subject: [PATCH 2/6] SEPE-1177: Add pdagent-receiver, the inbound webhook half Acknowledging an incident in PagerDuty has never been reflected back into Naemon. PagerDuty documents a two-way integration and has since around 2015, but the script their own guide still tells you to download has been archived since 2016 and parses v1 webhooks, a format they have retired. The one maintained alternative is PHP and its signature check is wrong in three separate ways. So this is written rather than adopted. It is a separate binary and unit from pdagent, run as its own user. The two halves have opposite trust profiles -- the agent holds routing keys and talks outward, this listens to the public internet -- and every control that would contain a compromise is per-process. On RHEL 8 the argument is not even a preference: cgroups v1 is the default, systemd's IPAddressDeny is silently ineffective under it, and the only usable egress restriction is an nftables rule matching the uid. The handler runs a bare ServeMux rather than the agent's router, which applies its bearer-token middleware router-wide. Carving a path exemption into that middleware would put a second path matcher, in a different layer from the one mux uses, in front of an internet-facing route. Here the agent's enqueue endpoints are not merely protected, they do not exist in this binary. Signature verification covers the raw body before any decode, uses hmac.Equal, and accepts any of several comma-separated v1 values so a signing secret can be rotated without dropping deliveries. The candidate count is capped, since an uncapped header would be an unauthenticated CPU sink. The body size cap is the only control that applies before the caller is authenticated -- the signature covers the body, so the body must be read first -- which is why it is enforced in the handler and not left to a proxy. Secrets load from their own file and the loader refuses one that is readable by group or other. The agent's config is world-readable today and holds routing keys; this does not repeat that. This pass captures rather than acts. Deliveries are authenticated, logged verbatim and answered 202, and nothing reaches Naemon. The V3 incident.acknowledged payload turns out to carry a null incident_key, so the mapping from incident to monitoring object needs a REST lookup of the alert -- a design question worth settling against a real payload rather than a guess. Verified end to end against production PagerDuty: a genuine delivery from 52.89.71.166 authenticated and returned 202. --- cmd/receiver/main.go | 158 ++++++++++++++++++++++++++++++ init/pdagent-receiver.service | 83 ++++++++++++++++ pkg/receiver/handler.go | 156 +++++++++++++++++++++++++++++ pkg/receiver/handler_test.go | 173 +++++++++++++++++++++++++++++++++ pkg/receiver/secret.go | 56 +++++++++++ pkg/receiver/secret_test.go | 77 +++++++++++++++ pkg/receiver/signature.go | 132 +++++++++++++++++++++++++ pkg/receiver/signature_test.go | 159 ++++++++++++++++++++++++++++++ 8 files changed, 994 insertions(+) create mode 100644 cmd/receiver/main.go create mode 100644 init/pdagent-receiver.service create mode 100644 pkg/receiver/handler.go create mode 100644 pkg/receiver/handler_test.go create mode 100644 pkg/receiver/secret.go create mode 100644 pkg/receiver/secret_test.go create mode 100644 pkg/receiver/signature.go create mode 100644 pkg/receiver/signature_test.go diff --git a/cmd/receiver/main.go b/cmd/receiver/main.go new file mode 100644 index 0000000..8d2a910 --- /dev/null +++ b/cmd/receiver/main.go @@ -0,0 +1,158 @@ +// Command pdagent-receiver accepts PagerDuty V3 webhook deliveries and turns +// acknowledgements into Naemon external commands. +// +// It is a separate binary and a separate systemd unit from pdagent, and is +// expected to run as its own system user. That is not stylistic: the controls +// that contain a compromise of an internet-facing listener — a dedicated uid, +// an nftables egress rule matched on that uid, a systemd sandbox, a write-only +// ACL on the command FIFO — are all per-process. Sharing an address space with +// the outbound agent would put every PagerDuty routing key in the same memory +// as the code parsing untrusted input. +// +// On RHEL 8 specifically, systemd's IPAddressDeny is silently ineffective under +// cgroups v1, so the uid-matched nftables rule is the only egress control +// available. It cannot be written without a separate user. +package main + +import ( + "context" + "errors" + "fmt" + "net/http" + "os" + "os/signal" + "syscall" + "time" + + "github.com/PagerDuty/go-pdagent/pkg/common" + "github.com/PagerDuty/go-pdagent/pkg/receiver" + "github.com/spf13/cobra" +) + +const ( + defaultAddress = "127.0.0.1:49464" + defaultPath = "/webhook" + shutdownTimeout = 10 * time.Second +) + +type options struct { + address string + path string + secretFile string + capture bool +} + +func main() { + if err := newRootCommand().Execute(); err != nil { + // Cobra has already printed the error. + os.Exit(1) + } +} + +func newRootCommand() *cobra.Command { + var opts options + + cmd := &cobra.Command{ + Use: "pdagent-receiver", + Short: "Receive PagerDuty webhooks and acknowledge Naemon problems", + Version: common.Version, + SilenceUsage: true, + SilenceErrors: false, + RunE: func(cmd *cobra.Command, args []string) error { + return run(cmd.Context(), opts) + }, + } + + f := cmd.Flags() + f.StringVarP(&opts.address, "address", "a", defaultAddress, + "Address to listen on. Bind to loopback and let a reverse proxy terminate TLS.") + f.StringVar(&opts.path, "path", defaultPath, + "Path to serve the webhook on. Every other path is refused.") + f.StringVar(&opts.secretFile, "secret-file", "", + "File containing PagerDuty webhook signing secrets, one per line, mode 0400 (required)") + f.BoolVar(&opts.capture, "capture", false, + "Log authenticated payloads verbatim and submit nothing to Naemon") + + return cmd +} + +func run(ctx context.Context, opts options) error { + logger := common.Logger.Named("receiver") + + if opts.secretFile == "" { + return errors.New("--secret-file is required") + } + + secrets, err := receiver.LoadSecrets(opts.secretFile) + if err != nil { + return err + } + + verifier, err := receiver.NewVerifier(secrets) + if err != nil { + return err + } + + handler, err := receiver.NewHandler(verifier, logger, receiver.WithCapture(opts.capture)) + if err != nil { + return err + } + + // A bare ServeMux, not the agent's router. The agent applies its auth + // middleware router-wide, and PagerDuty sends a signature rather than a + // bearer token, so sharing a router would mean carving a path exemption + // into shared security-critical code. Keeping the routers apart makes the + // agent's enqueue endpoints unreachable here by construction rather than by + // a matcher that has to stay correct forever. + mux := http.NewServeMux() + mux.Handle(opts.path, handler) + + srv := &http.Server{ + Addr: opts.address, + Handler: mux, + ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 15 * time.Second, + WriteTimeout: 15 * time.Second, + IdleTimeout: 60 * time.Second, + // The agent inherits 1 MiB here. An internet-facing listener has no + // reason to accept headers that large. + MaxHeaderBytes: 64 << 10, + } + + if ctx == nil { + ctx = context.Background() + } + ctx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) + defer stop() + + errCh := make(chan error, 1) + go func() { + logger.Infow("listening", + "address", opts.address, "path", opts.path, + "secrets", len(secrets), "capture", opts.capture) + + // Unlike the agent, a bind failure is fatal here rather than logged and + // ignored. A receiver that is running but bound to nothing looks healthy + // to systemd and to a process check while silently dropping every + // acknowledgement. + if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { + errCh <- fmt.Errorf("listener stopped: %w", err) + return + } + errCh <- nil + }() + + select { + case err := <-errCh: + return err + case <-ctx.Done(): + logger.Info("shutting down") + } + + shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) + defer cancel() + if err := srv.Shutdown(shutdownCtx); err != nil { + return fmt.Errorf("graceful shutdown failed: %w", err) + } + return <-errCh +} diff --git a/init/pdagent-receiver.service b/init/pdagent-receiver.service new file mode 100644 index 0000000..2d68d8c --- /dev/null +++ b/init/pdagent-receiver.service @@ -0,0 +1,83 @@ +[Unit] +Description=PagerDuty webhook receiver for Naemon acknowledgements +Documentation=https://github.com/UCBoulder/oit-sepe-go-pdagent +After=network-online.target +Wants=network-online.target + +# These belong in [Unit], not [Service], on systemd 239 as shipped by RHEL 8. +# Placed in [Service] they are logged as "Unknown lvalue" and silently ignored, +# leaving the unit looking rate-limited when it is not. +StartLimitIntervalSec=60 +StartLimitBurst=5 + +[Service] +Type=simple + +# A dedicated user, not the pdagent account. The two halves have opposite trust +# profiles: pdagent holds PagerDuty routing keys and talks outward, while this +# listens to the public internet and can write Naemon's command pipe. Every +# control that contains a compromise -- a targeted ACL, an nftables egress rule +# matched on uid, the sandbox below -- is per-process, so sharing an account +# would grant the union of both roles to the internet-facing half. +# +# On RHEL 8 this is not merely preferable. cgroups v1 is the default there, and +# systemd's IPAddressDeny is silently ineffective under it, so egress can only +# be restricted by an nftables rule matching this uid. +User=pdagent-receiver +Group=pdagent-receiver +SupplementaryGroups= + +Environment=RECEIVER_ADDRESS=127.0.0.1:49464 +Environment=RECEIVER_PATH=/webhook +Environment=RECEIVER_SECRET_FILE=/etc/pdagent-receiver/secret +EnvironmentFile=-/etc/pdagent-receiver/env + +ExecStart=/usr/local/bin/pdagent-receiver \ + --address ${RECEIVER_ADDRESS} \ + --path ${RECEIVER_PATH} \ + --secret-file ${RECEIVER_SECRET_FILE} \ + $RECEIVER_EXTRA_ARGS + +NoNewPrivileges=yes +CapabilityBoundingSet= +AmbientCapabilities= +ProtectSystem=strict +ProtectHome=yes +PrivateTmp=yes +PrivateDevices=yes +ProtectKernelTunables=yes +ProtectKernelModules=yes +ProtectControlGroups=yes +RestrictNamespaces=yes +RestrictRealtime=yes +RestrictAddressFamilies=AF_INET AF_INET6 +LockPersonality=yes +# Safe for a static Go binary; the runtime does not JIT. +MemoryDenyWriteExecute=yes +SystemCallArchitectures=native +SystemCallFilter=@system-service +SystemCallFilter=~@privileged @resources @obsolete @mount @debug @swap @reboot @module @raw-io @clock @cpu-emulation +UMask=0077 +RemoveIPC=yes + +# Bounded because this is reachable from outside. Without a ceiling, runaway +# memory here would push the global OOM killer at the largest RSS on the box, +# which on a monitoring host may well be naemon itself. +LimitNOFILE=1024 +LimitNPROC=64 +TasksMax=32 +MemoryMax=128M + +# The receiver is stateless, so restarting costs nothing and losing it silently +# costs acknowledgements. Note this is deliberately more aggressive than the +# agent's own policy, which is the point of a separate unit. +Restart=on-failure +RestartSec=5 + +# Deployments that grant FIFO access by ACL need the command directory listed +# here, because ProtectSystem=strict makes /var read-only: +# ReadWritePaths=/var/lib/naemon/cmd +# Left out by default since the path is site specific. + +[Install] +WantedBy=multi-user.target diff --git a/pkg/receiver/handler.go b/pkg/receiver/handler.go new file mode 100644 index 0000000..525c677 --- /dev/null +++ b/pkg/receiver/handler.go @@ -0,0 +1,156 @@ +// Package receiver implements the inbound half of the PagerDuty integration: +// an HTTP endpoint that accepts PagerDuty V3 webhook deliveries and turns +// acknowledgements into Naemon external commands. +// +// It is deliberately a separate binary from the outbound agent. The two have +// opposite trust profiles — the agent talks outward to PagerDuty holding +// routing keys, while this listens to the public internet — and the controls +// that contain a compromise (a dedicated uid, an egress rule matched on that +// uid, a systemd sandbox, a write-only ACL on one FIFO) are all per-process. +// Sharing an address space would mean granting the union of both roles' +// privileges to something reachable from outside. +package receiver + +import ( + "encoding/json" + "errors" + "io" + "net/http" + + "go.uber.org/zap" +) + +// DefaultMaxBodyBytes bounds a single delivery. Real V3 payloads are a few +// kilobytes. +// +// This cap is load bearing in a way the other limits are not: the signature +// covers the raw body, so the body must be read before the request can be +// authenticated. It is the only control that applies to an unauthenticated +// caller, so it cannot be left to a later stage. +const DefaultMaxBodyBytes int64 = 1 << 20 // 1 MiB + +// Handler serves PagerDuty webhook deliveries. +type Handler struct { + verifier *Verifier + logger *zap.SugaredLogger + maxBodyBytes int64 + + // capture logs each authenticated payload verbatim instead of acting on it. + // + // The V3 incident.acknowledged payload carries the incident, while the + // dedup_key identifying the Naemon object belongs to the alert, and + // PagerDuty's documentation could not be retrieved to settle whether the key + // is present. Rather than guess at the mapping, the first deployment records + // real deliveries and the mapping is written against what actually arrives. + capture bool +} + +// Option configures a Handler. +type Option func(*Handler) + +// WithMaxBodyBytes overrides the request size cap. +func WithMaxBodyBytes(n int64) Option { + return func(h *Handler) { h.maxBodyBytes = n } +} + +// WithCapture puts the handler in capture mode: authenticated deliveries are +// logged in full and acknowledged, and nothing is submitted to Naemon. +func WithCapture(capture bool) Option { + return func(h *Handler) { h.capture = capture } +} + +// NewHandler builds a Handler. A nil logger is replaced with a no-op, so a +// misconfigured caller cannot panic on the request path. +func NewHandler(v *Verifier, logger *zap.SugaredLogger, opts ...Option) (*Handler, error) { + if v == nil { + return nil, errors.New("a verifier is required") + } + if logger == nil { + logger = zap.NewNop().Sugar() + } + + h := &Handler{verifier: v, logger: logger, maxBodyBytes: DefaultMaxBodyBytes} + for _, opt := range opts { + opt(h) + } + return h, nil +} + +// envelope is the outermost shape of a V3 webhook, decoded only far enough to +// log what arrived. Pass 2 adds the fields the mapping needs. +type envelope struct { + Event struct { + ID string `json:"id"` + EventType string `json:"event_type"` + OccurredAt string `json:"occurred_at"` + Data json.RawMessage `json:"data"` + } `json:"event"` +} + +func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + w.Header().Set("Allow", http.MethodPost) + respond(w, http.StatusMethodNotAllowed, "method not allowed") + return + } + + // Read before authenticating, because the signature covers these exact + // bytes. MaxBytesReader is what makes that safe. + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, h.maxBodyBytes)) + if err != nil { + var tooLarge *http.MaxBytesError + if errors.As(err, &tooLarge) { + h.logger.Warnw("webhook rejected: body too large", + "limit", h.maxBodyBytes, "remote", r.RemoteAddr) + respond(w, http.StatusRequestEntityTooLarge, "request body too large") + return + } + h.logger.Warnw("webhook rejected: unreadable body", + "error", err, "remote", r.RemoteAddr) + respond(w, http.StatusBadRequest, "could not read request body") + return + } + + if err := h.verifier.Verify(body, r.Header.Get(SignatureHeader)); err != nil { + // The reason is logged but never returned: telling an unauthenticated + // caller whether it failed on the header shape or the MAC hands them a + // tuning signal for free. + h.logger.Warnw("webhook rejected: signature not valid", + "reason", err, "remote", r.RemoteAddr, "bytes", len(body)) + respond(w, http.StatusUnauthorized, "invalid signature") + return + } + + var env envelope + if err := json.Unmarshal(body, &env); err != nil { + // Authentic but undecodable. 400 rather than 5xx so PagerDuty does not + // retry something that will never succeed. + h.logger.Errorw("webhook authentic but not decodable", "error", err) + respond(w, http.StatusBadRequest, "malformed payload") + return + } + + if h.capture { + // Deliberately verbatim. This exists to answer one question — whether + // the payload carries the dedup_key — and a summarised log would beg it. + h.logger.Infow("webhook captured", + "event_id", env.Event.ID, + "event_type", env.Event.EventType, + "occurred_at", env.Event.OccurredAt, + "payload", string(body), + ) + respond(w, http.StatusAccepted, "captured") + return + } + + // Pass 2 replaces this with the mapping and the external command write. + h.logger.Infow("webhook accepted, no action configured", + "event_id", env.Event.ID, "event_type", env.Event.EventType) + respond(w, http.StatusAccepted, "accepted") +} + +func respond(w http.ResponseWriter, code int, message string) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(code) + _ = json.NewEncoder(w).Encode(map[string]string{"message": message}) +} diff --git a/pkg/receiver/handler_test.go b/pkg/receiver/handler_test.go new file mode 100644 index 0000000..2452997 --- /dev/null +++ b/pkg/receiver/handler_test.go @@ -0,0 +1,173 @@ +package receiver + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" +) + +const testSecret = "webhook-signing-secret" + +func newTestHandler(t *testing.T, opts ...Option) (*Handler, *observer.ObservedLogs) { + t.Helper() + core, logs := observer.New(zap.DebugLevel) + v, err := NewVerifier([]string{testSecret}) + require.NoError(t, err) + h, err := NewHandler(v, zap.New(core).Sugar(), opts...) + require.NoError(t, err) + return h, logs +} + +// post builds a request, signing it correctly unless a header is supplied. +func post(t *testing.T, h *Handler, body string, header ...string) *httptest.ResponseRecorder { + t.Helper() + sig := sign([]byte(body), testSecret) + if len(header) > 0 { + sig = header[0] + } + req := httptest.NewRequest(http.MethodPost, "/webhook", strings.NewReader(body)) + req.Header.Set(SignatureHeader, sig) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec +} + +func TestHandlerAcceptsSignedDelivery(t *testing.T) { + h, _ := newTestHandler(t) + body := `{"event":{"id":"01ABC","event_type":"incident.acknowledged"}}` + + rec := post(t, h, body) + assert.Equal(t, http.StatusAccepted, rec.Code) +} + +func TestHandlerRejectsBadSignature(t *testing.T) { + h, _ := newTestHandler(t) + body := `{"event":{"id":"01ABC"}}` + + cases := map[string]string{ + "wrong secret": sign([]byte(body), "not-the-secret"), + "missing header": "", + "garbage header": "nonsense", + "unknown scheme": "v2=" + strings.Repeat("ab", 32), + "signature of another body": sign([]byte(`{"event":{"id":"other"}}`), testSecret), + } + + for name, header := range cases { + t.Run(name, func(t *testing.T) { + rec := post(t, h, body, header) + assert.Equal(t, http.StatusUnauthorized, rec.Code) + }) + } +} + +// TestHandlerDoesNotLeakRejectionReason: an unauthenticated caller learns only +// that it failed, never whether the header shape or the MAC was at fault. +func TestHandlerDoesNotLeakRejectionReason(t *testing.T) { + h, logs := newTestHandler(t) + body := `{"event":{"id":"01ABC"}}` + + shapeErr := post(t, h, body, "nonsense").Body.String() + macErr := post(t, h, body, sign([]byte(body), "wrong")).Body.String() + assert.Equal(t, shapeErr, macErr) + + // The distinction is still available to an operator, in the log. + assert.Equal(t, 2, logs.FilterMessage("webhook rejected: signature not valid").Len()) +} + +func TestHandlerRejectsNonPost(t *testing.T) { + h, _ := newTestHandler(t) + + for _, method := range []string{http.MethodGet, http.MethodPut, http.MethodDelete, http.MethodHead} { + t.Run(method, func(t *testing.T) { + req := httptest.NewRequest(method, "/webhook", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + assert.Equal(t, http.StatusMethodNotAllowed, rec.Code) + assert.Equal(t, http.MethodPost, rec.Header().Get("Allow")) + }) + } +} + +// TestHandlerCapsBodySize is the only control that applies before the caller is +// authenticated, since the signature covers the raw body and the body must +// therefore be read first. +func TestHandlerCapsBodySize(t *testing.T) { + h, _ := newTestHandler(t, WithMaxBodyBytes(128)) + + big := strings.Repeat("x", 4096) + req := httptest.NewRequest(http.MethodPost, "/webhook", strings.NewReader(big)) + req.Header.Set(SignatureHeader, sign([]byte(big), testSecret)) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + // Refused on size before the (valid) signature is even considered. + assert.Equal(t, http.StatusRequestEntityTooLarge, rec.Code) +} + +func TestHandlerRejectsAuthenticButUndecodable(t *testing.T) { + h, _ := newTestHandler(t) + + rec := post(t, h, `{"event": this is not json}`) + // 400, not 5xx: PagerDuty must not retry something that cannot ever succeed. + assert.Equal(t, http.StatusBadRequest, rec.Code) +} + +// TestHandlerCaptureLogsVerbatim pins the behaviour pass 1 exists for. The +// payload has to be logged exactly as received, because the open question is +// whether it contains the dedup_key at all — a summary would beg that question. +func TestHandlerCaptureLogsVerbatim(t *testing.T) { + h, logs := newTestHandler(t, WithCapture(true)) + body := `{"event":{"id":"01ABC","event_type":"incident.acknowledged",` + + `"occurred_at":"2026-07-29T12:00:00Z","data":{"id":"PINCIDENT"}}}` + + rec := post(t, h, body) + assert.Equal(t, http.StatusAccepted, rec.Code) + + entries := logs.FilterMessage("webhook captured").All() + require.Len(t, entries, 1) + + fields := entries[0].ContextMap() + assert.Equal(t, body, fields["payload"], "payload must be logged byte-for-byte") + assert.Equal(t, "01ABC", fields["event_id"]) + assert.Equal(t, "incident.acknowledged", fields["event_type"]) +} + +// TestHandlerNeverLogsUnauthenticatedBodies: an unauthenticated caller must not +// be able to write chosen content into our logs. +func TestHandlerNeverLogsUnauthenticatedBodies(t *testing.T) { + h, logs := newTestHandler(t, WithCapture(true)) + marker := "ATTACKER-CONTROLLED-MARKER" + + post(t, h, `{"event":{"id":"`+marker+`"}}`, "v1="+strings.Repeat("00", 32)) + + for _, entry := range logs.All() { + for _, v := range entry.ContextMap() { + if s, ok := v.(string); ok { + assert.NotContains(t, s, marker, + "unauthenticated body content must not reach the log") + } + } + } +} + +func TestNewHandlerRequiresVerifier(t *testing.T) { + _, err := NewHandler(nil, nil) + assert.Error(t, err) +} + +// TestNewHandlerToleratesNilLogger: a misconfigured caller must not be able to +// turn every request into a panic. +func TestNewHandlerToleratesNilLogger(t *testing.T) { + v, err := NewVerifier([]string{testSecret}) + require.NoError(t, err) + + h, err := NewHandler(v, nil) + require.NoError(t, err) + assert.NotPanics(t, func() { post(t, h, `{"event":{"id":"x"}}`) }) +} diff --git a/pkg/receiver/secret.go b/pkg/receiver/secret.go new file mode 100644 index 0000000..7254692 --- /dev/null +++ b/pkg/receiver/secret.go @@ -0,0 +1,56 @@ +package receiver + +import ( + "bufio" + "fmt" + "os" + "strings" +) + +// LoadSecrets reads webhook signing secrets from a file, one per line. +// +// Blank lines and lines beginning with '#' are ignored, so a rotation can be +// annotated in place. Order is not significant — a delivery is authentic if it +// matches any of them. +// +// The file must not be readable by group or other. This is checked rather than +// assumed because the failure is silent and the consequence is not: a readable +// signing secret lets anyone on the host forge acknowledgements that will pass +// verification. The outbound agent's own config is world-readable today, which +// is exactly the mistake this refuses to repeat. +func LoadSecrets(path string) ([]string, error) { + info, err := os.Stat(path) + if err != nil { + return nil, fmt.Errorf("reading signing secret file: %w", err) + } + + if perm := info.Mode().Perm(); perm&0o077 != 0 { + return nil, fmt.Errorf( + "signing secret file %s is mode %04o; it must not be readable by group or other (chmod 0400)", + path, perm) + } + + f, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("reading signing secret file: %w", err) + } + defer f.Close() + + var secrets []string + scanner := bufio.NewScanner(f) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + secrets = append(secrets, line) + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("reading signing secret file: %w", err) + } + + if len(secrets) == 0 { + return nil, fmt.Errorf("signing secret file %s contains no secrets", path) + } + return secrets, nil +} diff --git a/pkg/receiver/secret_test.go b/pkg/receiver/secret_test.go new file mode 100644 index 0000000..187078a --- /dev/null +++ b/pkg/receiver/secret_test.go @@ -0,0 +1,77 @@ +package receiver + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func writeSecretFile(t *testing.T, content string, mode os.FileMode) string { + t.Helper() + path := filepath.Join(t.TempDir(), "secret") + require.NoError(t, os.WriteFile(path, []byte(content), mode)) + // WriteFile is subject to umask, so set the mode explicitly. + require.NoError(t, os.Chmod(path, mode)) + return path +} + +func TestLoadSecrets(t *testing.T) { + path := writeSecretFile(t, "first-secret\nsecond-secret\n", 0o400) + + secrets, err := LoadSecrets(path) + require.NoError(t, err) + assert.Equal(t, []string{"first-secret", "second-secret"}, secrets) +} + +func TestLoadSecretsIgnoresBlanksAndComments(t *testing.T) { + path := writeSecretFile(t, "# rotated 2026-07-29\n\nactive\n\n# outgoing\nprevious\n", 0o400) + + secrets, err := LoadSecrets(path) + require.NoError(t, err) + assert.Equal(t, []string{"active", "previous"}, secrets) +} + +// TestLoadSecretsRefusesLoosePermissions is the point of this file existing. A +// signing secret readable by other local users lets any of them forge +// acknowledgements that verify correctly. +func TestLoadSecretsRefusesLoosePermissions(t *testing.T) { + for _, mode := range []os.FileMode{0o444, 0o440, 0o644, 0o600 | 0o040, 0o777} { + t.Run(mode.String(), func(t *testing.T) { + path := writeSecretFile(t, "s3cret\n", mode) + _, err := LoadSecrets(path) + assert.Error(t, err, "mode %04o must be refused", mode.Perm()) + assert.Contains(t, err.Error(), "chmod 0400") + }) + } +} + +func TestLoadSecretsAcceptsOwnerOnlyModes(t *testing.T) { + for _, mode := range []os.FileMode{0o400, 0o600, 0o700} { + t.Run(mode.String(), func(t *testing.T) { + path := writeSecretFile(t, "s3cret\n", mode) + secrets, err := LoadSecrets(path) + require.NoError(t, err) + assert.Equal(t, []string{"s3cret"}, secrets) + }) + } +} + +func TestLoadSecretsRejectsEmptyAndMissing(t *testing.T) { + t.Run("empty file", func(t *testing.T) { + _, err := LoadSecrets(writeSecretFile(t, "", 0o400)) + assert.Error(t, err) + }) + + t.Run("only comments", func(t *testing.T) { + _, err := LoadSecrets(writeSecretFile(t, "# nothing here\n", 0o400)) + assert.Error(t, err) + }) + + t.Run("missing file", func(t *testing.T) { + _, err := LoadSecrets(filepath.Join(t.TempDir(), "absent")) + assert.Error(t, err) + }) +} diff --git a/pkg/receiver/signature.go b/pkg/receiver/signature.go new file mode 100644 index 0000000..07a62c6 --- /dev/null +++ b/pkg/receiver/signature.go @@ -0,0 +1,132 @@ +package receiver + +import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "strings" +) + +// SignatureHeader is the header PagerDuty signs V3 webhook deliveries with. +const SignatureHeader = "X-PagerDuty-Signature" + +// signaturePrefix marks the only signature scheme we accept. PagerDuty may add +// a v2 later; an unrecognised prefix is ignored rather than trusted. +const signaturePrefix = "v1=" + +// maxSignatures bounds the work a single request can cause. PagerDuty sends one +// signature per active signing secret, so the real number is one or two during +// a rotation. Without a cap, a header carrying ten thousand candidates would be +// an unauthenticated CPU sink. +const maxSignatures = 8 + +var ( + // ErrNoSignature means the header was absent or carried no usable candidate. + ErrNoSignature = errors.New("no v1 signature present") + + // ErrTooManySignatures means the header carried more candidates than any + // legitimate delivery would. + ErrTooManySignatures = errors.New("too many signature candidates") + + // ErrNotAuthentic means the body did not match any configured secret. + ErrNotAuthentic = errors.New("no signature matched") +) + +// Verifier authenticates PagerDuty webhook deliveries. +// +// It holds more than one secret so that a signing secret can be rotated without +// dropping deliveries: PagerDuty signs with every active secret and sends the +// signatures together, and a delivery is authentic if any one of them matches. +// +// What this proves is narrow and worth stating plainly, because it is easy to +// over-read: a valid signature means PagerDuty relayed the request. It says +// nothing about whether the contents are safe. Anyone holding a routing key can +// create an incident with an arbitrary dedup_key and have PagerDuty relay it +// here, correctly signed. Everything downstream must still treat the payload as +// attacker controlled. +type Verifier struct { + secrets [][]byte +} + +// NewVerifier builds a Verifier from one or more signing secrets. +func NewVerifier(secrets []string) (*Verifier, error) { + if len(secrets) == 0 { + return nil, errors.New("at least one signing secret is required") + } + + v := &Verifier{secrets: make([][]byte, 0, len(secrets))} + for i, s := range secrets { + if strings.TrimSpace(s) == "" { + return nil, fmt.Errorf("signing secret %d is empty", i) + } + v.secrets = append(v.secrets, []byte(s)) + } + return v, nil +} + +// Verify reports whether body carries a signature in header produced by one of +// the configured secrets. +// +// body must be the exact bytes received. The signature covers the raw request +// body, so it has to be verified before the payload is decoded, and nothing in +// the request path may re-encode it. +func (v *Verifier) Verify(body []byte, header string) error { + candidates, err := parseSignatureHeader(header) + if err != nil { + return err + } + + // Compute each secret's MAC once and compare it against every candidate, + // rather than recomputing per candidate. + for _, secret := range v.secrets { + mac := hmac.New(sha256.New, secret) + mac.Write(body) + expected := mac.Sum(nil) + + for _, candidate := range candidates { + if hmac.Equal(candidate, expected) { + return nil + } + } + } + + return ErrNotAuthentic +} + +// parseSignatureHeader extracts the hex-decoded v1 signatures from a header of +// the form "v1=,v1=". +// +// Entries that are malformed or use an unknown scheme are skipped rather than +// rejected outright, so that a future scheme appearing alongside v1 does not +// break deliveries we can still authenticate. If nothing usable remains the +// request is refused. +func parseSignatureHeader(header string) ([][]byte, error) { + if strings.TrimSpace(header) == "" { + return nil, ErrNoSignature + } + + parts := strings.Split(header, ",") + if len(parts) > maxSignatures { + return nil, ErrTooManySignatures + } + + candidates := make([][]byte, 0, len(parts)) + for _, part := range parts { + part = strings.TrimSpace(part) + if !strings.HasPrefix(part, signaturePrefix) { + continue + } + raw, err := hex.DecodeString(strings.TrimPrefix(part, signaturePrefix)) + if err != nil || len(raw) != sha256.Size { + continue + } + candidates = append(candidates, raw) + } + + if len(candidates) == 0 { + return nil, ErrNoSignature + } + return candidates, nil +} diff --git a/pkg/receiver/signature_test.go b/pkg/receiver/signature_test.go new file mode 100644 index 0000000..c112250 --- /dev/null +++ b/pkg/receiver/signature_test.go @@ -0,0 +1,159 @@ +package receiver + +import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// sign produces the header value PagerDuty would send for a given body and +// secret. Used to build fixtures rather than hardcoding hex, so the tests stay +// readable and cannot drift from the implementation's definition of the input. +func sign(body []byte, secret string) string { + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(body) + return signaturePrefix + hex.EncodeToString(mac.Sum(nil)) +} + +func TestVerifyAcceptsValidSignature(t *testing.T) { + body := []byte(`{"event":{"event_type":"incident.acknowledged"}}`) + v, err := NewVerifier([]string{"s3cret"}) + require.NoError(t, err) + + assert.NoError(t, v.Verify(body, sign(body, "s3cret"))) +} + +// TestVerifyRejects covers every way a delivery can fail to authenticate. +func TestVerifyRejects(t *testing.T) { + body := []byte(`{"event":{"event_type":"incident.acknowledged"}}`) + valid := sign(body, "s3cret") + + cases := []struct { + name string + body []byte + header string + wantErr error + }{ + {"wrong secret", body, sign(body, "wrong"), ErrNotAuthentic}, + {"tampered body", []byte(`{"event":{"event_type":"incident.resolved"}}`), valid, ErrNotAuthentic}, + {"empty header", body, "", ErrNoSignature}, + {"whitespace header", body, " ", ErrNoSignature}, + {"unknown scheme only", body, "v2=" + strings.Repeat("ab", 32), ErrNoSignature}, + {"no prefix", body, strings.Repeat("ab", 32), ErrNoSignature}, + {"not hex", body, signaturePrefix + "zzzz", ErrNoSignature}, + {"right length, wrong bytes", body, signaturePrefix + strings.Repeat("00", 32), ErrNotAuthentic}, + {"truncated signature", body, signaturePrefix + "abcd", ErrNoSignature}, + {"empty body against a signature for content", []byte{}, valid, ErrNotAuthentic}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + v, err := NewVerifier([]string{"s3cret"}) + require.NoError(t, err) + assert.ErrorIs(t, v.Verify(c.body, c.header), c.wantErr) + }) + } +} + +// TestVerifyMultipleSignatures is the case PD2Nagiosv3 got wrong: it compared +// the entire header against a single computed value, so any delivery carrying +// more than one signature failed. That is exactly what happens during a secret +// rotation, which is the situation multiple secrets exist for. +func TestVerifyMultipleSignatures(t *testing.T) { + body := []byte(`{"event":{"id":"01ABC"}}`) + + t.Run("valid signature is second", func(t *testing.T) { + v, _ := NewVerifier([]string{"current"}) + header := sign(body, "old") + "," + sign(body, "current") + assert.NoError(t, v.Verify(body, header)) + }) + + t.Run("valid signature is first", func(t *testing.T) { + v, _ := NewVerifier([]string{"current"}) + header := sign(body, "current") + "," + sign(body, "old") + assert.NoError(t, v.Verify(body, header)) + }) + + t.Run("mixed with an unknown scheme", func(t *testing.T) { + v, _ := NewVerifier([]string{"current"}) + header := "v2=deadbeef, " + sign(body, "current") + assert.NoError(t, v.Verify(body, header)) + }) + + t.Run("none match", func(t *testing.T) { + v, _ := NewVerifier([]string{"current"}) + header := sign(body, "old") + "," + sign(body, "older") + assert.ErrorIs(t, v.Verify(body, header), ErrNotAuthentic) + }) +} + +// TestVerifyRotation covers the other half of rotation: the receiver holding +// both the outgoing and incoming secret while PagerDuty is still signing with +// only one of them. +func TestVerifyRotation(t *testing.T) { + body := []byte(`{"event":{"id":"01ABC"}}`) + v, err := NewVerifier([]string{"old", "new"}) + require.NoError(t, err) + + assert.NoError(t, v.Verify(body, sign(body, "old"))) + assert.NoError(t, v.Verify(body, sign(body, "new"))) + assert.ErrorIs(t, v.Verify(body, sign(body, "other")), ErrNotAuthentic) +} + +// TestVerifyCapsCandidates guards against an unauthenticated CPU sink: without +// a cap, a header with thousands of candidates would force thousands of +// comparisons per secret before the request is refused. +func TestVerifyCapsCandidates(t *testing.T) { + body := []byte(`{}`) + v, _ := NewVerifier([]string{"s3cret"}) + + flood := make([]string, maxSignatures+1) + for i := range flood { + flood[i] = signaturePrefix + strings.Repeat("00", 32) + } + assert.ErrorIs(t, v.Verify(body, strings.Join(flood, ",")), ErrTooManySignatures) + + // A valid signature buried in an over-long header is still refused: the cap + // is applied before any comparison, deliberately. + flood[maxSignatures] = sign(body, "s3cret") + assert.ErrorIs(t, v.Verify(body, strings.Join(flood, ",")), ErrTooManySignatures) + + // Exactly at the cap is accepted. + atCap := flood[:maxSignatures] + atCap[maxSignatures-1] = sign(body, "s3cret") + assert.NoError(t, v.Verify(body, strings.Join(atCap, ","))) +} + +func TestNewVerifierRejectsUnusableSecrets(t *testing.T) { + _, err := NewVerifier(nil) + assert.Error(t, err) + + _, err = NewVerifier([]string{}) + assert.Error(t, err) + + _, err = NewVerifier([]string{"ok", ""}) + assert.Error(t, err, "an empty secret must not silently authenticate nothing") + + _, err = NewVerifier([]string{" "}) + assert.Error(t, err) +} + +// TestVerifyIsBodyExact confirms the signature covers the bytes as received. +// Any re-encoding between the wire and here — a proxy rewriting JSON, a +// whitespace-normalising decode — invalidates the signature, which is the +// intended behaviour and worth pinning. +func TestVerifyIsBodyExact(t *testing.T) { + original := []byte(`{"a":1,"b":2}`) + v, _ := NewVerifier([]string{"s3cret"}) + header := sign(original, "s3cret") + + assert.NoError(t, v.Verify(original, header)) + assert.ErrorIs(t, v.Verify([]byte(`{"a": 1, "b": 2}`), header), ErrNotAuthentic) + assert.ErrorIs(t, v.Verify([]byte(`{"b":2,"a":1}`), header), ErrNotAuthentic) + assert.ErrorIs(t, v.Verify(append(original, '\n'), header), ErrNotAuthentic) +} From 4dabe2e80882bc2aebac0a22a737aec3955cc4b3 Mon Sep 17 00:00:00 2001 From: Eric Schoeller Date: Wed, 29 Jul 2026 22:52:52 -0600 Subject: [PATCH 3/6] SEPE-1177: Add the Naemon external command builder and FIFO writer Naemon reads its command file as newline-delimited records with semicolon-separated arguments, and neither delimiter can be escaped. The values interpolated into those records come from PagerDuty webhook payloads, and a valid signature on such a payload proves only that PagerDuty relayed it: anyone holding a routing key can create an incident carrying text of their choosing and have it relayed here, correctly signed. A newline in the wrong place turns an acknowledgement into arbitrary command submission, including CHANGE_SVC_CHECK_COMMAND with attacker-supplied arguments, which Naemon expands into a shell call. Command names are a closed type rather than strings, so a name cannot be built from input at all. Fields are sanitised individually, and then Render checks the fully assembled bytes: exactly one newline and it is last, no other control characters, valid UTF-8, within PIPE_BUF. That final check is the control that matters, because it does not depend on each field having been sanitised correctly and so still holds when someone adds a field and forgets. The author is namespaced with a constant prefix. Without it a PagerDuty user could set their display name to a colleague's and produce an acknowledgement in Thruk indistinguishable from one that colleague made. notify=0 on the acknowledge command is deliberate and load bearing. The outbound path already maps Naemon's ACKNOWLEDGEMENT notification type to a PagerDuty acknowledge event, so notifying here would send an acknowledge back to PagerDuty for an incident PagerDuty just told us about. Suppressing at source terminates that at depth zero rather than relying on PagerDuty declining to re-fire. The writer opens the pipe non-blocking. A blocking open of a reader-less FIFO never returns, and because it blocks inside open(2) the Go scheduler cannot preempt it -- each one holds an OS thread, so a request handler doing this with Naemon stopped accumulates threads until the runtime's limit kills the process. Writes are a single call within PIPE_BUF, which POSIX guarantees is atomic against other writers; Naemon, Thruk and merlin all write to this same pipe. EPIPE and ENXIO are deliberately distinguished. A dead descriptor is worth one reopen because the pipe may have been recreated with a live reader, while no-reader-right-now is not. Conflating them makes acknowledgements stop working silently after the first Naemon restart -- a test covers exactly that sequence. --- pkg/naemoncmd/acknowledge.go | 103 ++++++++++++ pkg/naemoncmd/command.go | 228 +++++++++++++++++++++++++++ pkg/naemoncmd/command_test.go | 288 ++++++++++++++++++++++++++++++++++ pkg/naemoncmd/writer.go | 172 ++++++++++++++++++++ pkg/naemoncmd/writer_test.go | 185 ++++++++++++++++++++++ 5 files changed, 976 insertions(+) create mode 100644 pkg/naemoncmd/acknowledge.go create mode 100644 pkg/naemoncmd/command.go create mode 100644 pkg/naemoncmd/command_test.go create mode 100644 pkg/naemoncmd/writer.go create mode 100644 pkg/naemoncmd/writer_test.go diff --git a/pkg/naemoncmd/acknowledge.go b/pkg/naemoncmd/acknowledge.go new file mode 100644 index 0000000..0285786 --- /dev/null +++ b/pkg/naemoncmd/acknowledge.go @@ -0,0 +1,103 @@ +package naemoncmd + +import ( + "time" + + "github.com/PagerDuty/go-pdagent/pkg/dedupkey" +) + +// Acknowledgement describes an acknowledgement arriving from PagerDuty. +type Acknowledgement struct { + // Object identifies the host or service, recovered from the alert's dedup + // key. Its fields have already been validated by dedupkey.Parse. + Object dedupkey.Key + + // Author is the PagerDuty user who acknowledged, unsanitised. + Author string + + // Comment is free text, unsanitised. Typically the incident title and URL. + Comment string + + At time.Time +} + +// Acknowledge flag values. +// +// sticky=2 keeps the acknowledgement in place when the object changes state +// within the same problem, so a service going WARNING to CRITICAL does not +// silently drop an acknowledgement someone is acting on. +// +// persistent=1 keeps the comment across a Naemon restart, so the audit trail +// survives a package upgrade. +// +// notify=0 is the one that matters, and the one most likely to be "corrected" +// by a future reader. Naemon's outbound path already maps its ACKNOWLEDGEMENT +// notification type to a PagerDuty acknowledge event. If this command asked +// Naemon to notify, acknowledging in PagerDuty would make Naemon send an +// acknowledge back to PagerDuty for an incident PagerDuty just told us was +// acknowledged. Setting it to zero suppresses the notification at source, so +// the loop terminates at depth zero rather than relying on PagerDuty declining +// to re-fire. +// +// The cost is real and worth stating: notify=0 suppresses the acknowledgement +// notification to every channel, not just PagerDuty. If Naemon should ever tell +// the email or Teams contacts that PagerDuty acknowledged something, this flag +// is too blunt an instrument and real suppression state would be needed. +const ( + ackSticky = "2" + ackNotify = "0" + ackPersistent = "1" +) + +// BuildAcknowledge renders the external command that acknowledges a problem. +// +// Merlin forwards acknowledgements to every peer rather than suppressing them +// by object ownership, unlike check results. So this command reaches its object +// whichever peer's command file it is written to, and the caller does not need +// to work out which peer owns what. +func BuildAcknowledge(a Acknowledgement) (Command, error) { + author := SanitiseAuthor(a.Author) + comment := SanitiseComment(a.Comment) + + if a.Object.IsHost { + return Command{ + Name: AcknowledgeHostProblem, + At: a.At, + Args: []string{ + a.Object.HostName, + ackSticky, ackNotify, ackPersistent, + author, comment, + }, + }, nil + } + + return Command{ + Name: AcknowledgeSvcProblem, + At: a.At, + Args: []string{ + a.Object.HostName, a.Object.ServiceDesc, + ackSticky, ackNotify, ackPersistent, + author, comment, + }, + }, nil +} + +// BuildRemoveAcknowledge renders the command that clears an acknowledgement, +// for when an incident is un-acknowledged in PagerDuty. +// +// The remove commands take only the object, so there is no author or comment to +// carry and no way to record who cleared it. Naemon will resume notifying. +func BuildRemoveAcknowledge(object dedupkey.Key, at time.Time) (Command, error) { + if object.IsHost { + return Command{ + Name: RemoveHostAcknowledgement, + At: at, + Args: []string{object.HostName}, + }, nil + } + return Command{ + Name: RemoveSvcAcknowledgement, + At: at, + Args: []string{object.HostName, object.ServiceDesc}, + }, nil +} diff --git a/pkg/naemoncmd/command.go b/pkg/naemoncmd/command.go new file mode 100644 index 0000000..eb7c0f2 --- /dev/null +++ b/pkg/naemoncmd/command.go @@ -0,0 +1,228 @@ +// Package naemoncmd builds Naemon external commands. +// +// Naemon reads its command file as newline-delimited records of the form +// +// [] COMMAND_NAME;arg;arg;... +// +// Both delimiters are structural and neither is escapable, so any untrusted +// value interpolated into a command is an injection vector. A newline ends the +// record and begins another, which lets an attacker submit a command of their +// choosing — including CHANGE_SVC_CHECK_COMMAND with attacker-supplied +// arguments, which Naemon expands into a shell invocation. A semicolon shifts +// every following field, which can retarget an acknowledgement or flip its +// flags. +// +// The values reaching this package come from PagerDuty webhook payloads. A +// valid HMAC signature on such a payload proves only that PagerDuty relayed it; +// anyone holding a routing key can create an incident carrying arbitrary text +// and have it relayed here, correctly signed. So these are attacker-controlled +// values from a trusted transport, and are treated accordingly. +// +// Every command is assembled through Command.Render, which applies a final +// check on the fully rendered bytes. That check is the control that matters: it +// does not depend on each individual field having been sanitised correctly, so +// it still holds if a future caller adds a field and forgets. +package naemoncmd + +import ( + "errors" + "fmt" + "strings" + "time" + "unicode/utf8" +) + +// MaxCommandBytes bounds a rendered command. +// +// PIPE_BUF on Linux is 4096, and a write of at most that size to a FIFO is +// guaranteed atomic against other writers. That guarantee is load bearing here: +// Naemon itself, Thruk and merlin all write to the same command file, and an +// interleaved write would splice our bytes into the middle of someone else's +// record. +// +// The limit also bounds a subtler attack. Naemon reads the command file through +// a fixed-size buffer, and an over-long line is split with the remainder parsed +// as a fresh record — which can smuggle a command past a filter that only looks +// for newline characters. +const MaxCommandBytes = 4096 + +// Field limits. Deliberately well inside MaxCommandBytes so no combination of +// maximum-length fields can approach it. +const ( + MaxAuthorLen = 64 + MaxCommentLen = 300 +) + +// AuthorPrefix namespaces the author of every command this package produces. +// +// Without it, a PagerDuty user who sets their display name to that of a +// colleague would produce an acknowledgement in Thruk indistinguishable from +// one that colleague made by hand. The prefix keeps the audit trail honest +// about where an acknowledgement came from. +const AuthorPrefix = "pagerduty:" + +var ( + // ErrUnsafeRender means the rendered command failed its final check. It + // indicates a bug in this package or a gap in field validation, and the + // command must not be written. + ErrUnsafeRender = errors.New("rendered command failed its safety check") + + // ErrTooLong means the rendered command exceeded MaxCommandBytes. + ErrTooLong = errors.New("rendered command is too long to write atomically") +) + +// Name is a Naemon external command name. +// +// It is a distinct type with a closed set of values so a command name can never +// be built from input: the only names that exist are the ones declared here. +type Name string + +const ( + AcknowledgeHostProblem Name = "ACKNOWLEDGE_HOST_PROBLEM" + AcknowledgeSvcProblem Name = "ACKNOWLEDGE_SVC_PROBLEM" + RemoveHostAcknowledgement Name = "REMOVE_HOST_ACKNOWLEDGEMENT" + RemoveSvcAcknowledgement Name = "REMOVE_SVC_ACKNOWLEDGEMENT" +) + +// Command is a rendered-but-not-yet-written external command. +type Command struct { + Name Name + Args []string + At time.Time +} + +// Render produces the bytes to write, including the trailing newline. +func (c Command) Render() ([]byte, error) { + if c.Name == "" { + return nil, errors.New("command name is empty") + } + + line := fmt.Sprintf("[%d] %s", c.At.Unix(), c.Name) + if len(c.Args) > 0 { + line += ";" + strings.Join(c.Args, ";") + } + line += "\n" + + if err := checkRendered(line); err != nil { + return nil, err + } + return []byte(line), nil +} + +// checkRendered validates the fully assembled command. +// +// This runs on the final bytes rather than on each field, so it cannot be +// bypassed by a caller who adds an argument and forgets to sanitise it. Every +// rule here is about what Naemon's parser does with the bytes, not about what +// the values mean. +func checkRendered(line string) error { + if len(line) > MaxCommandBytes { + return fmt.Errorf("%w: %d bytes, limit %d", ErrTooLong, len(line), MaxCommandBytes) + } + + if !strings.HasSuffix(line, "\n") { + return fmt.Errorf("%w: no trailing newline", ErrUnsafeRender) + } + + // Exactly one newline, and it is the last byte. Anything else means a second + // record was smuggled in. + if strings.Count(line, "\n") != 1 { + return fmt.Errorf("%w: embedded newline", ErrUnsafeRender) + } + + // No other control characters. A carriage return can terminate a record on + // some readers, and a NUL truncates the command mid-field because Naemon is + // written in C. + for i := 0; i < len(line)-1; i++ { + if b := line[i]; b < 0x20 || b == 0x7F { + return fmt.Errorf("%w: control byte 0x%02x at offset %d", ErrUnsafeRender, b, i) + } + } + + if !utf8.ValidString(line) { + return fmt.Errorf("%w: invalid UTF-8", ErrUnsafeRender) + } + + return nil +} + +// SanitiseAuthor renders an acknowledging user's name safe to interpolate, and +// namespaces it so it cannot be mistaken for a local operator. +// +// The author field is not the last argument of an acknowledge command, so an +// unescaped semicolon here would shift every field after it. +func SanitiseAuthor(name string) string { + cleaned := keepRunes(name, func(r rune) bool { + return r >= 'A' && r <= 'Z' || + r >= 'a' && r <= 'z' || + r >= '0' && r <= '9' || + r == ' ' || r == '.' || r == '_' || r == '-' || r == '@' + }) + cleaned = strings.TrimSpace(cleaned) + cleaned = truncateRunes(cleaned, MaxAuthorLen-len(AuthorPrefix)) + if cleaned == "" { + cleaned = "unknown" + } + return AuthorPrefix + cleaned +} + +// SanitiseComment renders free text safe to interpolate as the trailing +// argument of a command. +// +// Being last makes a stray semicolon harmless today, but that is a property of +// the current command set rather than of the field, so semicolons are stripped +// regardless. Truncation is by rune: cutting a multi-byte character in half +// would produce invalid UTF-8 and fail the render check. +// +// Disallowed characters become a space rather than being dropped. Dropping them +// would run words together — a two-line incident description would render as +// "line oneline two" — which is both harder to read and quietly misleading +// about what the original text said. +func SanitiseComment(text string) string { + var b strings.Builder + b.Grow(len(text)) + for _, r := range text { + switch { + case r == utf8.RuneError: + // Drop entirely: substituting a space would let malformed input + // pad the field towards the length limit. + case r >= 0x20 && r != 0x7F && r != ';': + b.WriteRune(r) + default: + b.WriteRune(' ') + } + } + cleaned := strings.Join(strings.Fields(b.String()), " ") + return truncateRunes(cleaned, MaxCommentLen) +} + +func keepRunes(s string, keep func(rune) bool) string { + var b strings.Builder + b.Grow(len(s)) + for _, r := range s { + if r == utf8.RuneError { + continue + } + if keep(r) { + b.WriteRune(r) + } + } + return b.String() +} + +func truncateRunes(s string, max int) string { + if max <= 0 { + return "" + } + if utf8.RuneCountInString(s) <= max { + return s + } + count := 0 + for i := range s { + if count == max { + return strings.TrimSpace(s[:i]) + } + count++ + } + return s +} diff --git a/pkg/naemoncmd/command_test.go b/pkg/naemoncmd/command_test.go new file mode 100644 index 0000000..f3129e8 --- /dev/null +++ b/pkg/naemoncmd/command_test.go @@ -0,0 +1,288 @@ +package naemoncmd + +import ( + "strings" + "testing" + "time" + + "github.com/PagerDuty/go-pdagent/pkg/dedupkey" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var testTime = time.Unix(1785300000, 0) + +func TestBuildAcknowledgeService(t *testing.T) { + cmd, err := BuildAcknowledge(Acknowledgement{ + Object: dedupkey.Key{HostName: "web01", ServiceDesc: "HTTP"}, + Author: "Eric Schoeller", + Comment: "Acknowledged in PagerDuty", + At: testTime, + }) + require.NoError(t, err) + + out, err := cmd.Render() + require.NoError(t, err) + assert.Equal(t, + "[1785300000] ACKNOWLEDGE_SVC_PROBLEM;web01;HTTP;2;0;1;"+ + "pagerduty:Eric Schoeller;Acknowledged in PagerDuty\n", + string(out)) +} + +func TestBuildAcknowledgeHost(t *testing.T) { + cmd, err := BuildAcknowledge(Acknowledgement{ + Object: dedupkey.Key{IsHost: true, HostName: "web01"}, + Author: "Eric Schoeller", + Comment: "ack", + At: testTime, + }) + require.NoError(t, err) + + out, err := cmd.Render() + require.NoError(t, err) + assert.Equal(t, + "[1785300000] ACKNOWLEDGE_HOST_PROBLEM;web01;2;0;1;pagerduty:Eric Schoeller;ack\n", + string(out)) +} + +// TestAcknowledgeSuppressesNotification pins notify=0. If this test fails +// because someone changed the flag, read the comment on ackNotify before +// "fixing" the test: notify=1 reintroduces an acknowledgement loop between +// Naemon and PagerDuty. +func TestAcknowledgeSuppressesNotification(t *testing.T) { + cmd, err := BuildAcknowledge(Acknowledgement{ + Object: dedupkey.Key{HostName: "web01", ServiceDesc: "HTTP"}, + Author: "someone", Comment: "c", At: testTime, + }) + require.NoError(t, err) + + // ACKNOWLEDGE_SVC_PROBLEM;host;svc;sticky;notify;persistent;author;comment + assert.Equal(t, "0", cmd.Args[3], "notify must be 0 to break the ack loop") + assert.Equal(t, "2", cmd.Args[2], "sticky") + assert.Equal(t, "1", cmd.Args[4], "persistent") +} + +// TestNewlineInjection is the attack that turns a webhook into arbitrary +// command submission. Every field an attacker can influence is tried. +func TestNewlineInjection(t *testing.T) { + payloads := []string{ + "x\n[1785300000] SHUTDOWN_PROGRAM", + "x\n[1785300000] DISABLE_NOTIFICATIONS", + "x\r[1785300000] SHUTDOWN_PROGRAM", + "x\r\n[1785300000] PROCESS_FILE;/tmp/evil;0", + "x\n[1785300000] CHANGE_SVC_CHECK_COMMAND;web01;HTTP;check_by_ssh!evil", + "\n", + "\r", + } + + for _, p := range payloads { + t.Run(strings.ReplaceAll(p[:min(len(p), 24)], "\n", "\\n"), func(t *testing.T) { + cmd, err := BuildAcknowledge(Acknowledgement{ + Object: dedupkey.Key{HostName: "web01", ServiceDesc: "HTTP"}, + Author: p, + Comment: p, + At: testTime, + }) + require.NoError(t, err) + + out, err := cmd.Render() + require.NoError(t, err, "sanitisation should make this renderable") + + s := string(out) + + // The property that matters is structural, not lexical. A command + // name appearing as inert text inside the comment field is + // harmless — free text is what that field is for, and Naemon shows + // it in Thruk rather than executing it. What must never happen is a + // second record, or a field boundary in a place we did not put one. + assert.Equal(t, 1, strings.Count(s, "\n"), "exactly one newline: %q", s) + assert.True(t, strings.HasSuffix(s, "\n")) + assert.NotContains(t, s, "\r") + + body := strings.TrimSuffix(s, "\n") + assert.Equal(t, 8, len(strings.Split(body, ";")), + "field count must be exactly name plus 7 args: %q", body) + assert.True(t, strings.HasPrefix(body, "[1785300000] ACKNOWLEDGE_SVC_PROBLEM;"), + "the record must still be the command we built: %q", body) + }) + } +} + +// TestSemicolonInjection: the author field is not last, so an unescaped +// semicolon there shifts every following argument. +func TestSemicolonInjection(t *testing.T) { + cmd, err := BuildAcknowledge(Acknowledgement{ + Object: dedupkey.Key{HostName: "web01", ServiceDesc: "HTTP"}, + Author: "evil;1;1;0;spoofed", + Comment: "also;evil", + At: testTime, + }) + require.NoError(t, err) + + out, err := cmd.Render() + require.NoError(t, err) + + // Field count must be exactly what the command expects: name plus 7 args. + // The attacker supplied four extra semicolons; if any survived, Naemon + // would read the tail of the author as sticky/notify/persistent flags. + body := strings.TrimSuffix(string(out), "\n") + fields := strings.Split(body, ";") + require.Equal(t, 8, len(fields), "extra semicolons would shift fields: %q", body) + + // The flags must still be the ones we set, not values shifted in from the + // author field. Fields are: [ts] NAME, host, svc, sticky, notify, + // persistent, author, comment. + assert.Equal(t, "2", fields[3], "sticky") + assert.Equal(t, "0", fields[4], "notify") + assert.Equal(t, "1", fields[5], "persistent") + assert.Equal(t, "pagerduty:evil110spoofed", fields[6], + "the whole hostile author must collapse into one field") +} + +func TestControlCharactersStripped(t *testing.T) { + cmd, err := BuildAcknowledge(Acknowledgement{ + Object: dedupkey.Key{HostName: "web01", ServiceDesc: "HTTP"}, + Author: "a\x00b\x07c", + Comment: "d\te\x1bf\x7fg", + At: testTime, + }) + require.NoError(t, err) + + out, err := cmd.Render() + require.NoError(t, err) + + for i, b := range out[:len(out)-1] { + require.Falsef(t, b < 0x20 || b == 0x7F, + "control byte 0x%02x survived at offset %d", b, i) + } +} + +// TestOverLongInputTruncated guards the buffer-split smuggling route: an +// over-long record is split by Naemon and its remainder parsed as a new +// command, which can carry an injected command past a newline filter. +func TestOverLongInputTruncated(t *testing.T) { + cmd, err := BuildAcknowledge(Acknowledgement{ + Object: dedupkey.Key{HostName: "web01", ServiceDesc: "HTTP"}, + Author: strings.Repeat("A", 5000), + Comment: strings.Repeat("B", 5000), + At: testTime, + }) + require.NoError(t, err) + + out, err := cmd.Render() + require.NoError(t, err) + assert.LessOrEqual(t, len(out), MaxCommandBytes) +} + +// TestRenderRejectsUnsafeArgs proves the final check is a real backstop and not +// merely a restatement of the field sanitisers: a Command assembled directly, +// bypassing BuildAcknowledge, must still be refused. +func TestRenderRejectsUnsafeArgs(t *testing.T) { + cases := map[string][]string{ + "embedded newline": {"web01", "HTTP\n[1] SHUTDOWN_PROGRAM"}, + "carriage return": {"web01", "HTTP\rmore"}, + "nul byte": {"web01", "HTTP\x00"}, + "escape": {"web01", "HTTP\x1b[0m"}, + "over-long comment": {"web01", strings.Repeat("x", MaxCommandBytes+1)}, + } + + for name, args := range cases { + t.Run(name, func(t *testing.T) { + _, err := Command{Name: AcknowledgeSvcProblem, Args: args, At: testTime}.Render() + assert.Error(t, err, "the render check must catch what field sanitisation missed") + }) + } +} + +func TestRenderRejectsEmptyName(t *testing.T) { + _, err := Command{Args: []string{"web01"}, At: testTime}.Render() + assert.Error(t, err) +} + +func TestSanitiseAuthor(t *testing.T) { + cases := map[string]string{ + "Eric Schoeller": "pagerduty:Eric Schoeller", + "eric@colorado.edu": "pagerduty:eric@colorado.edu", + "": "pagerduty:unknown", + "\n\n": "pagerduty:unknown", + "