SEPE-1165: Fix defects inherited from the upstream beta

cmd/init.go

@@ -5,7 +5,7 @@ Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
 You may obtain a copy of the License at
 
-    http://www.apache.org/licenses/LICENSE-2.0
+	http://www.apache.org/licenses/LICENSE-2.0
 
 Unless required by applicable law or agreed to in writing, software
 distributed under the License is distributed on an "AS IS" BASIS,
@@ -18,7 +18,9 @@ package cmd
 import (
 	"fmt"
 	"os"
+	"os/user"
 	"path"
+	"strconv"
 
 	"github.com/PagerDuty/go-pdagent/pkg/cmdutil"
 	"github.com/PagerDuty/go-pdagent/pkg/common"
@@ -45,7 +47,12 @@ func NewInitCmd() *cobra.Command {
 				fmt.Printf("Generating config to %v\n", configFile)
 			}
 
-			if err := os.MkdirAll(path.Dir(configFile), 0744); err != nil {
+			// 0755, matching what the package ships for /etc/pdagent. At 0744 a
+			// directory grants read without traverse, which is no access at all
+			// for anyone but the owner -- so a config created outside the
+			// package would be unreachable by the daemon and the CLI even with
+			// the file's own mode set correctly.
+			if err := os.MkdirAll(path.Dir(configFile), 0755); err != nil {
 				fmt.Println(err)
 				os.Exit(1)
 			}
@@ -57,9 +64,70 @@ func NewInitCmd() *cobra.Command {
 				os.Exit(1)
 			}
 
+			// viper writes 0644. This file holds the daemon's auth secret and,
+			// for anyone who puts one here, a PagerDuty routing key -- which
+			// permits resolving arbitrary incidents on its service. Neither
+			// belongs in a file every local process can read.
+			//
+			// Ownership has to move with the mode. Run as root, which is how
+			// the package scriptlets and most manual invocations call it, the
+			// file lands root-owned; tightening the mode without reassigning it
+			// would leave the daemon unable to read its own config. It would
+			// then fall back to a freshly generated secret, because the config
+			// read error is discarded, and the CLI would fail to authenticate
+			// with nothing indicating why. Mirrors the postinstall logic so
+			// both paths converge on the same end state.
+			if err := secureConfig(configFile); err != nil {
+				fmt.Printf("Error securing config: %v\n", err)
+				os.Exit(1)
+			}
+
 			fmt.Printf("Config file generated to %v\n", configFile)
 		},
 	}
 
 	return initCmd
 }
+
+// secureConfig removes world access from the generated config, and reassigns
+// ownership so the daemon and the monitoring CLI can both still read it.
+//
+// The mode depends on whether a monitoring group exists: the CLI runs as that
+// user and needs read access, but where there is no such group there is nobody
+// to grant it to and the file stays owner-only.
+func secureConfig(path string) error {
+	owner, err := user.Lookup("pdagent")
+	if err != nil {
+		// No pdagent account, so this is not a packaged install and there is
+		// no separate daemon identity to accommodate. Owner-only is correct.
+		return os.Chmod(path, 0600)
+	}
+	uid, err := strconv.Atoi(owner.Uid)
+	if err != nil {
+		return err
+	}
+
+	mode := os.FileMode(0600)
+	gid := -1
+	for _, name := range []string{"naemon", "nagios"} {
+		g, err := user.LookupGroup(name)
+		if err != nil {
+			continue
+		}
+		if parsed, err := strconv.Atoi(g.Gid); err == nil {
+			gid, mode = parsed, 0640
+		}
+		break
+	}
+	if gid == -1 {
+		if parsed, err := strconv.Atoi(owner.Gid); err == nil {
+			gid = parsed
+		}
+	}
+
+	// Chown fails for an unprivileged caller, which is fine -- such a caller
+	// could not have written into /etc either, so this is a developer running
+	// against a local config. The mode still applies.
+	_ = os.Chown(path, uid, gid)
+	return os.Chmod(path, mode)
+}

init/pdagent.service

@@ -2,13 +2,25 @@
 Description=PagerDuty Agent
 After=network.target
 
+# StartLimit* 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=300
+StartLimitBurst=5
+
 [Service]
 Type=simple
 Environment=APP_ENV=production
 ExecStart=/usr/local/bin/pdagent server
 ExecStop=/usr/local/bin/pdagent server stop
 KillMode=process
 TimeoutStopSec=30
+
+# Without Restart= the RestartSec below is inert -- the default is
+# Restart=no, so the agent stayed dead until a human intervened. This is the
+# process that delivers pages, so any crash silently ended paging until
+# someone noticed by other means.
+Restart=on-failure
 RestartSec=15
 User=pdagent
 Group=pdagent

pkg/common/helpers.go

@@ -1,21 +1,20 @@
 package common
 
 import (
-	"math/rand"
-	"time"
+	"crypto/rand"
+	"math/big"
 )
 
 var rkChars = []byte("abcdefghijklmnopqrstuvwxyz0123456789")
