Skip to content

Adds user/group values for optional auth control #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Adds user/group values for optional auth control #30

wants to merge 1 commit into from

Conversation

casusbelli
Copy link

@casusbelli casusbelli commented May 30, 2017

This change adds optional user and group information in
order to enable applying basic authorization control during
creation and publishing of volumes, e.g. for shared file
system backends.
Further details in issue #17 .

@casusbelli casusbelli changed the title [WIP]Adds user/group values for optional auth control Adds user/group values for optional auth control Jun 13, 2017
Copy link
Member

@jdef jdef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the per-line comments, I'm wondering how Principal fits in w/ respect to pre-created volumes:

  1. Should there be a way to discover the Principal with which a volume was originally created (via List)?
  2. What about validating intended usage of a precreated volume via ValidateVolumeCapabilities?

spec.md Outdated
// the creation of this volume. If the principal is present the
// plugin MAY ensure the volume created will be accessible for the
// herein specified entities only.
Principal volume_root_owner = 6;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that this field volume_root_owner may be different than the container_acts_as field. How is a CO supposed to reason about the relationship between these fields? Principal is not a trivial structure and it's unclear to me how a CO is supposed to understand, for example, relationship between namespaces, users, and groups (which, on some systems may exist within some hierarchical structure).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field may not be different, volume_root_owner and container_acts_as have to be the same.
Having said that, i'll remove this part as it does cover a point slightly beyond the core aspect to be fixed here which is to ensure authorization information for shared filesystem backends is provided. I'll add this in a different ticket/PR.

