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

@@ -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
 

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),
 		)
 	}

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(

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, &notSecure)
+	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 {

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