remote/transport: Make bearer transport go-routine-safe #1806

2opremio · 2023-10-10T02:40:11Z

Make the bearer transport safe for use by multiple go-routines.

I found the following race conditions in my application, when using the transport in a client making multiple concurrent requests.

Data race backtraces

==================
WARNING: DATA RACE
Read at 0x00c00014c248 by goroutine 18:
  github.com/google/go-containerregistry/pkg/v1/remote/transport.(*bearerTransport).RoundTrip()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/bearer.go:163 +0x1fc
  github.com/google/go-containerregistry/pkg/v1/remote/transport.(*Wrapper).RoundTrip()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/transport.go:108 +0x4c
  net/http.send()
      /opt/local/lib/go/src/net/http/client.go:260 +0x6cc
  net/http.(*Client).send()
      /opt/local/lib/go/src/net/http/client.go:181 +0x100
  net/http.(*Client).do()
      /opt/local/lib/go/src/net/http/client.go:724 +0xc3c
  net/http.(*Client).Do()
      /opt/local/lib/go/src/net/http/client.go:590 +0x1cc
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.Proxy.getResponse()
      /Users/fons/docker-registry-proxy/internal/proxy/main.go:124 +0x1c4
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.Proxy.ServeHTTP()
      /Users/fons/docker-registry-proxy/internal/proxy/main.go:178 +0x714
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.(*Proxy).ServeHTTP()
      <autogenerated>:1 +0x84
  net/http.serverHandler.ServeHTTP()
      /opt/local/lib/go/src/net/http/server.go:2938 +0x298
  net/http.(*conn).serve()
      /opt/local/lib/go/src/net/http/server.go:2009 +0x8ec
  net/http.(*Server).Serve.func3()
      /opt/local/lib/go/src/net/http/server.go:3086 +0x4c

Previous write at 0x00c00014c248 by goroutine 25:
  github.com/google/go-containerregistry/pkg/v1/remote/transport.(*bearerTransport).RoundTrip()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/bearer.go:174 +0x3d8
  github.com/google/go-containerregistry/pkg/v1/remote/transport.(*Wrapper).RoundTrip()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/transport.go:108 +0x4c
  net/http.send()
      /opt/local/lib/go/src/net/http/client.go:260 +0x6cc
  net/http.(*Client).send()
      /opt/local/lib/go/src/net/http/client.go:181 +0x100
  net/http.(*Client).do()
      /opt/local/lib/go/src/net/http/client.go:724 +0xc3c
  net/http.(*Client).Do()
      /opt/local/lib/go/src/net/http/client.go:590 +0x1cc
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.Proxy.getResponse()
      /Users/fons/docker-registry-proxy/internal/proxy/main.go:124 +0x1c4
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.Proxy.ServeHTTP()
      /Users/fons/docker-registry-proxy/internal/proxy/main.go:178 +0x714
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.(*Proxy).ServeHTTP()
      <autogenerated>:1 +0x84
  net/http.serverHandler.ServeHTTP()
      /opt/local/lib/go/src/net/http/server.go:2938 +0x298
  net/http.(*conn).serve()
      /opt/local/lib/go/src/net/http/server.go:2009 +0x8ec
  net/http.(*Server).Serve.func3()
      /opt/local/lib/go/src/net/http/server.go:3086 +0x4c

Goroutine 18 (running) created at:
  net/http.(*Server).Serve()
      /opt/local/lib/go/src/net/http/server.go:3086 +0x5f4
  net/http.(*Server).ListenAndServe()
      /opt/local/lib/go/src/net/http/server.go:2985 +0xb0
  main.main.func3()
      /Users/fons/docker-registry-proxy/main.go:127 +0x2c

Goroutine 25 (running) created at:
  net/http.(*Server).Serve()
      /opt/local/lib/go/src/net/http/server.go:3086 +0x5f4
  net/http.(*Server).ListenAndServe()
      /opt/local/lib/go/src/net/http/server.go:2985 +0xb0
  main.main.func3()
      /Users/fons/docker-registry-proxy/main.go:127 +0x2c
