fix: adjust IAM token expiration time (#209)

This commit changes the IAM, Container and VPC Instance authenticators slightly so that an IAM access token will be viewed as "expired" when the current time is within 10 seconds of the official expiration time. IOW, we'll expire the access token 10 secs earlier than the IAM server-computed expiration time. We're doing this to avoid a scenario where an IBM Cloud service receives a request along with an "almost expired" access token and then uses that token to perform downstream requests in a somewhat longer-running transaction and then the access token expires while that transaction is still active. Signed-off-by: Phil Adams <[email protected]>
IBM · Feb 27, 2024 · 0fdd924 · 0fdd924
1 parent a0d6466
commit 0fdd924
Show file tree

Hide file tree

Showing 5 changed files with 199 additions and 28 deletions.
diff --git a/.secrets.baseline b/.secrets.baseline
@@ -3,7 +3,7 @@
     "files": "go.sum|package-lock.json|^.secrets.baseline$",
     "lines": null
   },
-  "generated_at": "2024-02-22T22:43:25Z",
+  "generated_at": "2024-02-23T18:07:30Z",
   "plugins_used": [
     {
       "name": "AWSKeyDetector"
@@ -116,7 +116,7 @@
         "hashed_secret": "bc2f74c22f98f7b6ffbc2f67453dbfa99bce9a32",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 758,
+        "line_number": 765,
         "type": "Secret Keyword",
         "verified_result": null
       }
@@ -380,7 +380,7 @@
         "hashed_secret": "10ef99be8df801b05b5933e121e85385edf6b98a",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 613,
+        "line_number": 663,
         "type": "Secret Keyword",
         "verified_result": null
       }
@@ -516,7 +516,7 @@
         "hashed_secret": "8b142a91cfb6e617618ad437cedf74a6745f8926",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 130,
+        "line_number": 134,
         "type": "Secret Keyword",
         "verified_result": null
       }
@@ -582,23 +582,23 @@
         "hashed_secret": "7480f0b7140317bd82ade3c7a9526408304d5a7f",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 537,
+        "line_number": 603,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "6a0a3e8036180c23da91ede4f9d7bbfefd56e1a9",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 1096,
+        "line_number": 1162,
         "type": "Secret Keyword",
         "verified_result": null
       },
       {
         "hashed_secret": "32e8612d8ca77c7ea8374aa7918db8e5df9252ed",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 1118,
+        "line_number": 1184,
         "type": "Secret Keyword",
         "verified_result": null
       }
@@ -736,7 +736,7 @@
         "hashed_secret": "af83c79c5d4a8d171a2ca5aa132013f3020c518a",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 841,
+        "line_number": 887,
         "type": "Secret Keyword",
         "verified_result": null
       }

diff --git a/core/container_authenticator_test.go b/core/container_authenticator_test.go
@@ -2,7 +2,7 @@
 
 package core
 
-// (C) Copyright IBM Corp. 2021.
+// (C) Copyright IBM Corp. 2021, 2024.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -375,7 +375,7 @@ func TestContainerAuthGetTokenSuccess(t *testing.T) {
 
 	// Call GetToken() again and verify that we get the cached value.
 	// Note: we'll Set Scope so that if the IAM operation is actually called again,
-	// we'll receive the second access token.  We don't want the IAM operation called again yet.
+	// we'll receive the second access token. We don't want the IAM operation called again yet.
 	auth.Scope = "send-second-token"
 	accessToken, err = auth.GetToken()
 	assert.Nil(t, err)
@@ -392,6 +392,56 @@ func TestContainerAuthGetTokenSuccess(t *testing.T) {
 	assert.Equal(t, containerAuthTestAccessToken2, auth.getTokenData().AccessToken)
 }
 
