From 4d173e93491c04727321ead2eb3a84e2bf5cf7d5 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Thu, 31 Oct 2024 09:10:17 -0700 Subject: [PATCH 1/5] Upgrade to Go 1.22 --- .github/workflows/acceptance-tests.yml | 2 +- .github/workflows/release.yml | 2 +- go.mod | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/workflows/acceptance-tests.yml b/.github/workflows/acceptance-tests.yml index f6c06d5..e526804 100644 --- a/.github/workflows/acceptance-tests.yml +++ b/.github/workflows/acceptance-tests.yml @@ -31,7 +31,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: 1.21 + go-version: 1.22 - name: Install Helm uses: azure/setup-helm@v4.2.0 - name: Install GoReleaser diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 483a67a..29897d4 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -19,7 +19,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: 1.21 + go-version: 1.22 - name: Import GPG key id: import_gpg uses: crazy-max/ghaction-import-gpg@v6 diff --git a/go.mod b/go.mod index 39b38bc..04c4fc1 100644 --- a/go.mod +++ b/go.mod @@ -1,7 +1,6 @@ module github.com/jfrog/vault-plugin-secrets-artifactory -go 1.21 -toolchain go1.22.5 +go 1.22 require ( github.com/golang-jwt/jwt/v4 v4.5.0 From 42b8c2a9f1fa20ecb32f555de804d59fdc42a5cd Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Wed, 6 Nov 2024 10:57:02 -0800 Subject: [PATCH 2/5] Update GoReleaser config for deprecated key --- .goreleaser.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.goreleaser.yaml b/.goreleaser.yaml index ec8a346..9bd0f06 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -32,7 +32,7 @@ builds: goarch: '386' binary: artifactory-secrets-plugin snapshot: - name_template: '{{ .Version }}' + version_template: '{{ .Version }}' archives: - format: binary checksum: From 974c55739dea1b57fb08da55e2fa06ad2676445a Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Wed, 6 Nov 2024 13:43:40 -0800 Subject: [PATCH 3/5] Handle expired access token when reading user config And attempt to refresh the token Split off code for refreshing token from create token. Move code that determine whether to use new or old API to earlier when config is being created. The value is then stored in the config for later use. Use token ID only when revoking token More logging with func context --- artifactory.go | 420 +++++++++++++++++++++++---------- path_config.go | 25 +- path_config_rotate.go | 2 +- path_config_rotate_test.go | 2 +- path_config_test.go | 4 +- path_config_user_token.go | 62 ++++- path_config_user_token_test.go | 12 +- path_user_token_create.go | 35 +-- secret_access_token.go | 3 +- test_utils.go | 16 +- ttl_test.go | 2 +- 11 files changed, 400 insertions(+), 183 deletions(-) diff --git a/artifactory.go b/artifactory.go index 154f786..bc6599b 100644 --- a/artifactory.go +++ b/artifactory.go @@ -2,6 +2,7 @@ package artifactory import ( "bytes" + "context" "crypto/x509" "encoding/base64" "encoding/json" @@ -13,11 +14,11 @@ import ( "net/url" "regexp" "strings" - "time" jwt "github.com/golang-jwt/jwt/v4" "github.com/hashicorp/go-version" "github.com/hashicorp/vault/sdk/helper/template" + "github.com/hashicorp/vault/sdk/logical" "github.com/samber/lo" ) @@ -34,6 +35,7 @@ type baseConfiguration struct { ArtifactoryURL string `json:"artifactory_url"` UseExpiringTokens bool `json:"use_expiring_tokens,omitempty"` ForceRevocable *bool `json:"force_revocable,omitempty"` + UseNewAccessAPI bool `json:"use_new_access_api,omitempty"` } type errorResponse struct { @@ -42,7 +44,7 @@ type errorResponse struct { Detail string `json:"detail"` } -func (b *backend) RevokeToken(config baseConfiguration, tokenId, accessToken string) error { +func (b *backend) RevokeToken(config baseConfiguration, tokenId string) error { if config.AccessToken == "" { return fmt.Errorf("empty access token not allowed") } @@ -56,18 +58,23 @@ func (b *backend) RevokeToken(config baseConfiguration, tokenId, accessToken str } var resp *http.Response - - if b.useNewAccessAPI(config) { + if config.UseNewAccessAPI { resp, err = b.performArtifactoryDelete(config, "/access/api/v1/tokens/"+tokenId) if err != nil { logger.Error("error deleting access token", "tokenId", tokenId, "response", resp, "err", err) return err } } else { + path, err := url.JoinPath(u.Path, "/artifactory/api/security/token/revoke") + if err != nil { + logger.Error("error joining url path", "err", err) + return err + } + values := url.Values{} - values.Set("token", accessToken) + values.Set("token_id", tokenId) - resp, err = b.performArtifactoryPost(config, u.Path+"/artifactory/api/security/token/revoke", values) + resp, err = b.performArtifactoryPost(config, path, values) if err != nil { logger.Error("error deleting token", "tokenId", tokenId, "response", resp, "err", err) return err @@ -83,7 +90,7 @@ func (b *backend) RevokeToken(config baseConfiguration, tokenId, accessToken str return fmt.Errorf("could not parse response body. Err: %v", err) } logger.Error("revokenToken got non-200 status code", "statusCode", resp.StatusCode, "body", string(body)) - return fmt.Errorf("could not revoke tokenID: %v - HTTP response %v", tokenId, body) + return fmt.Errorf("could not revoke tokenID: %v - HTTP response %v", tokenId, string(body)) } return nil @@ -102,17 +109,32 @@ type CreateTokenRequest struct { RefreshToken string `json:"refresh_token,omitempty"` } -type createTokenErrorResponse struct { +type artifactoryErrorResponse struct { Errors []errorResponse `json:"errors"` } +func (r artifactoryErrorResponse) String() string { + return lo.Reduce(r.Errors, func(agg string, e errorResponse, _ int) string { + if agg == "" { + return e.Message + } + return fmt.Sprintf("%s, %s", agg, e.Message) + }, "") +} + type TokenExpiredError struct{} func (e *TokenExpiredError) Error() string { return "token has expired" } +var expiredTokenRegex = regexp.MustCompile(`.*Invalid token, expired.*`) + func (b *backend) CreateToken(config baseConfiguration, role artifactoryRole) (*createTokenResponse, error) { + if config.AccessToken == "" { + return nil, fmt.Errorf("empty access token not allowed") + } + request := CreateTokenRequest{ GrantType: role.GrantType, Username: role.Username, @@ -124,36 +146,11 @@ func (b *backend) CreateToken(config baseConfiguration, role artifactoryRole) (* RefreshToken: role.RefreshToken, } - return b.createToken(config, role.ExpiresIn, request) -} - -func (b *backend) RefreshToken(config baseConfiguration, role artifactoryRole) (*createTokenResponse, error) { - if config.AccessToken == "" { - return nil, fmt.Errorf("empty access token not allowed") - } - - if role.RefreshToken == "" { - return nil, fmt.Errorf("no refresh token supplied") - } - - request := CreateTokenRequest{ - GrantType: grantTypeRefreshToken, - RefreshToken: role.RefreshToken, - } - - return b.createToken(config, role.ExpiresIn, request) -} - -func (b *backend) createToken(config baseConfiguration, expiresIn time.Duration, request CreateTokenRequest) (*createTokenResponse, error) { - if config.AccessToken == "" { - return nil, fmt.Errorf("empty access token not allowed") - } - - if request.GrantType == "client_credentials" && len(request.Username) == 0 { + if request.GrantType == grantTypeClientCredentials && len(request.Username) == 0 { return nil, fmt.Errorf("empty username not allowed, possibly a template error") } - logger := b.Logger().With("func", "createToken") + logger := b.Logger().With("func", "CreateToken") // Artifactory will not let you revoke a token that has an expiry unless it also meets // criteria that can only be set in its configuration file. The version of Artifactory @@ -161,8 +158,14 @@ func (b *backend) createToken(config baseConfiguration, expiresIn time.Duration, // but the token is still usable even after it's deleted. See RTFACT-15293. request.ExpiresIn = 0 // never expires - if config.UseExpiringTokens && b.supportForceRevocable(config) && expiresIn > 0 { - request.ExpiresIn = int64(expiresIn.Seconds()) + supportForceRevocable, err := b.supportForceRevocable(config) + if err != nil { + logger.Error("failed to determine if force_revocable is supported", "err", err) + return nil, err + } + + if config.UseExpiringTokens && supportForceRevocable && role.ExpiresIn > 0 { + request.ExpiresIn = int64(role.ExpiresIn.Seconds()) if config.ForceRevocable != nil { request.ForceRevocable = *config.ForceRevocable } else { @@ -177,60 +180,142 @@ func (b *backend) createToken(config baseConfiguration, expiresIn time.Duration, path := "" - if b.useNewAccessAPI(config) { + var resp *http.Response + var createErr error + + if config.UseNewAccessAPI { path = "/access/api/v1/tokens" + + jsonReq, err := json.Marshal(request) + if err != nil { + return nil, err + } + + resp, createErr = b.performArtifactoryPostWithJSON(config, path, jsonReq) } else { - path = u.Path + "/artifactory/api/security/token" - } + path, err = url.JoinPath(u.Path, "/artifactory/api/security/token") + if err != nil { + logger.Error("error joining url path", "err", err) + return nil, err + } - jsonReq, err := json.Marshal(request) - if err != nil { - return nil, err + values := url.Values{ + "grant_type": []string{grantTypeClientCredentials}, + "username": []string{request.Username}, + "scope": []string{request.Scope}, + "expires_in": []string{fmt.Sprintf("%d", request.ExpiresIn)}, + "refreshable": []string{fmt.Sprintf("%t", request.Refreshable)}, + "audience": []string{request.Audience}, + } + + resp, createErr = b.performArtifactoryPost(config, path, values) } - resp, err := b.performArtifactoryPostWithJSON(config, path, jsonReq) - if err != nil { - logger.Error("error making token request", "response", resp, "err", err) - return nil, err + if createErr != nil { + logger.Error("error making token request", "response", resp, "err", createErr) + return nil, createErr } //noinspection GoUnhandledErrorResult defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - e := fmt.Errorf("could not create access token: HTTP response %v", resp.StatusCode) - - if resp.StatusCode == http.StatusUnauthorized { - var errResp createTokenErrorResponse - err := json.NewDecoder(resp.Body).Decode(&errResp) - if err != nil { - logger.Error("could not parse error response", "response", resp, "err", err) - return nil, fmt.Errorf("could not create access token. Err: %v", err) - } + 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 nil, fmt.Errorf("could not create access token. Err: %v", err) + } - errMessages := lo.Reduce(errResp.Errors, func(agg string, e errorResponse, _ int) string { - return fmt.Sprintf("%s, %s", agg, e.Message) - }, "") + if resp.StatusCode == http.StatusUnauthorized && expiredTokenRegex.MatchString(errResp.String()) { + return nil, &TokenExpiredError{} + } - expiredTokenRe := regexp.MustCompile(`.*Invalid token, expired.*`) - if expiredTokenRe.MatchString(errMessages) { - return nil, &TokenExpiredError{} - } + logger.Error("got non-200 status code", "statusCode", resp.StatusCode, "message", errResp.String()) + return nil, fmt.Errorf("could not create access token: %s", errResp) + } + + var createdToken createTokenResponse + if err := json.NewDecoder(resp.Body).Decode(&createdToken); err != nil { + logger.Error("could not parse response", "response", resp, "err", err) + return nil, fmt.Errorf("could not create access token. Err: %v", err) + } + + return &createdToken, nil +} + +func (b *backend) RefreshToken(config baseConfiguration, refreshToken string) (*createTokenResponse, error) { + if config.AccessToken == "" { + return nil, fmt.Errorf("empty access token not allowed") + } + + if refreshToken == "" { + return nil, fmt.Errorf("no refresh token supplied") + } + + logger := b.Logger().With("func", "RefreshToken") + + u, err := url.Parse(config.ArtifactoryURL) + if err != nil { + logger.Error("could not parse artifactory url", "url", config.ArtifactoryURL, "err", err) + return nil, err + } + + request := CreateTokenRequest{ + GrantType: grantTypeRefreshToken, + RefreshToken: refreshToken, + } + + var resp *http.Response + var refreshErr error + + if config.UseNewAccessAPI { + jsonReq, err := json.Marshal(request) + if err != nil { + return nil, err } - body, err := io.ReadAll(resp.Body) + resp, refreshErr = b.performArtifactoryPostWithJSON(config, "/access/api/v1/tokens", jsonReq) + } else { + path, err := url.JoinPath(u.Path, "/artifactory/api/security/token") if err != nil { - logger.Error("createToken could not read error response body", "err", err) - return nil, fmt.Errorf("could not parse response body. Err: %v", e) + logger.Error("error joining url path", "err", err) + return nil, err + } + + values := url.Values{ + "grant_type": []string{grantTypeRefreshToken}, + "refresh_token": []string{refreshToken}, + "access_token": []string{config.AccessToken}, + } + + resp, refreshErr = b.performArtifactoryPost(config, path, values) + } + + if refreshErr != nil { + logger.Error("error making token request", "response", resp, "err", refreshErr) + return nil, refreshErr + } + + //noinspection GoUnhandledErrorResult + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + 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 nil, fmt.Errorf("could not refresh access token. Err: %v", err) } - logger.Error("createToken got non-200 status code", "statusCode", resp.StatusCode, "body", string(body)) - return nil, fmt.Errorf("could not create access token. HTTP response: %s", body) + + logger.Error("got non-200 status code", "statusCode", resp.StatusCode, "message", errResp.String()) + return nil, fmt.Errorf("could not refresh access token: %s", errResp.String()) } var createdToken createTokenResponse if err := json.NewDecoder(resp.Body).Decode(&createdToken); err != nil { logger.Error("could not parse response", "response", resp, "err", err) - return nil, fmt.Errorf("could not create access token. Err: %v", err) + return nil, fmt.Errorf("could not refresh access token. Err: %w", err) } return &createdToken, nil @@ -239,7 +324,7 @@ func (b *backend) createToken(config baseConfiguration, expiresIn time.Duration, // supportForceRevocable verifies whether or not the Artifactory version is 7.50.3 or higher. // The access API changes in v7.50.3 to support force_revocable to allow us to set the expiration for the tokens. // REF: https://www.jfrog.com/confluence/display/JFROG/JFrog+Platform+REST+API#JFrogPlatformRESTAPI-CreateToken -func (b *backend) supportForceRevocable(config baseConfiguration) bool { +func (b *backend) supportForceRevocable(config baseConfiguration) (bool, error) { return b.checkVersion("7.50.3", config) } @@ -247,13 +332,53 @@ func (b *backend) supportForceRevocable(config baseConfiguration) bool { // The access API changed in v7.21.1 // REF: https://www.jfrog.com/confluence/display/JFROG/Artifactory+REST+API#ArtifactoryRESTAPI-AccessTokens func (b *backend) useNewAccessAPI(config baseConfiguration) bool { - return b.checkVersion("7.21.1", config) + compatible, err := b.checkVersion("7.21.1", config) + if err != nil { + b.Logger(). + With("func", "useNewAccessAPI"). + Warn("failed to check for Artifactory version. Default to 'true'", "err", err) + return true // default to use new API + } + + return compatible +} + +func (b *backend) refreshExpiredAccessToken(ctx context.Context, req *logical.Request, config *baseConfiguration, userTokenConfig *userTokenConfiguration, username string) error { + 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 + if err != nil { + logger.Debug("failed checkVersion", "err", err) + + if _, ok := err.(*TokenExpiredError); ok { + logger.Info("access token expired. Attempt to refresh using the refresh token.", "err", err) + refreshErr := userTokenConfig.RefreshAccessToken(ctx, req, username, b, *config) + if refreshErr != nil { + logger.Error("failed to refresh access token.", "err", refreshErr) + return refreshErr + } + + config.AccessToken = userTokenConfig.AccessToken + return nil + } + + return err + } + + return nil } +var tokenFailedValidationRegex = regexp.MustCompile(`.*Token failed verification: expired.*`) + // getVersion will fetch the current Artifactory version and store it in the backend func (b *backend) getVersion(config baseConfiguration) (version string, err error) { logger := b.Logger().With("func", "getVersion") + logger.Debug("fetching Artifactory version") + resp, err := b.performArtifactoryGet(config, "/artifactory/api/system/version") if err != nil { logger.Error("error making system version request", "response", resp, "err", err) @@ -264,7 +389,19 @@ func (b *backend) getVersion(config baseConfiguration) (version string, err erro if resp.StatusCode != http.StatusOK { logger.Error("got non-200 status code", "statusCode", resp.StatusCode) - return "", fmt.Errorf("could not get the system version: HTTP response %v", 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 Artifactory version. Err: %v", err) + } + + if resp.StatusCode == http.StatusUnauthorized && tokenFailedValidationRegex.MatchString(errResp.String()) { + return "", &TokenExpiredError{} + } + + return "", fmt.Errorf("could not get the system version: HTTP response %v", errResp.String()) } var systemVersion systemVersionResponse @@ -273,14 +410,18 @@ func (b *backend) getVersion(config baseConfiguration) (version string, err erro return "", err } + logger.Debug("found Artifactory version", "version", systemVersion.Version) + return systemVersion.Version, 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) { +func (b *backend) checkVersion(ver string, config baseConfiguration) (compatible bool, err error) { logger := b.Logger().With("func", "checkVersion") + compatible = false + artifactoryVersion, err := b.getVersion(config) if err != nil { logger.Error("Unable to get Artifactory Version. Check url and access_token fields. TLS connection verification with Artifactory can be skipped by setting bypass_artifactory_tls_verification field to 'true'", "ver", artifactoryVersion, "err", err) @@ -299,6 +440,7 @@ func (b *backend) checkVersion(ver string, config baseConfiguration) (compatible return } + logger.Trace("comparing versions", "v1", v1.String(), "v2", v2.String()) if v1.GreaterThanOrEqual(v2) { compatible = true } @@ -306,53 +448,6 @@ func (b *backend) checkVersion(ver string, config baseConfiguration) (compatible return } -// parseJWT will parse a JWT token string from Artifactory and return a *jwt.Token, err -func (b *backend) parseJWT(config baseConfiguration, token string) (jwtToken *jwt.Token, err error) { - if config.AccessToken == "" { - return nil, fmt.Errorf("empty access token not allowed") - } - - validate := true - - logger := b.Logger().With("func", "parseJWT") - - cert, err := b.getRootCert(config) - if err != nil { - if errors.Is(err, ErrIncompatibleVersion) { - logger.Error("outdated artifactory, unable to retrieve root cert, skipping token validation") - validate = false - } else { - logger.Error("error retrieving root cert", "err", err.Error()) - return - } - } - - // Parse Token - if validate { - jwtToken, err = jwt.Parse(token, - func(token *jwt.Token) (interface{}, error) { return cert.PublicKey, nil }, - jwt.WithValidMethods([]string{"RS256"})) - if err != nil { - return - } - if !jwtToken.Valid { - return - } - } else { // SKIP Validation - // -- NOTE THIS IGNORES THE SIGNATURE, which is probably bad, - // but it is artifactory's job to validate the token, right? - // p := jwt.Parser{} - // token, _, err := p.ParseUnverified(oldAccessToken, jwt.MapClaims{}) - jwtToken, err = jwt.Parse(token, nil, jwt.WithoutClaimsValidation()) - if err != nil { - return - } - } - - // If we got here, we should have a jwtToken and nil err - return -} - type TokenInfo struct { TokenID string `json:"token_id"` Scope string `json:"scope"` @@ -362,7 +457,10 @@ type TokenInfo struct { // getTokenInfo will parse the provided token to return useful information about it func (b *backend) getTokenInfo(config baseConfiguration, token string) (info *TokenInfo, err error) { + logger := b.Logger().With("func", "getTokenInfo") + if config.AccessToken == "" { + logger.Error("config.AccessToken is empty") return nil, fmt.Errorf("empty access token not allowed") } @@ -385,8 +483,6 @@ func (b *backend) getTokenInfo(config baseConfiguration, token string) (info *To Username: strings.Join(sub[2:], "/"), // 3rd+ elements (incase username has / in it) } - logger := b.Logger().With("func", "getTokenInfo") - // exp -> expires at (unixtime) - may not be present switch exp := claims["exp"].(type) { case int64: @@ -404,20 +500,72 @@ func (b *backend) getTokenInfo(config baseConfiguration, token string) (info *To return } +// parseJWT will parse a JWT token string from Artifactory and return a *jwt.Token, err +func (b *backend) parseJWT(config baseConfiguration, token string) (jwtToken *jwt.Token, err error) { + logger := b.Logger().With("func", "parseJWT") + + if config.AccessToken == "" { + logger.Error("config.AccessToken is empty") + return nil, fmt.Errorf("empty access token not allowed") + } + + validate := true + + cert, err := b.getRootCert(config) + if err != nil { + if errors.Is(err, ErrIncompatibleVersion) { + logger.Error("outdated artifactory, unable to retrieve root cert, skipping token validation") + validate = false + } else { + logger.Error("error retrieving root cert", "err", err.Error()) + return + } + } + + // Parse Token + if validate { + jwtToken, err = jwt.Parse(token, + func(token *jwt.Token) (interface{}, error) { return cert.PublicKey, nil }, + jwt.WithValidMethods([]string{"RS256"})) + if err != nil { + return + } + if !jwtToken.Valid { + return + } + } else { // SKIP Validation + // -- NOTE THIS IGNORES THE SIGNATURE, which is probably bad, + // but it is artifactory's job to validate the token, right? + // p := jwt.Parser{} + // token, _, err := p.ParseUnverified(oldAccessToken, jwt.MapClaims{}) + jwtToken, err = jwt.Parse(token, nil, jwt.WithoutClaimsValidation()) + if err != nil { + return + } + } + + // If we got here, we should have a jwtToken and nil err + return +} + // getRootCert will return the Artifactory access root certificate's public key, for validating token signatures func (b *backend) getRootCert(config baseConfiguration) (cert *x509.Certificate, err error) { + logger := b.Logger().With("func", "getRootCert") + if config.AccessToken == "" { return nil, fmt.Errorf("empty access token not allowed") } // Verify Artifactory version is at 7.12.0 or higher, prior versions will not work // REF: https://jfrog.com/help/r/jfrog-rest-apis/get-root-certificate - if !b.checkVersion("7.12.0", config) { + valid, err := b.checkVersion("7.12.0", config) + if err != nil { + return nil, err + } + if !valid { return cert, ErrIncompatibleVersion } - logger := b.Logger().With("func", "getRootCert") - resp, err := b.performArtifactoryGet(config, "/access/api/v1/cert/root") if err != nil { logger.Error("error requesting cert/root", "response", resp, "err", err) @@ -427,8 +575,19 @@ func (b *backend) getRootCert(config baseConfiguration) (cert *x509.Certificate, defer resp.Body.Close() if resp.StatusCode != http.StatusOK { + 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 nil, fmt.Errorf("could not create access token. Err: %v", err) + } + + if resp.StatusCode == http.StatusUnauthorized && tokenFailedValidationRegex.MatchString(errResp.String()) { + return nil, &TokenExpiredError{} + } + logger.Error("got non-200 status code", "statusCode", resp.StatusCode) - return cert, fmt.Errorf("could not get the certificate: HTTP response %v", resp.StatusCode) + return cert, fmt.Errorf("could not get the certificate: HTTP response %v", errResp.String()) } body, err := io.ReadAll(resp.Body) @@ -498,7 +657,10 @@ func (b *backend) sendUsage(config baseConfiguration, featureId string) { } func (b *backend) performArtifactoryGet(config baseConfiguration, path string) (*http.Response, error) { + logger := b.Logger().With("func", "performArtifactoryGet") + if config.AccessToken == "" { + logger.Error("config.AccessToken is empty") return nil, fmt.Errorf("empty access token not allowed") } @@ -549,7 +711,10 @@ func (b *backend) performArtifactoryPost(config baseConfiguration, path string, // performArtifactoryPost will HTTP POST data to the Artifactory API. func (b *backend) performArtifactoryPostWithJSON(config baseConfiguration, path string, postData []byte) (*http.Response, error) { + logger := b.Logger().With("func", "performArtifactoryPostWithJSON") + if config.AccessToken == "" { + logger.Error("config.AccessToken is empty") return nil, fmt.Errorf("empty access token not allowed") } @@ -577,7 +742,10 @@ func (b *backend) performArtifactoryPostWithJSON(config baseConfiguration, path // performArtifactoryDelete will HTTP DELETE to the Artifactory API. // The path will be appended to the configured configured URL Path (usually /artifactory) func (b *backend) performArtifactoryDelete(config baseConfiguration, path string) (*http.Response, error) { + logger := b.Logger().With("func", "performArtifactoryDelete") + if config.AccessToken == "" { + logger.Error("config.AccessToken is empty") return nil, fmt.Errorf("empty access token not allowed") } diff --git a/path_config.go b/path_config.go index 3710859..eda0d76 100644 --- a/path_config.go +++ b/path_config.go @@ -133,7 +133,11 @@ func (b *backend) pathConfigUpdate(ctx context.Context, req *logical.Request, da } if config == nil { - config = &adminConfiguration{} + config = &adminConfiguration{ + baseConfiguration: baseConfiguration{ + UseNewAccessAPI: true, + }, + } } if val, ok := data.GetOk("url"); ok { @@ -185,6 +189,8 @@ func (b *backend) pathConfigUpdate(ctx context.Context, req *logical.Request, da go b.sendUsage(config.baseConfiguration, "pathConfigRotateUpdate") + config.UseNewAccessAPI = b.useNewAccessAPI(config.baseConfiguration) + entry, err := logical.StorageEntryJSON(configAdminPath, config) if err != nil { return nil, err @@ -226,7 +232,7 @@ func (b *backend) pathConfigDelete(ctx context.Context, req *logical.Request, _ return nil, nil } - err = b.RevokeToken(config.baseConfiguration, token.TokenID, config.AccessToken) + err = b.RevokeToken(config.baseConfiguration, token.TokenID) if err != nil { b.Logger().Warn("error revoking existing access token", "tokenId", token.TokenID, "err", err) return nil, nil @@ -240,6 +246,8 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, _ *f b.configMutex.RLock() defer b.configMutex.RUnlock() + logger := b.Logger().With("func", "pathConfigRead") + config, err := b.fetchAdminConfiguration(ctx, req.Storage) if err != nil { return nil, err @@ -253,7 +261,8 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, _ *f version, err := b.getVersion(config.baseConfiguration) if err != nil { - b.Logger().Warn("failed to get system version", "err", err) + logger.Error("failed to get system version", "err", err) + return nil, err } // I'm not sure if I should be returning the access token, so I'll hash it. @@ -278,7 +287,7 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, _ *f // Optionally include token info if it parses properly token, err := b.getTokenInfo(config.baseConfiguration, config.AccessToken) if err != nil { - b.Logger().Warn("Error parsing AccessToken", "err", err.Error()) + logger.Warn("Error parsing AccessToken", "err", err.Error()) } else { configMap["token_id"] = token.TokenID configMap["username"] = token.Username @@ -290,7 +299,13 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, _ *f } } - if b.supportForceRevocable(config.baseConfiguration) { + supportForceRevocable, err := b.supportForceRevocable(config.baseConfiguration) + if err != nil { + logger.Warn("failed to determine if force_revocable is supported. Set 'supportForceRevocable' to 'false'.", "err", err) + supportForceRevocable = false + } + + if supportForceRevocable { configMap["use_expiring_tokens"] = config.UseExpiringTokens } diff --git a/path_config_rotate.go b/path_config_rotate.go index ba38a76..5881224 100644 --- a/path_config_rotate.go +++ b/path_config_rotate.go @@ -104,7 +104,7 @@ func (b *backend) pathConfigRotateWrite(ctx context.Context, req *logical.Reques } // Invalidate Old Token - err = b.RevokeToken(config.baseConfiguration, token.TokenID, oldAccessToken) + err = b.RevokeToken(config.baseConfiguration, token.TokenID) if err != nil { return logical.ErrorResponse("error revoking existing access token %s", token.TokenID), err } diff --git a/path_config_rotate_test.go b/path_config_rotate_test.go index 016e272..81607b6 100644 --- a/path_config_rotate_test.go +++ b/path_config_rotate_test.go @@ -89,7 +89,7 @@ func (e *accTestEnv) PathConfigRotateCreateTokenErr(t *testing.T) { assert.NotNil(t, resp) assert.Contains(t, resp.Data["error"], "error creating new access token") assert.ErrorContains(t, err, "could not create access token") - e.revokeTestToken(t, e.AccessToken, tokenId) + e.revokeTestToken(t, tokenId) } func (e *accTestEnv) PathConfigRotateBadAccessToken(t *testing.T) { diff --git a/path_config_test.go b/path_config_test.go index 988beb8..5d06a86 100644 --- a/path_config_test.go +++ b/path_config_test.go @@ -169,8 +169,8 @@ func (e *accTestEnv) PathConfigReadBadAccessToken(t *testing.T) { assert.NoError(t, err) resp, err := e.read(configAdminPath) - assert.NoError(t, err) - assert.NotNil(t, resp) + assert.Error(t, err) + assert.Nil(t, resp) // Otherwise success, we don't need to re-test for this } diff --git a/path_config_user_token.go b/path_config_user_token.go index 43a8e12..5e66cc1 100644 --- a/path_config_user_token.go +++ b/path_config_user_token.go @@ -3,6 +3,7 @@ package artifactory import ( "context" "crypto/sha256" + "errors" "fmt" "strings" "time" @@ -92,6 +93,25 @@ type userTokenConfiguration struct { DefaultDescription string `json:"default_description,omitempty"` } +func (c *userTokenConfiguration) RefreshAccessToken(ctx context.Context, req *logical.Request, username string, b *backend, adminBaseConfig baseConfiguration) error { + logger := b.Logger().With("func", "RefreshAccessToken") + + if c.RefreshToken == "" { + return fmt.Errorf("refresh_token is empty") + } + + refreshResp, err := b.RefreshToken(adminBaseConfig, c.RefreshToken) + if err != nil { + return err + } + logger.Info("access token refresh successful") + + c.AccessToken = refreshResp.AccessToken + c.RefreshToken = refreshResp.RefreshToken + + return b.storeUserTokenConfiguration(ctx, req, username, c) +} + // fetchAdminConfiguration will return nil,nil if there's no configuration func (b *backend) fetchUserTokenConfiguration(ctx context.Context, storage logical.Storage, username string) (*userTokenConfiguration, error) { // If username is not empty, then append to the path to fetch username specific configuration @@ -100,15 +120,15 @@ func (b *backend) fetchUserTokenConfiguration(ctx context.Context, storage logic path = fmt.Sprintf("%s/%s", path, username) } + logger := b.Logger().With("func", "fetchUserTokenConfiguration") + // Read in the backend configuration - b.Logger().Info("fetching user token configuration", "path", path) + logger.Info("fetching user token configuration", "path", path) entry, err := storage.Get(ctx, path) if err != nil { return nil, err } - logger := b.Logger().With("func", "fetchUserTokenConfiguration") - if entry == nil && len(username) > 0 { logger.Info(fmt.Sprintf("no configuration for username %s. Fetching default user token configuration", username), "path", configUserTokenPath) e, err := storage.Get(ctx, configUserTokenPath) @@ -124,6 +144,9 @@ func (b *backend) fetchUserTokenConfiguration(ctx context.Context, storage logic if entry == nil { logger.Warn("no configuration found. Using system default configuration.") return &userTokenConfiguration{ + baseConfiguration: baseConfiguration{ + UseNewAccessAPI: true, + }, DefaultTTL: b.Backend.System().DefaultLeaseTTL(), MaxTTL: b.Backend.System().MaxLeaseTTL(), }, nil @@ -171,8 +194,6 @@ func (b *backend) pathConfigUserTokenUpdate(ctx context.Context, req *logical.Re return logical.ErrorResponse("backend not configured"), nil } - go b.sendUsage(adminConfig.baseConfiguration, "pathConfigUserTokenUpdate") - username := "" if val, ok := data.GetOk("username"); ok { username = val.(string) @@ -193,6 +214,8 @@ func (b *backend) pathConfigUserTokenUpdate(ctx context.Context, req *logical.Re userTokenConfig.AccessToken = adminConfig.AccessToken } + go b.sendUsage(userTokenConfig.baseConfiguration, "pathConfigUserTokenUpdate") + if val, ok := data.GetOk("refresh_token"); ok { userTokenConfig.RefreshToken = val.(string) } @@ -234,6 +257,8 @@ func (b *backend) pathConfigUserTokenUpdate(ctx context.Context, req *logical.Re userTokenConfig.DefaultDescription = val.(string) } + userTokenConfig.UseNewAccessAPI = b.useNewAccessAPI(userTokenConfig.baseConfiguration) + err = b.storeUserTokenConfiguration(ctx, req, username, userTokenConfig) if err != nil { return nil, err @@ -246,6 +271,8 @@ func (b *backend) pathConfigUserTokenRead(ctx context.Context, req *logical.Requ b.configMutex.RLock() defer b.configMutex.RUnlock() + baseConfig := baseConfiguration{} + adminConfig, err := b.fetchAdminConfiguration(ctx, req.Storage) if err != nil { return nil, err @@ -255,7 +282,7 @@ func (b *backend) pathConfigUserTokenRead(ctx context.Context, req *logical.Requ return logical.ErrorResponse("backend not configured"), nil } - go b.sendUsage(adminConfig.baseConfiguration, "pathConfigUserTokenRead") + baseConfig = adminConfig.baseConfiguration username := "" if val, ok := data.GetOk("username"); ok { @@ -267,6 +294,21 @@ func (b *backend) pathConfigUserTokenRead(ctx context.Context, req *logical.Requ return nil, err } + if userTokenConfig.baseConfiguration.AccessToken != "" { + baseConfig.AccessToken = userTokenConfig.baseConfiguration.AccessToken + } + + if baseConfig.AccessToken == "" { + return logical.ErrorResponse("missing access token"), errors.New("missing access token") + } + + err = b.refreshExpiredAccessToken(ctx, req, &baseConfig, userTokenConfig, username) + if err != nil { + return logical.ErrorResponse("failed to refresh access token"), err + } + + go b.sendUsage(baseConfig, "pathConfigUserTokenRead") + accessTokenHash := sha256.Sum256([]byte(userTokenConfig.AccessToken)) refreshTokenHash := sha256.Sum256([]byte(userTokenConfig.RefreshToken)) @@ -284,10 +326,12 @@ func (b *backend) pathConfigUserTokenRead(ctx context.Context, req *logical.Requ } // Optionally include token info if it parses properly - token, err := b.getTokenInfo(adminConfig.baseConfiguration, userTokenConfig.AccessToken) + token, err := b.getTokenInfo(baseConfig, userTokenConfig.AccessToken) if err != nil { - b.Logger().With("func", "pathConfigUserTokenRead").Warn("Error parsing AccessToken", "err", err.Error()) - } else { + return logical.ErrorResponse("failed to get token info"), err + } + + if token != nil { configMap["token_id"] = token.TokenID configMap["username"] = token.Username configMap["scope"] = token.Scope diff --git a/path_config_user_token_test.go b/path_config_user_token_test.go index 80d282f..8b5702b 100644 --- a/path_config_user_token_test.go +++ b/path_config_user_token_test.go @@ -25,15 +25,18 @@ func TestAcceptanceBackend_PathConfigUserToken(t *testing.T) { } func (e *accTestEnv) PathConfigAccessTokenUpdate(t *testing.T) { + _, accessToken1 := e.createNewTestToken(t) + e.UpdateConfigUserToken(t, "", testData{ - "access_token": "test123", + "access_token": accessToken1, }) data := e.ReadConfigUserToken(t, "") accessTokenHash := data["access_token_sha256"] assert.NotEmpty(t, "access_token_sha256") + _, accessToken2 := e.createNewTestToken(t) e.UpdateConfigUserToken(t, "", testData{ - "access_token": "test456", + "access_token": accessToken2, }) data = e.ReadConfigUserToken(t, "") assert.NotEqual(t, data["access_token_sha256"], accessTokenHash) @@ -139,6 +142,11 @@ func TestAcceptanceBackend_PathConfigUserToken_UseSystemDefault(t *testing.T) { } accTestEnv := NewConfiguredAcceptanceTestEnv(t) + _, accessToken1 := accTestEnv.createNewTestToken(t) + accTestEnv.UpdateConfigUserToken(t, "", testData{ + "access_token": accessToken1, + }) + data := accTestEnv.ReadConfigUserToken(t, "") assert.Equal(t, accTestEnv.Backend.System().DefaultLeaseTTL().Seconds(), data["default_ttl"]) assert.Equal(t, accTestEnv.Backend.System().MaxLeaseTTL().Seconds(), data["max_ttl"]) diff --git a/path_user_token_create.go b/path_user_token_create.go index 7db0c07..14d6275 100644 --- a/path_user_token_create.go +++ b/path_user_token_create.go @@ -3,7 +3,6 @@ package artifactory import ( "context" "errors" - "fmt" "time" "github.com/hashicorp/vault/sdk/framework" @@ -83,8 +82,6 @@ func (b *backend) pathUserTokenCreatePerform(ctx context.Context, req *logical.R return logical.ErrorResponse("backend not configured"), nil } - go b.sendUsage(adminConfig.baseConfiguration, "pathUserTokenCreatePerform") - baseConfig = adminConfig.baseConfiguration username := data.Get("username").(string) @@ -102,6 +99,13 @@ func (b *backend) pathUserTokenCreatePerform(ctx context.Context, req *logical.R return logical.ErrorResponse("missing access token"), errors.New("missing access token") } + err = b.refreshExpiredAccessToken(ctx, req, &baseConfig, userTokenConfig, username) + if err != nil { + return logical.ErrorResponse("failed to refresh access token"), err + } + + go b.sendUsage(baseConfig, "pathUserTokenCreatePerform") + baseConfig.UseExpiringTokens = userTokenConfig.UseExpiringTokens if value, ok := data.GetOk("use_expiring_tokens"); ok { baseConfig.UseExpiringTokens = value.(bool) @@ -191,30 +195,7 @@ func (b *backend) pathUserTokenCreatePerform(ctx context.Context, req *logical.R resp, err := b.CreateToken(baseConfig, role) if err != nil { - if _, ok := err.(*TokenExpiredError); ok { - logger.Info("access token expired. Attempt to refresh using the refresh token.") - refreshResp, err := b.RefreshToken(baseConfig, role) - if err != nil { - return nil, fmt.Errorf("failed to refresh access token. err: %v", err) - } - logger.Info("access token refresh successful") - - userTokenConfig.AccessToken = refreshResp.AccessToken - userTokenConfig.RefreshToken = refreshResp.RefreshToken - b.storeUserTokenConfiguration(ctx, req, username, userTokenConfig) - - baseConfig.AccessToken = userTokenConfig.AccessToken - role.RefreshToken = userTokenConfig.RefreshToken - - // try again after token was refreshed - logger.Info("attempt to create user token again after access token refresh") - resp, err = b.CreateToken(baseConfig, role) - if err != nil { - return nil, err - } - } else { - return nil, err - } + return logical.ErrorResponse("failed to create new token"), err } response := b.Secret(SecretArtifactoryAccessTokenType).Response(map[string]interface{}{ diff --git a/secret_access_token.go b/secret_access_token.go index d949092..3fd0945 100644 --- a/secret_access_token.go +++ b/secret_access_token.go @@ -85,8 +85,7 @@ func (b *backend) secretAccessTokenRevoke(ctx context.Context, req *logical.Requ } tokenId := req.Secret.InternalData["token_id"].(string) - accessToken := req.Secret.InternalData["access_token"].(string) - if err := b.RevokeToken(config.baseConfiguration, tokenId, accessToken); err != nil { + if err := b.RevokeToken(config.baseConfiguration, tokenId); err != nil { return nil, err } diff --git a/test_utils.go b/test_utils.go index eadea35..1bd8043 100644 --- a/test_utils.go +++ b/test_utils.go @@ -36,8 +36,9 @@ type testData map[string]interface{} func (e *accTestEnv) createNewTestToken(t *testing.T) (string, string) { config := adminConfiguration{ baseConfiguration: baseConfiguration{ - AccessToken: e.AccessToken, - ArtifactoryURL: e.URL, + AccessToken: e.AccessToken, + ArtifactoryURL: e.URL, + UseNewAccessAPI: true, }, } @@ -62,8 +63,9 @@ func (e *accTestEnv) createNewTestToken(t *testing.T) (string, string) { func (e *accTestEnv) createNewNonAdminTestToken(t *testing.T) (string, string) { config := adminConfiguration{ baseConfiguration: baseConfiguration{ - AccessToken: e.AccessToken, - ArtifactoryURL: e.URL, + AccessToken: e.AccessToken, + ArtifactoryURL: e.URL, + UseNewAccessAPI: true, }, } @@ -83,13 +85,13 @@ func (e *accTestEnv) createNewNonAdminTestToken(t *testing.T) (string, string) { return resp.TokenId, resp.AccessToken } -func (e *accTestEnv) revokeTestToken(t *testing.T, accessToken string, tokenID string) { +func (e *accTestEnv) revokeTestToken(t *testing.T, tokenID string) { config := baseConfiguration{ AccessToken: e.AccessToken, ArtifactoryURL: e.URL, } - err := e.Backend.(*backend).RevokeToken(config, tokenID, accessToken) + err := e.Backend.(*backend).RevokeToken(config, tokenID) if err != nil { t.Fatal(err) } @@ -505,7 +507,7 @@ func (e *accTestEnv) Cleanup(t *testing.T) { e.DeleteConfigAdmin(t) // revoke the test token - e.revokeTestToken(t, e.AccessToken, data["token_id"].(string)) + e.revokeTestToken(t, data["token_id"].(string)) } func newAcceptanceTestEnv() (*accTestEnv, error) { diff --git a/ttl_test.go b/ttl_test.go index 20d7bbe..7d19690 100644 --- a/ttl_test.go +++ b/ttl_test.go @@ -218,7 +218,7 @@ func TestBackend_UserTokenConfigMaxTTLUseConfigMaxTTL(t *testing.T) { configPath := configUserTokenPath + "/admin" backend_max_ttl := b.System().MaxLeaseTTL() - user_token_config_ttl := backend_max_ttl - 1*time.Minute + user_token_config_ttl := backend_max_ttl - 10*time.Minute SetUserTokenMaxTTL(t, b, config.StorageView, configPath, user_token_config_ttl) resp, err := b.HandleRequest(context.Background(), &logical.Request{ From 9e9867e4b4b040d5dedb0efde32dd740df6cde78 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Wed, 6 Nov 2024 13:48:14 -0800 Subject: [PATCH 4/5] Update CHANGELOG --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef6461f..041ef02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## 1.8.1 (November 8, 2024) + +BUG FIXES: + +* Fix error when reading `config/user_token` with expired access token and failed to refresh. Issue: [#217](https://github.com/jfrog/artifactory-secrets-plugin/issues/217) PR: [#223](https://github.com/jfrog/artifactory-secrets-plugin/pull/223) + ## 1.8.0 (June 7, 2024) IMPROVEMENTS: From 6d57bd4ae6821bfffd0d8427aa716ff1faddd0e6 Mon Sep 17 00:00:00 2001 From: JFrog CI Date: Wed, 6 Nov 2024 21:51:49 +0000 Subject: [PATCH 5/5] JFrog Pipelines - Add Artifactory version to CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 041ef02..86c7e40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## 1.8.1 (November 8, 2024) +## 1.8.1 (November 8, 2024). Tested on Artifactory 7.98.8 with Vault v1.18.1 and OpenBao v2.0.0 BUG FIXES: