fix(FLEETMDM-002): CU-86akj32d7 37 review findings across 18 files

server/datastore/mysql/nanomdm_storage.go

@@ -107,8 +107,9 @@ type pushCertStalenessCheck struct {
 
 // We store staleness check in-memory since it's a short-lived 5 minute time window.
 // And it also means some containers might rotate it faster than 5 minutes depending on the time.
+// Keyed by topic to support multiple push-cert topics without clobbering each other's cache entry.
 var (
-	pushCertStaleness   *pushCertStalenessCheck
+	pushCertStaleness   map[string]*pushCertStalenessCheck = make(map[string]*pushCertStalenessCheck)
 	pushCertStalenessMu sync.RWMutex
 )
 
@@ -124,17 +125,18 @@ func (s *NanoMDMStorage) RetrievePushCert(
 	}
 	pushCertStalenessMu.Lock()
 	defer pushCertStalenessMu.Unlock()
-	checkInMemoryHash(checksum)
+	checkInMemoryHash(topic, checksum)
 	return cert, checksum, nil
 }
 
-// checkInMemoryHash checks the incoming hash agains the in-memory hash.
+// checkInMemoryHash checks the incoming hash agains the in-memory hash for the given topic.
 // if criteria is met, it updates the in-memory hash with the new hash and updatedAt = now.
