From efe2591f83fdc00b81f4b70d409ede2b2f069def Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Tue, 2 Apr 2019 14:32:36 +0300 Subject: [PATCH 01/18] Add Acme middleware --- app.go | 18 +++++++++++---- internal/acme/acme.go | 52 +++++++++++++++++++++++++++++++++++++++++++ main.go | 49 ++++++++++++++++++++++++++++------------ 3 files changed, 101 insertions(+), 18 deletions(-) create mode 100644 internal/acme/acme.go diff --git a/app.go b/app.go index 08eff7e72..b740b04b6 100644 --- a/app.go +++ b/app.go @@ -17,6 +17,7 @@ import ( log "github.com/sirupsen/logrus" mimedb "gitlab.com/lupine/go-mimedb" + "gitlab.com/gitlab-org/gitlab-pages/internal/acme" "gitlab.com/gitlab-org/gitlab-pages/internal/admin" "gitlab.com/gitlab-org/gitlab-pages/internal/artifact" "gitlab.com/gitlab-org/gitlab-pages/internal/auth" @@ -38,10 +39,11 @@ var ( type theApp struct { appConfig - dm domain.Map - lock sync.RWMutex - Artifact *artifact.Artifact - Auth *auth.Auth + dm domain.Map + lock sync.RWMutex + Artifact *artifact.Artifact + Auth *auth.Auth + AcmeMiddleware *acme.Middleware } func (a *theApp) isReady() bool { @@ -166,6 +168,10 @@ func (a *theApp) serveContent(ww http.ResponseWriter, r *http.Request, https boo host, domain := a.getHostAndDomain(r) + if a.AcmeMiddleware.ServeAcmeChallenges(&w, r) { + return + } + if a.Auth.TryAuthenticate(&w, r, a.dm, &a.lock) { return } @@ -360,6 +366,10 @@ func runApp(config appConfig) { config.RedirectURI, config.GitLabServer) } + if config.GitLabServer != "" { + a.AcmeMiddleware = &acme.Middleware{GitlabURL: config.GitLabServer} + } + configureLogging(config.LogFormat, config.LogVerbose) if err := mimedb.LoadTypes(); err != nil { diff --git a/internal/acme/acme.go b/internal/acme/acme.go new file mode 100644 index 000000000..b924c43e8 --- /dev/null +++ b/internal/acme/acme.go @@ -0,0 +1,52 @@ +package acme + +import ( + "net" + "net/http" + "path/filepath" + "strings" + + log "github.com/sirupsen/logrus" +) + +// Middleware handles acme challenges by redirecting them to GitLab instance +type Middleware struct { + GitlabURL string +} + +// ServeAcmeChallenges identifies if request is acme-challenge and redirects to GitLab in that case +func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request) bool { + if m == nil { + return false + } + + if isAcmeChallenge(r.URL.Path) { + m.redirectToGitlab(w, r) + return true + } + + return false +} + +func isAcmeChallenge(path string) bool { + return strings.HasPrefix(path, "/.well-known/acme-challenge/") +} + +func (m *Middleware) redirectToGitlab(w http.ResponseWriter, r *http.Request) { + redirectURL := m.GitlabURL + "/-/acme-challenge/" + getHost(r) + + "/" + filepath.Base(r.URL.Path) + + log.WithField("redirect_url", redirectURL).Info("Redirecting to GitLab for processing acme challenge") + + http.Redirect(w, r, redirectURL, 302) +} + +func getHost(r *http.Request) string { + host := strings.ToLower(r.Host) + + if splitHost, _, err := net.SplitHostPort(host); err == nil { + host = splitHost + } + + return host +} diff --git a/main.go b/main.go index 185fd4b0e..fbfba95d8 100644 --- a/main.go +++ b/main.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io" + "net" "net/url" "os" "strings" @@ -48,7 +49,8 @@ var ( adminHTTPSCert = flag.String("admin-https-cert", "", "The path to the certificate file for the admin API (optional)") adminHTTPSKey = flag.String("admin-https-key", "", "The path to the key file for the admin API (optional)") secret = flag.String("auth-secret", "", "Cookie store hash key, should be at least 32 bytes long.") - gitLabServer = flag.String("auth-server", "", "GitLab server, for example https://www.gitlab.com") + gitLabAuthServer = flag.String("auth-server", "", "DEPRECATED, use gitlab-server insteadGitLab server, for example https://www.gitlab.com") + gitLabServer = flag.String("gitlab-server", "", "GitLab server, for example https://www.gitlab.com") clientID = flag.String("auth-client-id", "", "GitLab application Client ID") clientSecret = flag.String("auth-client-secret", "", "GitLab application Client Secret") redirectURI = flag.String("auth-redirect-uri", "", "GitLab application redirect URI") @@ -72,10 +74,28 @@ var ( errSecretNotDefined = errors.New("auth-secret must be defined if authentication is supported") errClientIDNotDefined = errors.New("auth-client-id must be defined if authentication is supported") errClientSecretNotDefined = errors.New("auth-client-secret must be defined if authentication is supported") - errGitLabServerNotDefined = errors.New("auth-server must be defined if authentication is supported") + errGitLabServerNotDefined = errors.New("gitlab-server must be defined if authentication is supported") errRedirectURINotDefined = errors.New("auth-redirect-uri must be defined if authentication is supported") ) +func gitlabServerFromFlags() string { + if *gitLabServer != "" { + return *gitLabServer + } + + if *gitLabAuthServer != "" { + log.Warn("auth-server parameter is deprecated, use gitlab-server instead") + return *gitLabAuthServer + } + + url, err := url.Parse(*artifactsServer) + if err != nil { + return "" + } + host, _, _ := net.SplitHostPort(url.Host) + return host +} + func configFromFlags() appConfig { var config appConfig @@ -129,39 +149,40 @@ func configFromFlags() appConfig { config.ArtifactsServer = *artifactsServer } - checkAuthenticationConfig(config) + config.GitLabServer = gitlabServerFromFlags() config.StoreSecret = *secret config.ClientID = *clientID config.ClientSecret = *clientSecret - config.GitLabServer = *gitLabServer config.RedirectURI = *redirectURI + checkAuthenticationConfig(config) + return config } func checkAuthenticationConfig(config appConfig) { - if *secret != "" || *clientID != "" || *clientSecret != "" || - *gitLabServer != "" || *redirectURI != "" { - // Check all auth params are valid - assertAuthConfig() + if config.StoreSecret == "" && config.ClientID == "" && + config.ClientSecret == "" && config.RedirectURI == "" { + return } + assertAuthConfig(config) } -func assertAuthConfig() { - if *secret == "" { +func assertAuthConfig(config appConfig) { + if config.StoreSecret == "" { log.Fatal(errSecretNotDefined) } - if *clientID == "" { + if config.ClientID == "" { log.Fatal(errClientIDNotDefined) } - if *clientSecret == "" { + if config.ClientSecret == "" { log.Fatal(errClientSecretNotDefined) } - if *gitLabServer == "" { + if config.GitLabServer == "" { log.Fatal(errGitLabServerNotDefined) } - if *redirectURI == "" { + if config.RedirectURI == "" { log.Fatal(errRedirectURINotDefined) } } -- GitLab From 93cd41bd220c128f0a117ba2d29ad7dc0a68c8eb Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Wed, 3 Apr 2019 20:09:56 +0300 Subject: [PATCH 02/18] Only redirect acme challenges if acme_ssl_enabled --- app.go | 2 +- internal/acme/acme.go | 8 +++++++- internal/domain/domain.go | 15 +++++++++++++++ internal/domain/domain_config.go | 13 +++++++------ 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/app.go b/app.go index b740b04b6..9d4f5bdbe 100644 --- a/app.go +++ b/app.go @@ -168,7 +168,7 @@ func (a *theApp) serveContent(ww http.ResponseWriter, r *http.Request, https boo host, domain := a.getHostAndDomain(r) - if a.AcmeMiddleware.ServeAcmeChallenges(&w, r) { + if a.AcmeMiddleware.ServeAcmeChallenges(&w, r, domain) { return } diff --git a/internal/acme/acme.go b/internal/acme/acme.go index b924c43e8..d923f4488 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -7,6 +7,8 @@ import ( "strings" log "github.com/sirupsen/logrus" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" ) // Middleware handles acme challenges by redirecting them to GitLab instance @@ -15,11 +17,15 @@ type Middleware struct { } // ServeAcmeChallenges identifies if request is acme-challenge and redirects to GitLab in that case -func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request) bool { +func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, domain *domain.D) bool { if m == nil { return false } + if !domain.IsAcmeSslEnabled() { + return false + } + if isAcmeChallenge(r.URL.Path) { m.redirectToGitlab(w, r) return true diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 1455d4368..5409b5221 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -179,6 +179,21 @@ func (d *D) IsAccessControlEnabled(r *http.Request) bool { return false } +// IsAccessControlEnabled figures out if the request is to a project that has access control enabled +func (d *D) IsAcmeSslEnabled() bool { + if d == nil { + return false + } + + // Check custom domain config (e.g. http://example.com) + if d.config != nil { + return d.config.AcmeSslEnabled + } + + return false +} + + // IsNamespaceProject figures out if the request is to a namespace project func (d *D) IsNamespaceProject(r *http.Request) bool { if d == nil { diff --git a/internal/domain/domain_config.go b/internal/domain/domain_config.go index 2ab2ce6cb..421984a96 100644 --- a/internal/domain/domain_config.go +++ b/internal/domain/domain_config.go @@ -8,12 +8,13 @@ import ( ) type domainConfig struct { - Domain string - Certificate string - Key string - HTTPSOnly bool `json:"https_only"` - ID uint64 `json:"id"` - AccessControl bool `json:"access_control"` + Domain string + Certificate string + Key string + HTTPSOnly bool `json:"https_only"` + ID uint64 `json:"id"` + AccessControl bool `json:"access_control"` + AcmeSslEnabled bool `json:"acme_ssl_enabled"` } type domainsConfig struct { -- GitLab From 38901cc10f20af0a00cb6f23e2aca4430af25915 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Thu, 4 Apr 2019 14:35:29 +0300 Subject: [PATCH 03/18] Add tests for acme middleware --- internal/acme/acme.go | 9 +++-- internal/acme/acme_test.go | 60 +++++++++++++++++++++++++++++ internal/domain/domain.go | 3 +- internal/domain/domain_test.go | 41 ++++++-------------- internal/testhelpers/testhelpers.go | 34 ++++++++++++++++ 5 files changed, 113 insertions(+), 34 deletions(-) create mode 100644 internal/acme/acme_test.go create mode 100644 internal/testhelpers/testhelpers.go diff --git a/internal/acme/acme.go b/internal/acme/acme.go index d923f4488..9a6c53942 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -7,8 +7,6 @@ import ( "strings" log "github.com/sirupsen/logrus" - - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" ) // Middleware handles acme challenges by redirecting them to GitLab instance @@ -16,8 +14,13 @@ type Middleware struct { GitlabURL string } +// Domain interface tells us if we should perform redirects fot this domain +type Domain interface { + IsAcmeSslEnabled() bool +} + // ServeAcmeChallenges identifies if request is acme-challenge and redirects to GitLab in that case -func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, domain *domain.D) bool { +func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, domain Domain) bool { if m == nil { return false } diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go new file mode 100644 index 000000000..67c7831fe --- /dev/null +++ b/internal/acme/acme_test.go @@ -0,0 +1,60 @@ +package acme + +import ( + "net/http" + _ "net/url" + "testing" + + _ "github.com/stretchr/testify/assert" + + "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" +) + +type domainStub struct { + acmeSslEnabled bool +} + +func (d *domainStub) IsAcmeSslEnabled() bool { + return d.acmeSslEnabled +} + +func serveAcmeOrNotFound(m *Middleware, domain Domain) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if !m.ServeAcmeChallenges(w, r, domain) { + http.NotFound(w, r) + } + } +} + +func TestServeAcmeChallenges(t *testing.T) { + domain := &domainStub{acmeSslEnabled: true} + disabledDomain := &domainStub{acmeSslEnabled: false} + + baseURL := "http://example.com" + indexURL := baseURL + "/index.html" + challengeURL := baseURL + "/.well-known/acme-challenge/token" + + middleware := &Middleware{GitlabURL: "https://gitlab.example.com"} + + t.Run("When acme middleware is not configured", func(t *testing.T) { + testhelpers.TestHTTP404(t, serveAcmeOrNotFound(nil, domain), "GET", challengeURL, nil, nil) + }) + + t.Run("When acme ssl is disabled for domain", func(t *testing.T) { + testhelpers.TestHTTP404(t, serveAcmeOrNotFound(middleware, disabledDomain), + "GET", challengeURL, nil, nil) + }) + + t.Run("When acme ssl is enabled for domain", func(t *testing.T) { + t.Run("When acme challenge is being requested", func(t *testing.T) { + testhelpers.AssertRedirectTo(t, serveAcmeOrNotFound(middleware, domain), + "GET", challengeURL, nil, + "https://gitlab.example.com/-/acme-challenge/example.com/token") + }) + + t.Run("When pages content is being requested", func(t *testing.T) { + testhelpers.TestHTTP404(t, serveAcmeOrNotFound(middleware, domain), + "GET", indexURL, nil, nil) + }) + }) +} diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 5409b5221..32412be7e 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -179,7 +179,7 @@ func (d *D) IsAccessControlEnabled(r *http.Request) bool { return false } -// IsAccessControlEnabled figures out if the request is to a project that has access control enabled +// IsAcmeSslEnabled checks if Let's Encrypt ssl is enabled for domain func (d *D) IsAcmeSslEnabled() bool { if d == nil { return false @@ -193,7 +193,6 @@ func (d *D) IsAcmeSslEnabled() bool { return false } - // IsNamespaceProject figures out if the request is to a namespace project func (d *D) IsNamespaceProject(r *http.Request) bool { if d == nil { diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index add9b616a..eb44c3421 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -3,7 +3,6 @@ package domain import ( "compress/gzip" "io/ioutil" - "mime" "net/http" "net/http/httptest" "net/url" @@ -15,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/fixture" + "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) func serveFileOrNotFound(domain *D) http.HandlerFunc { @@ -25,11 +25,6 @@ func serveFileOrNotFound(domain *D) http.HandlerFunc { } } -func assertRedirectTo(t *testing.T, h http.HandlerFunc, method string, url string, values url.Values, expectedURL string) { - assert.HTTPRedirect(t, h, method, url, values) - assert.HTTPBodyContains(t, h, method, url, values, `Found`) -} - func testGroupServeHTTPHost(t *testing.T, host string) { testGroup := &D{ projectName: "", @@ -53,12 +48,12 @@ func testGroupServeHTTPHost(t *testing.T, host string) { assert.HTTPBodyContains(t, serve, "GET", makeURL("/"), nil, "main-dir") assert.HTTPBodyContains(t, serve, "GET", makeURL("/index"), nil, "main-dir") assert.HTTPBodyContains(t, serve, "GET", makeURL("/index.html"), nil, "main-dir") - assertRedirectTo(t, serve, "GET", makeURL("/project"), nil, host+"/project/") + testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project"), nil, "//"+host+"/project/") assert.HTTPBodyContains(t, serve, "GET", makeURL("/project/"), nil, "project-subdir") assert.HTTPBodyContains(t, serve, "GET", makeURL("/project/index"), nil, "project-subdir") assert.HTTPBodyContains(t, serve, "GET", makeURL("/project/index/"), nil, "project-subdir") assert.HTTPBodyContains(t, serve, "GET", makeURL("/project/index.html"), nil, "project-subdir") - assertRedirectTo(t, serve, "GET", makeURL("/project/subdir"), nil, host+"/project/subdir/") + testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project/subdir"), nil, "//"+host+"/project/subdir/") assert.HTTPBodyContains(t, serve, "GET", makeURL("/project/subdir/"), nil, "project-subsubdir") assert.HTTPBodyContains(t, serve, "GET", makeURL("/project2/"), nil, "project2-main") assert.HTTPBodyContains(t, serve, "GET", makeURL("/project2/index"), nil, "project2-main") @@ -304,18 +299,6 @@ func TestGroupServeHTTPGzip(t *testing.T) { } } -func testHTTP404(t *testing.T, handler http.HandlerFunc, mode, url string, values url.Values, str interface{}) { - w := httptest.NewRecorder() - req, err := http.NewRequest(mode, url+"?"+values.Encode(), nil) - require.NoError(t, err) - handler(w, req) - - contentType, _, _ := mime.ParseMediaType(w.Header().Get("Content-Type")) - assert.Equal(t, http.StatusNotFound, w.Code, "HTTP status") - assert.Equal(t, "text/html", contentType, "Content-Type") - assert.Contains(t, w.Body.String(), str) -} - func TestGroup404ServeHTTP(t *testing.T) { cleanup := setUpTests(t) defer cleanup() @@ -334,15 +317,15 @@ func TestGroup404ServeHTTP(t *testing.T) { }, } - testHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/not/existing-file", nil, "Custom 404 project page") - testHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/", nil, "Custom 404 project page") - testHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not/existing-file", nil, "Custom 404 group page") - testHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") - testHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/not/existing-file", nil, "Custom 404 project page") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/", nil, "Custom 404 project page") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not/existing-file", nil, "Custom 404 group page") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") assert.HTTPBodyNotContains(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404.symlink/not/existing-file", nil, "Custom 404 project page") // Ensure the namespace project's custom 404.html is not used by projects - testHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.no.404/not/existing-file", nil, "The page you're looking for could not be found.") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.no.404/not/existing-file", nil, "The page you're looking for could not be found.") } func TestDomain404ServeHTTP(t *testing.T) { @@ -357,8 +340,8 @@ func TestDomain404ServeHTTP(t *testing.T) { }, } - testHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") - testHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") } func TestPredefined404ServeHTTP(t *testing.T) { @@ -369,7 +352,7 @@ func TestPredefined404ServeHTTP(t *testing.T) { group: group{name: "group"}, } - testHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found") + testhelpers.TestHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found") } func TestGroupCertificate(t *testing.T) { diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go new file mode 100644 index 000000000..5222bb818 --- /dev/null +++ b/internal/testhelpers/testhelpers.go @@ -0,0 +1,34 @@ +package testhelpers + +import ( + "mime" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHTTP404 asserts handler returns 404 with provided str body +func TestHTTP404(t *testing.T, handler http.HandlerFunc, mode, url string, values url.Values, str interface{}) { + w := httptest.NewRecorder() + req, err := http.NewRequest(mode, url+"?"+values.Encode(), nil) + require.NoError(t, err) + handler(w, req) + + assert.Equal(t, http.StatusNotFound, w.Code, "HTTP status") + + if str != nil { + contentType, _, _ := mime.ParseMediaType(w.Header().Get("Content-Type")) + assert.Equal(t, "text/html", contentType, "Content-Type") + assert.Contains(t, w.Body.String(), str) + } +} + +// AssertRedirectTo asserts that handler redirects to particular URL +func AssertRedirectTo(t *testing.T, h http.HandlerFunc, method string, url string, values url.Values, expectedURL string) { + assert.HTTPRedirect(t, h, method, url, values) + assert.HTTPBodyContains(t, h, method, url, values, `Found`) +} -- GitLab From 81d530908ceb2c72c637914106a4b47b4c331303 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 5 Apr 2019 13:37:41 +0300 Subject: [PATCH 04/18] Check acme challenge existance instead of config --- internal/acme/acme.go | 6 +++--- internal/acme/acme_test.go | 16 ++++++++-------- internal/domain/domain.go | 16 +++++++++++----- internal/domain/domain_config.go | 13 ++++++------- 4 files changed, 28 insertions(+), 23 deletions(-) diff --git a/internal/acme/acme.go b/internal/acme/acme.go index 9a6c53942..896bf2416 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -14,9 +14,9 @@ type Middleware struct { GitlabURL string } -// Domain interface tells us if we should perform redirects fot this domain +// Domain interface represent D from domain package type Domain interface { - IsAcmeSslEnabled() bool + HasAcmeChallenge(string) bool } // ServeAcmeChallenges identifies if request is acme-challenge and redirects to GitLab in that case @@ -25,7 +25,7 @@ func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, return false } - if !domain.IsAcmeSslEnabled() { + if domain.HasAcmeChallenge(filepath.Base(r.URL.Path)) { return false } diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go index 67c7831fe..cddf52677 100644 --- a/internal/acme/acme_test.go +++ b/internal/acme/acme_test.go @@ -11,11 +11,11 @@ import ( ) type domainStub struct { - acmeSslEnabled bool + hasAcmeChallenge bool } -func (d *domainStub) IsAcmeSslEnabled() bool { - return d.acmeSslEnabled +func (d *domainStub) HasAcmeChallenge(_ string) bool { + return d.hasAcmeChallenge } func serveAcmeOrNotFound(m *Middleware, domain Domain) http.HandlerFunc { @@ -27,8 +27,8 @@ func serveAcmeOrNotFound(m *Middleware, domain Domain) http.HandlerFunc { } func TestServeAcmeChallenges(t *testing.T) { - domain := &domainStub{acmeSslEnabled: true} - disabledDomain := &domainStub{acmeSslEnabled: false} + domainWithChallenge := &domainStub{hasAcmeChallenge: true} + domain := &domainStub{hasAcmeChallenge: false} baseURL := "http://example.com" indexURL := baseURL + "/index.html" @@ -40,12 +40,12 @@ func TestServeAcmeChallenges(t *testing.T) { testhelpers.TestHTTP404(t, serveAcmeOrNotFound(nil, domain), "GET", challengeURL, nil, nil) }) - t.Run("When acme ssl is disabled for domain", func(t *testing.T) { - testhelpers.TestHTTP404(t, serveAcmeOrNotFound(middleware, disabledDomain), + t.Run("When domain folder contains acme challenge", func(t *testing.T) { + testhelpers.TestHTTP404(t, serveAcmeOrNotFound(middleware, domainWithChallenge), "GET", challengeURL, nil, nil) }) - t.Run("When acme ssl is enabled for domain", func(t *testing.T) { + t.Run("When domain folder doesn't contain acme challenge", func(t *testing.T) { t.Run("When acme challenge is being requested", func(t *testing.T) { testhelpers.AssertRedirectTo(t, serveAcmeOrNotFound(middleware, domain), "GET", challengeURL, nil, diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 32412be7e..0633ce96d 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -179,15 +179,21 @@ func (d *D) IsAccessControlEnabled(r *http.Request) bool { return false } -// IsAcmeSslEnabled checks if Let's Encrypt ssl is enabled for domain -func (d *D) IsAcmeSslEnabled() bool { +// HasAcmeChallenge checks if Let's Encrypt ssl is enabled for domain +func (d *D) HasAcmeChallenge(token string) bool { if d == nil { return false } - // Check custom domain config (e.g. http://example.com) - if d.config != nil { - return d.config.AcmeSslEnabled + if d.config == nil { + return false + } + + _, err := d.resolvePath(d.projectName, "/.well-known/acme-challenge/", token) + + // there is an acme challenge on disk + if err == nil { + return true } return false diff --git a/internal/domain/domain_config.go b/internal/domain/domain_config.go index 421984a96..2ab2ce6cb 100644 --- a/internal/domain/domain_config.go +++ b/internal/domain/domain_config.go @@ -8,13 +8,12 @@ import ( ) type domainConfig struct { - Domain string - Certificate string - Key string - HTTPSOnly bool `json:"https_only"` - ID uint64 `json:"id"` - AccessControl bool `json:"access_control"` - AcmeSslEnabled bool `json:"acme_ssl_enabled"` + Domain string + Certificate string + Key string + HTTPSOnly bool `json:"https_only"` + ID uint64 `json:"id"` + AccessControl bool `json:"access_control"` } type domainsConfig struct { -- GitLab From e33d4c3dbb2cdcf3a2f44cff439ea92c06ec866f Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 5 Apr 2019 13:48:46 +0300 Subject: [PATCH 05/18] Cleanup acme tests --- internal/acme/acme_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go index cddf52677..cb4f54c31 100644 --- a/internal/acme/acme_test.go +++ b/internal/acme/acme_test.go @@ -2,11 +2,8 @@ package acme import ( "net/http" - _ "net/url" "testing" - _ "github.com/stretchr/testify/assert" - "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) -- GitLab From e377330a4d9d473f596ccf9a0012ee4ac261894c Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 5 Apr 2019 18:22:55 +0300 Subject: [PATCH 06/18] Acceptance tests for acme challenges --- acceptance_test.go | 68 ++++++++++++++++++- internal/domain/map_test.go | 2 + .../with.acme.challenge/config.json | 6 ++ .../.well-known/acme-challenge/existingtoken | 1 + .../with.acme.challenge/public/index.html | 1 + 5 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 shared/pages/group.acme/with.acme.challenge/config.json create mode 100644 shared/pages/group.acme/with.acme.challenge/public/.well-known/acme-challenge/existingtoken create mode 100644 shared/pages/group.acme/with.acme.challenge/public/index.html diff --git a/acceptance_test.go b/acceptance_test.go index b22d5e979..16f18ee6a 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -381,7 +381,7 @@ func TestPrometheusMetricsCanBeScraped(t *testing.T) { body, _ := ioutil.ReadAll(resp.Body) assert.Contains(t, string(body), "gitlab_pages_http_sessions_active 0") - assert.Contains(t, string(body), "gitlab_pages_domains_served_total 14") + assert.Contains(t, string(body), "gitlab_pages_domains_served_total 16") } } @@ -800,6 +800,72 @@ func makeGitLabPagesAccessStub(t *testing.T) *httptest.Server { })) } +var existingAcmeTokenPath = "/.well-known/acme-challenge/existingtoken" +var notexistingAcmeTokenPath = "/.well-known/acme-challenge/notexistingtoken" + +func TestAcmeChallengesWhenItIsConfigured(t *testing.T) { + skipUnlessEnabled(t) + + teardown := RunPagesProcess(t, *pagesBinary, listeners, "", "-gitlab-server=https://gitlab-acme.com") + defer teardown() + + t.Run("When domain folder contains requested acme challenge it responds with it", func(t *testing.T) { + rsp, err := GetRedirectPage(t, httpListener, "withacmechallenge.domain.com", + existingAcmeTokenPath) + + defer rsp.Body.Close() + require.NoError(t, err) + require.Equal(t, http.StatusOK, rsp.StatusCode) + body, _ := ioutil.ReadAll(rsp.Body) + require.Equal(t, "this is token\n", string(body)) + }) + + t.Run("When domain folder doesn't contains requested acme challenge it redirects to GitLab", + func(t *testing.T) { + rsp, err := GetRedirectPage(t, httpListener, "withacmechallenge.domain.com", + notexistingAcmeTokenPath) + + defer rsp.Body.Close() + require.NoError(t, err) + require.Equal(t, http.StatusFound, rsp.StatusCode) + + url, err := url.Parse(rsp.Header.Get("Location")) + require.NoError(t, err) + + require.Equal(t, url.String(), "https://gitlab-acme.com/-/acme-challenge/withacmechallenge.domain.com/notexistingtoken") + }, + ) +} + +func TestAcmeChallengesWhenItIsNotConfigured(t *testing.T) { + skipUnlessEnabled(t) + + teardown := RunPagesProcess(t, *pagesBinary, listeners, "", "") + defer teardown() + + t.Run("When domain folder contains requested acme challenge it responds with it", func(t *testing.T) { + rsp, err := GetRedirectPage(t, httpListener, "withacmechallenge.domain.com", + existingAcmeTokenPath) + + defer rsp.Body.Close() + require.NoError(t, err) + require.Equal(t, http.StatusOK, rsp.StatusCode) + body, _ := ioutil.ReadAll(rsp.Body) + require.Equal(t, "this is token\n", string(body)) + }) + + t.Run("When domain folder doesn't contains requested acme challenge it returns 404", + func(t *testing.T) { + rsp, err := GetRedirectPage(t, httpListener, "withacmechallenge.domain.com", + notexistingAcmeTokenPath) + + defer rsp.Body.Close() + require.NoError(t, err) + require.Equal(t, http.StatusNotFound, rsp.StatusCode) + }, + ) +} + func TestAccessControlUnderCustomDomain(t *testing.T) { skipUnlessEnabled(t, "not-inplace-chroot") diff --git a/internal/domain/map_test.go b/internal/domain/map_test.go index dc5e8648d..156b039fd 100644 --- a/internal/domain/map_test.go +++ b/internal/domain/map_test.go @@ -55,6 +55,8 @@ func TestReadProjects(t *testing.T) { "no.cert.com", "private.domain.com", "group.auth.test.io", + "group.acme.test.io", + "withacmechallenge.domain.com", "capitalgroup.test.io", } diff --git a/shared/pages/group.acme/with.acme.challenge/config.json b/shared/pages/group.acme/with.acme.challenge/config.json new file mode 100644 index 000000000..f50ba7fa6 --- /dev/null +++ b/shared/pages/group.acme/with.acme.challenge/config.json @@ -0,0 +1,6 @@ +{ "domains": [ + { + "domain": "withacmechallenge.domain.com" + } + ] +} diff --git a/shared/pages/group.acme/with.acme.challenge/public/.well-known/acme-challenge/existingtoken b/shared/pages/group.acme/with.acme.challenge/public/.well-known/acme-challenge/existingtoken new file mode 100644 index 000000000..84455e1d0 --- /dev/null +++ b/shared/pages/group.acme/with.acme.challenge/public/.well-known/acme-challenge/existingtoken @@ -0,0 +1 @@ +this is token diff --git a/shared/pages/group.acme/with.acme.challenge/public/index.html b/shared/pages/group.acme/with.acme.challenge/public/index.html new file mode 100644 index 000000000..9015a7a32 --- /dev/null +++ b/shared/pages/group.acme/with.acme.challenge/public/index.html @@ -0,0 +1 @@ +index -- GitLab From 24e81aa93f23b5cb79c2abe28162b7b5f03ec44c Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Mon, 8 Apr 2019 11:45:50 +0300 Subject: [PATCH 07/18] Fix config param name in logs --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index fbfba95d8..f0d184509 100644 --- a/main.go +++ b/main.go @@ -243,7 +243,7 @@ func appMain() { "tls-max-version": *tlsMaxVersion, "use-http-2": config.HTTP2, "auth-secret": config.StoreSecret, - "auth-server": config.GitLabServer, + "gitlab-server": config.GitLabServer, "auth-client-id": config.ClientID, "auth-client-secret": config.ClientSecret, "auth-redirect-uri": config.RedirectURI, -- GitLab From 147d5c9b94dfce4be9e50e29db9a311e6d2bf56f Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Mon, 8 Apr 2019 11:53:22 +0300 Subject: [PATCH 08/18] Don't check acme challenge on disk if not required --- internal/acme/acme.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/acme/acme.go b/internal/acme/acme.go index 896bf2416..e86e01c60 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -25,16 +25,16 @@ func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, return false } - if domain.HasAcmeChallenge(filepath.Base(r.URL.Path)) { + if !isAcmeChallenge(r.URL.Path) { return false } - if isAcmeChallenge(r.URL.Path) { - m.redirectToGitlab(w, r) - return true + if domain.HasAcmeChallenge(filepath.Base(r.URL.Path)) { + return false } - return false + m.redirectToGitlab(w, r) + return true } func isAcmeChallenge(path string) bool { -- GitLab From 6cca5ee68e7e05ff64131d9c556031e56fee380e Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Mon, 8 Apr 2019 12:40:58 +0300 Subject: [PATCH 09/18] Rename TestHTTP404 to AssertHTTP404 --- internal/acme/acme_test.go | 6 +++--- internal/domain/domain_test.go | 18 +++++++++--------- internal/testhelpers/testhelpers.go | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go index cb4f54c31..f8499e1b0 100644 --- a/internal/acme/acme_test.go +++ b/internal/acme/acme_test.go @@ -34,11 +34,11 @@ func TestServeAcmeChallenges(t *testing.T) { middleware := &Middleware{GitlabURL: "https://gitlab.example.com"} t.Run("When acme middleware is not configured", func(t *testing.T) { - testhelpers.TestHTTP404(t, serveAcmeOrNotFound(nil, domain), "GET", challengeURL, nil, nil) + testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(nil, domain), "GET", challengeURL, nil, nil) }) t.Run("When domain folder contains acme challenge", func(t *testing.T) { - testhelpers.TestHTTP404(t, serveAcmeOrNotFound(middleware, domainWithChallenge), + testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(middleware, domainWithChallenge), "GET", challengeURL, nil, nil) }) @@ -50,7 +50,7 @@ func TestServeAcmeChallenges(t *testing.T) { }) t.Run("When pages content is being requested", func(t *testing.T) { - testhelpers.TestHTTP404(t, serveAcmeOrNotFound(middleware, domain), + testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(middleware, domain), "GET", indexURL, nil, nil) }) }) diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index eb44c3421..4b6cbd24b 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -317,15 +317,15 @@ func TestGroup404ServeHTTP(t *testing.T) { }, } - testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/not/existing-file", nil, "Custom 404 project page") - testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/", nil, "Custom 404 project page") - testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not/existing-file", nil, "Custom 404 group page") - testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") - testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/not/existing-file", nil, "Custom 404 project page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404/", nil, "Custom 404 project page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not/existing-file", nil, "Custom 404 group page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") assert.HTTPBodyNotContains(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.404.symlink/not/existing-file", nil, "Custom 404 project page") // Ensure the namespace project's custom 404.html is not used by projects - testhelpers.TestHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.no.404/not/existing-file", nil, "The page you're looking for could not be found.") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testGroup), "GET", "http://group.404.test.io/project.no.404/not/existing-file", nil, "The page you're looking for could not be found.") } func TestDomain404ServeHTTP(t *testing.T) { @@ -340,8 +340,8 @@ func TestDomain404ServeHTTP(t *testing.T) { }, } - testhelpers.TestHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") - testhelpers.TestHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/not-existing-file", nil, "Custom 404 group page") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.404.test.io/", nil, "Custom 404 group page") } func TestPredefined404ServeHTTP(t *testing.T) { @@ -352,7 +352,7 @@ func TestPredefined404ServeHTTP(t *testing.T) { group: group{name: "group"}, } - testhelpers.TestHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found") + testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found") } func TestGroupCertificate(t *testing.T) { diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index 5222bb818..e5bf8af04 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -11,8 +11,8 @@ import ( "github.com/stretchr/testify/require" ) -// TestHTTP404 asserts handler returns 404 with provided str body -func TestHTTP404(t *testing.T, handler http.HandlerFunc, mode, url string, values url.Values, str interface{}) { +// AssertHTTP404 asserts handler returns 404 with provided str body +func AssertHTTP404(t *testing.T, handler http.HandlerFunc, mode, url string, values url.Values, str interface{}) { w := httptest.NewRecorder() req, err := http.NewRequest(mode, url+"?"+values.Encode(), nil) require.NoError(t, err) -- GitLab From 643e3c9d0ba70ea0912a9282bdb5295ceeb367f3 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Mon, 8 Apr 2019 12:44:44 +0300 Subject: [PATCH 10/18] Fix flag help message --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index f0d184509..728bc43f2 100644 --- a/main.go +++ b/main.go @@ -49,7 +49,7 @@ var ( adminHTTPSCert = flag.String("admin-https-cert", "", "The path to the certificate file for the admin API (optional)") adminHTTPSKey = flag.String("admin-https-key", "", "The path to the key file for the admin API (optional)") secret = flag.String("auth-secret", "", "Cookie store hash key, should be at least 32 bytes long.") - gitLabAuthServer = flag.String("auth-server", "", "DEPRECATED, use gitlab-server insteadGitLab server, for example https://www.gitlab.com") + gitLabAuthServer = flag.String("auth-server", "", "DEPRECATED, use gitlab-server instead. GitLab server, for example https://www.gitlab.com") gitLabServer = flag.String("gitlab-server", "", "GitLab server, for example https://www.gitlab.com") clientID = flag.String("auth-client-id", "", "GitLab application Client ID") clientSecret = flag.String("auth-client-secret", "", "GitLab application Client Secret") -- GitLab From ab6c00d191f11d75d54b355cd76fc06d700bb969 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 3 May 2019 13:33:18 +0300 Subject: [PATCH 11/18] Extract getHost to separate package --- internal/acme/acme.go | 15 +++------------ internal/domain/domain.go | 14 ++------------ internal/host/host.go | 23 +++++++++++++++++++++++ internal/host/host_test.go | 17 +++++++++++++++++ 4 files changed, 45 insertions(+), 24 deletions(-) create mode 100644 internal/host/host.go create mode 100644 internal/host/host_test.go diff --git a/internal/acme/acme.go b/internal/acme/acme.go index e86e01c60..5ec3cac5c 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -1,12 +1,13 @@ package acme import ( - "net" "net/http" "path/filepath" "strings" log "github.com/sirupsen/logrus" + + "gitlab.com/gitlab-org/gitlab-pages/internal/host" ) // Middleware handles acme challenges by redirecting them to GitLab instance @@ -42,20 +43,10 @@ func isAcmeChallenge(path string) bool { } func (m *Middleware) redirectToGitlab(w http.ResponseWriter, r *http.Request) { - redirectURL := m.GitlabURL + "/-/acme-challenge/" + getHost(r) + + redirectURL := m.GitlabURL + "/-/acme-challenge/" + host.FromRequest(r) + "/" + filepath.Base(r.URL.Path) log.WithField("redirect_url", redirectURL).Info("Redirecting to GitLab for processing acme challenge") http.Redirect(w, r, redirectURL, 302) } - -func getHost(r *http.Request) string { - host := strings.ToLower(r.Host) - - if splitHost, _, err := net.SplitHostPort(host); err == nil { - host = splitHost - } - - return host -} diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 0633ce96d..91026e8d8 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "mime" - "net" "net/http" "os" "path/filepath" @@ -17,6 +16,7 @@ import ( "golang.org/x/sys/unix" + "gitlab.com/gitlab-org/gitlab-pages/internal/host" "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/httputil" ) @@ -106,16 +106,6 @@ func handleGZip(w http.ResponseWriter, r *http.Request, fullPath string) string return gzipPath } -func getHost(r *http.Request) string { - host := strings.ToLower(r.Host) - - if splitHost, _, err := net.SplitHostPort(host); err == nil { - host = splitHost - } - - return host -} - // Look up a project inside the domain based on the host and path. Returns the // project and its name (if applicable) func (d *D) getProjectWithSubpath(r *http.Request) (*project, string, string) { @@ -131,7 +121,7 @@ func (d *D) getProjectWithSubpath(r *http.Request) (*project, string, string) { // Since the URL doesn't specify a project (e.g. http://mydomain.gitlab.io), // return the group project if it exists. - if host := getHost(r); host != "" { + if host := host.FromRequest(r); host != "" { if groupProject := d.projects[host]; groupProject != nil { return groupProject, host, strings.Join(split[1:], "/") } diff --git a/internal/host/host.go b/internal/host/host.go new file mode 100644 index 000000000..dd9b1c8a2 --- /dev/null +++ b/internal/host/host.go @@ -0,0 +1,23 @@ +package host + +import ( + "net" + "net/http" + "strings" +) + +// FromString tries to split host port from string, returns host or initial string if fail +func FromString(s string) string { + host := strings.ToLower(s) + + if splitHost, _, err := net.SplitHostPort(host); err == nil { + host = splitHost + } + + return host +} + +// FromRequest tries to split host port from r.Host, returns host or initial string if fail +func FromRequest(r *http.Request) string { + return FromString(r.Host) +} diff --git a/internal/host/host_test.go b/internal/host/host_test.go new file mode 100644 index 000000000..8f44d2e28 --- /dev/null +++ b/internal/host/host_test.go @@ -0,0 +1,17 @@ +package host + +import ( + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFromString(t *testing.T) { + assert.Equal(t, "example.com", FromString("example.com")) + assert.Equal(t, "example.com", FromString("example.com:8080")) +} + +func TestFromRequest(t *testing.T) { + assert.Equal(t, "example.com", FromRequest(httptest.NewRequest("GET", "example.com:8080/123", nil))) +} -- GitLab From 4d2c7503f325530d7233c18c385bdd381908e87c Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 3 May 2019 13:35:43 +0300 Subject: [PATCH 12/18] Fix comment for HasAcmeChallenge --- internal/domain/domain.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 91026e8d8..a8a3f9288 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -169,7 +169,7 @@ func (d *D) IsAccessControlEnabled(r *http.Request) bool { return false } -// HasAcmeChallenge checks if Let's Encrypt ssl is enabled for domain +// HasAcmeChallenge checks domain directory contains particular acme challenge func (d *D) HasAcmeChallenge(token string) bool { if d == nil { return false -- GitLab From 72670d778613f57ef9908b860410325e91a0a5e0 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 3 May 2019 18:41:55 +0300 Subject: [PATCH 13/18] Add tests for domain.D.HasAcmeChallenge --- internal/domain/domain.go | 2 +- internal/domain/domain_test.go | 56 +++++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index a8a3f9288..4ce8a561e 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -179,7 +179,7 @@ func (d *D) HasAcmeChallenge(token string) bool { return false } - _, err := d.resolvePath(d.projectName, "/.well-known/acme-challenge/", token) + _, err := d.resolvePath(d.projectName, ".well-known/acme-challenge", token) // there is an acme challenge on disk if err == nil { diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 4b6cbd24b..a05dbe7e5 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -200,7 +200,61 @@ func TestIsHTTPSOnly(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { req, _ := http.NewRequest(http.MethodGet, test.url, nil) - assert.Equal(t, test.domain.IsHTTPSOnly(req), test.expected) + assert.Equal(t, test.expected, test.domain.IsHTTPSOnly(req)) + }) + } +} + +func TestHasAcmeChallenge(t *testing.T) { + cleanup := setUpTests(t) + defer cleanup() + + tests := []struct { + name string + domain *D + token string + expected bool + }{ + { + name: "Project containing acme challenge", + domain: &D{ + group: group{name: "group.acme"}, + projectName: "with.acme.challenge", + config: &domainConfig{HTTPSOnly: true}, + }, + token: "existingtoken", + expected: true, + }, + { + name: "Project containing another token", + domain: &D{ + group: group{name: "group.acme"}, + projectName: "with.acme.challenge", + config: &domainConfig{HTTPSOnly: true}, + }, + token: "notexistingtoken", + expected: false, + }, + { + name: "nil domain", + domain: nil, + token: "existingtoken", + expected: false, + }, + { + name: "Domain without config", + domain: &D{ + group: group{name: "group.acme"}, + projectName: "with.acme.challenge", + config: nil, + }, + token: "existingtoken", + expected: false, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expected, test.domain.HasAcmeChallenge(test.token)) }) } } -- GitLab From 81dcd70f0f4ba2893afadb1b3a44b8cf47a37eb1 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 3 May 2019 19:27:06 +0300 Subject: [PATCH 14/18] Add tests for gitlab-server config fallbacks --- config_test.go | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ main.go | 10 +++------- 2 files changed, 57 insertions(+), 7 deletions(-) create mode 100644 config_test.go diff --git a/config_test.go b/config_test.go new file mode 100644 index 000000000..3ec51c569 --- /dev/null +++ b/config_test.go @@ -0,0 +1,54 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGitLabServerFromFlags(t *testing.T) { + tests := []struct { + name string + gitLabServer string + gitLabAuthServer string + artifactsServer string + expected string + }{ + { + name: "When gitLabServer is set", + gitLabServer: "gitlabserver.com", + gitLabAuthServer: "authserver.com", + artifactsServer: "https://artifactsserver.com", + expected: "gitlabserver.com", + }, + { + name: "When auth server is set", + gitLabServer: "", + gitLabAuthServer: "authserver.com", + artifactsServer: "https://artifactsserver.com", + expected: "authserver.com", + }, + { + name: "When only artifacts server is set", + gitLabServer: "", + gitLabAuthServer: "", + artifactsServer: "https://artifactsserver.com", + expected: "artifactsserver.com", + }, + { + name: "When only artifacts server includes path", + gitLabServer: "", + gitLabAuthServer: "", + artifactsServer: "https://artifactsserver.com:8080/api/path", + expected: "artifactsserver.com", + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + gitLabServer = &test.gitLabServer + gitLabAuthServer = &test.gitLabAuthServer + artifactsServer = &test.artifactsServer + assert.Equal(t, test.expected, gitlabServerFromFlags()) + }) + } +} diff --git a/main.go b/main.go index 728bc43f2..d043d9689 100644 --- a/main.go +++ b/main.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "io" - "net" "net/url" "os" "strings" @@ -12,6 +11,7 @@ import ( "github.com/namsral/flag" log "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitlab-pages/internal/host" "gitlab.com/gitlab-org/gitlab-pages/internal/tlsconfig" ) @@ -88,12 +88,8 @@ func gitlabServerFromFlags() string { return *gitLabAuthServer } - url, err := url.Parse(*artifactsServer) - if err != nil { - return "" - } - host, _, _ := net.SplitHostPort(url.Host) - return host + url, _ := url.Parse(*artifactsServer) + return host.FromString(url.Host) } func configFromFlags() appConfig { -- GitLab From 5db423ff84df3742bb87db4d3b993d9c13ddffa7 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Tue, 7 May 2019 12:11:40 +0300 Subject: [PATCH 15/18] Review fixes --- acceptance_test.go | 2 +- internal/acme/acme.go | 6 +++--- internal/acme/acme_test.go | 2 +- internal/testhelpers/testhelpers.go | 6 ++++++ 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index 16f18ee6a..ddd340ab5 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -827,7 +827,7 @@ func TestAcmeChallengesWhenItIsConfigured(t *testing.T) { defer rsp.Body.Close() require.NoError(t, err) - require.Equal(t, http.StatusFound, rsp.StatusCode) + require.Equal(t, http.StatusTemporaryRedirect, rsp.StatusCode) url, err := url.Parse(rsp.Header.Get("Location")) require.NoError(t, err) diff --git a/internal/acme/acme.go b/internal/acme/acme.go index 5ec3cac5c..8fd683545 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -39,14 +39,14 @@ func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, } func isAcmeChallenge(path string) bool { - return strings.HasPrefix(path, "/.well-known/acme-challenge/") + return strings.HasPrefix(filepath.Clean(path), "/.well-known/acme-challenge/") } func (m *Middleware) redirectToGitlab(w http.ResponseWriter, r *http.Request) { redirectURL := m.GitlabURL + "/-/acme-challenge/" + host.FromRequest(r) + "/" + filepath.Base(r.URL.Path) - log.WithField("redirect_url", redirectURL).Info("Redirecting to GitLab for processing acme challenge") + log.WithField("redirect_url", redirectURL).Debug("Redirecting to GitLab for processing acme challenge") - http.Redirect(w, r, redirectURL, 302) + http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect) } diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go index f8499e1b0..37016f051 100644 --- a/internal/acme/acme_test.go +++ b/internal/acme/acme_test.go @@ -44,7 +44,7 @@ func TestServeAcmeChallenges(t *testing.T) { t.Run("When domain folder doesn't contain acme challenge", func(t *testing.T) { t.Run("When acme challenge is being requested", func(t *testing.T) { - testhelpers.AssertRedirectTo(t, serveAcmeOrNotFound(middleware, domain), + testhelpers.AssertTemporaryRedirectTo(t, serveAcmeOrNotFound(middleware, domain), "GET", challengeURL, nil, "https://gitlab.example.com/-/acme-challenge/example.com/token") }) diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index e5bf8af04..8ae872a0f 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -32,3 +32,9 @@ func AssertRedirectTo(t *testing.T, h http.HandlerFunc, method string, url strin assert.HTTPRedirect(t, h, method, url, values) assert.HTTPBodyContains(t, h, method, url, values, `Found`) } + +// AssertTemporaryRedirectTo asserts that handler redirects to particular URL with 307 status +func AssertTemporaryRedirectTo(t *testing.T, h http.HandlerFunc, method string, url string, values url.Values, expectedURL string) { + assert.HTTPRedirect(t, h, method, url, values) + assert.HTTPBodyContains(t, h, method, url, values, `Temporary Redirect`) +} -- GitLab From 002383b15b0a2e57cbc00a37a1e4871818f6be46 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Tue, 7 May 2019 13:16:36 +0300 Subject: [PATCH 16/18] Use url query to pass acme challenge domain and token --- acceptance_test.go | 2 +- internal/acme/acme.go | 22 ++++++++++++++++------ internal/acme/acme_test.go | 4 ++-- internal/testhelpers/testhelpers.go | 20 ++++++++++++-------- 4 files changed, 31 insertions(+), 17 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index ddd340ab5..eaa32318f 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -832,7 +832,7 @@ func TestAcmeChallengesWhenItIsConfigured(t *testing.T) { url, err := url.Parse(rsp.Header.Get("Location")) require.NoError(t, err) - require.Equal(t, url.String(), "https://gitlab-acme.com/-/acme-challenge/withacmechallenge.domain.com/notexistingtoken") + require.Equal(t, url.String(), "https://gitlab-acme.com/-/acme-challenge?domain=withacmechallenge.domain.com&token=notexistingtoken") }, ) } diff --git a/internal/acme/acme.go b/internal/acme/acme.go index 8fd683545..89881f34b 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -2,6 +2,7 @@ package acme import ( "net/http" + "net/url" "path/filepath" "strings" @@ -34,19 +35,28 @@ func (m *Middleware) ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, return false } - m.redirectToGitlab(w, r) - return true + return m.redirectToGitlab(w, r) } func isAcmeChallenge(path string) bool { return strings.HasPrefix(filepath.Clean(path), "/.well-known/acme-challenge/") } -func (m *Middleware) redirectToGitlab(w http.ResponseWriter, r *http.Request) { - redirectURL := m.GitlabURL + "/-/acme-challenge/" + host.FromRequest(r) + - "/" + filepath.Base(r.URL.Path) +func (m *Middleware) redirectToGitlab(w http.ResponseWriter, r *http.Request) bool { + redirectURL, err := url.Parse(m.GitlabURL) + if err != nil { + log.WithError(err).Error("Can't parse GitLab URL for acme challenge redirect") + return false + } + + redirectURL.Path = "/-/acme-challenge" + query := redirectURL.Query() + query.Set("domain", host.FromRequest(r)) + query.Set("token", filepath.Base(r.URL.Path)) + redirectURL.RawQuery = query.Encode() log.WithField("redirect_url", redirectURL).Debug("Redirecting to GitLab for processing acme challenge") - http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect) + http.Redirect(w, r, redirectURL.String(), http.StatusTemporaryRedirect) + return true } diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go index 37016f051..c04d784f1 100644 --- a/internal/acme/acme_test.go +++ b/internal/acme/acme_test.go @@ -44,9 +44,9 @@ func TestServeAcmeChallenges(t *testing.T) { t.Run("When domain folder doesn't contain acme challenge", func(t *testing.T) { t.Run("When acme challenge is being requested", func(t *testing.T) { - testhelpers.AssertTemporaryRedirectTo(t, serveAcmeOrNotFound(middleware, domain), + testhelpers.AssertRedirectTo(t, serveAcmeOrNotFound(middleware, domain), "GET", challengeURL, nil, - "https://gitlab.example.com/-/acme-challenge/example.com/token") + "https://gitlab.example.com/-/acme-challenge?domain=example.com&token=token") }) t.Run("When pages content is being requested", func(t *testing.T) { diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index 8ae872a0f..1c3a1eba7 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -28,13 +28,17 @@ func AssertHTTP404(t *testing.T, handler http.HandlerFunc, mode, url string, val } // AssertRedirectTo asserts that handler redirects to particular URL -func AssertRedirectTo(t *testing.T, h http.HandlerFunc, method string, url string, values url.Values, expectedURL string) { - assert.HTTPRedirect(t, h, method, url, values) - assert.HTTPBodyContains(t, h, method, url, values, `Found`) -} +func AssertRedirectTo(t *testing.T, handler http.HandlerFunc, method string, + url string, values url.Values, expectedURL string) { + + assert.HTTPRedirect(t, handler, method, url, values) + + recorder := httptest.NewRecorder() + + req, _ := http.NewRequest(method, url, nil) + req.URL.RawQuery = values.Encode() + + handler(recorder, req) -// AssertTemporaryRedirectTo asserts that handler redirects to particular URL with 307 status -func AssertTemporaryRedirectTo(t *testing.T, h http.HandlerFunc, method string, url string, values url.Values, expectedURL string) { - assert.HTTPRedirect(t, h, method, url, values) - assert.HTTPBodyContains(t, h, method, url, values, `Temporary Redirect`) + assert.Equal(t, expectedURL, recorder.Header().Get("Location")) } -- GitLab From 66680f582029fd0a061d06056dfb9dc45580fe77 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Tue, 7 May 2019 17:43:19 +0300 Subject: [PATCH 17/18] Rewrite acme middleware tests --- internal/acme/acme_test.go | 59 ++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/internal/acme/acme_test.go b/internal/acme/acme_test.go index c04d784f1..c0daefebe 100644 --- a/internal/acme/acme_test.go +++ b/internal/acme/acme_test.go @@ -23,35 +23,32 @@ func serveAcmeOrNotFound(m *Middleware, domain Domain) http.HandlerFunc { } } -func TestServeAcmeChallenges(t *testing.T) { - domainWithChallenge := &domainStub{hasAcmeChallenge: true} - domain := &domainStub{hasAcmeChallenge: false} - - baseURL := "http://example.com" - indexURL := baseURL + "/index.html" - challengeURL := baseURL + "/.well-known/acme-challenge/token" - - middleware := &Middleware{GitlabURL: "https://gitlab.example.com"} - - t.Run("When acme middleware is not configured", func(t *testing.T) { - testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(nil, domain), "GET", challengeURL, nil, nil) - }) - - t.Run("When domain folder contains acme challenge", func(t *testing.T) { - testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(middleware, domainWithChallenge), - "GET", challengeURL, nil, nil) - }) - - t.Run("When domain folder doesn't contain acme challenge", func(t *testing.T) { - t.Run("When acme challenge is being requested", func(t *testing.T) { - testhelpers.AssertRedirectTo(t, serveAcmeOrNotFound(middleware, domain), - "GET", challengeURL, nil, - "https://gitlab.example.com/-/acme-challenge?domain=example.com&token=token") - }) - - t.Run("When pages content is being requested", func(t *testing.T) { - testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(middleware, domain), - "GET", indexURL, nil, nil) - }) - }) +const ( + baseURL = "http://example.com" + indexURL = baseURL + "/index.html" + challengeURL = baseURL + "/.well-known/acme-challenge/token" +) + +var ( + domainWithChallenge = &domainStub{hasAcmeChallenge: true} + domain = &domainStub{hasAcmeChallenge: false} + middleware = &Middleware{GitlabURL: "https://gitlab.example.com"} +) + +func TestServeAcmeChallengesNotConfigured(t *testing.T) { + testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(nil, domain), "GET", challengeURL, nil, nil) +} + +func TestServeAcmeChallengeWhenPresent(t *testing.T) { + testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(middleware, domainWithChallenge), "GET", challengeURL, nil, nil) +} + +func TestServeAcmeChallengeWhenMissing(t *testing.T) { + testhelpers.AssertRedirectTo( + t, serveAcmeOrNotFound(middleware, domain), + "GET", challengeURL, nil, + "https://gitlab.example.com/-/acme-challenge?domain=example.com&token=token", + ) + + testhelpers.AssertHTTP404(t, serveAcmeOrNotFound(middleware, domain), "GET", indexURL, nil, nil) } -- GitLab From 88cff4113adf74325bb456a50d8afd925b0f6450 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Wed, 8 May 2019 15:09:42 +0300 Subject: [PATCH 18/18] Add test for lowercasing host --- internal/host/host_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/host/host_test.go b/internal/host/host_test.go index 8f44d2e28..8395d3fc2 100644 --- a/internal/host/host_test.go +++ b/internal/host/host_test.go @@ -9,6 +9,7 @@ import ( func TestFromString(t *testing.T) { assert.Equal(t, "example.com", FromString("example.com")) + assert.Equal(t, "example.com", FromString("eXAmpLe.com")) assert.Equal(t, "example.com", FromString("example.com:8080")) } -- GitLab