mon/AuthMonitor: Add special syntax for cephfs #16761

fullerdj · 2017-08-02T15:49:20Z

Add a new command, ceph auth fs, to generate and apply auth caps for
cephfs.

Syntax:

ceph auth fs <fs_name> <entity_name> (

)...

Notes:

In this case, the AuthMonitor will determine the correct osd and
mds cap syntax based on the data pools used by <fs_name>. The same
caps will be applied to all data pools regardless of the filesystem
configuration. If the write cap is enabled on any directory, the write
cap for every data pool will be obtained.

It is legal to run ceph auth fs on an entity that already has auth
caps. In this case, the new values will overwrite the old ones and the
old key will be invalidated. In this case, clients with existing
mounts will retain their old auth caps, but will be unable to mount
with old keys. Administrative care is required to unmount or fence any
old clients after auth cap updates.

Fixes: http://tracker.ceph.com/issues/20885
Signed-off-by: Douglas Fuller dfuller@redhat.com

fullerdj · 2017-08-02T16:00:09Z

src/mon/MonCommands.h

@@ -175,6 +175,12 @@ COMMAND("auth get-or-create " \
 	"name=caps,type=CephString,n=N,req=false", \
 	"add auth info for <entity> from input file, or random key if no input given, and/or any caps specified in the command", \
 	"auth", "rwx", "cli,rest")
+COMMAND("auth fs " \


Opinions as to exact syntax welcome.

My preference would be fs authorize -- feels like it's more naturally going to be used alongside all the other fs commands, even if it's implemented in authmonitor.

I'd like that syntax better, too. Is there precedent for implementing commands in the "wrong" monitor like this?

I'd prefer this to be something like fs authorize as well. And there is a precedent. The most obvious would be pg map, which is handled by the OSDMonitor.
Basically, you'll have to add an exception to Monitor::handle_command(), where it checks if (module == "mds" || module == "fs"), and add a new || prefix == "fs authorize" (or whatever ends up being the name of the command.
For correctness' sake, you'll have to keep the COMMAND's third field, which now reads auth, as auth. This is used to validate caps during Monitor::_allowed_command().

Let me know if you bump into issues, and I'll be happy to help you out.

fullerdj · 2017-08-02T16:01:48Z

src/mon/AuthMonitor.cc

+    };
+
+    KeyServerData::Incremental auth_inc;
+    auth_inc.op = KeyServerData::AUTH_INC_ADD;


This is unconditional; I think this is probably what we want, but feedback is welcome.

We should check if the entry already exists; if so, it's basically a no-op and return to the user the appropriate info we'd be returning if this was the first time the command was being run.
If we don't, given we'll be creating a new key, we'll be replacing the key, and I don't think that's what a user would expect.
Given the user isn't providing a key, I suggest checking if the entity name exists, alongside with the same set of caps. If the caps for a given entity name differ from what we are expecting, fail; otherwise, succeed.

Ok, I implemented that behavior.

ceph-jenkins · 2017-08-02T16:46:37Z

make check succeeded

jecluis · 2017-08-02T16:54:39Z

src/mon/AuthMonitor.cc

+    };
+
+    KeyServerData::Incremental auth_inc;
+    auth_inc.op = KeyServerData::AUTH_INC_ADD;


We should check if the entry already exists; if so, it's basically a no-op and return to the user the appropriate info we'd be returning if this was the first time the command was being run.
If we don't, given we'll be creating a new key, we'll be replacing the key, and I don't think that's what a user would expect.
Given the user isn't providing a key, I suggest checking if the entity name exists, alongside with the same set of caps. If the caps for a given entity name differ from what we are expecting, fail; otherwise, succeed.

Add a new command, ceph auth fs, to generate and apply auth caps for cephfs. Syntax: ceph auth fs <fs_name> <entity_name> (<dir> <cap>)... Notes: In this case, the AuthMonitor will determine the correct osd and mds cap syntax based on the data pools used by <fs_name>. The same caps will be applied to all data pools regardless of the filesystem configuration. If the write cap is enabled on any directory, the write cap for every data pool will be obtained. If different auth caps for this entity already exist, the result is -EINVAL. Fixes: http://tracker.ceph.com/issues/20885 Signed-off-by: Douglas Fuller <dfuller@redhat.com> fixup

batrick

Docs and command are out of sync.

Change the CephFS auth caps documentation to reflect the new ceph auth fs command. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

fullerdj · 2017-08-04T19:52:05Z

Docs updated; this should be good to merge.

batrick · 2017-08-04T20:19:48Z

Just waiting for @liewegas to say this looks like the right approach and then I'll merge.

liewegas · 2017-08-04T20:25:19Z

This looks fine to me. My only concern is what happens when a new data pool is added at a later date. I'm not sure if/how the existing caps can be updated given that we're not flagging the caps as "magic" ones that need to be redone later.

Maybe we could stash that information (i.e., teh original path list with r or rw) in a separate cap (cephfs-auth) that isn't being used so that the cap can be identified later. (Just brainstorming)

batrick · 2017-08-04T20:33:37Z

