Just build a new AKS cluster (1.12.5) and when initially deploying the helm chart none of the secret store pods in the dev namespace are able to run do to a volume not being mounted.

Warning FailedMount 1m kubelet, aks-nodepool1-26873499-2 Unable to mount volumes for pod "csi-secrets-store-secrets-store-csi-driver-bg2wf_dev(385cb061-3a0f-11e9-8715-4e92e05d98d9)": timeout expired waiting for volumes to attach or mount for pod "dev"/"csi-secrets-store-secrets-store-csi-driver-bg2wf". list of unmounted volumes=[registration-dir]. list of unattached volumes=[socket-dir mountpoint-dir registration-dirazure-cred msi csi-driver-registrar-token-vfmtq]

Ref: Azure/kubernetes-keyvault-flexvol#128

Update from csi.storage.k8s.io/v1alpha1 to storage.k8s.io/v1beta1 when old apiversion is no longer supported as per #38

Need to document HashiCorp Vault provider in detail.

• secrets-store-csi-driver
• secrets-store-csi-driver-provider-azure
• secrets-store-csi-driver-provider-vault

kubernetes/kubernetes#64984
kubernetes/kubernetes#67945
kubernetes/enhancements#716

• Should not end in runtime panic when URL is bad
• Handle first path segment in URL cannot contain colon for URL starting with IP
• When endpoint is not correct, should return error to user, do not mount

When creating 2 different persistent volumes for 2 different pods, if the first one contains an error, the driver will keep trying to get the secret out of Vault before handling the second volume/pod or any other.

In the logs I kept seeing the objectPath from an unrelated secret. You would expect the driver to get secrets in parallel or in a queue but it remains stuck on the first one even if it gives errors forever?
kubectl logs -f csi-vault-secrets-store-csi-driver-8gjxm -c secrets-store

provider.go:64] NewProvider 
objectsStrings: [array:                                                                                                                                                            
  - |                                                                                                                                                                                  objectPath: "/secret/nexus"                                                                                                                                          
    objectName: "username"                                                                                                                                                             objectVersion: ""                                                                                                                                                              
]
...

Just tried to run the example with CS In Line Volume, but my secret path in HashiCorp Vault is "sandbox/k8s_infra/marketplace/account/my-new-app/db"
I spected that driver requests the path "/sandbox/data/k8s_infra/marketplace/account/my-new-app/db" instead its try to request "secret/data/sandbox/k8s_infra/marketplace/account/my-new-app/db".

Here my pod spec:

kind: Pod
apiVersion: v1
metadata:
  name: nginx-secrets-store-inline
  namespace: my-new-app
spec:
  containers:
  - image: nginx
    name: nginx
    volumeMounts:
    - name: secrets-store-inline
      mountPath: "/mnt/secrets-store"
      readOnly: true
  volumes:
    - name: secrets-store-inline
      csi:
        driver: secrets-store.csi.k8s.com
        readOnly: true
        volumeAttributes:
          providerName: "vault" # Vault ou azure
          roleName: "k8s_sre_infra_sandbox_own_namespace_role"
          vaultAddress: "https://vault.dc.infra.ifood-sandbox.com.br"
          objects:  |
            array:
              - |
                objectPath: "sandbox/k8s_infra/marketplace/account/my-new-app/db"
                objectName: "db_password"
              - |
                objectPath: "sandbox/k8s_infra/marketplace/account/my-new-app/app"
                objectName: "bla"

Looks like NodeUnpublishVolume is not being called when targetPath is not mounted properly.

This is a suggestion for a new feature. In addition to specifying the objectType and objectName, it would be good to include something like a outputFileName. There are times when a service needs different secrets for.e.g based on the region/environment although these are present in the same keyvault. If we support the outputFileName, the underlying service can still be agnostic of this.

For e.g Let us say there is a CognitiveServicesWestUs key and a CognitiveServicesEastUs key.
The definition for pod in westus would look like
objects: |
array: # array of objects

