mon/AuthMonitor: don't validate fs authorize caps with valid_caps()
#21418
Conversation
jecluis
force-pushed
the
wip-fix-mon-fs-caps
branch
from
272318c
to
b3ca43b
Compare
src/mon/AuthMonitor.cc
Outdated
| @@ -1106,9 +1106,6 @@ bool AuthMonitor::valid_caps(const vector<string>& caps, ostream *out) | |||
| if (!mdscap.parse(g_ceph_context, *(p+1), out)) { | |||
| return false; | |||
| } | |||
| } else { | |||
| *out << "unknown cap type '" << *p << "'"; | |||
| return false; | |||
There was a problem hiding this comment.
why should we allow unrecognized services?
There was a problem hiding this comment.
to be perfectly honest, I'm not entirely sure whether we should or should not. The prototype's comment in the header file states we are going to ignore unrecognized services though, and I see calls to valid_caps() that I imagine may contain unrecognized services (e.g., auth add, auth caps, etc). If we do not allow unrecognized services, then we must be very sure we're either not using caps for other services (which we clearly do), or go through each one of the commands using caps and find ways to validate other caps that are not mon, mgr, osd or mds.
There was a problem hiding this comment.
whelp, when I say "we clearly do", I should have said "I think we do"; then again, maybe we don't. Do we?
There was a problem hiding this comment.
I don't think we do. We might let people store them, but they can't be used because the auth code has a hard coded set of services it recognizes.
IMO we should leave this new restriction in place and fix the header comment you saw?
jecluis
force-pushed
the
wip-fix-mon-fs-caps
branch
from
b3ca43b
to
cacad26
Compare
|
@liewegas repushed |
|
@batrick does validating just osd/mds caps in |
| } else { | ||
| if (out) { | ||
| *out << "unknown cap type '" << type << "'"; | ||
| } |
There was a problem hiding this comment.
this breaks
2018-04-14T05:51:59.176 INFO:tasks.workunit.client.0.mira022.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:49: expect_false: ceph auth add client.xx mon 'allow *' osd 'allow *' invalid 'allow *'
2018-04-14T05:51:59.608 INFO:tasks.workunit.client.0.mira022.stderr:unknown cap type 'invalid'
2018-04-14T05:51:59.627 INFO:tasks.workunit.client.0.mira022.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:49: expect_false: return 1
There was a problem hiding this comment.
Right, I suppose the test needs to be amended as per my earlier discussion with @liewegas
There was a problem hiding this comment.
huh, my bad; the problem is not returning false there
jecluis
force-pushed
the
wip-fix-mon-fs-caps
branch
from
cacad26
to
db39386
Compare
|
repushed. This was an unfortunate copy/paste error when factoring out caps validation to its own function. |
src/mon/AuthMonitor.cc
Outdated
| } else { | ||
| if (out) { | ||
| *out << "unknown cap type '" << type << "'"; | ||
| return false; |
There was a problem hiding this comment.
might want to move "return false" out of if (out).
jecluis
force-pushed
the
wip-fix-mon-fs-caps
branch
from
db39386
to
cdfed34
Compare
|
retest this please |
|
retest this please. |
The monitor will have no idea how to validate fs caps during `fs authorize`. Instead, validate solely the built osd and mds caps. Signed-off-by: Joao Eduardo Luis <joao@suse.de>
jecluis
force-pushed
the
wip-fix-mon-fs-caps
branch
from
cdfed34
to
4db0b88
Compare
|
@tchaikov this is finally passing checks (seems like it took a rebase on top of latest master); how did your run go? is this good to merge? |
|
@jecluis yes, it is =D |
Signed-off-by: Joao Eduardo Luis <joao@suse.com>