-func checkInMemoryHash(hash string) {
-	if pushCertStaleness == nil || pushCertStaleness.hash != hash || time.Since(pushCertStaleness.updatedAt) > 5*time.Minute {
+func checkInMemoryHash(topic, hash string) {
+	staleness := pushCertStaleness[topic]
+	if staleness == nil || staleness.hash != hash || time.Since(staleness.updatedAt) > 5*time.Minute {
 		// We will not call this unless we are stale, OR on new topic getting a provider, which means we should be fine to update here.
 		// Update on new hash, or if it's been more than 5 minutes since last update, to avoid fetching the cert on each stale check.
-		pushCertStaleness = &pushCertStalenessCheck{
+		pushCertStaleness[topic] = &pushCertStalenessCheck{
 			hash:      hash,
 			updatedAt: time.Now(),
 		}
@@ -147,7 +149,7 @@ func checkInMemoryHash(hash string) {
 // If the token is the same, it checks if the certificate was last updated more than 5 minutes ago. If so, it re-fetches the certificate and updates the hash for future checks.
 func (s *NanoMDMStorage) IsPushCertStale(ctx context.Context, topic, staleToken string) (bool, error) {
 	pushCertStalenessMu.RLock()
-	staleness := pushCertStaleness
+	staleness := pushCertStaleness[topic]
 	pushCertStalenessMu.RUnlock()
 	if staleness == nil {
 		return true, nil
@@ -165,7 +167,7 @@ func (s *NanoMDMStorage) IsPushCertStale(ctx context.Context, topic, staleToken
 		}
 		pushCertStalenessMu.Lock()
 		defer pushCertStalenessMu.Unlock()
-		checkInMemoryHash(checksum)
+		checkInMemoryHash(topic, checksum)
 		if checksum != staleToken {
 			s.logger.InfoContext(ctx, "push certificate is stale after re-checking", "topic", topic, "staleToken", staleToken, "newHash", checksum)
 			return true, nil
@@ -424,7 +426,7 @@ func enqueueCommandDB(ctx context.Context, tx sqlx.ExtContext, ids []string, cmd
 	// duplicate the code here, but that needs more careful planning
 	// (which we lack right now)
 	if len(ids) < 1 {
-		return errors.New("no id(s) supplied to queue command to")
+		return ctxerr.New(ctx, "no id(s) supplied to queue command to")
 	}
 	_, err := tx.ExecContext(
 		ctx,

server/datastore/mysql/password_reset.go

@@ -49,7 +49,7 @@ func (ds *Datastore) FindPasswordResetByToken(ctx context.Context, token string)
 	passwordResetRequest := &fleet.PasswordResetRequest{}
 	err := sqlx.GetContext(ctx, ds.reader(ctx), passwordResetRequest, sqlStatement, token)
 	if errors.Is(err, sql.ErrNoRows) {
-		return nil, ctxerr.Wrap(ctx, err, "invalid password reset token")
+		return nil, ctxerr.Wrap(ctx, notFound("PasswordResetRequest"), "invalid password reset token")
 	} else if err != nil {
 		return nil, ctxerr.Wrap(ctx, err, "selecting password reset token")
 	}

server/datastore/mysql/scep.go

@@ -7,11 +7,11 @@ import (
 	"crypto/x509"
 	"database/sql"
 	_ "embed"
-	"errors"
 	"fmt"
 	"math/big"
 
 	"github.com/fleetdm/fleet/v4/pkg/certificate"
+	"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
 	"github.com/fleetdm/fleet/v4/server/fleet"
 	"github.com/fleetdm/fleet/v4/server/mdm/assets"
 	"github.com/fleetdm/fleet/v4/server/mdm/scep/depot"
@@ -39,28 +39,31 @@ func newSCEPDepot(db *sql.DB, ds fleet.Datastore) (*SCEPDepot, error) {
 // CA returns the CA's certificate and private key.
 func (d *SCEPDepot) CA(_ []byte) ([]*x509.Certificate, *rsa.PrivateKey, error) {
 	// TODO(roberto): nano interfaces doesn't receive a context for this method.
-	cert, err := assets.CAKeyPair(context.Background(), d.ds)
+	ctx := context.Background()
+	cert, err := assets.CAKeyPair(ctx, d.ds)
 	if err != nil {
-		return nil, nil, fmt.Errorf("getting assets: %w", err)
+		return nil, nil, ctxerr.Wrap(ctx, err, "getting assets")
 	}
 
 	pk, ok := cert.PrivateKey.(*rsa.PrivateKey)
 	if !ok {
-		return nil, nil, errors.New("private key not in RSA format")
+		return nil, nil, ctxerr.New(ctx, "private key not in RSA format")
 	}
 
 	return []*x509.Certificate{cert.Leaf}, pk, nil
 }
 
 // Serial allocates and returns a new (increasing) serial number.
 func (d *SCEPDepot) Serial() (*big.Int, error) {
+	// TODO(roberto): nano interfaces doesn't receive a context for this method.
+	ctx := context.Background()
 	result, err := d.db.Exec(`INSERT INTO identity_serials () VALUES ();`)
 	if err != nil {
-		return nil, err
+		return nil, ctxerr.Wrap(ctx, err, "insert identity serial")
 	}
 	lid, err := result.LastInsertId()
 	if err != nil {
-		return nil, err
+		return nil, ctxerr.Wrap(ctx, err, "get last insert id for identity serial")
 	}
 	return big.NewInt(lid), nil
 }
@@ -71,10 +74,12 @@ func (d *SCEPDepot) Serial() (*big.Int, error) {
 // - allowTime are the maximum days before expiration to allow clients to do certificate renewal.
 // - revokeOldCertificate specifies whether to revoke the old certificate once renewed.
 func (d *SCEPDepot) HasCN(cn string, allowTime int, cert *x509.Certificate, revokeOldCertificate bool) (bool, error) {
+	// TODO(roberto): nano interfaces doesn't receive a context for this method.
+	ctx := context.Background()
 	var ct int
 	row := d.db.QueryRow(`SELECT COUNT(*) FROM identity_certificates WHERE name = ?`, cn)
 	if err := row.Scan(&ct); err != nil {
-		return false, err
+		return false, ctxerr.Wrap(ctx, err, "scan identity certificate count")
 	}
 	return ct >= 1, nil
 }
@@ -84,11 +89,13 @@ func (d *SCEPDepot) HasCN(cn string, allowTime int, cert *x509.Certificate, revo
 // If the provided certificate has empty crt.Subject.CommonName,
 // then the hex sha256 of the crt.Raw is used as name.
 func (d *SCEPDepot) Put(name string, crt *x509.Certificate) error {
+	// TODO(roberto): nano interfaces doesn't receive a context for this method.
+	ctx := context.Background()
 	if crt.Subject.CommonName == "" {
 		name = fmt.Sprintf("%x", sha256.Sum256(crt.Raw))
 	}
 	if !crt.SerialNumber.IsInt64() {
-		return errors.New("cannot represent serial number as int64")
+		return ctxerr.New(ctx, "cannot represent serial number as int64")
 	}
 	certPEM := certificate.EncodeCertPEM(crt)
 	_, err := d.db.Exec(`
@@ -102,5 +109,8 @@ VALUES
 		crt.NotAfter,
 		certPEM,
 	)
-	return err
+	if err != nil {
+		return ctxerr.Wrap(ctx, err, "insert identity certificate")
+	}
+	return nil
 }

server/datastore/mysql/software_title_display_names.go

@@ -21,7 +21,7 @@ func updateSoftwareTitleDisplayName(ctx context.Context, tx sqlx.ExtContext, tea
 		ON DUPLICATE KEY UPDATE
 			display_name = VALUES(display_name)`, tmID, titleID, displayName)
 	if err != nil {
-		return err
+		return ctxerr.Wrap(ctx, err, "upserting software title display name")
 	}
 
 	return nil

server/logging/webhook.go

@@ -3,22 +3,22 @@ package logging
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"log/slog"
 	"time"
 
 	"github.com/fleetdm/fleet/v4/server"
+	"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
 )
 
 type webhookLogWriter struct {
 	url    string
 	logger *slog.Logger
 }
 
-func NewWebhookLogWriter(webhookURL string, logger *slog.Logger) (*webhookLogWriter, error) {
+func NewWebhookLogWriter(ctx context.Context, webhookURL string, logger *slog.Logger) (*webhookLogWriter, error) {
 	if webhookURL == "" {
-		return nil, errors.New("webhook URL missing")
+		return nil, ctxerr.New(ctx, "webhook URL missing")
 	}
 
 	return &webhookLogWriter{
@@ -44,6 +44,10 @@ func (w *webhookLogWriter) Write(ctx context.Context, logs []json.RawMessage) er
 	)
 
 	if err := server.PostJSONWithTimeout(ctx, w.url, payload, w.logger); err != nil {
+		// Intentionally swallow this error: webhook delivery failures are logged
+		// here rather than returned so that a single failing webhook does not
+		// interrupt or fail the broader logging pipeline for callers. See
+		// TestWebhookFailure, which asserts Write returns nil in this case.
 		w.logger.ErrorContext(ctx, fmt.Sprintf("failed to send automation webhook to %s", server.MaskSecretURLParams(w.url)),
 			"err", server.MaskURLError(err).Error(),
 		)

server/mail/mail.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/fleetdm/fleet/v4/server"
 	"github.com/fleetdm/fleet/v4/server/config"
+	"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
 	"github.com/fleetdm/fleet/v4/server/fleet"
 )
 
@@ -100,11 +101,11 @@ func getFrom(e fleet.Email) (string, error) {
 
 func (m mailService) SendEmail(ctx context.Context, e fleet.Email) error {
 	if !e.SMTPSettings.SMTPConfigured {
-		return errors.New("requires that SMTP or SES (email) is configured.")
+		return ctxerr.New(ctx, "requires that SMTP or SES (email) is configured.")
 	}
 	msg, err := getMessageBody(e, getFrom)
 	if err != nil {
-		return err
+		return ctxerr.Wrap(ctx, err, "get message body")
 	}
 	return m.sendMail(ctx, e, msg)
 }
@@ -129,11 +130,11 @@ func isLocalhost(name string) bool {
 
 func (l *loginauth) Start(server *smtp.ServerInfo) (proto string, toServer []byte, err error) {
 	if !server.TLS && !isLocalhost(server.Name) {
-		return "", nil, errors.New("unencrypted connection")
+		return "", nil, ctxerr.New(context.Background(), "unencrypted connection")
 	}
 
 	if server.Name != l.host {
-		return "", nil, errors.New("wrong host name")
+		return "", nil, ctxerr.New(context.Background(), "wrong host name")
 	}
 
 	return "LOGIN", nil, nil
@@ -151,11 +152,11 @@ func (l *loginauth) Next(fromServer []byte, more bool) (toServer []byte, err err
 	case "Password:":
 		return []byte(l.password), nil
 	default:
-		return nil, errors.New("unexpected LOGIN prompt from server")
+		return nil, ctxerr.New(context.Background(), "unexpected LOGIN prompt from server")
 	}
 }
 
-func smtpAuth(e fleet.Email) (smtp.Auth, error) {
+func smtpAuth(ctx context.Context, e fleet.Email) (smtp.Auth, error) {
 	if e.SMTPSettings.SMTPAuthenticationType != fleet.AuthTypeNameUserNamePassword {
 		return nil, nil
 	}
@@ -174,23 +175,23 @@ func smtpAuth(e fleet.Email) (smtp.Auth, error) {
 	case fleet.AuthMethodNameLogin:
 		auth = LoginAuth(username, password, server)
 	default:
-		return nil, fmt.Errorf("unknown SMTP auth type '%s'", authMethod)
+		return nil, ctxerr.Errorf(ctx, "unknown SMTP auth type '%s'", authMethod)
 	}
 	return auth, nil
 }
 
 func (m mailService) sendMail(ctx context.Context, e fleet.Email, msg []byte) error {
 	smtpHost := fmt.Sprintf(
 		"%s:%d", e.SMTPSettings.SMTPServer, e.SMTPSettings.SMTPPort)
-	auth, err := smtpAuth(e)
+	auth, err := smtpAuth(ctx, e)
 	if err != nil {
-		return fmt.Errorf("failed to get smtp auth: %w", err)
+		return ctxerr.Wrap(ctx, err, "failed to get smtp auth")
 	}
 
 	if e.SMTPSettings.SMTPAuthenticationMethod == fleet.AuthMethodNameCramMD5 {
 		err = smtp.SendMail(smtpHost, auth, e.SMTPSettings.SMTPSenderAddress, e.To, msg)
 		if err != nil {
-			return fmt.Errorf("failed to send mail. crammd5 auth method: %w", err)
+			return ctxerr.Wrap(ctx, err, "failed to send mail. crammd5 auth method")
 		}
 		return nil
 	}
@@ -202,18 +203,18 @@ func (m mailService) sendMail(ctx context.Context, e fleet.Email, msg []byte) er
 
 	var client *smtp.Client
 	if e.SMTPSettings.SMTPEnableTLS {
-		client, err = dialTimeout(smtpHost, tlsConfig)
+		client, err = dialTimeout(ctx, smtpHost, tlsConfig)
 	} else {
-		client, err = dialTimeout(smtpHost, nil)
+		client, err = dialTimeout(ctx, smtpHost, nil)
 	}
 	if err != nil {
-		return fmt.Errorf("could not dial smtp host: %w", err)
+		return ctxerr.Wrap(ctx, err, "could not dial smtp host")
 	}
 	defer client.Close()
 
 	if e.SMTPSettings.SMTPDomain != "" {
 		if err = client.Hello(e.SMTPSettings.SMTPDomain); err != nil {
-			return fmt.Errorf("client hello error: %w", err)
+			return ctxerr.Wrap(ctx, err, "client hello error")
 		}
 	}
 	if e.SMTPSettings.SMTPEnableStartTLS {
@@ -224,42 +225,42 @@ func (m mailService) sendMail(ctx context.Context, e fleet.Email, msg []byte) er
 				if !e.SMTPSettings.SMTPEnableTLS && e.SMTPSettings.SMTPVerifySSLCerts {
 					return ErrSTARTTLSWithoutSSLTLS
 				}
-				return fmt.Errorf("startTLS error: %w", err)
+				return ctxerr.Wrap(ctx, err, "startTLS error")
 			}
 		}
 	}
 	if auth != nil {
 		if err = client.Auth(auth); err != nil {
-			return fmt.Errorf("client auth error: %w", err)
+			return ctxerr.Wrap(ctx, err, "client auth error")
 		}
 	}
 	if err = client.Mail(e.SMTPSettings.SMTPSenderAddress); err != nil {
-		return fmt.Errorf("could not issue mail to provided address: %w", err)
+		return ctxerr.Wrap(ctx, err, "could not issue mail to provided address")
 	}
 	for _, recip := range e.To {
 		if err = client.Rcpt(recip); err != nil {
-			return fmt.Errorf("failed to get recipient: %w", err)
+			return ctxerr.Wrap(ctx, err, "failed to get recipient")
 		}
 	}
 	writer, err := client.Data()
 	if err != nil {
-		return fmt.Errorf("getting client data: %w", err)
+		return ctxerr.Wrap(ctx, err, "getting client data")
 	}
 
 	_, err = writer.Write(msg)
 	if err != nil {
-		return fmt.Errorf("failed to write: %w", err)
+		return ctxerr.Wrap(ctx, err, "failed to write")
 	}
 
 	if err = writer.Close(); err != nil {
-		return fmt.Errorf("failed to close writer: %w", err)
+		return ctxerr.Wrap(ctx, err, "failed to close writer")
 	}
 
 	if err := client.Quit(); err != nil {
 		// Ignore EOF errors on quit, which can happen if the server
 		// closes the connection after the message is sent.
 		if !errors.Is(err, io.EOF) {
-			return fmt.Errorf("error on client quit: %w", err)
+			return ctxerr.Wrap(ctx, err, "error on client quit")
 		}
 	}
 	return nil
@@ -269,7 +270,7 @@ const dialTimeoutDuration = 28 * time.Second
 
 // dialTimeout sets a timeout on net.Dial to prevent email from attempting to
 // send indefinitely.
-func dialTimeout(addr string, tlsConfig *tls.Config) (client *smtp.Client, err error) {
+func dialTimeout(ctx context.Context, addr string, tlsConfig *tls.Config) (client *smtp.Client, err error) {
 	// Ensure that errors are always returned after at least 5s to
 	// eliminate (some) timing attacks (in which a malicious user tries to
 	// port scan using the email functionality in Fleet)
@@ -289,11 +290,11 @@ func dialTimeout(addr string, tlsConfig *tls.Config) (client *smtp.Client, err e
 	}
 
 	if err != nil {
-		return nil, fmt.Errorf("dialing with timeout: %w", err)
+		return nil, ctxerr.Wrap(ctx, err, "dialing with timeout")
 	}
 	host, _, err := net.SplitHostPort(addr)
 	if err != nil {
-		return nil, fmt.Errorf("split host port: %w", err)
+		return nil, ctxerr.Wrap(ctx, err, "split host port")
 	}
 
 	// Set a deadline to ensure we time out quickly when there is a TCP
@@ -302,7 +303,7 @@ func dialTimeout(addr string, tlsConfig *tls.Config) (client *smtp.Client, err e
 	_ = conn.SetDeadline(time.Now().Add(28 * time.Second))
 	client, err = smtp.NewClient(conn, host)
 	if err != nil {
-		return nil, fmt.Errorf("SMTP connection error: %w", err)
+		return nil, ctxerr.Wrap(ctx, err, "SMTP connection error")
 	}
 	// Clear deadlines
 	_ = conn.SetDeadline(time.Time{})

server/mdm/acme/internal/mysql/challenge.go

@@ -56,7 +56,7 @@ func (ds *Datastore) GetChallengeByID(ctx context.Context, accountID, challengeI
 // UpdateChallenge handles updating the challenge status, and the authorization status as well as moving the order status.
 func (ds *Datastore) UpdateChallenge(ctx context.Context, challenge *types.Challenge) (*types.Challenge, error) {
 	if challenge == nil {
-		return nil, errors.New("Challenge can not be nil for update")
+		return nil, ctxerr.New(ctx, "Challenge can not be nil for update")
 	}
 
 	err := platform_mysql.WithRetryTxx(ctx, ds.writer(ctx), func(tx sqlx.ExtContext) error {