From 99ae85e0ea4f3bdf43b4c20ee57ebdf72b31d6ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 22:24:45 +0200 Subject: [PATCH 1/6] Split obtainBearerToken from dockerClient.setupRequestAuth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... because we will make it more complex. Should not change behavior. Signed-off-by: Miloslav Trmač --- image/docker/docker_client.go | 95 +++++++++++++++++++---------------- 1 file changed, 52 insertions(+), 43 deletions(-) diff --git a/image/docker/docker_client.go b/image/docker/docker_client.go index 1c0d67105e..486f16fd98 100644 --- a/image/docker/docker_client.go +++ b/image/docker/docker_client.go @@ -716,50 +716,11 @@ func (c *dockerClient) setupRequestAuth(req *http.Request, extraScope *authScope req.SetBasicAuth(c.auth.Username, c.auth.Password) return nil case "bearer": - registryToken := c.registryToken - if registryToken == "" { - cacheKey := "" - scopes := []authScope{c.scope} - if extraScope != nil { - // Using ':' as a separator here is unambiguous because getBearerToken below - // uses the same separator when formatting a remote request (and because - // repository names that we create can't contain colons, and extraScope values - // coming from a server come from `parseAuthScope`, which also splits on colons). - cacheKey = fmt.Sprintf("%s:%s:%s", extraScope.resourceType, extraScope.remoteName, extraScope.actions) - if colonCount := strings.Count(cacheKey, ":"); colonCount != 2 { - return fmt.Errorf( - "Internal error: there must be exactly 2 colons in the cacheKey ('%s') but got %d", - cacheKey, - colonCount, - ) - } - scopes = append(scopes, *extraScope) - } - var token bearerToken - t, inCache := c.tokenCache.Load(cacheKey) - if inCache { - token = t.(bearerToken) - } - if !inCache || time.Now().After(token.expirationTime) { - var ( - t *bearerToken - err error - ) - if c.auth.IdentityToken != "" { - t, err = c.getBearerTokenOAuth2(req.Context(), challenge, scopes) - } else { - t, err = c.getBearerToken(req.Context(), challenge, scopes) - } - if err != nil { - return err - } - - token = *t - c.tokenCache.Store(cacheKey, token) - } - registryToken = token.token + token, err := c.obtainBearerToken(req.Context(), challenge, extraScope) + if err != nil { + return err } - req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", registryToken)) + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) return nil default: logrus.Debugf("no handler for %s authentication", challenge.Scheme) @@ -769,6 +730,54 @@ func (c *dockerClient) setupRequestAuth(req *http.Request, extraScope *authScope return nil } +// obtainBearerToken gets an "Authorization: Bearer" token if one is available, or obtains a fresh one. +func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challenge, extraScope *authScope) (string, error) { + registryToken := c.registryToken + if registryToken == "" { + cacheKey := "" + scopes := []authScope{c.scope} + if extraScope != nil { + // Using ':' as a separator here is unambiguous because getBearerToken below + // uses the same separator when formatting a remote request (and because + // repository names that we create can't contain colons, and extraScope values + // coming from a server come from `parseAuthScope`, which also splits on colons). + cacheKey = fmt.Sprintf("%s:%s:%s", extraScope.resourceType, extraScope.remoteName, extraScope.actions) + if colonCount := strings.Count(cacheKey, ":"); colonCount != 2 { + return "", fmt.Errorf( + "Internal error: there must be exactly 2 colons in the cacheKey ('%s') but got %d", + cacheKey, + colonCount, + ) + } + scopes = append(scopes, *extraScope) + } + var token bearerToken + t, inCache := c.tokenCache.Load(cacheKey) + if inCache { + token = t.(bearerToken) + } + if !inCache || time.Now().After(token.expirationTime) { + var ( + t *bearerToken + err error + ) + if c.auth.IdentityToken != "" { + t, err = c.getBearerTokenOAuth2(ctx, challenge, scopes) + } else { + t, err = c.getBearerToken(ctx, challenge, scopes) + } + if err != nil { + return "", err + } + + token = *t + c.tokenCache.Store(cacheKey, token) + } + registryToken = token.token + } + return registryToken, nil +} + func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, challenge challenge, scopes []authScope) (*bearerToken, error) { realm, ok := challenge.Parameters["realm"] From 06a3fd2e05c47a869588765d6b6cdecd727487db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 22:26:25 +0200 Subject: [PATCH 2/6] Exit early from obtainBearerToken if c.registryToken is set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to decrease indentation and remove a variable. Should not change behavior. Signed-off-by: Miloslav Trmač --- image/docker/docker_client.go | 81 ++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 40 deletions(-) diff --git a/image/docker/docker_client.go b/image/docker/docker_client.go index 486f16fd98..8a64a7f8cc 100644 --- a/image/docker/docker_client.go +++ b/image/docker/docker_client.go @@ -732,50 +732,51 @@ func (c *dockerClient) setupRequestAuth(req *http.Request, extraScope *authScope // obtainBearerToken gets an "Authorization: Bearer" token if one is available, or obtains a fresh one. func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challenge, extraScope *authScope) (string, error) { - registryToken := c.registryToken - if registryToken == "" { - cacheKey := "" - scopes := []authScope{c.scope} - if extraScope != nil { - // Using ':' as a separator here is unambiguous because getBearerToken below - // uses the same separator when formatting a remote request (and because - // repository names that we create can't contain colons, and extraScope values - // coming from a server come from `parseAuthScope`, which also splits on colons). - cacheKey = fmt.Sprintf("%s:%s:%s", extraScope.resourceType, extraScope.remoteName, extraScope.actions) - if colonCount := strings.Count(cacheKey, ":"); colonCount != 2 { - return "", fmt.Errorf( - "Internal error: there must be exactly 2 colons in the cacheKey ('%s') but got %d", - cacheKey, - colonCount, - ) - } - scopes = append(scopes, *extraScope) - } - var token bearerToken - t, inCache := c.tokenCache.Load(cacheKey) - if inCache { - token = t.(bearerToken) - } - if !inCache || time.Now().After(token.expirationTime) { - var ( - t *bearerToken - err error + if c.registryToken != "" { + return c.registryToken, nil + } + + cacheKey := "" + scopes := []authScope{c.scope} + if extraScope != nil { + // Using ':' as a separator here is unambiguous because getBearerToken below + // uses the same separator when formatting a remote request (and because + // repository names that we create can't contain colons, and extraScope values + // coming from a server come from `parseAuthScope`, which also splits on colons). + cacheKey = fmt.Sprintf("%s:%s:%s", extraScope.resourceType, extraScope.remoteName, extraScope.actions) + if colonCount := strings.Count(cacheKey, ":"); colonCount != 2 { + return "", fmt.Errorf( + "Internal error: there must be exactly 2 colons in the cacheKey ('%s') but got %d", + cacheKey, + colonCount, ) - if c.auth.IdentityToken != "" { - t, err = c.getBearerTokenOAuth2(ctx, challenge, scopes) - } else { - t, err = c.getBearerToken(ctx, challenge, scopes) - } - if err != nil { - return "", err - } + } + scopes = append(scopes, *extraScope) + } - token = *t - c.tokenCache.Store(cacheKey, token) + var token bearerToken + t, inCache := c.tokenCache.Load(cacheKey) + if inCache { + token = t.(bearerToken) + } + if !inCache || time.Now().After(token.expirationTime) { + var ( + t *bearerToken + err error + ) + if c.auth.IdentityToken != "" { + t, err = c.getBearerTokenOAuth2(ctx, challenge, scopes) + } else { + t, err = c.getBearerToken(ctx, challenge, scopes) } - registryToken = token.token + if err != nil { + return "", err + } + + token = *t + c.tokenCache.Store(cacheKey, token) } - return registryToken, nil + return token.token, nil } func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, challenge challenge, From 5fd9138d0e26063109d4750e09155e4bd2e28599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 21:43:41 +0200 Subject: [PATCH 3/6] Use an explicit lock for dockerClient.tokenCache instead of sync.Map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will want to manage concurrency in more detail. Should not change behavior. Signed-off-by: Miloslav Trmač --- image/docker/docker_client.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/image/docker/docker_client.go b/image/docker/docker_client.go index 8a64a7f8cc..87a1241c52 100644 --- a/image/docker/docker_client.go +++ b/image/docker/docker_client.go @@ -116,8 +116,9 @@ type dockerClient struct { challenges []challenge supportsSignatures bool - // Private state for setupRequestAuth (key: string, value: bearerToken) - tokenCache sync.Map + // Private state for setupRequestAuth + tokenCacheLock sync.Mutex // Protects tokenCache. + tokenCache map[string]bearerToken // Private state for detectProperties: detectPropertiesOnce sync.Once // detectPropertiesOnce is used to execute detectProperties() at most once. detectPropertiesError error // detectPropertiesError caches the initial error. @@ -274,6 +275,7 @@ func newDockerClient(sys *types.SystemContext, registry, reference string) (*doc registry: registry, userAgent: userAgent, tlsClientConfig: tlsClientConfig, + tokenCache: map[string]bearerToken{}, reportedWarnings: set.New[string](), }, nil } @@ -755,10 +757,12 @@ func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challeng } var token bearerToken - t, inCache := c.tokenCache.Load(cacheKey) - if inCache { - token = t.(bearerToken) - } + var inCache bool + func() { // A scope for defer + c.tokenCacheLock.Lock() + defer c.tokenCacheLock.Unlock() + token, inCache = c.tokenCache[cacheKey] + }() if !inCache || time.Now().After(token.expirationTime) { var ( t *bearerToken @@ -774,7 +778,11 @@ func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challeng } token = *t - c.tokenCache.Store(cacheKey, token) + func() { // A scope for defer + c.tokenCacheLock.Lock() + defer c.tokenCacheLock.Unlock() + c.tokenCache[cacheKey] = token + }() } return token.token, nil } From 6e98370b3985271553d10e74889db2ff16ebea2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 22:29:17 +0200 Subject: [PATCH 4/6] Store a pointer to a bearerToken in tokenCache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will want to add a lock to it, so we must stop copying it by value. Should not change behavior. Signed-off-by: Miloslav Trmač --- image/docker/docker_client.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/image/docker/docker_client.go b/image/docker/docker_client.go index 87a1241c52..fe8ac37836 100644 --- a/image/docker/docker_client.go +++ b/image/docker/docker_client.go @@ -118,7 +118,7 @@ type dockerClient struct { // Private state for setupRequestAuth tokenCacheLock sync.Mutex // Protects tokenCache. - tokenCache map[string]bearerToken + tokenCache map[string]*bearerToken // Private state for detectProperties: detectPropertiesOnce sync.Once // detectPropertiesOnce is used to execute detectProperties() at most once. detectPropertiesError error // detectPropertiesError caches the initial error. @@ -275,7 +275,7 @@ func newDockerClient(sys *types.SystemContext, registry, reference string) (*doc registry: registry, userAgent: userAgent, tlsClientConfig: tlsClientConfig, - tokenCache: map[string]bearerToken{}, + tokenCache: map[string]*bearerToken{}, reportedWarnings: set.New[string](), }, nil } @@ -756,7 +756,7 @@ func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challeng scopes = append(scopes, *extraScope) } - var token bearerToken + var token *bearerToken var inCache bool func() { // A scope for defer c.tokenCacheLock.Lock() @@ -777,7 +777,7 @@ func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challeng return "", err } - token = *t + token = t func() { // A scope for defer c.tokenCacheLock.Lock() defer c.tokenCacheLock.Unlock() From c2308988b0715f253bfc4a91ad47530609a94037 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 22:41:25 +0200 Subject: [PATCH 5/6] Move creation of bearerToken to obtainBearerToken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of having getBearerToken* construct a new bearerToken object, have the caller provide one. This will allow us to record that a token is being obtained, so that others can wait for it. Should not change behavior. Signed-off-by: Miloslav Trmač --- image/docker/docker_client.go | 60 +++++++++++++++--------------- image/docker/docker_client_test.go | 10 +++-- 2 files changed, 37 insertions(+), 33 deletions(-) diff --git a/image/docker/docker_client.go b/image/docker/docker_client.go index fe8ac37836..2d4d064253 100644 --- a/image/docker/docker_client.go +++ b/image/docker/docker_client.go @@ -764,20 +764,18 @@ func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challeng token, inCache = c.tokenCache[cacheKey] }() if !inCache || time.Now().After(token.expirationTime) { - var ( - t *bearerToken - err error - ) + token = &bearerToken{} + + var err error if c.auth.IdentityToken != "" { - t, err = c.getBearerTokenOAuth2(ctx, challenge, scopes) + err = c.getBearerTokenOAuth2(ctx, token, challenge, scopes) } else { - t, err = c.getBearerToken(ctx, challenge, scopes) + err = c.getBearerToken(ctx, token, challenge, scopes) } if err != nil { return "", err } - token = t func() { // A scope for defer c.tokenCacheLock.Lock() defer c.tokenCacheLock.Unlock() @@ -787,16 +785,19 @@ func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challeng return token.token, nil } -func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, challenge challenge, - scopes []authScope) (*bearerToken, error) { +// getBearerTokenOAuth2 obtains an "Authorization: Bearer" token using a pre-existing identity token per +// https://github.com/distribution/distribution/blob/main/docs/spec/auth/oauth.md for challenge and scopes, +// and writes it into dest. +func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, dest *bearerToken, challenge challenge, + scopes []authScope) error { realm, ok := challenge.Parameters["realm"] if !ok { - return nil, errors.New("missing realm in bearer auth challenge") + return errors.New("missing realm in bearer auth challenge") } authReq, err := http.NewRequestWithContext(ctx, http.MethodPost, realm, nil) if err != nil { - return nil, err + return err } // Make the form data required against the oauth2 authentication @@ -821,26 +822,29 @@ func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, challenge chall logrus.Debugf("%s %s", authReq.Method, authReq.URL.Redacted()) res, err := c.client.Do(authReq) if err != nil { - return nil, err + return err } defer res.Body.Close() if err := httpResponseToError(res, "Trying to obtain access token"); err != nil { - return nil, err + return err } - return newBearerTokenFromHTTPResponseBody(res) + return dest.readFromHTTPResponseBody(res) } -func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge, - scopes []authScope) (*bearerToken, error) { +// getBearerToken obtains an "Authorization: Bearer" token using a GET request, per +// https://github.com/distribution/distribution/blob/main/docs/spec/auth/token.md for challenge and scopes, +// and writes it into dest. +func (c *dockerClient) getBearerToken(ctx context.Context, dest *bearerToken, challenge challenge, + scopes []authScope) error { realm, ok := challenge.Parameters["realm"] if !ok { - return nil, errors.New("missing realm in bearer auth challenge") + return errors.New("missing realm in bearer auth challenge") } authReq, err := http.NewRequestWithContext(ctx, http.MethodGet, realm, nil) if err != nil { - return nil, err + return err } params := authReq.URL.Query() @@ -868,22 +872,22 @@ func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge, logrus.Debugf("%s %s", authReq.Method, authReq.URL.Redacted()) res, err := c.client.Do(authReq) if err != nil { - return nil, err + return err } defer res.Body.Close() if err := httpResponseToError(res, "Requesting bearer token"); err != nil { - return nil, err + return err } - return newBearerTokenFromHTTPResponseBody(res) + return dest.readFromHTTPResponseBody(res) } -// newBearerTokenFromHTTPResponseBody parses a http.Response to obtain a bearerToken. +// readFromHTTPResponseBody sets token data by parsing a http.Response. // The caller is still responsible for ensuring res.Body is closed. -func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error) { +func (bt *bearerToken) readFromHTTPResponseBody(res *http.Response) error { blob, err := iolimits.ReadAtMost(res.Body, iolimits.MaxAuthTokenBodySize) if err != nil { - return nil, err + return err } var token struct { @@ -899,12 +903,10 @@ func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error if len(bodySample) > bodySampleLength { bodySample = bodySample[:bodySampleLength] } - return nil, fmt.Errorf("decoding bearer token (last URL %q, body start %q): %w", res.Request.URL.Redacted(), string(bodySample), err) + return fmt.Errorf("decoding bearer token (last URL %q, body start %q): %w", res.Request.URL.Redacted(), string(bodySample), err) } - bt := &bearerToken{ - token: token.Token, - } + bt.token = token.Token if bt.token == "" { bt.token = token.AccessToken } @@ -917,7 +919,7 @@ func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error token.IssuedAt = time.Now().UTC() } bt.expirationTime = token.IssuedAt.Add(time.Duration(token.ExpiresIn) * time.Second) - return bt, nil + return nil } // detectPropertiesHelper performs the work of detectProperties which executes diff --git a/image/docker/docker_client_test.go b/image/docker/docker_client_test.go index e383188bc6..d5b9fd460f 100644 --- a/image/docker/docker_client_test.go +++ b/image/docker/docker_client_test.go @@ -106,7 +106,7 @@ func testTokenHTTPResponse(t *testing.T, body string) *http.Response { } } -func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { +func TestBearerTokenReadFromHTTPResponseBody(t *testing.T) { for _, c := range []struct { input string expected *bearerToken // or nil if a failure is expected @@ -128,7 +128,8 @@ func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { expected: &bearerToken{token: "IAmAToken", expirationTime: time.Unix(1514800802+60, 0)}, }, } { - token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, c.input)) + token := &bearerToken{} + err := token.readFromHTTPResponseBody(testTokenHTTPResponse(t, c.input)) if c.expected == nil { assert.Error(t, err, c.input) } else { @@ -140,11 +141,12 @@ func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { } } -func TestNewBearerTokenFromHTTPResponseBodyIssuedAtZero(t *testing.T) { +func TestBearerTokenReadFromHTTPResponseBodyIssuedAtZero(t *testing.T) { zeroTime := time.Time{}.Format(time.RFC3339) now := time.Now() tokenBlob := fmt.Sprintf(`{"token":"IAmAToken","expires_in":100,"issued_at":"%s"}`, zeroTime) - token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, tokenBlob)) + token := &bearerToken{} + err := token.readFromHTTPResponseBody(testTokenHTTPResponse(t, tokenBlob)) require.NoError(t, err) expectedExpiration := now.Add(time.Duration(100) * time.Second) require.False(t, token.expirationTime.Before(expectedExpiration), From de2a06c21bd7fea2dfb046c1a7fa43951f4b6709 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 23:14:03 +0200 Subject: [PATCH 6/6] Only obtain a bearer token once at a time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, on pushes, we can start several concurrent layer pushes; each one will check for a bearer token in tokenCache, find none, and ask the server for one, and then write it into the cache. So, we can hammer the server with 6 basically-concurrent token requests. That's unnecessary, slower than just asking once, and potentially might impact rate limiting heuristics. Instead, serialize writes to a bearerToken so that we only have one request in flight at a time. This does not benefit pulls, where the first request is for a manifest; that obtains a token, so subsequent concurrent layer pulls will not request a token again. Signed-off-by: Miloslav Trmač --- image/docker/docker_client.go | 72 ++++++++++++++++++++++++++--------- 1 file changed, 53 insertions(+), 19 deletions(-) diff --git a/image/docker/docker_client.go b/image/docker/docker_client.go index 2d4d064253..467879c236 100644 --- a/image/docker/docker_client.go +++ b/image/docker/docker_client.go @@ -35,6 +35,7 @@ import ( "go.podman.io/image/v5/types" "go.podman.io/storage/pkg/fileutils" "go.podman.io/storage/pkg/homedir" + "golang.org/x/sync/semaphore" ) const ( @@ -86,8 +87,19 @@ type extensionSignatureList struct { Signatures []extensionSignature `json:"signatures"` } -// bearerToken records a cached token we can use to authenticate. +// bearerToken records a cached token we can use to authenticate, or a pending process to obtain one. +// +// The goroutine obtaining the token holds lock to block concurrent token requests, and fills the structure (err and possibly the other fields) +// before releasing the lock. +// Other goroutines obtain lock to block on the token request, if any; and then inspect err to see if the token is usable. +// If it is not, they try to get a new one. type bearerToken struct { + // lock is held while obtaining the token. Potentially nested inside dockerClient.tokenCacheLock. + // This is a counting semaphore only because we need a cancellable lock operation. + lock *semaphore.Weighted + + // The following fields can only be accessed with lock held. + err error // nil if the token was successfully obtained (but may be expired); an error if the next lock holder _must_ obtain a new token. token string expirationTime time.Time } @@ -756,31 +768,53 @@ func (c *dockerClient) obtainBearerToken(ctx context.Context, challenge challeng scopes = append(scopes, *extraScope) } - var token *bearerToken - var inCache bool - func() { // A scope for defer + token, newEntry, err := func() (*bearerToken, bool, error) { // A scope for defer c.tokenCacheLock.Lock() defer c.tokenCacheLock.Unlock() - token, inCache = c.tokenCache[cacheKey] - }() - if !inCache || time.Now().After(token.expirationTime) { - token = &bearerToken{} - - var err error - if c.auth.IdentityToken != "" { - err = c.getBearerTokenOAuth2(ctx, token, challenge, scopes) + token, ok := c.tokenCache[cacheKey] + if ok { + return token, false, nil } else { - err = c.getBearerToken(ctx, token, challenge, scopes) + token = &bearerToken{ + lock: semaphore.NewWeighted(1), + } + // If this is a new *bearerToken, lock the entry before adding it to the cache, so that any other goroutine that finds + // this entry blocks until we obtain the token for the first time, and does not see an empty object + // (and does not try to obtain the token itself when we are going to do so). + if err := token.lock.Acquire(ctx, 1); err != nil { + // We do not block on this Acquire, so we don’t really expect to fail here — but if ctx is canceled, + // there is no point in trying to continue anyway. + return nil, false, err + } + c.tokenCache[cacheKey] = token + return token, true, nil } - if err != nil { + }() + if err != nil { + return "", err + } + if !newEntry { + // If this is an existing *bearerToken, obtain the lock only after releasing c.tokenCacheLock, + // so that users of other cacheKey values are not blocked for the whole duration of our HTTP roundtrip. + if err := token.lock.Acquire(ctx, 1); err != nil { return "", err } + } - func() { // A scope for defer - c.tokenCacheLock.Lock() - defer c.tokenCacheLock.Unlock() - c.tokenCache[cacheKey] = token - }() + defer token.lock.Release(1) + + if !newEntry && token.err == nil && !time.Now().After(token.expirationTime) { + return token.token, nil // We have a usable token already. + } + + if c.auth.IdentityToken != "" { + err = c.getBearerTokenOAuth2(ctx, token, challenge, scopes) + } else { + err = c.getBearerToken(ctx, token, challenge, scopes) + } + token.err = err + if token.err != nil { + return "", token.err } return token.token, nil }