Merge pull request #230 from jfrog/GH-225-fix-concurrency-issue

Switched to use Access API for checking expired token

CHANGELOG.md

@@ -1,3 +1,9 @@
+## 1.8.4 (December 9, 2024). Tested on Artifactory 7.98.10 with Vault v1.18.2 and OpenBao v2.0.0
+
+BUG FIXES:
+
+* Fix error when refreshing expired user token. Issue: [#225](https://github.com/jfrog/artifactory-secrets-plugin/issues/225) PR: [#230](https://github.com/jfrog/artifactory-secrets-plugin/pull/230)
+
 ## 1.8.3 (November 22, 2024)
 
 NOTES:

artifactory.go

@@ -128,7 +128,7 @@ func (e *TokenExpiredError) Error() string {
 	return "token has expired"
 }
 
-var expiredTokenRegex = regexp.MustCompile(`.*Invalid token, expired.*`)
+var invalidTokenRegex = regexp.MustCompile(`.*Invalid token, expired.*`)
 
 func (b *backend) CreateToken(config baseConfiguration, role artifactoryRole) (*createTokenResponse, error) {
 	if config.AccessToken == "" {
@@ -226,7 +226,7 @@ func (b *backend) CreateToken(config baseConfiguration, role artifactoryRole) (*
 			return nil, fmt.Errorf("could not create access token. Err: %v", err)
 		}
 
-		if resp.StatusCode == http.StatusUnauthorized && expiredTokenRegex.MatchString(errResp.String()) {
+		if resp.StatusCode == http.StatusUnauthorized && invalidTokenRegex.MatchString(errResp.String()) {
 			return nil, &TokenExpiredError{}
 		}
 
@@ -343,14 +343,14 @@ func (b *backend) useNewAccessAPI(config baseConfiguration) bool {
 }
 
 func (b *backend) refreshExpiredAccessToken(ctx context.Context, req *logical.Request, config *baseConfiguration, userTokenConfig *userTokenConfiguration, username string) error {
-	logger := b.Logger().With("func", "RefreshExpiredAccessToken")
+	logger := b.Logger().With("func", "refreshExpiredAccessToken")
 
 	// check if user access token is expired or not
 	// if so, refresh it with new tokens
-	logger.Debug("use checkVersion to see if access token is expired")
-	_, err := b.checkVersion("7.50.3", *config) // doesn't matter which version to check
+	logger.Debug("check if access token is expired by getting token itself")
+	err := b.getTokenByID(*config)
 	if err != nil {
-		logger.Debug("failed checkVersion", "err", err)
+		logger.Debug("failed to get Viewer role", "err", err)
 
 		if _, ok := err.(*TokenExpiredError); ok {
 			logger.Info("access token expired. Attempt to refresh using the refresh token.", "err", err)
@@ -414,6 +414,41 @@ func (b *backend) getVersion(config baseConfiguration) (version string, err erro
 	return systemVersion.Version, nil
 }
 
+func (b *backend) getTokenByID(config baseConfiguration) error {
+	logger := b.Logger().With("func", "getTokenByID")
+
+	logger.Debug("fetching Viewer role")
+
+	// '/me' is special value to get info about token itself
+	// https://jfrog.com/help/r/jfrog-rest-apis/get-token-by-id
+	resp, err := b.performArtifactoryGet(config, "/access/api/v1/tokens/me")
+	if err != nil {
+		logger.Error("error making get token request", "response", resp, "err", err)
+		return err
+	}
+
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		logger.Error("got non-200 status code", "statusCode", resp.StatusCode)
+
+		var errResp artifactoryErrorResponse
+		err := json.NewDecoder(resp.Body).Decode(&errResp)
+		if err != nil {
+			logger.Error("could not parse error response", "response", resp, "err", err)
+			return fmt.Errorf("could not get token. Err: %w", err)
+		}
+
+		if resp.StatusCode == http.StatusUnauthorized && invalidTokenRegex.MatchString(errResp.String()) {
+			return &TokenExpiredError{}
+		}
+
+		return fmt.Errorf("could not get the token: HTTP response %v", errResp.String())
+	}
+
+	return nil
+}
+
 // checkVersion will return a boolean and error to check compatibility before making an API call
 // -- This was formerly "checkSystemStatus" but that was hard-coded, that method now calls this one
 func (b *backend) checkVersion(ver string, config baseConfiguration) (compatible bool, err error) {
@@ -581,7 +616,7 @@ func (b *backend) getRootCert(config baseConfiguration) (cert *x509.Certificate,
 			return nil, fmt.Errorf("could not create access token. Err: %v", err)
 		}
 
-		if resp.StatusCode == http.StatusUnauthorized && tokenFailedValidationRegex.MatchString(errResp.String()) {
+		if resp.StatusCode == http.StatusUnauthorized && invalidTokenRegex.MatchString(errResp.String()) {
 			return nil, &TokenExpiredError{}
 		}
 

backend.go

@@ -100,7 +100,9 @@ func (b *backend) initialize(ctx context.Context, req *logical.InitializationReq
 		if err != nil {
 			return err
 		}
+		b.configMutex.Lock()
 		b.usernameProducer = up
+		b.configMutex.Unlock()
 	}
 
 	return nil

path_config_user_token.go

@@ -94,9 +94,6 @@ type userTokenConfiguration struct {
 }
 
 func (c *userTokenConfiguration) RefreshAccessToken(ctx context.Context, req *logical.Request, username string, b *backend, adminBaseConfig baseConfiguration) error {
-	b.configMutex.Lock()
-	defer b.configMutex.Unlock()
-
 	logger := b.Logger().With("func", "RefreshAccessToken")
 
 	if c.RefreshToken == "" {
@@ -271,8 +268,8 @@ func (b *backend) pathConfigUserTokenUpdate(ctx context.Context, req *logical.Re
 }
 
 func (b *backend) pathConfigUserTokenRead(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	b.configMutex.RLock()
-	defer b.configMutex.RUnlock()
+	b.configMutex.Lock()
+	defer b.configMutex.Unlock()
 
 	baseConfig := baseConfiguration{}
 

path_user_token_create.go

@@ -50,7 +50,7 @@ func (b *backend) pathUserTokenCreate() *framework.Path {
 			},
 			"ttl": {
 				Type:        framework.TypeDurationSecond,
-				Description: `Optional. Override the default TTL when issuing this access token. Cappaed at the smallest maximum TTL (system, mount, backend, request).`,
+				Description: `Optional. Override the default TTL when issuing this access token. Capped at the smallest maximum TTL (system, mount, backend, request).`,
 			},
 			"scope": {
 				Type:        framework.TypeString,
@@ -68,8 +68,10 @@ func (b *backend) pathUserTokenCreate() *framework.Path {
 }
 
 func (b *backend) pathUserTokenCreatePerform(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	b.configMutex.RLock()
-	defer b.configMutex.RUnlock()
+	b.configMutex.Lock()
+	defer b.configMutex.Unlock()
+
+	logger := b.Logger().With("func", "pathUserTokenCreatePerform")
 
 	baseConfig := baseConfiguration{}
 
@@ -126,8 +128,6 @@ func (b *backend) pathUserTokenCreatePerform(ctx context.Context, req *logical.R
 		RefreshToken:          userTokenConfig.RefreshToken,
 	}
 
-	logger := b.Logger().With("func", "pathUserTokenCreatePerform")
-
 	maxLeaseTTL := b.Backend.System().MaxLeaseTTL()
 	logger.Debug("initialize maxLeaseTTL to system value", "maxLeaseTTL", maxLeaseTTL)
 

test/concurrent_read.sh

@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+vault read artifactory/user_token/test &
+vault read artifactory/user_token/test &
+wait

test/expired.sh

@@ -0,0 +1,22 @@
+#!/usr/bin/env bash
+
+vault write artifactory/config/admin url=$JFROG_URL use_expiring_tokens=true max_ttl=14400 default_ttl=3600
+
+# create non-admin token
+# ensure there's a non-admin user named `test` in Artifactory first
+USER_TOKEN=$(curl -s -L "${JFROG_URL}/access/api/v1/tokens" -H 'Content-Type: application/json' -H "Authorization: Bearer ${JFROG_ACCESS_TOKEN}" --data-raw '{"grant_type":"client_credentials","username":"test","scope":"applied-permissions/user","refreshable":true,"audience":"*@*","expires_in":60,"force_revocable":false,"include_reference_token":false}')
+
+# create admin token
+# USER_TOKEN=$(curl -s -L "${JFROG_URL}/access/api/v1/tokens" -H 'Content-Type: application/json' -H "Authorization: Bearer ${JFROG_ACCESS_TOKEN}" --data-raw '{"grant_type":"client_credentials","username":"admin","scope":"applied-permissions/admin","refreshable":true,"audience":"*@*","expires_in":60,"force_revocable":false,"include_reference_token":false}')
+
+USER_ACCESS_TOKEN=$(echo ${USER_TOKEN} | jq -r ".access_token")
+echo "USER_ACCESS_TOKEN: ${USER_ACCESS_TOKEN}"
+
+USER_REFRESH_TOKEN=$(echo ${USER_TOKEN} | jq -r ".refresh_token")
+echo "USER_REFRESH_TOKEN: ${USER_REFRESH_TOKEN}"
+
+vault write artifactory/config/user_token access_token=${USER_ACCESS_TOKEN} refresh_token=${USER_REFRESH_TOKEN} refreshable=true use_expiring_tokens=true max_ttl=14400 default_ttl=3600
+vault read artifactory/config/user_token
+vault read artifactory/user_token/test
+
+date

test_utils.go

@@ -652,3 +652,10 @@ func mockArtifactoryUsageVersionRequests(version string) {
 		"http://myserver.com:80/artifactory/api/system/version",
 		httpmock.NewStringResponder(200, versionString))
 }
+
+func mockArtifactoryTokenRequest() {
+	httpmock.RegisterResponder(
+		http.MethodGet,
+		"http://myserver.com:80/access/api/v1/tokens/me",
+		httpmock.NewStringResponder(200, ""))
+}

ttl_test.go

@@ -122,6 +122,7 @@ func TestBackend_NoUserTokensMaxTTLUsesSystemMaxTTL(t *testing.T) {
 	defer httpmock.DeactivateAndReset()
 
 	mockArtifactoryUsageVersionRequests("")
+	mockArtifactoryTokenRequest()
 
 	httpmock.RegisterResponder(
 		http.MethodPost,
@@ -171,6 +172,7 @@ func TestBackend_UserTokenConfigMaxTTLUseSystem(t *testing.T) {
 	defer httpmock.DeactivateAndReset()
 
 	mockArtifactoryUsageVersionRequests("")
+	mockArtifactoryTokenRequest()
 
 	httpmock.RegisterResponder(
 		http.MethodPost,
@@ -205,6 +207,7 @@ func TestBackend_UserTokenConfigMaxTTLUseConfigMaxTTL(t *testing.T) {
 	defer httpmock.DeactivateAndReset()
 
 	mockArtifactoryUsageVersionRequests("")
+	mockArtifactoryTokenRequest()
 
 	httpmock.RegisterResponder(
 		http.MethodPost,
@@ -239,6 +242,7 @@ func TestBackend_UserTokenMaxTTLUseRequestTTL(t *testing.T) {
 	defer httpmock.DeactivateAndReset()
 
 	mockArtifactoryUsageVersionRequests("")
+	mockArtifactoryTokenRequest()
 
 	httpmock.RegisterResponder(
 		http.MethodPost,
@@ -273,6 +277,7 @@ func TestBackend_UserTokenMaxTTLEnforced(t *testing.T) {
 	defer httpmock.DeactivateAndReset()
 
 	mockArtifactoryUsageVersionRequests("")
+	mockArtifactoryTokenRequest()
 
 	httpmock.RegisterResponder(
 		http.MethodPost,
@@ -308,6 +313,7 @@ func TestBackend_UserTokenTTLRequest(t *testing.T) {
 	defer httpmock.DeactivateAndReset()
 
 	mockArtifactoryUsageVersionRequests("")
+	mockArtifactoryTokenRequest()
 
 	httpmock.RegisterResponder(
 		http.MethodPost,
@@ -339,6 +345,7 @@ func TestBackend_UserTokenDefaultTTL(t *testing.T) {
 	defer httpmock.DeactivateAndReset()
 
 	mockArtifactoryUsageVersionRequests("")
+	mockArtifactoryTokenRequest()
 
 	httpmock.RegisterResponder(
 		http.MethodPost,