// This field is OPTIONAL. If specified it contains the CO namespace
// identity in which the the following identity information (user /
// group) exists.
string namespace = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If namespace isn't supported, but user and group are, how might a CO discover that?
Likewise for the other fields.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The information herein is based on the COs domain so the CO provides principal information from his own ownership model in here. The principals structure is strongly based on the kubernetes model.
This means there is no discovery required for the CO. The plugin has to be able to interpret the principal input if it wants to enforce authorization. As providing the principal is optional the OS can provide no principal information (no authorization) or the plugin can also ignore the principal (as it's support is optional, too, resulting also in no authorization), e.g. if it is not able to handle the COs principal input.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort of access mgmt could be done with a third party IAM/RBAC solution. The users and groups and roles being defined there. Is that also an assumption here? The SP could be using these to manage access to its storage already. The CO need not be the one managing users/groups/roles.

I think the spec also needs to call out and define what is user/group/role and the fact that both CO and the SP can be defining these. In which case how is that proposed to be handled.

Please also define the behavior for "no authorization" - does it mean all access or no access.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@govint Let me state the requirements so that you see where this is coming from:

  • For doing access control, filesystems need posix user and group (or SID on Windows), abstracted as "Principal" here.
  • The design posits that a container acts as one specific Principal when accessing storage. This information is a part of the container deployment. Hence, the CO must provide this information.
  • This implies that there is container -> volume relationship to be established by the SP, not only a machine -> volume as the spec currently assumes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design posits that a container acts as one specific Principal when accessing storage

I think this assumption is pretty limiting. I'd like to see something a bit more flexible.

Copy link
Contributor

@julian-hj julian-hj Jul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the very late response, but I am a bit unclear as to how this change addresses the case that @govint raised. Say I am a plugin developer, and my storage solution has some proprietary IdP or authentication scheme that is not known to the CO. The end user has to issue some sort of command to the CO that associates the storage instance to the container, and presumably provides credentials for the storage in some kind of data blob. The CO stores that data blob somewhere and then calls controller publish & node publish. So far so good.

What I don't get is, why would the Principal we pass to the plugin be a CO user and group? What would a plugin author do with that information in a plugin implementation that isn't CO specific? Why not instead just leave the credentials as a set of key/value pairs that are opaque to the CO and interpreted by the plugin? That way if the plugin requires a ceph keyring or kerberos keytab file or what have you, the CO can just pass it through.

Or is the idea that proprietary/plugin-specific credentials blocks can be nested into PublishVolumeInfo and we just want the Principal for plugins that use the CO as an IdP?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this assumption is pretty limiting. I'd like to see something a bit more flexible.

@jdef which dimensions are you thinking of in terms of flexibility? More than one use per container?

spec.md Outdated
// the publishing of this volume. If the principal is present the
// plugin MAY ensure the volume published will be accessible for
// the herein specified entities only.
Principal container_acts_as = 6;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the obligation of the CO w/ respect to passing the same container_acts_as value to both ControllerVolumePublish and NodeVolumePublish RPCs? For example, must they be identical, or is it possible that the principal of the latter call is a subset of the principal of the former call? How would a CO understand the limitations here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

container_acts_as is a confusing name given the "perspective" from which the other fields were named (a volume's, not a container's). Please pick a name from the perspective of the volume not that of the container, for example, accessible_by or some such

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The container_acts_as values must be identical.
Personally speaking our plugin will use only the NodePublishVolume principal field but others may require preparatory steps and need this information in the ControllerPublishVolume call, too.

I'll rename the field to accessed_as , as the principal describes the acting id for a given binding. One volume can be published in different containers with different principals at the same time, resulting in different containers acting as different principals on the files in that volume.
This also relates to issue #40 .

@casusbelli
Copy link
Author

casusbelli commented Jun 16, 2017

@jdef Thanks for the input! Please find the answers in your comments, i'll update the PR according to the comments shortly.

Update:
Regarding your questions for the pre-existing volumes. The principals data is used for the binding of the volume to the container. If set and used the principal ensures access to the given volume by the given container is done using the given principals data. So e.g.:

Container A accesses volume X as principal M
Container B accesses volume X as principal K
Container C accesses volume Y as principal M
...

This does not require a discovery of principal information from this volume and i think it does not require a capabilities detection.

Copy link
Member

@jdef jdef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the simplification. I've thought about this a bit more and left some additional commentary. Interested in your thoughts.

spec.md Outdated
// group) exists.
string namespace = 1;
// This field is OPTIONAL. If specified it contains the CO user
// identity holding ownership of a volume, similar to the user_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A string user implies that this is a textual label, like "james". But when I see user_id in the commentary for this field, I think of a numerical ID like 600.

Is it the intent that this field should only carry textual labels, or numerical IDs, or both (and the plugin just has to deal with whatever the CO spits out)?

We also clearly call out in the spec that we're not aiming for POSIX compliance. Is there a need to mention POSIX at all in these comments?

(ditto for group and its comments)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields use the type string in order to allow both, text or numerical ids.
The POSIX references were added to provide an example how this could be used for clarification. Should I remove the references?

spec.md Outdated
// identities holding ownership of a volume, similar to the group_id
// in a POSIX file system.
repeated string group = 3;
}
Copy link
Member

@jdef jdef Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is trying to establish some ACLs. The "single user, multi group" feels a bit too constraining. Many systems support more flexible models than this. Perhaps we should model Principal a bit more along the lines of:

message Principal {
  message User {
    string name
  }
  message Group {
    string name
  }
  message Role {
    string name
  }
  message Everyone {}

  string namespace

  oneof value {
    User user
    Group group
    Role role
    Everyone everyone
  }
}

...and then make it a repeated field in the publish calls?

It's also weird because the publish calls assume that all parties will be granted the same "access mode".

Perhaps we should remodel the readonly/accessed_as fields as a list of tuples?

