allow ACME HTTP-01 challenge with https redirect from port 80

Signed-off-by: Nicola Murino <nicola.murino@gmail.com>

docs/full-configuration.md

@@ -330,7 +330,7 @@ The configuration file contains the following sections:
       - `allowed_hosts`, list of strings. Fully qualified domain names that are allowed. An empty list allows any and all host names. Default: empty.
       - `allowed_hosts_are_regex`, boolean. Determines if the provided allowed hosts contains valid regular expressions. Default: `false`.
       - `hosts_proxy_headers`, list of string. Defines a set of header keys that may hold a proxied hostname value for the request, for example `X-Forwarded-Host`. Default: empty.
-      - `https_redirect`, boolean. Set to `true` to redirect HTTP requests to HTTPS. Default: `false`.
+      - `https_redirect`, boolean. Set to `true` to redirect HTTP requests to HTTPS. If you redirect from port `80` and you get your TLS certificates using the built-in ACME protocol and the `HTTP-01` challenge type, you need to use the webroot method and set the ACME web root to a path writable by SFTPGo in order to renew your certificates. Default: `false`.
       - `https_host`, string. Defines the host name that is used to redirect HTTP requests to HTTPS. Default is blank, which indicates to use the same host. For example, if `https_redirect` is enabled and `https_host` is blank, a request for `http://127.0.0.1/web/client/login` will be redirected to `https://127.0.0.1/web/client/login`, if `https_host` is set to `www.example.com` the same request will be redirected to `https://www.example.com/web/client/login`.
       - `https_proxy_headers`, list of struct, each struct contains the fields `key` and `value`. Defines a a list of header keys with associated values that would indicate a valid https request. For example `key` could be `X-Forwarded-Proto` and `value` `https`. Default: empty.
       - `sts_seconds`, integer. Defines the max-age of the `Strict-Transport-Security` header. This header will be included for `https` responses or for HTTP request if the request includes a defined HTTPS proxy header. Default: `0`, which would NOT include the header.

internal/acme/acme.go

@@ -48,7 +48,6 @@ import (
 	"github.com/drakkan/sftpgo/v2/internal/common"
 	"github.com/drakkan/sftpgo/v2/internal/dataprovider"
 	"github.com/drakkan/sftpgo/v2/internal/ftpd"
-	"github.com/drakkan/sftpgo/v2/internal/httpd"
 	"github.com/drakkan/sftpgo/v2/internal/logger"
 	"github.com/drakkan/sftpgo/v2/internal/telemetry"
 	"github.com/drakkan/sftpgo/v2/internal/util"
@@ -72,10 +71,12 @@ var (
 		string(certcrypto.RSA4096),
 		string(certcrypto.RSA8192),
 	}
+	fnReloadHTTPDCerts func() error
 )
 
