krz/gitbay

mail: require TLS to the relay !491

merged merged by cmc on 2026-09-28 21:50 UTC · krz/gitbay:mail-require-tls into main

10 files changed, +253 −14

Layout: unified · split

.gitbay/wiki/Admin.org +9 −2
@@ -125,10 +125,17 @@ build page's live log arrives only when the build ends;
125 registration. Requires [mail]. 125 registration. Requires [mail].
126 126
127** [mail] 127** [mail]
128- =smtp_host= (host:port, 587 assumed), =from=, optional =smtp_user= / 128- =smtp_host= (host:port; 587 assumed, 465 with =tls = "implicit"=),
129 =smtp_pass=. STARTTLS when offered. Required for invite/open 129 =from=, optional =smtp_user= / =smtp_pass=. Required for invite/open
130 registration and self-service =email add=; in closed mode you may omit 130 registration and self-service =email add=; in closed mode you may omit
131 it entirely and assert addresses by hand (below). 131 it entirely and assert addresses by hand (below).
132- =tls= — =starttls= (default) or =implicit= (TLS from the first byte,
133 for relays on 465).
134- =require_tls= — with =starttls=, a relay that does not offer STARTTLS
135 gets no mail: delivery fails and retries, and the admin page's Mail
136 table shows the error. Unset, it is on for every relay except
137 =localhost= and loopback addresses; set =false= to allow plaintext
138 to a remote relay.
132 139
133** [push] 140** [push]
134Push notifications to Apple devices, delivered by gitbayd talking to 141Push notifications to Apple devices, delivered by gitbayd talking to
.gitbay/wiki/Architecture/03-Deployment.org +1 −1
@@ -57,7 +57,7 @@ a database check.
57| Destination | Trigger | TLS | Guard | 57| Destination | Trigger | TLS | Guard |
58|------------------------+-------------------------------+----------------------------------------------+----------------------------------------------------------------| 58|------------------------+-------------------------------+----------------------------------------------+----------------------------------------------------------------|
59| ACME directory | certificate issue and renewal | yes | host policy limits names to the site and claimed pages domains (=main.go=) | 59| ACME directory | certificate issue and renewal | yes | host policy limits names to the site and claimed pages domains (=main.go=) |
60| SMTP relay | queued mail | STARTTLS when offered | Go's =PlainAuth= will not send credentials over plaintext to a non-local host (=internal/mail/mail.go=) | 60| SMTP relay | queued mail | STARTTLS required for a non-local relay, or implicit TLS | =mail.require_tls=; Go's =PlainAuth= will not send credentials over plaintext to a non-local host (=internal/mail/mail.go=) |
61| APNs | queued push | yes, HTTP/2 | provider token signed with the operator's .p8 key | 61| APNs | queued push | yes, HTTP/2 | provider token signed with the operator's .p8 key |
62| Webhook URLs | recorded events | yes when https; certificate verified | private, loopback and link-local targets refused at save and again at connect time; no redirects (=internal/webhook/webhook.go=) | 62| Webhook URLs | recorded events | yes when https; certificate verified | private, loopback and link-local targets refused at save and again at connect time; no redirects (=internal/webhook/webhook.go=) |
63| Mirror URLs | mirror schedule | per URL | the same address check at save time (=internal/control/mirrorcmd.go=); git makes the connection, so there is no connect-time re-check | 63| Mirror URLs | mirror schedule | per URL | the same address check at save time (=internal/control/mirrorcmd.go=); git makes the connection, so there is no connect-time re-check |
.gitbay/wiki/Architecture/06-Data-and-Cryptography.org +1 −1
@@ -60,7 +60,7 @@ secret, webhook secret and mirror token.
60| HTTP port 80 | ACME challenges and redirect only | 60| HTTP port 80 | ACME challenges and redirect only |
61| git:// | none (public data only; off by default) | 61| git:// | none (public data only; off by default) |
62| Runner ↔ server | SSH | 62| Runner ↔ server | SSH |
63| SMTP | STARTTLS when the relay offers it | 63| SMTP | STARTTLS required unless the relay is local (=mail.require_tls=), or implicit TLS (=mail.tls=) |
64| APNs | TLS, HTTP/2 | 64| APNs | TLS, HTTP/2 |
65| Webhooks | TLS when the URL is https; HMAC-SHA256 body signature in =X-Gitbay-Signature-256= (=internal/webhook/webhook.go=) | 65| Webhooks | TLS when the URL is https; HMAC-SHA256 body signature in =X-Gitbay-Signature-256= (=internal/webhook/webhook.go=) |
66| Mirrors | per URL; token passed through =GIT_ASKPASS=, never argv (=internal/mirror/mirror.go=) | 66| Mirrors | per URL; token passed through =GIT_ASKPASS=, never argv (=internal/mirror/mirror.go=) |
.gitbay/wiki/Architecture/09-Controls.org +1 −1
@@ -77,7 +77,7 @@ chapter names of OWASP ASVS 4.0 where one fits.
77|---------------------------------------------+----------+------------------------------------------------------------------| 77|---------------------------------------------+----------+------------------------------------------------------------------|
78| SSRF protection on user-supplied URLs | partial | webhooks at save and connect; mirrors at save only (#279) | 78| SSRF protection on user-supplied URLs | partial | webhooks at save and connect; mirrors at save only (#279) |
79| Webhook payload integrity | in place | HMAC-SHA256 header | 79| Webhook payload integrity | in place | HMAC-SHA256 header |
80| SMTP credentials protected in transit | partial | STARTTLS opportunistic (#280); Go refuses PLAIN auth without TLS to a remote host | 80| SMTP credentials protected in transit | in place | STARTTLS required for non-local relays, implicit TLS optional (=internal/mail/mail.go=) |
81| Upload size limits | in place | per-owner storage quota at push (=internal/sshd/sshd.go=); API body 1 MiB | 81| Upload size limits | in place | per-owner storage quota at push (=internal/sshd/sshd.go=); API body 1 MiB |
82 82
83** CI and build isolation 83** CI and build isolation
.gitbay/wiki/Architecture/10-Known-Gaps.org −1
@@ -20,7 +20,6 @@ what the 2026-09-27 review found; remove a row when its issue closes.
20| #274 | Backups | The local backup archive is not encrypted | medium | 20| #274 | Backups | The local backup archive is not encrypted | medium |
21| #275 | Audit | Refused writes are not audited; the audit table is writable by the daemon user | medium | 21| #275 | Audit | Refused writes are not audited; the audit table is writable by the daemon user | medium |
22| #279 | SSRF | Mirror URLs are checked when saved, not when git connects | medium | 22| #279 | SSRF | Mirror URLs are checked when saved, not when git connects | medium |
23| #280 | Mail | STARTTLS only when the relay offers it | medium |
24| #282 | Hook socket | Anything that can open =hook.sock= can act as any user | medium | 23| #282 | Hook socket | Anything that can open =hook.sock= can act as any user | medium |
25| #297 | Credentials | A browser session can mint tokens and keys that outlive it | low | 24| #297 | Credentials | A browser session can mint tokens and keys that outlive it | low |
26 25
CHANGELOG.org +4
@@ -35,6 +35,10 @@ must add =--scope full=. Existing tokens keep their scope.
35 the login page's mailed links have (#278). 35 the login page's mailed links have (#278).
36- The HTTPS listener refuses TLS below 1.2, in both certificate modes 36- The HTTPS listener refuses TLS below 1.2, in both certificate modes
37 (#281). 37 (#281).
38- Mail to a non-local relay requires TLS by default; a relay that does
39 not offer STARTTLS gets no mail unless =mail.require_tls = false=
40 restores the old behaviour. =mail.tls = "implicit"= speaks TLS from
41 the first byte, for relays on port 465 (#280).
38 42
39* v1.36.0 — 2026-09-23 43* v1.36.0 — 2026-09-23
40 44
internal/config/config.go +28 −1
@@ -209,10 +209,34 @@ type Limits struct {
209} 209}
210 210
211type Mail struct { 211type Mail struct {
212 SMTPHost string `toml:"smtp_host"` // host:port (port defaults to 587) 212 SMTPHost string `toml:"smtp_host"` // host:port (port defaults to 587, 465 with tls = "implicit")
213 From string `toml:"from"` 213 From string `toml:"from"`
214 SMTPUser string `toml:"smtp_user,omitempty"` 214 SMTPUser string `toml:"smtp_user,omitempty"`
215 SMTPPass string `toml:"smtp_pass,omitempty"` 215 SMTPPass string `toml:"smtp_pass,omitempty"`
216 // RequireTLS fails delivery when the relay does not offer STARTTLS,
217 // instead of sending in clear. Unset, it is on for any relay but
218 // localhost or a loopback address (TLSRequired).
219 RequireTLS *bool `toml:"require_tls,omitempty"`
220 // TLS is "starttls" (the default, also when empty) or "implicit":
221 // TLS from the first byte, as relays on port 465 expect.
222 TLS string `toml:"tls,omitempty"`
223}
224
225// TLSRequired reports whether mail must not go to the relay in clear.
226func (m Mail) TLSRequired() bool {
227 if m.RequireTLS != nil {
228 return *m.RequireTLS
229 }
230 host := m.SMTPHost
231 if h, _, err := net.SplitHostPort(host); err == nil {
232 host = h
233 }
234 host = strings.Trim(host, "[]")
235 if host == "localhost" {
236 return false
237 }
238 ip := net.ParseIP(host)
239 return ip == nil || !ip.IsLoopback()
216} 240}
217 241
218// Push is APNs delivery to registered Apple devices. A key belongs to a 242// Push is APNs delivery to registered Apple devices. A key belongs to a
@@ -406,6 +430,9 @@ func (c Config) Validate() error {
406 if c.Mail.SMTPHost != "" && c.Mail.From == "" { 430 if c.Mail.SMTPHost != "" && c.Mail.From == "" {
407 errs = append(errs, errors.New("[mail] from is required when smtp_host is set")) 431 errs = append(errs, errors.New("[mail] from is required when smtp_host is set"))
408 } 432 }
433 if t := c.Mail.TLS; t != "" && t != "starttls" && t != "implicit" {
434 errs = append(errs, fmt.Errorf("mail.tls must be starttls or implicit, got %q", t))
435 }
409 if c.Registration.Mode != "closed" && c.Mail.SMTPHost == "" { 436 if c.Registration.Mode != "closed" && c.Mail.SMTPHost == "" {
410 errs = append(errs, fmt.Errorf( 437 errs = append(errs, fmt.Errorf(
411 "registration.mode = %q requires [mail] smtp_host: email verification cannot run without SMTP", 438 "registration.mode = %q requires [mail] smtp_host: email verification cannot run without SMTP",
internal/config/config_test.go +26
@@ -65,6 +65,11 @@ func TestContradictions(t *testing.T) {
65 minimal + "\n[ssh]\nmode = \"system\"\n[registration]\nmode = \"open\"\n[mail]\nsmtp_host = \"mx.example\"\nfrom = \"gitbay@example\"\n", 65 minimal + "\n[ssh]\nmode = \"system\"\n[registration]\nmode = \"open\"\n[mail]\nsmtp_host = \"mx.example\"\nfrom = \"gitbay@example\"\n",
66 "requires registration.mode = \"closed\"", 66 "requires registration.mode = \"closed\"",
67 }, 67 },
68 {
69 "unknown mail.tls",
70 minimal + "\n[mail]\nsmtp_host = \"mx.example\"\nfrom = \"gitbay@example\"\ntls = \"ssl\"\n",
71 "mail.tls must be starttls or implicit",
72 },
68 { 73 {
69 "password auth in view_only", 74 "password auth in view_only",
70 minimal + "\n[web]\nmode = \"view_only\"\npassword_auth = true\n", 75 minimal + "\n[web]\nmode = \"view_only\"\npassword_auth = true\n",
@@ -249,3 +254,24 @@ func TestPushHost(t *testing.T) {
249 t.Fatalf("GITBAY_APNS_HOST ignored: %q", got) 254 t.Fatalf("GITBAY_APNS_HOST ignored: %q", got)
250 } 255 }
251} 256}
257
258func TestMailTLSRequired(t *testing.T) {
259 off, on := false, true
260 for _, tc := range []struct {
261 m Mail
262 want bool
263 }{
264 {Mail{SMTPHost: "smtp.example.com:587"}, true},
265 {Mail{SMTPHost: "smtp.example.com"}, true},
266 {Mail{SMTPHost: "localhost:25"}, false},
267 {Mail{SMTPHost: "localhost"}, false},
268 {Mail{SMTPHost: "127.0.0.1:25"}, false},
269 {Mail{SMTPHost: "[::1]:25"}, false},
270 {Mail{SMTPHost: "smtp.example.com:587", RequireTLS: &off}, false},
271 {Mail{SMTPHost: "127.0.0.1:25", RequireTLS: &on}, true},
272 } {
273 if got := tc.m.TLSRequired(); got != tc.want {
274 t.Errorf("%+v: TLSRequired = %v, want %v", tc.m, got, tc.want)
275 }
276 }
277}
internal/mail/mail.go +41 −7
@@ -1,10 +1,13 @@
1// Package mail sends transactional email over SMTP: verification codes and 1// Package mail sends transactional email over SMTP: verification codes and
2// invites. STARTTLS is used when the server offers it; PLAIN auth when 2// invites. The connection is encrypted with STARTTLS, or with TLS from the
3// credentials are configured. 3// first byte when mail.tls = "implicit"; a relay that offers neither gets
4// nothing unless mail.require_tls is off. PLAIN auth when credentials are
5// configured.
4package mail 6package mail
5 7
6import ( 8import (
7 "crypto/tls" 9 "crypto/tls"
10 "crypto/x509"
8 "fmt" 11 "fmt"
9 "net" 12 "net"
10 "net/smtp" 13 "net/smtp"
@@ -14,30 +17,43 @@ import (
14 "gitbay.org/gitbay/internal/config" 17 "gitbay.org/gitbay/internal/config"
15) 18)
16 19
20// rootCAs verifies the relay's certificate; nil is the system pool.
21var rootCAs *x509.CertPool
22
17// Send delivers one plain-text message. cfg.Mail.SMTPHost is host:port. 23// Send delivers one plain-text message. cfg.Mail.SMTPHost is host:port.
18func Send(cfg config.Config, to, subject, body string) error { 24func Send(cfg config.Config, to, subject, body string) error {
19 m := cfg.Mail 25 m := cfg.Mail
20 if m.SMTPHost == "" || m.From == "" { 26 if m.SMTPHost == "" || m.From == "" {
21 return fmt.Errorf("[mail] smtp_host and from must be configured") 27 return fmt.Errorf("[mail] smtp_host and from must be configured")
22 } 28 }
29 implicit := m.TLS == "implicit"
23 host := m.SMTPHost 30 host := m.SMTPHost
24 if !strings.Contains(host, ":") { 31 if !strings.Contains(host, ":") {
25 host += ":587" 32 if implicit {
33 host += ":465"
34 } else {
35 host += ":587"
36 }
26 } 37 }
27 hostname, _, _ := net.SplitHostPort(host) 38 hostname, _, _ := net.SplitHostPort(host)
39 tlsCfg := &tls.Config{ServerName: hostname, RootCAs: rootCAs}
28 40
29 msg := strings.NewReplacer("\n", "\r\n").Replace(fmt.Sprintf( 41 msg := strings.NewReplacer("\n", "\r\n").Replace(fmt.Sprintf(
30 "From: %s\nTo: %s\nSubject: %s\nDate: %s\nMIME-Version: 1.0\nContent-Type: text/plain; charset=utf-8\n\n%s\n", 42 "From: %s\nTo: %s\nSubject: %s\nDate: %s\nMIME-Version: 1.0\nContent-Type: text/plain; charset=utf-8\n\n%s\n",
31 m.From, to, subject, time.Now().Format(time.RFC1123Z), body)) 43 m.From, to, subject, time.Now().Format(time.RFC1123Z), body))
32 44
33 c, err := smtp.Dial(host) 45 c, err := dial(host, hostname, implicit, tlsCfg)
34 if err != nil { 46 if err != nil {
35 return fmt.Errorf("smtp dial %s: %w", host, err) 47 return fmt.Errorf("smtp dial %s: %w", host, err)
36 } 48 }
37 defer c.Close() 49 defer c.Close()
38 if ok, _ := c.Extension("STARTTLS"); ok { 50 if !implicit {
39 if err := c.StartTLS(&tls.Config{ServerName: hostname}); err != nil { 51 if ok, _ := c.Extension("STARTTLS"); ok {
40 return fmt.Errorf("starttls: %w", err) 52 if err := c.StartTLS(tlsCfg); err != nil {
53 return fmt.Errorf("starttls: %w", err)
54 }
55 } else if m.TLSRequired() {
56 return fmt.Errorf("%s does not offer STARTTLS and mail.require_tls is on; not sending in clear", host)
41 } 57 }
42 } 58 }
43 if m.SMTPUser != "" { 59 if m.SMTPUser != "" {
@@ -63,3 +79,21 @@ func Send(cfg config.Config, to, subject, body string) error {
63 } 79 }
64 return c.Quit() 80 return c.Quit()
65} 81}
82
83// dial opens the SMTP session: plain TCP for STARTTLS, or TLS from the
84// first byte.
85func dial(addr, hostname string, implicit bool, tlsCfg *tls.Config) (*smtp.Client, error) {
86 if !implicit {
87 return smtp.Dial(addr)
88 }
89 conn, err := tls.Dial("tcp", addr, tlsCfg)
90 if err != nil {
91 return nil, err
92 }
93 c, err := smtp.NewClient(conn, hostname)
94 if err != nil {
95 conn.Close()
96 return nil, err
97 }
98 return c, nil
99}
internal/mail/mail_test.go added +142
@@ -0,0 +1,142 @@
1package mail
2
3import (
4 "bufio"
5 "crypto/tls"
6 "crypto/x509"
7 "fmt"
8 "net"
9 "net/http"
10 "net/http/httptest"
11 "strings"
12 "sync"
13 "testing"
14
15 "gitbay.org/gitbay/internal/config"
16)
17
18// fakeRelay is an SMTP server that never offers STARTTLS. Given a TLS
19// config it speaks TLS from the first byte, as a port-465 relay does.
20type fakeRelay struct {
21 addr string
22 mu sync.Mutex
23 data []string
24}
25
26func startRelay(t *testing.T, tlsCfg *tls.Config) *fakeRelay {
27 t.Helper()
28 ln, err := net.Listen("tcp", "127.0.0.1:0")
29 if err != nil {
30 t.Fatal(err)
31 }
32 if tlsCfg != nil {
33 ln = tls.NewListener(ln, tlsCfg)
34 }
35 t.Cleanup(func() { ln.Close() })
36 f := &fakeRelay{addr: ln.Addr().String()}
37 go func() {
38 for {
39 conn, err := ln.Accept()
40 if err != nil {
41 return
42 }
43 go f.serve(conn)
44 }
45 }()
46 return f
47}
48
49func (f *fakeRelay) serve(conn net.Conn) {
50 defer conn.Close()
51 r := bufio.NewReader(conn)
52 fmt.Fprint(conn, "220 fake\r\n")
53 var body strings.Builder
54 inData := false
55 for {
56 line, err := r.ReadString('\n')
57 if err != nil {
58 return
59 }
60 line = strings.TrimRight(line, "\r\n")
61 switch {
62 case inData && line == ".":
63 f.mu.Lock()
64 f.data = append(f.data, body.String())
65 f.mu.Unlock()
66 inData = false
67 fmt.Fprint(conn, "250 ok\r\n")
68 case inData:
69 body.WriteString(line + "\n")
70 case strings.HasPrefix(line, "EHLO"), strings.HasPrefix(line, "HELO"):
71 fmt.Fprint(conn, "250-fake\r\n250 SIZE 1000000\r\n")
72 case line == "DATA":
73 inData = true
74 fmt.Fprint(conn, "354 go\r\n")
75 case line == "QUIT":
76 fmt.Fprint(conn, "221 bye\r\n")
77 return
78 default:
79 fmt.Fprint(conn, "250 ok\r\n")
80 }
81 }
82}
83
84func (f *fakeRelay) delivered() int {
85 f.mu.Lock()
86 defer f.mu.Unlock()
87 return len(f.data)
88}
89
90func mailCfg(host string) config.Config {
91 var cfg config.Config
92 cfg.Mail.SMTPHost, cfg.Mail.From = host, "gitbay@example.test"
93 return cfg
94}
95
96func TestRequireTLSRefusesPlaintextRelay(t *testing.T) {
97 relay := startRelay(t, nil)
98 cfg := mailCfg(relay.addr)
99 on := true
100 cfg.Mail.RequireTLS = &on
101 err := Send(cfg, "a@example.test", "subject", "body")
102 if err == nil || !strings.Contains(err.Error(), "STARTTLS") {
103 t.Fatalf("Send = %v, want a refusal naming STARTTLS", err)
104 }
105 if n := relay.delivered(); n != 0 {
106 t.Fatalf("%d message(s) sent in clear", n)
107 }
108}
109
110// A loopback relay has no network to cross; the default leaves it in
111// clear, which is what the e2e suite's fake relay relies on.
112func TestLoopbackRelayDefaultsToPlaintext(t *testing.T) {
113 relay := startRelay(t, nil)
114 if err := Send(mailCfg(relay.addr), "a@example.test", "subject", "body"); err != nil {
115 t.Fatal(err)
116 }
117 if n := relay.delivered(); n != 1 {
118 t.Fatalf("delivered %d, want 1", n)
119 }
120}
121
122func TestImplicitTLS(t *testing.T) {
123 ts := httptest.NewTLSServer(http.NotFoundHandler())
124 defer ts.Close()
125 pool := x509.NewCertPool()
126 pool.AddCert(ts.Certificate())
127 prev := rootCAs
128 rootCAs = pool
129 defer func() { rootCAs = prev }()
130
131 relay := startRelay(t, &tls.Config{Certificates: ts.TLS.Certificates})
132 cfg := mailCfg(relay.addr)
133 cfg.Mail.TLS = "implicit"
134 on := true
135 cfg.Mail.RequireTLS = &on
136 if err := Send(cfg, "a@example.test", "subject", "body"); err != nil {
137 t.Fatal(err)
138 }
139 if n := relay.delivered(); n != 1 {
140 t.Fatalf("delivered %d, want 1", n)
141 }
142}