Fix the clock skew calculation in auth tokens for IAM-based authentic…

…ation with AWS RDS (#5119)

* Fix the clock skew calculation in auth tokens for AWS RDS

Signed-off-by: Agustín Martínez Fayó <[email protected]>

* - Rename isExpired() function to shouldRotate() and add a comment
- Use the newly added clockSkew constant

Signed-off-by: Agustín Martínez Fayó <[email protected]>

---------

Signed-off-by: Agustín Martínez Fayó <[email protected]>
Co-authored-by: Andrew Harding <[email protected]>

pkg/server/datastore/sqldriver/awsrds/auth_token.go

@@ -13,19 +13,18 @@ import (
 	"github.com/aws/aws-sdk-go-v2/feature/rds/auth"
 )
 
-const iso8601BasicFormat = "20060102T150405Z"
+const (
+	iso8601BasicFormat = "20060102T150405Z"
+	clockSkew          = time.Minute // Make sure that the authentication token is valid for one more minute.
+)
 
 type authTokenBuilder interface {
 	buildAuthToken(ctx context.Context, endpoint string, region string, dbUser string, creds aws.CredentialsProvider, optFns ...func(options *auth.BuildAuthTokenOptions)) (string, error)
 }
 
-type tokenGetter interface {
-	getAuthToken(ctx context.Context, params *Config, tokenBuilder authTokenBuilder) (string, error)
-}
-
 type authToken struct {
-	token     string
-	expiresAt time.Time
+	cachedToken string
+	expiresAt   time.Time
 }
 
 func (a *authToken) getAuthToken(ctx context.Context, config *Config, tokenBuilder authTokenBuilder) (string, error) {
@@ -37,8 +36,8 @@ func (a *authToken) getAuthToken(ctx context.Context, config *Config, tokenBuild
 		return "", errors.New("missing token builder")
 	}
 
-	if !a.isExpired() {
-		return a.token, nil
+	if !a.shouldRotate() {
+		return a.cachedToken, nil
 	}
 
 	awsClientConfig, err := newAWSClientConfig(ctx, config)
@@ -79,14 +78,18 @@ func (a *authToken) getAuthToken(ctx context.Context, config *Config, tokenBuild
 	if err != nil {
 		return "", fmt.Errorf("failed to parse X-Amz-Expires duration: %w", err)
 	}
-	a.token = authenticationToken
+	a.cachedToken = authenticationToken
 	a.expiresAt = dateTime.Add(durationTime)
 	return authenticationToken, nil
 }
 
-func (a *authToken) isExpired() bool {
-	clockSkew := time.Minute // Make sure that the authentication token is valid for one more minute.
-	return nowFunc().Add(-clockSkew).Sub(a.expiresAt) >= 0
+// shouldRotate returns true if the cached token is either expired or is
+// expiring soon. This means that this function will return true also if the
+// token is still valid but should be rotated because it's expiring soon. The
+// time window that establish when a cached token should be rotated even if it's
+// still valid is adjusted by a clock skew, defined in the clockSkew constant.
+func (a *authToken) shouldRotate() bool {
+	return nowFunc().Add(clockSkew).Sub(a.expiresAt) >= 0
 }
 
 type awsTokenBuilder struct{}

pkg/server/datastore/sqldriver/awsrds/awsrds.go

@@ -67,7 +67,7 @@ func (c *Config) getConnStringWithPassword(password string) (string, error) {
 	}
 }
 
-type tokens map[string]tokenGetter
+type tokens map[string]*authToken
 
 // sqlDriverWrapper is a wrapper for SQL drivers, adding IAM authentication.
 type sqlDriverWrapper struct {

pkg/server/datastore/sqldriver/awsrds/awsrds_test.go

@@ -270,11 +270,12 @@ func TestCacheToken(t *testing.T) {
 	dsn, err := config.FormatDSN()
 	require.NoError(t, err)
 
-	now := time.Now().UTC()
-	nowString := now.Format(iso8601BasicFormat)
+	initialTime := time.Now().UTC()
+	nowString := initialTime.Format(iso8601BasicFormat)
+	ttl := 900
 
 	// Set a first token to be always returned by the token builder.
-	firstToken := fmt.Sprintf("X-Amz-Date=%s&X-Amz-Expires=900&X-Amz-Signature=first-token", nowString)
+	firstToken := fmt.Sprintf("X-Amz-Date=%s&X-Amz-Expires=%d&X-Amz-Signature=first-token", nowString, ttl)
 	fakeSQLDriverWrapper.tokenBuilder = &fakeTokenBuilder{
 		authToken: firstToken,
 	}
@@ -299,11 +300,15 @@ func TestCacheToken(t *testing.T) {
 	// token (not expired) that we can use. For that, we start by setting a new
 	// token that will be returned by the token builder when getAWSAuthToken is
 	// called.
-	newToken := fmt.Sprintf("X-Amz-Date=%s&X-Amz-Expires=900&X-Amz-Signature=second-token", nowString)
+
+	newToken := fmt.Sprintf("X-Amz-Date=%s&X-Amz-Expires=%d&X-Amz-Signature=second-token", nowString, ttl)
 	fakeSQLDriverWrapper.tokenBuilder = &fakeTokenBuilder{
 		authToken: newToken,
 	}
 
+	// Advance the clock just a few seconds.
+	nowFunc = func() time.Time { return initialTime.Add(time.Second * 15) }
+
 	// Call Open again, the cached token should be used.
 	db, err = gorm.Open(fakeSQLDriverName, dsn)
 	require.NoError(t, err)
@@ -318,8 +323,14 @@ func TestCacheToken(t *testing.T) {
 
 	// We will now make firstToken to expire, so we can test that the token
 	// builder is called to get a new token when the current token has expired.
-	// For that, we advance the clock one hour.
-	nowFunc = func() time.Time { return now.Add(time.Hour) }
+	// For that, we advance the clock the number of seconds of the ttl of the
+	// token.
+	newTime := initialTime.Add(time.Second * time.Duration(ttl))
+
+	// nowFunc will subtract the clock skew from the new time, to make sure
+	// that we get a new token even if it's not expired but it's within the
+	// clock skew period.
+	nowFunc = func() time.Time { return newTime.Add(-clockSkew) }
 
 	// Call Open again, the new token should be used.
 	db, err = gorm.Open(fakeSQLDriverName, dsn)