Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix secret revoke error with user_token #240

Merged
merged 7 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/acceptance-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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/[email protected]
- name: Install GoReleaser
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## 1.8.5 (January 14, 2025). Tested on Artifactory 7.98.13 with Vault v1.18.3 and OpenBao v2.0.0

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:
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
4 changes: 2 additions & 2 deletions artifactory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions go.mod
Original file line number Diff line number Diff line change
@@ -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
Expand Down
47 changes: 30 additions & 17 deletions path_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.")

Expand Down Expand Up @@ -257,28 +263,35 @@ 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,
"allow_scope_override": config.AllowScopeOverride,
"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
Expand Down
1 change: 1 addition & 0 deletions path_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
8 changes: 5 additions & 3 deletions path_config_user_token.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down
12 changes: 6 additions & 6 deletions path_token_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
16 changes: 8 additions & 8 deletions path_user_token_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
47 changes: 43 additions & 4 deletions secret_access_token.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package artifactory
import (
"context"
"fmt"
"strings"

"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/logical"
Expand All @@ -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`,
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -74,19 +84,48 @@ 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
}

if config == nil {
return logical.ErrorResponse("backend not configured"), nil
}

// logger.Debug("req", "Path", req.Path, "Secret.InternalData", req.Secret.InternalData)

if config.AccessToken == "" {
// 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.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)
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
Expand Down
3 changes: 1 addition & 2 deletions test_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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{
Expand Down
Loading