Bug in bearer token expiration check

[vlad-ivanov-name]

Bearer tokens expire after a while. Tokens also have an issue timestamp. Sometimes, that timestamp is zero, in which case buildkit takes current time as a reference point.

However, it fails to reliably detect when the timestamp is zero. It's using golang time.Time.IsZero() function, which checks whether the time is January 1, year 1, 00:00:00 UTC, but the timestamp is converted from Unix time. Therefore, 01-01-1970 is compared to 01-01-0001, which always yields false, and results in expiry time being recorded as time before issue timestamp, always at 01-01-0001.

buildkit/util/resolver/authorizer.go

Line 336 in 3187d2d

if issuedAt.IsZero() {

buildkit/util/resolver/authorizer.go

Line 359 in 3187d2d

issuedAt, expires = time.Unix(resp.IssuedAt, 0), int(resp.ExpiresIn)

Later in the code, there's a condition that reuses cached tokens if expiry time IsZero OR if the expiry time has passed. Because of the bug before, IsZero always returns true, making buildkit use expired tokens.

buildkit/util/resolver/authorizer.go

Lines 303 to 305 in 3187d2d

	if r.expires.IsZero() || r.expires.After(time.Now()) {
	return r, nil
	}

This results in errors, e. g. 401 Unauthorized when using Google Artifact Registry

Example patch:

From c5a8c767ef838abb689b241bb3ffc13486bfe49d Mon Sep 17 00:00:00 2001
From: Vlad Ivanov <[email protected]>
Date: Thu, 6 Apr 2023 16:54:51 +0200
Subject: [PATCH] Fix token issue time check

---
 util/resolver/authorizer.go | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/util/resolver/authorizer.go b/util/resolver/authorizer.go
index 6a4140d68b1..f499c697d9d 100644
--- a/util/resolver/authorizer.go
+++ b/util/resolver/authorizer.go
@@ -324,6 +324,11 @@ func (ah *authHandler) doBearerAuth(ctx context.Context, sm *session.Manager, g
 	return r.token, nil
 }
 
+func isTimeZero(t time.Time) bool {
+	unixTimeStart := time.Unix(0, 0).UTC()
+	return t.Equal(unixTimeStart) || t.IsZero()
+}
+
 func (ah *authHandler) fetchToken(ctx context.Context, sm *session.Manager, g session.Group, to auth.TokenOptions) (r *authResult, err error) {
 	var issuedAt time.Time
 	var expires int
@@ -333,7 +338,7 @@ func (ah *authHandler) fetchToken(ctx context.Context, sm *session.Manager, g se
 
 		if err == nil {
 			r = &authResult{token: token}
-			if issuedAt.IsZero() {
+			if isTimeZero(issuedAt) {
 				issuedAt = time.Now()
 			}
 			if exp := issuedAt.Add(time.Duration(float64(expires)*0.9) * time.Second); time.Now().Before(exp) {

[tonistiigi]

Good catch. I wonder if a better patch would be to wrap the Unix() with a special case that handles the zero value. Feel free to open PR.

[vlad-ivanov-name]

Good point. I opened a PR.

I'm still getting sporadic 401s with this patch, but the frequency appears to be reduced significantly. I'm now running docker with additional logging in buildkit authorizer to hopefully catch the remaining bit.