Improve SMTP authentication and Fix user creation bugs (#16612)

* Improve SMTP authentication, Fix user creation bugs and add LDAP cert/key options

This PR has two parts:

Improvements for SMTP authentication:

* Default to use SMTPS if port is 465, and allow setting of force SMTPS.
* Always use STARTTLS if available
* Provide CRAM-MD5 mechanism
* Add options for HELO hostname disabling
* Add options for providing certificates and keys
* Handle application specific password response as a failed user login
instead of as a 500.

Close #16104

Fix creation of new users:

* A bug was introduced when allowing users to change usernames which
prevents the creation of external users.
* The LoginSource refactor also broke this page.

Close #16104

Signed-off-by: Andrew Thornton <art27@cantab.net>

docs/content/doc/features/authentication.en-us.md

@@ -201,16 +201,18 @@ configure this, set the fields below:
     with multiple domains.
   - Example: `gitea.io,mydomain.com,mydomain2.com`
 
-- Enable TLS Encryption
+- Force SMTPS
 
-  - Enable TLS encryption on authentication.
+  - SMTPS will be used by default for connections to port 465, if you wish to use SMTPS 
+  for other ports. Set this value.
+  - Otherwise if the server provides the `STARTTLS` extension this will be used.
 
 - Skip TLS Verify
 
   - Disable TLS verify on authentication.
 
-- This authentication is activate
+- This Authentication Source is Activated
-  - Enable or disable this auth.
+  - Enable or disable this authentication source.
 
 ## FreeIPA
 

options/locale/locale_en-US.ini

@@ -2427,8 +2427,12 @@ auths.smtphost = SMTP Host
 auths.smtpport = SMTP Port
 auths.allowed_domains = Allowed Domains
 auths.allowed_domains_helper = Leave empty to allow all domains. Separate multiple domains with a comma (',').
-auths.enable_tls = Enable TLS Encryption
 auths.skip_tls_verify = Skip TLS Verify
+auths.force_smtps = Force SMTPS
+auths.force_smtps_helper = By default SMTPS will be used for port 465, set this to use smtps on other ports, otherwise STARTTLS is used if supported.
+auths.helo_hostname = HELO Hostname
+auths.helo_hostname_helper = Hostname sent with HELO. Leave blank to send current hostname.
+auths.disable_helo = Disable HELO
 auths.pam_service_name = PAM Service Name
 auths.pam_email_domain = PAM Email Domain (optional)
 auths.oauth2_provider = OAuth2 Provider

routers/web/admin/auths.go

@@ -154,8 +154,10 @@ func parseSMTPConfig(form forms.AuthenticationForm) *smtp.Source {
 		Host:           form.SMTPHost,
 		Port:           form.SMTPPort,
 		AllowedDomains: form.AllowedDomains,
-		TLS:            form.TLS,
+		ForceSMTPS:     form.ForceSMTPS,
 		SkipVerify:     form.SkipVerify,
+		HeloHostname:   form.HeloHostname,
+		DisableHelo:    form.DisableHelo,
 	}
 }
 

services/auth/source/ldap/source_search.go

