mon: validate capabilitys before add auth entity #19794
Conversation
|
/home/jenkins-build/build/workspace/ceph-pull-requests/src/mon/AuthMonitor.h:151:34: error: 'CephContext* MDSAuthCaps::cct' is private within this context |
bellaalleb
force-pushed
the
validate_caps
branch
4 times, most recently
from
eaef05e
to
27fd5ac
Compare
|
@joscollin the problem was fixed. Please kindly take a look~ |
src/mon/AuthMonitor.cc
Outdated
| @@ -1155,6 +1155,12 @@ bool AuthMonitor::prepare_command(MonOpRequestRef op) | |||
| } | |||
| err = 0; | |||
|
|
|||
| //if capability strings are malformed, return with error msg. | |||
| if (!valid_caps(caps_vec, &ss)) { | |||
There was a problem hiding this comment.
Please move this check to right after the vector is populated. That would be right after the block ending in line 1044.
|
@jecluis Is this a candidate for backport to luminous? (I ask because the backport field in the tracker is currently empty.) |
Fixes: http://tracker.ceph.com/issues/22525 Signed-off-by: Jing Li lijing@gohighsec.com
bellaalleb
force-pushed
the
validate_caps
branch
from
27fd5ac
to
8584639
Compare
|
@jecluis done. |
|
@jecluis changed as required. Do you have any time to review the patch? Thanks~ |
|
Hmm, this is clearly a win for usability in most circumstances. But when we've discussed doing auth caps validation on the monitor in the past, we've been a bit leery about what happens when the monitors have a different compiled-in version than the daemon the caps belong to. Consider upgrade scenarios and such. This works in both directions:
So I haven't looked at the specific code, but at this point I'm thinking it's a good idea. @liewegas, thoughts? We should ping the other teach leads as well if you agree. |
|
Yeah, these days I'm for doing the validation in the mon. The most bad scenario above is that the mon accepts something the osds won't understand, but I think that's a step up from accepting anything. (And it's something we could pretty easily address by keeping around a validator for old versions and check for compat validity if the osds haven't all been upgraded.) The converse situation where the mon's aren't upgraded is a non-issue I think.. if you haven't finished upgrading then it's perfectly reasonable for the new thing not to work, right? |
|
Sounds good to me! |
|
👍 |
|
@gregsfortytwo Do I need to do anything about this patch? Thanks~ |
| MDSAuthCaps mdscap; | ||
| if (!mdscap.parse(g_ceph_context, *(p+1), out)) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This construction appears to validate the caps (by falling through all the test cases) if it doesn't match any of the known cap users. That seems bad?
There was a problem hiding this comment.
Here it only try to validate the caps when it's type is either "mon" or "osd" or "mds" and won't validate any other unknown types.
Do you mean that it's bad for unknown types?
There was a problem hiding this comment.
Yeah, I don't think we want to accept capabilities that don't match a known type. Otherwise somebody could typo and end up with caps on the "min" and none on the mon, or similar.
| if (!valid_caps(caps_vec, &ss)) { | ||
| err = -EINVAL; | ||
| goto done; | ||
| } |
There was a problem hiding this comment.
Why did you shift this check earlier?
There was a problem hiding this comment.
It was suggested by @jecluis
And I think it's better to check it right after the caps are generated, so in each specific cmd we don't need to check it repeatedly.
There was a problem hiding this comment.
Hmm, does this check pass if they're using eg "auth import" rather than a per-key command?
|
@jecluis, you have any other concerns? |
|
@bellaalleb update? |
|
Replaced by #21311 |
Fixes: http://tracker.ceph.com/issues/22525
Signed-off-by: Jing Li lijing@gohighsec.com