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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,20 @@ message ControllerPublishVolumeRequest {
// Whether to publish the volume in readonly mode. This field is
// REQUIRED.
bool readonly = 5;

// OPTIONAL field for storing principal information to be used in
// this publishing of this volume. If the principal is present the
// plugin MAY ensure all access to the volumes contents through
// this mount is mapped to the identity specified by the principal.
// If no principal is provided the access ids are not mapped but handed
// through untouched.
// As this field is specific to a given publishing of a volume
// containers in a container hierarchy have to either have their own
// individual volume publishings or a parent container MAY provide
// 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 •
edited
Loading

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?

}

message ControllerPublishVolumeResponse {
Expand Down Expand Up @@ -615,6 +629,24 @@ message PublishVolumeInfo {
// possible. This field is OPTIONAL.
map<string, string> values = 1;
}

message Principal {
// describes attributes for defining the principal identity of
// a volume binding for a container.

// 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;
// This field is OPTIONAL. If specified it contains the CO user
// identity to which all access to a given volume through this publishings
// mounts is mapped to.
string user = 2;
// This field is OPTIONAL. If specified it contains the CO group
// identity to which all access to a given volume through this publishings
// mounts is mapped to.
string group = 3;
}
```

#### `ControllerUnpublishVolume`
Expand Down Expand Up @@ -875,6 +907,24 @@ message NodePublishVolumeRequest {
// Whether to publish the volume in readonly mode. This field is
// REQUIRED.
bool readonly = 7;

// OPTIONAL field for storing principal information to be used in
// this publishing of this volume. If the principal is present the
// plugin MAY ensure all access to the volumes contents through
// this mount is mapped to the identity specified by the principal.
// If no principal is provided the access ids are not mapped but handed
// through untouched.
//
// The principal of the NodePublishVolumeRequest MUST match the
// principal in the corresponding ControllerPublishVolumeRequest.
//
// As this field is specific to a given publishing of a volume
// containers in a container hierarchy have to either have their own
// individual volume publishings or a parent container MAY provide
// 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 = 8;
}

message NodePublishVolumeResponse {
Expand Down