-
Notifications
You must be signed in to change notification settings - Fork 570
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.
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
ObjectQueryHandler: Check user permissions on joined relations #9408
ObjectQueryHandler: Check user permissions on joined relations #9408
Conversation
d2acb74
to
21ffdac
Compare
b5a0291
to
952d404
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better! Now as we have a clear view, let’s have a look...
lib/remote/filterutility.cpp
Outdated
@@ -166,17 +166,43 @@ void FilterUtility::CheckPermission(const ApiUser::Ptr& user, const String& perm | |||
else | |||
*permissionFilter = new LogicalOrExpression(std::unique_ptr<Expression>(*permissionFilter), std::unique_ptr<Expression>(fexpr)); | |||
} | |||
|
|||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bad idea. Figure out and explain in your own words why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it probably due to the fact that the same permission might be used with different filters repeatedly? I don't know, why should one even do something like that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, not necessarily the same. Think of query/*
and query/Host
e.g..
// has been found!! | ||
continue; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, in case of insufficient permission you skip it... but otherwise you unconditionally include it, right? This is a also bad idea. Figure out and explain in your own words why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have to evaluate the permission filters as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. You know what to do...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be resolved, shouldn't it?
254df92
to
6483659
Compare
fe6ef0b
to
62698d1
Compare
ae93849
to
b90f8f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Riddle 1
The default config with icinga2 api setup
and
object ApiUser "root" {
password = "123456"
var perm = {
permission = "objects/query/checkcommand"
filter = {{ true }}
}
var fperm = () use (perm) => perm
permissions = [ "*" ] + range(10000).map(fperm)
}
leaks memory with
curl -ksSu root:123456 -H 'Accept: application/json' -X GET -d '{"joins":["check_command"],"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts
Why?
Riddle 2
The default config with icinga2 api setup
and
object ApiUser "root" {
password = "123456"
permissions = [ {
permission = "objects/query/service"
}, {
permission = "objects/query/checkcommand"
filter = {{ false }}
} ]
}
outputs some of the check commands with
curl -ksSu root:123456 -H 'Accept: application/json' -X GET -d '{"joins":["check_command", "host"],"pretty":true}' https://127.0.0.1:5665/v1/objects/services
Why?
Please only explanation in own words for now, thx.
Isn't that obvious to you? When you specify the same permission [2022-07-22 11:31:32 +0200] information/ApiListener: New client connection from [::ffff:127.0.0.1]:50112 (no client certificate)
Stack overflow while evaluating expression: Recursion level too deep.
[2022-07-22 11:31:32 +0200] information/HttpServerConnection: Request: GET /v1/objects/checkcommands (from [::ffff:127.0.0.1]:50112), user: dummy, agent: curl/7.79.1, status: Not Found).
[2022-07-22 11:31:32 +0200] information/HttpServerConnection: HTTP client disconnected (from [::ffff:127.0.0.1]:50112)
Yep, need to cache the permission filter of the individual types as well. |
34e0c31
to
f8bd84e
Compare
Oh, I exactly know why is what happening here. That’s why I called it a riddle. :-)
Absolutely. But Icinga even manages to properly handle that (pseudo) stack overflow. So not leaking memory is the least thing Icinga shall do. I'm joining check commands and only they have a such filter, so your join permission check is clearly responsible. Also I have no doubt it also works with a smaller filter, but mine leaks memory faster. |
f8bd84e
to
44ea4bc
Compare
Still happening. |
44ea4bc
to
cefeb5a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yhabteab @Al2Klimov Are you done with these riddles (i.e. do both of you agree that they are (re)solved)?
// has been found!! | ||
continue; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be resolved, shouldn't it?
cefeb5a
to
7243f5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it and worked fine. Just some formatting and details in the comments left (unless you want to do the extended suggestion within this PR).
4e29174
to
6aa6338
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine and my tests were successful, just a small formatting issue in one of the comments left.
6aa6338
to
72e6894
Compare
fixes #9339