diff --git a/CHANGELOG.md b/CHANGELOG.md index 413c8f7f12..05cafdfb43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +### 🐛 Fixes + +- Fixed an `https` remote Taskfile being downloaded over an unencrypted + connection when the server redirects to `http`. Such a redirect now requires + `--insecure`, like an `http` entrypoint (by @vmaerten). + ### 📦 Package API - Bumped the minimum Go version to 1.26. Task follows Go's two-latest support diff --git a/errors/errors_taskfile.go b/errors/errors_taskfile.go index 3b2b3795f8..725b865c0e 100644 --- a/errors/errors_taskfile.go +++ b/errors/errors_taskfile.go @@ -102,9 +102,18 @@ func (err *TaskfileNotTrustedError) Code() int { // remote Taskfile over an insecure connection. type TaskfileNotSecureError struct { URI string + // Redirect reports that the insecure URI was reached through a redirect + // rather than requested, in which case --insecure does not allow it. + Redirect bool } func (err *TaskfileNotSecureError) Error() string { + if err.Redirect { + return fmt.Sprintf( + `task: Taskfile %q was redirected to over an insecure connection. You can override this by using the --insecure flag`, + filepath.ToSlash(err.URI), + ) + } return fmt.Sprintf( `task: Taskfile %q cannot be downloaded 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 e8cbecba2d..d96cf7e8d2 100644 --- a/taskfile/node_http.go +++ b/taskfile/node_http.go @@ -32,9 +32,12 @@ func buildHTTPClient(insecure bool, caCert, cert, certKey string) (*http.Client, return nil, fmt.Errorf("both --cert and --cert-key must be provided together") } - // If no TLS customization is needed, return the default client + // If no TLS customization is needed, copy the default client rather than + // hand it out: setting CheckRedirect on it would apply process-wide. if !insecure && caCert == "" && cert == "" { - return http.DefaultClient, nil + client := *http.DefaultClient + client.CheckRedirect = checkRedirect(insecure) + return &client, nil } tlsConfig := &tls.Config{ @@ -67,9 +70,31 @@ func buildHTTPClient(insecure bool, caCert, cert, certKey string) (*http.Client, Transport: &http.Transport{ TLSClientConfig: tlsConfig, }, + CheckRedirect: checkRedirect(insecure), }, nil } +// 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 + } +} + func NewHTTPNode( entrypoint string, dir string, @@ -120,7 +145,7 @@ func (node *HTTPNode) ReadContext(ctx context.Context) ([]byte, error) { if ctx.Err() != nil { return nil, err } - return nil, errors.TaskfileFetchFailedError{URI: node.Location()} + return nil, taskfileFetchError(err, node.Location()) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { diff --git a/taskfile/node_http_test.go b/taskfile/node_http_test.go index 359ec798bf..f1e4c553bb 100644 --- a/taskfile/node_http_test.go +++ b/taskfile/node_http_test.go @@ -9,13 +9,17 @@ import ( "encoding/pem" "math/big" "net/http" + "net/http/httptest" "os" "path/filepath" + "sync/atomic" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/go-task/task/v3/errors" ) func TestHTTPNode_CacheKey(t *testing.T) { @@ -62,10 +66,14 @@ func TestHTTPNode_CacheKey(t *testing.T) { func TestBuildHTTPClient_Default(t *testing.T) { t.Parallel() - // When no TLS customization is needed, should return http.DefaultClient + // When no TLS customization is needed, should copy http.DefaultClient client, err := buildHTTPClient(false, "", "", "") require.NoError(t, err) - assert.Equal(t, http.DefaultClient, client) + assert.NotSame(t, http.DefaultClient, client) + assert.Equal(t, http.DefaultClient.Transport, client.Transport) + assert.NotNil(t, client.CheckRedirect) + // The shared client must keep following redirects as before. + assert.Nil(t, http.DefaultClient.CheckRedirect) } func TestBuildHTTPClient_Insecure(t *testing.T) { @@ -282,3 +290,115 @@ func generateTestCACert(t *testing.T) []byte { Bytes: certDER, }) } + +func TestCheckRedirect(t *testing.T) { + t.Parallel() + + tests := []struct { + 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"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + via := []*http.Request{mustGet(t, tt.from)} + err := checkRedirect(tt.insecure)(mustGet(t, tt.to), via) + if !tt.wantErr { + require.NoError(t, err) + return + } + var notSecure *errors.TaskfileNotSecureError + require.ErrorAs(t, err, ¬Secure) + }) + } +} + +func TestCheckRedirectFirstRequest(t *testing.T) { + t.Parallel() + + require.NoError(t, checkRedirect(false)(mustGet(t, "http://example.com"), nil)) +} + +func TestCheckRedirectStopsAfterTenHops(t *testing.T) { + t.Parallel() + + via := make([]*http.Request, 10) + for i := range via { + via[i] = mustGet(t, "https://example.com") + } + require.Error(t, checkRedirect(false)(mustGet(t, "https://example.com"), via)) +} + +// 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) + })) + 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) + })) + t.Cleanup(secure.Close) + + 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 { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, rawURL, nil) + require.NoError(t, err) + return req +} diff --git a/taskfile/taskfile.go b/taskfile/taskfile.go index 4251a20528..54d089458c 100644 --- a/taskfile/taskfile.go +++ b/taskfile/taskfile.go @@ -51,7 +51,7 @@ func RemoteExists(ctx context.Context, u url.URL, client *http.Client) (*url.URL if ctx.Err() != nil { return nil, fmt.Errorf("checking remote file: %w", ctx.Err()) } - return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()} + return nil, taskfileFetchError(err, u.Redacted()) } defer resp.Body.Close() @@ -80,7 +80,7 @@ func RemoteExists(ctx context.Context, u url.URL, client *http.Client) (*url.URL // Try the alternative URL resp, err = client.Do(req) if err != nil { - return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()} + return nil, taskfileFetchError(err, u.Redacted()) } defer resp.Body.Close() @@ -92,3 +92,12 @@ func RemoteExists(ctx context.Context, u url.URL, client *http.Client) (*url.URL return nil, errors.TaskfileNotFoundError{URI: u.Redacted(), Walk: false} } + +// taskfileFetchError preserves redirect-policy errors wrapped by http.Client. +// Other transport errors remain generic download failures. +func taskfileFetchError(err error, uri string) error { + if notSecure, ok := errors.AsType[*errors.TaskfileNotSecureError](err); ok { + return notSecure + } + return errors.TaskfileFetchFailedError{URI: uri} +} diff --git a/website/src/next/docs/remote-taskfiles.md b/website/src/next/docs/remote-taskfiles.md index 4d54918e61..a0ab10b5c1 100644 --- a/website/src/next/docs/remote-taskfiles.md +++ b/website/src/next/docs/remote-taskfiles.md @@ -260,6 +260,10 @@ 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 too, +unless you pass `--insecure` — the flag covers the whole download, not just the +URL you wrote. + #### Custom Certificates If your remote Taskfiles are hosted on a server that uses a custom CA