-var r *rand.Rand
-
-func init() {
-	r = rand.New(rand.NewSource(time.Now().UnixNano()))
-}
 
 // GenerateKey generates a random lowercase alphanumeric key.
 //
-// These keys approximate the Events API's routing keys for use in testing,
-// but may be useful more generally.
+// The doc comment used to describe these as approximating routing keys "for
+// use in testing", but the value is also what the daemon and its CLI use to
+// authenticate to each other, so it has to be unguessable. It was previously
+// drawn from math/rand seeded with the wall-clock nanosecond at process start:
+// the 36^32 keyspace is irrelevant when the real entropy is a single timestamp
+// that can be narrowed to a few thousand candidates from a file mtime.
 func GenerateKey() string {
 	rk := make([]byte, 32)
 	for i := range rk {
@@ -25,5 +24,14 @@ func GenerateKey() string {
 }
 
 func randChar() byte {
-	return rkChars[r.Intn(len(rkChars))]
+	// crypto/rand cannot fail in a way a caller could sensibly handle, and a
+	// host without a working CSPRNG is not one this daemon should keep running
+	// on. Panicking is better than the alternative, which is silently falling
+	// back to a predictable key -- exactly the outcome this function exists to
+	// prevent.
+	n, err := rand.Int(rand.Reader, big.NewInt(int64(len(rkChars))))
+	if err != nil {
+		panic("common: no source of secure randomness available: " + err.Error())
+	}
+	return rkChars[n.Int64()]
 }

pkg/server/middleware.go

@@ -1,15 +1,17 @@
 package server
 
 import (
+	"crypto/hmac"
 	"fmt"
-	"go.uber.org/zap"
 	"net/http"
+
+	"go.uber.org/zap"
 )
 
 func loggingMiddleware(logger *zap.SugaredLogger) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			logger.Infof("Handling request: %v", r.RequestURI)
+			logger.Info(r.RequestURI)
 			next.ServeHTTP(w, r)
 		})
 	}
@@ -20,14 +22,30 @@ func authMiddleware(s *Server) func(http.Handler) http.Handler {
 
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			// An empty secret used to allow every request through. That state
+			// cannot arise by accident -- the defaults generate a key and
+			// `pdagent init` writes it -- so reaching it means someone set
+			// secret to the empty string deliberately, and serving an
+			// unauthenticated enqueue endpoint is not a reasonable reading of
+			// that. Refuse instead, and say why: the alternative is a daemon
+			// that looks healthy while accepting events from any local process.
 			if s.secret == "" {
-				next.ServeHTTP(w, r)
+				s.logger.Error("refusing request: no secret is configured; " +
+					"set `secret` in /etc/pdagent/config.yaml or run `pdagent init`")
+				errorResp(w, 500, []string{"Server is misconfigured: no secret is set."})
 				return
 			}
 
+			// Constant-time. A byte-by-byte comparison leaks the shared secret
+			// to anyone who can time the response, and this endpoint accepts
+			// events that page people.
 			clientHeader := r.Header.Get("Authorization")
-			if clientHeader != serverHeader {
-				s.logger.Infof("Authorization failure with client auth header: %v", clientHeader)
+			if !hmac.Equal([]byte(clientHeader), []byte(serverHeader)) {
+				// The header value is deliberately not logged. It is a bearer
+				// credential, and a near-miss written to a world-readable log
+				// is a credential leak in its own right.
+				s.logger.Infow("authorization failure",
+					"remote", r.RemoteAddr, "path", r.URL.Path)
 				errorResp(w, 401, []string{"Unauthorized, expected matching secret token in Authorization header."})
 				return
 			}

pkg/server/middleware_test.go

@@ -0,0 +1,98 @@
+package server
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zaptest/observer"
+)
+
+// newAuthTest builds the auth middleware around a handler that records whether
+// it was reached, which is the only thing that matters: a request that gets
+// through has been authorised.
+func newAuthTest(t *testing.T, secret string) (http.Handler, *bool, *observer.ObservedLogs) {
+	t.Helper()
+	core, logs := observer.New(zap.DebugLevel)
+	s := &Server{secret: secret, logger: zap.New(core).Sugar()}
+
+	reached := false
+	inner := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true })
+	return authMiddleware(s)(inner), &reached, logs
+}
+
+func do(h http.Handler, authHeader string) *httptest.ResponseRecorder {
+	req := httptest.NewRequest(http.MethodPost, "/send", nil)
+	if authHeader != "" {
+		req.Header.Set("Authorization", authHeader)
+	}
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, req)
+	return rec
+}
+
+func TestAuthAcceptsMatchingToken(t *testing.T) {
+	h, reached, _ := newAuthTest(t, "s3cret")
+	rec := do(h, "token s3cret")
+
+	assert.Equal(t, http.StatusOK, rec.Code)
+	assert.True(t, *reached)
+}
+
+func TestAuthRejects(t *testing.T) {
+	cases := map[string]string{
+		"wrong token":     "token wrong",
+		"missing header":  "",
+		"no token prefix": "s3cret",
+		"empty token":     "token ",
+		"prefix only":     "token",
+		"bearer scheme":   "Bearer s3cret",
+	}
+
+	for name, header := range cases {
+		t.Run(name, func(t *testing.T) {
+			h, reached, _ := newAuthTest(t, "s3cret")
+			rec := do(h, header)
+
+			assert.Equal(t, http.StatusUnauthorized, rec.Code)
+			assert.False(t, *reached, "the handler must not be reached")
+		})
+	}
+}
+
+// TestAuthRefusesWhenNoSecretConfigured covers the behaviour change. An empty
+// secret used to allow every request through, which turned a misconfiguration
+// into an unauthenticated enqueue endpoint -- and enqueued events page people.
+func TestAuthRefusesWhenNoSecretConfigured(t *testing.T) {
+	h, reached, logs := newAuthTest(t, "")
+
+	for _, header := range []string{"", "token ", "token anything"} {
+		rec := do(h, header)
+		assert.Equal(t, http.StatusInternalServerError, rec.Code,
+			"an unconfigured server must refuse, not serve")
+		assert.False(t, *reached)
+	}
+
+	// And it must say why, since the symptom is otherwise indistinguishable
+	// from a wrong token.
+	assert.NotZero(t, logs.FilterMessageSnippet("no secret is configured").Len())
+}
+
+// TestAuthDoesNotLogCredentials: the header is a bearer credential and the log
+// is world-readable, so a near-miss written there is a leak in its own right.
+func TestAuthDoesNotLogCredentials(t *testing.T) {
+	h, _, logs := newAuthTest(t, "s3cret")
+	do(h, "token almost-the-real-secret")
+
+	for _, e := range logs.All() {
+		assert.NotContains(t, e.Message, "almost-the-real-secret")
+		for _, v := range e.ContextMap() {
+			if s, ok := v.(string); ok {
+				assert.NotContains(t, s, "almost-the-real-secret",
+					"the presented credential must not reach the log")
+			}
+		}
+	}
+}

