From 33d91fa247be37553345f0b0925ad213e6f84798 Mon Sep 17 00:00:00 2001 From: Lucas Rayan Date: Sun, 2 Aug 2026 07:57:22 -0300 Subject: [PATCH 1/3] #19: fix F1-F3 defects found auditing the phases against PLAN.md Every fix here is a case where the code and the plan disagreed, and the code was wrong. Certificates (section 8.2, 8.4). The parser documented that it skipped expired entries but never checked, so an expired certificate would be served until every handshake failed. LastReload() only advanced when acme.json changed - and it does not change for sixty days - which made the timestamp risk R2 monitors indistinguishable from a watcher gone blind. Expiry is now checked on every pass, LastReload records the check and LastChange the swap, and the <7d/<24h alerts of section 8.4 exist. ARC (section 5). buildArcSealer resolved the DKIM key at startup, before runDnsReconciler had provisioned one. On a first boot ARC was disabled until someone restarted the process, which the log even admitted while offering no way for it to start. A LazySealer binds on first use instead. Health (section 8.4). There was no degraded state at all. /health now reports one - still 200, because restarting cannot fix a certificate Traefik never issued. Bounces. DSNs were sent From postmaster@lambdamail.local, a domain that does not resolve, so the notification was itself undeliverable. They now carry the configured mail host, a Message-ID, and Auto-Submitted so a vacation responder cannot loop against them. Fan-out atomicity (section 6.1). Persist ran one transaction per recipient, so a failure halfway through an alias fan-out left earlier recipients with a copy while the sender was told to retry - and the retry delivered to them twice. PersistAll commits the whole delivery or none of it. Verified against Postgres by reverting the repository to the old behaviour and confirming the new test fails on it. SMTP replies. The use case encoded replies in error text ("550 5.7.1 ...") which the adapter re-parsed by prefix, so rewording a message could turn a permanent refusal into a retry. SmtpRejection carries the decision and the adapter renders it. Repository tests. These only run with TEST_DATABASE_URL, which CI does set - so the job was red: copy and expunge seeded a 68-character digest into a CHAR(64) column, and three others used fixed digests that collide with the UNIQUE index on any re-run. A shared testBlobDigest() helper fixes both; the suite now passes twice in a row against one database. Co-Authored-By: Claude Opus 5 --- .../cmd/lambdamail-protocols/main.go | 80 ++++++----- .../port/inbound_message_repository.go | 22 ++- .../application/usecase/outbound_worker.go | 4 +- .../usecase/process_inbound_email.go | 27 ++-- .../usecase/process_inbound_email_test.go | 52 +++++++- .../application/usecase/smtp_rejection.go | 55 ++++++++ .../internal/domain/valueobject/dsn_bounce.go | 33 ++++- .../domain/valueobject/dsn_bounce_test.go | 34 ++++- .../infrastructure/arc/lazy_sealer.go | 75 +++++++++++ .../infrastructure/arc/lazy_sealer_test.go | 108 +++++++++++++++ .../postgres/copy_repository_test.go | 2 +- .../postgres/expunge_repository_test.go | 2 +- .../postgres/flag_repository_test.go | 2 +- .../postgres/inbound_message_repository.go | 34 ++++- .../inbound_message_repository_test.go | 99 ++++++++++++-- .../postgres/mailbox_repository_test.go | 8 ++ .../postgres/message_query_repository_test.go | 2 +- .../infrastructure/tls/acme_cert_watcher.go | 125 +++++++++++++++--- .../tls/acme_cert_watcher_test.go | 74 +++++++++-- .../internal/presentation/http/router.go | 27 ++++ .../internal/presentation/http/router_test.go | 36 +++++ .../internal/presentation/smtp/backend.go | 34 ++--- .../presentation/smtp/backend_test.go | 110 ++++++++++++++- 23 files changed, 906 insertions(+), 139 deletions(-) create mode 100644 services/protocols/internal/application/usecase/smtp_rejection.go create mode 100644 services/protocols/internal/infrastructure/arc/lazy_sealer.go create mode 100644 services/protocols/internal/infrastructure/arc/lazy_sealer_test.go diff --git a/services/protocols/cmd/lambdamail-protocols/main.go b/services/protocols/cmd/lambdamail-protocols/main.go index 495103e..931dd18 100644 --- a/services/protocols/cmd/lambdamail-protocols/main.go +++ b/services/protocols/cmd/lambdamail-protocols/main.go @@ -10,6 +10,7 @@ package main import ( "context" "crypto/tls" + "errors" "fmt" "log" "net" @@ -80,7 +81,7 @@ func run(cfg config) { } secretVault, vaultErr := vault.New(cfg.MasterKey) - certProvider := buildCertProvider(ctx, cfg, pool, secretVault, vaultErr) + certProvider, certDegraded := buildCertProvider(ctx, cfg, pool, secretVault, vaultErr) // ---------------------------------------------------------------- repos mailboxes := postgres.NewMailboxRepository(pool) @@ -131,12 +132,13 @@ func run(cfg config) { log.Printf("lambdamail-protocols delivering through the smarthost %s", relay.Address()) } - // ARC needs a signing key, so it can only be enabled where DKIM is. + // ARC needs a signing key, so it can only be enabled where DKIM is. The + // key is resolved on the first message rather than here: on a first boot + // the provisioner below has not run yet, and binding now would leave ARC + // off until someone restarted the process. if cfg.ArcSealEnabled && dkimRepo != nil { - if sealer := buildArcSealer(ctx, cfg, dkimRepo); sealer != nil { - inboundUC.SetArcSealer(sealer) - log.Printf("lambdamail-protocols ARC sealing enabled for %s", cfg.domain()) - } + inboundUC.SetArcSealer(arc.NewLazySealer(dkimRepo, dkim.ParsePrivateKey, cfg.domain(), cfg.PrimaryMailHost)) + log.Printf("lambdamail-protocols ARC sealing enabled for %s", cfg.domain()) } // --------------------------------------------------------- access UCs @@ -161,6 +163,7 @@ func run(cfg config) { // ------------------------------------------------------------- HTTP API router := httppresentation.NewRouter(usecase.NewIngestReportsUseCase(reportRepo), func() error { return pool.Ping(ctx) }) + router.SetDegradedCheck(certDegraded) applyMtaStsMode(router, cfg) server := &http.Server{ @@ -187,7 +190,11 @@ func run(cfg config) { // store Traefik maintains and keeps watching it, because the certificate is // replaced every sixty days and a stale one takes every listener down // (PLAN.md section 8). -func buildCertProvider(ctx context.Context, cfg config, pool *pgxpool.Pool, secretVault *vault.SecretVault, vaultErr error) port.CertProvider { +// +// The second return value is the probe behind /health's degraded state: a +// certificate problem does not stop the process, so it has to be visible +// somewhere an operator is already looking (PLAN.md section 8.4). +func buildCertProvider(ctx context.Context, cfg config, pool *pgxpool.Pool, secretVault *vault.SecretVault, vaultErr error) (port.CertProvider, func() error) { // Mode B: LambdaMail obtains the certificate itself. This is the only // mode DANE can run in, because it is the only one where the key for the // next certificate exists before that certificate does @@ -204,7 +211,7 @@ func buildCertProvider(ctx context.Context, cfg config, pool *pgxpool.Pool, secr if err != nil { log.Fatalf("start self-managed ACME: %v", err) } - return provider + return provider, healthy } if cfg.TraefikAcmeDir != "" { @@ -215,14 +222,14 @@ func buildCertProvider(ctx context.Context, cfg config, pool *pgxpool.Pool, secr } go watcher.Watch(ctx.Done(), cfg.CertPollInterval) log.Printf("lambdamail-protocols reading TLS certificates from %s (polling every %s)", cfg.TraefikAcmeDir, cfg.CertPollInterval) - return watcher + return watcher, watcherDegradedCheck(watcher, cfg) } log.Printf("WARNING: could not read the ACME store (%v) - falling back", err) } if cfg.StaticCertPath != "" { if provider, err := tlsprovider.NewLocalFileCertProvider(cfg.StaticCertPath, cfg.StaticCertKeyPath); err == nil { - return provider + return provider, healthy } else { log.Printf("WARNING: could not load the configured TLS certificate: %v", err) } @@ -233,7 +240,35 @@ func buildCertProvider(ctx context.Context, cfg config, pool *pgxpool.Pool, secr if err != nil { log.Fatalf("generate fallback self-signed certificate: %v", err) } - return provider + return provider, func() error { + return errors.New("serving an ephemeral self-signed certificate: no ACME store could be read, so every client sees an untrusted certificate") + } +} + +// healthy is the degraded probe for a certificate source with nothing to +// report. +func healthy() error { return nil } + +// watcherDegradedCheck turns the watcher's state into the conditions PLAN.md +// section 8.4 tabulates: a missing certificate for the mail host, a store that +// has stopped being re-read, and an expiry closing in. +func watcherDegradedCheck(watcher *tlsprovider.AcmeCertWatcher, cfg config) func() error { + // Two missed polls is the smallest gap that cannot be a scheduling + // hiccup, and it is the symptom of risk R2: a watcher gone blind. + staleAfter := 3 * cfg.CertPollInterval + + return func() error { + if !watcher.HasCertificateFor(cfg.PrimaryMailHost) { + return fmt.Errorf("the ACME store holds no certificate for %s: declare a Traefik router for that host", cfg.PrimaryMailHost) + } + if last := watcher.LastReload(); !last.IsZero() && time.Since(last) > staleAfter { + return fmt.Errorf("the ACME store has not been re-read since %s", last.Format(time.RFC3339)) + } + if host, expiry, ok := watcher.EarliestExpiry(); ok && time.Until(expiry) < 24*time.Hour { + return fmt.Errorf("the certificate for %s expires at %s", host, expiry.Format(time.RFC3339)) + } + return nil + } } func relayConfig(cfg config) usecase.RelayConfig { @@ -245,29 +280,6 @@ func relayConfig(cfg config) usecase.RelayConfig { } } -// buildArcSealer signs ARC sets with the domain's RSA DKIM key, which is what -// a verifier resolves through the same selector. -func buildArcSealer(ctx context.Context, cfg config, keys port.DkimKeyRepository) port.ArcSealer { - domain := cfg.domain() - - active, err := keys.FindActiveKeys(ctx, domain) - if err != nil || len(active) == 0 { - log.Printf("WARNING: ARC sealing requested but no DKIM key is active for %s yet; it will start once one is provisioned", domain) - return nil - } - - for _, key := range active { - signer, err := dkim.ParsePrivateKey(key.PrivateKeyPEM) - if err != nil { - continue - } - return arc.NewSealer(domain, key.Selector, cfg.PrimaryMailHost, signer) - } - - log.Printf("WARNING: none of the DKIM keys for %s could be used for ARC", domain) - return nil -} - func buildScanner(cfg config) port.ContentScanner { var scanners []port.ContentScanner if cfg.ClamavAddr != "" { diff --git a/services/protocols/internal/application/port/inbound_message_repository.go b/services/protocols/internal/application/port/inbound_message_repository.go index 408e0bd..b59e341 100644 --- a/services/protocols/internal/application/port/inbound_message_repository.go +++ b/services/protocols/internal/application/port/inbound_message_repository.go @@ -19,12 +19,22 @@ type PersistInboundMessageInput struct { DMARCResult string } -// InboundMessageRepository durably records an accepted inbound message for -// one recipient: allocates the next folder UID, inserts the email_messages +// InboundMessageRepository durably records an accepted inbound message: for +// each recipient it allocates the next folder UID, inserts the email_messages // row, increments the blob's ref_count, and writes the transactional outbox -// event - all in a single transaction (PLAN.md section 6.1, section 9.4). +// event (PLAN.md section 6.1, section 9.4). type InboundMessageRepository interface { - // Persist returns the allocated IMAP UID on success. The caller may only - // report success to the SMTP client after this returns without error. - Persist(ctx context.Context, input PersistInboundMessageInput) (uid int64, err error) + // PersistAll records every recipient's copy in a single transaction and + // returns the allocated IMAP UIDs, in the order the inputs were given. + // + // One transaction for the whole delivery is a correctness requirement, + // not an optimisation. An alias fanning out to several mailboxes is one + // SMTP transaction with one reply: if the second insert failed after the + // first had committed, the sender would be told to retry and the first + // recipient would receive the message twice. Either all recipients get + // it, or the sender is asked to try again. + // + // The caller may only report success to the SMTP client after this + // returns without error. + PersistAll(ctx context.Context, inputs []PersistInboundMessageInput) (uids []int64, err error) } diff --git a/services/protocols/internal/application/usecase/outbound_worker.go b/services/protocols/internal/application/usecase/outbound_worker.go index 055c783..a33ab31 100644 --- a/services/protocols/internal/application/usecase/outbound_worker.go +++ b/services/protocols/internal/application/usecase/outbound_worker.go @@ -229,7 +229,7 @@ func (w *OutboundWorkerUseCase) sendDsn(ctx context.Context, action valueobject. return } - dsnBytes, isLoop := valueobject.BuildDsnReport(action, job.EnvelopeFrom, job.EnvelopeTo, job.ID.String(), reason) + dsnBytes, isLoop := valueobject.BuildDsnReport(action, w.localHost, job.EnvelopeFrom, job.EnvelopeTo, job.ID.String(), reason) if isLoop || dsnBytes == nil { return } @@ -252,7 +252,7 @@ func (w *OutboundWorkerUseCase) sendDsn(ctx context.Context, action valueobject. } _ = w.inboundUC.Handle(ctx, ProcessInboundEmailInput{ - Sender: "postmaster@lambdamail.local", + Sender: "postmaster@" + w.localHost, Recipients: targets, RecipientAddresses: addresses, Body: bytes.NewReader(dsnBytes), diff --git a/services/protocols/internal/application/usecase/process_inbound_email.go b/services/protocols/internal/application/usecase/process_inbound_email.go index 842fb97..116e04c 100644 --- a/services/protocols/internal/application/usecase/process_inbound_email.go +++ b/services/protocols/internal/application/usecase/process_inbound_email.go @@ -171,7 +171,7 @@ func (uc *ProcessInboundEmailUseCase) Handle(ctx context.Context, input ProcessI // having published it. Quarantine is left to the spam filter, which // files rather than refuses. if authResult.DMARC == port.AuthResultFail && authResult.DmarcPolicy == "reject" { - return errors.New("550 5.7.1 DMARC policy rejects this message") + return rejectDmarc() } if authResult.AuthenticationResults != "" { @@ -212,11 +212,11 @@ func (uc *ProcessInboundEmailUseCase) Handle(ctx context.Context, input ProcessI if scanRes != nil { switch scanRes.Verdict { case valueobject.ScanVerdictVirusReject: - return errors.New("554 5.7.1 Virus detected: " + scanRes.VirusName) + return rejectVirus(scanRes.VirusName) case valueobject.ScanVerdictSpamReject: - return errors.New("554 5.7.1 Spam threshold exceeded") + return rejectSpam() case valueobject.ScanVerdictGreylist: - return errors.New("451 4.7.1 Greylisted, please try again later") + return deferGreylisted() case valueobject.ScanVerdictSpamJunk: targetFolder = "Junk" } @@ -229,8 +229,14 @@ func (uc *ProcessInboundEmailUseCase) Handle(ctx context.Context, input ProcessI return err } + // Every recipient is handed to the repository at once so the whole + // delivery commits or none of it does. Persisting them one call at a time + // would mean a failure halfway through leaves the earlier recipients with + // a copy while the sender is told to retry - and the retry delivers to + // them a second time. + persistInputs := make([]port.PersistInboundMessageInput, len(input.Recipients)) for i, recipient := range input.Recipients { - _, err := uc.messages.Persist(ctx, port.PersistInboundMessageInput{ + persistInputs[i] = port.PersistInboundMessageInput{ MailboxID: recipient.ID, Blob: blob, SenderAddress: input.Sender, @@ -239,11 +245,16 @@ func (uc *ProcessInboundEmailUseCase) Handle(ctx context.Context, input ProcessI SPFResult: authResult.SPF, DKIMResult: authResult.DKIM, DMARCResult: authResult.DMARC, - }) - if err != nil { - return err } + } + if _, err := uc.messages.PersistAll(ctx, persistInputs); err != nil { + return err + } + + // Notifications happen only after the commit: an IDLE client told about a + // message that then rolled back would show one that does not exist. + for _, recipient := range input.Recipients { if uc.trackerManager != nil && uc.folders != nil { folderRec, err := uc.folders.FindByName(ctx, recipient.ID.String(), targetFolder) if err == nil && folderRec != nil { diff --git a/services/protocols/internal/application/usecase/process_inbound_email_test.go b/services/protocols/internal/application/usecase/process_inbound_email_test.go index b420d9c..1285a0f 100644 --- a/services/protocols/internal/application/usecase/process_inbound_email_test.go +++ b/services/protocols/internal/application/usecase/process_inbound_email_test.go @@ -105,15 +105,31 @@ func (s *stubBlobStorage) Store(_ context.Context, r io.Reader) (port.BlobRef, e return s.ref, s.err } +// errRepositoryFailure stands in for any persistence error. +var errRepositoryFailure = errors.New("repository is unavailable") + type recordingMessageRepository struct { persisted []port.PersistInboundMessageInput nextUID int64 + // failAfter makes PersistAll reject a batch larger than this many + // recipients, standing in for a partial write in the real repository. + failAfter int } -func (r *recordingMessageRepository) Persist(_ context.Context, input port.PersistInboundMessageInput) (int64, error) { - r.persisted = append(r.persisted, input) - r.nextUID++ - return r.nextUID, nil +func (r *recordingMessageRepository) PersistAll(_ context.Context, inputs []port.PersistInboundMessageInput) ([]int64, error) { + // The real repository commits once for the whole batch, so a failure + // records nothing. Mirroring that here keeps the mock honest. + if r.failAfter > 0 && len(inputs) > r.failAfter { + return nil, errRepositoryFailure + } + + uids := make([]int64, 0, len(inputs)) + for _, input := range inputs { + r.persisted = append(r.persisted, input) + r.nextUID++ + uids = append(uids, r.nextUID) + } + return uids, nil } func TestUseCase_Handle_StoresBlobOnceAndPersistsOncePerRecipient(t *testing.T) { @@ -171,3 +187,31 @@ func TestUseCase_Handle_PropagatesBlobStorageError(t *testing.T) { t.Fatal("expected error when blob storage fails, got nil") } } + +// One SMTP transaction gets one reply, so a fan-out delivery has to be +// all-or-nothing: if the sender is told to retry, no recipient may already +// hold a copy, or the retry duplicates it for them. +func TestUseCase_Handle_DoesNotPartiallyDeliverOnFanOut(t *testing.T) { + blobs := &stubBlobStorage{ref: port.BlobRef{ID: uuid.New(), SHA256: "abc123", SizeBytes: 42}} + messages := &recordingMessageRepository{failAfter: 1} + uc := NewProcessInboundEmailUseCase(nil, blobs, messages) + + recipients := []port.MailboxRecord{ + {ID: uuid.New()}, + {ID: uuid.New()}, + } + + err := uc.Handle(context.Background(), ProcessInboundEmailInput{ + Sender: "sender@remote.test", + Recipients: recipients, + RecipientAddresses: []string{"alias@example.test", "alias@example.test"}, + Body: bytes.NewBufferString("Subject: hi\r\n\r\nbody\r\n"), + }) + + if err == nil { + t.Fatal("Handle reported success even though persistence failed") + } + if len(messages.persisted) != 0 { + t.Errorf("%d recipients were left with a copy after a failed delivery; the sender's retry would duplicate it", len(messages.persisted)) + } +} diff --git a/services/protocols/internal/application/usecase/smtp_rejection.go b/services/protocols/internal/application/usecase/smtp_rejection.go new file mode 100644 index 0000000..61b6da7 --- /dev/null +++ b/services/protocols/internal/application/usecase/smtp_rejection.go @@ -0,0 +1,55 @@ +package usecase + +import "fmt" + +// SmtpRejection is a use case's decision to refuse a message, carrying the +// reply the SMTP layer must send back. +// +// The decision belongs to the application layer - only it knows that a virus +// is permanent and a greylist is temporary - but the wire format belongs to +// the presentation layer. This type is the seam: the use case names the +// outcome, the adapter renders it. Encoding the reply in the error *text* and +// re-parsing it downstream would make a reworded message silently change the +// code a remote MTA receives, turning a permanent refusal into a retry loop. +type SmtpRejection struct { + // Code is the three-digit reply code (RFC 5321 section 4.2.1). + Code int + // EnhancedCode is the class.subject.detail triplet of RFC 3463. + EnhancedCode [3]int + // Message is the human-readable text, without any code prefix. + Message string +} + +func (e *SmtpRejection) Error() string { + return fmt.Sprintf("%d %d.%d.%d %s", + e.Code, e.EnhancedCode[0], e.EnhancedCode[1], e.EnhancedCode[2], e.Message) +} + +// IsPermanent reports whether the sender must give up rather than retry. +func (e *SmtpRejection) IsPermanent() bool { + return e.Code >= 500 && e.Code < 600 +} + +// rejectDmarc refuses a message whose sender publishes p=reject and whose +// DMARC evaluation failed. +func rejectDmarc() *SmtpRejection { + return &SmtpRejection{Code: 550, EnhancedCode: [3]int{5, 7, 1}, Message: "DMARC policy rejects this message"} +} + +// rejectVirus refuses an infected message. It is permanent: the same bytes +// will still be infected on a retry. +func rejectVirus(virusName string) *SmtpRejection { + return &SmtpRejection{Code: 554, EnhancedCode: [3]int{5, 7, 1}, Message: "Virus detected: " + virusName} +} + +// rejectSpam refuses a message that scored above the reject threshold. +func rejectSpam() *SmtpRejection { + return &SmtpRejection{Code: 554, EnhancedCode: [3]int{5, 7, 1}, Message: "Spam threshold exceeded"} +} + +// deferGreylisted asks the sender to try again. It is deliberately temporary: +// greylisting works precisely because a legitimate MTA retries and a spam +// cannon does not. +func deferGreylisted() *SmtpRejection { + return &SmtpRejection{Code: 451, EnhancedCode: [3]int{4, 7, 1}, Message: "Greylisted, please try again later"} +} diff --git a/services/protocols/internal/domain/valueobject/dsn_bounce.go b/services/protocols/internal/domain/valueobject/dsn_bounce.go index d322c46..71490c2 100644 --- a/services/protocols/internal/domain/valueobject/dsn_bounce.go +++ b/services/protocols/internal/domain/valueobject/dsn_bounce.go @@ -13,14 +13,32 @@ const ( DsnActionDelayed DsnAction = "delayed" ) -// BuildDsnReport constructs an RFC 3464 compliant delivery status notification report payload. -// Returns (payload, isLoopPrevented). If envelopeFrom is empty or "<>", isLoopPrevented is true and payload is nil. -func BuildDsnReport(action DsnAction, envelopeFrom string, envelopeTo string, messageID string, reason string) ([]byte, bool) { +// defaultReportingMTA is the last resort when no mail host was configured. It +// is deliberately obviously wrong rather than plausible, so a misconfiguration +// shows up in the report instead of hiding in it. +const defaultReportingMTA = "mail.lambdamail.local" + +// BuildDsnReport constructs an RFC 3464 compliant delivery status notification +// report payload. +// +// reportingMTA is this server's own mail host. It has to be the real one: a +// notification whose From and Reporting-MTA name a domain that does not +// resolve is itself undeliverable at the next hop, which loses exactly the +// message the sender most needs to see. +// +// Returns (payload, isLoopPrevented). If envelopeFrom is empty or "<>", +// isLoopPrevented is true and payload is nil. +func BuildDsnReport(action DsnAction, reportingMTA string, envelopeFrom string, envelopeTo string, messageID string, reason string) ([]byte, bool) { cleanFrom := strings.Trim(strings.TrimSpace(envelopeFrom), "<>") if cleanFrom == "" { return nil, true // Loop prevention per RFC 5321 section 4.5.5 } + reportingMTA = strings.TrimSpace(reportingMTA) + if reportingMTA == "" { + reportingMTA = defaultReportingMTA + } + boundary := fmt.Sprintf("=_Boundary_DSN_%d", time.Now().UnixNano()) nowStr := time.Now().Format(time.RFC1123Z) @@ -32,10 +50,15 @@ func BuildDsnReport(action DsnAction, envelopeFrom string, envelopeTo string, me } var sb strings.Builder - sb.WriteString(fmt.Sprintf("From: Mail Delivery Subsystem \r\n")) + sb.WriteString(fmt.Sprintf("From: Mail Delivery Subsystem \r\n", reportingMTA)) sb.WriteString(fmt.Sprintf("To: <%s>\r\n", cleanFrom)) sb.WriteString(fmt.Sprintf("Subject: %s\r\n", subject)) sb.WriteString(fmt.Sprintf("Date: %s\r\n", nowStr)) + sb.WriteString(fmt.Sprintf("Message-ID: \r\n", messageID, reportingMTA)) + // RFC 3834 section 5: an automatic notification has to say so, or a + // vacation responder on the other side answers it and the two bounce back + // and forth forever. + sb.WriteString("Auto-Submitted: auto-replied\r\n") sb.WriteString(fmt.Sprintf("Content-Type: multipart/report; report-type=delivery-status; boundary=\"%s\"\r\n\r\n", boundary)) // Part 1: Human-readable narrative @@ -50,7 +73,7 @@ func BuildDsnReport(action DsnAction, envelopeFrom string, envelopeTo string, me // Part 2: Machine-readable delivery-status sb.WriteString(fmt.Sprintf("--%s\r\n", boundary)) sb.WriteString("Content-Type: message/delivery-status\r\n\r\n") - sb.WriteString("Reporting-MTA: dns; mail.lambdamail.local\r\n") + sb.WriteString(fmt.Sprintf("Reporting-MTA: dns; %s\r\n", reportingMTA)) sb.WriteString(fmt.Sprintf("Arrival-Date: %s\r\n\r\n", nowStr)) sb.WriteString(fmt.Sprintf("Final-Recipient: rfc822; %s\r\n", envelopeTo)) diff --git a/services/protocols/internal/domain/valueobject/dsn_bounce_test.go b/services/protocols/internal/domain/valueobject/dsn_bounce_test.go index c15b50e..1b501f5 100644 --- a/services/protocols/internal/domain/valueobject/dsn_bounce_test.go +++ b/services/protocols/internal/domain/valueobject/dsn_bounce_test.go @@ -6,19 +6,19 @@ import ( ) func TestBuildDsnReport_PreventsBounceLoopForEmptySender(t *testing.T) { - _, isLoop := BuildDsnReport(DsnActionFailed, "", "user@external.test", "msg123", "550 User unknown") + _, isLoop := BuildDsnReport(DsnActionFailed, "mail.local.test", "", "user@external.test", "msg123", "550 User unknown") if !isLoop { t.Errorf("expected loop prevention for empty sender") } - _, isLoop = BuildDsnReport(DsnActionFailed, "<>", "user@external.test", "msg123", "550 User unknown") + _, isLoop = BuildDsnReport(DsnActionFailed, "mail.local.test", "<>", "user@external.test", "msg123", "550 User unknown") if !isLoop { t.Errorf("expected loop prevention for <> sender") } } func TestBuildDsnReport_GeneratesValidMIME(t *testing.T) { - payload, isLoop := BuildDsnReport(DsnActionFailed, "sender@local.test", "recipient@remote.test", "msg123", "550 User unknown") + payload, isLoop := BuildDsnReport(DsnActionFailed, "mail.local.test", "sender@local.test", "recipient@remote.test", "msg123", "550 User unknown") if isLoop || payload == nil { t.Fatalf("unexpected loop signal for valid sender") } @@ -34,3 +34,31 @@ func TestBuildDsnReport_GeneratesValidMIME(t *testing.T) { t.Errorf("missing Diagnostic-Code: %s", raw) } } + +// A notification whose From and Reporting-MTA name a domain that does not +// resolve is itself undeliverable, which loses the very message the sender +// most needs to see. +func TestBuildDsnReport_UsesTheConfiguredMailHost(t *testing.T) { + payload, _ := BuildDsnReport(DsnActionFailed, "mail.example.test", "sender@example.test", "recipient@remote.test", "job-1", "550 User unknown") + + raw := string(payload) + if !strings.Contains(raw, "From: Mail Delivery Subsystem ") { + t.Errorf("From does not use the configured mail host: %s", raw) + } + if !strings.Contains(raw, "Reporting-MTA: dns; mail.example.test") { + t.Errorf("Reporting-MTA does not use the configured mail host: %s", raw) + } + if strings.Contains(raw, "lambdamail.local") { + t.Errorf("the placeholder host leaked into the report: %s", raw) + } +} + +// RFC 3834 section 5: without Auto-Submitted a vacation responder answers the +// bounce and the two loop against each other. +func TestBuildDsnReport_MarksItselfAutomatic(t *testing.T) { + payload, _ := BuildDsnReport(DsnActionDelayed, "mail.example.test", "sender@example.test", "recipient@remote.test", "job-1", "451 deferred") + + if !strings.Contains(string(payload), "Auto-Submitted: auto-replied") { + t.Errorf("the report is not marked as automatic: %s", payload) + } +} diff --git a/services/protocols/internal/infrastructure/arc/lazy_sealer.go b/services/protocols/internal/infrastructure/arc/lazy_sealer.go new file mode 100644 index 0000000..51d8811 --- /dev/null +++ b/services/protocols/internal/infrastructure/arc/lazy_sealer.go @@ -0,0 +1,75 @@ +package arc + +import ( + "context" + "crypto" + "sync" + + "lambdamail/protocols/internal/application/port" +) + +// KeyParser turns a stored private key into a signer. It is injected so this +// package stays independent of how DKIM keys are encoded. +type KeyParser func(privateKeyPEM []byte) (crypto.Signer, error) + +// LazySealer resolves the signing key on first use instead of at construction. +// +// The composition root builds the sealer before the DKIM provisioner has run, +// so on a first boot no key exists yet. Binding to that empty result would +// disable ARC until the process was restarted by hand - the failure PLAN.md +// section 5 cannot tolerate, because the whole point of ARC is that it is +// always applied, not applied when the ordering happened to work out. +type LazySealer struct { + keys port.DkimKeyRepository + parse KeyParser + domain string + authServID string + + mu sync.Mutex + resolved port.ArcSealer +} + +// NewLazySealer returns a sealer that binds to the domain's active DKIM key +// the first time a message needs sealing. +func NewLazySealer(keys port.DkimKeyRepository, parse KeyParser, domain, authServID string) *LazySealer { + return &LazySealer{keys: keys, parse: parse, domain: domain, authServID: authServID} +} + +// Seal delegates to the underlying sealer, resolving it if needed. Until a key +// exists the message is returned untouched: an unsealed message is delivered, +// which is strictly better than a rejected one. +func (l *LazySealer) Seal(ctx context.Context, message []byte, authResult port.InboundAuthResult) ([]byte, error) { + sealer, err := l.sealer(ctx) + if err != nil || sealer == nil { + return message, err + } + return sealer.Seal(ctx, message, authResult) +} + +func (l *LazySealer) sealer(ctx context.Context) (port.ArcSealer, error) { + l.mu.Lock() + defer l.mu.Unlock() + + if l.resolved != nil { + return l.resolved, nil + } + + active, err := l.keys.FindActiveKeys(ctx, l.domain) + if err != nil { + return nil, err + } + + for _, key := range active { + signer, err := l.parse(key.PrivateKeyPEM) + if err != nil { + continue + } + l.resolved = NewSealer(l.domain, key.Selector, l.authServID, signer) + return l.resolved, nil + } + + // Still no usable key. Leaving resolved nil means the next message tries + // again, which is what turns "not provisioned yet" into a delay rather + // than a permanent loss of sealing. + return nil, nil +} diff --git a/services/protocols/internal/infrastructure/arc/lazy_sealer_test.go b/services/protocols/internal/infrastructure/arc/lazy_sealer_test.go new file mode 100644 index 0000000..1d46595 --- /dev/null +++ b/services/protocols/internal/infrastructure/arc/lazy_sealer_test.go @@ -0,0 +1,108 @@ +package arc + +import ( + "context" + "crypto" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" + "strings" + "sync" + "testing" + + "lambdamail/protocols/internal/application/port" +) + +// stubKeyRepo serves whatever the test decides is provisioned at that moment. +type stubKeyRepo struct { + mu sync.Mutex + keys []port.DkimSigningKey + // calls counts lookups, to prove the resolved key is cached. + calls int +} + +func (s *stubKeyRepo) FindActiveKeys(_ context.Context, _ string) ([]port.DkimSigningKey, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.calls++ + return s.keys, nil +} + +func (s *stubKeyRepo) provision(t *testing.T) { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generate key: %v", err) + } + pemBytes := pem.EncodeToMemory(&pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(key), + }) + + s.mu.Lock() + defer s.mu.Unlock() + s.keys = []port.DkimSigningKey{{ + DomainName: "example.test", + Selector: "lmail", + Algorithm: "rsa2048", + PrivateKeyPEM: pemBytes, + }} +} + +func parseTestKey(pemBytes []byte) (crypto.Signer, error) { + block, _ := pem.Decode(pemBytes) + return x509.ParsePKCS1PrivateKey(block.Bytes) +} + +const testMessage = "From: sender@remote.test\r\n" + + "To: user@example.test\r\n" + + "Subject: hello\r\n" + + "\r\n" + + "body\r\n" + +// The composition root builds the sealer before the DKIM provisioner has run. +// Binding at construction would leave ARC off until the next restart. +func TestLazySealer_StartsSealingOnceAKeyIsProvisioned(t *testing.T) { + repo := &stubKeyRepo{} + sealer := NewLazySealer(repo, parseTestKey, "example.test", "mail.example.test") + ctx := context.Background() + authResult := port.InboundAuthResult{SPF: port.AuthResultPass} + + // No key yet: the message goes through unsealed rather than failing. + out, err := sealer.Seal(ctx, []byte(testMessage), authResult) + if err != nil { + t.Fatalf("Seal without a key: %v", err) + } + if strings.Contains(string(out), "ARC-Seal") { + t.Fatal("a message was sealed before any key existed") + } + + repo.provision(t) + + out, err = sealer.Seal(ctx, []byte(testMessage), authResult) + if err != nil { + t.Fatalf("Seal after provisioning: %v", err) + } + if !strings.Contains(string(out), "ARC-Seal") { + t.Fatal("sealing never started after the key was provisioned") + } +} + +// Once bound, the key is not looked up again on every message. +func TestLazySealer_CachesTheResolvedKey(t *testing.T) { + repo := &stubKeyRepo{} + repo.provision(t) + sealer := NewLazySealer(repo, parseTestKey, "example.test", "mail.example.test") + ctx := context.Background() + + for range 3 { + if _, err := sealer.Seal(ctx, []byte(testMessage), port.InboundAuthResult{}); err != nil { + t.Fatalf("Seal: %v", err) + } + } + + if repo.calls != 1 { + t.Errorf("FindActiveKeys called %d times, want 1", repo.calls) + } +} diff --git a/services/protocols/internal/infrastructure/postgres/copy_repository_test.go b/services/protocols/internal/infrastructure/postgres/copy_repository_test.go index ea397db..cd0beba 100644 --- a/services/protocols/internal/infrastructure/postgres/copy_repository_test.go +++ b/services/protocols/internal/infrastructure/postgres/copy_repository_test.go @@ -42,7 +42,7 @@ func TestCopyRepository_CopyMessages_AllocatesNewUIDsAndPreservesFlags(t *testin } blobID := uuid.New() _, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes, ref_count) VALUES ($1, $2, 'local', '/tmp/x', 10, 1)`, - blobID, uuid.New().String()+uuid.New().String()[:32]) + blobID, testBlobDigest()) if err != nil { t.Fatalf("seed blob: %v", err) } diff --git a/services/protocols/internal/infrastructure/postgres/expunge_repository_test.go b/services/protocols/internal/infrastructure/postgres/expunge_repository_test.go index 88303bd..76fd6d5 100644 --- a/services/protocols/internal/infrastructure/postgres/expunge_repository_test.go +++ b/services/protocols/internal/infrastructure/postgres/expunge_repository_test.go @@ -35,7 +35,7 @@ func TestExpungeRepository_Expunge_SetsExpungedAtAndDecrementsCounters(t *testin } blobID := uuid.New() _, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`, - blobID, uuid.New().String()+uuid.New().String()[:32]) + blobID, testBlobDigest()) if err != nil { t.Fatalf("seed blob: %v", err) } diff --git a/services/protocols/internal/infrastructure/postgres/flag_repository_test.go b/services/protocols/internal/infrastructure/postgres/flag_repository_test.go index 80a1d21..668c36c 100644 --- a/services/protocols/internal/infrastructure/postgres/flag_repository_test.go +++ b/services/protocols/internal/infrastructure/postgres/flag_repository_test.go @@ -37,7 +37,7 @@ func TestFlagRepository_SetFlags_AddThenDelThenSet(t *testing.T) { } blobID := uuid.New() _, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`, - blobID, "1111110000000000000000000000000000000000000000000000000000000000"[:64]) + blobID, testBlobDigest()) if err != nil { t.Fatalf("seed blob: %v", err) } diff --git a/services/protocols/internal/infrastructure/postgres/inbound_message_repository.go b/services/protocols/internal/infrastructure/postgres/inbound_message_repository.go index b32f37a..e7f7905 100644 --- a/services/protocols/internal/infrastructure/postgres/inbound_message_repository.go +++ b/services/protocols/internal/infrastructure/postgres/inbound_message_repository.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" "lambdamail/protocols/internal/application/port" @@ -21,13 +22,37 @@ func NewInboundMessageRepository(pool *pgxpool.Pool) *InboundMessageRepository { return &InboundMessageRepository{pool: pool} } -func (r *InboundMessageRepository) Persist(ctx context.Context, input port.PersistInboundMessageInput) (int64, error) { +// PersistAll records every recipient's copy in one transaction, so a delivery +// that fans out is all-or-nothing and a retried SMTP transaction cannot leave +// an earlier recipient with a duplicate. +func (r *InboundMessageRepository) PersistAll(ctx context.Context, inputs []port.PersistInboundMessageInput) ([]int64, error) { + if len(inputs) == 0 { + return nil, nil + } + tx, err := r.pool.Begin(ctx) if err != nil { - return 0, err + return nil, err } defer tx.Rollback(ctx) // no-op if Commit succeeds + uids := make([]int64, 0, len(inputs)) + for _, input := range inputs { + uid, err := persistOne(ctx, tx, input) + if err != nil { + return nil, err + } + uids = append(uids, uid) + } + + if err := tx.Commit(ctx); err != nil { + return nil, fmt.Errorf("commit: %w", err) + } + return uids, nil +} + +// persistOne writes one recipient's copy inside the caller's transaction. +func persistOne(ctx context.Context, tx pgx.Tx, input port.PersistInboundMessageInput) (int64, error) { targetFolder := input.TargetFolderName if targetFolder == "" { targetFolder = "INBOX" @@ -35,7 +60,7 @@ func (r *InboundMessageRepository) Persist(ctx context.Context, input port.Persi var folderID string var uid int64 - err = tx.QueryRow(ctx, ` + err := tx.QueryRow(ctx, ` SELECT id, uid_next FROM folders WHERE mailbox_id = $1 AND (special_use = LOWER($2) OR LOWER(name) = LOWER($2)) ORDER BY special_use IS NOT NULL DESC @@ -84,8 +109,5 @@ func (r *InboundMessageRepository) Persist(ctx context.Context, input port.Persi return 0, fmt.Errorf("insert outbox event: %w", err) } - if err := tx.Commit(ctx); err != nil { - return 0, fmt.Errorf("commit: %w", err) - } return uid, nil } diff --git a/services/protocols/internal/infrastructure/postgres/inbound_message_repository_test.go b/services/protocols/internal/infrastructure/postgres/inbound_message_repository_test.go index ada6fa1..500f296 100644 --- a/services/protocols/internal/infrastructure/postgres/inbound_message_repository_test.go +++ b/services/protocols/internal/infrastructure/postgres/inbound_message_repository_test.go @@ -38,7 +38,7 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T) blobID := uuid.New() _, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`, - blobID, "deadbeef00000000000000000000000000000000000000000000000000000000"[:64]) + blobID, testBlobDigest()) if err != nil { t.Fatalf("seed blob: %v", err) } @@ -46,22 +46,23 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T) repo := NewInboundMessageRepository(pool) blob := port.BlobRef{ID: blobID, SHA256: "deadbeef", SizeBytes: 10} - firstUID, err := repo.Persist(ctx, port.PersistInboundMessageInput{ + firstUIDs, err := repo.PersistAll(ctx, []port.PersistInboundMessageInput{{ MailboxID: mailboxID, Blob: blob, SenderAddress: "sender@example.test", RecipientAddress: "user@msgrepo-test-" + domainID.String() + ".invalid", SPFResult: "none", DKIMResult: "none", DMARCResult: "none", - }) + }}) if err != nil { - t.Fatalf("first Persist: %v", err) + t.Fatalf("first PersistAll: %v", err) } - secondUID, err := repo.Persist(ctx, port.PersistInboundMessageInput{ + secondUIDs, err := repo.PersistAll(ctx, []port.PersistInboundMessageInput{{ MailboxID: mailboxID, Blob: blob, SenderAddress: "sender2@example.test", RecipientAddress: "user@msgrepo-test-" + domainID.String() + ".invalid", SPFResult: "none", DKIMResult: "none", DMARCResult: "none", - }) + }}) if err != nil { - t.Fatalf("second Persist: %v", err) + t.Fatalf("second PersistAll: %v", err) } + firstUID, secondUID := firstUIDs[0], secondUIDs[0] if secondUID <= firstUID { t.Errorf("secondUID (%d) must be strictly greater than firstUID (%d) - RFC 3501 section 2.3.1.1", secondUID, firstUID) } @@ -71,7 +72,7 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T) t.Fatalf("query ref_count: %v", err) } if refCount != 2 { - t.Errorf("ref_count = %d, want 2 after two Persist calls referencing the same blob", refCount) + t.Errorf("ref_count = %d, want 2 after two PersistAll calls referencing the same blob", refCount) } var unreadCount, totalCount int @@ -79,10 +80,10 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T) t.Fatalf("query folder counters: %v", err) } if unreadCount != 2 { - t.Errorf("folder unread_count = %d, want 2 after two Persist calls", unreadCount) + t.Errorf("folder unread_count = %d, want 2 after two PersistAll calls", unreadCount) } if totalCount != 2 { - t.Errorf("folder total_count = %d, want 2 after two Persist calls", totalCount) + t.Errorf("folder total_count = %d, want 2 after two PersistAll calls", totalCount) } var usedBytes int64 @@ -90,7 +91,7 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T) t.Fatalf("query mailbox used_bytes: %v", err) } if usedBytes != blob.SizeBytes*2 { - t.Errorf("mailbox used_bytes = %d, want %d (two Persist calls of %d bytes each)", usedBytes, blob.SizeBytes*2, blob.SizeBytes) + t.Errorf("mailbox used_bytes = %d, want %d (two PersistAll calls of %d bytes each)", usedBytes, blob.SizeBytes*2, blob.SizeBytes) } } @@ -118,18 +119,88 @@ func TestInboundMessageRepository_Persist_ReturnsErrorWhenNoInboxFolderExists(t blobID := uuid.New() _, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`, - blobID, "cafebabe00000000000000000000000000000000000000000000000000000000"[:64]) + blobID, testBlobDigest()) if err != nil { t.Fatalf("seed blob: %v", err) } repo := NewInboundMessageRepository(pool) - _, err = repo.Persist(ctx, port.PersistInboundMessageInput{ + _, err = repo.PersistAll(ctx, []port.PersistInboundMessageInput{{ MailboxID: mailboxID, Blob: port.BlobRef{ID: blobID, SizeBytes: 10}, SenderAddress: "sender@example.test", RecipientAddress: "nofolder@example.test", SPFResult: "none", DKIMResult: "none", DMARCResult: "none", - }) + }}) if err == nil { t.Fatal("expected an error when the mailbox has no INBOX folder, got nil") } } + +// An alias fanning out is one SMTP transaction with one reply. If a later +// recipient's insert fails, the earlier ones must not remain: the sender is +// told to retry, and the retry would deliver to them a second time. +func TestInboundMessageRepository_PersistAll_RollsBackEveryRecipientOnFailure(t *testing.T) { + pool := testPool(t) + defer pool.Close() + ctx := context.Background() + + domainID := uuid.New() + domainName := "fanout-test-" + domainID.String() + ".invalid" + if _, err := pool.Exec(ctx, `INSERT INTO domains (id, name, punycode_name) VALUES ($1, $2, $3)`, + domainID, domainName, domainName); err != nil { + t.Fatalf("seed domain: %v", err) + } + defer pool.Exec(ctx, `DELETE FROM domains WHERE id = $1`, domainID) + + // The first mailbox has an INBOX, the second deliberately does not, so + // persisting the pair fails on the second entry. + goodMailboxID, badMailboxID := uuid.New(), uuid.New() + for i, id := range []uuid.UUID{goodMailboxID, badMailboxID} { + local := []string{"good", "bad"}[i] + if _, err := pool.Exec(ctx, `INSERT INTO mailboxes (id, domain_id, local_part, email_address, password_hash) VALUES ($1, $2, $3, $4, '$argon2id$v=19$m=65536,t=3,p=4$c29tZXNhbHQ$RdescudvJCsgt3ub+b+dWRWJTmaaJObG')`, + id, domainID, local, local+"@"+domainName); err != nil { + t.Fatalf("seed mailbox %s: %v", local, err) + } + } + + folderID := uuid.New() + if _, err := pool.Exec(ctx, `INSERT INTO folders (id, mailbox_id, name, special_use) VALUES ($1, $2, 'INBOX', 'inbox')`, + folderID, goodMailboxID); err != nil { + t.Fatalf("seed folder: %v", err) + } + + blobID := uuid.New() + blobDigest := testBlobDigest() + if _, err := pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`, + blobID, blobDigest); err != nil { + t.Fatalf("seed blob: %v", err) + } + defer pool.Exec(ctx, `DELETE FROM message_blobs WHERE id = $1`, blobID) + blob := port.BlobRef{ID: blobID, SHA256: blobDigest, SizeBytes: 10} + + repo := NewInboundMessageRepository(pool) + _, err := repo.PersistAll(ctx, []port.PersistInboundMessageInput{ + {MailboxID: goodMailboxID, Blob: blob, SenderAddress: "sender@remote.test", RecipientAddress: "alias@" + domainName, + SPFResult: "none", DKIMResult: "none", DMARCResult: "none"}, + {MailboxID: badMailboxID, Blob: blob, SenderAddress: "sender@remote.test", RecipientAddress: "alias@" + domainName, + SPFResult: "none", DKIMResult: "none", DMARCResult: "none"}, + }) + if err == nil { + t.Fatal("PersistAll succeeded even though the second recipient has no INBOX") + } + + var messageCount int + if err := pool.QueryRow(ctx, `SELECT count(*) FROM email_messages WHERE mailbox_id = $1`, goodMailboxID).Scan(&messageCount); err != nil { + t.Fatalf("count messages: %v", err) + } + if messageCount != 0 { + t.Errorf("the first recipient kept %d message(s) after a failed fan-out; the sender's retry would duplicate them", messageCount) + } + + var refCount int + if err := pool.QueryRow(ctx, `SELECT ref_count FROM message_blobs WHERE id = $1`, blobID).Scan(&refCount); err != nil { + t.Fatalf("query ref_count: %v", err) + } + if refCount != 0 { + t.Errorf("ref_count = %d after a rolled-back delivery, want 0", refCount) + } +} diff --git a/services/protocols/internal/infrastructure/postgres/mailbox_repository_test.go b/services/protocols/internal/infrastructure/postgres/mailbox_repository_test.go index b6b11c4..04d804e 100644 --- a/services/protocols/internal/infrastructure/postgres/mailbox_repository_test.go +++ b/services/protocols/internal/infrastructure/postgres/mailbox_repository_test.go @@ -3,6 +3,7 @@ package postgres import ( "context" "os" + "strings" "testing" "github.com/google/uuid" @@ -309,3 +310,10 @@ func TestMailboxRepository_ResolveDeliveryTargets_SkipsAliasDestinationThatIsNot t.Errorf("targets = %+v, want empty slice - external forwarding destinations are not internal mailboxes and outbound delivery does not exist yet", targets) } } + +// testBlobDigest produces a unique 64-character value for message_blobs' +// content_sha256, which is UNIQUE in the schema. A hardcoded digest makes the +// test pass exactly once per database and fail on every re-run. +func testBlobDigest() string { + return strings.ReplaceAll(uuid.New().String()+uuid.New().String(), "-", "")[:64] +} diff --git a/services/protocols/internal/infrastructure/postgres/message_query_repository_test.go b/services/protocols/internal/infrastructure/postgres/message_query_repository_test.go index 3eff5ce..e98ac18 100644 --- a/services/protocols/internal/infrastructure/postgres/message_query_repository_test.go +++ b/services/protocols/internal/infrastructure/postgres/message_query_repository_test.go @@ -35,7 +35,7 @@ func TestMessageQueryRepository_ListMessages_ReturnsRowsOrderedByUID(t *testing. } blobID := uuid.New() _, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 42)`, - blobID, "abcdef0000000000000000000000000000000000000000000000000000000000"[:64]) + blobID, testBlobDigest()) if err != nil { t.Fatalf("seed blob: %v", err) } diff --git a/services/protocols/internal/infrastructure/tls/acme_cert_watcher.go b/services/protocols/internal/infrastructure/tls/acme_cert_watcher.go index 995dad4..63fd651 100644 --- a/services/protocols/internal/infrastructure/tls/acme_cert_watcher.go +++ b/services/protocols/internal/infrastructure/tls/acme_cert_watcher.go @@ -3,6 +3,7 @@ package tlsprovider import ( "crypto/sha256" "crypto/tls" + "crypto/x509" "encoding/base64" "encoding/json" "errors" @@ -37,8 +38,15 @@ type AcmeCertWatcher struct { // using a consistent map. certificates atomic.Pointer[map[string]*tls.Certificate] - lastHash atomic.Pointer[[32]byte] + lastHash atomic.Pointer[[32]byte] + // lastReloadAt is the last time the store was successfully read and found + // usable, whether or not its content had changed. It is what PLAN.md risk + // R2 monitors: acme.json normally sits untouched for sixty days, so a + // timestamp that only advanced on change could not distinguish a healthy + // idle watcher from one that has gone blind. lastReloadAt atomic.Pointer[time.Time] + // lastChangeAt is the last time the certificates themselves were swapped. + lastChangeAt atomic.Pointer[time.Time] // unknownSNI counts handshakes for names we hold no certificate for; it // backs the tls_unknown_sni_total metric of PLAN.md section 8.4. @@ -110,31 +118,80 @@ func (w *AcmeCertWatcher) reload() error { return fmt.Errorf("read %s: %w", w.path, err) } - hash := sha256.Sum256(raw) - if previous := w.lastHash.Load(); previous != nil && *previous == hash { - return nil - } - - certificates, err := parseAcmeStore(raw) - if err != nil { - return err - } - if len(certificates) == 0 { - return fmt.Errorf("acme watcher: %s holds no usable certificate", w.path) - } - - w.certificates.Store(&certificates) - w.lastHash.Store(&hash) now := time.Now() - w.lastReloadAt.Store(&now) + hash := sha256.Sum256(raw) + if previous := w.lastHash.Load(); previous == nil || *previous != hash { + certificates, err := parseAcmeStore(raw, now) + if err != nil { + return err + } + if len(certificates) == 0 { + return fmt.Errorf("acme watcher: %s holds no usable certificate", w.path) + } + + w.certificates.Store(&certificates) + w.lastHash.Store(&hash) + w.lastChangeAt.Store(&now) + } + + // Expiry is checked on every pass, not only on change: a certificate ages + // out while the file it lives in stays byte-identical. + w.reportExpiry(now) + + w.lastReloadAt.Store(&now) return nil } +// reportExpiry raises the alerts of PLAN.md section 8.4 for certificates that +// are close to expiring. Renewal is Traefik's job, so all this side can do is +// make the failure loud before it becomes an outage. +func (w *AcmeCertWatcher) reportExpiry(now time.Time) { + host, expiry, ok := w.EarliestExpiry() + if !ok { + return + } + + switch remaining := expiry.Sub(now); { + case remaining <= 0: + log.Printf("CRITICAL: the certificate for %s expired at %s and Traefik has not replaced it", host, expiry.Format(time.RFC3339)) + case remaining < 24*time.Hour: + log.Printf("CRITICAL: the certificate for %s expires in %s", host, remaining.Round(time.Minute)) + case remaining < 7*24*time.Hour: + log.Printf("WARNING: the certificate for %s expires in %s", host, remaining.Round(time.Hour)) + } +} + +// EarliestExpiry reports the soonest expiry across the loaded certificates, +// which is the one that decides when the deployment breaks. +func (w *AcmeCertWatcher) EarliestExpiry() (string, time.Time, bool) { + loaded := w.certificates.Load() + if loaded == nil { + return "", time.Time{}, false + } + + var ( + soonestHost string + soonest time.Time + ) + for host, cert := range *loaded { + if cert.Leaf == nil { + continue + } + if soonest.IsZero() || cert.Leaf.NotAfter.Before(soonest) { + soonest, soonestHost = cert.Leaf.NotAfter, host + } + } + if soonest.IsZero() { + return "", time.Time{}, false + } + return soonestHost, soonest, true +} + // parseAcmeStore walks every resolver in the file. A single unusable entry - // an expired certificate, or one whose key does not match - is skipped rather // than failing the whole load, so one stale domain cannot take the server down. -func parseAcmeStore(raw []byte) (map[string]*tls.Certificate, error) { +func parseAcmeStore(raw []byte, now time.Time) (map[string]*tls.Certificate, error) { var store acmeFile if err := json.Unmarshal(raw, &store); err != nil { return nil, fmt.Errorf("parse acme store: %w", err) @@ -161,6 +218,23 @@ func parseAcmeStore(raw []byte) (map[string]*tls.Certificate, error) { continue } + // X509KeyPair leaves Leaf nil. Parsing it here is what makes the + // expiry checks below and in reportExpiry possible at all. + leaf, err := x509.ParseCertificate(pair.Certificate[0]) + if err != nil { + log.Printf("acme watcher: skipping %s in resolver %q: leaf certificate is unparseable: %v", entry.Domain.Main, resolverName, err) + continue + } + // An expired entry is worse than a missing one: it would be served + // in a handshake every peer then rejects. Skipping it lets the SNI + // fallback or the self-signed provider take over instead + // (PLAN.md section 8.2, trap 3). + if now.After(leaf.NotAfter) { + log.Printf("acme watcher: skipping %s in resolver %q: the certificate expired at %s", entry.Domain.Main, resolverName, leaf.NotAfter.Format(time.RFC3339)) + continue + } + pair.Leaf = leaf + for _, name := range append([]string{entry.Domain.Main}, entry.Domain.SANs...) { if name != "" { certificates[strings.ToLower(name)] = &pair @@ -210,9 +284,9 @@ func wildcardOf(name string) string { return "" } -// LastReload reports when the store was last successfully refreshed. A -// timestamp that stops advancing is the signal that the watcher went blind -// (PLAN.md risk R2). +// LastReload reports when the store was last read successfully, changed or +// not. A timestamp that stops advancing is the signal that the watcher went +// blind (PLAN.md risk R2). func (w *AcmeCertWatcher) LastReload() time.Time { if t := w.lastReloadAt.Load(); t != nil { return *t @@ -220,6 +294,15 @@ func (w *AcmeCertWatcher) LastReload() time.Time { return time.Time{} } +// LastChange reports when the served certificates were last swapped, which is +// how a renewal that never arrived is detected. +func (w *AcmeCertWatcher) LastChange() time.Time { + if t := w.lastChangeAt.Load(); t != nil { + return *t + } + return time.Time{} +} + // UnknownSNICount backs the tls_unknown_sni_total metric. func (w *AcmeCertWatcher) UnknownSNICount() int64 { return w.unknownSNI.Load() diff --git a/services/protocols/internal/infrastructure/tls/acme_cert_watcher_test.go b/services/protocols/internal/infrastructure/tls/acme_cert_watcher_test.go index 102e58f..d076fb6 100644 --- a/services/protocols/internal/infrastructure/tls/acme_cert_watcher_test.go +++ b/services/protocols/internal/infrastructure/tls/acme_cert_watcher_test.go @@ -20,6 +20,13 @@ import ( // makeCertPEM issues a self-signed certificate for the given names. func makeCertPEM(t *testing.T, names ...string) (certPEM, keyPEM []byte) { t.Helper() + return makeCertPEMWithValidity(t, -time.Hour, 90*24*time.Hour, names...) +} + +// makeCertPEMWithValidity issues a certificate whose validity window is given +// relative to now, so a test can produce an expired or nearly expired one. +func makeCertPEMWithValidity(t *testing.T, notBefore, notAfter time.Duration, names ...string) (certPEM, keyPEM []byte) { + t.Helper() key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) if err != nil { @@ -29,8 +36,8 @@ func makeCertPEM(t *testing.T, names ...string) (certPEM, keyPEM []byte) { SerialNumber: big.NewInt(time.Now().UnixNano()), Subject: pkix.Name{CommonName: names[0]}, DNSNames: names, - NotBefore: time.Now().Add(-time.Hour), - NotAfter: time.Now().Add(90 * 24 * time.Hour), + NotBefore: time.Now().Add(notBefore), + NotAfter: time.Now().Add(notAfter), } der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) if err != nil { @@ -49,6 +56,13 @@ func makeCertPEM(t *testing.T, names ...string) (certPEM, keyPEM []byte) { // resolver name to that resolver's account and certificates. func writeAcmeStore(t *testing.T, dir, resolverName string, entries map[string][]string) string { t.Helper() + return writeAcmeStoreWithValidity(t, dir, resolverName, entries, -time.Hour, 90*24*time.Hour) +} + +// writeAcmeStoreWithValidity is writeAcmeStore with control over how long the +// issued certificates are valid for. +func writeAcmeStoreWithValidity(t *testing.T, dir, resolverName string, entries map[string][]string, notBefore, notAfter time.Duration) string { + t.Helper() type domain struct { Main string `json:"main"` @@ -62,7 +76,7 @@ func writeAcmeStore(t *testing.T, dir, resolverName string, entries map[string][ var certificates []certificate for main, sans := range entries { - certPEM, keyPEM := makeCertPEM(t, append([]string{main}, sans...)...) + certPEM, keyPEM := makeCertPEMWithValidity(t, notBefore, notAfter, append([]string{main}, sans...)...) certificates = append(certificates, certificate{ Domain: domain{Main: main, SANs: sans}, Certificate: base64.StdEncoding.EncodeToString(certPEM), @@ -240,21 +254,63 @@ func TestAcmeCertWatcher_FailsWhenStoreMissing(t *testing.T) { } } -// An identical file must not cause a pointless swap; LastReload only advances -// when the content actually changed. -func TestAcmeCertWatcher_SkipsReloadWhenContentUnchanged(t *testing.T) { +// An identical file must not cause a pointless swap, but the watcher must +// still record that it looked. acme.json sits untouched for sixty days, so a +// LastReload that only advanced on change could not tell a healthy idle +// watcher apart from one that went blind (PLAN.md risk R2). +func TestAcmeCertWatcher_SkipsSwapButRecordsCheckWhenContentUnchanged(t *testing.T) { dir := t.TempDir() writeAcmeStore(t, dir, "letsencrypt", map[string][]string{"mail.example.test": nil}) watcher, _ := NewAcmeCertWatcher(dir, "acme.json", "mail.example.test") - first := watcher.LastReload() + firstReload := watcher.LastReload() + firstChange := watcher.LastChange() time.Sleep(2 * time.Millisecond) if err := watcher.reload(); err != nil { t.Fatalf("reload: %v", err) } - if !watcher.LastReload().Equal(first) { - t.Error("an unchanged file triggered a reload") + if !watcher.LastChange().Equal(firstChange) { + t.Error("an unchanged file swapped the certificate store") + } + if !watcher.LastReload().After(firstReload) { + t.Error("LastReload did not advance, so a blind watcher would look identical to an idle one") + } +} + +// An expired entry must not be served: every peer would reject the handshake, +// whereas skipping it lets the SNI fallback take over (PLAN.md section 8.2). +func TestAcmeCertWatcher_SkipsExpiredCertificate(t *testing.T) { + dir := t.TempDir() + writeAcmeStoreWithValidity(t, dir, "letsencrypt", + map[string][]string{"expired.example.test": nil}, -48*time.Hour, -24*time.Hour) + + if _, err := NewAcmeCertWatcher(dir, "acme.json", "expired.example.test"); err == nil { + t.Fatal("a store holding only an expired certificate must not load as usable") + } +} + +// The soonest expiry is what decides when the deployment breaks, so it has to +// be readable for the alerts of PLAN.md section 8.4. +func TestAcmeCertWatcher_ReportsEarliestExpiry(t *testing.T) { + dir := t.TempDir() + writeAcmeStoreWithValidity(t, dir, "letsencrypt", + map[string][]string{"mail.example.test": nil}, -time.Hour, 72*time.Hour) + + watcher, err := NewAcmeCertWatcher(dir, "acme.json", "mail.example.test") + if err != nil { + t.Fatalf("NewAcmeCertWatcher: %v", err) + } + + host, expiry, ok := watcher.EarliestExpiry() + if !ok { + t.Fatal("EarliestExpiry reported nothing for a loaded store") + } + if host != "mail.example.test" { + t.Errorf("EarliestExpiry host = %q, want mail.example.test", host) + } + if remaining := time.Until(expiry); remaining > 73*time.Hour || remaining < 71*time.Hour { + t.Errorf("expiry is %s away, want roughly 72h", remaining) } } diff --git a/services/protocols/internal/presentation/http/router.go b/services/protocols/internal/presentation/http/router.go index 8386131..2f8167e 100644 --- a/services/protocols/internal/presentation/http/router.go +++ b/services/protocols/internal/presentation/http/router.go @@ -22,6 +22,11 @@ type Router struct { reportUseCase *appusecase.IngestReportsUseCase pingFunc func() error mtaStsMode valueobject.MtaStsMode + // degradedFunc reports a condition that leaves the service running but + // not fit for production, the clearest case being a self-signed + // certificate standing in for one Traefik never issued + // (PLAN.md section 8.4). + degradedFunc func() error } func NewRouter(reportUseCase *appusecase.IngestReportsUseCase, pingFunc func() error) *Router { @@ -62,6 +67,13 @@ func (r *Router) registerRoutes() { r.mux.HandleFunc("/api/v1/reports/tlsrpt", r.handleTlsRptIngest) } +// SetDegradedCheck registers the probe behind the "degraded" health state. +// Returning an error means the process is serving but something an operator +// must fix is wrong. +func (r *Router) SetDegradedCheck(check func() error) { + r.degradedFunc = check +} + func (r *Router) handleHealth(w http.ResponseWriter, _ *http.Request) { if r.pingFunc != nil { if err := r.pingFunc(); err != nil { @@ -69,6 +81,21 @@ func (r *Router) handleHealth(w http.ResponseWriter, _ *http.Request) { return } } + + // Degraded still answers 200: the container is alive and must not be + // restarted in a loop over a certificate problem a restart cannot fix. + // The distinction is carried in the body and the header so monitoring can + // alert on it (PLAN.md section 8.4). + if r.degradedFunc != nil { + if err := r.degradedFunc(); err != nil { + w.Header().Set("X-LambdaMail-Health", "degraded") + w.WriteHeader(http.StatusOK) + fmt.Fprintf(w, "DEGRADED: %v\n", err) + return + } + } + + w.Header().Set("X-LambdaMail-Health", "ok") w.WriteHeader(http.StatusOK) w.Write([]byte("OK\n")) } diff --git a/services/protocols/internal/presentation/http/router_test.go b/services/protocols/internal/presentation/http/router_test.go index 60e8a67..fb98db3 100644 --- a/services/protocols/internal/presentation/http/router_test.go +++ b/services/protocols/internal/presentation/http/router_test.go @@ -2,6 +2,7 @@ package httppresentation import ( "bytes" + "errors" "net/http" "net/http/httptest" "strings" @@ -67,3 +68,38 @@ func TestRouter_MtaSts_Autoconfig_SecurityTxt_Ingest(t *testing.T) { t.Errorf("dmarc ingest status = %d, body = %q", w.Code, w.Body.String()) } } + +// A certificate problem must be visible without taking the container down: +// restarting cannot fix a certificate Traefik never issued, so /health stays +// 200 and carries the degraded state instead (PLAN.md section 8.4). +func TestRouter_HealthReportsDegradedWithoutFailing(t *testing.T) { + router := NewRouter(appusecase.NewIngestReportsUseCase(nil), func() error { return nil }) + router.SetDegradedCheck(func() error { return errors.New("self-signed certificate in use") }) + + req := httptest.NewRequest(http.MethodGet, "/health", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("degraded health status = %d, want 200 so the orchestrator does not restart-loop", w.Code) + } + if got := w.Header().Get("X-LambdaMail-Health"); got != "degraded" { + t.Errorf("X-LambdaMail-Health = %q, want degraded", got) + } + if !strings.Contains(w.Body.String(), "self-signed certificate in use") { + t.Errorf("the degraded body does not name the cause: %q", w.Body.String()) + } +} + +// A dependency that is actually down is a different state: that one must fail. +func TestRouter_HealthFailsWhenDependencyIsDown(t *testing.T) { + router := NewRouter(appusecase.NewIngestReportsUseCase(nil), func() error { return errors.New("database unreachable") }) + + req := httptest.NewRequest(http.MethodGet, "/health", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusServiceUnavailable { + t.Errorf("health status = %d, want 503", w.Code) + } +} diff --git a/services/protocols/internal/presentation/smtp/backend.go b/services/protocols/internal/presentation/smtp/backend.go index ad74aff..255a5d1 100644 --- a/services/protocols/internal/presentation/smtp/backend.go +++ b/services/protocols/internal/presentation/smtp/backend.go @@ -8,7 +8,6 @@ import ( "errors" "io" "net" - "strings" gosmtp "github.com/emersion/go-smtp" @@ -141,28 +140,23 @@ func (s *session) Data(r io.Reader) error { if errors.As(err, &smtpErr) { return smtpErr } - errMsg := err.Error() - if strings.HasPrefix(errMsg, "550 ") { + + // A refusal the use case decided on carries its own reply. Rendering + // it here is the only place the wire format is chosen. + var rejection *usecase.SmtpRejection + if errors.As(err, &rejection) { return &gosmtp.SMTPError{ - Code: 550, - EnhancedCode: gosmtp.EnhancedCode{5, 7, 1}, - Message: strings.TrimPrefix(errMsg, "550 5.7.1 "), - } - } - if strings.HasPrefix(errMsg, "554 ") { - return &gosmtp.SMTPError{ - Code: 554, - EnhancedCode: gosmtp.EnhancedCode{5, 7, 1}, - Message: strings.TrimPrefix(errMsg, "554 5.7.1 "), - } - } - if strings.HasPrefix(errMsg, "451 ") { - return &gosmtp.SMTPError{ - Code: 451, - EnhancedCode: gosmtp.EnhancedCode{4, 7, 1}, - Message: strings.TrimPrefix(errMsg, "451 4.7.1 "), + Code: rejection.Code, + EnhancedCode: gosmtp.EnhancedCode{ + rejection.EnhancedCode[0], rejection.EnhancedCode[1], rejection.EnhancedCode[2], + }, + Message: rejection.Message, } } + + // Anything else is an internal failure. It is reported as temporary so + // the sender retries: the message was not stored, and telling the peer + // it failed permanently would discard mail over a bug on this side. return &gosmtp.SMTPError{ Code: 451, EnhancedCode: gosmtp.EnhancedCode{4, 3, 0}, diff --git a/services/protocols/internal/presentation/smtp/backend_test.go b/services/protocols/internal/presentation/smtp/backend_test.go index b5ef21f..ef93af4 100644 --- a/services/protocols/internal/presentation/smtp/backend_test.go +++ b/services/protocols/internal/presentation/smtp/backend_test.go @@ -12,6 +12,7 @@ import ( "lambdamail/protocols/internal/application/port" "lambdamail/protocols/internal/application/usecase" + "lambdamail/protocols/internal/domain/valueobject" ) type fakeMailboxRepository struct { @@ -37,6 +38,8 @@ func (f *fakeMailboxRepository) ResolveDeliveryTargets(_ context.Context, addres type recordingBlobStorage struct { stored []byte ref port.BlobRef + // err makes Store fail, standing in for an unexpected internal failure. + err error } func (b *recordingBlobStorage) Store(_ context.Context, r io.Reader) (port.BlobRef, error) { @@ -44,21 +47,44 @@ func (b *recordingBlobStorage) Store(_ context.Context, r io.Reader) (port.BlobR if err != nil { return port.BlobRef{}, err } + if b.err != nil { + return port.BlobRef{}, b.err + } b.stored = data return b.ref, nil } +// rejectingScanner stands in for the spam or virus scanner refusing a message. +type rejectingScanner struct{ rejection error } + +func (s *rejectingScanner) Scan(_ context.Context, _ port.ScanInput) (*valueobject.ScanResult, error) { + return nil, s.rejection +} + type recordingMessageRepository struct { persisted []port.PersistInboundMessageInput } -func (m *recordingMessageRepository) Persist(_ context.Context, input port.PersistInboundMessageInput) (int64, error) { - m.persisted = append(m.persisted, input) - return int64(len(m.persisted)), nil +func (m *recordingMessageRepository) PersistAll(_ context.Context, inputs []port.PersistInboundMessageInput) ([]int64, error) { + uids := make([]int64, 0, len(inputs)) + for _, input := range inputs { + m.persisted = append(m.persisted, input) + uids = append(uids, int64(len(m.persisted))) + } + return uids, nil } func newTestSession(mailboxes *fakeMailboxRepository, blobs *recordingBlobStorage, messages *recordingMessageRepository) gosmtp.Session { + return newTestSessionWithScanner(mailboxes, blobs, messages, nil) +} + +// newTestSessionWithScanner builds a session whose scanner refuses the message +// with the given error, which is how a rejection reaches the Data handler. +func newTestSessionWithScanner(mailboxes *fakeMailboxRepository, blobs *recordingBlobStorage, messages *recordingMessageRepository, rejection error) gosmtp.Session { uc := usecase.NewProcessInboundEmailUseCase(mailboxes, blobs, messages) + if rejection != nil { + uc.SetScanner(&rejectingScanner{rejection: rejection}) + } backend := NewBackend(uc) session, err := backend.NewSession(nil) if err != nil { @@ -194,3 +220,81 @@ func TestSession_Reset_ClearsAccumulatedRecipients(t *testing.T) { t.Errorf("persisted %d messages after Reset, want 0", len(messages.persisted)) } } + +// The reply a remote MTA receives must come from the use case's decision, not +// from parsing its error text: rewording a message would otherwise change a +// permanent refusal into a retry, or the reverse. +func TestSession_Data_RendersUseCaseRejectionVerbatim(t *testing.T) { + cases := []struct { + name string + rejection *usecase.SmtpRejection + wantCode int + wantEnhancedCode gosmtp.EnhancedCode + }{ + {"virus is permanent", &usecase.SmtpRejection{Code: 554, EnhancedCode: [3]int{5, 7, 1}, Message: "Virus detected: Eicar-Test"}, 554, gosmtp.EnhancedCode{5, 7, 1}}, + {"greylist is temporary", &usecase.SmtpRejection{Code: 451, EnhancedCode: [3]int{4, 7, 1}, Message: "Greylisted, please try again later"}, 451, gosmtp.EnhancedCode{4, 7, 1}}, + {"dmarc reject is permanent", &usecase.SmtpRejection{Code: 550, EnhancedCode: [3]int{5, 7, 1}, Message: "DMARC policy rejects this message"}, 550, gosmtp.EnhancedCode{5, 7, 1}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mailboxID := uuid.New() + mailboxes := &fakeMailboxRepository{byAddress: map[string]port.MailboxRecord{ + "user@example.test": {ID: mailboxID, QuotaBytes: 1000}, + }} + session := newTestSessionWithScanner(mailboxes, + &recordingBlobStorage{ref: port.BlobRef{ID: uuid.New()}}, &recordingMessageRepository{}, tc.rejection) + + if err := session.Mail("sender@remote.test", nil); err != nil { + t.Fatalf("MAIL FROM: %v", err) + } + if err := session.Rcpt("user@example.test", nil); err != nil { + t.Fatalf("RCPT TO: %v", err) + } + + err := session.Data(bytes.NewBufferString("Subject: hi\r\n\r\nbody\r\n")) + + var smtpErr *gosmtp.SMTPError + if !errors.As(err, &smtpErr) { + t.Fatalf("Data returned %v, want an *gosmtp.SMTPError", err) + } + if smtpErr.Code != tc.wantCode { + t.Errorf("reply code = %d, want %d", smtpErr.Code, tc.wantCode) + } + if smtpErr.EnhancedCode != tc.wantEnhancedCode { + t.Errorf("enhanced code = %v, want %v", smtpErr.EnhancedCode, tc.wantEnhancedCode) + } + if smtpErr.Message != tc.rejection.Message { + t.Errorf("message = %q, want %q", smtpErr.Message, tc.rejection.Message) + } + }) + } +} + +// An unexpected internal failure must be temporary: the message was not +// stored, and a permanent reply would discard mail over a bug on this side. +func TestSession_Data_ReportsInternalFailureAsTemporary(t *testing.T) { + mailboxID := uuid.New() + mailboxes := &fakeMailboxRepository{byAddress: map[string]port.MailboxRecord{ + "user@example.test": {ID: mailboxID, QuotaBytes: 1000}, + }} + session := newTestSession(mailboxes, + &recordingBlobStorage{err: errors.New("disk full")}, &recordingMessageRepository{}) + + if err := session.Mail("sender@remote.test", nil); err != nil { + t.Fatalf("MAIL FROM: %v", err) + } + if err := session.Rcpt("user@example.test", nil); err != nil { + t.Fatalf("RCPT TO: %v", err) + } + + err := session.Data(bytes.NewBufferString("Subject: hi\r\n\r\nbody\r\n")) + + var smtpErr *gosmtp.SMTPError + if !errors.As(err, &smtpErr) { + t.Fatalf("Data returned %v, want an *gosmtp.SMTPError", err) + } + if smtpErr.Code != 451 { + t.Errorf("reply code = %d, want 451 so the sender retries", smtpErr.Code) + } +} From 3deb328bbfb864f3a0750b6cde6b36bef09af79b Mon Sep 17 00:00:00 2001 From: Lucas Rayan Date: Sun, 2 Aug 2026 07:57:39 -0300 Subject: [PATCH 2/3] #19: repair the compose stack and wire Rspamd to Redis "docker compose up" could not work, which means the F0 acceptance criterion was not being met: - ghcr.io/rspamd/rspamd:3.9 returns "denied" and clamav/clamav:1.3 does not exist. Both were verified against the registries; the working references are rspamd/rspamd:3.9 and clamav/clamav:1.4. - Rspamd received RSPAMD_REDIS_SERVERS and RSPAMD_REDIS_PASSWORD as environment variables it never reads. Confirmed in the container that it does not expand ${VAR} in UCL: the password reached Redis as that literal string. Greylisting and ratelimit keep their state in Redis, so both were silently doing nothing while appearing configured. The credential is now rendered at startup into override.d, a tmpfs, so it touches neither the image nor a volume. - The ratelimit module ships enabled with no rates defined, which accepts everything. ratelimit.conf defines the per-IP, per-user and per-recipient buckets of section 5.2. Verified by bringing redis and rspamd up: the real password resolves, the rates load, and a probe message returns "Action: greylist" with the state written to Redis. Services also gained an image: alongside build:, so the deploy host pulls instead of compiling while "docker compose build" still works locally. Co-Authored-By: Claude Opus 5 --- .env.example | 9 ++++++++ config/rspamd/ratelimit.conf | 41 ++++++++++++++++++++++++++++++++++++ docker-compose.yaml | 25 ++++++++++++++++++---- 3 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 config/rspamd/ratelimit.conf diff --git a/.env.example b/.env.example index 283d73b..bc171bd 100644 --- a/.env.example +++ b/.env.example @@ -1,3 +1,12 @@ +# --- Container images --- +# Published to GHCR by .github/workflows/release.yml, multi-architecture +# (linux/amd64 and linux/arm64). The deploy host pulls these instead of +# compiling: `docker compose pull && docker compose up -d`. Running +# `docker compose build` locally still overrides them with a local build. +# Pin IMAGE_TAG to a released version (e.g. 1.2.3) or a sha- tag to roll back. +IMAGE_NAMESPACE=ghcr.io/lucasrguerra/lambdamail +IMAGE_TAG=latest + # --- Postgres --- POSTGRES_DB=lambdamail POSTGRES_USER=lambdamail diff --git a/config/rspamd/ratelimit.conf b/config/rspamd/ratelimit.conf new file mode 100644 index 0000000..c158782 --- /dev/null +++ b/config/rspamd/ratelimit.conf @@ -0,0 +1,41 @@ +# Rate limiting of PLAN.md section 5.2. +# +# This file exists because the module ships enabled but with no rates defined, +# which means it accepts everything: "rspamadm configdump ratelimit" on a +# stock image returns only whitelisted_rcpts. Without explicit buckets there +# is no limit at all, and the defence reads as present while doing nothing. +# +# Each rate is " / ". The bucket leaks over the period, so the +# burst is what a sender may do at once and the period sets the refill rate. + +rates { + # Per connecting IP: generous enough for a busy legitimate MTA relaying a + # mailing list, tight enough to matter for a spam cannon. + to_ip = { + bucket = [ + { burst = 100; rate = "60 / 1min"; } + ]; + } + + # Per authenticated sender. This is the containment for a compromised + # account, and it backs up the per-mailbox max_recipients_per_hour enforced + # in ProcessOutboundEmailUseCase - that one is authoritative, this one + # catches the same abuse earlier and without a database round trip. + user = { + bucket = [ + { burst = 50; rate = "30 / 1min"; } + ]; + } + + # Per recipient, to blunt a dictionary attack enumerating local parts. + to = { + bucket = [ + { burst = 20; rate = "10 / 1min"; } + ]; + } +} + +# Mail we authenticated ourselves is not what this limit is aimed at, but a +# compromised account is - so authenticated users stay limited, and only the +# addresses RFC 2142 requires to always be reachable are exempt. +whitelisted_rcpts = "postmaster,mailer-daemon,abuse"; diff --git a/docker-compose.yaml b/docker-compose.yaml index 06f50c4..b173c22 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -71,15 +71,28 @@ services: # ------------------------------------------------------------ RSPAMD rspamd: - image: ghcr.io/rspamd/rspamd:3.9 + image: rspamd/rspamd:3.9 restart: *restart logging: *default-logging environment: - RSPAMD_REDIS_SERVERS: redis - RSPAMD_REDIS_PASSWORD: ${REDIS_PASSWORD} + REDIS_PASSWORD: ${REDIS_PASSWORD} volumes: - ./config/rspamd:/etc/rspamd/local.d:ro - rspamd_data:/var/lib/rspamd + # Rspamd does NOT expand environment variables in its UCL config - a + # "${REDIS_PASSWORD}" written into a .conf reaches Redis as that literal + # string. The credential is therefore rendered at startup into override.d, + # which is a tmpfs so the secret never touches the image or a volume. + # Without this, greylisting and ratelimit silently do nothing: both keep + # their state in Redis. + tmpfs: + - /etc/rspamd/override.d:uid=11333,gid=11333,mode=0700 + entrypoint: + - sh + - -c + - >- + printf 'servers = "redis:6379";\npassword = "%s";\n' "$$REDIS_PASSWORD" + > /etc/rspamd/override.d/redis.conf && exec /usr/bin/rspamd -f depends_on: redis: { condition: service_healthy } healthcheck: @@ -91,7 +104,7 @@ services: # ------------------------------------------------------------ CLAMAV clamav: - image: clamav/clamav:1.3 + image: clamav/clamav:1.4 restart: *restart logging: *default-logging volumes: [clamav_data:/var/lib/clamav] @@ -105,6 +118,7 @@ services: # -------------------------------------------------------------- AUTH auth: + image: ${IMAGE_NAMESPACE:-ghcr.io/lucasrguerra/lambdamail}/auth:${IMAGE_TAG:-latest} build: { context: ./services/auth } restart: *restart logging: *default-logging @@ -134,6 +148,7 @@ services: # ----------------------------------------------------------- STORAGE storage: + image: ${IMAGE_NAMESPACE:-ghcr.io/lucasrguerra/lambdamail}/storage:${IMAGE_TAG:-latest} build: { context: ./services/storage } restart: *restart logging: *default-logging @@ -163,6 +178,7 @@ services: # --------------------------------------------------------- PROTOCOLS protocols: + image: ${IMAGE_NAMESPACE:-ghcr.io/lucasrguerra/lambdamail}/protocols:${IMAGE_TAG:-latest} build: { context: ./services/protocols } restart: *restart logging: *default-logging @@ -238,6 +254,7 @@ services: # -------------------------------------------------------------- WEB web: + image: ${IMAGE_NAMESPACE:-ghcr.io/lucasrguerra/lambdamail}/webmail:${IMAGE_TAG:-latest} build: { context: ./services/webmail } restart: *restart logging: *default-logging From 46d421c8748e6ba0ae264724be2be958b78d29d8 Mon Sep 17 00:00:00 2001 From: Lucas Rayan Date: Sun, 2 Aug 2026 07:58:02 -0300 Subject: [PATCH 3/3] #19: add Trivy scanning and multi-architecture GHCR publishing CI gains a security job (Trivy fs: vuln, secret and misconfig) and an image job that builds each of the four services and scans the result. The secret scanner earns its place here: a DKIM key or Cloudflare token committed by accident is a full compromise of every domain served. release.yml publishes on a merge into main and on v* tags. Four services on two architectures is eight build jobs pushing by digest, then four publish jobs joining each pair into one manifest list, tagged latest, sha and semver. github.repository is lowercased first because GHCR rejects an uppercase path and this repository is lucasrguerra/LambdaMail. Both scanners failed on first run, so this commit also fixes what they found rather than shipping a workflow that is red on arrival: - All four images ran as root (DS-0002), which section 17 risk R5 explicitly argues against. They now run unprivileged; protocols keeps its eight ports below 1024 through cap_net_bind_service on the binary rather than through being root. - A HIGH advisory in postcss, reached only through Next. "npm audit fix --force" would have resolved it by downgrading Next from 15 to 9, so it is pinned with an override instead. - The Node images copied the build stage's node_modules wholesale, shipping typescript, eslint and vitest into production, and the base image's own bundled npm accounted for the rest of the CVEs. Runtime dependencies now come from npm ci --omit=dev and npm is removed from the final layer. - compose-up could never have passed: the proxy network is external because Coolify owns it on the deploy host, and nothing creates it in CI, so compose refused to start the stack at all. Verified locally: actionlint clean, all ten action refs confirmed to exist, Trivy fs and all four image scans exit 0, the full stack reaches Healthy on every container, and the containers run as uid 10001/1000 with all eight privileged ports bound and SMTP answering on 25. The arm64 build itself is unverified - this machine has no buildx - and will first be exercised by release.yml. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 73 +++++++++++- .github/workflows/release.yml | 179 +++++++++++++++++++++++++++++ services/auth/Dockerfile | 28 ++++- services/protocols/Dockerfile | 21 +++- services/storage/Dockerfile | 32 +++++- services/webmail/Dockerfile | 21 +++- services/webmail/package-lock.json | 72 +----------- services/webmail/package.json | 3 +- 8 files changed, 350 insertions(+), 79 deletions(-) create mode 100644 .github/workflows/release.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5a8a6b5..958e9d0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,18 +1,28 @@ name: CI +# Runs on every branch and pull request. Publishing lives in release.yml and +# only runs once this workflow has passed. on: push: - branches: [main] + branches: ['**'] pull_request: + branches: [main] + workflow_call: + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true jobs: lang-lint: + name: Lint de idioma (R9) runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - run: node scripts/lang-lint.mjs go-protocols: + name: Go (protocols) runs-on: ubuntu-latest services: postgres: @@ -49,6 +59,7 @@ jobs: TEST_DATABASE_URL: postgres://lambdamail_test:lambdamail_test@localhost:5432/lambdamail_test?sslmode=disable ts-auth: + name: TypeScript (auth) runs-on: ubuntu-latest defaults: run: @@ -62,6 +73,7 @@ jobs: - run: npm test ts-storage: + name: TypeScript (storage) runs-on: ubuntu-latest defaults: run: @@ -75,6 +87,7 @@ jobs: - run: npm test webmail-build: + name: Build (webmail) runs-on: ubuntu-latest defaults: run: @@ -86,12 +99,70 @@ jobs: - run: npm install - run: npm run build + security: + name: Trivy (dependências, segredos e Dockerfiles) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + # A mail server holds credentials for every mailbox it serves, so the + # secret scanner matters as much as the vulnerability one here: a DKIM + # key or a Cloudflare token committed by accident is a full compromise. + - name: Repositório + uses: aquasecurity/trivy-action@v0.36.0 + with: + scan-type: fs + scanners: vuln,secret,misconfig + severity: HIGH,CRITICAL + exit-code: '1' + skip-dirs: services/auth/node_modules,services/storage/node_modules,services/webmail/node_modules,services/webmail/.next + + image: + name: Imagem (${{ matrix.service }}) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + service: [protocols, auth, storage, webmail] + steps: + - uses: actions/checkout@v4 + + - uses: docker/setup-buildx-action@v3 + + # Built but not pushed: this proves each Dockerfile still works on every + # branch, while publishing stays in release.yml. + - name: Build + uses: docker/build-push-action@v6 + with: + context: services/${{ matrix.service }} + push: false + load: true + tags: lambdamail-${{ matrix.service }}:ci + cache-from: type=gha,scope=${{ matrix.service }} + cache-to: type=gha,mode=max,scope=${{ matrix.service }} + + - name: Trivy na imagem + uses: aquasecurity/trivy-action@v0.36.0 + with: + image-ref: lambdamail-${{ matrix.service }}:ci + severity: HIGH,CRITICAL + exit-code: '1' + compose-up: + name: Stack completa runs-on: ubuntu-latest needs: [go-protocols, ts-auth, ts-storage, webmail-build] steps: - uses: actions/checkout@v4 - run: cp .env.example .env + # The proxy network is declared external because Coolify owns it on the + # deploy host. Nothing creates it here, and without it compose refuses to + # start the stack at all. + - name: Cria a rede do proxy + run: docker network create "${COOLIFY_PROXY_NETWORK:-coolify}" + # --build makes compose build each service locally and tag it under the + # same name the published image uses, so the stack comes up without ever + # reaching the registry. - run: docker compose --env-file .env up --build --wait - run: docker compose ps - run: docker compose down -v diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..20d062e --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,179 @@ +name: Release + +# Publishes the four service images to GHCR after a merge into main, and again +# under semver tags when a v* tag is pushed. The deploy host only pulls, so +# nothing has to be built on the server. +on: + push: + branches: [main] + tags: ['v*'] + workflow_dispatch: + +concurrency: + group: release + cancel-in-progress: false + +env: + REGISTRY: ghcr.io + +jobs: + # Nothing is published unless the full suite passes first. + ci: + uses: ./.github/workflows/ci.yml + + # GHCR rejects an uppercase path, and this repository is "lucasrguerra/LambdaMail". + # Lowercasing github.repository here keeps the workflow fork-safe instead of + # hardcoding the owner. + prepare: + name: Namespace das imagens + runs-on: ubuntu-latest + outputs: + namespace: ${{ steps.namespace.outputs.value }} + steps: + - id: namespace + run: echo "value=${GITHUB_REPOSITORY,,}" >> "$GITHUB_OUTPUT" + + # Each architecture is built on a runner of its own kind. QEMU emulation + # would also work, but it makes the Go and Next builds roughly an order of + # magnitude slower. + build: + name: Build ${{ matrix.service }} (${{ matrix.platform }}) + needs: [ci, prepare] + runs-on: ${{ matrix.runner }} + permissions: + contents: read + packages: write + strategy: + fail-fast: false + matrix: + # Both are axes, so this is a cross product: four services on two + # architectures is eight jobs. Declaring platform only under `include` + # would not multiply anything - include can add keys to existing + # combinations, but it cannot create the combinations themselves. + service: [protocols, auth, storage, webmail] + platform: [linux/amd64, linux/arm64] + include: + - platform: linux/amd64 + runner: ubuntu-latest + arch: amd64 + - platform: linux/arm64 + runner: ubuntu-24.04-arm + arch: arm64 + steps: + - uses: actions/checkout@v4 + + - uses: docker/setup-buildx-action@v3 + + - name: Autentica no GHCR + uses: docker/login-action@v3 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Metadados + id: meta + uses: docker/metadata-action@v5 + with: + images: ${{ env.REGISTRY }}/${{ needs.prepare.outputs.namespace }}/${{ matrix.service }} + + # Pushed by digest only: the tags are attached later, once both + # architectures exist and can be joined into a single manifest list. + - name: Build e push por digest + id: build + uses: docker/build-push-action@v6 + with: + context: services/${{ matrix.service }} + platforms: ${{ matrix.platform }} + labels: ${{ steps.meta.outputs.labels }} + outputs: type=image,name=${{ env.REGISTRY }}/${{ needs.prepare.outputs.namespace }}/${{ matrix.service }},push-by-digest=true,name-canonical=true,push=true + cache-from: type=gha,scope=${{ matrix.service }}-${{ matrix.arch }} + cache-to: type=gha,mode=max,scope=${{ matrix.service }}-${{ matrix.arch }} + + - name: Exporta o digest + run: | + mkdir -p /tmp/digests + # The output already carries the `sha256:` prefix; the file name must + # not, since the merge step prepends it again. + digest='${{ steps.build.outputs.digest }}' + touch "/tmp/digests/${digest#sha256:}" + + - name: Guarda o digest + uses: actions/upload-artifact@v4 + with: + # Slashes are not allowed in artifact names, hence the arch alias. + name: digest-${{ matrix.service }}-${{ matrix.arch }} + path: /tmp/digests/* + retention-days: 1 + if-no-files-found: error + + publish: + name: Publica ${{ matrix.service }} + needs: [build, prepare] + runs-on: ubuntu-latest + permissions: + contents: read + packages: write + strategy: + fail-fast: false + matrix: + service: [protocols, auth, storage, webmail] + steps: + - uses: actions/checkout@v4 + + - uses: docker/setup-buildx-action@v3 + + - name: Autentica no GHCR + uses: docker/login-action@v3 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Recupera os digests + uses: actions/download-artifact@v4 + with: + pattern: digest-${{ matrix.service }}-* + merge-multiple: true + path: /tmp/digests + + # `latest` is what compose pulls. The semver tags only materialise on a + # v* tag push and make it possible to pin or roll back to an exact + # release; the sha tag does the same for any main build. + - name: Metadados + id: meta + uses: docker/metadata-action@v5 + with: + images: ${{ env.REGISTRY }}/${{ needs.prepare.outputs.namespace }}/${{ matrix.service }} + tags: | + type=semver,pattern={{version}} + type=semver,pattern={{major}}.{{minor}} + type=semver,pattern={{major}} + type=raw,value=latest + type=sha,format=long + + # Joins both architectures under one manifest list, so `docker pull` + # resolves to the right one automatically. + - name: Cria o manifesto multi-arquitetura + working-directory: /tmp/digests + run: | + docker buildx imagetools create \ + $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< '${{ steps.meta.outputs.json }}') \ + $(printf '${{ env.REGISTRY }}/${{ needs.prepare.outputs.namespace }}/${{ matrix.service }}@sha256:%s ' *) + + - name: Confere o resultado + run: | + docker buildx imagetools inspect \ + "${{ env.REGISTRY }}/${{ needs.prepare.outputs.namespace }}/${{ matrix.service }}:latest" \ + --format '{{json .Manifest}}' \ + | jq -r '.manifests[] | select(.platform.os == "linux") | "\(.platform.os)/\(.platform.architecture)"' + + - name: Resumo + run: | + { + echo "### ${{ matrix.service }} publicado (linux/amd64 e linux/arm64)" + echo + echo '```' + echo "${{ steps.meta.outputs.tags }}" + echo '```' + } >> "$GITHUB_STEP_SUMMARY" diff --git a/services/auth/Dockerfile b/services/auth/Dockerfile index e2f95cf..908ddb7 100644 --- a/services/auth/Dockerfile +++ b/services/auth/Dockerfile @@ -1,14 +1,36 @@ FROM node:20-alpine AS build WORKDIR /app COPY package.json package-lock.json* ./ -RUN npm install +# npm ci, not npm install: the image must be the lockfile's content, not +# whatever resolves the day it is built. +RUN npm ci COPY . . RUN npm run build -FROM node:20-alpine +# Production dependencies only. The previous image copied the build stage's +# node_modules wholesale, shipping typescript, eslint and vitest into +# production - most of the CVEs the image scan reported came from that +# tooling, which never runs in the container. +FROM node:20-alpine AS prod-deps WORKDIR /app -COPY --from=build /app/node_modules ./node_modules +COPY package.json package-lock.json* ./ +RUN npm ci --omit=dev + +FROM node:20-alpine +# The node:20-alpine tag lags its Alpine base, so pending security patches +# (openssl, busybox) land only with an explicit upgrade. +RUN apk upgrade --no-cache \ + # The runtime only ever runs "node"; npm is not invoked and its own + # bundled dependency tree (sigstore, cross-spawn, glob, ...) is what + # every remaining image CVE came from. Removing it drops the whole + # class instead of chasing each advisory. + && rm -rf /usr/local/lib/node_modules/npm /usr/local/bin/npm /usr/local/bin/npx +WORKDIR /app +COPY --from=prod-deps /app/node_modules ./node_modules COPY --from=build /app/dist ./dist COPY package.json ./ + +# The node images ship an unprivileged "node" user (uid 1000). +USER node EXPOSE 3001 CMD ["node", "dist/server.js"] diff --git a/services/protocols/Dockerfile b/services/protocols/Dockerfile index c4ba5d9..8f3fcfb 100644 --- a/services/protocols/Dockerfile +++ b/services/protocols/Dockerfile @@ -6,7 +6,26 @@ COPY . . RUN CGO_ENABLED=0 go build -o /out/lambdamail-protocols ./cmd/lambdamail-protocols FROM alpine:3.20 -RUN apk add --no-cache ca-certificates +# libcap provides setcap and is dropped again once the capability is set. +RUN apk add --no-cache ca-certificates libcap + COPY --from=build /out/lambdamail-protocols /app/lambdamail-protocols + +# This process terminates SMTP from the open internet, so it must not run as +# root (PLAN.md section 17, risk R5). Every mail port it binds is below 1024, +# which normally requires root - the file capability grants exactly that one +# privilege to this one binary instead. +RUN setcap 'cap_net_bind_service=+ep' /app/lambdamail-protocols \ + && apk del libcap + +RUN addgroup -g 10001 -S lambdamail \ + && adduser -u 10001 -S -G lambdamail lambdamail \ + # Created here so a fresh named volume mounted at this path inherits the + # ownership: Docker seeds an empty volume from the image directory. + && mkdir -p /var/lambdamail/spool \ + && chown -R lambdamail:lambdamail /var/lambdamail + +USER lambdamail:lambdamail + EXPOSE 8080 ENTRYPOINT ["/app/lambdamail-protocols"] diff --git a/services/storage/Dockerfile b/services/storage/Dockerfile index a758cee..96ccdf8 100644 --- a/services/storage/Dockerfile +++ b/services/storage/Dockerfile @@ -1,14 +1,40 @@ FROM node:20-alpine AS build WORKDIR /app COPY package.json package-lock.json* ./ -RUN npm install +# npm ci, not npm install: the image must be the lockfile's content, not +# whatever resolves the day it is built. +RUN npm ci COPY . . RUN npm run build -FROM node:20-alpine +# Production dependencies only. The previous image copied the build stage's +# node_modules wholesale, shipping typescript, eslint and vitest into +# production - most of the CVEs the image scan reported came from that +# tooling, which never runs in the container. +FROM node:20-alpine AS prod-deps WORKDIR /app -COPY --from=build /app/node_modules ./node_modules +COPY package.json package-lock.json* ./ +RUN npm ci --omit=dev + +FROM node:20-alpine +# The node:20-alpine tag lags its Alpine base, so pending security patches +# (openssl, busybox) land only with an explicit upgrade. +RUN apk upgrade --no-cache \ + # The runtime only ever runs "node"; npm is not invoked and its own + # bundled dependency tree (sigstore, cross-spawn, glob, ...) is what + # every remaining image CVE came from. Removing it drops the whole + # class instead of chasing each advisory. + && rm -rf /usr/local/lib/node_modules/npm /usr/local/bin/npm /usr/local/bin/npx +WORKDIR /app +COPY --from=prod-deps /app/node_modules ./node_modules COPY --from=build /app/dist ./dist COPY package.json ./ + +# Created before dropping privileges so a fresh named volume mounted here +# inherits the ownership: Docker seeds an empty volume from the image. +RUN mkdir -p /var/lambdamail/mails && chown -R node:node /var/lambdamail + +# The node images ship an unprivileged "node" user (uid 1000). +USER node EXPOSE 3002 CMD ["node", "dist/server.js"] diff --git a/services/webmail/Dockerfile b/services/webmail/Dockerfile index 4da61b9..d16f193 100644 --- a/services/webmail/Dockerfile +++ b/services/webmail/Dockerfile @@ -1,15 +1,28 @@ FROM node:20-alpine AS build WORKDIR /app COPY package.json package-lock.json* ./ -RUN npm install +RUN npm ci COPY . . RUN npm run build FROM node:20-alpine +# The node:20-alpine tag lags its Alpine base, so pending security patches +# (openssl, busybox) land only with an explicit upgrade. +RUN apk upgrade --no-cache \ + # The runtime only ever runs "node"; npm is not invoked and its own + # bundled dependency tree (sigstore, cross-spawn, glob, ...) is what + # every remaining image CVE came from. Removing it drops the whole + # class instead of chasing each advisory. + && rm -rf /usr/local/lib/node_modules/npm /usr/local/bin/npm /usr/local/bin/npx WORKDIR /app -COPY --from=build /app/.next/standalone ./ -COPY --from=build /app/.next/static ./.next/static -COPY --from=build /app/public ./public +# --chown matters here: Next's standalone server writes its cache under +# .next at runtime, which it cannot do if the tree belongs to root. +COPY --from=build --chown=node:node /app/.next/standalone ./ +COPY --from=build --chown=node:node /app/.next/static ./.next/static +COPY --from=build --chown=node:node /app/public ./public + +# The node images ship an unprivileged "node" user (uid 1000). +USER node EXPOSE 3000 # Docker sets HOSTNAME to the container ID by default; Next's standalone # server.js honors that env var and binds only to it, refusing localhost diff --git a/services/webmail/package-lock.json b/services/webmail/package-lock.json index 93a57bb..c3bd744 100644 --- a/services/webmail/package-lock.json +++ b/services/webmail/package-lock.json @@ -140,9 +140,6 @@ "cpu": [ "arm" ], - "libc": [ - "glibc" - ], "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -159,9 +156,6 @@ "cpu": [ "arm64" ], - "libc": [ - "glibc" - ], "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -178,9 +172,6 @@ "cpu": [ "ppc64" ], - "libc": [ - "glibc" - ], "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -197,9 +188,6 @@ "cpu": [ "riscv64" ], - "libc": [ - "glibc" - ], "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -216,9 +204,6 @@ "cpu": [ "s390x" ], - "libc": [ - "glibc" - ], "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -235,9 +220,6 @@ "cpu": [ "x64" ], - "libc": [ - "glibc" - ], "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -254,9 +236,6 @@ "cpu": [ "arm64" ], - "libc": [ - "musl" - ], "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -273,9 +252,6 @@ "cpu": [ "x64" ], - "libc": [ - "musl" - ], "license": "LGPL-3.0-or-later", "optional": true, "os": [ @@ -292,9 +268,6 @@ "cpu": [ "arm" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0", "optional": true, "os": [ @@ -317,9 +290,6 @@ "cpu": [ "arm64" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0", "optional": true, "os": [ @@ -342,9 +312,6 @@ "cpu": [ "ppc64" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0", "optional": true, "os": [ @@ -367,9 +334,6 @@ "cpu": [ "riscv64" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0", "optional": true, "os": [ @@ -392,9 +356,6 @@ "cpu": [ "s390x" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0", "optional": true, "os": [ @@ -417,9 +378,6 @@ "cpu": [ "x64" ], - "libc": [ - "glibc" - ], "license": "Apache-2.0", "optional": true, "os": [ @@ -442,9 +400,6 @@ "cpu": [ "arm64" ], - "libc": [ - "musl" - ], "license": "Apache-2.0", "optional": true, "os": [ @@ -467,9 +422,6 @@ "cpu": [ "x64" ], - "libc": [ - "musl" - ], "license": "Apache-2.0", "optional": true, "os": [ @@ -622,9 +574,6 @@ "cpu": [ "arm64" ], - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -641,9 +590,6 @@ "cpu": [ "arm64" ], - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -660,9 +606,6 @@ "cpu": [ "x64" ], - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -679,9 +622,6 @@ "cpu": [ "x64" ], - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -898,9 +838,9 @@ "license": "ISC" }, "node_modules/postcss": { - "version": "8.4.31", - "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.4.31.tgz", - "integrity": "sha512-PS08Iboia9mts/2ygV3eLpY5ghnUcfLV/EXTOW1E2qYxJKGGBUtNjN76FYHnMs36RmARn41bC0AZmn+rR0OVpQ==", + "version": "8.5.25", + "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.25.tgz", + "integrity": "sha512-DTPx3RWSSnWyzLxQnlH0rJP+EW5ekl16ZU4/psbIhA0e53kJfdgaN5vKM+xP7yJtXVu+nfdVFmlgFDEKAe4Pyw==", "funding": [ { "type": "opencollective", @@ -917,9 +857,9 @@ ], "license": "MIT", "dependencies": { - "nanoid": "^3.3.6", - "picocolors": "^1.0.0", - "source-map-js": "^1.0.2" + "nanoid": "^3.3.16", + "picocolors": "^1.1.1", + "source-map-js": "^1.2.1" }, "engines": { "node": "^10 || ^12 || >=14" diff --git a/services/webmail/package.json b/services/webmail/package.json index 98336d6..7e71b98 100644 --- a/services/webmail/package.json +++ b/services/webmail/package.json @@ -18,6 +18,7 @@ "@types/react": "^18.3.3" }, "overrides": { - "sharp": "^0.35.0" + "sharp": "^0.35.0", + "postcss": "^8.5.25" } }