diff --git a/CHANGELOG.md b/CHANGELOG.md index 413c8f7f..e1b0778f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +### 🐛 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). + ### 📦 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 3b2b3795..fa7fab61 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. Point the URL at the final location instead`, + 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 e8cbecba..ce79fadb 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 + return &client, nil } tlsConfig := &tls.Config{ @@ -67,9 +70,26 @@ func buildHTTPClient(insecure bool, caCert, cert, certKey string) (*http.Client, Transport: &http.Transport{ TLSClientConfig: tlsConfig, }, + CheckRedirect: checkRedirect, }, 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 { + 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( entrypoint string, dir string, @@ -120,6 +140,9 @@ func (node *HTTPNode) ReadContext(ctx context.Context) ([]byte, error) { if ctx.Err() != nil { return nil, err } + if notSecure, ok := errors.AsType[*errors.TaskfileNotSecureError](err); ok { + return nil, notSecure + } return nil, errors.TaskfileFetchFailedError{URI: node.Location()} } defer resp.Body.Close() diff --git a/taskfile/node_http_test.go b/taskfile/node_http_test.go index 359ec798..b49346f4 100644 --- a/taskfile/node_http_test.go +++ b/taskfile/node_http_test.go @@ -9,6 +9,7 @@ import ( "encoding/pem" "math/big" "net/http" + "net/http/httptest" "os" "path/filepath" "testing" @@ -16,6 +17,8 @@ import ( "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 +65,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 +289,80 @@ func generateTestCACert(t *testing.T) []byte { Bytes: certDER, }) } + +func TestCheckRedirect(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + from string + to string + 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 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(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(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(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() + + plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer 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() + + client, err := buildHTTPClient(true, "", "", "") + 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) +} + +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 4251a205..5a8555d7 100644 --- a/taskfile/taskfile.go +++ b/taskfile/taskfile.go @@ -51,6 +51,9 @@ 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()) } + if notSecure, ok := errors.AsType[*errors.TaskfileNotSecureError](err); ok { + return nil, notSecure + } return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()} } defer resp.Body.Close() @@ -80,6 +83,9 @@ 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 { + if notSecure, ok := errors.AsType[*errors.TaskfileNotSecureError](err); ok { + return nil, notSecure + } return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()} } defer resp.Body.Close() diff --git a/website/src/next/docs/remote-taskfiles.md b/website/src/next/docs/remote-taskfiles.md index 4d54918e..4899a3f9 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, even +with `--insecure`. Requesting an `http` entrypoint is your decision; being sent +to one is the server's. + #### Custom Certificates If your remote Taskfiles are hosted on a server that uses a custom CA