Update "Windows Storage Support" KEP status to implementable and address prior feedback #1184

ddebroy · 2019-07-30T01:25:40Z

Updates:

Clarify deprecation policy for API members.
Add pointer to golang issue to track supporting detection of domain socket files in Windows.
Compare Linux Node Plugin attack surface to CSI Proxy.
Update diagram and API members from Get => List.
Specify generation of eventlog entries that can be used as an audit trail.

ddebroy · 2019-07-30T01:26:07Z

/sig storage

ddebroy · 2019-07-30T01:27:13Z

/assign @PatrickLang @msau42 @liggitt

msau42 · 2019-07-30T02:22:33Z

keps/sig-windows/20190714-windows-csi-support.md


-![Kubernetes CSI Node Components and Interactions](https://ddebroywin1.s3-us-west-2.amazonaws.com/csi-proxy2.png "Kubernetes CSI Node Components and Interactions")
+![Kubernetes CSI Node Components and Interactions](https://ddebroywin1.s3-us-west-2.amazonaws.com/csi-proxy3.png "Kubernetes CSI Node Components and Interactions")


The image can be checked into this directory directly

keps/sig-windows/20190714-windows-csi-support.md

ddebroy · 2019-07-30T14:25:09Z

/assign @calebamiles

liggitt · 2019-07-30T15:19:40Z

keps/sig-windows/20190714-windows-csi-support.md


 A GRPC based interface, CSIProxyService, will be used by CSI Node Plugins to invoke privileged operations on the host through csi-proxy.exe. CSIProxyService will be versioned and any release of csi-proxy.exe binary will strive to maintain backward compatibility across as many prior stable versions of the API as possible. This avoids having to run multiple versions of the csi-proxy.exe binary on the same Windows host if multiple CSI Node Plugins (that do not depend on APIs in Alpha or Beta versions) have been configured and the version of the csi-proxy API required by the plugins are different. For every version of the API supported, csi-proxy.exe will first probe for and then expose a `\\.\pipe\csi-proxy-v[N]` pipe where v[N] can be v1, v2alpha1, v3beta1, etc. If during the initial probe phase, csi-proxy.exe determines that another process is already listening on a `\\.\pipe\csi-proxy-v[N]` named pipe, it will not try to create and listen on that named pipe. This allows multiple versions of csi-proxy.exe to run side-by-side if absolutely necessary to support multiple CSI Node Plugins that require widely different versions of CSIProxyService that no single version of csi-proxy.exe can support.

-If an old stable version of CSIProxyService, say vN, can no longer be supported, support for it may be dropped from the latest version of csi-proxy.exe. To continue running CSI Node Plugins that depend on the old version vN, Kubernetes administrators will be required to run the latest version of the csi-proxy.exe (that will be used by CSI Node Plugins that use versions of CSIProxyService more recent than vN) along with an old version of csi-proxy.exe that does support vN.
-
 The following are the main RPC calls that will comprise a v1 version of the CSIProxyService API:

 ```


are the RPC messages described below (MkdirRequest, RmdirRequest, PartitionDisk, FormatVolume, LinkVolume, etc) pre-existing or introduced as part of this feature?

since this is exposing an API that allows things running inside containers to escalate permissions to do host-level operations, we need clear information about the following aspects:

what values are allowed in paths

are special characters restricted?

are backstep path segments allowed?

are forward and backslashes (and international path separators like ¥) allowed and normalized?

which of the filepath variants in https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats are allowed?

is the csi-proxy process or the caller responsible for normalization and validation of paths?

do symlink traversal and time-of-check-time-of-use issues exist in windows? for example, could a container create a link in a filesystem it controlled that pointed to a host filepath it should not have access to, then request a CSI volume be mounted at a path beneath that link, and get the privileged csi-proxy to follow the link and overwrite/expose host files to the container? if so, how does the caller indicate to the csi-proxy what the acceptable boundary is for resolved paths that include links?

It's possible some or many of the validation bits are constrained by the CSI spec already and the kubelet is responsible for filtering out some of those things. If so, just noting that and ensuring test coverage of the kubelet is fine.

The symlink/link/traversal issues are more interesting, since the csi-proxy would be the only actor capable of performing those checks.

All the RPCs mentioned are new. Will clarify the path semantics and validations.

Rather than repeating everything in the doc - I think we can reference https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file

@liggitt - that doc you linked to is a subset implemented in .Net. https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfullpathnamew is the native API that should be used to canonicalize file names that follows the rules listed in the prior doc

+1 for linking to existing references, clarifying which API is used, and being explicit about where normalization happens (which component(s) are responsible for doing it)

path normalization on the node side can be done with https://github.com/golang/go/blob/2b8cbc384d092dc63e9dc18ec318d0682185611c/src/syscall/syscall_windows.go#L224

I have updated the Request and Response struct descriptions for the RPCs along with restrictions. /cc @liggitt and @PatrickLang PTAL.

Thanks. Do these map closely to windows filesystem APIs? If so, that would be good to reference.

I'll defer to the windows/storage experts to determine if these provide a reasonable API that covers the information/operations a CSI driver would need.

liggitt · 2019-07-30T15:23:09Z

keps/sig-windows/20190714-windows-csi-support.md

@@ -371,7 +379,7 @@ New YAMLs for DaemonSets associated with CSI Node Plugins needs to be authored t

 ### Risks and Mitigations

-Any pod on a Windows node can be configured to mount `\\.\pipe\csi-proxy-v[N]` and perform privileged operations. Thus csi-proxy presents a potential security risk. To mitigate the risk, here are some options:
+Any pod on a Windows node can be configured to mount `\\.\pipe\csi-proxy-v[N]` and perform privileged operations. Thus csi-proxy presents a potential security risk. To mitigate the risk, some options are described below:


are there already things that could be mounted into pods that allowed arbitrary hostpath mounts that would allow similar escalations/capabilities? if so, the mere existence of the csi proxy isn't really an increased risk for those pods.

the increased risk comes from the csi-proxy handling of data plumbed to it by the csi drivers from restricted user-specified PVCs/Pods that were not allowed hostPath access, hence the data validation questions in https://github.com/kubernetes/enhancements/pull/1184/files#r308785333

Windows doesn't support container mount projection today, so there's no way to create new mounts that can be interpreted by the host.

msau42 · 2019-07-30T15:44:37Z

cc @tallclair

liggitt · 2019-07-30T20:23:55Z

keps/sig-windows/20190714-windows-csi-support.md

@@ -290,7 +296,7 @@ CSI Node Plugins need to execute certain privileged operations at the host level

 Registration of CSI Node Plugins on a Kubernetes node is handled by the Kubelet plugin watcher using the fsnotify package. This component needs to convert paths detected by fsnotify to Windows paths in handleCreateEvent() and handleDeleteEvent() before the paths are passed to AddOrUpdatePlugin() RemovePlugin() routines in desiredStateOfTheWorld. A new utility function, NormalizeWindowsPath(), will be added in utils to handle this.

-Given `os.ModeSocket` is not set on a socket file's `os.FileMode` in Windows, a specific check for `os.ModeSocket` in handleCreateEvent() will need to be relaxed.
+Given `os.ModeSocket` is not set on a socket file's `os.FileMode` in Windows (due to golang [issue](https://github.com/golang/go/issues/33357)), a specific check for `os.ModeSocket` in handleCreateEvent() will need to be relaxed for Windows until the golang issue is addressed.


is there an alternative check that can be done on windows to avoid spawning lots of events for normal files/dirs (like ensuring it is not a directory, and is not a regular file)? I know there were bugs with the plugin manager getting overwhelmed trying to process change events for non-socket files under that dir.

The code fragment I am referring to that needs to be relaxed is: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/pluginmanager/pluginwatcher/plugin_watcher.go#L193-L196

I think the rest of the code around the above does get invoked regardless of file type - so I don't think the invocation of handleCreateEvent itself is being filtered based on file type but the downstream calls like handlePluginRegistration are filtered.

We can explore sending down a FSCTL to the file to check for the Windows reparse points (through go wrappers) to align with fi.Mode()&os.ModeSocket == 0. I can call that plan out here.

liggitt · 2019-07-30T20:27:26Z

keps/sig-windows/20190714-windows-csi-support.md

+    CONTAINER = 1;
+}
+
+message MkdirRequest {


what owner/permissions are associated with the new directory? are all parent directories created if missing as well?

I went ahead and added a Windows OS error code for all responses as that is critical. Also clarified paths will not be pre-created for parents and a PathExists RPC. Also mentioned the typical user SID under which csi-proxy.exe will be started and the permissions (read, write) that will be set when creating directories.

liggitt · 2019-07-30T20:30:13Z

keps/sig-windows/20190714-windows-csi-support.md

+    // Restrictions:
+    // Needs to be an absolute path under kubernetes-csi-plugins-path
+    // or container-runtime-storage-path based on context
+    // Cannot exist already in host


if it does exist already, is an error returned? is the error distinguishable as an "already exists" error?

Yes. I went ahead and added a Windows OS error code for all responses to make error detection and reaction to them easier.

keps/sig-windows/20190714-windows-csi-support.md

liggitt · 2019-07-30T20:48:22Z

keps/sig-windows/20190714-windows-csi-support.md


 A GRPC based interface, CSIProxyService, will be used by CSI Node Plugins to invoke privileged operations on the host through csi-proxy.exe. CSIProxyService will be versioned and any release of csi-proxy.exe binary will strive to maintain backward compatibility across as many prior stable versions of the API as possible. This avoids having to run multiple versions of the csi-proxy.exe binary on the same Windows host if multiple CSI Node Plugins (that do not depend on APIs in Alpha or Beta versions) have been configured and the version of the csi-proxy API required by the plugins are different. For every version of the API supported, csi-proxy.exe will first probe for and then expose a `\\.\pipe\csi-proxy-v[N]` pipe where v[N] can be v1, v2alpha1, v3beta1, etc. If during the initial probe phase, csi-proxy.exe determines that another process is already listening on a `\\.\pipe\csi-proxy-v[N]` named pipe, it will not try to create and listen on that named pipe. This allows multiple versions of csi-proxy.exe to run side-by-side if absolutely necessary to support multiple CSI Node Plugins that require widely different versions of CSIProxyService that no single version of csi-proxy.exe can support.

-If an old stable version of CSIProxyService, say vN, can no longer be supported, support for it may be dropped from the latest version of csi-proxy.exe. To continue running CSI Node Plugins that depend on the old version vN, Kubernetes administrators will be required to run the latest version of the csi-proxy.exe (that will be used by CSI Node Plugins that use versions of CSIProxyService more recent than vN) along with an old version of csi-proxy.exe that does support vN.
-
 The following are the main RPC calls that will comprise a v1 version of the CSIProxyService API:

 ```


Thanks. Do these map closely to windows filesystem APIs? If so, that would be good to reference.

I'll defer to the windows/storage experts to determine if these provide a reasonable API that covers the information/operations a CSI driver would need.

keps/sig-windows/20190714-windows-csi-support.md

liggitt · 2019-07-30T20:51:53Z

keps/sig-windows/20190714-windows-csi-support.md

-If an old stable version of CSIProxyService, say vN, can no longer be supported, support for it may be dropped from the latest version of csi-proxy.exe. To continue running CSI Node Plugins that depend on the old version vN, Kubernetes administrators will be required to run the latest version of the csi-proxy.exe (that will be used by CSI Node Plugins that use versions of CSIProxyService more recent than vN) along with an old version of csi-proxy.exe that does support vN.
+##### CSI Proxy Configuration
+There will be two command line parameters that may be passed to csi-proxy.exe:
+1. kubernetes-csi-plugins-path: String parameter pointing to the path used by Kubernetes CSI plugins. Will default to: `C:\var\lib\kubelet\plugins\kubernetes.io\csi`. All requests for creation and deletion of paths through the CSIProxyService RPCs (detailed below) will need to be under this path.


will need to be under this path

Including resolution of any links in the parent dir?

For example, in the following setup:

kubernetes-csi-plugins-path = C:\var\lib\kubelet\plugins\kubernetes.io\csi

C:\var\lib\kubelet\plugins\kubernetes.io\csi\foo\bar is a link that points to Z:\baz

Would a request to delete C:\var\lib\kubelet\plugins\kubernetes.io\csi\foo\bar\quux be allowed (since it really resolves to Z:\baz\quux?

@msau42 is more familiar with what the CSI drivers are required to do, and how much visibility (if any) they have to the mountPath/subPath inputs the user provides. The less direct input from untrusted pvc/pod creators the drivers make use of, the less concerned I am about these aspects.

/var/lib/kubelet/plugins/kubernetes.io/csi is where the staging mounts go, but I think we also need /var/lib/kubelet/pods/ for the per pod link right?

Regarding @liggitt's concern, I think we should disallow symlinks for the paths that get passed to the proxy.

Kubelet is supposed to give the driver only system-controlled paths, but that doesn't stop the driver from giving another path to the proxy.

CSI drivers don't handle subpaths. It's handled directly by the kubelet, and IIUC, this proxy is only for CSI driver file/mount operations, not kubelet.

I'm not familiar with windows symlink mechanics. Does it have the same issue as linux where O_NOFOLLOW only applies to the past element of the path?

@msau42 @liggitt I specifically mentioned for RmdirRequest: Path cannot be a file of type symlink as a restriction https://github.com/kubernetes/enhancements/pull/1184/files#diff-0533ba89d026317907f55974130ce267R419 . The file type of the path will be checked and request to delete a path of type symlink will be rejected. So that should take care of rmdir being invoked on a symlink path to remove a target somewhere else.

For MkdirRequest, the proxy creates the path and will make sure they are regular directories and not symlinks.

liggitt · 2019-07-30T20:52:54Z

keps/sig-windows/20190714-windows-csi-support.md

+2. container-runtime-storage-path: String parameter pointing to the path used by the container runtime to store images, snapshots and other data related to containers. Possible values could be: `F:\ProgramData\docker`, `C:\ProgramData\containerd` or corresponding paths used by a Windows runtime. Parameters to `LinkVolume` (detailed below) will need to be under this path.
+
+##### CSI Proxy GRPC API
+The following are the main RPC calls that will comprise a v1 version of the CSIProxyService API. A preliminary structure for Request and Response associated with each RPC is described below. Note that the specific structures as well as restrictions on them are expected to evolve during Alpha phase and are expected to be in a final form at the end of Beta.


The following are the main RPC calls that will comprise a v1 version of the CSIProxyService API

isn't what is described below the v1alpha1 version of the CSIProxyService API?

Correct. I will update this.

saad-ali · 2019-07-30T21:06:32Z

/assign

Signed-off-by: Deep Debroy <ddebroy@docker.com>

verult · 2019-07-30T22:40:44Z

keps/sig-windows/20190714-windows-csi-support.md

+      - [CSI Proxy Configuration](#csi-proxy-configuration)
+      - [CSI Proxy GRPC API](#csi-proxy-grpc-api)
+      - [CSI Proxy GRPC API Graduation and Deprecation Policy](#csi-proxy-grpc-api-graduation-and-deprecation-policy)
+      - [CSI Proxy Event Logs](#csi-proxy-event-logs)
    - [Enhancements in Kubernetes/Utils/mounter](#enhancements-in-kubernetesutilsmounter)
    - [Enhancements in CSI Node Plugins](#enhancements-in-csi-node-plugins)


nit: Could you define CSI Node Plugins? Maybe in a glossary section? I wasn't sure if this is referring to the CSI driver to be installed on nodes or the DaemonSet or something else without reading deeper

PatrickLang · 2019-07-30T23:08:38Z

/lgtm
looking for approval from a sig-storage rep

k8s-ci-robot · 2019-07-30T23:08:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, PatrickLang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~keps/sig-windows/OWNERS~~ [PatrickLang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ddebroy · 2019-07-30T23:14:33Z

Well that was strange - tide feels the PR has not been approved but the k8s-ci-robot added approved tag and merged the PR?

msau42 · 2019-07-30T23:20:13Z

keps/sig-windows/20190714-windows-csi-support.md


-The following are the main RPC calls that will comprise a v1 version of the CSIProxyService API:
+2. container-runtime-storage-path: String parameter pointing to the path used by the container runtime to store images, snapshots and other data related to containers. Possible values could be: `F:\ProgramData\docker`, `C:\ProgramData\containerd` or corresponding paths used by a Windows runtime. Parameters to `LinkVolume` (detailed below) will need to be under this path.


Why do CSI drivers need the container runtime paths?

This is for the LinkVolume step where the proxy needs to invoke mklink from the host staging path to a path inside the container's fs as part of node publish.

Could you explain a little bit more? I assume CSI plugin only needs to know the /var/lib/kubelet paths, and the container runtime will be the one to handle paths for the container rootfs

I ran through the flow again and what I stated above is incorrect. The node publish path on the container side is indeed under C:\var\lib\kubelet\pods. I will correct this in a fresh PR.

k8s-ci-robot requested review from michmike and PatrickLang July 30, 2019 01:26

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 30, 2019

k8s-ci-robot assigned liggitt, msau42 and PatrickLang Jul 30, 2019

ddebroy mentioned this pull request Jul 30, 2019

Support CSI plugins in Windows #1122

Closed

msau42 reviewed Jul 30, 2019

View reviewed changes

k8s-ci-robot added the sig/pm label Jul 30, 2019

k8s-ci-robot assigned calebamiles Jul 30, 2019

liggitt reviewed Jul 30, 2019

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 30, 2019

liggitt reviewed Jul 30, 2019

View reviewed changes

saad-ali changed the title ~~Update KEP status to implementable and address prior feedback~~ Update "Windows Storage Support" KEP status to implementable and address prior feedback Jul 30, 2019

k8s-ci-robot assigned saad-ali Jul 30, 2019

Update KEP status to implementable and address prior feedback

5f9bc2c

Signed-off-by: Deep Debroy <ddebroy@docker.com>

verult reviewed Jul 30, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2019

k8s-ci-robot merged commit 4eee22d into kubernetes:master Jul 30, 2019

msau42 reviewed Jul 30, 2019

View reviewed changes

ddebroy mentioned this pull request Oct 29, 2019

FileSystem APIs kubernetes-csi/csi-proxy#11

Merged

justaugustus added area/enhancements Issues or PRs related to the Enhancements subproject and removed sig/pm area/enhancements Issues or PRs related to the Enhancements subproject labels Apr 19, 2020


		![Kubernetes CSI Node Components and Interactions](https://ddebroywin1.s3-us-west-2.amazonaws.com/csi-proxy2.png "Kubernetes CSI Node Components and Interactions")
		![Kubernetes CSI Node Components and Interactions](https://ddebroywin1.s3-us-west-2.amazonaws.com/csi-proxy3.png "Kubernetes CSI Node Components and Interactions")


		The following are the main RPC calls that will comprise a v1 version of the CSIProxyService API:
		2. container-runtime-storage-path: String parameter pointing to the path used by the container runtime to store images, snapshots and other data related to containers. Possible values could be: `F:\ProgramData\docker`, `C:\ProgramData\containerd` or corresponding paths used by a Windows runtime. Parameters to `LinkVolume` (detailed below) will need to be under this path.

Update "Windows Storage Support" KEP status to implementable and address prior feedback #1184

Update "Windows Storage Support" KEP status to implementable and address prior feedback #1184

Conversation

ddebroy commented Jul 30, 2019 • edited Loading

ddebroy commented Jul 30, 2019

ddebroy commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy commented Jul 30, 2019

liggitt Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jul 30, 2019

Choose a reason for hiding this comment

ddebroy Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Jul 30, 2019

Choose a reason for hiding this comment

PatrickLang commented Jul 30, 2019

k8s-ci-robot commented Jul 30, 2019

ddebroy commented Jul 30, 2019

Choose a reason for hiding this comment

ddebroy Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy commented Jul 30, 2019 •

edited

Loading

liggitt Jul 30, 2019 •

edited

Loading

ddebroy Jul 30, 2019 •

edited

Loading

ddebroy Jul 30, 2019 •

edited

Loading

ddebroy Jul 30, 2019 •

edited

Loading

ddebroy Jul 30, 2019 •

edited

Loading