diff options
author | Filip Aleksic <faleksic@gitlab.com> | 2022-07-26 10:53:41 +0300 |
---|---|---|
committer | Jaime Martinez <jmartinez@gitlab.com> | 2022-07-26 10:53:41 +0300 |
commit | a131f2bdf10e2815d79e3581fe6536b1499839b6 (patch) | |
tree | 723189f5398744b7e5a0e260835a0c38f84f2f66 | |
parent | 0674de2e8e74a35bec70380b02da63dd7db1b8bb (diff) |
Fixes acme redirection issues when using a wildcard redirect
Changelog: fixed
-rw-r--r-- | internal/acme/acme.go | 4 | ||||
-rw-r--r-- | internal/redirects/redirects.go | 5 | ||||
-rw-r--r-- | internal/redirects/redirects_test.go | 8 | ||||
-rw-r--r-- | shared/pages/group.acme/with.redirects/config.json | 6 | ||||
-rw-r--r-- | shared/pages/group.acme/with.redirects/public.zip | bin | 0 -> 1607 bytes | |||
-rw-r--r-- | shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/existingtoken | 1 | ||||
-rw-r--r-- | shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/foldertoken/index.html | 1 | ||||
-rw-r--r-- | shared/pages/group.acme/with.redirects/public/_redirects | 1 | ||||
-rw-r--r-- | shared/pages/group.acme/with.redirects/public/index.html | 1 | ||||
-rw-r--r-- | test/acceptance/acme_test.go | 54 | ||||
-rw-r--r-- | test/gitlabstub/api_responses.go | 4 |
11 files changed, 83 insertions, 2 deletions
diff --git a/internal/acme/acme.go b/internal/acme/acme.go index c83fcc08..ae808cdd 100644 --- a/internal/acme/acme.go +++ b/internal/acme/acme.go @@ -15,7 +15,7 @@ type FallbackStrategy func(http.ResponseWriter, *http.Request) bool // ServeAcmeChallenges identifies if request is acme-challenge and redirects to GitLab in that case func ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, fallback FallbackStrategy, gitlabURL *url.URL) bool { - if !isAcmeChallenge(r.URL.Path) { + if !IsAcmeChallenge(r.URL.Path) { return false } @@ -27,7 +27,7 @@ func ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, fallback Fallba return true } -func isAcmeChallenge(path string) bool { +func IsAcmeChallenge(path string) bool { return strings.HasPrefix(filepath.Clean(path), "/.well-known/acme-challenge/") } diff --git a/internal/redirects/redirects.go b/internal/redirects/redirects.go index 4add6537..4903518a 100644 --- a/internal/redirects/redirects.go +++ b/internal/redirects/redirects.go @@ -13,6 +13,7 @@ import ( netlifyRedirects "github.com/tj/go-redirects" "gitlab.com/gitlab-org/labkit/log" + "gitlab.com/gitlab-org/gitlab-pages/internal/acme" "gitlab.com/gitlab-org/gitlab-pages/internal/config" "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" ) @@ -106,6 +107,10 @@ func (r *Redirects) Status() string { // Rewrite takes in a URL and uses the parsed Netlify rules to rewrite // the URL to the new location if it matches any rule func (r *Redirects) Rewrite(originalURL *url.URL) (*url.URL, int, error) { + if acme.IsAcmeChallenge(originalURL.Path) { + return nil, 0, ErrNoRedirect + } + rule, newPath := r.match(originalURL.Path) if rule == nil { return nil, 0, ErrNoRedirect diff --git a/internal/redirects/redirects_test.go b/internal/redirects/redirects_test.go index d40b065b..4ec34222 100644 --- a/internal/redirects/redirects_test.go +++ b/internal/redirects/redirects_test.go @@ -107,6 +107,14 @@ func TestRedirectsRewrite(t *testing.T) { expectedURL: "/the/cake/is/a/lie", expectedStatus: http.StatusOK, }, + { + name: "does_not_redirect_acme_challenges", + url: "/.well-known/acme-challenge/token", + rule: "/* /to/path 200", + expectedURL: "", + expectedStatus: 0, + expectedErr: ErrNoRedirect, + }, } for _, tt := range tests { diff --git a/shared/pages/group.acme/with.redirects/config.json b/shared/pages/group.acme/with.redirects/config.json new file mode 100644 index 00000000..cc0d8ce6 --- /dev/null +++ b/shared/pages/group.acme/with.redirects/config.json @@ -0,0 +1,6 @@ +{ "domains": [ + { + "domain": "acmewithredirects.domain.com" + } +] +} diff --git a/shared/pages/group.acme/with.redirects/public.zip b/shared/pages/group.acme/with.redirects/public.zip Binary files differnew file mode 100644 index 00000000..d6142bec --- /dev/null +++ b/shared/pages/group.acme/with.redirects/public.zip diff --git a/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/existingtoken b/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/existingtoken new file mode 100644 index 00000000..84455e1d --- /dev/null +++ b/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/existingtoken @@ -0,0 +1 @@ +this is token diff --git a/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/foldertoken/index.html b/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/foldertoken/index.html new file mode 100644 index 00000000..3212dd99 --- /dev/null +++ b/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/foldertoken/index.html @@ -0,0 +1 @@ +foldertoken content diff --git a/shared/pages/group.acme/with.redirects/public/_redirects b/shared/pages/group.acme/with.redirects/public/_redirects new file mode 100644 index 00000000..f8243379 --- /dev/null +++ b/shared/pages/group.acme/with.redirects/public/_redirects @@ -0,0 +1 @@ +/* /index.html 200
\ No newline at end of file diff --git a/shared/pages/group.acme/with.redirects/public/index.html b/shared/pages/group.acme/with.redirects/public/index.html new file mode 100644 index 00000000..2f47ed25 --- /dev/null +++ b/shared/pages/group.acme/with.redirects/public/index.html @@ -0,0 +1 @@ +acme-challenge-project diff --git a/test/acceptance/acme_test.go b/test/acceptance/acme_test.go index 40d98a21..89c57c96 100644 --- a/test/acceptance/acme_test.go +++ b/test/acceptance/acme_test.go @@ -7,6 +7,8 @@ import ( "testing" "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) const ( @@ -97,3 +99,55 @@ func TestAcmeChallengesWhenItIsConfigured(t *testing.T) { }) } } + +func TestAcmeChallengesNoRedirection(t *testing.T) { + RunPagesProcess(t, + withListeners([]ListenSpec{httpListener}), + withExtraArgument("gitlab-server", "https://gitlab-acme.com"), + ) + + tests := map[string]struct { + path string + expectedStatus int + expectedContent string + expectedLocation string + }{ + "wildcard redirect should not redirect acme challenge": { + path: existingAcmeTokenPath, + expectedStatus: http.StatusOK, + expectedContent: "this is token\n", + }, + "non-acme paths should be redirected": { + path: "/example", + // rule inside _redirects is a 200 rewrite of /index.html + expectedStatus: http.StatusOK, + expectedContent: "acme-challenge-project\n", + }, + "When domain folder doesn't contain requested acme challenge it redirects to GitLab": { + path: notExistingAcmeTokenPath, + expectedStatus: http.StatusTemporaryRedirect, + expectedContent: "", + expectedLocation: "https://gitlab-acme.com/-/acme-challenge?domain=acmewithredirects.domain.com&token=notexistingtoken", + }, + } + + for tn, tt := range tests { + t.Run(tn, func(t *testing.T) { + rsp, err := GetRedirectPage(t, httpListener, "acmewithredirects.domain.com", tt.path) + require.NoError(t, err) + testhelpers.Close(t, rsp.Body) + + require.Equal(t, tt.expectedStatus, rsp.StatusCode) + body, err := io.ReadAll(rsp.Body) + require.NoError(t, rsp.Body.Close()) + require.NoError(t, err) + + require.Contains(t, string(body), tt.expectedContent) + + redirectURL, err := url.Parse(rsp.Header.Get("Location")) + require.NoError(t, err) + + require.Equal(t, redirectURL.String(), tt.expectedLocation) + }) + } +} diff --git a/test/gitlabstub/api_responses.go b/test/gitlabstub/api_responses.go index 3ebf4e1f..93bc8995 100644 --- a/test/gitlabstub/api_responses.go +++ b/test/gitlabstub/api_responses.go @@ -110,6 +110,10 @@ var domainResponses = map[string]responseFn{ accessControl: true, pathOnDisk: "group.auth/private.project", }), + "acmewithredirects.domain.com": customDomain(projectConfig{ + projectID: 1008, + pathOnDisk: "group.acme/with.redirects", + }), // NOTE: before adding more domains here, generate the zip archive by running (per project) // make zip PROJECT_SUBDIR=group/serving // make zip PROJECT_SUBDIR=group/project2 |