• |
  objectName: CognitiveServicesWestUs
  objectType: secret # object types: secret, key or cert
  outputFileName: CognitiveServicesKey

The definition for pod in eastus would look like
objects: |
array: # array of objects

• |
  objectName: CognitiveServicesEastUs
  objectType: secret # object types: secret, key or cert
  outputFileName: CognitiveServicesKey
  objectVersion: "" # [OPTIONAL] object versions, default to latest if empty

It is very easy to use control structures in helm to specify the keys here in the yaml as opposed to the underlying service making the decision.

Also when you add new regions for e.g. it would not require changes in the underlying service.

move main packages out of pkg/ and into a cmd/ directory to designate that they are not shared packages.

part of #92

• deployment yamls and instructions for v1.13 and v1.15 seems to be almost identical from what i can tell
  https://github.com/deislabs/secrets-store-csi-driver/tree/master/pkg/providers/vault#install-the-secrets-store-csi-driver-kubernetes-version-113x
  https://github.com/deislabs/secrets-store-csi-driver/tree/master/pkg/providers/vault#install-the-secrets-store-csi-driver-kubernetes-version-115x
  Should this just split up PV (for v1.13+) vs inline volume (for v1.15+)?

• does vault provider work with Helm installation? seems a bit confusing to have instructions on main readme use Helm and then vault has yamls for deployment

• on the vault setup, vault role creation seems to need the namespace which is used after the k8s components are created. since this is preliminary setup, this seems to be out of order slightly.
  https://github.com/deislabs/secrets-store-csi-driver/blob/master/pkg/providers/vault/docs/vault-setup.md#vault-provider-for-secret-store-csi-driver

As really briefly outlined at KubeCon: It would be super awesome if it would be possible to just have the secrets mounted to the pod for a very limited amount of time, just enough to pick them up and store them in memory.

The use case for this would typically be things like database credentials - the pod starts, picks up the credential, and then it wouldn't need it anymore. It feels (almost) just as wrong as using environment variables to leave credentials laying around inside the pod as a permanent mount; anybody who would be able to open up a terminal (or similar) inside the container would be able to read them out.

Having the credentials only at startup could prevent these to leak too easily. Admitted, they would probably still be in memory then, but reading out memory from a running process is not that easy, especially if you don't have root rights inside the container.

Implementation options

• Have a certain amount of time in which the secrets are available
• Couple it e.g. to pod readiness, and take away the credentials when the pod has signalized it's ready (and thus has already read the credentials)
• Allow for deletion of the credential files; the process can then simply delete the credential file when it has read them

This enhancement is inspired by the way how HashiCorp Vault can handle secrets: Giving out one time access tokens to retrieve the secrets directly from vault; even if this one time token leaks, nobody can make use of it, as it has already been used. This proposal would serve the same type of purpose - making sure it's as difficult as possible to steal your credentials, even if you happen to get access to the running pod.

Currently, Only Kubernetes Auth Method is supported as a method to authenticate to Vault from HashiCorp Vault Provider, but what do you think about supporting TLS Certificates Auth Method in addition?

I think that it is good specification ( I am currently investigating how to implement it and whether it is a possible specification... Please let me know if there is other good spec... ) to pass a TLS certificate from the Pod Spec's volume to Secrets Store CSI Driver running as a Node Plugin and use it to authenticate to Vault. The authentication method is specified with a parameter such as authMethod (default: k8s), and the file to be used in the Volume is specified with a parameter such as cert.

example1. Pass tls cert using config map

kind: Pod
apiVersion: v1
metadata:
  name: nginx-secrets-store-inline
spec:
  containers:
  - image: nginx
    name: nginx
    volumeMounts:
    - name: secrets-store-inline
      mountPath: "/mnt/secrets-store"
      readOnly: true
  volumes:
    - name: secrets-store-inline
      csi:
        driver: secrets-store.csi.k8s.com
        readOnly: true
        volumeAttributes:
          providerName: "vault"
          authMethod: "cert" # New param (default: k8s)
          cert: cert.pem # New param
          roleName: "example-role"
          vaultAddress: "http://vault:8200"
          vaultSkipTLSVerify: "true"
          objects:  |
            array:
              - |
                objectPath: "/foo"
                objectName: "bar"
                objectVersion: ""
    - name: certs
      configMap:
        name: certs

example2. Pass tls cert using share volume

kind: Pod
apiVersion: v1
metadata:
  name: nginx-secrets-store-inline
spec:
  initContainers:
    - name: tls-certs-fetcher
      image: tls-certs-fetcher
      command:
        - tls-certs-fetch
      args:
        - -write
        - /certs
      volumeMounts:
        - name: certs-dir
          mountPath: /certs
  containers:
  - image: nginx
    name: nginx
    volumeMounts:
    - name: secrets-store-inline
      mountPath: "/mnt/secrets-store"
      readOnly: true
  volumes:
    - name: secrets-store-inline
      csi:
        driver: secrets-store.csi.k8s.com
        readOnly: true
        volumeAttributes:
          providerName: "vault"
          authMethod: "cert" # New param (default: k8s)
          cert: cert.pem # New param
          roleName: "example-role"
          vaultAddress: "http://vault:8200"
          vaultSkipTLSVerify: "true"
          objects:  |
            array:
              - |
                objectPath: "/foo"
                objectName: "bar"
                objectVersion: ""
    - name: certs-dir
      emptyDir: {}

there are mentions of apache license in the repo (while repo is MIT license)

Provide commands the user could use for observability

Issue:

After I go through the steps required to setup the secret-store-csi-driver, I see that we log the client secret used to talk to the keyvault is logged.
Provider Used: Azure Keyvault Porvider
This could cause compliance issues since it is common to hookup these logs with external log analytic tools like OMS on azure. It is a way for the secret to go out of the kubernetes cluster.

Steps to Reproduce

1. Setup a CSI Driver, PV, PVC and a POD to fetch secrets from keyvault. Make sure that the secrets are deployed on a POD
2. kubectl get pods -n
3. For each of the pods, do kubectl logs csi-secrets-store-driver-secrets-store-csi-driver-c5xd9 -n csi-azure-secretsv1 |grep secret
4. You will find the below result atleast for one of them
   I0822 18:54:53.160864 1 utils.go:97] secrets:<key:"clientid" value:"*****" > secrets:<key:"clientsecret" value:" ** The actual client secret is logged here **" >

Expected Behaviour:

The client secrets are not logged or logged as ****

• use node-driver-registrar

Controller server races. Read operations need to hold the lock that guards cs.vols.

part of #92

I would strongly recommend enable sanity test for CSI driver, it will help you find lots of issues in driver implementation, e.g. in create volume scenario, it has cases like 1) create a volume with existing name but with different capacity 2) create a volume with existing name and same capacity

Here is the example how sanity test is enabled in azure file driver:
https://github.com/kubernetes-sigs/azurefile-csi-driver/tree/master/test/sanity

And we fixed lots of sanity test failures in azure file & azure disk drivers:
kubernetes-sigs/azurefile-csi-driver#27
kubernetes-sigs/azuredisk-csi-driver#46

cc @ritazh

Bind mounting "/tmp" to the targetPath seems odd

part of #100

• run unit test
• run e2e test
• update doc
• badge

I tried to run "HashiCorp Vault Provider for Secret Store CSI Driver" on minikube according to the guide.

1. Start kubernetes cluster (v1.15.2) using minikube.

$ minikube start --feature-gates=CSIInlineVolume=true

Versions of minikube and docker are below.

$ minikube version: v1.3.0
commit: 43969594266d77b555a207b0f3e9b3fa1dc92b1f

$ docker version
Client: Docker Engine - Community
 Version:           19.03.1
 API version:       1.40
 Go version:        go1.12.5
 Git commit:        74b1e89
 Built:             Thu Jul 25 21:18:17 2019
 OS/Arch:           darwin/amd64
 Experimental:      false