Okay, I think we'll merge this as-is for now to set a path forward. I agree automating the cap changes when a new data pool is added would be smart. Also, the command could allow overwriting the caps (useful for qa at least) and maybe support multifs.

* refs/remotes/upstream/pull/16761/head: doc/cephfs: Document ceph auth fs mon/AuthMonitor: Add special syntax for cephfs Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

fullerdj · 2017-08-04T20:39:47Z

It might make more sense to tag data pools with the file system they service, then have an OSD cap for access to pools by tag.

…

On Fri, Aug 4, 2017 at 4:33 PM Patrick Donnelly ***@***.***> wrote: Okay, I think we'll merge this as-is for now to set a path forward. I agree automating the cap changes when a new data pool is added would be smart. Also, the command could allow overwriting the caps (useful for qa at least) and maybe support multifs. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16761 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6lgFWF5vctU7zqUt6E2PZVz8lI9BC8ks5sU4AlgaJpZM4OrSs6> .

gregsfortytwo · 2017-08-04T20:46:33Z

Actually I don't think we want to automatically add caps when new pools go into the FS (at least, not all the time) — they're a separate authentication/authorization zone.

liewegas · 2017-08-04T20:48:12Z

YES!

batrick · 2017-08-04T20:51:27Z

Great point @gregsfortytwo. @fullerdj please make a new tracker ticket for potential enhancements to "fs authorize" to consider.

Introduced by ceph#16761. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

fullerdj force-pushed the wip-djf-20885 branch 3 times, most recently from 0af0a30 to 1fb9547 Compare August 2, 2017 15:58

fullerdj added cephfs Ceph File System feature needs-review labels Aug 2, 2017

fullerdj commented Aug 2, 2017

View reviewed changes

fullerdj requested review from jcsp and batrick August 2, 2017 16:00

fullerdj commented Aug 2, 2017

View reviewed changes

jecluis requested changes Aug 2, 2017

View reviewed changes

jecluis added the mon label Aug 2, 2017

fullerdj force-pushed the wip-djf-20885 branch from 1fb9547 to f2a1f40 Compare August 2, 2017 20:02

batrick added the wip-pdonnell-testing label Aug 3, 2017

batrick requested changes Aug 3, 2017

View reviewed changes

doc/cephfs: Document ceph auth fs

f249e3d

Change the CephFS auth caps documentation to reflect the new ceph auth fs command. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

fullerdj force-pushed the wip-djf-20885 branch from f2a1f40 to f249e3d Compare August 4, 2017 15:10

batrick added the wip-pdonnell-testing label Aug 4, 2017

batrick approved these changes Aug 4, 2017

View reviewed changes

batrick added the wip-pdonnell-testing label Aug 4, 2017

batrick merged commit f249e3d into ceph:master Aug 4, 2017

batrick added a commit that referenced this pull request Aug 4, 2017

Merge PR #16761 into master

b327451

* refs/remotes/upstream/pull/16761/head: doc/cephfs: Document ceph auth fs mon/AuthMonitor: Add special syntax for cephfs Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

fullerdj deleted the wip-djf-20885 branch August 7, 2017 13:38

batrick added a commit to batrick/ceph that referenced this pull request Aug 7, 2017

doc: add new fs auth command to release notes

0540cb7

Introduced by ceph#16761. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick mentioned this pull request Aug 7, 2017

doc: add new fs auth command to release notes #16876

Merged

batrick added a commit to batrick/ceph that referenced this pull request Aug 7, 2017

doc: add new fs auth command to release notes

f2051c0

Introduced by ceph#16761. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mon/AuthMonitor: Add special syntax for cephfs #16761

mon/AuthMonitor: Add special syntax for cephfs #16761

fullerdj commented Aug 2, 2017

fullerdj Aug 2, 2017

jcsp Aug 2, 2017

fullerdj Aug 2, 2017

jecluis Aug 2, 2017

fullerdj Aug 2, 2017

jecluis Aug 2, 2017

fullerdj Aug 2, 2017

ceph-jenkins commented Aug 2, 2017

jecluis Aug 2, 2017

batrick left a comment

fullerdj commented Aug 4, 2017

batrick commented Aug 4, 2017

liewegas commented Aug 4, 2017

batrick commented Aug 4, 2017

fullerdj commented Aug 4, 2017 via email

gregsfortytwo commented Aug 4, 2017

liewegas commented Aug 4, 2017 via email

batrick commented Aug 4, 2017

mon/AuthMonitor: Add special syntax for cephfs #16761

mon/AuthMonitor: Add special syntax for cephfs #16761

Conversation

fullerdj commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceph-jenkins commented Aug 2, 2017

Choose a reason for hiding this comment

batrick left a comment

Choose a reason for hiding this comment

fullerdj commented Aug 4, 2017

batrick commented Aug 4, 2017

liewegas commented Aug 4, 2017

batrick commented Aug 4, 2017

fullerdj commented Aug 4, 2017 via email

gregsfortytwo commented Aug 4, 2017

liewegas commented Aug 4, 2017 via email

batrick commented Aug 4, 2017