From 9a1074021843b2a3e94d33c00f437bbafd035094 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Fri, 24 Feb 2023 20:08:14 +0100 Subject: [PATCH] allow ACME HTTP-01 challenge with https redirect from port 80 Signed-off-by: Nicola Murino --- docs/full-configuration.md | 2 +- internal/acme/acme.go | 22 ++++++---- internal/httpd/httpd.go | 46 +++++++++++++-------- internal/httpd/httpd_test.go | 37 +++++++++-------- internal/httpd/internal_test.go | 72 +++++++++++++++++++++++++++++++++ internal/httpd/server.go | 45 +++++++++++++-------- internal/httpd/webadmin.go | 3 +- 7 files changed, 169 insertions(+), 58 deletions(-) diff --git a/docs/full-configuration.md b/docs/full-configuration.md index 600aacce..aefc0520 100644 --- a/docs/full-configuration.md +++ b/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. diff --git a/internal/acme/acme.go b/internal/acme/acme.go index c25906de..dfd3181e 100644 --- a/internal/acme/acme.go +++ b/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() diff --git a/internal/httpd/httpd.go b/internal/httpd/httpd.go index 0881f01d..ee2dda2f 100644 --- a/internal/httpd/httpd.go +++ b/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) -} diff --git a/internal/httpd/httpd_test.go b/internal/httpd/httpd_test.go index 2e8911ea..629a0bb4 100644 --- a/internal/httpd/httpd_test.go +++ b/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) } diff --git a/internal/httpd/internal_test.go b/internal/httpd/internal_test.go index 0988f0d5..f9015f57 100644 --- a/internal/httpd/internal_test.go +++ b/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 diff --git a/internal/httpd/server.go b/internal/httpd/server.go index 0a2b0e91..80546848 100644 --- a/internal/httpd/server.go +++ b/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) diff --git a/internal/httpd/webadmin.go b/internal/httpd/webadmin.go index 34e86cd7..af734533 100644 --- a/internal/httpd/webadmin.go +++ b/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 }