remote.vault adds "data" to the url at the wrong position

[PatMis16]

What's wrong?

When using remote.vault in the alloy configuration, w have the issue, that "data" is added to toe URL twice:

solution/grafana-it/kv/alloy/it10

becomes to

solution/data/grafana-it/kv/alloy/it10

However, it should be

solution/grafana-it/kv/data/alloy/it10

Because the url is wrong, we get

Error: it10:11:5: Failed to build component: building component: failed to get token: error encountered while reading secret at solution/data/grafana-it/kv/alloy/it10: Error making API request.

URL: GET https://<redacted>/v1/solution/data/grafana-it/kv/alloy/it10
Code: 403. Errors:

* 1 error occurred:
        * permission denied



Error: it10:24:57: field "oracle_padasa_int" does not exist

Steps to reproduce

We use the following configuration:

remote.vault "oracle_secret" {
        server = "https://<redacted>"
        path = "solution/grafana-it/kv/alloy/it10"

        auth.approle {
            role_id = "<redacted>"
            secret = "<redacted>"
            mount_path = "approle"
        } 
    }

System information

Ubuntu 22.04.4 LTS running on WSL2

Software version

Grafana Alloy version v1.3.1 (branch: HEAD, revision: e4979b2)

Configuration

remote.vault "oracle_secret" {
        server = "https://<redacted>"
        path = "solution/grafana-it/kv/alloy/it10"

        auth.approle {
            role_id = "<redacted>"
            secret = "<redacted>"
            mount_path = "approle"
        } 
    }

Logs

Error: it10:11:5: Failed to build component: building component: failed to get token: error encountered while reading secret at solution/data/grafana-it/kv/alloy/it10: Error making API request.

URL: GET https://<redacted>/v1/solution/data/grafana-it/kv/alloy/it10
Code: 403. Errors:

* 1 error occurred:
        * permission denied



Error: it10:24:57: field "oracle_padasa_int" does not exist

[fnerdwq]

having exactly the same error here -> it might be better to split the parameters into path and secret explicitly

This split is the problem
https://github.com/grafana/alloy/blob/v1.3.1/internal/component/remote/vault/client.go#L23

[PatMis16]

@fnerdwq
So in the file internal/component/remote/vault/vault.go a variable for the secret would have to be added:

// Arguments configures remote.vault.
type Arguments struct {
	Server    string `alloy:"server,attr"`
	Namespace string `alloy:"namespace,attr,optional"`

	Path string `alloy:"path,attr"`
	Secret string `alloy:"secret,attr"`

	RereadFrequency time.Duration `alloy:"reread_frequency,attr,optional"`

	ClientOptions ClientOptions `alloy:"client_options,block,optional"`

	// The user *must* provide exactly one Auth blocks. This must be a slice
	// because the enum flag requires a slice and being tagged as optional.
	//
	// TODO(rfratto): allow the enum flag to be used with a non-slice type.

	Auth []AuthArguments `alloy:"auth,enum,optional"`
}

Then in the file internal/component/remote/client.go we have to read both vars:

func (ks *kvStore) Read(ctx context.Context, args *Arguments) (*vault.Secret, error) {
	kv := ks.c.KVv2(args.Path)
	kvSecret, err := kv.Get(ctx, args.Secret)
	if err != nil {
		return nil, err
	}

	// kvSecret.Data contains unwrapped data. Let's assign that to the raw secret
	// and return it. This is a bit of a hack, but should work just fine.
	kvSecret.Raw.Data = kvSecret.Data
	return kvSecret.Raw, nil
}

What do you think?

[PatMis16]

I built it locally and it seems to work. Can simply fork an create a PR for that?

[fnerdwq]

@PatMis16 yes thanks, this look perfectly right. Just wondering about the naming. Probably instead of secrets (I know, I suggested it myself...) the parameter key would be better? See https://developer.hashicorp.com/vault/docs/commands/kv/get

A "secret" is read under a "path" with given "key". What do you think?

[PatMis16]

@fnerdwq You might be right. I change that accordingly.

[carlos-lehmann]

@PatMis16 yes thanks, this look perfectly right. Just wondering about the naming. Probably instead of secrets (I know, I suggested it myself...) the parameter key would be better? See https://developer.hashicorp.com/vault/docs/commands/kv/get

A "secret" is read under a "path" with given "key". What do you think?

While you are not wrong @fnerdwq I think this can be very misleading, since a secret consist of a key and a value. One could assume defining key as a value in remote.vault would be the key of the secret, but what you are referring to is rather the secret_path. The documentation states: The key to retrieve a secret from. does give it away a bit but is not clear enough I think.

But not just that I'd argue that introducing this flexibility changes the scope of path which originally was the actual secret path mentioned above, including the mount. But using this flexible option you suddenly switch path to the part before /data/ which usually consist of namespace/mount (namespace obviously optional) and that's not really path anymore?

Before:
path = mount/path/to/mysecret
namespace = mynamespace (optional)

which translates to something like: /v1/mynamespace/data/mount/path/to/mysecret
which is what the @PatMis16 raised as a problem because actually it should be: /v1/mynamespace/mount/data/path/to/mysecret

Now with key added or rather the flexible configuration:

path = mount
key = path/to/mysecret

the path to my secret is suddenly defined in key. and the mount is suddenly in path.
Here also namespace is likely irrelevant as you can just specify it in path: mount/namespace

Comparing this to vault kv get you usually have something like this:
vault kv get -field=<key-of-the-secret> -mount=<mount-path> -namespace=<namespace> <path/to/your/secret>
You can also specify it without:

~  vault kv get namespace/mount/path/to/your/secret
============================================ Secret Path ============================================
namespace/mount/data/path/to/your/secret

I'm not sure if this helps but I think you should review your wording and configuration again