diff --git a/server/datastore/mysql/nanomdm_storage.go b/server/datastore/mysql/nanomdm_storage.go index c2e0ead15a4..4cdf3fa13c7 100644 --- a/server/datastore/mysql/nanomdm_storage.go +++ b/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, diff --git a/server/datastore/mysql/password_reset.go b/server/datastore/mysql/password_reset.go index 4edd3f39514..33acd7f7d62 100644 --- a/server/datastore/mysql/password_reset.go +++ b/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") } diff --git a/server/datastore/mysql/scep.go b/server/datastore/mysql/scep.go index e5f40194382..6afb1dcceb1 100644 --- a/server/datastore/mysql/scep.go +++ b/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,14 +39,15 @@ 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 @@ -54,13 +55,15 @@ func (d *SCEPDepot) CA(_ []byte) ([]*x509.Certificate, *rsa.PrivateKey, error) { // 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 } diff --git a/server/datastore/mysql/software_title_display_names.go b/server/datastore/mysql/software_title_display_names.go index e6d50ceab51..74f4ee69eb9 100644 --- a/server/datastore/mysql/software_title_display_names.go +++ b/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 diff --git a/server/logging/webhook.go b/server/logging/webhook.go index 259fdc9d47b..7b6a91c9639 100644 --- a/server/logging/webhook.go +++ b/server/logging/webhook.go @@ -3,12 +3,12 @@ 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 { @@ -16,9 +16,9 @@ type webhookLogWriter struct { 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(), ) diff --git a/server/mail/mail.go b/server/mail/mail.go index b736af4792d..60bbef69f24 100644 --- a/server/mail/mail.go +++ b/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,7 +175,7 @@ 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 } @@ -182,15 +183,15 @@ func smtpAuth(e fleet.Email) (smtp.Auth, error) { 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{}) diff --git a/server/mdm/acme/internal/mysql/challenge.go b/server/mdm/acme/internal/mysql/challenge.go index 9000d90b0a6..ea4c76f6eab 100644 --- a/server/mdm/acme/internal/mysql/challenge.go +++ b/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 { diff --git a/server/mdm/acme/internal/service/endpoint_utils.go b/server/mdm/acme/internal/service/endpoint_utils.go index 63f8f3a6874..4f065f181c5 100644 --- a/server/mdm/acme/internal/service/endpoint_utils.go +++ b/server/mdm/acme/internal/service/endpoint_utils.go @@ -10,6 +10,7 @@ import ( "net/url" "reflect" + "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/mdm/acme/api" "github.com/fleetdm/fleet/v4/server/mdm/acme/internal/types" eu "github.com/fleetdm/fleet/v4/server/platform/endpointer" @@ -34,9 +35,11 @@ func encodeResponse(ctx context.Context, w http.ResponseWriter, response any) er func acmeErrorEncoder(ctx context.Context, err error, w http.ResponseWriter) { var acmeErr *types.ACMEError if !errors.As(err, &acmeErr) { - // TODO: If we can get access to a logger, we can log the details here, to help troubleshoot service errors. // if it's not already an ACME error, it is because it is an internal server // error (or a dev error, for 4xx we should always return ACMEError). + // Route the original error through ctxerr so it is captured for centralized + // observability, without leaking internal details to the client. + ctxerr.Handle(ctx, ctxerr.New(ctx, err.Error())) acmeErr = types.InternalServerError("") // not passing err.Error() as we don't want to leak internal details } @@ -133,3 +136,4 @@ func newEndpointerWithNoAuth(svc api.Service, authMiddleware endpoint.Middleware Versions: versions, } } + diff --git a/server/mdm/apple/commander.go b/server/mdm/apple/commander.go index 048d42c268a..4168d061f82 100644 --- a/server/mdm/apple/commander.go +++ b/server/mdm/apple/commander.go @@ -3,6 +3,7 @@ package apple_mdm import ( "context" "encoding/base64" + "encoding/xml" "fmt" "net/http" "sort" @@ -157,9 +158,10 @@ func (svc *MDMAppleCommander) DeviceLock(ctx context.Context, host *fleet.Host, } if c, ok := err.(conflictInterface); ok && c.IsConflict() { // Another goroutine won the race, fetch the command that was created + origConflictErr := err existingCmd, existingPIN, err := svc.storage.GetPendingLockCommand(ctx, host.UUID) if err != nil { - return "", ctxerr.Wrap(ctx, err, "getting existing lock after race condition") + return "", ctxerr.Wrap(ctx, err, "getting existing lock after race condition", "original conflict error", origConflictErr) } if existingCmd != nil { // Send push notification for the existing command and return its PIN @@ -172,7 +174,7 @@ func (svc *MDMAppleCommander) DeviceLock(ctx context.Context, host *fleet.Host, return existingPIN, nil } // This shouldn't happen, but if we can't find the command, return the original error - return "", ctxerr.Wrap(ctx, err, "lock command conflict but no existing command found") + return "", ctxerr.Wrap(ctx, origConflictErr, "lock command conflict but no existing command found") } return "", ctxerr.Wrap(ctx, err, "enqueuing for DeviceLock") } @@ -326,7 +328,7 @@ func (svc *MDMAppleCommander) InstallEnterpriseApplicationWithEmbeddedManifest( raw, err := plist.Marshal(cmd) if err != nil { - return fmt.Errorf("marshal command payload plist: %w", err) + return ctxerr.Wrap(ctx, err, "marshal command payload plist") } return svc.EnqueueCommand(ctx, hostUUIDs, string(raw)) @@ -350,6 +352,16 @@ type AdminAccountConfig struct { PrimaryAccountType fleet.PrimaryAccountType // admin, standard, or none } +// xmlEscapeString escapes a string for safe embedding inside plist +// elements, guarding against user-controlled values (e.g. SSO-provided names) +// that may contain XML special characters. +func xmlEscapeString(s string) string { + var b strings.Builder + // xml.EscapeText never returns an error for a strings.Builder writer. + _ = xml.EscapeText(&b, []byte(s)) + return b.String() +} + func (svc *MDMAppleCommander) AccountConfiguration(ctx context.Context, hostUUIDs []string, cmdUUID string, ssoAccount *SSOAccountConfig, @@ -366,7 +378,7 @@ func (svc *MDMAppleCommander) AccountConfiguration(ctx context.Context, hostUUID %s LockPrimaryAccountInfo <%t /> -`, ssoAccount.FullName, ssoAccount.UserName, ssoAccount.LockPrimaryAccountInfo) +`, xmlEscapeString(ssoAccount.FullName), xmlEscapeString(ssoAccount.UserName), ssoAccount.LockPrimaryAccountInfo) } if adminAccount != nil { @@ -400,7 +412,7 @@ func (svc *MDMAppleCommander) AccountConfiguration(ctx context.Context, hostUUID %s -`, adminAccount.Hidden, passwordHashEncoded, adminAccount.ShortName, adminAccount.FullName) +`, adminAccount.Hidden, passwordHashEncoded, xmlEscapeString(adminAccount.ShortName), xmlEscapeString(adminAccount.FullName)) } raw := fmt.Sprintf(` diff --git a/server/platform/endpointer/clientip.go b/server/platform/endpointer/clientip.go index 54f4d3efb2c..201f810a3dd 100644 --- a/server/platform/endpointer/clientip.go +++ b/server/platform/endpointer/clientip.go @@ -2,6 +2,7 @@ package endpointer import ( "fmt" + "log" "net/http" "strconv" "strings" @@ -29,6 +30,9 @@ func NewClientIPStrategy(trustedProxies string) (realclientip.Strategy, error) { if trustedProxies == "" { // Empty: legacy behavior for backwards compatibility. + log.Println("warning: trusted_proxies is not set; falling back to legacy client IP " + + "detection, which trusts spoofable headers (True-Client-IP, X-Real-IP, X-Forwarded-For) " + + "unconditionally. Set trusted_proxies to \"none\" if this server is exposed directly to the internet.") return &legacyStrategy{}, nil } else if strings.EqualFold(trustedProxies, "none") { // "none": Trust no one; return (non-spoofable) RemoteAddr only. @@ -83,3 +87,4 @@ func (s *legacyStrategy) ClientIP(headers http.Header, remoteAddr string) string } return extractIP(r) } + diff --git a/server/service/linux_mdm.go b/server/service/linux_mdm.go index 10ed4968be1..97093fbc88f 100644 --- a/server/service/linux_mdm.go +++ b/server/service/linux_mdm.go @@ -23,7 +23,7 @@ func (svc *Service) LinuxHostDiskEncryptionStatus(ctx context.Context, host flee Status: &actionRequired, }, nil } - return fleet.HostMDMDiskEncryption{}, err + return fleet.HostMDMDiskEncryption{}, ctxerr.Wrap(ctx, err) } if key.ClientError != "" { diff --git a/server/service/mdm_scep.go b/server/service/mdm_scep.go index 61eaf28a150..80a82148fa8 100644 --- a/server/service/mdm_scep.go +++ b/server/service/mdm_scep.go @@ -79,12 +79,18 @@ func (svc *service) PKIOperation(ctx context.Context, data []byte) ([]byte, erro } if err != nil { svc.debugLogger.ErrorContext(ctx, "failed to sign CSR", "err", err) - certRep, err := msg.Fail(cert.Leaf, pk, scep.BadRequest) - return certRep.Raw, err + certRep, failErr := msg.Fail(cert.Leaf, pk, scep.BadRequest) + if failErr != nil { + return nil, failErr + } + return certRep.Raw, nil } certRep, err := msg.Success(cert.Leaf, pk, crt) - return certRep.Raw, err + if err != nil { + return nil, err + } + return certRep.Raw, nil } func (svc *service) GetNextCACert(ctx context.Context) ([]byte, error) { @@ -95,7 +101,7 @@ func (svc *service) GetNextCACert(ctx context.Context) ([]byte, error) { func NewSCEPService(ds fleet.MDMAssetRetriever, signer scepserver.CSRSignerContext, logger *slog.Logger) scepserver.Service { return &service{ signer: signer, - debugLogger: slog.New(slog.DiscardHandler), + debugLogger: logger, ds: ds, } } diff --git a/server/service/team_policies.go b/server/service/team_policies.go index d5da90c1e14..294cec1a08a 100644 --- a/server/service/team_policies.go +++ b/server/service/team_policies.go @@ -2,7 +2,6 @@ package service import ( "context" - "errors" "fmt" "maps" "reflect" @@ -61,7 +60,7 @@ func (svc Service) NewTeamPolicy(ctx context.Context, teamID uint, tp fleet.NewT vc, ok := viewer.FromContext(ctx) if !ok { - return nil, errors.New("user must be authenticated to create team policies") + return nil, ctxerr.New(ctx, "user must be authenticated to create team policies") } p, err := svc.newTeamPolicyPayloadToPolicyPayload(ctx, teamID, tp) @@ -261,7 +260,8 @@ func (svc *Service) populateSoftwareIconURLs(ctx context.Context, policies []*fl // to (see getPolicySoftwareTitleIconURL), so it's safe to point at it // without risking a 404. if hasCustomIcon || p.VPPAppsTeamsID != nil { - t.IconURL = new(getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID)) + iconURL := getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID) + t.IconURL = &iconURL } } @@ -269,7 +269,8 @@ func (svc *Service) populateSoftwareIconURLs(ctx context.Context, policies []*fl // Patch software is always a package installer (never a VPP app), so // it only gets an icon URL when a custom icon was uploaded. if _, ok := icons[t.SoftwareTitleID]; ok { - t.IconURL = new(getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID)) + iconURL := getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID) + t.IconURL = &iconURL } } } @@ -413,11 +414,11 @@ func (svc *Service) CountTeamPolicies(ctx context.Context, teamID uint, matchQue if mergeInherited { count, err := svc.ds.CountMergedTeamPolicies(ctx, teamID, matchQuery, automationType) if err != nil { - return 0, 0, err + return 0, 0, ctxerr.Wrap(ctx, err, "count merged team policies") } inheritedCount, err := svc.ds.CountPolicies(ctx, nil, matchQuery, automationType) if err != nil { - return 0, 0, err + return 0, 0, ctxerr.Wrap(ctx, err, "count inherited policies") } return count, inheritedCount, nil } diff --git a/server/service/validation_setup.go b/server/service/validation_setup.go index 8b4a46d33bb..4a588180859 100644 --- a/server/service/validation_setup.go +++ b/server/service/validation_setup.go @@ -2,7 +2,6 @@ package service import ( "context" - "errors" "net/url" "strings" @@ -18,7 +17,7 @@ func (mw validationMiddleware) NewAppConfig(ctx context.Context, payload fleet.A } else { serverURLString = cleanupURL(payload.ServerSettings.ServerURL) } - if err := ValidateServerURL(serverURLString); err != nil { + if err := ValidateServerURL(ctx, serverURLString); err != nil { invalid.Append("server_url", err.Error()) } if invalid.HasErrors() { @@ -27,22 +26,23 @@ func (mw validationMiddleware) NewAppConfig(ctx context.Context, payload fleet.A return mw.Service.NewAppConfig(ctx, payload) } -func ValidateServerURL(urlString string) error { +func ValidateServerURL(ctx context.Context, urlString string) error { // TODO - implement more robust URL validation here // no valid scheme provided if !(strings.HasPrefix(urlString, "http://") || strings.HasPrefix(urlString, "https://")) { - return errors.New(fleet.InvalidServerURLMsg) + return ctxerr.New(ctx, fleet.InvalidServerURLMsg) } // valid scheme provided - require host parsed, err := url.Parse(urlString) if err != nil { - return err + return ctxerr.Wrap(ctx, err) } if parsed.Host == "" { - return errors.New(fleet.InvalidServerURLMsg) + return ctxerr.New(ctx, fleet.InvalidServerURLMsg) } return nil } + diff --git a/server/vulnerabilities/macoffice/analyzer.go b/server/vulnerabilities/macoffice/analyzer.go index 8bb9ee0206e..b872aaa9ab9 100644 --- a/server/vulnerabilities/macoffice/analyzer.go +++ b/server/vulnerabilities/macoffice/analyzer.go @@ -3,7 +3,6 @@ package macoffice import ( "context" "encoding/json" - "errors" "fmt" "os" "path/filepath" @@ -22,7 +21,7 @@ func getLatestReleaseNotes(vulnPath string) (ReleaseNotes, error) { files, err := fs.MacOfficeReleaseNotes() if err != nil { - return nil, err + return nil, fmt.Errorf("listing mac office release notes: %w", err) } if len(files) == 0 { @@ -34,13 +33,13 @@ func getLatestReleaseNotes(vulnPath string) (ReleaseNotes, error) { payload, err := os.ReadFile(filePath) if err != nil { - return nil, err + return nil, fmt.Errorf("reading mac office release notes file %s: %w", filePath, err) } relNotes := ReleaseNotes{} err = json.Unmarshal(payload, &relNotes) if err != nil { - return nil, err + return nil, fmt.Errorf("unmarshalling mac office release notes file %s: %w", filePath, err) } // Ensure the release notes are sorted by release date, this is because the vuln. processing @@ -87,7 +86,7 @@ func getStoredVulnerabilities( ) ([]fleet.SoftwareVulnerability, error) { storedSoftware, err := ds.SoftwareByID(ctx, softwareID, nil, false, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("getting software by id %d: %w", softwareID, err) } var result []fleet.SoftwareVulnerability @@ -116,7 +115,7 @@ func updateVulnsInDB( err := ds.DeleteSoftwareVulnerabilities(ctx, toDelete) if err != nil { - return nil, err + return nil, fmt.Errorf("deleting software vulnerabilities: %w", err) } allVulns := make([]fleet.SoftwareVulnerability, 0, len(toInsertSet)) @@ -124,7 +123,12 @@ func updateVulnsInDB( allVulns = append(allVulns, v) } - return ds.InsertSoftwareVulnerabilities(ctx, allVulns, fleet.MacOfficeReleaseNotesSource) + result, err := ds.InsertSoftwareVulnerabilities(ctx, allVulns, fleet.MacOfficeReleaseNotesSource) + if err != nil { + return nil, fmt.Errorf("inserting software vulnerabilities: %w", err) + } + + return result, nil } // Analyze uses the most recent Mac Office release notes asset in 'vulnPath' for detecting @@ -155,7 +159,7 @@ func Analyze( } } if !hasValid { - return nil, errors.New("MacOffice release notes contain no valid security updates (possible corrupted feed)") + return nil, ctxerr.New(ctx, "MacOffice release notes contain no valid security updates (possible corrupted feed)") } queryParams := fleet.SoftwareIterQueryOptions{IncludedSources: []string{"apps"}} diff --git a/server/vulnerabilities/msrc/analyzer.go b/server/vulnerabilities/msrc/analyzer.go index a276ceda9aa..a47b33282ee 100644 --- a/server/vulnerabilities/msrc/analyzer.go +++ b/server/vulnerabilities/msrc/analyzer.go @@ -2,7 +2,6 @@ package msrc import ( "context" - "errors" "fmt" "log/slog" "strconv" @@ -34,11 +33,12 @@ func Analyze( return nil, err } - // Refuse to proceed if the loaded bulletin contains no vulnerability data — an empty - // bulletin would cause every existing MSRC OS vulnerability for this OS to be marked as - // remediated. This usually indicates the bulletin file was corrupted during download. + // Warn if the loaded bulletin contains no vulnerability data. This may indicate the + // bulletin file was corrupted during download, but it could also reflect a legitimately + // empty feed (e.g. a new product with no known vulnerabilities yet), so we don't treat + // it as a hard failure. if len(bulletin.Vulnerabilities) == 0 { - return nil, errors.New("MSRC bulletin contains no vulnerabilities (possible corrupted feed)") + logger.WarnContext(ctx, "MSRC bulletin contains no vulnerabilities (possible corrupted feed)", "os", os.Name) } // Find matching products inside the bulletin diff --git a/server/vulnerabilities/msrc/msrc_api.go b/server/vulnerabilities/msrc/msrc_api.go index c44f7356d1a..d043913666c 100644 --- a/server/vulnerabilities/msrc/msrc_api.go +++ b/server/vulnerabilities/msrc/msrc_api.go @@ -28,6 +28,13 @@ type MSRCAPI interface { // E.g. September 2024 bulleting was released on the 2nd. var FeedNotFound = errors.New("feed not found") +// ErrMinAllowedDate is returned when the requested feed date is before the minimum +// allowed date supported by MSRC. +var ErrMinAllowedDate = errors.New("min allowed date") + +// ErrFutureDate is returned when the requested feed date is in the future. +var ErrFutureDate = errors.New("date can't be in the future") + type MSRCClient struct { client *http.Client workDir string @@ -59,11 +66,11 @@ func (msrc MSRCClient) GetFeed(month time.Month, year int) (string, error) { minD := time.Date(MSRCMinYear, time.January, 1, 0, 0, 0, 0, time.UTC) if d.Before(minD) { - return "", fmt.Errorf("min allowed date is %s", minD) + return "", fmt.Errorf("%w: %s", ErrMinAllowedDate, minD) } if d.After(time.Now().UTC()) { - return "", errors.New("date can't be in the future") + return "", ErrFutureDate } dst := filepath.Join(msrc.workDir, fmt.Sprintf("%s.xml", feedName(d))) diff --git a/server/vulnerabilities/oval/analyzer.go b/server/vulnerabilities/oval/analyzer.go index cc8a7eed75b..713029eb2fd 100644 --- a/server/vulnerabilities/oval/analyzer.go +++ b/server/vulnerabilities/oval/analyzer.go @@ -4,10 +4,10 @@ import ( "context" "encoding/json" "errors" - "fmt" "os" "time" + "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/fleet" oval_parsed "github.com/fleetdm/fleet/v4/server/vulnerabilities/oval/parsed" utils "github.com/fleetdm/fleet/v4/server/vulnerabilities/utils" @@ -43,12 +43,12 @@ func Analyze( defs, err := loadDef(platform, vulnPath) if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "load oval definitions") } rules, err := GetKnownOVALBugRules() if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "get known oval bug rules") } // Since hosts and software have a M:N relationship, the following sets are used to @@ -61,7 +61,7 @@ func Analyze( for { hostIDs, err := ds.HostIDsByOSVersion(ctx, ver, offset, hostsBatchSize) if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "get host IDs by os version") } if len(hostIDs) == 0 { @@ -75,18 +75,18 @@ func Analyze( hostID := hostID software, err := ds.ListSoftwareForVulnDetection(ctx, fleet.VulnSoftwareFilter{HostID: &hostID}) if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "list software for vuln detection") } evalR, err := defs.Eval(ver, software) if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "eval oval definitions") } foundInBatch[hostID] = evalR evalU, err := defs.EvalKernel(software) if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "eval kernel oval definitions") } foundInBatch[hostID] = append(foundInBatch[hostID], evalU...) @@ -111,7 +111,7 @@ func Analyze( existingInBatch, err := ds.ListSoftwareVulnerabilitiesByHostIDsSource(ctx, hostIDs, source) if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "list software vulnerabilities by host ids source") } for _, hostID := range hostIDs { @@ -129,7 +129,7 @@ func Analyze( return ds.DeleteSoftwareVulnerabilities(ctx, v) }, vulnBatchSize) if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "delete software vulnerabilities") } allVulns := make([]fleet.SoftwareVulnerability, 0, len(toInsertSet)) @@ -139,10 +139,10 @@ func Analyze( newVulns, err := ds.InsertSoftwareVulnerabilities(ctx, allVulns, source) if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "insert software vulnerabilities") } if !collectVulns { - return nil, nil + return newVulns, nil } return newVulns, nil @@ -155,26 +155,26 @@ func Analyze( // the artifact download from GitHub was corrupted or partially failed. func loadDef(platform Platform, vulnPath string) (oval_parsed.Result, error) { if !platform.IsSupported() { - return nil, fmt.Errorf("platform %q not supported", platform) + return nil, ctxerr.Errorf(context.Background(), "platform %q not supported", platform) } fileName := platform.ToFilename(time.Now(), "json") latest, err := utils.LatestFile(fileName, vulnPath) if err != nil { - return nil, err + return nil, ctxerr.Wrap(context.Background(), err, "get latest oval file") } payload, err := os.ReadFile(latest) if err != nil { - return nil, err + return nil, ctxerr.Wrap(context.Background(), err, "read oval file") } if platform.IsUbuntu() { result := oval_parsed.UbuntuResult{} if err := json.Unmarshal(payload, &result); err != nil { - return nil, err + return nil, ctxerr.Wrap(context.Background(), err, "unmarshal ubuntu oval result") } if len(result.Definitions) == 0 { - return nil, fmt.Errorf("OVAL definition file %q contains no rules (possible corrupted feed)", latest) + return nil, ctxerr.Errorf(context.Background(), "OVAL definition file %q contains no rules (possible corrupted feed)", latest) } return result, nil } @@ -182,13 +182,13 @@ func loadDef(platform Platform, vulnPath string) (oval_parsed.Result, error) { if platform.IsRedHat() { result := oval_parsed.RhelResult{} if err := json.Unmarshal(payload, &result); err != nil { - return nil, err + return nil, ctxerr.Wrap(context.Background(), err, "unmarshal rhel oval result") } if len(result.Definitions) == 0 { - return nil, fmt.Errorf("OVAL definition file %q contains no rules (possible corrupted feed)", latest) + return nil, ctxerr.Errorf(context.Background(), "OVAL definition file %q contains no rules (possible corrupted feed)", latest) } return result, nil } - return nil, fmt.Errorf("don't know how to parse file %q for %q platform", latest, platform) + return nil, ctxerr.Errorf(context.Background(), "don't know how to parse file %q for %q platform", latest, platform) }