message PrincipalAccess {
  // both fields are REQUIRED
  bool readonly
  Principal principal
}
message ControllerPublishVolumeRequest {
  ...
  // replace readonly and accessed_as with the following:
  // REQUIRED. If not interested in user/group/role or namespace, then specify
  // a single PrincipalAccess { readonly = ..., principal = { everyone = {} } }
  repeated PrincipalAccess principal_access
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACL might be a bit over the top. As the user and groups settings inside a container are completely arbitrary we need to pin the identity with which the given containers operations access the volume and it's content. All parties from that container use the same identity and access mode. Other Containers can access the same volume with different settings. That's why we also need the publishing on a per container and not per volume based (see #40 ).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"All parties from that container use the same identity and access mode..."

What if you have a container hierarchy, with the volume mounted at the "root container" (and shared with lower levels) in the hierarchy, but sub-containers that run as different users? Also the RPCs here (as you've indirectly implied) don't apply per-container, they're aimed a bit "higher" (e.g. node-level).

Agree that there's overlap here with #40. Need to chew on this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, i'm not familiar with container hierarchies. Do you have a link (example, desc, etc.) for me to elaborate on this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdef : Thanks for the hierarchy pointers, that helped!

In essence the question if containers in a hierarchy use the same principal or not is up to the CO as the CO provides the principal information for publishing a given mount. If the CO enforces using the same principal for different mounts in a set of containers, then all containers in that set will use the same identity to access the volume. If the CO allows using different principals, e.g. based on a pod configuration, these individual identities will be applied for the different configured mounts (in this scenario the sub containers have their own individual mounts of the external volumes provided via the SP plugin).
If a root container provides access to his own external volume mount for his child containers by different means the scenario goes out of scope for the SP plugin.

Copy link

@oritnm oritnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An additional info regarding secrets (which can be tokens/digest password/passwords) are required as well.
There will be SP may needs (or better say should) be able to authenticate the principal and not just enforce authorization

@jdef
Copy link
Member

jdef commented Jun 19, 2017

@oritnm yes, credentials are important. current thinking is that they could be specified either: (a) OOB as part of plugin configuration, or else; (b) as "mount parameters" via VolumeCapability.

If that seems insufficient, then we should probably track that in a separate PR/issue

@oritnm
Copy link

oritnm commented Jun 19, 2017

@jdef I think secrets should be part of the principal definition, it could be an optional param although IMHO it is best to provide the it thru this mechanism.
Having the secrets/credentials thru the mount options (volume capabilities) is insufficient since every prinicpal will have different credential that should be used to authenticate it in front of the SP.

@casusbelli
Copy link
Author

casusbelli commented Jun 20, 2017

Yes, I think this should be part of the Principal, too.
I'll update accordingly unless different arguments come up.

Update: I added this in a new PR in order reduce complexity in this change.

@jdef
Copy link
Member

jdef commented Jun 26, 2017 via email

@casusbelli
Copy link
Author

@jdef : I replied in the inline comment.

From my point of view I'd do one more update on the change adding clarifications in the comments regarding:

  • no authorization meaning access for all
  • removing/reducing the POSIX example in attribute clarifications
  • adding a note regarding possible container hierarchies

This change adds an optional principal in
order to enable applying basic authorization control,
e.g. for shared filesystem backends. This is
done by mapping all volume access through a volume
volume publication to the principals ids.
Further details in issue #17 .
@casusbelli
Copy link
Author

I added the clarifying comments and squashed the commits.

// access to his volume mount throught some CO specific internal
// mechanism. In the latter case still all access through that volume
// mount is mapped to the given principal, as in a standalone container.
Principal accessed_as = 6;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we insist on this field as part of ControllerPublish then should we make this a repeated field since the same vol might be accessed by multiple users/groups (via multiple NodePublish calls)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather think that would require multiple ControllerPublish calls as there might be multiple NodeIDs involved and that information is outside the principal, or not?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the credential does not need to be passed as part of the controllerPublish. The credentials are to protect the data - to identify on behalf of which user every IO should be counted (and authorized against).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oritnm can you clarify how you imagine that working? The container will have a bind mounted device or volume that is accessed by container users using the regular POSIX conventions. How would implementation specific credentials get passed to the plugin via that channel? Also, even if it were possible for the container application to pass credentials through the standard file APIs, we also need credentials in some cases in order to perform mounts or initiate iscsi or whatever happens for the specific storage device we are attaching.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julian-hj , Within the NodePublish the CO will provide all required credentials, such that the SP can authenticate the user prior to the mounting itself, and do the mount on behalf of this user. Future IO request using this mount point will be send to the storage and will be accounted as that user/group. At this point the storage itself (the file system for example) can verify the user is authorized for the operation using POSIX rules.
I don't think this is required at the containerPublish as well, as before being able to send IOs to the device it needs to be published using the NodePublish

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the controller plugin passed some token to use for node publish (in VolumeInfo?), this would be fine, and ultimately up to the SP to implement rather than relying on the plugin spec to deal explicitly with the credentials in this case.

Copy link

@oritnm oritnm Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A principal parameter has been added to NodePublish (in this PR)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 The credentials in here are used to define how the processes in a container access a volume published in that container. All access through that volumes mount in that container are mapped to the credentials defined in the principal, pinning the access to the SPs volume to a specific ID. I do not think it is possible to avoid that information on the node.
In a setup where this is not preferred or possible it can be omitted as the whole field is optional. So if the cluster is setup to use untrusted nodes there's no need to transfer that information to the nodes.

@oritnm do you mean PR #50 ? From the current discussions i get the thinking that this might be handled differently with credentials outside the herein defined principal fields. Please feel free to comment on this in PR #50

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@casusbelli I do not see it as something that can be optional as it will make plugins work on some CO's and not others.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 oh, that's not what i intended. In the use cases i see the CO can pin volume access to a specific identity but he can also just leave that untouched resulting in the identity from inside the container being handed through to the storage backend without a specific mapping. Why would not using a mapping result in non-working plugins?

@jdef
Copy link
Member

jdef commented Jul 13, 2017

The user/group/namespace model for Principal, as proposed, has some drawbacks:

  1. it doesn't consider alternative container representations (e.g. hierarchical) where multiple users may access the same volume from multiple sub-containers in a hierarchy, or that a container that runs multiple processes may run each process w/ a separate user. Assuming that "NodePublish" is called for every sub-container is probably not valid.
  2. it's closely aligned with the k8s model (possibly at the expense of others)
  3. credentials (secrets, whatever) to authenticate volume operations should probably not be embedded within a Principal, but are likely better off as first-class fields of their own (they merit an API that's separate from the concept of Principal). Principal is intended to reflect how a volume will be used by a container, which MAY be independent of authn required for a volume operation executed by a CO.

My understanding of this PR was that some user/group/role should be granted access to a volume. Maybe this ends up being implemented in the form of one or more supplemental groups that are given access to the volume when it's mounted. Maybe it's an SE label. Maybe it's Windows ACLs. We don't need to solve for all possibilities here, but I'd like something that's non-OS and non-CO specific.

A secure mechanism that enables that access (password, certificate, whatever) is out of scope for this ticket.

Even the inclusion of namespace is starting to feel weird and a bit too CO-specific (k8s).

I'd like to re-propose a Principal model that I wrote about earlier: #30 (comment)

message Principal {
  message User {
    string name
  }
  message Group {
    string name
  }
  message Role {
    string name
  }
  message Everyone {}

  string namespace

  oneof value {
    User user
    Group group
    Role role
    Everyone everyone
  }
}

message PrincipalAccess {
  // both fields are REQUIRED
  bool readonly
  Principal principal
}

message ControllerPublishVolumeRequest {
  ...
  // replace readonly and accessed_as with the following:
  // REQUIRED. If not interested in user/group/role or namespace, then specify
  // a single PrincipalAccess { readonly = ..., principal = { everyone = {} } }
  repeated PrincipalAccess principal_access
}

@jdef
Copy link
Member

jdef commented Jul 13, 2017

I'd like to clarify my prior comment further: my interpretation of #17 as it relates to "authorization of IO requests" (read and write IO requests) is that such authorization MAY be orthogonal to the authn/authz required by a CSI operation to complete.

@jieyu
Copy link
Member

jieyu commented Jul 13, 2017

I feel that it'll be very hard for us to agree on a first class Principal in the spec because there are so many ways to do access control (e.g., RBAL, ABAC, DAC, LBAC, ...).

Given that, I'd like to propose that we introduce a more general field in NodePublishVolumeRequest. Ultimately, what we want here is a way to allow the CO to pass per "use" information to the SP that is opaque to the CO (because different SP might have different ways to specify principals, credentials, etc.).

Each plugin might be expecting different "keys". For instance, one plugin might expect "user" and "group" to be specified in parameters (this naming can be discussed), while others might expect "token" to be specified in parameters. The reason the value is bytes here is because some credentials might be a binary file. This model maps to most CO's secret store well.

message NodePublishVolumeRequest {
  ...
  map<string, bytes> parameters;
}

@julian-hj
Copy link
Contributor

+1 to the suggestion from @jieyu above. That looks much better to me and solves the use cases I'm aware of.

@casusbelli
Copy link
Author

Generic k/v as proposed by @jieyu should work if there are clearly defined default keys for the different scenarios, e.g. 'auth_token', 'user_id', 'namespace_id', etc. . The idea is to not let this drop to generic k/v passing without commonly defined content. Would that work for everybody?

Just to make sure: This is for pinning i/o from processes in a specific container to a specific identity when accessing a volume. This is not about authorizing nodes to access a storage backend.

@oritnm
Copy link

oritnm commented Jul 16, 2017

Agree with @casusbelli, I think it is problematic to have a generic key/value pairs as it requires strict definition of what each key is. These information needs to be provided explicitly as first class citizen to NodePublish such that it can be used against different COs and different SPs.
Not all parameters must be provided, but a clear naming is a requirement in my perspective from a spec as we can see from this discussion

@jdef
Copy link
Member

jdef commented Jul 17, 2017 via email

@jieyu
Copy link
Member

jieyu commented Jul 17, 2017

@casusbelli @oritnm Introducing default keys will be no different than first class those fields. I don't think there's a standard for access control scheme that all COs can agree on. Without that, access control information will be better be left opaque to the CO.

@quolix
Copy link

quolix commented Jul 18, 2017

@jieyu the goal of this proposal is to get access to the user identity that is associated with a container so that it can be mapped to the user identity that does file system access. No user identity, no access control on files and directories.

While the user identity concept on the CO side may not be well-defined yet, I think it is safe to say that in many cases CO will have a meaningful user identity associated with a running container. Further, this user identity will usually be already authenticated by the CO as it carries other semantics and rights. As storage access is an important aspect of infrastructure, I would even posit that its requirements should be considered when defining a user identity concept, but that's another discussion. For this purpose, we just need to assume that a relevant number of COs have at least a user name associated with the container that requests volume mappings.

On the other side we have file systems that have a very well-defined mechanism for authorizing access, which needs on UNIX-like systems user name / group names (from uid and gids) and on Windows a user name (from the sid).

I think together this makes a strong case for providing SPs with a user identity that at least carries a user name, if the the CO is able to provide that in a meaningful way. And groups and some form of tenant identifier, if it is available.

From my understanding the discussion here identified a second scenario where storage access needs unspecified credentials that are authenticated by the storage system and not the CO. While this is a valid scenario, which may need a different level abstraction, we should no let it weaken the well-defined scenario that I laid out above. For me it is unclear if the two should be solved by the same mechanism (I tend to say no, because one is for tunneling unspecified secrets while the other provides a well-defined mapping), but at least tenant/user/groups needs e a well-defined mapping, either as a protocol buffer as we proposed, or well-defined keys.

@oritnm
Copy link

oritnm commented Jul 19, 2017

@casusbelli , are you referring to the user running the container itself or the user within it? the user within it is somewhat unusable since it could be from a different namespace and in many cases will be root which is obviously not the user you would like to expose to the SP for authorization.

I think the ability to provide a user which is known to the SP is also important in addition to secrets if provided to the container in a secret form or using kerberos other mechanism. (as noted in the original ticket of #17. Could be handled in a different mechanism but i think a generic one that coudl cover all is best

@casusbelli
Copy link
Author

@oritnm The mapping maps the arbitrary IDs from inside the container to the mapping ID specified the CO in the publish call. The mapping ID should be known by the cluster infrastructure. Does that answer your q?
I think adding a secret to either the k/v list or the principal message should be fine.

@yaronha
Copy link

yaronha commented Jul 19, 2017

@jieyu i hope its clear by now why a generic k/v wont work, user name/id is not a generic field used by the file system and just "stored" by the orchestrator, it is the thing that correlate the container name/id from the container user namespace, PAM, POSIX etc' with the shared file system resources and identities. its also mandatory for POSIX to work, so e.g. "owner" identity of a file must be a vaiable one, and the user should be able to access the file from the container or from other places assuming he had the same identity and credentials. Secrets can be provided in a k/v map.

@jdef
Copy link
Member

jdef commented Jul 19, 2017

Straw man CO --> plugin process:

        1. CO --> Plugin: CreateVolume
                a. plugin carves a lun according to parameters
                b. plugin returns vol-info for the lun
        2. CO --> Plugin: ControllerPublishVolume
                a. CO wants the (external) lun attached to the node
                b. plugin orchestrates a connection between backend system and node X
                        i. may need cluster-wide identity information to proceed
                        ii. may need workload-specific credentials (secrets) to proceed
                        iii. may need workload-specific params to proceed
                c. lun appears as /dev/xyz on node X
        3. CO --> Plugin: NodePublishVolume
                a. CO wants the vol mounted to some container path /target/path
                b. plugin executes mount operation, mounting /dev/xyz to /target/path
                        i. may need cluster-wide identity information to proceed
                        ii. may need workload-specific credentials (secrets) to proceed
                        iii. may need workload-specific params to proceed
                c. CO sets volume ownership and related security context for /target/path

I think this PR is aiming to resolve (2.b.i.) and (3.b.i.)

It's unlikely that, for CSI v1, the CO's will agree on a standard representation for "cluster wide identity". Is it workable to, instead, clearly separate the notions of identity, credentials, and other parameters using first-class API fields? Does this approach provide sufficient boundaries/scoping for the information that plugins want access to? For example:

map<string,string> identity; // OPTIONAL: CO-specific identity information here
map<string,bytes> credentials; // OPTIONAL: requisite "secrets" go here
map<string,string> parameters; // OPTIONAL: other CO/user-specified publish parameters go here

In this case, k8s might provide an identity map:

identity := map[string]string {
  "namespace": "xyz/123",
  "service": "foo",
}

... and Mesos might provide an identity map:

identity := map[string]string {
  "user": "nobody",
  "role": "production",
}

As long as CO's publish documentation as to how they'll present identity to the plugins, is that sufficient for CSI v1? And we can tackle a more specific, cross-CO compatible cluster-wide identity API post v1?

@yaronha
Copy link

yaronha commented Jul 19, 2017

@jdef this proposal seem to be very block (lun) centric, users & groups will act different when it comes to shared files, e.g. different sub folders can be owned by different users and groups, same files may be accessed by multiple containers, Creation of a share is not exclusive, no need to attach Luns to nodes, etc.
For such a proposal to be applicable it must take shared files into consideration, IMO shared files make even more sense with containers than luns, and we cant just ignore them.

@jdef
Copy link
Member

jdef commented Jul 19, 2017

Thanks, @yaronha. Would you mind putting together a straw-man workflow for the use case you have in mind and explain how the fields I've proposed above don't solve the problem at hand?

@yaronha
Copy link

yaronha commented Jul 19, 2017

@jdef i will let @oritnm fill the flow, but it seems like you think of identity in terms of zoaning (e.g. k8s namespace) and not as a way to provide global file authorization, I’m not surprised because in block land you cant authorize file level access. User name is a MUST for the file systems to work, this is the ID used for mounting the share and must be correlated with the user identity associated with the running container. We cant have different COs interpret the user name in different ways, we must have uniform behavior.

I guess it would be a good exercise for you to try and see if/how the flow you provided will map to CIFS/NFS or GlusterFS.

lets take your Mesos example, if i check the username inside the container, would that print out "nobody" ? and would "nobody" be a known in some identity system like LDAP/Kerb/PAM/.. ? Would "nobody" on Node A mean exactly the same user in Node B or they are part of different user namespaces? if the answer is NO it means it failed the test and created data breach.

i dont get the "role" field example, is the CO also the identity management system ? i would assume the storage takes the user/group identities & secret to an identity manager (e.g. LDAP) and it will return the role, wouldnt expect the role to come from the CO unless its acting as the org identity manager or an identity proxy.

@jdef
Copy link
Member

jdef commented Jul 20, 2017

The former example that showed a Mesos identity with user and role was intending to illustrate identity in terms of Mesos concepts. You can find out more about Mesos roles by reading the docs here: http://mesos.apache.org/documentation/latest/roles/. Apologies for any confusion.

@yaronha I asked for a flow because even among the short list of filesystem types you've named, there's a variety of integration options/potential solutions. I can post another straw man flow for one of them, making some assumptions along the way. Maybe it's helpful, maybe not. If you have a specific use case that breaks down, it would be really helpful if you could provide a workflow and highlight where the breakdown happens.

Here's a straw man for using CIFS with pods:
[EDIT]

    1. Controller service does NOT advertise **any** RPC capabilities (e.g. CREATE_DELETE_VOLUME)
    2. CO --> Plugin: NodePublishVolume
        a. CO wants the CIFS share mounted to some container path /target/path
        b. plugin executes mount operation, mounting //share/xyz to /target/path
            i. may need cluster-wide identity information to proceed
            ii. may need workload-specific credentials (secrets) to proceed
            iii. may need workload-specific params to proceed
        c. (noop?) CO sets volume ownership and related security context for /target/path

    Assumptions, given that we're aiming for AD/Kerberos integration

    I.   AD controller maps Unix IDs to Windows SIDs
    II.  Pod-like multi-container unit runs requisite krb daemons, maybe renews krb tickets
    III. User namespacing enforced by container runtime, all pod CT's share a userns
    IV.  Avoid in-kernel keyring caching (https://docs.pagure.org/SSSD.sssd/design_pages/kcm.html)
    V.   Non-secret krb configuration may be supplied by per-workload params

    - CO's cluster-wide identity might not be useful here (1.b.i.), let's ignore it
    - workload-specific credentials (secrets) are probably useful here, e.g. keytab (1.b.ii.)
    - probably need specific mount.cifs options (sec=krb5); probably a plugin default option

@oritnm
Copy link

oritnm commented Jul 23, 2017

@jdef, thanks for the thorough explanation here, I see however several issues:

  1. within this representation, although the identities and credential are provided as first class params they are still requiring the SP to understand how each one of the COs are working and map stuff, I think one of the benefit is to have a consolidate definition.
  2. We must support the following use case
     
  • a filesystem/bucket was created out of scope
  • container running on CO of type A was launched for user X (whatever user is in CO A is called), with provided credentials, within the container the used user is root, the CO calls NodePublish with the information of user X (not root). The SP makes the required checks (possibly authentication and authorization) and complete the NodePublish with a mount point. Every IO that will be sent to that bucket/file system/share will be accounted for user X to be used for authorization.
  • An additional container is launched on CO A for user Y. same flow with authentication and authorization for user Y.
  • An additional container is launched on CO of type B under user X for the same shared bucket/filesystem. The same flow should be able to carry on, where the SP (although a different instance of it , preferably same code) would create the mount for user X.
     
    Few notes from this flow:
  1. identities in some use-cases may be larger than a cluster-wide identity of a CO. It could be shared by different Cos of different types, in cases where there is an external source of identities such e.g LDAP or even Kerberos. Specifically Kerberos is somewhat more complex:
    a. The KDC can and should be shared by many containers/COs and should not be run within the Pod/Container itself
    b. Kerberos ticket should be on a per service, thus each SP would need to have its own, creating
  2. Users identities must be of the launching user and not of the user within the container namespace (which is usually used as 0 which is equivalent to root)

@jdef
Copy link
Member

jdef commented Sep 7, 2017

Is this superseded by #99 ? If so, does it make sense to close out this PR?

@jdef jdef added the lifecycle/stale PR has been inactive for 90+ days. label Oct 4, 2018
@saad-ali
Copy link
Member

saad-ali commented Nov 7, 2018

@saad-ali saad-ali added lifecycle/rotten Stale PR has been inactive for 30+ days. and removed lifecycle/stale PR has been inactive for 90+ days. labels Nov 7, 2018
@saad-ali
Copy link
Member

saad-ali commented Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Stale PR has been inactive for 30+ days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants