From dfc530e97ea3ebdaff6aaa246415254918c22f98 Mon Sep 17 00:00:00 2001 From: Valentin Maerten Date: Sun, 23 Aug 2026 13:28:20 +0200 Subject: [PATCH] fix(remote): allow a downgrade redirect under --insecure Refusing it unconditionally was security theatre: --insecure also sets InsecureSkipVerify, so an attacker in position to intercept can already serve anything over the https leg with a self-signed certificate. It also broke an internal server that redirects and works today. --insecure now means one thing everywhere: the transport guarantees are waived. --- CHANGELOG.md | 9 ++-- errors/errors_taskfile.go | 2 +- taskfile/node_http.go | 33 +++++++----- taskfile/node_http_test.go | 64 ++++++++++++++++++----- website/src/next/docs/remote-taskfiles.md | 6 +-- 5 files changed, 77 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e1b0778f..2b75c250 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,10 @@ ### 🐛 Fixes -- Fixed a remote Taskfile served over `https` being downloaded in the clear when - the server redirects to `http`. The scheme was only checked on the URL you - wrote, not on the ones you were redirected to, so neither the detection - request nor the download was protected. Such a redirect is now refused, even - with `--insecure` (by @vmaerten). +- Fixed an `https` remote Taskfile being downloaded in the clear if the server + redirected to `http`, leaving the file that is about to be executed open to + tampering. Such a redirect now requires `--insecure`, like an `http` + entrypoint (by @vmaerten). ### 📦 Package API diff --git a/errors/errors_taskfile.go b/errors/errors_taskfile.go index fa7fab61..725b865c 100644 --- a/errors/errors_taskfile.go +++ b/errors/errors_taskfile.go @@ -110,7 +110,7 @@ type TaskfileNotSecureError struct { func (err *TaskfileNotSecureError) Error() string { if err.Redirect { return fmt.Sprintf( - `task: Taskfile %q was redirected to over an insecure connection. Point the URL at the final location instead`, + `task: Taskfile %q was redirected to over an insecure connection. You can override this by using the --insecure flag`, filepath.ToSlash(err.URI), ) } diff --git a/taskfile/node_http.go b/taskfile/node_http.go index cfcc9614..d96cf7e8 100644 --- a/taskfile/node_http.go +++ b/taskfile/node_http.go @@ -36,7 +36,7 @@ func buildHTTPClient(insecure bool, caCert, cert, certKey string) (*http.Client, // hand it out: setting CheckRedirect on it would apply process-wide. if !insecure && caCert == "" && cert == "" { client := *http.DefaultClient - client.CheckRedirect = checkRedirect + client.CheckRedirect = checkRedirect(insecure) return &client, nil } @@ -70,24 +70,29 @@ func buildHTTPClient(insecure bool, caCert, cert, certKey string) (*http.Client, Transport: &http.Transport{ TLSClientConfig: tlsConfig, }, - CheckRedirect: checkRedirect, + CheckRedirect: checkRedirect(insecure), }, nil } -// checkRedirect refuses a redirect that would drop TLS. --insecure does not -// loosen it: an http:// entrypoint is the user's choice, a redirect is not. -func checkRedirect(req *http.Request, via []*http.Request) error { - // Setting CheckRedirect replaces the default cap, so it has to be kept. - if len(via) >= 10 { - return fmt.Errorf("stopped after 10 redirects") - } - if len(via) == 0 { +// checkRedirect refuses a redirect that would drop TLS, unless --insecure was +// given: that flag also disables certificate verification, so refusing the +// plaintext hop would guard nothing an attacker could not walk around. +func checkRedirect(insecure bool) func(*http.Request, []*http.Request) error { + return func(req *http.Request, via []*http.Request) error { + // Setting CheckRedirect replaces the default cap, so it has to be kept. + if len(via) >= 10 { + return fmt.Errorf("stopped after 10 redirects") + } + if len(via) == 0 { + return nil + } + if !insecure && + via[len(via)-1].URL.Scheme == "https" && + req.URL.Scheme == "http" { + return &errors.TaskfileNotSecureError{URI: req.URL.Redacted(), Redirect: true} + } return nil } - if via[len(via)-1].URL.Scheme == "https" && req.URL.Scheme == "http" { - return &errors.TaskfileNotSecureError{URI: req.URL.Redacted(), Redirect: true} - } - return nil } func NewHTTPNode( diff --git a/taskfile/node_http_test.go b/taskfile/node_http_test.go index b49346f4..f1e4c553 100644 --- a/taskfile/node_http_test.go +++ b/taskfile/node_http_test.go @@ -12,6 +12,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "sync/atomic" "testing" "time" @@ -294,13 +295,15 @@ func TestCheckRedirect(t *testing.T) { t.Parallel() tests := []struct { - name string - from string - to string - wantErr bool + name string + from string + to string + insecure bool + wantErr bool }{ {name: "https to http is refused", from: "https://example.com", to: "http://example.com", wantErr: true}, {name: "https to http on another host is refused", from: "https://example.com", to: "http://evil.test", wantErr: true}, + {name: "https to http is allowed with insecure", from: "https://example.com", to: "http://example.com", insecure: true}, {name: "https to https is allowed", from: "https://example.com", to: "https://other.example.com"}, {name: "http to https is allowed", from: "http://example.com", to: "https://example.com"}, {name: "http to http is allowed, the entrypoint already opted in", from: "http://example.com", to: "http://other.example.com"}, @@ -310,7 +313,7 @@ func TestCheckRedirect(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() via := []*http.Request{mustGet(t, tt.from)} - err := checkRedirect(mustGet(t, tt.to), via) + err := checkRedirect(tt.insecure)(mustGet(t, tt.to), via) if !tt.wantErr { require.NoError(t, err) return @@ -324,7 +327,7 @@ func TestCheckRedirect(t *testing.T) { func TestCheckRedirectFirstRequest(t *testing.T) { t.Parallel() - require.NoError(t, checkRedirect(mustGet(t, "http://example.com"), nil)) + require.NoError(t, checkRedirect(false)(mustGet(t, "http://example.com"), nil)) } func TestCheckRedirectStopsAfterTenHops(t *testing.T) { @@ -334,30 +337,63 @@ func TestCheckRedirectStopsAfterTenHops(t *testing.T) { for i := range via { via[i] = mustGet(t, "https://example.com") } - require.Error(t, checkRedirect(mustGet(t, "https://example.com"), via)) + require.Error(t, checkRedirect(false)(mustGet(t, "https://example.com"), via)) } -// The downgrade is refused even with --insecure, which here also makes the -// client accept the test server's self-signed certificate. -func TestBuildHTTPClientRefusesDowngradeWithInsecure(t *testing.T) { - t.Parallel() +// downgradeServers returns a TLS server redirecting to a plaintext one, and a +// flag reporting whether the plaintext one was ever reached. +func downgradeServers(t *testing.T) (*httptest.Server, *atomic.Bool) { + t.Helper() + var plainReached atomic.Bool plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + plainReached.Store(true) w.WriteHeader(http.StatusOK) })) - defer plain.Close() + t.Cleanup(plain.Close) secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, plain.URL+"/Taskfile.yml", http.StatusFound) })) - defer secure.Close() + t.Cleanup(secure.Close) - client, err := buildHTTPClient(true, "", "", "") + return secure, &plainReached +} + +func TestBuildHTTPClientRefusesDowngrade(t *testing.T) { + t.Parallel() + + secure, plainReached := downgradeServers(t) + + // The test server's certificate has to be trusted explicitly, so that the + // refusal is the redirect and not the handshake. + caCert := filepath.Join(t.TempDir(), "ca.crt") + require.NoError(t, os.WriteFile(caCert, pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", Bytes: secure.Certificate().Raw, + }), 0o600)) + + client, err := buildHTTPClient(false, caCert, "", "") require.NoError(t, err) _, err = client.Do(mustGet(t, secure.URL+"/Taskfile.yml")) //nolint:bodyclose // the request never completes var notSecure *errors.TaskfileNotSecureError require.ErrorAs(t, err, ¬Secure) + assert.False(t, plainReached.Load(), "the plaintext server must never be contacted") +} + +func TestBuildHTTPClientFollowsDowngradeWithInsecure(t *testing.T) { + t.Parallel() + + secure, plainReached := downgradeServers(t) + + client, err := buildHTTPClient(true, "", "", "") + require.NoError(t, err) + + resp, err := client.Do(mustGet(t, secure.URL+"/Taskfile.yml")) + require.NoError(t, err) + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.True(t, plainReached.Load()) } func mustGet(t *testing.T, rawURL string) *http.Request { diff --git a/website/src/next/docs/remote-taskfiles.md b/website/src/next/docs/remote-taskfiles.md index 4899a3f9..a0ab10b5 100644 --- a/website/src/next/docs/remote-taskfiles.md +++ b/website/src/next/docs/remote-taskfiles.md @@ -260,9 +260,9 @@ Taskfile that is downloaded via an unencrypted connection. Sources that are not protected by TLS are vulnerable to man-in-the-middle attacks and should be avoided unless you know what you are doing. -A server answering an `https` URL with a redirect to `http` is refused, even -with `--insecure`. Requesting an `http` entrypoint is your decision; being sent -to one is the server's. +A server answering an `https` URL with a redirect to `http` is refused too, +unless you pass `--insecure` — the flag covers the whole download, not just the +URL you wrote. #### Custom Certificates