ObjectQueryHandler: Check user permissions on joined relations #9408

yhabteab · 2022-06-20T13:36:13Z

lib/remote/filterutility.cpp

Al2Klimov

Much better! Now as we have a clear view, let’s have a look...

Al2Klimov · 2022-06-23T07:54:33Z

lib/remote/filterutility.cpp

@@ -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;


This is a bad idea. Figure out and explain in your own words why.

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!

Oh, not necessarily the same. Think of query/* and query/Host e.g..

Al2Klimov · 2022-06-23T07:59:16Z

lib/remote/objectqueryhandler.cpp

+ // has been found!!
+ continue;
+ }
+


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.

Have to evaluate the permission filters as well!

Exactly. You know what to do...

This comment should be resolved, shouldn't it?

lib/remote/filterutility.cpp

lib/remote/objectqueryhandler.cpp

Al2Klimov

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.

yhabteab · 2022-07-22T09:37:26Z

Riddle 1

Isn't that obvious to you? When you specify the same permission 10K times with a callable filter, what would you expect to happen? I also get a stack overflow error but that is not something being added by this PR. Just try querying the CheckCommands directly and see what happens there.

[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)

Riddle 2

Yep, need to cache the permission filter of the individual types as well.

Al2Klimov · 2022-07-25T09:41:28Z

Isn't that obvious to you?

Oh, I exactly know why is what happening here. That’s why I called it a riddle. :-)

When you specify the same permission 10K times with a callable filter, what would you expect to happen? I also get a stack overflow error but that is not something being added by this PR.

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.

Al2Klimov · 2022-07-25T11:01:40Z

Riddle 2

Still happening.

julianbrost

@yhabteab @Al2Klimov Are you done with these riddles (i.e. do both of you agree that they are (re)solved)?

lib/remote/objectqueryhandler.cpp

julianbrost · 2022-10-05T14:36:36Z

lib/remote/objectqueryhandler.cpp

+ // has been found!!
+ continue;
+ }
+


This comment should be resolved, shouldn't it?

lib/remote/objectqueryhandler.cpp

lib/remote/filterutility.cpp

julianbrost

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).

lib/remote/filterutility.cpp

lib/remote/objectqueryhandler.cpp

julianbrost

Code looks fine and my tests were successful, just a small formatting issue in one of the comments left.

lib/remote/filterutility.cpp

…o a separate method

yhabteab requested a review from julianbrost June 20, 2022 13:36

cla-bot bot added the cla/signed label Jun 20, 2022

icinga-probot bot added area/api REST API ref/NC labels Jun 20, 2022

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch from d2acb74 to 21ffdac Compare June 21, 2022 13:53

yhabteab requested a review from Al2Klimov June 21, 2022 13:54

Al2Klimov reviewed Jun 22, 2022

View reviewed changes

lib/remote/filterutility.cpp Show resolved Hide resolved

lib/remote/filterutility.cpp Outdated Show resolved Hide resolved

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch 3 times, most recently from b5a0291 to 952d404 Compare June 23, 2022 06:55

Al2Klimov requested changes Jun 23, 2022

View reviewed changes

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch 2 times, most recently from 254df92 to 6483659 Compare June 23, 2022 11:00

Al2Klimov requested changes Jun 23, 2022

View reviewed changes

lib/remote/filterutility.cpp Outdated Show resolved Hide resolved

lib/remote/objectqueryhandler.cpp Outdated Show resolved Hide resolved

lib/remote/objectqueryhandler.cpp Show resolved Hide resolved

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch 2 times, most recently from fe6ef0b to 62698d1 Compare June 23, 2022 16:15

yhabteab requested a review from Al2Klimov June 23, 2022 16:15

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch from ae93849 to b90f8f8 Compare June 24, 2022 10:25

Al2Klimov reviewed Jun 27, 2022

View reviewed changes

yhabteab requested a review from Al2Klimov July 18, 2022 07:40

Al2Klimov requested changes Jul 20, 2022

View reviewed changes

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch 2 times, most recently from 34e0c31 to f8bd84e Compare July 25, 2022 08:23

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch from f8bd84e to 44ea4bc Compare July 25, 2022 10:34

yhabteab requested a review from Al2Klimov July 25, 2022 10:36

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch from 44ea4bc to cefeb5a Compare July 25, 2022 11:24

julianbrost requested changes Oct 5, 2022

View reviewed changes

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch from cefeb5a to 7243f5c Compare October 5, 2022 15:53

yhabteab requested a review from julianbrost October 5, 2022 15:54

julianbrost requested changes Oct 6, 2022

View reviewed changes

lib/remote/filterutility.cpp Outdated Show resolved Hide resolved

lib/remote/objectqueryhandler.cpp Outdated Show resolved Hide resolved

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch from 4e29174 to 6aa6338 Compare October 7, 2022 08:01

yhabteab requested a review from julianbrost October 7, 2022 08:02

julianbrost requested changes Oct 10, 2022

View reviewed changes

lib/remote/filterutility.cpp Outdated Show resolved Hide resolved

yhabteab added 3 commits October 10, 2022 12:33

FilterUtility: Outsource permission matching from CheckPermission() t…

1bb2d65

…o a separate method

ObjectQueryHandler: Check user permissions on joined relations

607f7ab

Evaluate permission filters also on all joined relations

72e6894

yhabteab force-pushed the bugfix/match-api-permissions-against-join-relations branch from 6aa6338 to 72e6894 Compare October 10, 2022 10:33

yhabteab requested a review from julianbrost October 10, 2022 10:34

julianbrost approved these changes Oct 10, 2022

View reviewed changes

Al2Klimov approved these changes Oct 11, 2022

View reviewed changes

Al2Klimov added this to the 2.14.0 milestone Oct 11, 2022

Al2Klimov merged commit 363f4d3 into master Oct 11, 2022

icinga-probot bot deleted the bugfix/match-api-permissions-against-join-relations branch October 11, 2022 11:42

julianbrost mentioned this pull request Oct 14, 2022

FilterUtility: Replace some nested raw pointers by unique_ptr<>* #9537

Merged

yhabteab mentioned this pull request Oct 31, 2022

Match api permissions against joined relations #9561

Merged

yhabteab added the backported Fix was included in a bugfix release label Oct 31, 2022

Al2Klimov mentioned this pull request Dec 6, 2022

Type::GetByName(): cache results for faster lookup #9553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ObjectQueryHandler: Check user permissions on joined relations #9408

ObjectQueryHandler: Check user permissions on joined relations #9408

yhabteab commented Jun 20, 2022

Al2Klimov left a comment

Al2Klimov Jun 23, 2022

yhabteab Jun 23, 2022 •

edited

Loading

Al2Klimov Jun 23, 2022

Al2Klimov Jun 23, 2022

yhabteab Jun 23, 2022

Al2Klimov Jun 23, 2022

julianbrost Oct 5, 2022

Al2Klimov left a comment

yhabteab commented Jul 22, 2022

Riddle 1

Riddle 2

Al2Klimov commented Jul 25, 2022

Al2Klimov commented Jul 25, 2022

Riddle 2

julianbrost left a comment

julianbrost Oct 5, 2022

julianbrost left a comment

julianbrost left a comment

ObjectQueryHandler: Check user permissions on joined relations #9408

ObjectQueryHandler: Check user permissions on joined relations #9408

Conversation

yhabteab commented Jun 20, 2022

Al2Klimov left a comment

Choose a reason for hiding this comment

Al2Klimov Jun 23, 2022

Choose a reason for hiding this comment

yhabteab Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Al2Klimov Jun 23, 2022

Choose a reason for hiding this comment

Al2Klimov Jun 23, 2022

Choose a reason for hiding this comment

yhabteab Jun 23, 2022

Choose a reason for hiding this comment

Al2Klimov Jun 23, 2022

Choose a reason for hiding this comment

julianbrost Oct 5, 2022

Choose a reason for hiding this comment

Al2Klimov left a comment

Choose a reason for hiding this comment

Riddle 1

Riddle 2

yhabteab commented Jul 22, 2022

Riddle 1

Riddle 2

Al2Klimov commented Jul 25, 2022

Al2Klimov commented Jul 25, 2022

Riddle 2

julianbrost left a comment

Choose a reason for hiding this comment

julianbrost Oct 5, 2022

Choose a reason for hiding this comment

julianbrost left a comment

Choose a reason for hiding this comment

julianbrost left a comment

Choose a reason for hiding this comment

yhabteab Jun 23, 2022 •

edited

Loading