+func TestContainerAuthGetTokenSuccess10SecWindow(t *testing.T) {
+	GetLogger().SetLogLevel(containerAuthTestLogLevel)
+
+	server := startMockIAMServer(t)
+	defer server.Close()
+
+	auth := &ContainerAuthenticator{
+		CRTokenFilename: containerAuthMockCRTokenFile,
+		IAMProfileName:  containerAuthMockIAMProfileName,
+		URL:             server.URL,
+	}
+	err := auth.Validate()
+	assert.Nil(t, err)
+
+	// Verify that we initially have no token data cached on the authenticator.
+	assert.Nil(t, auth.getTokenData())
+
+	// Force the first fetch and verify we got the first access token.
+	var accessToken string
+	accessToken, err = auth.GetToken()
+	assert.Nil(t, err)
+
+	// Verify that the access token was returned by GetToken() and also
+	// stored in the authenticator's tokenData field as well.
+	assert.NotNil(t, auth.getTokenData())
+	assert.Equal(t, containerAuthTestAccessToken1, accessToken)
+	assert.Equal(t, containerAuthTestAccessToken1, auth.getTokenData().AccessToken)
+
+	// Call GetToken() again and verify that we get the cached value.
+	// Note: we'll Set Scope so that if the IAM operation is actually called again,
+	// we'll receive the second access token. We don't want the IAM operation called again yet.
+	auth.Scope = "send-second-token"
+	accessToken, err = auth.GetToken()
+	assert.Nil(t, err)
+	assert.Equal(t, containerAuthTestAccessToken1, accessToken)
+
+	// Force expiration and verify that GetToken() fetched the second access token.
+	// We'll set expiration to be current-time + <iamExpirationWindow> (10 secs),
+	// to test the scenario where we should refresh the token when we are within 10 secs
+	// of expiration.
+	auth.getTokenData().Expiration = GetCurrentTime() + iamExpirationWindow
+	auth.IAMProfileName = ""
+	auth.IAMProfileID = containerAuthMockIAMProfileID
+	accessToken, err = auth.GetToken()
+	assert.Nil(t, err)
+	assert.NotNil(t, auth.getTokenData())
+	assert.Equal(t, containerAuthTestAccessToken2, accessToken)
+	assert.Equal(t, containerAuthTestAccessToken2, auth.getTokenData().AccessToken)
+}
+
 func TestContainerAuthRequestTokenSuccess(t *testing.T) {
 	GetLogger().SetLogLevel(containerAuthTestLogLevel)
 

diff --git a/core/iam_authenticator.go b/core/iam_authenticator.go
@@ -1,6 +1,6 @@
 package core
 
-// (C) Copyright IBM Corp. 2019, 2021.
+// (C) Copyright IBM Corp. 2019, 2024.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -93,6 +93,10 @@ const (
 	iamAuthOperationPathGetToken  = "/identity/token"
 	iamAuthGrantTypeApiKey        = "urn:ibm:params:oauth:grant-type:apikey" // #nosec G101
 	iamAuthGrantTypeRefreshToken  = "refresh_token"                          // #nosec G101
+
+	// The number of seconds before the IAM server-assigned (official) expiration time
+	// when we'll treat an otherwise valid access token as "expired".
+	iamExpirationWindow = 10
 )
 
 // IamAuthenticatorBuilder is used to construct an IamAuthenticator instance.