Server: Docker Engine - Community
 Engine:
  Version:          19.03.1
  API version:      1.40 (minimum version 1.12)
  Go version:       go1.12.5
  Git commit:       74b1e89
  Built:            Thu Jul 25 21:17:52 2019
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          v1.2.6
  GitCommit:        894b81a4b802e4eb2a91d1ce216b8817763c29fb
 runc:
  Version:          1.0.0-rc8
  GitCommit:        425e105d5a03fabd737a126ad93d62a9eeede87f
 docker-init:
  Version:          0.18.0
  GitCommit:        fec3683

2. Set up a vault cluster

I created a development HashiCorp Vault cluster in Kubernetes according to the following guide.
https://github.com/deislabs/secrets-store-csi-driver/blob/master/pkg/providers/vault/docs/vault-setup.md

3. Install the Secrets Store CSI Driver

I installed the Secrets Store CSI Driver according to the following guide.
https://github.com/deislabs/secrets-store-csi-driver/tree/master/pkg/providers/vault#install-the-secrets-store-csi-driver-kubernetes-version-115x

But creating the anubhavmishra/secrets-store-csi:v0.0.3 container failed.

$ kubectl apply -f pkg/providers/vault/examples/secrets-store-csi-driver.yaml

$ kubectl get po
NAME                           READY   STATUS              RESTARTS   AGE
csi-secrets-store-7gdsh        1/2     RunContainerError   0          47m
csi-secrets-store-attacher-0   1/1     Running             0          47m
vault-6df55d6866-js8zc         1/1     Running             0          54m

$ kubectl describe pod csi-secrets-store-7gdsh
...
Events:
  Type     Reason     Age   From               Message
  ----     ------     ----  ----               -------
  Normal   Scheduled  48m   default-scheduler  Successfully assigned default/csi-secrets-store-7gdsh to minikube
  Normal   Pulling    48m   kubelet, minikube  Pulling image "gcr.io/gke-release/csi-driver-registrar:v1.0.1-gke.0"
  Normal   Pulled     48m   kubelet, minikube  Successfully pulled image "gcr.io/gke-release/csi-driver-registrar:v1.0.1-gke.0"
  Normal   Created    48m   kubelet, minikube  Created container driver-registrar
  Normal   Started    48m   kubelet, minikube  Started container driver-registrar
  Normal   Pulling    48m   kubelet, minikube  Pulling image "anubhavmishra/secrets-store-csi:v0.0.3"
  Normal   Pulled     48m   kubelet, minikube  Successfully pulled image "anubhavmishra/secrets-store-csi:v0.0.3"
  Normal   Created    48m   kubelet, minikube  Created container secrets-store
  Warning  Failed     48m   kubelet, minikube  Error: failed to start container "secrets-store": Error response from daemon: OCI runtime create failed: open /var/run/docker/runtime-runc/moby/secrets-store/state.json: no such file or directory: unknown

How can I resolve this problem...?

NodePublishVolumeRequest.ReadOnly should be validated.

part of #100