pkg/server/send_handler.go

@@ -1,6 +1,7 @@
 package server
 
 import (
+	"fmt"
 	"io/ioutil"
 	"net/http"
 
@@ -14,10 +15,30 @@ func (s *Server) SendHandler(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	s.logger.Debugf("/send payload: %v", string(body))
+	// The body is deliberately not logged. It carries the PagerDuty routing
+	// key, and the log file is created world-readable, so debug logging would
+	// have published a credential that can resolve arbitrary incidents.
+	s.logger.Debugf("/send received %d bytes", len(body))
+
+	// Header.Get rather than indexing: a missing Pd-Event-Version yielded a nil
+	// slice and [0] panicked. net/http recovers that per connection so the
+	// process survived, but it was a trivially reachable fault on every request
+	// that omitted the header.
+	//
+	// Validated here rather than left to the map's zero value, which would send
+	// an unrecognised version downstream to fail as a 500 -- reporting a server
+	// fault for what is a malformed request, and telling the caller nothing
+	// about how to fix it.
+	rawVersion := req.Header.Get("Pd-Event-Version")
+	eventVersion, ok := eventsapi.StringToEventVersion[rawVersion]
+	if !ok {
+		errorResp(rw, 400, []string{fmt.Sprintf(
+			"Unrecognized Pd-Event-Version %q; expected \"v1\" or \"v2\".", rawVersion)})
+		return
+	}
 
 	eventContainer := eventsapi.EventContainer{
-		EventVersion: eventsapi.StringToEventVersion[req.Header["Pd-Event-Version"][0]],
+		EventVersion: eventVersion,
 		EventData:    body,
 	}
 

scripts/deb/postinstall.sh

@@ -56,6 +56,28 @@ if [ "$1" = "configure" ]; then
     # RuntimeDirectory= in pdagent.service, with the correct ownership.
     chown -R pdagent:pdagent /etc/pdagent /var/db/pdagent /var/lib/pdagent /var/log/pdagent
 
+    # viper writes config.yaml 0644 and the chown above does not change that, so
+    # the daemon's auth secret -- and any routing key placed here -- were readable
+    # by every local user, including every monitoring plugin and CGI on the box.
+    #
+    # Removing world-read cannot be done naively. The CLI runs as the monitoring
+    # user, not as pdagent, and reads this file to authenticate to the daemon; it
+    # currently reaches it only through that world-read bit. Taking it away without
+    # granting the monitoring user access another way would leave the CLI unable to
+    # enqueue, which stops paging silently.
+    #
+    # So group ownership carries the access where a monitoring user exists, and the
+    # file falls back to owner-only where one does not.
+    _cfg_mode=0600
+    for _grp in naemon nagios; do
+        if /usr/bin/getent group "$_grp" >/dev/null 2>&1; then
+            chown "pdagent:$_grp" /etc/pdagent/config.yaml 2>/dev/null || :
+            _cfg_mode=0640
+            break
+        fi
+    done
+    chmod "$_cfg_mode" /etc/pdagent/config.yaml 2>/dev/null || :
+
     if which systemctl >/dev/null; then
         install_systemd
     else