@@ -292,7 +296,7 @@ func (authenticator *IamAuthenticator) setTokenData(tokenData *iamTokenData) {
 //
 // Ensures that the ApiKey and RefreshToken properties are mutually exclusive,
 // and that the ClientId and ClientSecret properties are mutually inclusive.
-func (this *IamAuthenticator) Validate() error {
+func (authenticator *IamAuthenticator) Validate() error {
 
 	// The user should specify at least one of ApiKey or RefreshToken.
 	// Note: We'll allow both ApiKey and RefreshToken to be specified,
@@ -311,25 +315,25 @@ func (this *IamAuthenticator) Validate() error {
 	// instance of the authenticator doesn't become invalidated simply through
 	// normal use.
 	//
-	if this.ApiKey == "" && this.RefreshToken == "" {
+	if authenticator.ApiKey == "" && authenticator.RefreshToken == "" {
 		return fmt.Errorf(ERRORMSG_EXCLUSIVE_PROPS_ERROR, "ApiKey", "RefreshToken")
 	}
 
-	if this.ApiKey != "" && HasBadFirstOrLastChar(this.ApiKey) {
+	if authenticator.ApiKey != "" && HasBadFirstOrLastChar(authenticator.ApiKey) {
 		return fmt.Errorf(ERRORMSG_PROP_INVALID, "ApiKey")
 	}
 
 	// Validate ClientId and ClientSecret.
 	// Either both or neither should be specified.
-	if this.ClientId == "" && this.ClientSecret == "" {
+	if authenticator.ClientId == "" && authenticator.ClientSecret == "" {
 		// Do nothing as this is the valid scenario.
 	} else {
 		// Since it is NOT the case that both properties are empty, make sure BOTH are specified.
-		if this.ClientId == "" {
+		if authenticator.ClientId == "" {
 			return fmt.Errorf(ERRORMSG_PROP_MISSING, "ClientId")
 		}
 
-		if this.ClientSecret == "" {
+		if authenticator.ClientSecret == "" {
 			return fmt.Errorf(ERRORMSG_PROP_MISSING, "ClientSecret")
 		}
 	}
@@ -533,23 +537,28 @@ func newIamTokenData(tokenResponse *IamTokenServerResponse) (*iamTokenData, erro
 }
 
 // isTokenValid: returns true iff the IamTokenData instance represents a valid (non-expired) access token.
-func (this *iamTokenData) isTokenValid() bool {
-	if this.AccessToken != "" && GetCurrentTime() < this.Expiration {
+func (td *iamTokenData) isTokenValid() bool {
+	// We'll use "exp - 10" so that we'll treat an otherwise valid (unexpired) access token
+	// to be expired if we're within 10 seconds of its TTL.
+	// This is because some IBM Cloud services reject valid access tokens with a short
+	// TTL remaining (TTL < 5 secs) because the access token might be used in a longer-running
+	// transaction and could potentially expire in the middle of the transaction.
+	if td.AccessToken != "" && GetCurrentTime() < (td.Expiration-iamExpirationWindow) {
 		return true
 	}
 	return false
 }
 
-// needsRefresh: synchronously returns true iff the currently stored access token should be refreshed. This method also
-// updates the refresh time if it determines the token needs refreshed to prevent other threads from
-// making multiple refresh calls.
-func (this *iamTokenData) needsRefresh() bool {
+// needsRefresh: synchronously returns true iff the currently stored access token should be refreshed.
+// This method also updates the refresh time if it determines the token needs to be refreshed
+// to prevent other threads from making multiple refresh calls.
+func (td *iamTokenData) needsRefresh() bool {
 	iamNeedsRefreshMutex.Lock()
 	defer iamNeedsRefreshMutex.Unlock()
 
 	// Advance refresh by one minute
-	if this.RefreshTime >= 0 && GetCurrentTime() > this.RefreshTime {
-		this.RefreshTime = GetCurrentTime() + 60
+	if td.RefreshTime >= 0 && GetCurrentTime() > td.RefreshTime {
+		td.RefreshTime = GetCurrentTime() + 60
 		return true
 	}
 

diff --git a/core/iam_authenticator_test.go b/core/iam_authenticator_test.go
@@ -2,7 +2,7 @@
 
 package core
 
-// (C) Copyright IBM Corp. 2019, 2021.
+// (C) Copyright IBM Corp. 2019, 2024.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -388,7 +388,7 @@ func TestIamGetTokenSuccess(t *testing.T) {
 	assert.Nil(t, authenticator.synchronizedRequestToken())
 
 	// Force expiration and verify that we got the second access token.
-	authenticator.getTokenData().Expiration = GetCurrentTime() - 3600
+	authenticator.getTokenData().Expiration = GetCurrentTime() - 1
 	authenticator.ClientId = iamAuthMockClientID
 	authenticator.ClientSecret = iamAuthMockClientSecret
 	_, err = authenticator.GetToken()
@@ -403,6 +403,68 @@ func TestIamGetTokenSuccess(t *testing.T) {
 	assert.Equal(t, iamAuthTestRefreshToken, tokenResponse.RefreshToken)
 }
 
+func TestIamGetTokenSuccess10SecWindow(t *testing.T) {
+	GetLogger().SetLogLevel(iamAuthTestLogLevel)
+
+	firstCall := true
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		err := r.ParseForm()
+		assert.Nil(t, err)
+		assert.Len(t, r.Form["apikey"], 1)
+		assert.Len(t, r.Form["grant_type"], 1)
+		assert.Len(t, r.Form["response_type"], 1)
+		assert.Empty(t, r.Form["scope"])
+
+		w.WriteHeader(http.StatusOK)
+		expiration := GetCurrentTime() + 3600
+		if firstCall {
+			fmt.Fprintf(w, `{
+				"access_token": "%s",
+				"token_type": "Bearer",
+				"expires_in": 3600,
+				"expiration": %d,
+				"refresh_token": "%s"
+			}`, iamAuthTestAccessToken1, expiration, iamAuthTestRefreshToken)
+			firstCall = false
+			_, _, ok := r.BasicAuth()
+			assert.False(t, ok)
+		} else {
+			fmt.Fprintf(w, `{
+				"access_token": "%s",
+				"token_type": "Bearer",
+				"expires_in": 3600,
+				"expiration": %d,
+				"refresh_token": "%s"
+			}`, iamAuthTestAccessToken2, expiration, iamAuthTestRefreshToken)
+		}
+	}))
+	defer server.Close()
+
+	authenticator, err := NewIamAuthenticatorBuilder().
+		SetApiKey(iamAuthMockApiKey).
+		SetURL(server.URL).
+		Build()
+	assert.Nil(t, err)
+	assert.NotNil(t, authenticator)
+	assert.Nil(t, authenticator.getTokenData())
+
+	// Force the first fetch and verify we got the first access token.
+	token, err := authenticator.GetToken()
+	assert.Nil(t, err)
+	assert.Equal(t, iamAuthTestAccessToken1, token)
+	assert.NotNil(t, authenticator.getTokenData())
+
+	// Force expiration and verify that we got the second access token.
+	// We'll set expiration to be current-time + <iamExpirationWindow> (10 secs),
+	// to test the scenario where we should refresh the token when we are within 10 secs
+	// of expiration.
+	authenticator.getTokenData().Expiration = GetCurrentTime() + iamExpirationWindow
+	_, err = authenticator.GetToken()
+	assert.Nil(t, err)
+	assert.NotNil(t, authenticator.getTokenData())
+	assert.Equal(t, iamAuthTestAccessToken2, authenticator.getTokenData().AccessToken)
+}
+
 func TestIamGetTokenSuccessRT(t *testing.T) {
 	GetLogger().SetLogLevel(iamAuthTestLogLevel)
 
@@ -442,6 +504,10 @@ func TestIamGetTokenSuccessRT(t *testing.T) {
 				"expiration": %d,
 				"refresh_token": "%s"
 			}`, iamAuthTestAccessToken2, expiration, newRefreshToken)
+			username, password, ok := r.BasicAuth()
+			assert.True(t, ok)
+			assert.Equal(t, iamAuthMockClientID, username)
+			assert.Equal(t, iamAuthMockClientSecret, password)
 		}
 	}))
 	defer server.Close()