From e29e1637370ad95e4ca9f861c25d366b74829dcc Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 11 Aug 2021 21:42:58 +0100 Subject: [PATCH] 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 --- .../doc/features/authentication.en-us.md | 10 +-- options/locale/locale_en-US.ini | 6 +- routers/web/admin/auths.go | 4 +- services/auth/source/ldap/source_search.go | 29 +++++---- services/auth/source/smtp/auth.go | 62 +++++++++++++------ services/auth/source/smtp/source.go | 6 +- .../auth/source/smtp/source_authenticate.go | 15 +++-- services/forms/auth_form.go | 3 + templates/admin/auth/edit.tmpl | 53 ++++++++++------ templates/admin/auth/new.tmpl | 12 ---- templates/admin/auth/source/ldap.tmpl | 6 ++ templates/admin/auth/source/smtp.tmpl | 24 +++++++ templates/admin/user/edit.tmpl | 4 +- templates/admin/user/new.tmpl | 2 +- web_src/js/index.js | 4 +- 15 files changed, 162 insertions(+), 78 deletions(-) diff --git a/docs/content/doc/features/authentication.en-us.md b/docs/content/doc/features/authentication.en-us.md index 6a3938384..b075d4a26 100644 --- a/docs/content/doc/features/authentication.en-us.md +++ b/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 - - Enable or disable this auth. +- This Authentication Source is Activated + - Enable or disable this authentication source. ## FreeIPA diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index ccf19293f..9ec706157 100644 --- a/options/locale/locale_en-US.ini +++ b/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 diff --git a/routers/web/admin/auths.go b/routers/web/admin/auths.go index 2e9697533..342318e04 100644 --- a/routers/web/admin/auths.go +++ b/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, } } diff --git a/services/auth/source/ldap/source_search.go b/services/auth/source/ldap/source_search.go index e99fc6790..f2acbb0d4 100644 --- a/services/auth/source/ldap/source_search.go +++ b/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) { - log.Trace("Dialing LDAP with security protocol (%v) without verifying: %v", ls.SecurityProtocol, ls.SkipVerify) +func dial(source *Source) (*ldap.Conn, error) { + log.Trace("Dialing LDAP with security protocol (%v) without verifying: %v", source.SecurityProtocol, source.SkipVerify) - tlsCfg := &tls.Config{ - ServerName: ls.Host, - InsecureSkipVerify: ls.SkipVerify, - } - if ls.SecurityProtocol == SecurityProtocolLDAPS { - return ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", ls.Host, ls.Port), tlsCfg) + tlsConfig := &tls.Config{ + ServerName: source.Host, + InsecureSkipVerify: source.SkipVerify, } - 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 err = conn.StartTLS(tlsCfg); err != nil { + if source.SecurityProtocol == SecurityProtocolStartTLS { + 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) } } diff --git a/services/auth/source/smtp/auth.go b/services/auth/source/smtp/auth.go index 8edf4fca1..d797982da 100644 --- a/services/auth/source/smtp/auth.go +++ b/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" - LoginAuthentication = "LOGIN" + PlainAuthentication = "PLAIN" + 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 { - return err + if source.UseTLS() { + conn = tls.Client(conn, tlsConfig) } - if source.TLS { - if ok, _ := c.Extension("STARTTLS"); ok { - if err = c.StartTLS(&tls.Config{ - InsecureSkipVerify: source.SkipVerify, - ServerName: source.Host, - }); err != nil { - return err + client, err := smtp.NewClient(conn, source.Host) + if err != nil { + return fmt.Errorf("failed to create NewClient: %w", err) + } + defer client.Close() + + 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 { - return c.Auth(a) + // If not using SMTPS, always use STARTTLS if available + 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 } diff --git a/services/auth/source/smtp/source.go b/services/auth/source/smtp/source.go index 3d80aea68..39c9851ed 100644 --- a/services/auth/source/smtp/source.go +++ b/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 diff --git a/services/auth/source/smtp/source_authenticate.go b/services/auth/source/smtp/source_authenticate.go index 9bab86604..e3dcd8322 100644 --- a/services/auth/source/smtp/source_authenticate.go +++ b/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 { - return nil, errors.New("Unsupported SMTP auth type") + case CRAMMD5Authentication: + 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 } diff --git a/services/forms/auth_form.go b/services/forms/auth_form.go index b78fa9217..b45ea6ea1 100644 --- a/services/forms/auth_form.go +++ b/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 diff --git a/templates/admin/auth/edit.tmpl b/templates/admin/auth/edit.tmpl index 2b499c7c7..109186a17 100644 --- a/templates/admin/auth/edit.tmpl +++ b/templates/admin/auth/edit.tmpl @@ -44,6 +44,12 @@ +
+
+ + +
+
{{if .Source.IsLDAP}}
@@ -173,6 +179,30 @@
+
+
+ + +
+

{{.i18n.Tr "admin.auths.force_smtps_helper"}}

+
+
+
+ + +
+
+
+ + +

{{.i18n.Tr "admin.auths.helo_hostname_helper"}}

+
+
+
+ + +
+
@@ -308,26 +338,13 @@

{{.i18n.Tr "admin.auths.sspi_default_language_helper"}}

{{end}} - -
-
- - -
-
-
-
- - -
-
{{if .Source.IsLDAP}} -
-
- - +
+
+ + +
-
{{end}}
diff --git a/templates/admin/auth/new.tmpl b/templates/admin/auth/new.tmpl index 6addc50d0..ba1f145a4 100644 --- a/templates/admin/auth/new.tmpl +++ b/templates/admin/auth/new.tmpl @@ -54,18 +54,6 @@
-
-
- - -
-
-
-
- - -
-
diff --git a/templates/admin/auth/source/ldap.tmpl b/templates/admin/auth/source/ldap.tmpl index 0b7ad7a4d..295e001cf 100644 --- a/templates/admin/auth/source/ldap.tmpl +++ b/templates/admin/auth/source/ldap.tmpl @@ -20,6 +20,12 @@
+
+
+ + +
+
diff --git a/templates/admin/auth/source/smtp.tmpl b/templates/admin/auth/source/smtp.tmpl index 670c4b3b5..b0f643b8c 100644 --- a/templates/admin/auth/source/smtp.tmpl +++ b/templates/admin/auth/source/smtp.tmpl @@ -20,6 +20,30 @@
+
+
+ + +

{{.i18n.Tr "admin.auths.force_smtps_helper"}}

+
+
+
+
+ + +
+
+
+ + +

{{.i18n.Tr "admin.auths.helo_hostname_helper"}}

+
+
+
+ + +
+
diff --git a/templates/admin/user/edit.tmpl b/templates/admin/user/edit.tmpl index 5e5bc75c9..60cd8ad52 100644 --- a/templates/admin/user/edit.tmpl +++ b/templates/admin/user/edit.tmpl @@ -17,13 +17,13 @@
- +
{{.i18n.Tr "admin.users.local"}}
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
{{.i18n.Tr "admin.users.local"}}
{{range .Sources}} -
{{.Name}}
+
{{.Name}}
{{end}}
diff --git a/templates/admin/user/new.tmpl b/templates/admin/user/new.tmpl index a433c5a7c..d454d1cd9 100644 --- a/templates/admin/user/new.tmpl +++ b/templates/admin/user/new.tmpl @@ -19,7 +19,7 @@
{{.i18n.Tr "admin.users.local"}}
{{range .Sources}} -
{{.Name}}
+
{{.Name}}
{{end}}
diff --git a/web_src/js/index.js b/web_src/js/index.js index 17bf31d6a..b12180717 100644 --- a/web_src/js/index.js +++ b/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();