From b47e339ceada8dca7b34ce2e91205cd5733ccd76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 17 Jun 2024 19:21:11 +0200 Subject: [PATCH 1/3] Refactor tests of newBearerTokenFromJSONBlob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make them table-driven where possible. Use testify. Signed-off-by: Miloslav Trmač --- docker/docker_client_test.go | 94 ++++++++++++++---------------------- 1 file changed, 35 insertions(+), 59 deletions(-) diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index a7d16ef7b..1db5c4364 100644 --- a/docker/docker_client_test.go +++ b/docker/docker_client_test.go @@ -91,72 +91,48 @@ func TestDockerCertDir(t *testing.T) { } } -func TestNewBearerTokenFromJsonBlob(t *testing.T) { - expected := &bearerToken{Token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)} - tokenBlob := []byte(`{"token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`) - token, err := newBearerTokenFromJSONBlob(tokenBlob) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - assertBearerTokensEqual(t, expected, token) -} - -func TestNewBearerAccessTokenFromJsonBlob(t *testing.T) { - expected := &bearerToken{Token: "IAmAToken", ExpiresIn: 100, IssuedAt: time.Unix(1514800802, 0)} - tokenBlob := []byte(`{"access_token":"IAmAToken","expires_in":100,"issued_at":"2018-01-01T10:00:02+00:00"}`) - token, err := newBearerTokenFromJSONBlob(tokenBlob) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - assertBearerTokensEqual(t, expected, token) -} - -func TestNewBearerTokenFromInvalidJsonBlob(t *testing.T) { - tokenBlob := []byte("IAmNotJson") - _, err := newBearerTokenFromJSONBlob(tokenBlob) - if err == nil { - t.Fatalf("unexpected an error unmarshaling JSON") - } -} - -func TestNewBearerTokenSmallExpiryFromJsonBlob(t *testing.T) { - expected := &bearerToken{Token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0)} - tokenBlob := []byte(`{"token":"IAmAToken","expires_in":1,"issued_at":"2018-01-01T10:00:02+00:00"}`) - token, err := newBearerTokenFromJSONBlob(tokenBlob) - if err != nil { - t.Fatalf("unexpected error: %v", err) +func TestNewBearerTokenFromJSONBlob(t *testing.T) { + for _, c := range []struct { + input string + expected *bearerToken // or nil if a failure is expected + }{ + { // Invalid JSON + input: "IAmNotJson", + expected: nil, + }, + { // "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)}, + }, + { // "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)}, + }, + { // 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)}, + }, + } { + token, err := newBearerTokenFromJSONBlob([]byte(c.input)) + if c.expected == nil { + 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.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) + } } - - assertBearerTokensEqual(t, expected, token) } -func TestNewBearerTokenIssuedAtZeroFromJsonBlob(t *testing.T) { +func TestNewBearerTokenFromJSONBlobIssuedAtZero(t *testing.T) { zeroTime := time.Time{}.Format(time.RFC3339) now := time.Now() tokenBlob := []byte(fmt.Sprintf(`{"token":"IAmAToken","expires_in":100,"issued_at":"%s"}`, zeroTime)) token, err := newBearerTokenFromJSONBlob(tokenBlob) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - if token.IssuedAt.Before(now) { - t.Fatalf("expected [%s] not to be before [%s]", token.IssuedAt, now) - } - -} - -func assertBearerTokensEqual(t *testing.T, expected, subject *bearerToken) { - if expected.Token != subject.Token { - t.Fatalf("expected [%s] to equal [%s], it did not", subject.Token, expected.Token) - } - if expected.ExpiresIn != subject.ExpiresIn { - t.Fatalf("expected [%d] to equal [%d], it did not", subject.ExpiresIn, expected.ExpiresIn) - } - if !expected.IssuedAt.Equal(subject.IssuedAt) { - t.Fatalf("expected [%s] to equal [%s], it did not", subject.IssuedAt, expected.IssuedAt) - } + require.NoError(t, err) + assert.False(t, token.IssuedAt.Before(now), "expected [%s] not to be before [%s]", token.IssuedAt, now) } func TestUserAgent(t *testing.T) { From b9806105bb523a77af4808889bee3ba4b8db79e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 17 Jun 2024 19:29:40 +0200 Subject: [PATCH 2/3] Turn newBearerTokenFromJSON into newBearerTokenFromHTTPResponseBody MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we have more context for error reporting. Should not change behavior. Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 22 ++++++++++------------ docker/docker_client_test.go | 25 ++++++++++++++++++++----- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/docker/docker_client.go b/docker/docker_client.go index 94cbcb1d9..44e66d6a5 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -147,7 +147,14 @@ const ( noAuth ) -func newBearerTokenFromJSONBlob(blob []byte) (*bearerToken, error) { +// 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 + } + token := new(bearerToken) if err := json.Unmarshal(blob, &token); err != nil { return nil, err @@ -827,12 +834,7 @@ func (c *dockerClient) getBearerTokenOAuth2(ctx context.Context, challenge chall return nil, err } - tokenBlob, err := iolimits.ReadAtMost(res.Body, iolimits.MaxAuthTokenBodySize) - if err != nil { - return nil, err - } - - return newBearerTokenFromJSONBlob(tokenBlob) + return newBearerTokenFromHTTPResponseBody(res) } func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge, @@ -878,12 +880,8 @@ func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge, if err := httpResponseToError(res, "Requesting bearer token"); err != nil { return nil, err } - tokenBlob, err := iolimits.ReadAtMost(res.Body, iolimits.MaxAuthTokenBodySize) - if err != nil { - return nil, err - } - return newBearerTokenFromJSONBlob(tokenBlob) + return newBearerTokenFromHTTPResponseBody(res) } // detectPropertiesHelper performs the work of detectProperties which executes diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index 1db5c4364..563358bbd 100644 --- a/docker/docker_client_test.go +++ b/docker/docker_client_test.go @@ -5,8 +5,10 @@ import ( "bytes" "context" "fmt" + "io" "net/http" "net/http/httptest" + "net/url" "path/filepath" "strings" "testing" @@ -91,7 +93,20 @@ func TestDockerCertDir(t *testing.T) { } } -func TestNewBearerTokenFromJSONBlob(t *testing.T) { +// testTokenHTTPResponse creates just enough of a *http.Response to work with newBearerTokenFromHTTPResponseBody. +func testTokenHTTPResponse(t *testing.T, body string) *http.Response { + requestURL, err := url.Parse("https://example.com/token") + require.NoError(t, err) + return &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte(body))), + Request: &http.Request{ + Method: "", + URL: requestURL, + }, + } +} + +func TestNewBearerTokenFromHTTPResponseBody(t *testing.T) { for _, c := range []struct { input string expected *bearerToken // or nil if a failure is expected @@ -113,7 +128,7 @@ func TestNewBearerTokenFromJSONBlob(t *testing.T) { expected: &bearerToken{Token: "IAmAToken", ExpiresIn: 60, IssuedAt: time.Unix(1514800802, 0)}, }, } { - token, err := newBearerTokenFromJSONBlob([]byte(c.input)) + token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, c.input)) if c.expected == nil { assert.Error(t, err, c.input) } else { @@ -126,11 +141,11 @@ func TestNewBearerTokenFromJSONBlob(t *testing.T) { } } -func TestNewBearerTokenFromJSONBlobIssuedAtZero(t *testing.T) { +func TestNewBearerTokenFromHTTPResponseBodyIssuedAtZero(t *testing.T) { zeroTime := time.Time{}.Format(time.RFC3339) now := time.Now() - tokenBlob := []byte(fmt.Sprintf(`{"token":"IAmAToken","expires_in":100,"issued_at":"%s"}`, zeroTime)) - token, err := newBearerTokenFromJSONBlob(tokenBlob) + tokenBlob := fmt.Sprintf(`{"token":"IAmAToken","expires_in":100,"issued_at":"%s"}`, zeroTime) + token, err := newBearerTokenFromHTTPResponseBody(testTokenHTTPResponse(t, string(tokenBlob))) require.NoError(t, err) assert.False(t, token.IssuedAt.Before(now), "expected [%s] not to be before [%s]", token.IssuedAt, now) } From 2044bb94120f76a6c958f1c68b1db8cf96a80b87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 17 Jun 2024 19:33:09 +0200 Subject: [PATCH 3/3] Include more data when we fail to fetch a token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docker/docker_client.go b/docker/docker_client.go index 44e66d6a5..6b65c52ed 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -157,7 +157,12 @@ func newBearerTokenFromHTTPResponseBody(res *http.Response) (*bearerToken, error token := new(bearerToken) if err := json.Unmarshal(blob, &token); err != nil { - return nil, err + 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) } if token.Token == "" { token.Token = token.AccessToken