From 45bc032169b0eb9de41128ce481d5af8405a7025 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Mon, 13 Jan 2025 12:33:38 -0800 Subject: [PATCH 1/7] Bump Go version to 1.23 --- .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 e526804..9fb6372 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.22 + go-version: 1.23 - 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 29897d4..abcf5aa 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.22 + go-version: 1.23 - name: Import GPG key id: import_gpg uses: crazy-max/ghaction-import-gpg@v6 diff --git a/go.mod b/go.mod index b730acc..315f41a 100644 --- a/go.mod +++ b/go.mod @@ -1,7 +1,6 @@ module github.com/jfrog/vault-plugin-secrets-artifactory -go 1.22 -toolchain go1.23.4 +go 1.23.4 require ( github.com/golang-jwt/jwt/v4 v4.5.1 From 1522f779a30264fc32657aa081a1d27288dc5395 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Mon, 13 Jan 2025 12:35:01 -0800 Subject: [PATCH 2/7] Fix incorrect log statements and ttl values in logs Code tidy up --- artifactory.go | 4 ++-- backend.go | 4 ++-- path_config_test.go | 1 + path_token_create.go | 12 ++++++------ path_user_token_create.go | 16 ++++++++-------- test_utils.go | 3 +-- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/artifactory.go b/artifactory.go index a493270..93bdf57 100644 --- a/artifactory.go +++ b/artifactory.go @@ -350,7 +350,7 @@ func (b *backend) refreshExpiredAccessToken(ctx context.Context, req *logical.Re logger.Debug("check if access token is expired by getting token itself") err := b.getTokenByID(*config) if err != nil { - logger.Debug("failed to get Viewer role", "err", err) + logger.Debug("failed to get token by ID", "err", err) if _, ok := err.(*TokenExpiredError); ok { logger.Info("access token expired. Attempt to refresh using the refresh token.", "err", err) @@ -417,7 +417,7 @@ func (b *backend) getVersion(config baseConfiguration) (version string, err erro func (b *backend) getTokenByID(config baseConfiguration) error { logger := b.Logger().With("func", "getTokenByID") - logger.Debug("fetching Viewer role") + logger.Debug("fetching token by ID") // '/me' is special value to get info about token itself // https://jfrog.com/help/r/jfrog-rest-apis/get-token-by-id diff --git a/backend.go b/backend.go index 054bce8..235e801 100644 --- a/backend.go +++ b/backend.go @@ -36,7 +36,7 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, return nil, fmt.Errorf("configuration passed into backend is nil") } - b, err := Backend(conf) + b, err := Backend() if err != nil { return nil, err } @@ -48,7 +48,7 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, return b, nil } -func Backend(_ *logical.BackendConfig) (*backend, error) { +func Backend() (*backend, error) { b := &backend{} up, err := testUsernameTemplate(defaultUserNameTemplate) diff --git a/path_config_test.go b/path_config_test.go index 5d06a86..177da9c 100644 --- a/path_config_test.go +++ b/path_config_test.go @@ -225,6 +225,7 @@ func TestBackend_AccessTokenAsSHA256(t *testing.T) { assert.NotNil(t, resp) assert.EqualValues(t, correctSHA256, resp.Data["access_token_sha256"]) } + func TestBackend_RevokeOnDelete(t *testing.T) { httpmock.Activate() diff --git a/path_token_create.go b/path_token_create.go index 5f141f9..fc1691d 100644 --- a/path_token_create.go +++ b/path_token_create.go @@ -111,7 +111,7 @@ func (b *backend) pathTokenCreatePerform(ctx context.Context, req *logical.Reque logger := b.Logger().With("func", "pathTokenCreatePerform") maxLeaseTTL := b.Backend.System().MaxLeaseTTL() - logger.Debug("initialize maxLeaseTTL to system value", "maxLeaseTTL", maxLeaseTTL) + logger.Debug("initialize maxLeaseTTL to system value", "maxLeaseTTL", maxLeaseTTL.Seconds()) if value, ok := data.GetOk("max_ttl"); ok && value.(int) > 0 { logger.Debug("max_ttl is set", "max_ttl", value) @@ -122,26 +122,26 @@ func (b *backend) pathTokenCreatePerform(ctx context.Context, req *logical.Reque maxLeaseTTL = maxTTL } } else if role.MaxTTL > 0 && role.MaxTTL < maxLeaseTTL { - logger.Debug("using role MaxTTL", "role.MaxTTL", role.MaxTTL) + logger.Debug("using role MaxTTL", "role.MaxTTL", role.MaxTTL.Seconds()) maxLeaseTTL = role.MaxTTL } - logger.Debug("Max lease TTL (sec)", "maxLeaseTTL", maxLeaseTTL) + logger.Debug("Max lease TTL (sec)", "maxLeaseTTL", maxLeaseTTL.Seconds()) ttl := b.Backend.System().DefaultLeaseTTL() if value, ok := data.GetOk("ttl"); ok && value.(int) > 0 { logger.Debug("ttl is set", "ttl", value) ttl = time.Second * time.Duration(value.(int)) } else if role.DefaultTTL != 0 { - logger.Debug("using role DefaultTTL", "role.DefaultTTL", role.DefaultTTL) + logger.Debug("using role DefaultTTL", "role.DefaultTTL", role.DefaultTTL.Seconds()) ttl = role.DefaultTTL } // cap ttl to maxLeaseTTL if ttl > maxLeaseTTL { - logger.Debug("ttl is longer than maxLeaseTTL", "ttl", ttl, "maxLeaseTTL", maxLeaseTTL) + logger.Debug("ttl is longer than maxLeaseTTL", "ttl", ttl, "maxLeaseTTL", maxLeaseTTL.Seconds()) ttl = maxLeaseTTL } - logger.Debug("TTL (sec)", "ttl", ttl) + logger.Debug("TTL (sec)", "ttl", ttl.Seconds()) // Set the role.ExpiresIn based on maxLeaseTTL if use_expiring_tokens is set to tru in config // - This value will be passed to createToken and used as expires_in for versions of Artifactory 7.50.3 or higher diff --git a/path_user_token_create.go b/path_user_token_create.go index f8756c1..17ce5fd 100644 --- a/path_user_token_create.go +++ b/path_user_token_create.go @@ -93,8 +93,8 @@ func (b *backend) pathUserTokenCreatePerform(ctx context.Context, req *logical.R return nil, err } - if userTokenConfig.baseConfiguration.AccessToken != "" { - baseConfig.AccessToken = userTokenConfig.baseConfiguration.AccessToken + if userTokenConfig.AccessToken != "" { + baseConfig.AccessToken = userTokenConfig.AccessToken } if baseConfig.AccessToken == "" { @@ -129,7 +129,7 @@ func (b *backend) pathUserTokenCreatePerform(ctx context.Context, req *logical.R } maxLeaseTTL := b.Backend.System().MaxLeaseTTL() - logger.Debug("initialize maxLeaseTTL to system value", "maxLeaseTTL", maxLeaseTTL) + logger.Debug("initialize maxLeaseTTL to system value", "maxLeaseTTL", maxLeaseTTL.Seconds()) if value, ok := data.GetOk("max_ttl"); ok && value.(int) > 0 { logger.Debug("max_ttl is set", "max_ttl", value) @@ -140,27 +140,27 @@ func (b *backend) pathUserTokenCreatePerform(ctx context.Context, req *logical.R maxLeaseTTL = maxTTL } } else if userTokenConfig.MaxTTL > 0 && userTokenConfig.MaxTTL < maxLeaseTTL { - logger.Debug("using user token config MaxTTL", "userTokenConfig.MaxTTL", userTokenConfig.MaxTTL) + logger.Debug("using user token config MaxTTL", "userTokenConfig.MaxTTL", userTokenConfig.MaxTTL.Seconds()) // use max TTL from user config if set and is less than system max lease TTL maxLeaseTTL = userTokenConfig.MaxTTL } - logger.Debug("Max lease TTL (sec)", "maxLeaseTTL", maxLeaseTTL) + logger.Debug("Max lease TTL (sec)", "maxLeaseTTL", maxLeaseTTL.Seconds()) ttl := b.Backend.System().DefaultLeaseTTL() if value, ok := data.GetOk("ttl"); ok && value.(int) > 0 { logger.Debug("ttl is set", "ttl", value) ttl = time.Second * time.Duration(value.(int)) } else if userTokenConfig.DefaultTTL != 0 { - logger.Debug("using user config DefaultTTL", "userTokenConfig.DefaultTTL", userTokenConfig.DefaultTTL) + logger.Debug("using user config DefaultTTL", "userTokenConfig.DefaultTTL", userTokenConfig.DefaultTTL.Seconds()) ttl = userTokenConfig.DefaultTTL } // cap ttl to maxLeaseTTL if maxLeaseTTL > 0 && ttl > maxLeaseTTL { - logger.Debug("ttl is longer than maxLeaseTTL", "ttl", ttl, "maxLeaseTTL", maxLeaseTTL) + logger.Debug("ttl is longer than maxLeaseTTL", "ttl", ttl, "maxLeaseTTL", maxLeaseTTL.Seconds()) ttl = maxLeaseTTL } - logger.Debug("TTL (sec)", "ttl", ttl) + logger.Debug("TTL (sec)", "ttl", ttl.Seconds()) // now ttl is determined, we set role.ExpiresIn so this value so expirable token has the correct expiration if baseConfig.UseExpiringTokens { diff --git a/test_utils.go b/test_utils.go index 46b74a4..79c9a03 100644 --- a/test_utils.go +++ b/test_utils.go @@ -608,7 +608,7 @@ func makeBackend(t *testing.T) (*backend, *logical.BackendConfig) { config := logical.TestBackendConfig() config.StorageView = &logical.InmemStorage{} - b, err := Backend(config) + b, err := Backend() if err != nil { t.Fatal(err) } @@ -623,7 +623,6 @@ func makeBackend(t *testing.T) (*backend, *logical.BackendConfig) { func configuredBackend(t *testing.T, adminConfig map[string]interface{}) (*backend, *logical.BackendConfig) { b, config := makeBackend(t) - t.Logf("b.System().MaxLeaseTTL(): %v\n", b.System().MaxLeaseTTL()) b.InitializeHttpClient(&adminConfiguration{}) _, err := b.HandleRequest(context.Background(), &logical.Request{ From 06ccd35935ce518f771e4e3a3c8048c7d6977884 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Mon, 13 Jan 2025 12:36:00 -0800 Subject: [PATCH 3/7] Fix token revoke func doesn't work with user_token path --- secret_access_token.go | 46 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/secret_access_token.go b/secret_access_token.go index 3fd0945..ffe2ed9 100644 --- a/secret_access_token.go +++ b/secret_access_token.go @@ -3,6 +3,7 @@ package artifactory import ( "context" "fmt" + "strings" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" @@ -18,6 +19,14 @@ func (b *backend) secretAccessToken() *framework.Secret { Type: framework.TypeString, Description: `Artifactory Access Token`, }, + "refresh_token": { + Type: framework.TypeString, + Description: `Artifactory Refresh Token`, + }, + "reference_token": { + Type: framework.TypeString, + Description: `Artifactory Reference Token`, + }, "token_id": { Type: framework.TypeString, Description: `Artifactory Access Token Id`, @@ -34,8 +43,6 @@ func (b *backend) secretAccessToken() *framework.Secret { } func (b *backend) secretAccessTokenRenew(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) { - resp := &logical.Response{Secret: req.Secret} - config, err := b.fetchAdminConfiguration(ctx, req.Storage) if err != nil { return nil, err @@ -62,6 +69,9 @@ func (b *backend) secretAccessTokenRenew(ctx context.Context, req *logical.Reque if err != nil { return nil, err } + + resp := &logical.Response{Secret: req.Secret} + if len(warnings) > 0 { for _, warning := range warnings { resp.AddWarning(warning) @@ -74,9 +84,11 @@ func (b *backend) secretAccessTokenRenew(ctx context.Context, req *logical.Reque } func (b *backend) secretAccessTokenRevoke(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) { - config, err := b.fetchAdminConfiguration(ctx, req.Storage) + logger := b.Logger().With("func", "secretAccessTokenRevoke") + config, err := b.fetchAdminConfiguration(ctx, req.Storage) if err != nil { + logger.Debug("failed to fetch admin config", "err", err) return nil, err } @@ -84,9 +96,35 @@ func (b *backend) secretAccessTokenRevoke(ctx context.Context, req *logical.Requ return logical.ErrorResponse("backend not configured"), nil } + // logger.Debug("request", "Path", req.Path, "Secret.InternalData", req.Secret.InternalData) + + if config.AccessToken == "" { + if strings.Contains(req.Path, "token/") { + return logical.ErrorResponse("admin access_token is not configured"), nil + } + + // try to use user token + if strings.Contains(req.Path, "user_token/") { + logger.Debug("admin access token is empty and request path is user_token") + username := req.Secret.InternalData["username"].(string) + userTokenConfig, err := b.fetchUserTokenConfiguration(ctx, req.Storage, username) + if err != nil { + logger.Debug("failed to fetch user config", "err", err) + return nil, err + } + + if userTokenConfig.AccessToken == "" { + return logical.ErrorResponse("user access_token is not configured"), nil + } + + config.AccessToken = userTokenConfig.AccessToken + } + } + tokenId := req.Secret.InternalData["token_id"].(string) + if err := b.RevokeToken(config.baseConfiguration, tokenId); err != nil { - return nil, err + return logical.ErrorResponse("failed to revoke access token"), err } return nil, nil From 753ee178a317b48f229bf26f8933d63fce43abc0 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Mon, 13 Jan 2025 14:01:36 -0800 Subject: [PATCH 4/7] Fix errors when access token is not set for admin path --- path_config.go | 47 +++++++++++++++++++++++++-------------- path_config_user_token.go | 8 ++++--- 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/path_config.go b/path_config.go index eda0d76..bd4cb2f 100644 --- a/path_config.go +++ b/path_config.go @@ -187,9 +187,11 @@ func (b *backend) pathConfigUpdate(ctx context.Context, req *logical.Request, da b.InitializeHttpClient(config) - go b.sendUsage(config.baseConfiguration, "pathConfigRotateUpdate") + if config.AccessToken != "" { + go b.sendUsage(config.baseConfiguration, "pathConfigRotateUpdate") - config.UseNewAccessAPI = b.useNewAccessAPI(config.baseConfiguration) + config.UseNewAccessAPI = b.useNewAccessAPI(config.baseConfiguration) + } entry, err := logical.StorageEntryJSON(configAdminPath, config) if err != nil { @@ -217,12 +219,16 @@ func (b *backend) pathConfigDelete(ctx context.Context, req *logical.Request, _ return logical.ErrorResponse("backend not configured"), nil } - go b.sendUsage(config.baseConfiguration, "pathConfigDelete") - if err := req.Storage.Delete(ctx, configAdminPath); err != nil { return nil, err } + if config.AccessToken == "" { + return nil, nil + } + + go b.sendUsage(config.baseConfiguration, "pathConfigDelete") + if config.RevokeOnDelete { b.Logger().Info("config.RevokeOnDelete is 'true'. Attempt to revoke access token.") @@ -257,21 +263,8 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, _ *f return logical.ErrorResponse("backend not configured"), nil } - go b.sendUsage(config.baseConfiguration, "pathConfigRead") - - version, err := b.getVersion(config.baseConfiguration) - if err != nil { - 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. - accessTokenHash := sha256.Sum256([]byte(config.AccessToken)) - configMap := map[string]interface{}{ - "access_token_sha256": fmt.Sprintf("%x", accessTokenHash[:]), "url": config.ArtifactoryURL, - "version": version, "use_expiring_tokens": config.UseExpiringTokens, "force_revocable": config.ForceRevocable, "bypass_artifactory_tls_verification": config.BypassArtifactoryTLSVerification, @@ -279,6 +272,26 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, _ *f "revoke_on_delete": config.RevokeOnDelete, } + if config.AccessToken == "" { + return &logical.Response{ + Warnings: []string{"access_token is not set"}, + Data: configMap, + }, nil + } + + // I'm not sure if I should be returning the access token, so I'll hash it. + accessTokenHash := sha256.Sum256([]byte(config.AccessToken)) + configMap["access_token_sha256"] = fmt.Sprintf("%x", accessTokenHash[:]) + + go b.sendUsage(config.baseConfiguration, "pathConfigRead") + + version, err := b.getVersion(config.baseConfiguration) + if err != nil { + logger.Error("failed to get system version", "err", err) + return nil, err + } + configMap["version"] = version + // Optionally include username_template if len(config.UsernameTemplate) > 0 { configMap["username_template"] = config.UsernameTemplate diff --git a/path_config_user_token.go b/path_config_user_token.go index ffcf5cb..a0b2bdc 100644 --- a/path_config_user_token.go +++ b/path_config_user_token.go @@ -214,8 +214,6 @@ 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) } @@ -257,7 +255,11 @@ func (b *backend) pathConfigUserTokenUpdate(ctx context.Context, req *logical.Re userTokenConfig.DefaultDescription = val.(string) } - userTokenConfig.UseNewAccessAPI = b.useNewAccessAPI(userTokenConfig.baseConfiguration) + if userTokenConfig.AccessToken != "" { + go b.sendUsage(userTokenConfig.baseConfiguration, "pathConfigUserTokenUpdate") + + userTokenConfig.UseNewAccessAPI = b.useNewAccessAPI(userTokenConfig.baseConfiguration) + } err = b.storeUserTokenConfiguration(ctx, req, username, userTokenConfig) if err != nil { From 520995114c8d384a044b703b564b04f92084497d Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Mon, 13 Jan 2025 14:13:42 -0800 Subject: [PATCH 5/7] Update CHANGELOG and README --- CHANGELOG.md | 7 +++++++ README.md | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4e0edf..c6d29b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## 1.8.5 (January 14, 2025) + +BUG FIXES: + +* Fix error when access token is not set for `config/admin` path when using user token. Issue: [#236](https://github.com/jfrog/artifactory-secrets-plugin/issues/236) PR: [#240](https://github.com/jfrog/artifactory-secrets-plugin/pull/240) +* Fix token lease revoke error when access token is not set for `config/admin` path when using user token. Issue: [#237](https://github.com/jfrog/artifactory-secrets-plugin/issues/237) PR: [#240](https://github.com/jfrog/artifactory-secrets-plugin/pull/240) + ## 1.8.4 (December 9, 2024). Tested on Artifactory 7.98.10 with Vault v1.18.2 and OpenBao v2.0.0 BUG FIXES: diff --git a/README.md b/README.md index 2227831..fcbb544 100644 --- a/README.md +++ b/README.md @@ -852,7 +852,7 @@ See the [contribution guide](./CONTRIBUTING.md). ## License -Copyright (c) 2024 JFrog. +Copyright (c) 2025 JFrog. Apache 2.0 licensed, see [LICENSE][LICENSE] file. From 74bda83272b7683da97f53e9fe6f9d1738abc6db Mon Sep 17 00:00:00 2001 From: JFrog CI Date: Mon, 13 Jan 2025 22:17:20 +0000 Subject: [PATCH 6/7] 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 c6d29b0..e650be6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## 1.8.5 (January 14, 2025) +## 1.8.5 (January 14, 2025). Tested on Artifactory 7.98.13 with Vault v1.18.3 and OpenBao v2.0.0 BUG FIXES: From 079b81b2552604369bbc0fc213318fd926afd11a Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Tue, 14 Jan 2025 09:15:09 -0800 Subject: [PATCH 7/7] Improve request path matching for lease revoke --- secret_access_token.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/secret_access_token.go b/secret_access_token.go index ffe2ed9..ce4c1f5 100644 --- a/secret_access_token.go +++ b/secret_access_token.go @@ -96,15 +96,16 @@ func (b *backend) secretAccessTokenRevoke(ctx context.Context, req *logical.Requ return logical.ErrorResponse("backend not configured"), nil } - // logger.Debug("request", "Path", req.Path, "Secret.InternalData", req.Secret.InternalData) + // logger.Debug("req", "Path", req.Path, "Secret.InternalData", req.Secret.InternalData) if config.AccessToken == "" { - if strings.Contains(req.Path, "token/") { + // check if this is admin token + if strings.HasPrefix(req.Path, "token/") { return logical.ErrorResponse("admin access_token is not configured"), nil } // try to use user token - if strings.Contains(req.Path, "user_token/") { + if strings.HasPrefix(req.Path, "user_token/") { logger.Debug("admin access token is empty and request path is user_token") username := req.Secret.InternalData["username"].(string) userTokenConfig, err := b.fetchUserTokenConfiguration(ctx, req.Storage, username)