==================
==================
WARNING: DATA RACE
Read at 0x00c00039a1c0 by goroutine 18:
  github.com/google/go-containerregistry/pkg/v1/remote/transport.stringSet()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/bearer.go:127 +0x4f4
  github.com/google/go-containerregistry/pkg/v1/remote/transport.(*bearerTransport).RoundTrip()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/bearer.go:163 +0x1f0
  github.com/google/go-containerregistry/pkg/v1/remote/transport.(*Wrapper).RoundTrip()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/transport.go:108 +0x4c
  net/http.send()
      /opt/local/lib/go/src/net/http/client.go:260 +0x6cc
  net/http.(*Client).send()
      /opt/local/lib/go/src/net/http/client.go:181 +0x100
  net/http.(*Client).do()
      /opt/local/lib/go/src/net/http/client.go:724 +0xc3c
  net/http.(*Client).Do()
      /opt/local/lib/go/src/net/http/client.go:590 +0x1cc
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.Proxy.getResponse()
      /Users/fons/docker-registry-proxy/internal/proxy/main.go:124 +0x1c4
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.Proxy.ServeHTTP()
      /Users/fons/docker-registry-proxy/internal/proxy/main.go:178 +0x714
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.(*Proxy).ServeHTTP()
      <autogenerated>:1 +0x84
  net/http.serverHandler.ServeHTTP()
      /opt/local/lib/go/src/net/http/server.go:2938 +0x298
  net/http.(*conn).serve()
      /opt/local/lib/go/src/net/http/server.go:2009 +0x8ec
  net/http.(*Server).Serve.func3()
      /opt/local/lib/go/src/net/http/server.go:3086 +0x4c

Previous write at 0x00c00039a1c0 by goroutine 25:
  runtime.slicecopy()
      /opt/local/lib/go/src/runtime/slice.go:310 +0x0
  github.com/google/go-containerregistry/pkg/v1/remote/transport.(*bearerTransport).RoundTrip()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/bearer.go:173 +0x3d0
  github.com/google/go-containerregistry/pkg/v1/remote/transport.(*Wrapper).RoundTrip()
      /Users/fons/go/pkg/mod/github.com/google/go-containerregistry@v0.16.1/pkg/v1/remote/transport/transport.go:108 +0x4c
  net/http.send()
      /opt/local/lib/go/src/net/http/client.go:260 +0x6cc
  net/http.(*Client).send()
      /opt/local/lib/go/src/net/http/client.go:181 +0x100
  net/http.(*Client).do()
      /opt/local/lib/go/src/net/http/client.go:724 +0xc3c
  net/http.(*Client).Do()
      /opt/local/lib/go/src/net/http/client.go:590 +0x1cc
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.Proxy.getResponse()
      /Users/fons/docker-registry-proxy/internal/proxy/main.go:124 +0x1c4
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.Proxy.ServeHTTP()
      /Users/fons/docker-registry-proxy/internal/proxy/main.go:178 +0x714
  dev.azure.com/cur8/TheRigg/_git/docker-registry-proxy/internal/proxy.(*Proxy).ServeHTTP()
      <autogenerated>:1 +0x84
  net/http.serverHandler.ServeHTTP()
      /opt/local/lib/go/src/net/http/server.go:2938 +0x298
  net/http.(*conn).serve()
      /opt/local/lib/go/src/net/http/server.go:2009 +0x8ec
  net/http.(*Server).Serve.func3()
      /opt/local/lib/go/src/net/http/server.go:3086 +0x4c

Goroutine 18 (running) created at:
  net/http.(*Server).Serve()
      /opt/local/lib/go/src/net/http/server.go:3086 +0x5f4
  net/http.(*Server).ListenAndServe()
      /opt/local/lib/go/src/net/http/server.go:2985 +0xb0
  main.main.func3()
      /Users/fons/docker-registry-proxy/main.go:127 +0x2c

Goroutine 25 (running) created at:
  net/http.(*Server).Serve()
      /opt/local/lib/go/src/net/http/server.go:3086 +0x5f4
  net/http.(*Server).ListenAndServe()
      /opt/local/lib/go/src/net/http/server.go:2985 +0xb0
  main.main.func3()
      /Users/fons/docker-registry-proxy/main.go:127 +0x2c
==================

A simplified application could be:

package main

import (
	"net/http"
	
	"github.com/google/go-containerregistry/pkg/authn"
	"github.com/google/go-containerregistry/pkg/name"
	"github.com/google/go-containerregistry/pkg/v1/remote/transport"
)

func newAuthenticatingClient(host, user, password string) (*http.Client, error) {
	registry, err := name.NewRegistry(host)
	if err != nil {
		return nil, err
	}
	basicAuth := &authn.Basic{
		Username: user,
		Password: password,
	}
	tr, err := transport.New(registry, basicAuth, http.DefaultTransport, nil)
	if err != nil {
	  return nil, err
	}
	return &http.Client{Transport: tr}
}

func main() {
	client, err := newAuthenticatingClient("foo", "bar", "buzz")
	if err != nil {
		panic(err)
	}
	// make a lot of concurrent requests with client
}

google-cla · 2023-10-10T02:40:15Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

codecov-commenter · 2023-10-10T02:42:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.69%. Comparing base (55ffb00) to head (b666669).
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1806      +/-   ##
==========================================
+ Coverage   71.65%   71.69%   +0.03%     
==========================================
  Files         123      123              
  Lines        9928     9941      +13     
==========================================
+ Hits         7114     7127      +13     
  Misses       2115     2115              
  Partials      699      699

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dmitris · 2023-11-24T15:11:44Z

@2opremio - could you rebase the branch of this PR on the current trunk to pick up the latest changes, please?

Make the bearer transport safe for use by multiple go-routines.

2opremio · 2024-01-02T16:21:31Z

@dmitris done!

dmitris · 2024-01-11T18:57:27Z

It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

any chance you could do the CLA, @2opremio? Want to see your change in the main repo rather than having to apply it as a patch in CI 😄

2opremio · 2024-01-11T19:15:59Z

I did! It should be approved, isn't it?

dmitris · 2024-02-09T20:54:28Z

@jonjohnsonjr - is this ready to merge or what do we need to move this ahead? Could you please review it? 🙏 Thanks!

github-actions · 2024-05-10T01:26:28Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

github-actions · 2024-08-09T01:29:21Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

2opremio · 2024-08-09T08:12:09Z

@dmitris can we please get this merged?

dmitris · 2024-08-09T10:03:53Z

@dmitris can we please get this merged?

@2opremio I would love to but I'm not a repo owner or admin, don't have the rights to do this.

dmitris · 2024-08-21T08:17:49Z

thanks @jonjohnsonjr - could you also cut a new release (v0.20.3), please? /cc @imjasonh

2opremio force-pushed the make-bearer-transport-thread-safe branch from 95eb9fd to ef93d22 Compare October 10, 2023 02:42

dmitris mentioned this pull request Nov 24, 2023

ggcr: 'go test -race' fails on github.com/sigstore/cosign commit 38e2b30b270128998cf7fc819ffa08fca41cda7e #1849

Closed

remote/transport: Make bearer transport go-routine-safe

b666669

Make the bearer transport safe for use by multiple go-routines.

2opremio force-pushed the make-bearer-transport-thread-safe branch from ef93d22 to b666669 Compare January 2, 2024 16:21

github-actions bot added lifecycle/stale and removed lifecycle/stale labels May 10, 2024

github-actions bot added the lifecycle/stale label Aug 9, 2024

github-actions bot removed the lifecycle/stale label Aug 10, 2024

jonjohnsonjr approved these changes Aug 10, 2024

View reviewed changes

jonjohnsonjr merged commit b8e87ed into google:main Aug 10, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remote/transport: Make bearer transport go-routine-safe #1806

remote/transport: Make bearer transport go-routine-safe #1806

2opremio commented Oct 10, 2023 •

edited

Loading

google-cla bot commented Oct 10, 2023

codecov-commenter commented Oct 10, 2023 •

edited

Loading

dmitris commented Nov 24, 2023

2opremio commented Jan 2, 2024

dmitris commented Jan 11, 2024

2opremio commented Jan 11, 2024

dmitris commented Feb 9, 2024

github-actions bot commented May 10, 2024

github-actions bot commented Aug 9, 2024

2opremio commented Aug 9, 2024

dmitris commented Aug 9, 2024

dmitris commented Aug 21, 2024

remote/transport: Make bearer transport go-routine-safe #1806

remote/transport: Make bearer transport go-routine-safe #1806

Conversation

2opremio commented Oct 10, 2023 • edited Loading

google-cla bot commented Oct 10, 2023

codecov-commenter commented Oct 10, 2023 • edited Loading

Codecov Report

dmitris commented Nov 24, 2023

2opremio commented Jan 2, 2024

dmitris commented Jan 11, 2024

2opremio commented Jan 11, 2024

dmitris commented Feb 9, 2024

github-actions bot commented May 10, 2024

github-actions bot commented Aug 9, 2024

2opremio commented Aug 9, 2024

dmitris commented Aug 9, 2024

dmitris commented Aug 21, 2024

2opremio commented Oct 10, 2023 •

edited

Loading

codecov-commenter commented Oct 10, 2023 •

edited

Loading