From c132a171c723e3ca99156c9bbb460a2a9cfc7ed5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 21:50:16 +0200 Subject: [PATCH 1/6] Beautify a test of newBearerTokenFromHTTPResponseBody MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove an unnecessary cast. Should not change (test) behavior. Signed-off-by: Miloslav Trmač --- docker/docker_client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index 563358bbd..1ffe324fe 100644 --- a/docker/docker_client_test.go +++ b/docker/docker_client_test.go @@ -145,7 +145,7 @@ func TestNewBearerTokenFromHTTPResponseBodyIssuedAtZero(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, string(tokenBlob))) + token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, tokenBlob)) require.NoError(t, err) assert.False(t, token.IssuedAt.Before(now), "expected [%s] not to be before [%s]", token.IssuedAt, now) } From 57d7e8347668bf562b9934e071afda149d0a8370 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 21:56:23 +0200 Subject: [PATCH 2/6] Split the token storage type from the JSON representation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will want to add locks and more to the in-memory type; sharing that with JSON gets awkward, and an explicit separation between the externally-imposed structure and internal records is cleaner anyway. For now, just introduces a separate type with the same structure, should not change behavior. Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/docker/docker_client.go b/docker/docker_client.go index 6b65c52ed..06f9dddc4 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -87,10 +87,10 @@ type extensionSignatureList struct { } type bearerToken struct { - Token string `json:"token"` - AccessToken string `json:"access_token"` - ExpiresIn int `json:"expires_in"` - IssuedAt time.Time `json:"issued_at"` + Token string + AccessToken string + ExpiresIn int + IssuedAt time.Time expirationTime time.Time } @@ -155,7 +155,13 @@ func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error return nil, err } - token := new(bearerToken) + var token struct { + Token string `json:"token"` + AccessToken string `json:"access_token"` + ExpiresIn int `json:"expires_in"` + IssuedAt time.Time `json:"issued_at"` + expirationTime time.Time + } if err := json.Unmarshal(blob, &token); err != nil { const bodySampleLength = 50 bodySample := blob @@ -164,18 +170,25 @@ func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error } return nil, fmt.Errorf("decoding bearer token (last URL %q, body start %q): %w", res.Request.URL.Redacted(), string(bodySample), err) } - if token.Token == "" { - token.Token = token.AccessToken + + bt := &bearerToken{ + Token: token.Token, + AccessToken: token.AccessToken, + ExpiresIn: token.ExpiresIn, + IssuedAt: token.IssuedAt, + } + if bt.Token == "" { + bt.Token = bt.AccessToken } - if token.ExpiresIn < minimumTokenLifetimeSeconds { - token.ExpiresIn = minimumTokenLifetimeSeconds - logrus.Debugf("Increasing token expiration to: %d seconds", token.ExpiresIn) + if bt.ExpiresIn < minimumTokenLifetimeSeconds { + bt.ExpiresIn = minimumTokenLifetimeSeconds + logrus.Debugf("Increasing token expiration to: %d seconds", bt.ExpiresIn) } - if token.IssuedAt.IsZero() { - token.IssuedAt = time.Now().UTC() + if bt.IssuedAt.IsZero() { + bt.IssuedAt = time.Now().UTC() } - token.expirationTime = token.IssuedAt.Add(time.Duration(token.ExpiresIn) * time.Second) - return token, nil + bt.expirationTime = bt.IssuedAt.Add(time.Duration(bt.ExpiresIn) * time.Second) + return bt, nil } // dockerCertDir returns a path to a directory to be consumed by tlsclientconfig.SetupCertificates() depending on ctx and hostPort. From dc3703be47f860a50ba50a46dab8e50ee8ee9ac0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 21:59:50 +0200 Subject: [PATCH 3/6] Make bearerToken.Token private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No need to make it a public field now. Should not change behavior. Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 11 ++++++----- docker/docker_client_test.go | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/docker/docker_client.go b/docker/docker_client.go index 06f9dddc4..b5657387e 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -86,8 +86,9 @@ type extensionSignatureList struct { Signatures []extensionSignature `json:"signatures"` } +// bearerToken records a cached token we can use to authenticate. type bearerToken struct { - Token string + token string AccessToken string ExpiresIn int IssuedAt time.Time @@ -172,13 +173,13 @@ func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error } bt := &bearerToken{ - Token: token.Token, + token: token.Token, AccessToken: token.AccessToken, ExpiresIn: token.ExpiresIn, IssuedAt: token.IssuedAt, } - if bt.Token == "" { - bt.Token = bt.AccessToken + if bt.token == "" { + bt.token = bt.AccessToken } if bt.ExpiresIn < minimumTokenLifetimeSeconds { bt.ExpiresIn = minimumTokenLifetimeSeconds @@ -799,7 +800,7 @@ func (c *dockerClient) setupRequestAuth(req *http.Request, extraScope *authScope token = *t c.tokenCache.Store(cacheKey, token) } - registryToken = token.Token + registryToken = token.token } req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", registryToken)) return nil diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index 1ffe324fe..5d566900b 100644 --- a/docker/docker_client_test.go +++ b/docker/docker_client_test.go @@ -117,15 +117,15 @@ func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { }, { // "token" input: `{"token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{Token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)}, + expected: &bearerToken{token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)}, }, { // "access_token" input: `{"access_token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{Token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)}, + expected: &bearerToken{token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)}, }, { // Small expiry input: `{"token":"IAmAToken","expires_in":1,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{Token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0)}, + expected: &bearerToken{token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0)}, }, } { token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, c.input)) @@ -133,7 +133,7 @@ func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { assert.Error(t, err, c.input) } else { require.NoError(t, err, c.input) - assert.Equal(t, c.expected.Token, token.Token, c.input) + assert.Equal(t, c.expected.token, token.token, c.input) assert.Equal(t, c.expected.ExpiresIn, token.ExpiresIn, c.input) assert.True(t, c.expected.IssuedAt.Equal(token.IssuedAt), "expected [%s] to equal [%s], it did not", token.IssuedAt, c.expected.IssuedAt) From 1b7daaf858e14c7ceca0652d8549a2b4ca54ce82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 22:04:55 +0200 Subject: [PATCH 4/6] Add tests for bearerToken.expirationTime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit That's the value that really matters, not the inputs; and we will remove the inputs from bearerToken. Signed-off-by: Miloslav Trmač --- docker/docker_client_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index 5d566900b..c01957f4b 100644 --- a/docker/docker_client_test.go +++ b/docker/docker_client_test.go @@ -117,15 +117,15 @@ func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { }, { // "token" input: `{"token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)}, + expected: &bearerToken{token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0), expirationTime: time.Unix(1514800802+100, 0)}, }, { // "access_token" input: `{"access_token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)}, + expected: &bearerToken{token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0), expirationTime: time.Unix(1514800802+100, 0)}, }, { // Small expiry input: `{"token":"IAmAToken","expires_in":1,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0)}, + expected: &bearerToken{token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0), expirationTime: time.Unix(1514800802+60, 0)}, }, } { token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, c.input)) @@ -137,6 +137,8 @@ func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { assert.Equal(t, c.expected.ExpiresIn, token.ExpiresIn, c.input) assert.True(t, c.expected.IssuedAt.Equal(token.IssuedAt), "expected [%s] to equal [%s], it did not", token.IssuedAt, c.expected.IssuedAt) + assert.True(t, c.expected.expirationTime.Equal(token.expirationTime), + "expected [%s] to equal [%s], it did not", token.expirationTime, c.expected.expirationTime) } } } @@ -148,6 +150,9 @@ func TestNewBearerTokenFromHTTPResponseBodyIssuedAtZero(t *testing.T) { token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, tokenBlob)) require.NoError(t, err) assert.False(t, token.IssuedAt.Before(now), "expected [%s] not to be before [%s]", token.IssuedAt, now) + expectedExpiration := now.Add(time.Duration(100) * time.Second) + require.False(t, token.expirationTime.Before(expectedExpiration), + "expected [%s] not to be before [%s]", token.expirationTime, expectedExpiration) } func TestUserAgent(t *testing.T) { From 9b43674741d807723990c6a3f7d8ef34ebe19c96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 22:09:07 +0200 Subject: [PATCH 5/6] Remove unnecessary fields from bearerToken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These fields need to exist when parsing JSON; but we can just record the outcome of processing them. Should not change behavior. Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 23 +++++++++-------------- docker/docker_client_test.go | 10 +++------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/docker/docker_client.go b/docker/docker_client.go index b5657387e..b683203e9 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -89,9 +89,6 @@ type extensionSignatureList struct { // bearerToken records a cached token we can use to authenticate. type bearerToken struct { token string - AccessToken string - ExpiresIn int - IssuedAt time.Time expirationTime time.Time } @@ -173,22 +170,20 @@ func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error } bt := &bearerToken{ - token: token.Token, - AccessToken: token.AccessToken, - ExpiresIn: token.ExpiresIn, - IssuedAt: token.IssuedAt, + token: token.Token, } if bt.token == "" { - bt.token = bt.AccessToken + bt.token = token.AccessToken } - if bt.ExpiresIn < minimumTokenLifetimeSeconds { - bt.ExpiresIn = minimumTokenLifetimeSeconds - logrus.Debugf("Increasing token expiration to: %d seconds", bt.ExpiresIn) + + if token.ExpiresIn < minimumTokenLifetimeSeconds { + token.ExpiresIn = minimumTokenLifetimeSeconds + logrus.Debugf("Increasing token expiration to: %d seconds", token.ExpiresIn) } - if bt.IssuedAt.IsZero() { - bt.IssuedAt = time.Now().UTC() + if token.IssuedAt.IsZero() { + token.IssuedAt = time.Now().UTC() } - bt.expirationTime = bt.IssuedAt.Add(time.Duration(bt.ExpiresIn) * time.Second) + bt.expirationTime = token.IssuedAt.Add(time.Duration(token.ExpiresIn) * time.Second) return bt, nil } diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index c01957f4b..16e2c286b 100644 --- a/docker/docker_client_test.go +++ b/docker/docker_client_test.go @@ -117,15 +117,15 @@ func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { }, { // "token" input: `{"token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0), expirationTime: time.Unix(1514800802+100, 0)}, + expected: &bearerToken{token: "IAmAToken", expirationTime: time.Unix(1514800802+100, 0)}, }, { // "access_token" input: `{"access_token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0), expirationTime: time.Unix(1514800802+100, 0)}, + expected: &bearerToken{token: "IAmAToken", expirationTime: time.Unix(1514800802+100, 0)}, }, { // Small expiry input: `{"token":"IAmAToken","expires_in":1,"issued_at":"2018-01-01T10:00:02+00:00"}`, - expected: &bearerToken{token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0), expirationTime: time.Unix(1514800802+60, 0)}, + expected: &bearerToken{token: "IAmAToken", expirationTime: time.Unix(1514800802+60, 0)}, }, } { token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, c.input)) @@ -134,9 +134,6 @@ func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { } else { require.NoError(t, err, c.input) assert.Equal(t, c.expected.token, token.token, c.input) - assert.Equal(t, c.expected.ExpiresIn, token.ExpiresIn, c.input) - assert.True(t, c.expected.IssuedAt.Equal(token.IssuedAt), - "expected [%s] to equal [%s], it did not", token.IssuedAt, c.expected.IssuedAt) assert.True(t, c.expected.expirationTime.Equal(token.expirationTime), "expected [%s] to equal [%s], it did not", token.expirationTime, c.expected.expirationTime) } @@ -149,7 +146,6 @@ func TestNewBearerTokenFromHTTPResponseBodyIssuedAtZero(t *testing.T) { tokenBlob := fmt.Sprintf(`{"token":"IAmAToken","expires_in":100,"issued_at":"%s"}`, zeroTime) token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, tokenBlob)) require.NoError(t, err) - assert.False(t, token.IssuedAt.Before(now), "expected [%s] not to be before [%s]", token.IssuedAt, now) expectedExpiration := now.Add(time.Duration(100) * time.Second) require.False(t, token.expirationTime.Before(expectedExpiration), "expected [%s] not to be before [%s]", token.expirationTime, expectedExpiration) From d6f4f35c13a5d392947d3d014d7076d57685a628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 29 May 2023 22:47:11 +0200 Subject: [PATCH 6/6] Move newBearerTokenFromHTTPResponseBody closer to its callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 84 ++++++++++++++++++++--------------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/docker/docker_client.go b/docker/docker_client.go index b683203e9..97d97fed5 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -145,48 +145,6 @@ const ( noAuth ) -// newBearerTokenFromHTTPResponseBody parses a http.Response to obtain a bearerToken. -// The caller is still responsible for ensuring res.Body is closed. -func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error) { - blob, err := iolimits.ReadAtMost(res.Body, iolimits.MaxAuthTokenBodySize) - if err != nil { - return nil, err - } - - var token struct { - Token string `json:"token"` - AccessToken string `json:"access_token"` - ExpiresIn int `json:"expires_in"` - IssuedAt time.Time `json:"issued_at"` - expirationTime time.Time - } - if err := json.Unmarshal(blob, &token); err != nil { - const bodySampleLength = 50 - bodySample := blob - 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) - } - - bt := &bearerToken{ - token: token.Token, - } - if bt.token == "" { - bt.token = token.AccessToken - } - - if token.ExpiresIn < minimumTokenLifetimeSeconds { - token.ExpiresIn = minimumTokenLifetimeSeconds - logrus.Debugf("Increasing token expiration to: %d seconds", token.ExpiresIn) - } - if token.IssuedAt.IsZero() { - token.IssuedAt = time.Now().UTC() - } - bt.expirationTime = token.IssuedAt.Add(time.Duration(token.ExpiresIn) * time.Second) - return bt, nil -} - // dockerCertDir returns a path to a directory to be consumed by tlsclientconfig.SetupCertificates() depending on ctx and hostPort. func dockerCertDir(sys *types.SystemContext, hostPort string) (string, error) { if sys != nil && sys.DockerCertPath != "" { @@ -898,6 +856,48 @@ func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge, return newBearerTokenFromHTTPResponseBody(res) } +// newBearerTokenFromHTTPResponseBody parses a http.Response to obtain a bearerToken. +// The caller is still responsible for ensuring res.Body is closed. +func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error) { + blob, err := iolimits.ReadAtMost(res.Body, iolimits.MaxAuthTokenBodySize) + if err != nil { + return nil, err + } + + var token struct { + Token string `json:"token"` + AccessToken string `json:"access_token"` + ExpiresIn int `json:"expires_in"` + IssuedAt time.Time `json:"issued_at"` + expirationTime time.Time + } + if err := json.Unmarshal(blob, &token); err != nil { + const bodySampleLength = 50 + bodySample := blob + 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) + } + + bt := &bearerToken{ + token: token.Token, + } + if bt.token == "" { + bt.token = token.AccessToken + } + + if token.ExpiresIn < minimumTokenLifetimeSeconds { + token.ExpiresIn = minimumTokenLifetimeSeconds + logrus.Debugf("Increasing token expiration to: %d seconds", token.ExpiresIn) + } + if token.IssuedAt.IsZero() { + token.IssuedAt = time.Now().UTC() + } + bt.expirationTime = token.IssuedAt.Add(time.Duration(token.ExpiresIn) * time.Second) + return bt, nil +} + // detectPropertiesHelper performs the work of detectProperties which executes // it at most once. func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error {