diff --git a/cache/cache.go b/cache/cache.go index c5534c8e..41dccdc6 100644 --- a/cache/cache.go +++ b/cache/cache.go @@ -9,6 +9,7 @@ import ( "time" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/sources" "github.com/cashapp/hermit/ui" "github.com/cashapp/hermit/util" ) @@ -25,8 +26,12 @@ type Cache struct { // BasePath returns the subfolder in the cache path for the given file func BasePath(checksum, uri string) string { - hash := util.Hash(uri, checksum) - return filepath.Join(hash[:2], hash+"-"+filepath.Base(uri)) + return basePath(checksum, sources.NewSourceURI(uri)) +} + +func basePath(checksum string, uri sources.SourceURI) string { + hash := util.Hash(uri.Get(), checksum) + return filepath.Join(hash[:2], hash+"-"+filepath.Base(uri.Get())) } // Open or create a Cache at the given directory, using the given http client. @@ -65,13 +70,13 @@ func (c *Cache) Root() string { // Mkdir makes a directory for the given URI. func (c *Cache) Mkdir(uri string) (string, error) { - path := c.Path("", uri) + path := c.path("", sources.NewSourceURI(uri)) return path, os.MkdirAll(path, os.ModePerm) //nolint:gosec } // Create a new, empty, cache entry. func (c *Cache) Create(checksum, uri string) (*os.File, error) { - path := c.Path(checksum, uri) + path := c.path(checksum, sources.NewSourceURI(uri)) dir := filepath.Dir(path) err := os.MkdirAll(dir, os.ModePerm) //nolint:gosec if err != nil { @@ -82,7 +87,7 @@ func (c *Cache) Create(checksum, uri string) (*os.File, error) { // OpenLocal opens a local cached copy of "uri", or errors. func (c *Cache) OpenLocal(checksum, uri string) (*os.File, error) { - source, err := c.GetSource(c.httpClient, uri) + source, err := c.GetSource(c.httpClient, sources.NewSourceURI(uri)) if err != nil { return nil, errors.WithStack(err) } @@ -93,10 +98,11 @@ func (c *Cache) OpenLocal(checksum, uri string) (*os.File, error) { // // If checksum is present it must be the SHA256 hash of the downloaded artifact. func (c *Cache) Open(b *ui.Task, checksum, uri string, mirrors ...string) (*os.File, error) { - cachePath := c.Path(checksum, uri) + source := sources.NewSourceURI(uri) + cachePath := c.path(checksum, source) _, err := os.Stat(cachePath) if err == nil { - b.Tracef("returning cached path %s for %s", cachePath, uri) + b.Tracef("returning cached path %s for %s", cachePath, source) // TODO: Checksum it again? return os.Open(cachePath) } else if !os.IsNotExist(err) { @@ -104,7 +110,7 @@ func (c *Cache) Open(b *ui.Task, checksum, uri string, mirrors ...string) (*os.F } // No local cached copy, download it. - path, _, _, err := c.Download(b, checksum, uri, mirrors...) + path, _, _, err := c.download(b, checksum, source, wrapSources(mirrors)) if err != nil { return nil, err } @@ -115,13 +121,17 @@ func (c *Cache) Open(b *ui.Task, checksum, uri string, mirrors ...string) (*os.F // // If checksum is present it must be the SHA256 hash of the downloaded artifact. func (c *Cache) Download(b *ui.Task, checksum, uri string, mirrors ...string) (path string, etag string, actualChecksum string, err error) { - uris := append([]string{uri}, mirrors...) + return c.download(b, checksum, sources.NewSourceURI(uri), wrapSources(mirrors)) +} + +func (c *Cache) download(b *ui.Task, checksum string, uri sources.SourceURI, mirrors []sources.SourceURI) (path string, etag string, actualChecksum string, err error) { + uris := append([]sources.SourceURI{uri}, mirrors...) var lastError error attempts := 3 for attempt := 1; attempt <= attempts; attempt++ { - for _, uri := range uris { - defer ui.LogElapsed(b, "Download %s", uri)() - source, err := c.GetSource(c.httpClient, uri) + for _, sourceURI := range uris { + defer ui.LogElapsed(b, "Download %s", sourceURI)() + source, err := c.GetSource(c.httpClient, sourceURI) if err != nil { return "", "", "", errors.WithStack(err) } @@ -130,12 +140,12 @@ func (c *Cache) Download(b *ui.Task, checksum, uri string, mirrors ...string) (p return path, etag, actualChecksum, nil } lastError = err - b.Debugf("%s: %s", uri, err) + b.Debugf("%s: %s", sourceURI, err) } if lastError == nil { - return "", "", "", errors.Errorf("failed to download from any of %s", strings.Join(uris, ", ")) + return "", "", "", errors.Errorf("failed to download from any of %s", joinSources(uris)) } - msg := fmt.Sprintf("Failed to download any of %s on attempt %d/%d: %s", strings.Join(uris, ", "), attempt, attempts, lastError) + msg := fmt.Sprintf("Failed to download any of %s on attempt %d/%d: %s", joinSources(uris), attempt, attempts, lastError) if attempt < attempts { msg = "Retrying. " + msg } @@ -153,14 +163,18 @@ func (c *Cache) Download(b *ui.Task, checksum, uri string, mirrors ...string) (p // ETag fetches the etag from given URI if available. // Otherwise an empty string is returned func (c *Cache) ETag(b *ui.Task, uri string, mirrors ...string) (etag string, err error) { - for _, uri := range append([]string{uri}, mirrors...) { - source, err := c.GetSource(c.fastFailHTTPClient, uri) + return c.etag(b, sources.NewSourceURI(uri), wrapSources(mirrors)) +} + +func (c *Cache) etag(b *ui.Task, uri sources.SourceURI, mirrors []sources.SourceURI) (etag string, err error) { + for _, sourceURI := range append([]sources.SourceURI{uri}, mirrors...) { + source, err := c.GetSource(c.fastFailHTTPClient, sourceURI) if err != nil { return "", errors.WithStack(err) } result, err := source.ETag(b) if err != nil { - b.Debugf("%s failed: %s", uri, err) + b.Debugf("%s failed: %s", sourceURI, err) continue } return result, nil @@ -170,14 +184,15 @@ func (c *Cache) ETag(b *ui.Task, uri string, mirrors ...string) (etag string, er // IsCached returns true if the URI is cached. func (c *Cache) IsCached(checksum, uri string) bool { - _, err := os.Stat(c.Path(checksum, uri)) + _, err := os.Stat(c.path(checksum, sources.NewSourceURI(uri))) return err == nil } // Evict a file from the cache. func (c *Cache) Evict(b *ui.Task, checksum, uri string) error { - b.SubTask("remove").Debugf("rm -rf %s", c.Path(checksum, uri)) - err := os.RemoveAll(c.Path(checksum, uri)) + path := c.path(checksum, sources.NewSourceURI(uri)) + b.SubTask("remove").Debugf("rm -rf %s", path) + err := os.RemoveAll(path) if err != nil && !os.IsNotExist(err) { return errors.WithStack(err) } @@ -191,19 +206,39 @@ func (c *Cache) Clean() error { // Path to cached object. func (c *Cache) Path(checksum, uri string) string { - base := BasePath(checksum, uri) + return c.path(checksum, sources.NewSourceURI(uri)) +} + +func (c *Cache) path(checksum string, uri sources.SourceURI) string { + base := basePath(checksum, uri) return filepath.Join(c.root, base) } +func wrapSources(raw []string) []sources.SourceURI { + wrapped := make([]sources.SourceURI, len(raw)) + for i, uri := range raw { + wrapped[i] = sources.NewSourceURI(uri) + } + return wrapped +} + +func joinSources(uris []sources.SourceURI) string { + redacted := make([]string, len(uris)) + for i, uri := range uris { + redacted[i] = uri.String() + } + return strings.Join(redacted, ", ") +} + // UnavailableError returns 101 for the exit code. type UnavailableError struct { - URI string + URI sources.SourceURI Err error } // Error returns the error string for the unavailable error func (e *UnavailableError) Error() string { - msg := e.URI + " is unavailable" + msg := e.URI.String() + " is unavailable" if e.Err != nil { return fmt.Sprintf("%s: %v", msg, e.Err) } diff --git a/cache/cachew.go b/cache/cachew.go index d88812f6..fb13f4ed 100644 --- a/cache/cachew.go +++ b/cache/cachew.go @@ -3,13 +3,15 @@ package cache import ( "net/http" "net/url" + + "github.com/cashapp/hermit/sources" ) // CachewSourceSelector wraps another PackageSourceSelector to redirect HTTP/HTTPS // downloads through a Cachew proxy server https://github.com/block/cachew. func CachewSourceSelector(getSource PackageSourceSelector, cachewURL string) PackageSourceSelector { - return func(client *http.Client, uri string) (PackageSource, error) { - u, err := url.Parse(uri) + return func(client *http.Client, uri sources.SourceURI) (PackageSource, error) { + u, err := url.Parse(uri.Get()) if err != nil { return getSource(client, uri) } @@ -25,6 +27,6 @@ func CachewSourceSelector(getSource PackageSourceSelector, cachewURL string) Pac rewrittenURI += "?" + u.RawQuery } - return HTTPSource(client, rewrittenURI), nil + return HTTPSource(client, sources.NewSourceURI(rewrittenURI)), nil } } diff --git a/cache/cachew_test.go b/cache/cachew_test.go index 6421a6f8..54b14229 100644 --- a/cache/cachew_test.go +++ b/cache/cachew_test.go @@ -3,11 +3,13 @@ package cache import ( "net/http" "testing" + + "github.com/cashapp/hermit/sources" ) func TestCachewSourceSelector(t *testing.T) { // Mock base selector that always returns a file source - baseSelector := func(client *http.Client, uri string) (PackageSource, error) { + baseSelector := func(client *http.Client, uri sources.SourceURI) (PackageSource, error) { return &fileSource{path: "/tmp/test"}, nil } @@ -54,7 +56,7 @@ func TestCachewSourceSelector(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - source, err := selector(nil, tt.inputURI) + source, err := selector(nil, sources.NewSourceURI(tt.inputURI)) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -64,7 +66,7 @@ func TestCachewSourceSelector(t *testing.T) { if !ok { t.Fatalf("expected httpSource, got %T", source) } - if httpSrc.url != tt.expectedURI { + if httpSrc.url.Get() != tt.expectedURI { t.Errorf("expected URL %s, got %s", tt.expectedURI, httpSrc.url) } } else { @@ -82,14 +84,14 @@ func TestCachewSourceSelector(t *testing.T) { } func TestCachewSourceSelectorInvalidURL(t *testing.T) { - baseSelector := func(client *http.Client, uri string) (PackageSource, error) { + baseSelector := func(client *http.Client, uri sources.SourceURI) (PackageSource, error) { return &fileSource{path: "/tmp/fallback"}, nil } selector := CachewSourceSelector(baseSelector, "https://cachew.example.com") // Test invalid URL - should fall back to base selector - source, err := selector(nil, "://invalid-url") + source, err := selector(nil, sources.NewSourceURI("://invalid-url")) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cache/git.go b/cache/git.go index 9344cf7c..bb24764a 100644 --- a/cache/git.go +++ b/cache/git.go @@ -6,21 +6,23 @@ import ( "strings" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/internal/redact" + "github.com/cashapp/hermit/sources" "github.com/cashapp/hermit/ui" "github.com/cashapp/hermit/util" ) type gitSource struct { - URL string + URL sources.SourceURI } func (s *gitSource) OpenLocal(c *Cache, checksum string) (*os.File, error) { - f, err := os.Open(c.Path(checksum, s.URL)) + f, err := os.Open(c.path(checksum, s.URL)) return f, errors.WithStack(err) } func (s *gitSource) Download(b *ui.Task, cache *Cache, checksum string) (string, string, string, error) { - base := BasePath(checksum, s.URL) + base := basePath(checksum, s.URL) checkoutDir := filepath.Join(cache.root, base) repo, tag, err := parseGitURL(s.URL) if err != nil { @@ -30,8 +32,8 @@ func (s *gitSource) Download(b *ui.Task, cache *Cache, checksum string) (string, if tag != "" { args = append(args, "--branch="+tag) } - args = append(args, "--", repo, checkoutDir) - err = util.RunSystemInDir(b, cache.root, args...) + args = append(args, "--") + err = util.RunSystemInDirWithSource(b, cache.root, args, repo, checkoutDir) if err != nil { return "", "", "", errors.WithStack(err) } @@ -53,14 +55,14 @@ func (s *gitSource) ETag(b *ui.Task) (etag string, err error) { if tag == "" { tag = "HEAD" } - bts, err := util.CaptureSystem(b, util.GitArgs("ls-remote", "--", repo, tag)...) + bts, err := util.CaptureSystemWithSource(b, util.GitArgs("ls-remote", "--"), repo, tag) if err != nil { - return "", errors.Wrap(err, s.URL) + return "", errors.Wrap(err, s.URL.String()) } str := string(bts) parts := strings.Split(str, "\t") if len(parts) != 2 { - return "", errors.Errorf("invalid HEAD: %s", str) + return "", errors.Errorf("invalid HEAD: %s", redact.Credentials(str)) } return parts[0], nil @@ -74,24 +76,24 @@ func (s *gitSource) Validate() error { if tag == "" { tag = "HEAD" } - args := util.GitArgs("ls-remote", "--", repo, tag) - cmd, err := util.SystemCommand(args...) + args := util.GitArgs("ls-remote", "--") + cmd, err := util.SystemCommandWithSource(args, repo, tag) if err != nil { return errors.WithStack(err) } out, err := cmd.CombinedOutput() if err != nil { - return errors.Wrapf(err, "error getting remote HEAD: %s", string(out)) + return errors.Wrapf(err, "error getting remote HEAD: %s", redact.Credentials(string(out))) } return nil } -func parseGitURL(source string) (repo, tag string, err error) { - parts := strings.SplitN(source, "#", 2) - repo = parts[0] +func parseGitURL(source sources.SourceURI) (repo sources.SourceURI, tag string, err error) { + parts := strings.SplitN(source.Get(), "#", 2) + repo = sources.NewSourceURI(parts[0]) if err := util.ValidateGitURL(repo); err != nil { - return "", "", errors.WithStack(err) + return sources.SourceURI{}, "", errors.WithStack(err) } if len(parts) > 1 { diff --git a/cache/github.go b/cache/github.go index 40f48809..6762a459 100644 --- a/cache/github.go +++ b/cache/github.go @@ -10,6 +10,7 @@ import ( "github.com/cashapp/hermit/errors" "github.com/cashapp/hermit/github" + "github.com/cashapp/hermit/sources" "github.com/cashapp/hermit/ui" ) @@ -18,7 +19,7 @@ var githubRe = regexp.MustCompile(`^https\://github\.com/([^/]+)/([^/]+)/release // GitHubSourceSelector can download private release assets from GitHub using an authenticated GitHub client. func GitHubSourceSelector(getSource PackageSourceSelector, ghclient *github.Client, match github.RepoMatcher) PackageSourceSelector { - return func(client *http.Client, uri string) (PackageSource, error) { + return func(client *http.Client, uri sources.SourceURI) (PackageSource, error) { info, ok := getGitHubReleaseInfo(uri) if !ok || match == nil || !match(info.owner, info.repo) { return getSource(client, uri) @@ -30,11 +31,11 @@ func GitHubSourceSelector(getSource PackageSourceSelector, ghclient *github.Clie type githubReleaseSource struct { info *githubReleaseInfo ghclient *github.Client - url string + url sources.SourceURI } func (g *githubReleaseSource) OpenLocal(c *Cache, checksum string) (*os.File, error) { - f, err := os.Open(c.Path(checksum, g.url)) + f, err := os.Open(c.path(checksum, g.url)) return f, errors.WithStack(err) } @@ -44,7 +45,7 @@ func (g *githubReleaseSource) Download(b *ui.Task, c *Cache, checksum string) (p return "", "", "", err } defer response.Body.Close() - cachePath := c.Path(checksum, g.url) + cachePath := c.path(checksum, g.url) return downloadHTTP(b, response, checksum, g.url, cachePath) } @@ -97,9 +98,9 @@ type githubReleaseInfo struct { owner, repo, tag, asset string } -func getGitHubReleaseInfo(uri string) (*githubReleaseInfo, bool) { +func getGitHubReleaseInfo(uri sources.SourceURI) (*githubReleaseInfo, bool) { g := &githubReleaseInfo{} - m := githubRe.FindStringSubmatch(uri) + m := githubRe.FindStringSubmatch(uri.Get()) if len(m) != 5 { return nil, false } diff --git a/cache/github_test.go b/cache/github_test.go index 79bf4c4d..cc712edd 100644 --- a/cache/github_test.go +++ b/cache/github_test.go @@ -4,12 +4,13 @@ import ( "testing" "github.com/alecthomas/assert/v2" + "github.com/cashapp/hermit/sources" ) func TestGetGitHubReleaseInfoRequiresGitHubHost(t *testing.T) { - _, ok := getGitHubReleaseInfo("https://github.com/owner/repo/releases/download/v1.0.0/tool.tar.gz") + _, ok := getGitHubReleaseInfo(sources.NewSourceURI("https://github.com/owner/repo/releases/download/v1.0.0/tool.tar.gz")) assert.True(t, ok) - _, ok = getGitHubReleaseInfo("https://githubXcom/owner/repo/releases/download/v1.0.0/tool.tar.gz") + _, ok = getGitHubReleaseInfo(sources.NewSourceURI("https://githubXcom/owner/repo/releases/download/v1.0.0/tool.tar.gz")) assert.False(t, ok) } diff --git a/cache/http.go b/cache/http.go index 6c121289..2de9f416 100644 --- a/cache/http.go +++ b/cache/http.go @@ -11,29 +11,30 @@ import ( "path/filepath" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/sources" "github.com/cashapp/hermit/ui" ) type httpSource struct { client *http.Client - url string + url sources.SourceURI } // HTTPSource is a PackageSource for a HTTP URL. -func HTTPSource(client *http.Client, url string) PackageSource { +func HTTPSource(client *http.Client, url sources.SourceURI) PackageSource { return &httpSource{client, url} } func (s *httpSource) OpenLocal(c *Cache, checksum string) (*os.File, error) { - f, err := os.Open(c.Path(checksum, s.url)) + f, err := os.Open(c.path(checksum, s.url)) return f, errors.WithStack(err) } func (s *httpSource) Download(b *ui.Task, cache *Cache, checksum string) (path string, etag string, actualChecksum string, err error) { - cachePath := cache.Path(checksum, s.url) + cachePath := cache.path(checksum, s.url) b.Tracef("cachePath %v checksum %v url %v \n", cachePath, checksum, s.url) ctx := context.Background() - req, err := http.NewRequestWithContext(ctx, "GET", s.url, &bytes.Reader{}) + req, err := http.NewRequestWithContext(ctx, "GET", s.url.Get(), &bytes.Reader{}) if err != nil { return "", "", "", errors.Wrap(err, "could not fetch") } @@ -47,13 +48,13 @@ func (s *httpSource) Download(b *ui.Task, cache *Cache, checksum string) (path s func (s *httpSource) ETag(b *ui.Task) (etag string, err error) { uri := s.url - req, err := http.NewRequestWithContext(context.Background(), http.MethodHead, uri, nil) + req, err := http.NewRequestWithContext(context.Background(), http.MethodHead, uri.Get(), nil) if err != nil { - return "", errors.Wrap(err, uri) + return "", errors.Wrap(err, uri.String()) } resp, err := s.client.Do(req) if err != nil { - return "", errors.Wrap(err, uri) + return "", errors.Wrap(err, uri.String()) } defer resp.Body.Close() // Normal HTTP error, log and try the next mirror. @@ -65,9 +66,9 @@ func (s *httpSource) ETag(b *ui.Task) (etag string, err error) { } func (s *httpSource) Validate() error { - req, err := http.NewRequestWithContext(context.Background(), http.MethodHead, s.url, nil) + req, err := http.NewRequestWithContext(context.Background(), http.MethodHead, s.url.Get(), nil) if err != nil { - return errors.Wrap(err, s.url) + return errors.Wrap(err, s.url.String()) } resp, err := s.client.Do(req) if err != nil { @@ -82,7 +83,7 @@ func (s *httpSource) Validate() error { return nil } -func downloadHTTP(b *ui.Task, response *http.Response, checksum string, uri string, cachePath string) (path string, etag string, returnChecksum string, err error) { +func downloadHTTP(b *ui.Task, response *http.Response, checksum string, uri sources.SourceURI, cachePath string) (path string, etag string, returnChecksum string, err error) { if response.StatusCode < 200 || response.StatusCode > 299 { return "", "", "", errors.Errorf("download failed: %s (%d), source url: %s", response.Status, response.StatusCode, uri) } diff --git a/cache/source.go b/cache/source.go index e4cc0fc5..06288307 100644 --- a/cache/source.go +++ b/cache/source.go @@ -9,13 +9,14 @@ import ( "github.com/cashapp/hermit/util" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/sources" "github.com/cashapp/hermit/ui" ) // PackageSourceSelector selects a PackageSource for a URI. // // If not provided to the Cache, GetSource() will be used. -type PackageSourceSelector func(client *http.Client, uri string) (PackageSource, error) +type PackageSourceSelector func(client *http.Client, uri sources.SourceURI) (PackageSource, error) // PackageSource for a specific version / system of a package type PackageSource interface { @@ -27,12 +28,13 @@ type PackageSource interface { } // GetSource for the given uri, or an error if the uri can not be parsed as a source -func GetSource(client *http.Client, uri string) (PackageSource, error) { - if strings.HasSuffix(uri, ".git") || strings.Contains(uri, ".git#") { +func GetSource(client *http.Client, uri sources.SourceURI) (PackageSource, error) { + rawURI := uri.Get() + if strings.HasSuffix(rawURI, ".git") || strings.Contains(rawURI, ".git#") { return &gitSource{URL: uri}, nil } - u, err := url.Parse(uri) + u, err := url.Parse(rawURI) if err != nil { return nil, errors.WithStack(err) } diff --git a/cache/source_test.go b/cache/source_test.go index 010f3d62..928da972 100644 --- a/cache/source_test.go +++ b/cache/source_test.go @@ -7,16 +7,17 @@ import ( "testing" "github.com/alecthomas/assert/v2" + "github.com/cashapp/hermit/sources" ) func TestGitParseRepo(t *testing.T) { - repo, tag, err := parseGitURL("org-49461806@github.com:squareup/orc.git") + repo, tag, err := parseGitURL(sources.NewSourceURI("org-49461806@github.com:squareup/orc.git")) assert.NoError(t, err) - assert.Equal(t, "org-49461806@github.com:squareup/orc.git", repo) + assert.Equal(t, "org-49461806@github.com:squareup/orc.git", repo.Get()) assert.Equal(t, "", tag) - repo, tag, err = parseGitURL("org-49461806@github.com:squareup/orc.git#v1.2.3") + repo, tag, err = parseGitURL(sources.NewSourceURI("org-49461806@github.com:squareup/orc.git#v1.2.3")) assert.NoError(t, err) - assert.Equal(t, "org-49461806@github.com:squareup/orc.git", repo) + assert.Equal(t, "org-49461806@github.com:squareup/orc.git", repo.Get()) assert.Equal(t, "v1.2.3", tag) } @@ -37,7 +38,7 @@ func TestParseGitURLArgumentInjection(t *testing.T) { } for _, tt := range tests { - _, _, err := parseGitURL(tt.url) + _, _, err := parseGitURL(sources.NewSourceURI(tt.url)) if tt.expectError { assert.Error(t, err, "Should reject: "+tt.url) } else { @@ -52,7 +53,7 @@ func TestGitSourcePreventRCE(t *testing.T) { pwnedFile := filepath.Join(tmpDir, "pwned") maliciousURL := "--upload-pack=sh -c 'echo OWNED > " + pwnedFile + "' #file://" + tmpDir + "/.git" - src := &gitSource{URL: maliciousURL} + src := &gitSource{URL: sources.NewSourceURI(maliciousURL)} err := src.Validate() assert.Error(t, err) @@ -81,7 +82,7 @@ func TestGitSourceRCEAttempt(t *testing.T) { pwnedFile := filepath.Join(tmpDir, "pwned") payload := "--upload-pack=sh -c 'echo OWNED > " + pwnedFile + "' #file://" + repoDir + "/.git" - src := &gitSource{URL: payload} + src := &gitSource{URL: sources.NewSourceURI(payload)} err := src.Validate() assert.Error(t, err) @@ -106,9 +107,25 @@ func TestGitURLParsing(t *testing.T) { } for _, tt := range tests { - repo, tag, err := parseGitURL(tt.url) + repo, tag, err := parseGitURL(sources.NewSourceURI(tt.url)) assert.NoError(t, err) - assert.Equal(t, tt.repo, repo) + assert.Equal(t, tt.repo, repo.Get()) assert.Equal(t, tt.tag, tag) } } + +func TestParseGitURLPreservesCredentialRedaction(t *testing.T) { + repo, tag, err := parseGitURL(sources.NewSourceURI("https://user:secret@host/repo.git#main")) + assert.NoError(t, err) + assert.Equal(t, "https://user:secret@host/repo.git", repo.Get()) + assert.Equal(t, "https://user:****@host/repo.git", repo.String()) + assert.Equal(t, "main", tag) +} + +func TestUnavailableErrorRedactsSourceCredentials(t *testing.T) { + err := (&UnavailableError{ + URI: sources.NewSourceURI("https://user:secret@host/package.tar.gz"), + }).Error() + assert.NotContains(t, err, "secret") + assert.Contains(t, err, "https://user:****@host/package.tar.gz") +} diff --git a/env.go b/env.go index 10ee8d67..749aa300 100644 --- a/env.go +++ b/env.go @@ -1539,7 +1539,7 @@ func (e *Env) Update(l *ui.UI, force bool) error { } // Sources enabled in this environment. -func (e *Env) Sources(l *ui.UI) ([]string, error) { +func (e *Env) Sources(l *ui.UI) ([]sources.SourceURI, error) { sources, err := e.sources(l) if err != nil { return nil, errors.WithStack(err) diff --git a/github/url.go b/github/url.go index 7e5dfedd..c14d9ea1 100644 --- a/github/url.go +++ b/github/url.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/sources" ) // isGitHubHTTPSURL checks if a URL is a GitHub HTTPS URL and returns owner/repo if it is @@ -22,21 +23,21 @@ func isGitHubHTTPSURL(u *url.URL) (owner, repo string, ok bool) { } // isGitHubSSHURL checks if a URL is a GitHub SSH URL (git@github.com:owner/repo.git) -func isGitHubSSHURL(uri string) bool { - return strings.HasPrefix(uri, "git@github.com:") +func isGitHubSSHURL(uri sources.SourceURI) bool { + return strings.HasPrefix(uri.Get(), "git@github.com:") } // AuthenticatedURLRewriter rewrites GitHub URLs to include an auth token if they match the provided pattern -func AuthenticatedURLRewriter(token string, matcher RepoMatcher) func(uri string) (string, error) { - return func(repo string) (string, error) { +func AuthenticatedURLRewriter(token string, matcher RepoMatcher) sources.URLRewriter { + return func(repo sources.SourceURI) (sources.SourceURI, error) { // Pass through SSH URLs unchanged if isGitHubSSHURL(repo) { return repo, nil } - u, err := url.Parse(repo) + u, err := url.Parse(repo.Get()) if err != nil { - return "", errors.WithStack(err) + return sources.SourceURI{}, errors.Errorf("invalid GitHub source %q", repo) } owner, repoName, ok := isGitHubHTTPSURL(u) @@ -45,7 +46,7 @@ func AuthenticatedURLRewriter(token string, matcher RepoMatcher) func(uri string } if matcher(owner, repoName) { u.User = url.UserPassword("x-access-token", token) - return u.String(), nil + return sources.NewSourceURI(u.String()), nil } return repo, nil } diff --git a/integration/integration_test.go b/integration/integration_test.go index 9de8694a..c2512ab3 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -190,6 +190,56 @@ EOF `, expectations: exp{outputContains("remote helpers are not supported")}, }, + { + // Regression test for DX-26: GitHub token authentication embeds the + // token in the internal git URL, but it must never reach user output. + name: "GitHubTokenIsRedactedFromOutput", + script: ` + hermit init --no-git . + mkdir host-bin + cat > host-bin/git <<'EOF' +#!/bin/sh +printf 'git received: %s\n' "$*" >&2 +exit 1 +EOF + chmod +x host-bin/git + export PATH="$PWD/host-bin:$PATH" + + cat > bin/hermit.hcl <<'EOF' +env = {} +sources = ["https://github.com/owner/repo.git"] +github-token-auth { + match = ["*/*"] +} +EOF + export HERMIT_GITHUB_TOKEN=dx26-super-secret + . bin/activate-hermit + + hermit status > status.out 2>&1 + if hermit --debug search > search.out 2>&1; then + hermit-send "error: failing git source unexpectedly succeeded" + exit 1 + fi + + # Also cover errors raised before a git command is started. + sed 's/repo\.git/repo/' bin/hermit.hcl > bin/hermit.hcl.tmp + mv bin/hermit.hcl.tmp bin/hermit.hcl + if hermit status > unsupported.out 2>&1; then + hermit-send "error: unsupported source unexpectedly succeeded" + exit 1 + fi + + cat status.out search.out unsupported.out + for output in status.out search.out unsupported.out; do + if grep -F 'dx26-super-secret' "$output"; then + hermit-send "error: GitHub token leaked in $output" + exit 1 + fi + assert grep -F 'https://x-access-token:****@github.com/owner/repo' "$output" + done + `, + expectations: exp{outputContains("https://x-access-token:****@github.com/owner/repo")}, + }, { // Regression test for DX-29: internal tools must still resolve from a // user's custom PATH before a Hermit environment is activated. diff --git a/internal/redact/redact.go b/internal/redact/redact.go new file mode 100644 index 00000000..540a4a1e --- /dev/null +++ b/internal/redact/redact.go @@ -0,0 +1,30 @@ +// Package redact masks credentials in values that have crossed an output boundary. +package redact + +import ( + "regexp" + "strings" +) + +const placeholder = "****" + +var urlUserinfoRE = regexp.MustCompile(`(?i)([a-z][a-z0-9+.-]*://)([^/?#\s]+)@`) + +// Credentials masks URL credentials in value. +func Credentials(value string) string { + return urlUserinfoRE.ReplaceAllStringFunc(value, func(match string) string { + parts := urlUserinfoRE.FindStringSubmatch(match) + scheme, userinfo := parts[1], parts[2] + if i := strings.IndexByte(userinfo, ':'); i >= 0 { + return scheme + userinfo[:i] + ":" + placeholder + "@" + } + // A username-only HTTP URL is how tokens are commonly supplied. Other + // schemes commonly use non-secret usernames, such as ssh://git@host. + switch strings.ToLower(strings.TrimSuffix(scheme, "://")) { + case "http", "https": + return scheme + placeholder + "@" + default: + return match + } + }) +} diff --git a/manifest/autoversion/git_tags.go b/manifest/autoversion/git_tags.go index 5f28f627..b6b79e91 100644 --- a/manifest/autoversion/git_tags.go +++ b/manifest/autoversion/git_tags.go @@ -8,19 +8,20 @@ import ( "github.com/cashapp/hermit/errors" "github.com/cashapp/hermit/manifest" + "github.com/cashapp/hermit/sources" "github.com/cashapp/hermit/util" ) func gitTagsAutoVersion(autoVersion *manifest.AutoVersionBlock) (string, error) { + remoteURL := sources.NewSourceURI(autoVersion.GitTags) versionRe, err := regexp.Compile(autoVersion.VersionPattern) if err != nil { return "", errors.WithStack(err) } if len(versionRe.SubexpNames()) != 2 { - return "", errors.Errorf("%s: version pattern %s must have exactly one named capture group", autoVersion.GitTags, autoVersion.VersionPattern) + return "", errors.Errorf("%s: version pattern %s must have exactly one named capture group", remoteURL, autoVersion.VersionPattern) } - remoteURL := autoVersion.GitTags if err := util.ValidateGitURL(remoteURL); err != nil { return "", errors.WithStack(err) } @@ -30,8 +31,8 @@ func gitTagsAutoVersion(autoVersion *manifest.AutoVersionBlock) (string, error) // output format of refs is // TAB LF // source: https://git-scm.com/docs/git-ls-remote - args := util.GitArgs("ls-remote", "--tags", "--refs", "--", remoteURL) - cmd, err := util.SystemCommand(args...) + args := util.GitArgs("ls-remote", "--tags", "--refs", "--") + cmd, err := util.SystemCommandWithSource(args, remoteURL) if err != nil { return "", errors.WithStack(err) } diff --git a/manifest/loader_test.go b/manifest/loader_test.go index 45cb64cc..f2bf107f 100644 --- a/manifest/loader_test.go +++ b/manifest/loader_test.go @@ -14,7 +14,7 @@ func TestLoader(t *testing.T) { stateDir := t.TempDir() srcs := sources.New(stateDir, []sources.Source{ - sources.NewLocalSource("test://", os.DirFS("./testdata")), + sources.NewLocalSource(sources.NewSourceURI("test://"), os.DirFS("./testdata")), }) loader := NewLoader(srcs) assert.Equal(t, len(srcs.Sources()), 1) diff --git a/manifest/validate.go b/manifest/validate.go index 017ca7fb..8e7c7f62 100644 --- a/manifest/validate.go +++ b/manifest/validate.go @@ -5,13 +5,15 @@ import ( "github.com/cashapp/hermit/cache" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/sources" ) // ValidatePackageSource checks that a package source is accessible. func ValidatePackageSource(packageSource cache.PackageSourceSelector, httpClient *http.Client, url string) error { - source, err := packageSource(httpClient, url) + urlSource := sources.NewSourceURI(url) + source, err := packageSource(httpClient, urlSource) if err != nil { - return errors.Wrap(err, url) + return errors.Wrap(err, urlSource.String()) } return errors.Wrapf(source.Validate(), "invalid source") } diff --git a/sources/builtin.go b/sources/builtin.go index 5c31b01e..aca2b11a 100644 --- a/sources/builtin.go +++ b/sources/builtin.go @@ -20,8 +20,8 @@ func (s *BuiltInSource) Sync(_ *ui.UI, _ bool) (bool, error) { return true, nil } -func (s *BuiltInSource) URI() string { - return "builtin:///" +func (s *BuiltInSource) URI() SourceURI { + return NewSourceURI("builtin:///") } func (s *BuiltInSource) Bundle() fs.FS { diff --git a/sources/git.go b/sources/git.go index ef254c8d..d96ee516 100644 --- a/sources/git.go +++ b/sources/git.go @@ -11,17 +11,35 @@ import ( "github.com/cashapp/hermit/util" ) -// GitSource is a new Source based on a git repo +// CommandRunner abstracts the git operations used to synchronise a source. +type CommandRunner interface { + RunInDir(log *ui.Task, dir string, args ...string) error + CloneInDir(log *ui.Task, dir string, source SourceURI, dest string) error +} + +// RealCommandRunner runs git operations through Hermit's system command path. +type RealCommandRunner struct{} + +func (r *RealCommandRunner) RunInDir(log *ui.Task, dir string, args ...string) error { + return errors.WithStack(util.RunSystemInDir(log, dir, args...)) +} + +func (r *RealCommandRunner) CloneInDir(log *ui.Task, dir string, source SourceURI, dest string) error { + args := util.GitArgs("clone", "--depth=1", "--") + return errors.WithStack(util.RunSystemInDirWithSource(log, dir, args, source, dest)) +} + +// GitSource is a manifest source backed by a git repository. type GitSource struct { fs *uriFS sourceDir string path string - runner util.CommandRunner + runner CommandRunner } // NewGitSource returns a new GitSource -func NewGitSource(uri, sourceDir string, runner util.CommandRunner) *GitSource { - key := util.Hash(uri) +func NewGitSource(uri SourceURI, sourceDir string, runner CommandRunner) *GitSource { + key := util.Hash(uri.Get()) path := filepath.Join(sourceDir, key) return &GitSource{&uriFS{ uri: uri, @@ -31,7 +49,7 @@ func NewGitSource(uri, sourceDir string, runner util.CommandRunner) *GitSource { func (s *GitSource) Sync(p *ui.UI, force bool) (bool, error) { info, _ := os.Stat(s.path) - task := p.Task(s.fs.uri) + task := p.Task(s.fs.uri.String()) if info == nil || force || time.Since(info.ModTime()) >= SyncFrequency { err := s.ensureSourcesDirExists() if err != nil { @@ -54,7 +72,7 @@ func (s *GitSource) Sync(p *ui.UI, force bool) (bool, error) { return false, nil } -func (s *GitSource) URI() string { +func (s *GitSource) URI() SourceURI { return s.fs.uri } @@ -70,7 +88,7 @@ func (s *GitSource) ensureSourcesDirExists() error { } // Atomically clone git repo. -func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunner) (err error) { +func syncGit(b *ui.Task, dir string, source SourceURI, finalDest string, runner CommandRunner) (err error) { task := b.SubProgress("sync", 1) defer func() { task.Done() @@ -94,7 +112,7 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne return errors.WithStack(err) } defer os.RemoveAll(dest) - if err = runner.RunInDir(b, dest, util.GitArgs("clone", "--depth=1", "--", source, dest)...); err != nil { + if err = runner.CloneInDir(b, dest, source, dest); err != nil { return errors.WithStack(err) } _ = os.RemoveAll(finalDest) diff --git a/sources/git_test.go b/sources/git_test.go index 3ae5e745..601b4186 100644 --- a/sources/git_test.go +++ b/sources/git_test.go @@ -18,10 +18,14 @@ func (f *FailingGit) RunInDir(_ *ui.Task, _ string, _ ...string) error { return f.err } +func (f *FailingGit) CloneInDir(_ *ui.Task, _ string, _ sources.SourceURI, _ string) error { + return f.err +} + func TestGitDoesNotRemoveSourceAfterSyncFailure(t *testing.T) { git := &FailingGit{} sourceDir := t.TempDir() - source := sources.NewGitSource("git://test", sourceDir, git) + source := sources.NewGitSource(sources.NewSourceURI("git://test"), sourceDir, git) // Create the initial directory for sources by successfully syncing u, _ := ui.NewForTesting() diff --git a/sources/local.go b/sources/local.go index 82886c90..3bcd808b 100644 --- a/sources/local.go +++ b/sources/local.go @@ -6,13 +6,13 @@ import ( "github.com/cashapp/hermit/ui" ) -// LocalSource is a new Source based on a local filesystem +// LocalSource is a manifest source backed by a local filesystem. type LocalSource struct { fs *uriFS } // NewLocalSource returns a new LocalSource -func NewLocalSource(uri string, f fs.FS) *LocalSource { +func NewLocalSource(uri SourceURI, f fs.FS) *LocalSource { return &LocalSource{&uriFS{ uri: uri, FS: f, @@ -23,7 +23,7 @@ func (s *LocalSource) Sync(_ *ui.UI, _ bool) (bool, error) { return true, nil } -func (s *LocalSource) URI() string { +func (s *LocalSource) URI() SourceURI { return s.fs.uri } diff --git a/sources/memory.go b/sources/memory.go index f327ba12..8c2e4d24 100644 --- a/sources/memory.go +++ b/sources/memory.go @@ -9,25 +9,25 @@ import ( // MemSource is a new Source based on a name and content kept in memory type MemSource struct { - name string + name SourceURI content string } // NewMemSource returns a new MemSource func NewMemSource(name, content string) *MemSource { - return &MemSource{name, content} + return &MemSource{NewSourceURI(name), content} } func (s *MemSource) Sync(_ *ui.UI, _ bool) (bool, error) { return true, nil } -func (s *MemSource) URI() string { +func (s *MemSource) URI() SourceURI { return s.name } func (s *MemSource) Bundle() fs.FS { return vfs.InMemoryFS(map[string]string{ - s.name: s.content, + s.name.Get(): s.content, }) } diff --git a/sources/source.go b/sources/source.go new file mode 100644 index 00000000..8325f078 --- /dev/null +++ b/sources/source.go @@ -0,0 +1,24 @@ +package sources + +import "github.com/cashapp/hermit/internal/redact" + +// SourceURI is a source URI. Its String method is safe for output; raw access is +// deliberately explicit through Get. +type SourceURI struct { + value string +} + +// NewSourceURI wraps a raw source URI. +func NewSourceURI(value string) SourceURI { + return SourceURI{value: value} +} + +// Get returns the raw source URI for operations that require it. +func (s SourceURI) Get() string { + return s.value +} + +// String returns the source URI with credentials redacted. +func (s SourceURI) String() string { + return redact.Credentials(s.value) +} diff --git a/sources/source_test.go b/sources/source_test.go new file mode 100644 index 00000000..45f5f18a --- /dev/null +++ b/sources/source_test.go @@ -0,0 +1,32 @@ +package sources + +import ( + "testing" + + "github.com/alecthomas/assert/v2" +) + +func TestSourceURI(t *testing.T) { + for _, test := range []struct { + name string + input string + expected string + }{ + {"NoCredentials", "https://github.com/cashapp/hermit.git", "https://github.com/cashapp/hermit.git"}, + {"UserAndPassword", "https://x-access-token:ghp_secret@github.com/o/r.git", "https://x-access-token:****@github.com/o/r.git"}, + {"TokenOnly", "https://ghp_secret@github.com/o/r.git", "https://****@github.com/o/r.git"}, + {"EmptyPassword", "https://user:@github.com/o/r.git", "https://user:****@github.com/o/r.git"}, + {"UnescapedAtInPassword", "https://user:@secret@host/repo.git", "https://user:****@host/repo.git"}, + {"MultipleURLs", "https://a:b@host/x https://c:d@host/y", "https://a:****@host/x https://c:****@host/y"}, + {"NonHTTPPassword", "ssh://git:secret@github.com/o/r.git", "ssh://git:****@github.com/o/r.git"}, + {"SSHUsername", "ssh://git@github.com/o/r.git", "ssh://git@github.com/o/r.git"}, + {"SCPStyle", "git@github.com:cashapp/hermit.git", "git@github.com:cashapp/hermit.git"}, + {"AtInPath", "https://github.com/o/r@v1.git", "https://github.com/o/r@v1.git"}, + } { + t.Run(test.name, func(t *testing.T) { + source := NewSourceURI(test.input) + assert.Equal(t, test.input, source.Get()) + assert.Equal(t, test.expected, source.String()) + }) + } +} diff --git a/sources/sources.go b/sources/sources.go index 7cf6e553..16b1a319 100644 --- a/sources/sources.go +++ b/sources/sources.go @@ -17,13 +17,13 @@ import ( // SyncFrequency determines how frequently sources will be synced. const SyncFrequency = time.Hour * 24 -// Source is a single source for manifest files +// Source provides manifest files from one source. type Source interface { // Sync synchronises these sources from the possibly remote origin. // Returns true if the source was actually updated. Sync(p *ui.UI, force bool) (bool, error) // URI returns a URI for the source - URI() string + URI() SourceURI // Bundle returns a fs.FS for the manifests from this source Bundle() fs.FS } @@ -47,7 +47,7 @@ func (s *Sources) LocalDirs() []string { var out []string for _, source := range s.sources { if local, ok := source.(*LocalSource); ok { - dir := strings.TrimPrefix(local.fs.uri, "env:///") + dir := strings.TrimPrefix(local.fs.uri.Get(), "env:///") out = append(out, dir) } } @@ -86,23 +86,23 @@ func (s *Sources) Sync(p *ui.UI, force bool) error { } // URLRewriter is a function that can transform a source URI -type URLRewriter func(uri string) (string, error) +type URLRewriter func(source SourceURI) (SourceURI, error) -// ForURIs returns Source instances for given uri strings +// ForURIs constructs manifest sources for the given raw URI strings. func ForURIs(b *ui.UI, dir, env string, uris []string, rewriters ...URLRewriter) (*Sources, error) { sources := make([]Source, 0, len(uris)) for _, uri := range uris { // Apply each rewriter in sequence - transformedURI := uri + transformedSource := NewSourceURI(uri) for _, rewrite := range rewriters { - rewritten, err := rewrite(transformedURI) + rewritten, err := rewrite(transformedSource) if err != nil { return nil, errors.WithStack(err) } - transformedURI = rewritten + transformedSource = rewritten } - s, err := getSource(b, transformedURI, dir, env) + s, err := getSource(b, transformedSource, dir, env) if err != nil { return nil, errors.WithStack(err) } @@ -116,20 +116,20 @@ func ForURIs(b *ui.UI, dir, env string, uris []string, rewriters ...URLRewriter) }, nil } -func getSource(b *ui.UI, source, dir, env string) (Source, error) { - task := b.Task(source) +func getSource(b *ui.UI, source SourceURI, dir, env string) (Source, error) { + task := b.Task(source.String()) defer task.Done() - if strings.HasSuffix(source, ".git") { + if strings.HasSuffix(source.Get(), ".git") { if err := util.ValidateGitURL(source); err != nil { return nil, errors.WithStack(err) } - return NewGitSource(source, dir, &util.RealCommandRunner{}), nil + return NewGitSource(source, dir, &RealCommandRunner{}), nil } - uri, err := url.Parse(source) + uri, err := url.Parse(source.Get()) if err != nil { - return nil, errors.Wrap(err, "invalid source") + return nil, errors.Errorf("invalid source %q", source) } var ( // Directory of source, if any, to check for existence. @@ -139,7 +139,7 @@ func getSource(b *ui.UI, source, dir, env string) (Source, error) { switch uri.Scheme { case "env": if uri.Path == "" { - task.Warnf("%s does not contain a path", uri) + task.Warnf("%s does not contain a path", source) return nil, nil } cleanPath := filepath.Clean(strings.TrimLeft(uri.Path, "/\\")) @@ -151,7 +151,7 @@ func getSource(b *ui.UI, source, dir, env string) (Source, error) { case "file": if uri.Path == "" { - task.Warnf("%s does not contain a path", uri) + task.Warnf("%s does not contain a path", source) return nil, nil } checkDir = uri.Path @@ -171,8 +171,8 @@ func getSource(b *ui.UI, source, dir, env string) (Source, error) { } // Sources returns the source URIs -func (s *Sources) Sources() []string { - combined := []string{} +func (s *Sources) Sources() []SourceURI { + combined := []SourceURI{} for _, s := range s.sources { combined = append(combined, s.URI()) } @@ -190,11 +190,11 @@ func (s *Sources) Bundles() []fs.FS { // This exists to provide useful debugging information back to the user. type uriFS struct { - uri string + uri SourceURI fs.FS } func (u *uriFS) Stat(name string) (fs.FileInfo, error) { return fs.Stat(u.FS, name) } func (u *uriFS) ReadDir(name string) ([]fs.DirEntry, error) { return fs.ReadDir(u.FS, name) } func (u *uriFS) Glob(pattern string) ([]string, error) { return fs.Glob(u.FS, pattern) } -func (u *uriFS) String() string { return u.uri } +func (u *uriFS) String() string { return u.uri.String() } diff --git a/sources/sources_test.go b/sources/sources_test.go index 696527ef..a07bd3bd 100644 --- a/sources/sources_test.go +++ b/sources/sources_test.go @@ -1,4 +1,4 @@ -package sources +package sources_test import ( "os" @@ -8,6 +8,7 @@ import ( "github.com/alecthomas/assert/v2" "github.com/cashapp/hermit/errors" "github.com/cashapp/hermit/github" + "github.com/cashapp/hermit/sources" "github.com/cashapp/hermit/ui" ) @@ -24,12 +25,23 @@ func TestEnvSourceRejectsPathTraversal(t *testing.T) { } { t.Run(uri, func(t *testing.T) { ui, _ := ui.NewForTesting() - _, err := ForURIs(ui, filepath.Join(root, "state"), env, []string{uri}) + _, err := sources.ForURIs(ui, filepath.Join(root, "state"), env, []string{uri}) assert.Error(t, err) }) } } +func TestInvalidSourceDoesNotLeakCredentials(t *testing.T) { + l, _ := ui.NewForTesting() + _, err := sources.ForURIs(l, "testdir", "testenv", []string{ + "https://x-access-token:secret-token@github.com/owner/%zz", + }) + + assert.Error(t, err) + assert.NotContains(t, err.Error(), "secret-token") + assert.Contains(t, err.Error(), "https://x-access-token:****@github.com/owner/%zz") +} + func TestGitHubTokenRewriter(t *testing.T) { tests := []struct { name string @@ -81,14 +93,25 @@ func TestGitHubTokenRewriter(t *testing.T) { assert.NoError(t, err) rewriter := github.AuthenticatedURLRewriter(tt.token, matcher) - result, err := rewriter(tt.uri) + result, err := rewriter(sources.NewSourceURI(tt.uri)) assert.NoError(t, err) - assert.Equal(t, tt.want, result) + assert.Equal(t, tt.want, result.Get()) }) } } +func TestGitHubTokenRewriterErrorDoesNotLeakCredentials(t *testing.T) { + matcher, err := github.GlobRepoMatcher([]string{"*/*"}) + assert.NoError(t, err) + rewriter := github.AuthenticatedURLRewriter("unused", matcher) + + _, err = rewriter(sources.NewSourceURI("https://x-access-token:secret-token@github.com/owner/%zz")) + assert.Error(t, err) + assert.NotContains(t, err.Error(), "secret-token") + assert.Contains(t, err.Error(), "https://x-access-token:****@github.com/owner/%zz") +} + // TestForURIsIntegration tests the integration of ForURIs with rewriters func TestForURIsIntegration(t *testing.T) { l, _ := ui.NewForTesting() @@ -103,44 +126,45 @@ func TestForURIsIntegration(t *testing.T) { "https://github.com/other/repo2.git", "git@github.com:owner/repo3.git", } - sources, err := ForURIs(l, "testdir", "testenv", uris, rewriter) + sourceSet, err := sources.ForURIs(l, "testdir", "testenv", uris, rewriter) assert.NoError(t, err) - assert.Equal(t, len(uris), len(sources.sources)) + assert.Equal(t, len(uris), len(sourceSet.Sources())) // Verify the sources were created with appropriate URIs // First URI should be rewritten with token, others should remain unchanged - assert.Contains(t, sources.sources[0].URI(), "x-access-token:test-token@github.com") - assert.Equal(t, uris[1], sources.sources[1].URI()) - assert.Equal(t, uris[2], sources.sources[2].URI()) + assert.Contains(t, sourceSet.Sources()[0].Get(), "x-access-token:test-token@github.com") + assert.Equal(t, "https://x-access-token:****@github.com/owner/repo1.git", sourceSet.Sources()[0].String()) + assert.Equal(t, uris[1], sourceSet.Sources()[1].Get()) + assert.Equal(t, uris[2], sourceSet.Sources()[2].Get()) }) t.Run("rewriter error", func(t *testing.T) { - errorRewriter := func(uri string) (string, error) { - return "", errors.New("rewriter error") + errorRewriter := func(_ sources.SourceURI) (sources.SourceURI, error) { + return sources.SourceURI{}, errors.New("rewriter error") } uris := []string{"https://github.com/owner/repo.git"} - _, err := ForURIs(l, "testdir", "testenv", uris, errorRewriter) + _, err := sources.ForURIs(l, "testdir", "testenv", uris, errorRewriter) assert.Error(t, err) assert.Contains(t, err.Error(), "rewriter error") }) t.Run("git remote helper uri", func(t *testing.T) { - _, err := ForURIs(l, "testdir", "testenv", []string{"zzq::x.git"}) + _, err := sources.ForURIs(l, "testdir", "testenv", []string{"zzq::x.git"}) assert.Error(t, err) assert.Contains(t, err.Error(), "remote helpers are not supported") }) t.Run("invalid rewritten uri", func(t *testing.T) { - invalidRewriter := func(uri string) (string, error) { - return "invalid://not-a-valid-source", nil + invalidRewriter := func(_ sources.SourceURI) (sources.SourceURI, error) { + return sources.NewSourceURI("invalid://not-a-valid-source"), nil } uris := []string{"https://github.com/owner/repo.git"} - _, err := ForURIs(l, "testdir", "testenv", uris, invalidRewriter) + _, err := sources.ForURIs(l, "testdir", "testenv", uris, invalidRewriter) assert.Error(t, err) assert.Contains(t, err.Error(), "unsupported source") diff --git a/ui/logger.go b/ui/logger.go index e9cb40ef..7be33697 100644 --- a/ui/logger.go +++ b/ui/logger.go @@ -10,6 +10,7 @@ import ( "time" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/internal/redact" ) //go:generate stringer -linecomment -type Level @@ -110,7 +111,7 @@ func (l *logWriter) Sync() error { if len(l.buf) > 0 { line := string(l.buf) l.buf = nil - l.logf(l.level, "%s", ansiStripRe.ReplaceAllString(line, "")) + l.logf(l.level, "%s", redact.Credentials(ansiStripRe.ReplaceAllString(line, ""))) } l.lock.Unlock() return nil @@ -131,7 +132,7 @@ func (l *logWriter) Write(b []byte) (int, error) { } l.lock.Unlock() for _, line := range lines { - l.logf(l.level, "%s", ansiStripRe.ReplaceAllString(line, "")) + l.logf(l.level, "%s", redact.Credentials(ansiStripRe.ReplaceAllString(line, ""))) } return len(b), nil } diff --git a/util/git.go b/util/git.go index fa701e71..03c6560b 100644 --- a/util/git.go +++ b/util/git.go @@ -24,21 +24,27 @@ func GitArgs(args ...string) []string { return append(out, args...) } -// ValidateGitURL rejects URLs selecting a transport Hermit does not support, and -// URLs that git would interpret as an option. -func ValidateGitURL(url string) error { +type sourceValue interface { + Get() string + String() string +} + +// ValidateGitURL rejects sources selecting a transport Hermit does not support, +// and sources that git would interpret as an option. +func ValidateGitURL(source sourceValue) error { + url := source.Get() if strings.HasPrefix(url, "-") { - return errors.Errorf("invalid git URL %q: cannot start with '-'", url) + return errors.Errorf("invalid git URL %q: cannot start with '-'", source) } scheme := gitURLSchemeRe.FindString(url) switch rest := url[len(scheme):]; { case strings.HasPrefix(rest, "://"): if !slices.Contains(allowedGitSchemes, strings.ToLower(scheme)) { - return errors.Errorf("invalid git URL %q: scheme must be one of %s", url, strings.Join(allowedGitSchemes, ", ")) + return errors.Errorf("invalid git URL %q: scheme must be one of %s", source, strings.Join(allowedGitSchemes, ", ")) } case strings.HasPrefix(rest, "::"): - return errors.Errorf("invalid git URL %q: remote helpers are not supported", url) + return errors.Errorf("invalid git URL %q: remote helpers are not supported", source) } return nil } diff --git a/util/git_test.go b/util/git_test.go index 4693738f..5953179c 100644 --- a/util/git_test.go +++ b/util/git_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/alecthomas/assert/v2" + "github.com/cashapp/hermit/sources" "github.com/cashapp/hermit/util" ) @@ -31,7 +32,7 @@ func TestValidateGitURL(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := util.ValidateGitURL(test.url) + err := util.ValidateGitURL(sources.NewSourceURI(test.url)) if test.fails { assert.Error(t, err) } else { @@ -54,3 +55,10 @@ func TestGitArgsPinsTransportPolicy(t *testing.T) { "clone", "--", "https://example.com/repo.git", }, args) } + +func TestValidateGitURLDoesNotLeakCredentials(t *testing.T) { + err := util.ValidateGitURL(sources.NewSourceURI("unknown://user:secret-token@example.com/repo.git")) + assert.Error(t, err) + assert.NotContains(t, err.Error(), "secret-token") + assert.Contains(t, err.Error(), "unknown://user:****@example.com/repo.git") +} diff --git a/util/run.go b/util/run.go index 8180a71f..7c9b025f 100644 --- a/util/run.go +++ b/util/run.go @@ -12,22 +12,10 @@ import ( "github.com/cashapp/hermit/envars" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/internal/redact" "github.com/cashapp/hermit/ui" ) -// CommandRunner abstracts how we run command in a given directory -type CommandRunner interface { - // RunInDir runs a command in the given directory. - RunInDir(log *ui.Task, dir string, args ...string) error -} - -// RealCommandRunner actually calls command -type RealCommandRunner struct{} - -func (g *RealCommandRunner) RunInDir(task *ui.Task, dir string, commands ...string) error { - return errors.WithStack(RunSystemInDir(task, dir, commands...)) -} - // SystemCommand constructs a command for an external tool used internally by // Hermit. The executable is resolved from PATH with the active Hermit // environment's changes reverted, and that same PATH is inherited by its @@ -61,6 +49,17 @@ func SystemCommand(args ...string) (*exec.Cmd, error) { return cmd, nil } +// SystemCommandWithSource constructs a system command whose arguments contain +// a source URI. The source remains wrapped until the exec.Cmd boundary. +func SystemCommandWithSource( + argsBeforeSource []string, + source sourceValue, + argsAfterSource ...string, +) (*exec.Cmd, error) { + rawArgs, _ := argsWithSource(argsBeforeSource, source, argsAfterSource) + return SystemCommand(rawArgs...) +} + // systemEnviron returns the current environment with PATH restored to its // state before Hermit activation. All other environment variables are left // unchanged. @@ -96,7 +95,7 @@ func RunSystem(log *ui.Task, args ...string) error { // Capture runs a command, returning combined stdout and stderr. func Capture(log ui.Logger, args ...string) ([]byte, error) { - log.Debugf("%s", shellquote.Join(args...)) + log.Debugf("%s", redact.Credentials(shellquote.Join(args...))) cmd := exec.Command(args[0], args[1:]...) //nolint:gosec,noctx return captureOutput(log, cmd) } @@ -109,7 +108,7 @@ func CaptureSystem(log ui.Logger, args ...string) ([]byte, error) { // CaptureSystemInDir runs an external tool used internally by Hermit in the given dir // and returns its output. func CaptureSystemInDir(log ui.Logger, dir string, args ...string) ([]byte, error) { - log.Debugf("%s", shellquote.Join(args...)) + log.Debugf("%s", redact.Credentials(shellquote.Join(args...))) cmd, err := SystemCommand(args...) if err != nil { return nil, errors.WithStack(err) @@ -118,20 +117,44 @@ func CaptureSystemInDir(log ui.Logger, dir string, args ...string) ([]byte, erro return captureOutput(log, cmd) } +// CaptureSystemWithSource runs a system command whose arguments contain a +// source URI and returns its output. The source remains wrapped until the +// exec.Cmd boundary and its display form is used for logs and errors. +func CaptureSystemWithSource( + log ui.Logger, + argsBeforeSource []string, + source sourceValue, + argsAfterSource ...string, +) ([]byte, error) { + rawArgs, displayArgs := argsWithSource(argsBeforeSource, source, argsAfterSource) + log.Debugf("%s", shellquote.Join(displayArgs...)) + cmd, err := SystemCommand(rawArgs...) + if err != nil { + return nil, errors.WithStack(err) + } + return captureOutputWithDisplay(log, cmd, displayArgs) +} + // CaptureInDir runs a command in the given dir, returning combined stdout and stderr. func CaptureInDir(log ui.Logger, dir string, args ...string) ([]byte, error) { - log.Debugf("%s", shellquote.Join(args...)) + log.Debugf("%s", redact.Credentials(shellquote.Join(args...))) cmd := exec.Command(args[0], args[1:]...) //nolint:gosec,noctx cmd.Dir = dir return captureOutput(log, cmd) } func captureOutput(log ui.Logger, cmd *exec.Cmd) ([]byte, error) { + return captureOutputWithDisplay(log, cmd, cmd.Args) +} + +func captureOutputWithDisplay(log ui.Logger, cmd *exec.Cmd, displayArgs []string) ([]byte, error) { out, err := cmd.CombinedOutput() if err != nil { - return out, errors.Wrapf(err, "%s: %s", shellquote.Join(cmd.Args...), strings.TrimSpace(string(out))) + return out, errors.Wrapf(err, "%s: %s", + redact.Credentials(shellquote.Join(displayArgs...)), + redact.Credentials(strings.TrimSpace(string(out)))) } - _, _ = log.Write(out) + _, _ = log.Write([]byte(redact.Credentials(string(out)))) return out, nil } @@ -143,17 +166,51 @@ func RunInDir(log *ui.Task, dir string, args ...string) error { if err != nil { // log.Write() goes to debug, so only dump the logs at error if we haven't already. if !log.WillLog(ui.LevelDebug) { - log.Errorf("%s", out.String()) + log.Errorf("%s", redact.Credentials(out.String())) } - return errors.Wrapf(err, "%s failed", shellquote.Join(args...)) + return errors.Wrapf(err, "%s failed", redact.Credentials(shellquote.Join(args...))) } return nil } // RunSystemInDir runs an external tool used internally by Hermit in the given dir. func RunSystemInDir(log *ui.Task, dir string, args ...string) error { + return runSystemInDir(log, dir, args, args) +} + +// RunSystemInDirWithSource runs a command whose arguments contain a source URI. +// The raw source is used only for execution; logging and errors use String(). +func RunSystemInDirWithSource( + log *ui.Task, + dir string, + argsBeforeSource []string, + source interface { + Get() string + String() string + }, + argsAfterSource ...string, +) error { + rawArgs, displayArgs := argsWithSource(argsBeforeSource, source, argsAfterSource) + return runSystemInDir(log, dir, rawArgs, displayArgs) +} + +func argsWithSource(argsBeforeSource []string, source sourceValue, argsAfterSource []string) (rawArgs, displayArgs []string) { + rawArgs = make([]string, 0, len(argsBeforeSource)+1+len(argsAfterSource)) + rawArgs = append(rawArgs, argsBeforeSource...) + rawArgs = append(rawArgs, source.Get()) + rawArgs = append(rawArgs, argsAfterSource...) + + displayArgs = make([]string, 0, len(rawArgs)) + displayArgs = append(displayArgs, argsBeforeSource...) + displayArgs = append(displayArgs, source.String()) + displayArgs = append(displayArgs, argsAfterSource...) + return rawArgs, displayArgs +} + +func runSystemInDir(log *ui.Task, dir string, args, displayArgs []string) error { log = log.SubTask("exec") - log.Debugf("%s", shellquote.Join(args...)) + display := redact.Credentials(shellquote.Join(displayArgs...)) + log.Debugf("%s", display) b := &bytes.Buffer{} w := io.MultiWriter(b, log) cmd, err := SystemCommand(args...) @@ -165,9 +222,9 @@ func RunSystemInDir(log *ui.Task, dir string, args ...string) error { cmd.Stderr = w if err = cmd.Run(); err != nil { if !log.WillLog(ui.LevelDebug) { - log.Errorf("%s", b.String()) + log.Errorf("%s", redact.Credentials(b.String())) } - return errors.Wrapf(err, "%s failed", shellquote.Join(args...)) + return errors.Wrapf(err, "%s failed", display) } return nil } @@ -178,7 +235,7 @@ func RunSystemInDir(log *ui.Task, dir string, args ...string) error { // of the execution func Command(log *ui.Task, args ...string) (*exec.Cmd, *bytes.Buffer) { log = log.SubTask("exec") - log.Debugf("%s", shellquote.Join(args...)) + log.Debugf("%s", redact.Credentials(shellquote.Join(args...))) b := &bytes.Buffer{} w := io.MultiWriter(b, log) cmd := exec.Command(args[0], args[1:]...) //nolint:gosec,noctx diff --git a/util/run_test.go b/util/run_test.go index 6c6d1043..e9b3085d 100644 --- a/util/run_test.go +++ b/util/run_test.go @@ -10,6 +10,8 @@ import ( "github.com/alecthomas/assert/v2" "github.com/cashapp/hermit/envars" + "github.com/cashapp/hermit/sources" + "github.com/cashapp/hermit/ui" "github.com/cashapp/hermit/util" ) @@ -52,3 +54,39 @@ func TestSystemCommandRejectsPaths(t *testing.T) { _, err := util.SystemCommand(path) assert.EqualError(t, err, `system command must be a bare name: "`+path+`"`) } + +func TestRunRedactsURLCredentialsFromLogsAndErrors(t *testing.T) { + const ( + secret = "dx26-super-secret" + rawURL = "https://x-access-token:" + secret + "@github.com/owner/repo.git" + ) + l, output := ui.NewForTesting() + l.SetProgressBarEnabled(false) + + err := util.Run(l.Task("source"), "/bin/sh", "-c", "printf '%s\\n' '"+rawURL+"'; exit 1") + assert.Error(t, err) + + combined := output.String() + err.Error() + assert.False(t, strings.Contains(combined, secret), "credential leaked in output: %s", combined) + assert.Contains(t, combined, "https://x-access-token:****@github.com/owner/repo.git") +} + +func TestCaptureSystemWithSourceKeepsRawURLAtExecutionBoundary(t *testing.T) { + const ( + secret = "dx26-super-secret" + rawURL = "https://x-access-token:" + secret + "@github.com/owner/repo.git" + ) + l, output := ui.NewForTesting() + source := sources.NewSourceURI(rawURL) + + _, err := util.CaptureSystemWithSource( + l, + []string{"sh", "-c", `printf '%s\n' "$1"; exit 1`, "sh"}, + source, + ) + assert.Error(t, err) + + combined := output.String() + err.Error() + assert.NotContains(t, combined, secret) + assert.Contains(t, combined, "https://x-access-token:****@github.com/owner/repo.git") +}