SEPE-1177: Add pdagent-receiver, the inbound webhook half - #10
Merged
Conversation
added 5 commits
July 29, 2026 20:27
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.
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.
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.
Completes the return path. An acknowledgement in PagerDuty now becomes an ACKNOWLEDGE_SVC_PROBLEM or ACKNOWLEDGE_HOST_PROBLEM in Naemon's command file, and un-acknowledging removes it. The dedup key needs a REST lookup, which was not the expected design. A V3 incident.acknowledged payload carries the incident, and its incident_key field is null; the key that identifies the Naemon object belongs to the incident's alert. That was settled by capturing a real delivery rather than reading documentation, because PagerDuty's docs are a JavaScript application that could not be retrieved, and no captured payload was findable. Subscribing to an alert-level event instead is not an option: incident.alert.triggered and its siblings are rejected by the subscription API. The captured payload is committed alongside the project documentation, and the parser reads incident_key first so the lookup is skipped if PagerDuty ever populates it. That lookup costs the receiver an API token and outbound network access, which weakens the isolation argument for a separate process without reversing it. The token should be read-only -- the receiver never writes to PagerDuty -- and egress can be restricted to the API by the same uid-matched nftables rule the separate user already exists for. A compromise then yields one FIFO write plus read-only API access, against the routing keys in the agent's memory, which can silently resolve live incidents. Resolve events are subscribed and logged but deliberately not acted on. A Naemon problem clears when its check recovers; letting PagerDuty force that would let the two disagree about whether the fault is actually fixed. Failures answer 503 rather than 4xx, including a dedup key that fails validation. PagerDuty retries a 5xx with backoff, which is a better place for that wait than a queue this process would have to build and persist, and a rejected key is worth surfacing through PagerDuty's own delivery-failure reporting rather than accepting quietly. Verified end to end against the compiled binary with a real FIFO, real HMACs and a stubbed API: acknowledge and un-acknowledge produce well-formed records, resolve produces none, an authentic delivery whose alert key carries a newline injection is refused with nothing written, and a hostile author collapses into a single field rather than shifting the acknowledgement's flags.
One release tag ships one package containing both binaries and both units. They share the dedup key format -- the agent writes it, the receiver parses it back -- so letting them reach a host at different versions would break acknowledgements in a way neither side could detect. Shipping them together makes that impossible rather than unlikely. The receiver's unit is staged under /var/lib/pdagent/scripts and deliberately not enabled. It cannot start without a webhook signing secret, which only configuration management can place, and a failed start inside a `set -e` postinstall scriptlet would abort before the agent is started -- taking down outbound paging to fix nothing. This repo has already shipped that exact failure once, for a different reason, and the comment explaining it is still in the scriptlet. Enabling the receiver is the deploying system's job, which also gives a dark launch: install everywhere, enable on one host. The package creates the pdagent-receiver account and its config directory at 0750. A separate account is the point of a separate process: the controls that keep an internet-facing listener away from the agent's routing keys -- a targeted ACL on Naemon's command pipe, an nftables egress rule matched on uid, the unit's sandbox -- are all per-user. The archive is now scoped to the agent. The receiver is Linux-only, so including it would give the tarball two binaries on Linux and one on macOS, which goreleaser rejects as confusing. The receiver builds for the same Linux architectures as the agent, including i386. Nobody runs a monitoring host on 32-bit, but a package that claims to be this product and silently lacks one of its two binaries on one architecture is the kind of inconsistency found at install time. The CI layout check caught exactly that while this was being written. That check now asserts both binaries against the ExecStart of their own unit, and that the receiver unit is staged rather than installed. It exists because a bindir default once shipped a package that installed cleanly and then failed to start.
There was a problem hiding this comment.
Pull request overview
Adds the inbound half of the PagerDuty↔Naemon integration: a dedicated pdagent-receiver webhook service that verifies PagerDuty V3 webhook signatures, resolves incident→alert dedup keys via the PagerDuty REST API, and writes safe Naemon external-command acknowledgements to the command FIFO. This complements the existing outbound-only agent while keeping the internet-facing listener isolated by user/unit boundaries.
Changes:
- Introduce
pdagent-receiverbinary, systemd unit, and packaging scriptlets (RPM/DEB) to ship the receiver staged-but-not-enabled. - Implement inbound receiver components: signature verification, secret loading, REST alert lookup for dedup key recovery, webhook handler, and acknowledgement/unacknowledgement mapping to Naemon commands.
- Add hardened Naemon command rendering/writing (FIFO writer, injection-resistant rendering), plus a shared
dedupkeypackage used by both outbound and inbound paths.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/rpm/preremove.sh | Stop/disable receiver unit during RPM removal and clean up the installed unit file. |
| scripts/rpm/postinstall.sh | Create receiver system user and set /etc/pdagent-receiver permissions; stage receiver unit without enabling/starting it. |
| scripts/deb/preremove.sh | Stop/disable receiver unit during DEB removal and clean up the installed unit file. |
| scripts/deb/postinstall.sh | Create receiver system user/group and set /etc/pdagent-receiver permissions; stage receiver unit without enabling/starting it. |
| pkg/receiver/signature.go | HMAC-SHA256 signature parsing/verification for PagerDuty V3 webhooks with candidate caps + secret rotation support. |
| pkg/receiver/signature_test.go | Tests for signature verification correctness, rotation behavior, and candidate cap. |
| pkg/receiver/secret.go | Secret file loader with strict permission checks and comment/blank-line handling. |
| pkg/receiver/secret_test.go | Tests for secret loading behavior and permission refusal/acceptance. |
| pkg/receiver/pdapi.go | PagerDuty REST client for incident→alerts lookup to recover alert_key dedup key. |
| pkg/receiver/handler.go | HTTP handler that caps body size, verifies signature on raw bytes, supports capture mode, and applies incident actions. |
| pkg/receiver/handler_test.go | Tests for method enforcement, body size cap, capture logging, and signature rejection behavior. |
| pkg/receiver/acknowledge.go | Incident event decoding and mapping to Naemon ACK / REMOVE_ACK commands with key lookup fallback. |
| pkg/receiver/acknowledge_test.go | End-to-end tests from real-ish payloads through rendered Naemon commands, including hostile inputs. |
| pkg/naemoncmd/writer.go | Non-blocking FIFO writer with stale-descriptor handling and atomic write guarantees. |
| pkg/naemoncmd/writer_test.go | FIFO writer behavior tests: non-blocking open, restart/recreate recovery, concurrency safety. |
| pkg/naemoncmd/command.go | Centralized safe rendering + final structural validation (newline/control char/UTF-8/PIPE_BUF bounds). |
| pkg/naemoncmd/command_test.go | Injection, truncation, and invariants tests for render and field sanitizers. |
| pkg/naemoncmd/acknowledge.go | Build ACK/REMOVE_ACK commands with load-bearing notify=0 behavior and sanitization of author/comment. |
| pkg/dedupkey/dedupkey.go | Shared dedup key format builder/parser with strict rejection for inbound safety. |
| pkg/dedupkey/dedupkey_test.go | Tests pinning legacy format and strict parsing/rejection behavior. |
| init/pdagent-receiver.service | Hardened systemd unit for receiver (dedicated user, sandboxing, resource limits). |
| cmd/receiver/main.go | New pdagent-receiver CLI/service entrypoint wiring secrets, lookup, FIFO writer, and HTTP server. |
| cmd/integrations/nagios/nagios_enqueue.go | Switch outbound dedup key construction to the shared dedupkey package. |
| .goreleaser.yml | Add receiver build, stage receiver unit in packages, and create /etc/pdagent-receiver directory in nfpms outputs. |
| .github/workflows/build.yml | Validate packaged artifacts include receiver binary and staged unit, and ensure the unit is not installed directly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two findings, both real. The `systemctl list-unit-files` guard in both preremove scripts never skipped anything. On systemd 239 as shipped by RHEL 8 -- the actual target -- it returns 0 whether or not the named unit exists, so the stop and disable ran on every host including those that never had the receiver, logging errors for a unit nobody installed. Worth noting the same test does behave as expected on newer systemd, which is how it passed a local check; the version that matters is the one in the RPM's target, not the one on the workstation. Now guarded on the unit file itself, which is unambiguous on both. The API token was read with LoadSecrets and used as token[0]. Signing secrets are plural by design, because PagerDuty signs with every active secret during a rotation, but an API token is not -- so a stray line or a half-finished rotation would silently authenticate with whichever token sorted first, and the symptom would be an authorization failure with nothing pointing at the file. LoadSingleSecret requires exactly one and says so when it does not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Acknowledging a PagerDuty incident now acknowledges the corresponding problem in Naemon. This is the return path; the agent has only ever sent events outward.
Why this is written rather than adopted
PagerDuty documents a two-way Nagios integration and has since around 2015. The script their own guide still tells you to
wget—mdcollins05/pd-nag-connector— has been archived since 2016, parses webhooks v1 (a format PagerDuty retired), and has no authentication of any kind. The one maintained alternative is PHP, needs a runtime our Naemon hosts do not have, and its signature check is wrong three separate ways, including one that lets the raw secret authenticate. Full comparison is on SEPE-1167.Shape
A separate binary, systemd unit and system user, shipped in the same RPM as the agent.
The separation is not stylistic. This process listens to the public internet and can write Naemon's command pipe, while the agent holds PagerDuty routing keys — a compromise of either should not reach the other, and every control that enforces that is per-user: a targeted ACL, an nftables egress rule matched on uid, the unit's sandbox. On RHEL 8 it is not even a preference: cgroups v1 is the default,
IPAddressDenyis silently ineffective under it, and a uid-matched nftables rule is the only egress control available.Four independent analyses informed this; three recommended separation and the designated advocate for a single binary conceded the privilege argument could not be closed.
Security
A valid HMAC proves PagerDuty relayed the message. It proves nothing about the contents — anyone holding a routing key can create an incident carrying text of their choosing and have it relayed here, correctly signed. Naemon's command file is newline-delimited with semicolon-separated arguments and neither delimiter can be escaped, so a naive writer turns that into arbitrary command submission,
CHANGE_SVC_CHECK_COMMANDincluded.Rendervalidates the fully assembled bytes — one trailing newline, no other control characters, valid UTF-8, withinPIPE_BUF. That check does not depend on each field having been sanitised correctly, so it still holds when someone adds a field and forgetsSignature verification reads the raw body before any decode, size-capped, constant-time, accepting any of several
v1=values so a secret can be rotated without dropping deliveries.The FIFO
Opens 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 withinPIPE_BUF, which POSIX guarantees is atomic against other writers; Naemon, Thruk and merlin all write to this same pipe.EPIPEandENXIOare deliberately distinguished. A dead descriptor is worth one reopen because the pipe may have been recreated with a live reader; no-reader-right-now is not. Conflating them makes acknowledgements stop working silently after the first Naemon restart — a test covers that exact sequence.Verified
incident_keyis null in a real V3 incident payload, so the dedup key is recovered by a REST lookup of the incident's alert. No alert-level webhook event exists to subscribe to instead — those types are rejected by the subscription APIsystemd-analyze securityreports 1.4 on the real targetNotes for review
set -epostinstall would abort before the paging daemon starts — this repo has already shipped that failure once for a different reasonnotify=0on the acknowledge command is load bearing. The outbound path maps Naemon'sACKNOWLEDGEMENTtype to a PagerDuty acknowledge event, so notifying here would send an acknowledge back for an incident PagerDuty just told us aboutgofmtdiffs onmainare untouched; they are Go 1.19 doc-comment reformatting of upstream Apache headers and would bury this diff