-func init() {
-	httpd.SetCertificatesGetter(getCertificatesForConfig)
+// SetReloadHTTPDCertsFn set the function to call to reload HTTPD certificates
+func SetReloadHTTPDCertsFn(fn func() error) {
+	fnReloadHTTPDCerts = fn
 }
 
 // GetCertificates tries to obtain the certificates using the global configuration
@@ -86,9 +87,9 @@ func GetCertificates() error {
 	return config.getCertificates()
 }
 
-// getCertificatesForConfig tries to obtain the certificates using the provided
+// GetCertificatesForConfig tries to obtain the certificates using the provided
 // configuration override. This is a NOOP if we already have certificates
-func getCertificatesForConfig(c *dataprovider.ACMEConfigs, configDir string) error {
+func GetCertificatesForConfig(c *dataprovider.ACMEConfigs, configDir string) error {
 	if c.Domain == "" {
 		acmeLog(logger.LevelDebug, "no domain configured, nothing to do")
 		return nil
@@ -107,6 +108,11 @@ func getCertificatesForConfig(c *dataprovider.ACMEConfigs, configDir string) err
 	return config.getCertificates()
 }
 
+// GetHTTP01WebRoot returns the web root for HTTP-01 challenge
+func GetHTTP01WebRoot() string {
+	return initialConfig.HTTP01Challenge.WebRoot
+}
+
 func mergeConfig(config Configuration, c *dataprovider.ACMEConfigs) Configuration {
 	config.Domains = []string{c.Domain}
 	config.Email = c.Email
@@ -723,8 +729,10 @@ func (c *Configuration) renewCertificates() error {
 		// at least one certificate has been renewed, sends a reload to all services that may be using certificates
 		err = ftpd.ReloadCertificateMgr()
 		acmeLog(logger.LevelInfo, "ftpd certificate manager reloaded , error: %v", err)
-		err = httpd.ReloadCertificateMgr()
-		acmeLog(logger.LevelInfo, "httpd certificates manager reloaded , error: %v", err)
+		if fnReloadHTTPDCerts != nil {
+			err = fnReloadHTTPDCerts()
+			acmeLog(logger.LevelInfo, "httpd certificates manager reloaded , error: %v", err)
+		}
 		err = webdavd.ReloadCertificateMgr()
 		acmeLog(logger.LevelInfo, "webdav certificates manager reloaded , error: %v", err)
 		err = telemetry.ReloadCertificateMgr()

internal/httpd/httpd.go

@@ -35,6 +35,7 @@ import (
 	"github.com/go-chi/jwtauth/v5"
 	"github.com/lestrrat-go/jwx/v2/jwa"
 
+	"github.com/drakkan/sftpgo/v2/internal/acme"
 	"github.com/drakkan/sftpgo/v2/internal/common"
 	"github.com/drakkan/sftpgo/v2/internal/dataprovider"
 	"github.com/drakkan/sftpgo/v2/internal/ftpd"
@@ -184,6 +185,7 @@ const (
 	osWindows            = "windows"
 	otpHeaderCode        = "X-SFTPGO-OTP"
 	mTimeHeader          = "X-SFTPGO-MTIME"
+	acmeChallengeURI     = "/.well-known/acme-challenge/"
 )
 
 var (
@@ -277,21 +279,18 @@ var (
 	installationCodeHint       string
 	fnInstallationCodeResolver FnInstallationCodeResolver
 	configurationDir           string
-	fnGetCetificates           FnGetCertificates
 )
 
 func init() {
 	updateWebAdminURLs("")
 	updateWebClientURLs("")
+	acme.SetReloadHTTPDCertsFn(ReloadCertificateMgr)
 }
 
 // FnInstallationCodeResolver defines a method to get the installation code.
 // If the installation code cannot be resolved the provided default must be returned
 type FnInstallationCodeResolver func(defaultInstallationCode string) string
 
-// FnGetCertificates defines the method to call to get TLS certificates
-type FnGetCertificates func(*dataprovider.ACMEConfigs, string) error
-
 // HTTPSProxyHeader defines an HTTPS proxy header as key/value.
 // For example Key could be "X-Forwarded-Proto" and Value "https"
 type HTTPSProxyHeader struct {
@@ -358,6 +357,30 @@ func (s *SecurityConf) getHTTPSProxyHeaders() map[string]string {
 	return headers
 }
 
+func (s *SecurityConf) redirectHandler(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !isTLS(r) && !strings.HasPrefix(r.RequestURI, acmeChallengeURI) {
+			url := r.URL
+			url.Scheme = "https"
+			if s.HTTPSHost != "" {
+				url.Host = s.HTTPSHost
+			} else {
+				host := r.Host
+				for _, header := range s.HostsProxyHeaders {
+					if h := r.Header.Get(header); h != "" {
+						host = h
+						break
+					}
+				}
+				url.Host = host
+			}
+			http.Redirect(w, r, url.String(), http.StatusTemporaryRedirect)
+			return
+		}
+		next.ServeHTTP(w, r)
+	})
+}
+
 // UIBranding defines the supported customizations for the web UIs
 type UIBranding struct {
 	// Name defines the text to show at the login page and as HTML title
@@ -872,6 +895,9 @@ func (c *Conf) loadFromProvider() error {
 			return nil
 		}
 		for idx := range c.Bindings {
+			if c.Bindings[idx].Security.Enabled && c.Bindings[idx].Security.HTTPSRedirect {
+				continue
+			}
 			c.Bindings[idx].EnableHTTPS = true
 		}
 		c.acmeDomain = configs.ACME.Domain
@@ -1189,15 +1215,3 @@ func resolveInstallationCode() string {
 	}
 	return installationCode
 }
-
-// SetCertificatesGetter sets the function to call to get TLS certificates
-func SetCertificatesGetter(fn FnGetCertificates) {
-	fnGetCetificates = fn
-}
-
-func getTLSCertificates(c *dataprovider.ACMEConfigs) error {
-	if fnGetCetificates == nil {
-		return errors.New("unable to get TLS certificates, callback not defined")
-	}
-	return fnGetCetificates(c, configurationDir)
-}

internal/httpd/httpd_test.go

@@ -57,6 +57,7 @@ import (
 	"golang.org/x/crypto/ssh"
 	"golang.org/x/net/html"
 
+	"github.com/drakkan/sftpgo/v2/internal/acme"
 	"github.com/drakkan/sftpgo/v2/internal/common"
 	"github.com/drakkan/sftpgo/v2/internal/config"
 	"github.com/drakkan/sftpgo/v2/internal/dataprovider"
@@ -12259,7 +12260,11 @@ func TestMaxSessions(t *testing.T) {
 }
 
 func TestWebConfigsMock(t *testing.T) {
-	err := dataprovider.UpdateConfigs(nil, "", "", "")
+	acmeConfig := config.GetACMEConfig()
+	acmeConfig.CertsPath = filepath.Clean(os.TempDir())
+	err := acme.Initialize(acmeConfig, configDir, true)
+	require.NoError(t, err)
+	err = dataprovider.UpdateConfigs(nil, "", "", "")
 	assert.NoError(t, err)
 	webToken, err := getJWTWebTokenFromTestServer(defaultTokenAuthUser, defaultTokenAuthPass)
 	assert.NoError(t, err)
@@ -12399,8 +12404,6 @@ func TestWebConfigsMock(t *testing.T) {
 	rr = executeRequest(req)
 	checkResponseCode(t, http.StatusOK, rr)
 	assert.Contains(t, rr.Body.String(), "Configurations updated")
-	// test ACME configs, set a fake callback to avoid Let's encrypt calls
-	httpd.SetCertificatesGetter(func(a *dataprovider.ACMEConfigs, s string) error { return nil })
 	form.Set("form_action", "acme_submit")
 	form.Set("acme_port", "") // on error will be set to 80
 	form.Set("acme_protocols", "1")
@@ -12414,7 +12417,7 @@ func TestWebConfigsMock(t *testing.T) {
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	rr = executeRequest(req)
 	checkResponseCode(t, http.StatusOK, rr)
-	assert.Contains(t, rr.Body.String(), "Validation error")
+	assert.Contains(t, rr.Body.String(), "invalid email address")
 	form.Set("acme_domain", "")
 	req, err = http.NewRequest(http.MethodPost, webConfigsPath, bytes.NewBuffer([]byte(form.Encode())))
 	assert.NoError(t, err)
@@ -12437,11 +12440,18 @@ func TestWebConfigsMock(t *testing.T) {
 	assert.True(t, configs.ACME.HasProtocol(common.ProtocolFTP))
 	assert.True(t, configs.ACME.HasProtocol(common.ProtocolWebDAV))
 	assert.True(t, configs.ACME.HasProtocol(common.ProtocolHTTP))
-
+	// create certificate files, so no real ACME call is done
+	domain := "acme.example.com"
+	crtPath := filepath.Join(acmeConfig.CertsPath, util.SanitizeDomain(domain)+".crt")
+	keyPath := filepath.Join(acmeConfig.CertsPath, util.SanitizeDomain(domain)+".key")
+	err = os.WriteFile(crtPath, nil, 0666)
+	assert.NoError(t, err)
+	err = os.WriteFile(keyPath, nil, 0666)
+	assert.NoError(t, err)
 	form.Set("acme_port", "402")
 	form.Set("acme_protocols", "1")
 	form.Add("acme_protocols", "1000")
-	form.Set("acme_domain", "acme.example.com")
+	form.Set("acme_domain", domain)
 	form.Set("acme_email", "email@example.com")
 	req, err = http.NewRequest(http.MethodPost, webConfigsPath, bytes.NewBuffer([]byte(form.Encode())))
 	assert.NoError(t, err)
@@ -12455,21 +12465,16 @@ func TestWebConfigsMock(t *testing.T) {
 	assert.Len(t, configs.SFTPD.HostKeyAlgos, 2)
 	assert.Equal(t, 402, configs.ACME.HTTP01Challenge.Port)
 	assert.Equal(t, 1, configs.ACME.Protocols)
-	assert.Equal(t, "acme.example.com", configs.ACME.Domain)
+	assert.Equal(t, domain, configs.ACME.Domain)
 	assert.Equal(t, "email@example.com", configs.ACME.Email)
 	assert.False(t, configs.ACME.HasProtocol(common.ProtocolFTP))
 	assert.False(t, configs.ACME.HasProtocol(common.ProtocolWebDAV))
 	assert.True(t, configs.ACME.HasProtocol(common.ProtocolHTTP))
-	// updates will fail, the get certificate fn will return error with nil callback
-	httpd.SetCertificatesGetter(nil)
-	req, err = http.NewRequest(http.MethodPost, webConfigsPath, bytes.NewBuffer([]byte(form.Encode())))
-	assert.NoError(t, err)
-	setJWTCookieForReq(req, webToken)
-	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
-	rr = executeRequest(req)
-	checkResponseCode(t, http.StatusOK, rr)
-	assert.Contains(t, rr.Body.String(), "unable to get TLS certificates")
 
+	err = os.Remove(crtPath)
+	assert.NoError(t, err)
+	err = os.Remove(keyPath)
+	assert.NoError(t, err)
 	err = dataprovider.UpdateConfigs(nil, "", "", "")
 	assert.NoError(t, err)
 }

internal/httpd/internal_test.go

@@ -47,6 +47,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/drakkan/sftpgo/v2/internal/acme"
 	"github.com/drakkan/sftpgo/v2/internal/common"
 	"github.com/drakkan/sftpgo/v2/internal/dataprovider"
 	"github.com/drakkan/sftpgo/v2/internal/kms"
@@ -3063,6 +3064,13 @@ func TestConfigsFromProvider(t *testing.T) {
 			{
 				Port: 1234,
 			},
+			{
+				Port: 80,
+				Security: SecurityConf{
+					Enabled:       true,
+					HTTPSRedirect: true,
+				},
+			},
 		},
 	}
 	err = c.loadFromProvider()
@@ -3109,6 +3117,7 @@ func TestConfigsFromProvider(t *testing.T) {
 	keyPairs = c.getKeyPairs(configDir)
 	assert.Len(t, keyPairs, 1)
 	assert.True(t, c.Bindings[0].EnableHTTPS)
+	assert.False(t, c.Bindings[1].EnableHTTPS)
 	// protocols does not match
 	configs.ACME.Protocols = 6
 	err = dataprovider.UpdateConfigs(&configs, "", "", "")
@@ -3129,6 +3138,69 @@ func TestConfigsFromProvider(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestHTTPSRedirect(t *testing.T) {
+	acmeWebRoot := filepath.Join(os.TempDir(), "acme")
+	err := os.MkdirAll(acmeWebRoot, os.ModePerm)
+	assert.NoError(t, err)
+	tokenName := "token"
+	err = os.WriteFile(filepath.Join(acmeWebRoot, tokenName), []byte("val"), 0666)
+	assert.NoError(t, err)
+
+	acmeConfig := acme.Configuration{
+		HTTP01Challenge: acme.HTTP01Challenge{WebRoot: acmeWebRoot},
+	}
+	err = acme.Initialize(acmeConfig, configDir, true)
+	require.NoError(t, err)
+
+	forwardedHostHeader := "X-Forwarded-Host"
+	server := httpdServer{
+		binding: Binding{
+			Security: SecurityConf{
+				Enabled:           true,
+				HTTPSRedirect:     true,
+				HostsProxyHeaders: []string{forwardedHostHeader},
+			},
+		},
+	}
+	server.initializeRouter()
+
+	rr := httptest.NewRecorder()
+	r, err := http.NewRequest(http.MethodGet, path.Join(acmeChallengeURI, tokenName), nil)
+	assert.NoError(t, err)
+	r.Host = "localhost"
+	r.RequestURI = path.Join(acmeChallengeURI, tokenName)
+	server.router.ServeHTTP(rr, r)
+	assert.Equal(t, http.StatusOK, rr.Code, rr.Body.String())
+
+	rr = httptest.NewRecorder()
+	r, err = http.NewRequest(http.MethodGet, webAdminLoginPath, nil)
+	assert.NoError(t, err)
+	r.RequestURI = webAdminLoginPath
+	server.router.ServeHTTP(rr, r)
+	assert.Equal(t, http.StatusTemporaryRedirect, rr.Code, rr.Body.String())
+
+	rr = httptest.NewRecorder()
+	r, err = http.NewRequest(http.MethodGet, webAdminLoginPath, nil)
+	assert.NoError(t, err)
+	r.RequestURI = webAdminLoginPath
+	r.Header.Set(forwardedHostHeader, "sftpgo.com")
+	server.router.ServeHTTP(rr, r)
+	assert.Equal(t, http.StatusTemporaryRedirect, rr.Code, rr.Body.String())
+	assert.Contains(t, rr.Body.String(), "https://sftpgo.com")
+
+	server.binding.Security.HTTPSHost = "myhost:1044"
+	rr = httptest.NewRecorder()
+	r, err = http.NewRequest(http.MethodGet, webAdminLoginPath, nil)
+	assert.NoError(t, err)
+	r.RequestURI = webAdminLoginPath
+	server.router.ServeHTTP(rr, r)
+	assert.Equal(t, http.StatusTemporaryRedirect, rr.Code, rr.Body.String())
+	assert.Contains(t, rr.Body.String(), "https://myhost:1044")
+
+	err = os.RemoveAll(acmeWebRoot)
+	assert.NoError(t, err)
+}
+
 func isSharedProviderSupported() bool {
 	// SQLite shares the implementation with other SQL-based provider but it makes no sense
 	// to use it outside test cases

internal/httpd/server.go

@@ -37,6 +37,7 @@ import (
 	"github.com/sftpgo/sdk"
 	"github.com/unrolled/secure"
 
+	"github.com/drakkan/sftpgo/v2/internal/acme"
 	"github.com/drakkan/sftpgo/v2/internal/common"
 	"github.com/drakkan/sftpgo/v2/internal/dataprovider"
 	"github.com/drakkan/sftpgo/v2/internal/logger"
@@ -1095,7 +1096,21 @@ func (s *httpdServer) badHostHandler(w http.ResponseWriter, r *http.Request) {
 			break
 		}
 	}
-	s.sendForbiddenResponse(w, r, fmt.Sprintf("The host %#v is not allowed", host))
+	s.sendForbiddenResponse(w, r, fmt.Sprintf("The host %q is not allowed", host))
+}
+
+func (s *httpdServer) notFoundHandler(w http.ResponseWriter, r *http.Request) {
+	r.Body = http.MaxBytesReader(w, r.Body, maxRequestSize)
+	if (s.enableWebAdmin || s.enableWebClient) && isWebRequest(r) {
+		r = s.updateContextFromCookie(r)
+		if s.enableWebClient && (isWebClientRequest(r) || !s.enableWebAdmin) {
+			s.renderClientNotFoundPage(w, r, nil)
+			return
+		}
+		s.renderNotFoundPage(w, r, nil)
+		return
+	}
+	sendAPIResponse(w, r, nil, http.StatusText(http.StatusNotFound), http.StatusNotFound)
 }
 
 func (s *httpdServer) redirectToWebPath(w http.ResponseWriter, r *http.Request, webPath string) {
@@ -1120,6 +1135,7 @@ func (s *httpdServer) isStaticFileURL(r *http.Request) bool {
 }
 
 func (s *httpdServer) initializeRouter() {
+	var hasHTTPSRedirect bool
 	s.tokenAuth = jwtauth.New(jwa.HS256.String(), getSigningKey(s.signingPassphrase), nil)
 	s.router = chi.NewRouter()
 
@@ -1132,9 +1148,6 @@ func (s *httpdServer) initializeRouter() {
 			AllowedHosts:            s.binding.Security.AllowedHosts,
 			AllowedHostsAreRegex:    s.binding.Security.AllowedHostsAreRegex,
 			HostsProxyHeaders:       s.binding.Security.HostsProxyHeaders,
-			SSLRedirect:             s.binding.Security.HTTPSRedirect,
-			SSLHost:                 s.binding.Security.HTTPSHost,
-			SSLTemporaryRedirect:    true,
 			SSLProxyHeaders:         s.binding.Security.getHTTPSProxyHeaders(),
 			STSSeconds:              s.binding.Security.STSSeconds,
 			STSIncludeSubdomains:    s.binding.Security.STSIncludeSubdomains,
@@ -1147,6 +1160,10 @@ func (s *httpdServer) initializeRouter() {
 		})
 		secureMiddleware.SetBadHostHandler(http.HandlerFunc(s.badHostHandler))
 		s.router.Use(secureMiddleware.Handler)
+		if s.binding.Security.HTTPSRedirect {
+			s.router.Use(s.binding.Security.redirectHandler)
+			hasHTTPSRedirect = true
+		}
 	}
 	if s.cors.Enabled {
 		c := cors.New(cors.Options{
@@ -1166,19 +1183,7 @@ func (s *httpdServer) initializeRouter() {
 	// StripSlashes causes infinite redirects at the root path if used with http.FileServer
 	s.router.Use(middleware.Maybe(middleware.StripSlashes, s.isStaticFileURL))
 
-	s.router.NotFound(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		r.Body = http.MaxBytesReader(w, r.Body, maxRequestSize)
-		if (s.enableWebAdmin || s.enableWebClient) && isWebRequest(r) {
-			r = s.updateContextFromCookie(r)
-			if s.enableWebClient && (isWebClientRequest(r) || !s.enableWebAdmin) {
-				s.renderClientNotFoundPage(w, r, nil)
-				return
-			}
-			s.renderNotFoundPage(w, r, nil)
-			return
-		}
-		sendAPIResponse(w, r, nil, http.StatusText(http.StatusNotFound), http.StatusNotFound)
-	}))
+	s.router.NotFound(s.notFoundHandler)
 
 	s.router.Get(healthzPath, func(w http.ResponseWriter, r *http.Request) {
 		render.PlainText(w, r, "ok")
@@ -1188,6 +1193,12 @@ func (s *httpdServer) initializeRouter() {
 		render.PlainText(w, r, "User-agent: *\nDisallow: /")
 	})
 
+	if hasHTTPSRedirect {
+		if p := acme.GetHTTP01WebRoot(); p != "" {
+			serveStaticDir(s.router, acmeChallengeURI, p)
+		}
+	}
+
 	if s.enableRESTAPI {
 		// share API available to external users
 		s.router.Get(sharesPath+"/{id}", s.downloadFromShare)

internal/httpd/webadmin.go

@@ -32,6 +32,7 @@ import (
 	"github.com/sftpgo/sdk"
 	sdkkms "github.com/sftpgo/sdk/kms"
 
+	"github.com/drakkan/sftpgo/v2/internal/acme"
 	"github.com/drakkan/sftpgo/v2/internal/common"
 	"github.com/drakkan/sftpgo/v2/internal/dataprovider"
 	"github.com/drakkan/sftpgo/v2/internal/kms"
@@ -4074,7 +4075,7 @@ func (s *httpdServer) handleWebConfigsPost(w http.ResponseWriter, r *http.Reques
 		configSection = 2
 		acmeConfigs := getACMEConfigsFromPostFields(r)
 		configs.ACME = acmeConfigs
-		if err := getTLSCertificates(acmeConfigs); err != nil {
+		if err := acme.GetCertificatesForConfig(acmeConfigs, configurationDir); err != nil {
 			s.renderConfigsPage(w, r, configs, err.Error(), configSection)
 			return
 		}