Add proposal for mount options #321
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
|
||
| To match options that are allowed - we will split the mount options in PV by ',' and then verify if key does not exist in blacklist, which will prevent mounting volumes with these options. | ||
|
|
||
| One upside of using these options in kubelet is - it will also work for external provisioners. One major downside is - we will be allowing creation of PV or SC which can be blocked later by a blacklisted option. i.e - A PV or SC with invalid mount option will not be rejected immediately. |
There was a problem hiding this comment.
letting potentially dangerous data in the API, then ignoring/failing on it at the kubelet level is a change from the direction of PodSecurityPolicy, etc.
enforcing it this way would not let you set advanced options in PVs (which require higher permissions to create) but forbid them in pod specs (which are accessible to normal namespace-constrained users)
it also would not let you allow privileged users (e.g. cluster admins setting up mounts in kube-system, etc) to make use of mount options, while limiting other users' access
There was a problem hiding this comment.
Agreed on all accounts but so far use cases we have seen - there hasn't been a case where we have a need of some PVs from same plugin configurable with wider(?) mount options and some with narrower.
The other options we have considered has similar problems. But this is still very much WIP - so we are considering all options.
There was a problem hiding this comment.
Added more options for avoiding a blacklist and still having saner set of mount options.
|
cc @thockin |
| Mount time options that are operationally important and have no security implications should be suppported. Examples are NFS's TCP mode, versions, lock mode, caching mode; Glusterfs's caching mode; SMB's version, locking, id mapping; and more. | ||
|
|
||
| ## Design | ||
|
|
There was a problem hiding this comment.
cc @kubernetes/api-reviewers @kubernetes/sig-storage-api-reviews @kubernetes/sig-auth-api-reviews
| To prevent users from using options which are not safe - Kubernetes admin can configure volume options and specify a blacklist of mount point parameters. For example: | ||
|
|
||
| ``` | ||
| ~> kubelet --volume-options <volume-config-file> |
There was a problem hiding this comment.
this requires deploying a config file to all nodes, which is hard to maintain.
There was a problem hiding this comment.
+1 please don't add more config files.
There was a problem hiding this comment.
+1 let's not make the white/black list cluster admin configurable. Just hard code it in the plugins for now. The addition of the mountOptions field to the volume specs for some plugins should be the only API change.
My thought process for this is that we should minimize changes to the API while making sure we don't paint ourselves in to a corner. And by implementing a minimally viable solution (hardcoded white/black list), we reduce complexity while leaving open the option of adding in cluster admin configurable list in the future if the need arises.
There was a problem hiding this comment.
didn't @derekwaynecarr add some crazy config resource type? Not that I think this is definitely the way to go...
| nfs: | ||
| path: /tmp | ||
| server: 172.17.0.2 | ||
| mountOption: "hard,nolock,nfsvers=3" |
There was a problem hiding this comment.
Can you elaborate how alpha/beta phase would look like? Annotations? Or are we aiming directly at stable API?
There was a problem hiding this comment.
Also, I forget how these things look internally in the kernel, etc. But, would it make sense to make this a list instead of an opaque CSV string? I see there is a concept around fields but I agree making a field for every option is going to be untenable to keep up with.
There was a problem hiding this comment.
+1 list seems like it would be more robust then a CSV string? Thoughts @gnufied?
There was a problem hiding this comment.
I think - it is more natural to have this as a string since that is something admins are used to anyways. Using a string doesn't prevent us from validating the options (split by , and treat them as a list internally).
There was a problem hiding this comment.
In general if we (in our validation) are going to deal with each of these individually, we should deal with them as a list. I'm pretty cautious anytime we accept a string that we parse where we don't own the underlying syntax. So if we want to parse and validate, list is preferable. Especially if we have other controls (like pod security policy?) that need to say which mount options are allowed.
There was a problem hiding this comment.
hmm, okay - It is purely aesthetic thing and I could be wrong, so if we use a list, mount options will look like:
mountOption: ["hard", "nolock", "nfsvers=3"]
Or in case when there is only one option, it will look like:
mountOption: ["volume-name=foobar"]
So even if we use a list here, we still have to do some string processing. such as split by = for key/values.
| ``` | ||
|
|
||
| This still has the problem that - each time a new option has to be supported user will have to | ||
| wait for new release. |
There was a problem hiding this comment.
I think each time a new option appears it has to be reviewed anyway (and blacklisted by default). In fact, whitelisting the options would look like a safer approach to me.
There was a problem hiding this comment.
+1 to whitelisting and making the whitelist a node property.
There was a problem hiding this comment.
should it be whitelisted by the node or by the API server? if it's at the node level, that implies you can have non-homogenous nodes. should the scheduler take into account which nodes allow which options?
There was a problem hiding this comment.
+1 to whitelisting. I propose whitelist would be implemented per volume plugin (set of options allowed will differ between NFS and GCE PD, for example). The whitelist will be checked at mount time, when the mount call happens, unsupported mount options will be dropped (with an warning even generated indicating that the specified options will be ignored since they are not in the whitelist).
|
cc @lpabon |
| nfs: | ||
| path: /tmp | ||
| server: 172.17.0.2 | ||
| mountOption: "hard,nolock,nfsvers=3" |
There was a problem hiding this comment.
Also, I forget how these things look internally in the kernel, etc. But, would it make sense to make this a list instead of an opaque CSV string? I see there is a concept around fields but I agree making a field for every option is going to be untenable to keep up with.
| To prevent users from using options which are not safe - Kubernetes admin can configure volume options and specify a blacklist of mount point parameters. For example: | ||
|
|
||
| ``` | ||
| ~> kubelet --volume-options <volume-config-file> |
There was a problem hiding this comment.
+1 please don't add more config files.
| "mount_option_blacklist": ["transport"] | ||
| }, | ||
| "nfs": { | ||
| "mount_option_blacklist": ["hard", "noacl"] |
There was a problem hiding this comment.
Hrm, it is a list here. Why not above?
There was a problem hiding this comment.
are keys enough? I don't know enough about mount options to know if you'd need to whitelist specific values
There was a problem hiding this comment.
It would be tough order to validate the values, they keys themselves are hard enough IMO. cc @rootfs
There was a problem hiding this comment.
Blacklists are bad. We should whitelist if we want to be secure.
EDIT: saw the conversation at the end.
| ``` | ||
|
|
||
| This still has the problem that - each time a new option has to be supported user will have to | ||
| wait for new release. |
There was a problem hiding this comment.
+1 to whitelisting and making the whitelist a node property.
|
I dont think we want the 'blacklist options' support section. The user will never see mount options, only admin |
| nfs: | ||
| path: /tmp | ||
| server: 172.17.0.2 | ||
| mountOption: "hard,nolock,nfsvers=3" |
There was a problem hiding this comment.
+1 list seems like it would be more robust then a CSV string? Thoughts @gnufied?
| ``` | ||
|
|
||
| This still has the problem that - each time a new option has to be supported user will have to | ||
| wait for new release. |
There was a problem hiding this comment.
+1 to whitelisting. I propose whitelist would be implemented per volume plugin (set of options allowed will differ between NFS and GCE PD, for example). The whitelist will be checked at mount time, when the mount call happens, unsupported mount options will be dropped (with an warning even generated indicating that the specified options will be ignored since they are not in the whitelist).
| To prevent users from using options which are not safe - Kubernetes admin can configure volume options and specify a blacklist of mount point parameters. For example: | ||
|
|
||
| ``` | ||
| ~> kubelet --volume-options <volume-config-file> |
There was a problem hiding this comment.
+1 let's not make the white/black list cluster admin configurable. Just hard code it in the plugins for now. The addition of the mountOptions field to the volume specs for some plugins should be the only API change.
My thought process for this is that we should minimize changes to the API while making sure we don't paint ourselves in to a corner. And by implementing a minimally viable solution (hardcoded white/black list), we reduce complexity while leaving open the option of adding in cluster admin configurable list in the future if the need arises.
@saad-ali Can you help me understand why a whitelist is needed? PVs are 'admin owned' objects, and we've treated Volumes as 'admin owned' in the past also. What are the use cases where untrusted users use Volumes that we'd want to limit access? |
The proposed API exposes the options in pod spec Volume definitions as well |
Where's the security boundary here? If i'm a user with a Volume specified in a POD.. can't I make that volume hostPath pointed at /etc and do all sorts of things? I thought the assumption is only Admin/System users are allowed to use Volumes directly. |
PodSecurityPolicy allows gating which types of volumes are allowed in pod specs. It does not have controls in place for details of particular volume types (particular host paths, mount options, etc) |
Ok that makes sense. So enhance PSP to include white list of Volume properties seems like a good spot for this function instead of CLI. |
There was a problem hiding this comment.
What is the security model here? Why are we going to this trouble? Why are we blocking a storage expert from setting the things they need?
Is there is actually a security issue here I think it better fixed by differentiating users and storage admins. If the person who bought the gigantic expensive storage array isn't allowed to use it the way they need to I think we have failed.
My opinion would be to just reject any pod definition with a mountOption by default. Allow any mountOption on any PV. If we later need to add a flag to allow pod definitions with mountOptions so be it.
But to think that we could possibly understand all of the potential mount options, all of the implications of those, and keep up with the every changing state of the world is a bit bold. My laptop, with no fancy enterprise storage of any kind has 50 unique mount options right now.
I'm convinced of the value of mountOptions. I'm not convinced of the value of the proposed restrictions.
|
|
||
| ## Goal | ||
|
|
||
| Enable Kubernetes users to specify mount options with mountable volumes |
There was a problem hiding this comment.
To me, this should not be the goal at all. We should be allowing kubernetes storage admins to specify mount options. I actually object to users having anything to do with it.
There was a problem hiding this comment.
yeah, I will change the wording there.
|
|
||
| We currently support network filesystems: NFS, Glusterfs, Ceph FS, SMB (Azure file), Quobytes, and local filesystems such as ext[3|4] and XFS. | ||
|
|
||
| Mount time options that are operationally important and have no security implications should be suppported. Examples are NFS's TCP mode, versions, lock mode, caching mode; Glusterfs's caching mode; SMB's version, locking, id mapping; and more. |
There was a problem hiding this comment.
What is the security model we are working with when determining if something has security implications ? Who are we trying to protect from what?
There was a problem hiding this comment.
I think the definition of 'security implications' is going to be context/site dependant. This makes implementing this in a way that is safe for everybody difficult. Certainly I'd like to see the threat models being considered.
| To prevent users from using options which are not safe - Kubernetes admin can configure volume options and specify a blacklist of mount point parameters. For example: | ||
|
|
||
| ``` | ||
| ~> kubelet --volume-options <volume-config-file> |
There was a problem hiding this comment.
didn't @derekwaynecarr add some crazy config resource type? Not that I think this is definitely the way to go...
|
|
||
| One upside of using these options in kubelet is - it will also work for external provisioners. One major downside is - we will be allowing creation of PV or SC which can be blocked later by a blacklisted option. i.e - A PV or SC with invalid mount option will not be rejected immediately. | ||
|
|
||
| 2. As suggested by @saad change mount options to key/value pairs such as: |
There was a problem hiding this comment.
What about things like noatime or nosuid that are not key/value pairs?
There was a problem hiding this comment.
We are not going to do key/value pair @eparis . That is something that was briefly considered and rejected. As I update the design, I will delete it.
|
I'm also not sure how any of these take into account mount options that aren't static. What if i need to mount with |
Starting with only allowing the options in PVs (blocking in pod specs for now via validation or something) and not bothering with restricting what can be specified in a PV makes sense to me. |
Is an enhancement along those lines tracked somewhere that you know of? We're interested in being able to allow a user to bind mount certain host paths but reject everything else. Wikimedia has an admission controller but it is a bit of a blunt instrument. Integrating this with PSPs would be great. |
Let's prevent it in the API in the first place, we don't want latent data persisted in the API that could spring to life in later releases. |
The security concern is exposing powerful options directly to users via the pod spec. Like you said, we could simply not allow
|
|
For discussion sake, can we explain a mount option that we consider to be 'unsafe' for a user to set, even in a pod spec? |
It could be done via validation ( |
|
@gnufied I like the mountOptions being set at the StorageClass (and possibly PV) because these settings are done by the administrator, but I am concerned with the settings being allowed in the Pod Spec. Is there a use case where the user would want to set their own mount options and override what the administrator had setup? |
|
@lpabon if we are talking about the pod spec we are not talking about 'overriding' anything an administrator set up. The pod spec has the option to inline anything which could be in a PV. No PV needed. For example entirely inside the pod spec you can define an NFS mount volume setting the export and the server and everything. While I completely understand why we control the use of inline volume spec I'm still fuzzy on why we are trying to put controls on the mount options altogether. Without understanding the risks of mount options its hard to see how to properly handle the PV vs inline Pod definition. If I let a user set the nfs server and export arbitrarily, what is the extra risk allowing them to set mountOptions? How are mountOptions dangerous. Can we find some specifics? |
|
if it's decided mount options can be dangerous & that admin control over pod spec mount options is needed, I agree it should be done via PSPs. Not another new security whitelist mechanism that admins will have to figure out. As already suggested, could start by disabilng pod spec mount options altogether via validation then implement PSP whitelists later if the need arises. Even if it's just temporary, a hardcoded blacklist/whitelist would block not only pod-spec mount options but also admin-created PVs which I think would be too annoying for admins who know what they're doing |
|
@matchstick I know you have a strong linux storage background. Can you come up with an example of a mount option that we could consider 'dangerous'? I'm having difficulty find the $problem that we are solving with all this white/black list stuff. I want something concrete to discuss. |
|
cc @pwittrock @ymqytw for adding another field to a union type (name + mountOptions + one-of(volume sources)) |
|
Do we really want to make a (possibly remote) plugin call during
validation? That worries me.
…On Thu, Feb 16, 2017 at 1:24 PM, Hemant Kumar ***@***.***> wrote:
@eparis <https://github.com/eparis> @saad-ali
<https://github.com/saad-ali> I have updated proposal with final(?)
conclusions so far:
1. Use PSP for enabling/disabling mount options in inline volume specs.
2. Allow plugins to define - if they want to support mount options.
3. Change mountOption from a string to list of strings.
PTAL
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#321 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVNJCF0BkKys4CHNCemtCmzfqkGtWks5rdL6GgaJpZM4L0Ywp>
.
|
|
@thockin Initialization of What other option we have? The alternative is - we can simply ignore the mount options on volume plugins that don't support it. May be some other option there is, that I am missing.... |
|
I have talked to @eparis @saad-ali and @childsb and we all agreed that after moving This removes the need for PSPs etc and I have updated the design proposal to reflect that. |
| restuser: "admin" | ||
| secretNamespace: "default" | ||
| secretName: "heketi-secret" | ||
| mountOption: ["volume-name=foobar"] |
There was a problem hiding this comment.
parameters is map[string]string, not map[string][]string
There was a problem hiding this comment.
this is the first thing that takes a storage class parameter and uses it to fill in a field in the PV, right? that's different than all other parameters, which are arbitrary and volume-plugin-defined. If we want fields in the storageclass that map to fields in the PV, that seems like it should be in a different field than parameters
There was a problem hiding this comment.
Good catch about map[string]string vs map[string][]string. I am thinking this will become a non-issue if we move this field to its own place.
There are other PRs such as - kubernetes/kubernetes#40805 which do something similar.
I am as well somewhat hesitant to piggyback on parameter field since it hinders discoverability of API (just a bag of key/value pairs). cc @childsb
There was a problem hiding this comment.
Agree - this feels like an abuse of parameters, which are promised to be opaque.
There was a problem hiding this comment.
The mountOptions in SCs are going to be phase2 of this implementation until we decide where it should appear. I am going to update the design to reflect this decision.
| accessModes: | ||
| - ReadWriteOnce | ||
| persistentVolumeReclaimPolicy: Recycle | ||
| mountOption: ["hard","nolock","nfsvers=3"] |
There was a problem hiding this comment.
Shouldn't it be alpha annotation first?
There was a problem hiding this comment.
Talking with @gnufied about this offline yesterday, i dont think the use cases for mountOptions in storageclass is large enough to warrant adding the function or a field now and should nix it from the proposal. It would be easier to solve mount options in storage class when someone needs it and has a good reason.
|
So what we've agreed on:
Will LGTM and merge once @gnufied gets a chance to update the proposal to reflect that. Implementation in progress here: kubernetes/kubernetes#41906 |
gnufied
force-pushed
the
mount-option-design-prop
branch
from
7dc85a4
to
2edc639
Compare
|
@saad-ali updated the PR to reflect all of latest changes. I have also dropped Storageclasses for now. We can amend the design when we need it. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
Merging this, if there are any concerns we can amend the design. |
Add proposal for mount options
…notes Fix incorrect release notes for kubernetes/kubernetes#44251
Add proposal for mount options
Adding initial proposal for mount options.
CC @saad-ali @matchstick @rootfs