@@ -8,6 +8,8 @@ package ldap
 import (
 	"crypto/tls"
 	"fmt"
+	"net"
+	"strconv"
 	"strings"
 
 	"code.gitea.io/gitea/modules/log"
@@ -103,26 +105,27 @@ func (ls *Source) findUserDN(l *ldap.Conn, name string) (string, bool) {
 	return userDN, true
 }
 
-func dial(ls *Source) (*ldap.Conn, error) {
+func dial(source *Source) (*ldap.Conn, error) {
-	log.Trace("Dialing LDAP with security protocol (%v) without verifying: %v", ls.SecurityProtocol, ls.SkipVerify)
+	log.Trace("Dialing LDAP with security protocol (%v) without verifying: %v", source.SecurityProtocol, source.SkipVerify)
 
-	tlsCfg := &tls.Config{
+	tlsConfig := &tls.Config{
-		ServerName:         ls.Host,
+		ServerName:         source.Host,
-		InsecureSkipVerify: ls.SkipVerify,
+		InsecureSkipVerify: source.SkipVerify,
-	}
-	if ls.SecurityProtocol == SecurityProtocolLDAPS {
-		return ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", ls.Host, ls.Port), tlsCfg)
 	}
 
-	conn, err := ldap.Dial("tcp", fmt.Sprintf("%s:%d", ls.Host, ls.Port))
+	if source.SecurityProtocol == SecurityProtocolLDAPS {
+		return ldap.DialTLS("tcp", net.JoinHostPort(source.Host, strconv.Itoa(source.Port)), tlsConfig)
+	}
+
+	conn, err := ldap.Dial("tcp", net.JoinHostPort(source.Host, strconv.Itoa(source.Port)))
 	if err != nil {
-		return nil, fmt.Errorf("Dial: %v", err)
+		return nil, fmt.Errorf("error during Dial: %v", err)
 	}
 
-	if ls.SecurityProtocol == SecurityProtocolStartTLS {
+	if source.SecurityProtocol == SecurityProtocolStartTLS {
-		if err = conn.StartTLS(tlsCfg); err != nil {
+		if err = conn.StartTLS(tlsConfig); err != nil {
 			conn.Close()
-			return nil, fmt.Errorf("StartTLS: %v", err)
+			return nil, fmt.Errorf("error during StartTLS: %v", err)
 		}
 	}
 

services/auth/source/smtp/auth.go

@@ -6,9 +6,11 @@ package smtp
 
 import (
 	"crypto/tls"
-	"errors"
 	"fmt"
+	"net"
 	"net/smtp"
+	"os"
+	"strconv"
 
 	"code.gitea.io/gitea/models"
 )
@@ -42,40 +44,62 @@ func (auth *loginAuthenticator) Next(fromServer []byte, more bool) ([]byte, erro
 
 // SMTP authentication type names.
 const (
-	PlainAuthentication = "PLAIN"
+	PlainAuthentication   = "PLAIN"
-	LoginAuthentication = "LOGIN"
+	LoginAuthentication   = "LOGIN"
+	CRAMMD5Authentication = "CRAM-MD5"
 )
 
 // Authenticators contains available SMTP authentication type names.
-var Authenticators = []string{PlainAuthentication, LoginAuthentication}
+var Authenticators = []string{PlainAuthentication, LoginAuthentication, CRAMMD5Authentication}
 
 // Authenticate performs an SMTP authentication.
 func Authenticate(a smtp.Auth, source *Source) error {
-	c, err := smtp.Dial(fmt.Sprintf("%s:%d", source.Host, source.Port))
+	tlsConfig := &tls.Config{
+		InsecureSkipVerify: source.SkipVerify,
+		ServerName:         source.Host,
+	}
+
+	conn, err := net.Dial("tcp", net.JoinHostPort(source.Host, strconv.Itoa(source.Port)))
 	if err != nil {
 		return err
 	}
-	defer c.Close()
+	defer conn.Close()
 
-	if err = c.Hello("gogs"); err != nil {
+	if source.UseTLS() {
-		return err
+		conn = tls.Client(conn, tlsConfig)
 	}
 
-	if source.TLS {
+	client, err := smtp.NewClient(conn, source.Host)
-		if ok, _ := c.Extension("STARTTLS"); ok {
+	if err != nil {
-			if err = c.StartTLS(&tls.Config{
+		return fmt.Errorf("failed to create NewClient: %w", err)
-				InsecureSkipVerify: source.SkipVerify,
+	}
-				ServerName:         source.Host,
+	defer client.Close()
-			}); err != nil {
+
-				return err
+	if !source.DisableHelo {
+		hostname := source.HeloHostname
+		if len(hostname) == 0 {
+			hostname, err = os.Hostname()
+			if err != nil {
+				return fmt.Errorf("failed to find Hostname: %w", err)
 			}
-		} else {
+		}
-			return errors.New("SMTP server unsupports TLS")
+
+		if err = client.Hello(hostname); err != nil {
+			return fmt.Errorf("failed to send Helo: %w", err)
 		}
 	}
 
-	if ok, _ := c.Extension("AUTH"); ok {
+	// If not using SMTPS, always use STARTTLS if available
-		return c.Auth(a)
+	hasStartTLS, _ := client.Extension("STARTTLS")
+	if !source.UseTLS() && hasStartTLS {
+		if err = client.StartTLS(tlsConfig); err != nil {
+			return fmt.Errorf("failed to start StartTLS: %v", err)
+		}
 	}
+
+	if ok, _ := client.Extension("AUTH"); ok {
+		return client.Auth(a)
+	}
+
 	return models.ErrUnsupportedLoginType
 }

services/auth/source/smtp/source.go

@@ -22,8 +22,10 @@ type Source struct {
 	Host           string
 	Port           int
 	AllowedDomains string `xorm:"TEXT"`
-	TLS            bool
+	ForceSMTPS     bool
 	SkipVerify     bool
+	HeloHostname   string
+	DisableHelo    bool
 
 	// reference to the loginSource
 	loginSource *models.LoginSource
@@ -51,7 +53,7 @@ func (source *Source) HasTLS() bool {
 
 // UseTLS returns if TLS is set
 func (source *Source) UseTLS() bool {
-	return source.TLS
+	return source.ForceSMTPS || source.Port == 465
 }
 
 // SetLoginSource sets the related LoginSource

services/auth/source/smtp/source_authenticate.go

@@ -28,12 +28,15 @@ func (source *Source) Authenticate(user *models.User, login, password string) (*
 	}
 
 	var auth smtp.Auth
-	if source.Auth == PlainAuthentication {
+	switch source.Auth {
+	case PlainAuthentication:
 		auth = smtp.PlainAuth("", login, password, source.Host)
-	} else if source.Auth == LoginAuthentication {
+	case LoginAuthentication:
 		auth = &loginAuthenticator{login, password}
-	} else {
+	case CRAMMD5Authentication:
-		return nil, errors.New("Unsupported SMTP auth type")
+		auth = smtp.CRAMMD5Auth(login, password)
+	default:
+		return nil, errors.New("unsupported SMTP auth type")
 	}
 
 	if err := Authenticate(auth, source); err != nil {
@@ -44,6 +47,10 @@ func (source *Source) Authenticate(user *models.User, login, password string) (*
 			strings.Contains(err.Error(), "Username and Password not accepted") {
 			return nil, models.ErrUserNotExist{Name: login}
 		}
+		if (ok && tperr.Code == 534) ||
+			strings.Contains(err.Error(), "Application-specific password required") {
+			return nil, models.ErrUserNotExist{Name: login}
+		}
 		return nil, err
 	}
 

services/forms/auth_form.go

@@ -50,6 +50,9 @@ type AuthenticationForm struct {
 	SecurityProtocol              int `binding:"Range(0,2)"`
 	TLS                           bool
 	SkipVerify                    bool
+	HeloHostname                  string
+	DisableHelo                   bool
+	ForceSMTPS                    bool
 	PAMServiceName                string
 	PAMEmailDomain                string
 	Oauth2Provider                string

templates/admin/auth/edit.tmpl

@@ -44,6 +44,12 @@
 						<label for="port">{{.i18n.Tr "admin.auths.port"}}</label>
 						<input id="port" name="port" value="{{$cfg.Port}}"  placeholder="e.g. 636" required>
 					</div>
+					<div class="has-tls inline field {{if not .HasTLS}}hide{{end}}">
+						<div class="ui checkbox">
+							<label><strong>{{.i18n.Tr "admin.auths.skip_tls_verify"}}</strong></label>
+							<input name="skip_verify" type="checkbox" {{if .Source.SkipVerify}}checked{{end}}>
+						</div>
+					</div>
 					{{if .Source.IsLDAP}}
 						<div class="field">
 							<label for="bind_dn">{{.i18n.Tr "admin.auths.bind_dn"}}</label>
@@ -173,6 +179,30 @@
 						<label for="smtp_port">{{.i18n.Tr "admin.auths.smtpport"}}</label>
 						<input id="smtp_port" name="smtp_port" value="{{$cfg.Port}}" required>
 					</div>
+					<div class="field">
+						<div class="ui checkbox">
+							<label for="force_smtps"><strong>{{.i18n.Tr "admin.auths.force_smtps"}}</strong></label>
+							<input id="force_smtps" name="force_smtps" type="checkbox" {{if $cfg.ForceSMTPS}}checked{{end}}>
+						</div>
+						<p class="help">{{.i18n.Tr "admin.auths.force_smtps_helper"}}</p>
+					</div>
+					<div class="has-tls inline field {{if not .HasTLS}}hide{{end}}">
+						<div class="ui checkbox">
+							<label><strong>{{.i18n.Tr "admin.auths.skip_tls_verify"}}</strong></label>
+							<input name="skip_verify" type="checkbox" {{if .Source.SkipVerify}}checked{{end}}>
+						</div>
+					</div>
+					<div class="field">
+						<label for="helo_hostname">{{.i18n.Tr "admin.auths.helo_hostname"}}</label>
+						<input id="helo_hostname" name="helo_hostname" value="{{$cfg.HeloHostname}}">
+						<p class="help">{{.i18n.Tr "admin.auths.helo_hostname_helper"}}</p>
+					</div>
+					<div class="inline field">
+						<div class="ui checkbox">
+							<label for="disable_helo"><strong>{{.i18n.Tr "admin.auths.disable_helo"}}</strong></label>
+							<input id="disable_helo" name="disable_helo" type="checkbox" {{if $cfg.DisableHelo}}checked{{end}}>
+						</div>
+					</div>
 					<div class="field">
 						<label for="allowed_domains">{{.i18n.Tr "admin.auths.allowed_domains"}}</label>
 						<input id="allowed_domains" name="allowed_domains" value="{{$cfg.AllowedDomains}}">
@@ -308,26 +338,13 @@
 						<p class="help">{{.i18n.Tr "admin.auths.sspi_default_language_helper"}}</p>
 					</div>
 				{{end}}
-
-				<div class="inline field {{if not .Source.IsSMTP}}hide{{end}}">
-					<div class="ui checkbox">
-						<label><strong>{{.i18n.Tr "admin.auths.enable_tls"}}</strong></label>
-						<input name="tls" type="checkbox" {{if .Source.UseTLS}}checked{{end}}>
-					</div>
-				</div>
-				<div class="has-tls inline field {{if not .HasTLS}}hide{{end}}">
-					<div class="ui checkbox">
-						<label><strong>{{.i18n.Tr "admin.auths.skip_tls_verify"}}</strong></label>
-						<input name="skip_verify" type="checkbox" {{if .Source.SkipVerify}}checked{{end}}>
-					</div>
-				</div>
 				{{if .Source.IsLDAP}}
-				<div class="inline field">
+					<div class="inline field">
-					<div class="ui checkbox">
+						<div class="ui checkbox">
-						<label><strong>{{.i18n.Tr "admin.auths.syncenabled"}}</strong></label>
+							<label><strong>{{.i18n.Tr "admin.auths.syncenabled"}}</strong></label>
-						<input name="is_sync_enabled" type="checkbox" {{if .Source.IsSyncEnabled}}checked{{end}}>
+							<input name="is_sync_enabled" type="checkbox" {{if .Source.IsSyncEnabled}}checked{{end}}>
+						</div>
 					</div>
-				</div>
 				{{end}}
 				<div class="inline field">
 					<div class="ui checkbox">

templates/admin/auth/new.tmpl

@@ -54,18 +54,6 @@
 						<input name="attributes_in_bind" type="checkbox" {{if .attributes_in_bind}}checked{{end}}>
 					</div>
 				</div>
-				<div class="smtp inline field {{if not (eq .type 3)}}hide{{end}}">
-					<div class="ui checkbox">
-						<label><strong>{{.i18n.Tr "admin.auths.enable_tls"}}</strong></label>
-						<input name="tls" type="checkbox" {{if .tls}}checked{{end}}>
-					</div>
-				</div>
-				<div class="has-tls inline field {{if not .HasTLS}}hide{{end}}">
-					<div class="ui checkbox">
-						<label><strong>{{.i18n.Tr "admin.auths.skip_tls_verify"}}</strong></label>
-						<input name="skip_verify" type="checkbox" {{if .skip_verify}}checked{{end}}>
-					</div>
-				</div>
 				<div class="ldap inline field {{if not (eq .type 2)}}hide{{end}}">
 					<div class="ui checkbox">
 						<label><strong>{{.i18n.Tr "admin.auths.syncenabled"}}</strong></label>

templates/admin/auth/source/ldap.tmpl

@@ -20,6 +20,12 @@
 		<label for="port">{{.i18n.Tr "admin.auths.port"}}</label>
 		<input id="port" name="port" value="{{.port}}"  placeholder="e.g. 636">
 	</div>
+	<div class="has-tls inline field {{if not .HasTLS}}hide{{end}}">
+		<div class="ui checkbox">
+			<label><strong>{{.i18n.Tr "admin.auths.skip_tls_verify"}}</strong></label>
+			<input name="skip_verify" type="checkbox" {{if .skip_verify}}checked{{end}}>
+		</div>
+	</div>
 	<div class="ldap field {{if not (eq .type 2)}}hide{{end}}">
 		<label for="bind_dn">{{.i18n.Tr "admin.auths.bind_dn"}}</label>
 		<input id="bind_dn" name="bind_dn" value="{{.bind_dn}}" placeholder="e.g. cn=Search,dc=mydomain,dc=com">

templates/admin/auth/source/smtp.tmpl

@@ -20,6 +20,30 @@
 		<label for="smtp_port">{{.i18n.Tr "admin.auths.smtpport"}}</label>
 		<input id="smtp_port" name="smtp_port" value="{{.smtp_port}}">
 	</div>
+	<div class="inline field">
+		<div class="ui checkbox">
+			<label for="force_smtps"><strong>{{.i18n.Tr "admin.auths.force_smtps"}}</strong></label>
+			<input id="force_smtps" name="force_smtps" type="checkbox" {{if .force_smtps}}checked{{end}}>
+			<p class="help">{{.i18n.Tr "admin.auths.force_smtps_helper"}}</p>
+		</div>
+	</div>
+	<div class="inline field">
+		<div class="ui checkbox">
+			<label><strong>{{.i18n.Tr "admin.auths.skip_tls_verify"}}</strong></label>
+			<input name="skip_verify" type="checkbox" {{if .skip_verify}}checked{{end}}>
+		</div>
+	</div>
+	<div class="field">
+		<label for="helo_hostname">{{.i18n.Tr "admin.auths.helo_hostname"}}</label>
+		<input id="helo_hostname" name="helo_hostname" value="{{.helo_hostname}}">
+		<p class="help">{{.i18n.Tr "admin.auths.helo_hostname_helper"}}</p>
+	</div>
+	<div class="inline field">
+		<div class="ui checkbox">
+			<label for="disable_helo"><strong>{{.i18n.Tr "admin.auths.disable_helo"}}</strong></label>
+			<input id="disable_helo" name="disable_helo" type="checkbox" {{if .disable_helo}}checked{{end}}>
+		</div>
+	</div>
 	<div class="field">
 		<label for="allowed_domains">{{.i18n.Tr "admin.auths.allowed_domains"}}</label>
 		<input id="allowed_domains" name="allowed_domains" value="{{.allowed_domains}}">

templates/admin/user/edit.tmpl

@@ -17,13 +17,13 @@
 				<div class="inline required field {{if .Err_LoginType}}error{{end}}">
 					<label>{{.i18n.Tr "admin.users.auth_source"}}</label>
 					<div class="ui selection type dropdown">
-						<input type="hidden" id="login_type" name="login_type" value="{{.LoginSource.Type}}-{{.LoginSource.ID}}" required>
+						<input type="hidden" id="login_type" name="login_type" value="{{.LoginSource.Type.Int}}-{{.LoginSource.ID}}" required>
 						<div class="text">{{.i18n.Tr "admin.users.local"}}</div>
 						{{svg "octicon-triangle-down" 14 "dropdown icon"}}
 						<div class="menu">
 							<div class="item" data-value="0-0">{{.i18n.Tr "admin.users.local"}}</div>
 							{{range .Sources}}
-								<div class="item" data-value="{{.Type}}-{{.ID}}">{{.Name}}</div>
+								<div class="item" data-value="{{.Type.Int}}-{{.ID}}">{{.Name}}</div>
 							{{end}}
 						</div>
 					</div>

templates/admin/user/new.tmpl

@@ -19,7 +19,7 @@
 						<div class="menu">
 							<div class="item" data-value="0-0">{{.i18n.Tr "admin.users.local"}}</div>
 							{{range .Sources}}
-								<div class="item" data-value="{{.Type}}-{{.ID}}">{{.Name}}</div>
+								<div class="item" data-value="{{.Type.Int}}-{{.ID}}">{{.Name}}</div>
 							{{end}}
 						</div>
 					</div>

web_src/js/index.js

@@ -1992,7 +1992,9 @@ function initAdmin() {
           $('#password').attr('required', 'required');
         }
       } else {
-        $('#user_name').attr('disabled', 'disabled');
+        if ($('.admin.edit.user').length > 0) {
+          $('#user_name').attr('disabled', 'disabled');
+        }
         $('#login_name').attr('required', 'required');
         $('.non-local').show();
         $('.local').hide();