Skip to content
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

user_credentials problems #140

Closed
saad-ali opened this issue Nov 8, 2017 · 6 comments
Closed

user_credentials problems #140

saad-ali opened this issue Nov 8, 2017 · 6 comments
Milestone

Comments

@saad-ali
Copy link
Member

saad-ali commented Nov 8, 2017

The CSI calls CreateVolumeRequest, DeleteVolumeRequest, ControllerPublishVolumeRequest, ControllerUnpublishVolume, NodePublishVolume, and NodeUnpublishVolume all allow user_credentials to be passed in.

  // End user credentials used to authenticate/authorize volume creation
  // request.
  // This field is OPTIONAL.
  Credentials user_credentials = ...

A few problems with this:

  1. If a SP requires user_credentials on Publish calls, and the volume is dynamically provisioned with a CreateVolumeRequest, how does the user or CO decide what credentials to pass into the Publish calls?
    • We should consider allowing SP to return ControllerPublish and NodePublish credentials in the CreateVolumeRequest.
  2. ControllerPublish is a call that is called once per VOLUME per node (not once per workload). Consider the case where two different workloads belonging to two different users both use the same volume. The credentials passed in on ControllerPublish therefore should not (can not) be used to auth a workload or user, but that the volume is allowed to be attached to that node.
    • We should consider changing user_credentials in ControllerPublish/Unpublish calls to volume_credentials and clarify in the comment that it should not (can not) be used to auth a workload or user). We would have to do the same thing for "MountDevice" proposed in Consider a "MountDevice" equivalent step #119.
@saad-ali
Copy link
Member Author

saad-ali commented Nov 9, 2017

For problem 2 above, a better solution would be to rename:

  • user_credentials in CreateVolumeRequest and DeleteVolumeRequest to create_credentials
  • user_credentials in ControllerPublishVolumeRequest and ControllerUnpublishVolume to controller_publish_credentials
  • user_credentials in NodePublishVolume and NodeUnpublishVolume to controller_unpublish_credentials

For problem 1, a kink with the proposed solution is that CreateVolume is called once per volume but there could be multiple controller_unpublish_credentials, which ones should be returned?

@akutz
Copy link
Contributor

akutz commented Nov 9, 2017

Hi @saad-ali,

We should consider allowing SP to return ControllerPublish and NodePublish credentials in the CreateVolumeRequest

Do you mean in the CreateVolumeResponse?

@akutz
Copy link
Contributor

akutz commented Nov 9, 2017

Hi @saad-ali,

I understand your concerns, but I do wonder if you're unintentionally overcomplicating things?

Issue 1

We should consider allowing SP to return ControllerPublish and NodePublish credentials in the CreateVolumeRequest CreateVolumeResponse.

This proposal complicates the relationship between the CO and the SP. The Controller SP is responsible for providing credentials to be used by the CO for calls to Node SPs? I thought the purpose of defining credentials as part of the spec was to avoid having to pre-configure credentials for the SP process/container? This seems to take things back to the way they were before .

Issue 2

a better solution would be to rename

I wonder if there is value in renaming the fields since credentials sent along with an RPC automatically become associated with that RPC's purpose IMO. For example:

message CreateVolume {
  // These are implied to be credentials used to 
  // create a volume.
  map<string, string> user_credentials
message ControllerPublishVolume {
  // These are implied to be credentials used by a 
  // controller to publish a volume.
  map<string, string> user_credentials

And so on and so forth. Is there value in renaming those fields to basically contain an RPC-eponymous prefix?

Besides, didn't we all agree you had to work on the error PR before touching anything else? :)

@akutz
Copy link
Contributor

akutz commented Nov 9, 2017

Hi @saad-ali,

@clintkitson explained what you were trying to convey, and it makes more sense now. I believe he'll follow up here in case anyone else is as easily confused as me :)

@clintkitson
Copy link

@akutz I belive the confusion here relates to how user_credentials introduces another set of credentials relevant to the plugins.

Storage system/API level creds are relevant and used for most platforms. But some platforms are have volume-level authorization that happen in many forms. I see the usefulness in either a) enabling the plugin to pass auth for the volume back on a create response or b) enabling the plugin to create auth, pass it to the storage platform in some way, and then pass this auth back. It sounds like Saad's suggestion would enable this.

@akutz
Copy link
Contributor

akutz commented Nov 9, 2017

Hi @clintkitson,

Thanks! Yeah, I think what confused me was the notion of the SP returning credentials. When you referred to the SP returning a token it made more sense. There are:

  1. Storage API/platform credentials
  2. Volume authorization tokens/encryption keys

I suppose the latter are credentials in the purest sense, but as I was already thinking about concrete implementations I was considering EBS roles and how there can be discrete roles for creation, publication, etc. In EBS the volume authorization key isn't really referred to as one of those roles, so I didn't consider it as a credential.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants