SEPE-1165: Fix defects inherited from the upstream beta - #11
Merged
Conversation
Six defects, all present before this fork and all in the path that delivers pages. pkg/server, pkg/common and cmd are byte-identical to upstream, so these came with the beta rather than from our changes. The systemd unit had no Restart=. RestartSec was set, which reads as though restarts were configured, but it is inert on its own and the default is Restart=no -- so any crash left the agent dead until a human noticed by other means. StartLimit* go in [Unit], because [Service] is silently ignored on systemd 239 as shipped by RHEL 8. The auth middleware allowed every request through when no secret was configured. That state cannot arise by accident, since the defaults generate a key and `pdagent init` writes it, so reaching it means someone set it empty deliberately -- and serving an unauthenticated enqueue endpoint is not a reasonable reading of that. It now refuses and logs why. The token comparison was also byte-by-byte, which leaks a shared secret to anyone who can time the response, and the presented credential was written to a world-readable log on every failure. GenerateKey drew from math/rand seeded with the wall-clock nanosecond at process start. The 36^32 keyspace is irrelevant when the real entropy is one timestamp that can be narrowed from a file mtime. send_handler indexed req.Header["Pd-Event-Version"][0], which panics on a missing header, and logged the full request body at debug -- including the routing key, into a log created world-readable. Config permissions needed care rather than a chmod. viper writes 0644 and the file holds the daemon's auth secret, but the CLI runs as the monitoring user and reaches it only through that world-read bit. Removing it naively would have left the CLI unable to enqueue, which stops paging silently -- the exact failure mode being fixed elsewhere in this commit. Group ownership now carries the access where a monitoring user exists, with a fallback to owner-only where one does not. Verified on RHEL 8: the file ends up 0640 pdagent:naemon, the CLI still reports healthy, and world access is gone. Residual worth stating: apache is a member of the naemon group on these hosts, so it can still read the file -- acceptable given it already holds command-pipe write access, which is strictly more power. pkg/server had no tests. These are the first, covering the two behaviour changes above.
There was a problem hiding this comment.
Pull request overview
This PR hardens the PagerDuty agent’s “page delivery” path by addressing several inherited reliability and security defects across packaging, systemd, server middleware, and key generation.
Changes:
- Improve service reliability via systemd start-limit placement and adding
Restart=on-failure. - Harden the
/sendrequest path by preventing credential/body logging, avoiding header-index panics, refusing empty shared secrets, and using constant-time token comparison. - Secure secret generation and reduce config-file secret exposure via postinstall permission tightening and new server middleware tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/rpm/postinstall.sh | Adjust config ownership/permissions to reduce secret exposure while keeping CLI access. |
| scripts/deb/postinstall.sh | Same as RPM postinstall, for Debian packaging. |
| pkg/server/send_handler.go | Stop logging request bodies; safer header access for event version. |
| pkg/server/middleware.go | Refuse empty secrets; constant-time auth compare; avoid logging bearer credentials. |
| pkg/server/middleware_test.go | Add tests for the new auth behavior and logging constraints. |
| pkg/common/helpers.go | Switch key generation from math/rand to CSPRNG-backed randomness. |
| init/pdagent.service | Add restart policy and correctly place StartLimit* in [Unit]. |
| cmd/init.go | Tighten config file permissions immediately after viper writes it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Four findings, all real, and one of them a regression this branch introduced. Tightening the config to 0600 in `pdagent init` without reassigning ownership would have left a root-created file the daemon could not read. It would then have fallen back to a freshly generated secret, because the config read error is discarded, and the CLI would have failed to authenticate with nothing indicating why. Ownership now moves with the mode, mirroring the postinstall so both paths converge. Proving that also turned up a second problem in the same function. The config directory was created 0744, which grants read without traverse -- no access at all for anyone but the owner. It never bit because the package ships /etc/pdagent at 0755 and MkdirAll is a no-op when the directory exists, so only a config created outside the package was affected. Now 0755, matching what is shipped. The postinstall comment claimed an owner-only fallback where no monitoring group exists, but the code applied 0640 unconditionally, leaving group read for the pdagent group. The fallback is now real. An unrecognised Pd-Event-Version mapped to the zero value and failed downstream as a 500, reporting a server fault for a malformed request and telling the caller nothing. Rejected at the handler with a 400 naming the accepted values. Verified on RHEL 8: config lands 0640 pdagent:naemon in a 0755 directory, the daemon and the monitoring CLI can both read it, and world access is gone.
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.
Six defects surfaced by four independent analyses during the SEPE-1177 design work. All predate this fork —
pkg/server,pkg/commonandcmdare byte-identical to upstream — and all sit in the path that delivers pages.Restart=in the unitRestartSecwas set, which reads as though restarts were configured, but it is inert alone.math/randseeded from the wall clock for the auth secretreq.Header["Pd-Event-Version"][0]The one that needed care
Config permissions were not a
chmod. viper writes0644, and the file holds the daemon's auth secret — but the CLI runs as the monitoring user and reaches it only through that world-read bit. Removing it naively would leave the CLI unable to enqueue, which stops paging silently — the exact failure mode this PR fixes elsewhere.Group ownership now carries the access where a monitoring user exists, falling back to owner-only where one does not. Verified on RHEL 8:
Residual, stated deliberately:
apacheis in thenaemongroup on these hosts, so it can still read the file. Acceptable — it already holds write access to Naemon's command pipe, which is strictly more power than a routing key.Notes
StartLimit*are in[Unit]. In[Service]they are silently ignored on systemd 239 as shipped by RHEL 8, leaving a unit that looks rate-limited and is not.pdagent initwrites it. It now says why, since the symptom is otherwise indistinguishable from a wrong token.pkg/serverhad no tests. These are the first, covering the two behaviour changes.pkg/persistentqueue/enqueue.goand the unbounded shutdown drain inpkg/eventqueue. Both are real and documented, but they change delivery semantics and cannot be validated end to end without PagerDuty access, which has lapsed. Fixing the paging queue without being able to run a real delivery is the wrong trade.