Skip to content
Open
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
7 changes: 7 additions & 0 deletions kv/internal/resolve/refs.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ type refKey struct {
fragment string
}

// ParseWholeValue is the exported boundary over parseWholeValue for callers
// outside the resolve engine that must parse a kv:// reference without resolving it.
func ParseWholeValue(input string) (store, path, fragment string, ok bool, err error) {
rk, ok, err := parseWholeValue(input)
return rk.store, rk.path, rk.fragment, ok, err
}

// parseWholeValue parses a whole-value reference of the form
// "kv://store/path#frag".
//
Expand Down
16 changes: 14 additions & 2 deletions kv/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,17 @@
Init(ctx context.Context) error
}

// Setter is an optional interface for providers that support writing values
// back to their backend.
type Setter interface {
Set(ctx context.Context, key, value string) error
}

// Lister is an optional interface for providers that support enumerating
// keys by prefix. This enables dynamic discovery of available secrets
// keys & values by prefix. This enables dynamic discovery of available secrets
// and operational tooling.
type Lister interface {

Check warning on line 113 in kv/provider.go

View check run for this annotation

probelabs / Visor: architecture

architecture Issue

The `Lister` interface has been changed from returning `[]string` to `map[string]string`. This is a breaking change for any external consumers of this interface. While it's a functional improvement for performance by preventing N+1 lookups, it violates semantic versioning if this is a library intended for external use without a major version bump.
Raw output
If this is a public library, this change should be part of a major version release. If it's for internal consumption and all consumers are updated within this PR or related work, it's acceptable. Consider adding a comment to the interface definition noting the change and the reason for it.
List(ctx context.Context, prefix string) ([]string, error)
List(ctx context.Context, prefix string) (map[string]string, error)
}

// Closer is an optional interface for providers that need graceful shutdown
Expand All @@ -116,7 +122,7 @@

// Standalone is an optional interface for providers that do not need
// to be combined with caching or singleflight mechanisms.
type Standalone interface {

Check warning on line 125 in kv/provider.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Rename this interface to follow Go naming conventions for single-method interfaces.

See more on https://sonarcloud.io/project/issues?id=TykTechnologies_storage&issues=AZ9wjF4NMh4lWgnPSwAO&open=AZ9wjF4NMh4lWgnPSwAO&pullRequest=155
IsStandalone() bool
}

Expand All @@ -126,6 +132,12 @@
Timeout() time.Duration
}

// AsSetter attempts to extract a Setter from a Provider,
// automatically unwrapping decorators.
func AsSetter(p Provider) (Setter, bool) {
return As[Setter](p)
}

// AsLister attempts to extract a Lister from a Provider,
// automatically unwrapping decorators.
func AsLister(p Provider) (Lister, bool) {
Expand Down
63 changes: 56 additions & 7 deletions kv/providers/consul/consul.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@
import (
"context"
"encoding/json"
"errors"
"fmt"
"strings"
"time"

"github.com/TykTechnologies/storage/kv"
"github.com/hashicorp/consul/api"
consulsdk "github.com/hashicorp/consul/api"
)

// Config is the JSON "config" block of a consul store.
Expand Down Expand Up @@ -62,7 +64,7 @@
// JSON, an unparseable wait_time, or TLS settings the client rejects (e.g. an
// unreadable ca_file). Absent config is valid — an empty blob builds a client
// against consul's default local agent.
func NewFactory() kv.ProviderFactory {

Check failure on line 67 in kv/providers/consul/consul.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 26 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=TykTechnologies_storage&issues=AZ9wjF0aMh4lWgnPSwAK&open=AZ9wjF0aMh4lWgnPSwAK&pullRequest=155
return func(rawJSON json.RawMessage) (kv.Provider, error) {
var conf Config

Expand All @@ -74,7 +76,7 @@
}
}

clientCfg := api.DefaultConfig()
clientCfg := consulsdk.DefaultConfig()

if conf.Address != "" {
clientCfg.Address = conf.Address
Expand All @@ -89,7 +91,7 @@
}

if conf.HttpAuth.Username != "" || conf.HttpAuth.Password != "" {
clientCfg.HttpAuth = &api.HttpBasicAuth{
clientCfg.HttpAuth = &consulsdk.HttpBasicAuth{
Username: conf.HttpAuth.Username,
Password: conf.HttpAuth.Password,
}
Expand All @@ -112,7 +114,7 @@

applyTLSConfig(clientCfg, &conf)

client, err := api.NewClient(clientCfg)
client, err := consulsdk.NewClient(clientCfg)
if err != nil {
return nil, fmt.Errorf("consul: create client: %w", err)
}
Expand All @@ -121,7 +123,7 @@
}
}

func applyTLSConfig(clientCfg *api.Config, conf *Config) {
func applyTLSConfig(clientCfg *consulsdk.Config, conf *Config) {
tls := conf.TLSConfig

if tls.Address != "" {
Expand Down Expand Up @@ -153,7 +155,7 @@
type consulProvider struct {
// kvClient is consul's KV endpoint, with the resolved Config already baked
// into the underlying client.
kvClient *api.KV
kvClient *consulsdk.KV
}

// Get reads the value at key and returns it verbatim: no trimming, no key
Expand All @@ -164,7 +166,7 @@
// distinguishes. ctx bounds the request via QueryOptions, so the SecretStore's
// per-operation deadline is honored.
func (cp *consulProvider) Get(ctx context.Context, key string) (string, error) {
pair, _, err := cp.kvClient.Get(key, (&api.QueryOptions{}).WithContext(ctx))
pair, _, err := cp.kvClient.Get(key, (&consulsdk.QueryOptions{}).WithContext(ctx))
if err != nil {
return "", &kv.StoreUnavailableError{KeyPath: key, Err: err}
}
Expand All @@ -175,3 +177,50 @@

return string(pair.Value), nil
}

// Set writes value verbatim as the raw bytes at key (PUT /v1/kv/<key>), with no
// key transformation or interpretation.
// A transport or backend failure returns *kv.StoreUnavailableError.
func (cp *consulProvider) Set(ctx context.Context, key, value string) error {
pair := &consulsdk.KVPair{
Key: key,
Value: []byte(value),
}

_, err := cp.kvClient.Put(pair, (&consulsdk.WriteOptions{}).WithContext(ctx))
if err != nil {
return &kv.StoreUnavailableError{KeyPath: key, Err: err}
}

return nil
}

// List returns every key/value pair under prefix, keyed by the FULL consul key

Check failure on line 198 in kv/providers/consul/consul.go

View check run for this annotation

probelabs / Visor: security

security Issue

The `Set` function writes to a given `key` in Consul without enforcing any path restrictions or prefix validation. If the `key` is controllable by an attacker, this could allow writing to arbitrary paths in the Consul KV store, potentially leading to configuration overwrite, privilege escalation, or denial of service. The scope of the damage is limited only by the permissions of the Consul token in use.
Raw output
Introduce a `write_prefix` in the Consul provider configuration. Before writing, validate that the provided `key` is within this configured prefix. If the prefix is not configured, either disallow writes or log a prominent security warning.
// (the caller strips the prefix if it wants relative keys). Consul directory
// markers — keys ending in "/" — are skipped; they are not real entries.
//
// An empty prefix is rejected: consul would treat it as "list the entire KV
// store", which is never what a reference resolver wants and is an easy footgun.
// A prefix that matches nothing is not an error — it returns an empty map.
func (cp *consulProvider) List(ctx context.Context, prefix string) (map[string]string, error) {
if prefix == "" {
return nil, errors.New("consul: list requires a non-empty prefix")
}

pairs, _, err := cp.kvClient.List(prefix, (&consulsdk.QueryOptions{}).WithContext(ctx))
if err != nil {
return nil, &kv.StoreUnavailableError{KeyPath: prefix, Err: err}
}

out := make(map[string]string, len(pairs))

Check warning on line 216 in kv/providers/consul/consul.go

View check run for this annotation

probelabs / Visor: performance

performance Issue

The `List` function fetches all key-value pairs under a given prefix and loads them entirely into memory. This is dictated by the `Lister` interface change in `kv/provider.go:113` which now returns `map[string]string`. If a prefix matches a large number of keys, this implementation can lead to excessive memory allocation and high latency, potentially causing Out-Of-Memory errors or service degradation.
Raw output
For use cases that might return a large number of items, consider redesigning the `Lister` interface to support streaming or pagination, for example by returning a channel or an iterator. If changing the interface is not feasible, ensure that this function is only called with prefixes known to match a small number of keys and document this limitation clearly in the `Lister` interface comments.
for _, p := range pairs {
if strings.HasSuffix(p.Key, "/") {
continue
}

out[p.Key] = string(p.Value)
}

Check warning on line 224 in kv/providers/consul/consul.go

View check run for this annotation

probelabs / Visor: security

security Issue

The `List` function checks for an empty prefix, which is a good mitigation against listing the entire KV store. However, it does not enforce any other restrictions on the prefix. A broad prefix (e.g., '/') could still be provided, leading to high resource consumption on the Consul server and potentially exposing more data than intended by the caller.
Raw output
Consider introducing a configurable `allowed_list_prefixes` or a single `root_prefix` in the provider configuration to ensure that `List` operations are constrained to intended namespaces, preventing overly broad queries.
return out, nil
}
Loading
Loading