ceph_volume_client: set an existing auth ID's default mon caps

[ajarr]

... as 'allow r' (the minimum mon caps required to access a share)
when:

• authorizing the auth ID to access a volume.

• deauthorizing the auth ID to access a volume, but the auth ID is
  authorized to access other volumes.

In both the above cases, the ceph_volume_client previously tried to
set the mon caps of the auth ID to an invalid value, None.

Fixes: http://tracker.ceph.com/issues/17800

[toabctl]

+1

[jcsp]

Hmm, so is this the case where someone has created an auth ID outside Manila, and it has no mon caps, and then they try to authorize it with Manila?

In this case we should take the existing mon caps if they exist, and only override it with "allow r" if none are set, i.e. just make the get('mon') into a get('mon', 'allow r'). I think it's okay to potentially have more mon caps than they need for accessing a manila share: if they do, it's because the administrator created the user identity like that.

There's a similar case in _deauthorize(), let's fix that at the same time, except in that case we should only set "allow r" if the number of authorized volumes is >0, otherwise we should leave the mon auth caps blank.

[ajarr]

Quite a few questions @jcsp , sorry about that.

Hmm, so is this the case where someone has created an auth ID outside Manila, and it has no mon caps, and then they try to authorize it with Manila?

Yeah, that's what I assumed.

I think it's okay to potentially have more mon caps than they need for accessing a manila share: if they do, it's because the administrator created the user identity like that.

Is it okay to expect the storage/cloud admin to be aware of all the cephx client.* IDs (and their authorization levels and their implications) that were created by a non-Manila user/admin for the Ceph cluster that is being reused for Manila/CephFS_native_driver use case (possibly the reason that this bug was found)?

The manila client (at least for now) in the case of cephfs_native driver needs direct access to the public network of the storage cluster. So if the manila client accidentally gets a ceph auth ID with MON caps of 'allow rwx' (the manila client can now create ceph auth IDs with excess permission levels?) instead of 'allow r' the manila setup is less safe even in a private cloud scenario?

What are the ills of always overwriting the MON caps of an auth ID to be used by Manila clients as 'allow r'? Are you worried about the case where a manila user tries to unwittingly modify the MON caps of commonly used ceph auth IDs e.g. 'client.admin' via the ceph_volume_client?

Side note: Maybe the ceph_volume_client should immediately disallow authorizing/deauthorizing auth ID 'client.admin' and return its access key as is currently possible?

As you suggest, if we just reuse existing MON caps of an auth ID, is it possible that the minimum MON capability 'allow r' needed by a Manila client not be granted to the auth ID?

There's a similar case in _deauthorize(), let's fix that at the same time, except in that case we should only set "allow r" if the number of authorized volumes is >0, otherwise we should leave the mon auth caps blank

I think it's okay for _deauthorize() to just get the existing MON caps, the current behaviour. What I missed to do in this PR: in case someone tries to _deauthorize() an auth ID that was not authorized by ceph_volume_client and which doesn't have any MON caps set, the ceph_volume_client should not set any MON caps, i.e., (if not cap['caps'].get['mon']) the ceph_volume_client should not set any MON caps.

I'm also confused here by your comment. Earlier for authorize() you wanted the auth ID's existing MON caps to be persisted, and here during deauthorize() you want it to be set to 'allow r'?

[jcsp]

Quite a few questions @jcsp , sorry about that.

Hmm, so is this the case where someone has created an auth ID outside Manila, and it has no mon caps, and then they try to authorize it with Manila?
Yeah, that's what I assumed.

I think it's okay to potentially have more mon caps than they need for accessing a manila share: if they do, it's because the administrator created the user identity like that.
Is it okay to expect the storage/cloud admin to be aware of all the cephx client.* IDs (and their authorization levels and their implications) that were created by a non-Manila user/admin for the Ceph cluster that is being reused for Manila/CephFS_native_driver use case (possibly the reason that this bug was found)?

The manila client (at least for now) in the case of cephfs_native driver needs direct access to the public network of the storage cluster. So if the manila client accidentally gets a ceph auth ID with MON caps of 'allow rwx' (the manila client can now create ceph auth IDs with excess permission levels?) instead of 'allow r' the manila setup is less safe even in a private cloud scenario?

The key thing is that we should never grant caps beyond allow r, but if they already exist, we shouldn't get rid of them.

I think you're pointing out that the Manila admin might be unaware that the identity he has authorized has greater mon caps, and so might not be aware that by giving that key to a guest, he is giving them greater power than just what he has explicitly authorized. I don't think we should try and second-guess the user on this; they are root and have the power to hand out whatever keys they want.

I think that is OK. We only get into this situation if the admin has explicitly gone and created his own identities and then reused those names in Manila.

What are the ills of always overwriting the MON caps of an auth ID to be used by Manila clients as 'allow r'? Are you worried about the case where a manila user tries to unwittingly modify the MON caps of commonly used ceph auth IDs e.g. 'client.admin' via the ceph_volume_client?

Yes.

Side note: Maybe the ceph_volume_client should immediately disallow authorizing/deauthorizing auth ID 'client.admin' and return its access key as is currently possible?

Hmm, maybe. But I'm not sure I want ceph_volume_client to be acting like a security gatekeeper -- its job is to add and remove the caps that are needed to access particular volumes, not to tell the user what other caps they are allowed to have on those identities.