The introduction of Vault ( #23 ) uses the permissions associated inside where the driver is running to authenticate to Vault (and therefore will likely have a superset of policies to serve up secrets).

Currently pods use ServiceAccounts as identity. Vault will consume the serialized form of the ServiceAccount for authentication. This makes it easy for pods to authenticate, but comes with issue that Vault could use that ServiceAccount token to masquerade as the service in question. Realistically this is an acceptable tradeoff.

In order for the Vault CSI driver to identify as a pod to Vault, it needs access to its ServiceAccount; but now the risk surface increases quite substantially.

Bound ServiceAccounts are currently under discussion ( Proposal: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/auth/bound-service-account-tokens.md Blog Post: https://thenewstack.io/no-more-forever-tokens-changes-in-identity-management-for-kubernetes/ ), this would be an ideal use case for them.

The current mechanism where people use init/sidecar containers to manage Vault secrets keeps the ServiceAccount inside the pod other than when authenticating.

also: hashicorp/vault#7365

(cc: @anubhavmishra @ritazh )

tldr: One suggestion for improvement: ensure "pod portability" across clusters while using this driver.

Problem

This driver asks users to specify the secret provider inline in pod definition. For example:

volumes:
  - name: secrets-store-inline
    csi:
      driver: secrets-store.csi.k8s.com
      readOnly: true
      volumeAttributes:
        providerName: "azure" <-- Avoid this

The problem with that is, if you use this pod definition and decide to move your pod to a non-azure cluster, you'll have to update your pod.

Background

In general, workload (pod) portability is something we try very hard to maintain in Kubernetes. With persistent volumes, for example, we discourage specifying a specific volume (e.g. AzureDisk or GCE PD) inline in a pod definition, because it makes the pod definition non-portable: i.e. if you move a pod directly referencing a GCE PD to a non-Google cluster, the pod won't work. So for persistent volumes, we encourage the use of PVC/PV so that your pod/PVC definition remains portable across clusters.

In this case, since this driver is ephemeral, PV/PVC wouldn't be appropriate.

Suggestion

One way to address this would be to create a layer of abstraction similar to a StorageClass for secret providers. Maybe call it SecretProviderClass? Similar to StorageClass, this new non-namespaced SecretProviderClass object would specify 1) the secret provider to use and
2) the (opaque) parameters, if any, to pass to it during provisioning. For example:

kind: SecretProviderClass
metadata:
  name: my-secret-provider
provisioner: azure
parameters:
  someStoreSpecificParameter: foo

On the pod side, instead of referencing specific providerName, the user would specify the name of a secretProviderClass:

volumes:
  - name: secrets-store-inline
    csi:
      driver: secrets-store.csi.k8s.com
      readOnly: true
      volumeAttributes:
        secretProviderClass: "my-secret-provider"
        ...

The pod volumeAttributes should only contain paramters that are generic and are understood by any secret provider (values here can be unique per pod). This assumes that you can define a generic way to identify a secret (and any other pod specific fields that you need).

The SecretProviderClass.parameters should can contain provider specific parameters (values here are can not be specific per provider).

This way, the user could move the pod object from one cluster to another, and not worry about who the secret provider is. As long as there is a SecretProviderClass named my-secret-provider, things will just work.

make the provider volume path configurable using a flag instead of the env variable.

part of #92

Starting as a running list as I review the code here for kubernetes/org#1245.

Ability to set cloud name to enable azure provider to work in Sovereign cloud.

log redaction of secrets is fragile. This is complex and is whitelist based.

part of #92

Comments on helm chart:

• Remove crd-csi-driver-registry.yaml. This is the alpha CRD that was used in 1.13, but since 1.14, K8s is using the in-tree v1beta object: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.14/#csidriver-v1beta1-storage-k8s-io
• Set CSIDriver.AttachedRequired = false in https://github.com/deislabs/secrets-store-csi-driver/blob/master/charts/secrets-store-csi-driver/templates/csidriver.yaml. This will allow you to delete csi-secrets-store-attacher.yaml entirely
• csi-attacher service account and roles/rolebindings can be removed once attacher is removed
• node-driver-registrar shouldn't require any rbacs anymore because it doesn't call k8s apis. So you can probably rename your service account to your driver name or something for the secretsClass rbacs. Also check with sig-auth on these rbacs. In general I recommend the names of objects, especially for non-namespaced objects like ClusterRoleBindings, to be specific, to not cause naming conflicts with other csi drivers.

part of #100

move init into handle. Does os.Exit(0) at the end of the main do anything? It looks like it should be removed.

part of #92

Assuming driver is only supported in 1.15+

Comments on helm chart:

• Remove crd-csi-driver-registry.yaml. This is the alpha CRD that was used in 1.13, but since 1.14, K8s is using the in-tree v1beta object: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.14/#csidriver-v1beta1-storage-k8s-io
• Set CSIDriver.AttachedRequired = false in https://github.com/deislabs/secrets-store-csi-driver/blob/master/charts/secrets-store-csi-driver/templates/csidriver.yaml. This will allow you to delete csi-secrets-store-attacher.yaml entirely
• csi-attacher service account and roles/rolebindings can be removed once attacher is removed
• node-driver-registrar shouldn't require any rbacs anymore because it doesn't call k8s apis. So you can probably rename your service account to your driver name or something for the secretsClass rbacs. Also check with sig-auth on these rbacs. In general I recommend the names of objects, especially for non-namespaced objects like ClusterRoleBindings, to be specific, to not cause naming conflicts with other csi drivers.

Comments on csi node implementation:

• In NodePublishVolume, are providers required to mount the volume, or do they just populate the directory with data?
• Bind mounting "/tmp" to the targetPath seems odd: https://github.com/deislabs/secrets-store-csi-driver/blob/51d64fcf33d62b84143f666ed8bc387011652de5/pkg/secrets-store/nodeserver.go#L164. What happens if the driver container restarts? I'm guessing since bind mounting is on the inode, the existing container will continue to work, and Unpublish should also work too.
• Double check idempotency of NodePublish/Unpublish. For example, if NodePublish gets called twice, and it's already correctly mounted, we should return success. Looking at the current code, it seems like we'll bind mount it twice? Which can potentially cause mounts to leak on cleanup. Similarly, Unpublish should return success if it's already unpublished.
• Usage of IsLikelyNotMountPoint() also doesn't work if the bind mount is from the same fs as the kubelet's rootfs (if you look at the implementation, it just checks if the parent directory is in the same fs or not).
• results from https://github.com/deislabs/secrets-store-csi-driver/blob/51d64fcf33d62b84143f666ed8bc387011652de5/pkg/secrets-store/nodeserver.go#L220 are unused?
• Make sure providers don't include any sensitive info in the error msg: https://github.com/deislabs/secrets-store-csi-driver/blob/51d64fcf33d62b84143f666ed8bc387011652de5/pkg/secrets-store/nodeserver.go#L213
• Do you only support read-only? If so, NodePublishVolumeRequest.ReadOnly should be validated.

General architecture questions:

• Can contents of the secrets change dynamically? Would you want the volume to be updated with the latest content, similar to how the current Secrets volume works?

I didn't review the CreateVolume/PV implementations since that's the legacy model. I also didn't review any of the providers

• Double check idempotency of NodePublish/Unpublish. For example, if NodePublish gets called twice, and it's already correctly mounted, we should return success. Looking at the current code, it seems like we'll bind mount it twice? Which can potentially cause mounts to leak on cleanup. Similarly, Unpublish should return success if it's already unpublished.
• Usage of IsLikelyNotMountPoint() also doesn't work if the bind mount is from the same fs as the kubelet's rootfs (if you look at the implementation, it just checks if the parent directory is in the same fs or not).
• results from https://github.com/deislabs/secrets-store-csi-driver/blob/51d64fcf33d62b84143f666ed8bc387011652de5/pkg/secrets-store/nodeserver.go#L220 are unused?

part of #100

This is a suggestion for a feature. Within a cluster, there would be different services deployed in different pods that might make use of the different secrets/keys/certs within a pod. It would be great if the resources can be fetched based on a regex or tags(in the case of azure).
For e.g. Pod A needs secret1, secret2 and Pod B needs Secret2, Secret3.
Secret 1 has tag (Key: Pod, Value: Pod A)
Secret 2 has tag (Key: Pod, Value: Pod A ,Pod B)
Secret 3 has tag (Key: Pod, Value: PodB)

Once tagging these resources are done when creating each of these secrets, the pods as such just need something like

objects: |
array: # array of objects
- |
objectTagKey: (string/regex)
objectTagValue: (string/regex)
objectType: secret # object types: secret, key or cert
objectVersion: "" # [OPTIONAL] object versions, default to latest if empty