KV2 secrets on nested path does not work

[ricoberger]

If the KV2 secrets engine is enabled under a nested path the operator does not work for these secrets. Steps to reproduce:

vault secrets enable -path=kv2/on/nested/path -version=2 kv

vault kv put kv2/on/nested/path/example-vaultsecret foo=bar

cat <<EOF | kubectl apply -f -
apiVersion: ricoberger.de/v1alpha1
kind: VaultSecret
metadata:
  name: kv2-example-vaultsecret-1
spec:
  # Incorrect path.
  path: kv2/on/nested/path/example-vaultsecret
  secretEngine: kv2
  type: Opaque
EOF

cat <<EOF | kubectl apply -f -
apiVersion: ricoberger.de/v1alpha1
kind: VaultSecret
metadata:
  name: kv2-example-vaultsecret-2
spec:
  # Correct path with the data part.
  path: kv2/on/nested/path/data/example-vaultsecret
  secretEngine: kv2
  type: Opaque
EOF

The first example contains an invalid path, the operator would look for the secret at kv2/data/on/nested/path/example-vaultsecret, which is also not valid.

The second example contains the correct path with the data part in it, but the operator looks under the following path for the secret: kv2/data/on/nested/path/data/example-vaultsecret.

The operator only looks at the second part of the path. If this part is not data, data will be added at the second position. This behavior is incorrect, if the secret engine is enabled under a nested path.

Possible solutions:

• Remove the path check at L169. This would break existing secrets, when they rely on this.

-	if secretEngine == "kv2" {
-		pathParts := strings.Split(path, "/")
-		if len(pathParts) < 2 {
-			return nil, ErrInvalidPath
-		}
-
-		if pathParts[1] != "data" {
-			path = pathParts[0] + "/data/" + strings.Join(pathParts[1:], "/")
-		}
-	}

• Check if the the slice of the path parts contains a data part. If not, we add it on the second position. If yes, we do not modify the path. This would break CRs which contains data in there path on (e.g. kv2/secret/on/nested/path/data).

	if secretEngine == "kv2" {
		pathParts := strings.Split(path, "/")
		if len(pathParts) < 2 {
			return nil, ErrInvalidPath
		}

-		if pathParts[1] != "data" {
+		if !contains("data", pathParts) {
			path = pathParts[0] + "/data/" + strings.Join(pathParts[1:], "/")
		}
	}

• Add a new field secretEnginePath to the CRD. Then the resulting path would be secretEnginePath + path.

[itsmeniko]

Personally, I would not expect the operator to modify the secret path, and instead just get exactly whatever I put there.

[ricoberger]

Hi, thanks for your input. I totally agree with you, normally you should get exactly what you put in the CR.

My only problem here is that the vault kv get kv2/on/nested/path/example-vaultsecret command does not require the data part in the path. If they detect a KV2 secrets engine under the given path, they also add the data part to the path: https://github.com/hashicorp/vault/blob/master/command/kv_get.go#L94

[itsmeniko]

Hi, thanks for this excellent project. hmm yeah I see the conundrum, other operators such as banzaicloud bank-vaults expect the user to specify /data in the secret path since it makes more sense in the context of hashicorp/vault/api pkg. I think this is reasonable approach, but it did result in a clumsy first try when I used that operator.

[ricoberger]

Currently I want to use the same logic as the Vault cli (see #12), but I'm open for suggestions.

@itsmeniko: I would like to improve the readme, do you have some suggestions based on your experience with the operator.

[itsmeniko]

sounds good! The only suggestion I have for the readme is that it should be clear that the helm chart has no default vault.address, or perhaps it should use http://vault:8200 as the default?

happy to submit a pr if you'd like

[ricoberger]

A PR from you would be very welcome.