As you suggest, if we just reuse existing MON caps of an auth ID, is it possible that the minimum MON capability 'allow r' needed by a Manila client not be granted to the auth ID?

IIRC "allow r" is the lowest mon cap that it's possible to have, so we shouldn't have that issue.

There's a similar case in _deauthorize(), let's fix that at the same time, except in that case we should only set "allow r" if the number of authorized volumes is >0, otherwise we should leave the mon auth caps blank
I think it's okay for _deauthorize() to just get the existing MON caps, the current behaviour. What I missed to do in this PR: in case someone tries to _deauthorize() an auth ID that was not authorized by ceph_volume_client and which doesn't have any MON caps set, the ceph_volume_client should not set any MON caps, i.e., (if not cap['caps'].get['mon']) the ceph_volume_client should not set any MON caps.

I'm also confused here by your comment. Earlier for authorize() you wanted the auth ID's existing MON caps to be persisted, and here during deauthorize() you want it to be set to 'allow r'?

What I should have said was that we should leave the existing mon auth caps in place in deauthorize, unless they are None, in which case we should put in an "allow r" if there are still volumes authorized, or we should leave them empty if there are no volumes authorized.

[ghost]

jenkins test this please (eio now ignored in master)

[ajarr]

What I should have said was that we should leave the existing mon auth caps in place in deauthorize, unless they are None, in which case we should put in an "allow r" if there are still volumes authorized, or we should leave them empty if there are no volumes authorized.

I think the case where there auth ID does not have access to any volume, but is still asked to have its access deauthorized for a volume is handled earlier, ec2e6e3#diff-8625369b924524f064e083e735bd34beR1041

[ajarr]

I think you're pointing out that the Manila admin might be unaware that the identity he has authorized has greater mon caps, and so might not be aware that by giving that key to a guest, he is giving them greater power than just what he has explicitly authorized. I don't think we should try and second-guess the user on this; they are root and have the power to hand out whatever keys they want.

By default, not just the admin, but any manila user belonging to the tenant to which the share belongs to can modify the share's access rules (https://github.com/openstack/manila/blob/master/etc/manila/policy.json#L28 ,
and https://github.com/openstack/manila/blob/master/etc/manila/policy.json#L4). I'm talking about when manila policies are set up in such a way, and in addition, an existing Ceph cluster with a lot of existing auth IDs and not all of whose permissions the Manila admin is aware of, is integrated with Manila/cephfs_native driver. In such a setup, any manila user (belonging to a tenant) can authorize any existing ID (possibly with excessive privileges) access to any share (belonging to the tenant) and fetch the ID's access key. The manila admin may not be aware that a user has unknowingly given himself excessive permissions as the admin did not explicitly create the auth ID or given it excessive mon caps. I guess my imagined scenario/setup, is not a real world concern?

Hmm, maybe. But I'm not sure I want ceph_volume_client to be acting like a security gatekeeper -- its job is to add and remove the caps that are needed to access particular volumes, not to tell the user what other caps they are allowed to have on those identities.

In addition, ceph_volume_client returns access keys of any existing client.* ceph auth ID that is not used by an another OpenStack tenant. I was concerned about this function of the volume_client. This is a concern that you'd noted as a code comment, see
d2e9eb55#diff-8625369b924524f064e083e735bd34beR791

Yeah, this concern may not be connected to this bug fix, but is still a valid concern, right?

[jcsp]

The code comment that you linked to is the crux of this. The way we implemented the auth metadata, we allowed claiming of existing keys by Manila tenants, and rely on the share service's key to restrict it from touching ceph keys outside a particular prefix.

The rules for people deploying manila should be:

• restrict client.manila (or whatever it's called) to a key prefix like "client.manila."
• don't issue any extra caps to client.manila.* keys unless you want your VM guests to have them

[ajarr]

@jcsp, the rules recommended would've worked if the volume client prefixed 'client.manila' instead of 'client.*' to the ceph auth IDs that it creates, or fetches and modifies it perms.

Sorry, about this jcsp. Just remembered that we'd this discussion back in May about 'prefixing client.manila' to IDs. And the conclusion was that we don't do it (as it would be non-intuitive to manila users who'd ask for access for 'alice', but would've to eventually use 'manila.alice' while mounting the shares), but instead use auth meta file to reject allowing access to auth IDs that weren't created by the volume client.

jcsp wrote in the earlier discussion about 'prefixing client.manila' to IDs,

We would then need to decide how to handle clients authorizing pre-existing keys (like client.admin for example). Maybe cephfsvolumeclient should forbid operations on keys that already
exist, unless there also exists a corresponding .json file?

I forgot to implement this. I think I should go ahead and do it. Plan sound OK?

[jcsp]

@ajarr I'm still kind of keen on requiring a manila prefix on the authorized names, and restricting the client.manila identity to only operate on keys with that prefix (using its mon caps) -- that way we are not relying on the correctness of our python code to prevent badness.

IIRC there was an issue with using "manila." as the prefix because our Manila code rejects names with dots in, but we could always suggest using "manila-" or similar as a prefix.

I don't want to get too tied up in this because it will all be less relevant once the NFS variant of the Manila driver is out there