use current frame scope for permission filter function calls #7155
Conversation
|
Possibly the Frames for Permission/Filter evaluation could be created with allocLocals set to false then? As far as I can see the empty Locals Dictionary is not used anywhere. (Just to avoid to create unnecessary Dictionary objects..) |
…missions filter and as far as I can see also not for query filters
There was a problem hiding this comment.
Test protocol
Environment:
- a fresh Icinga 2
- an API user ("notroot") with the permission
objects/query/Hostand filter{{ host }}
Before:
A query for Hosts with filter host or with no filter returns no objects to user notroot.
After:
Both queries return all objects.
Unchanged:
Both the query filter var%20x%20%3D%201%0Ax and the permission filter function() { var x = 1; return x && host } don't work as assignments aren't allowed in sandbox mode – so yes, we don't need locals here at all.
|
I think technically the functions would be able to use locals as every function-wrapper creates a locals dict in the frame dedicated to the function-run on each execution of function anyway (within function wrapper encapsulated in a Callback / std::function). The permissionFrame (or the frame passed to the DoEvaluate) is anyway not the frame the function Executes in. There is one more frame created in VMOps::FunctionCall -> Function::InvokeThis for calling the call Method/Function of Function (having no Locals and the Function Object as Self) and within call-method-function the Function to execute itself is taken from that frame and again Function::InvokeThis is called, again creating a ScriptFrame (having no Locals but setting Self to the Value of Arg0 wich is the one we specify in FilterUtility::CheckPermission). The Locals are then populated by the wrapper around the parsed function (which initializes the Locals with ClosedVars and Arguments for each execution). The thing that locals assignments are never allowed in sanboxed frames is more a shortcoming or a bug instead of expected behaviour from my point of view. (It wouldn't change any persistent state if the assignment affects only locals as the locals are thrown away at end of function execution...) |
|
Interesting, we had the same ideas on Tuesday (with the holidays up until now in between). I wouldn't bother with changing the scoping access, or behaviour. Things will break, and I expected this with the change of the Self attribute in #6874 to be honest (which is one reason to leave the ticket open up until @marcofl and others had tested it). Such scopes and frames are used inside the apply rules too, rendering breaking changes more visible. Script debugger for permission filtersIn terms of this problem, here's one hint for debugging/developing - you can attach to a thrown filter expression from inside a permission filter with running the daemon in foreground with There, you can print things like It isn't gdb in there, but allows to print things in a readable manner before deep-diving into the code analysis. Use the following configuration: And fire an API query for this user. Change back to the icinga2 daemon terminal. Now, do some inspectation. |
|
Ah interesting, now I known what the script debugging mode means. But for cases like this there is little information to get there. Just that it is not there (what can also be seen in the error message). |
|
I just thought I'd share it with you, since you're doing a marvelous job with fixing problems in Icinga :) It is true, this mainly is for developers while doing runtime tests with the API. The real true benefit for users is to use this within |
dnsmichi
added
bug
|
In terms of the expression language on its own - that's somewhat black magic written by Gunnar, now maintained by us. I am happy to share everything I know, and discuss it with you and others. The most problematic part here is that you likely need to take pen and paper and draw lots of things (which I am doing with @Al2Klimov and @htriem in our office). My plan during the trainee years of @htriem is also to provide deep technical insights into the config compiler and its expression language thus explaining the DSL on a technical level. And also to tackle ideas like changing the AST into a more compelling faster mode. Maybe this is a good thing for later this year once the major stability problems are fixed, who knows. Maybe we'll meet at OSMC or an Icinga Camp in the future, I'd like to thank you in person for all the effort you are putting here 😘 In terms of your patch, here's my test results. TestsMinimal test config for lldb/gdb for setting a break point inside the filter evaluation. QueryFilter debuggerChange the permissions to something like this. Run Fire the query again. Change back to Icinga running in foreground. My main concern was ConclusionWorks like a charm, many thanks. Would it be ok for you being mentioned in the release changelog with your full name/company? |
|
There was really once a situation where we could have needed the I think the AST is quite fast already (most time is accounted for transferring). I don't have an issue with being mentioned in changelog. And will ask my boss for participation in OSMC or Icinga Camp as I think that's something the company could pay for. |
proposed fix for follow-up issue/regression after implementing #7113 for #6874
should be reviewed by someone having the big-picture of the code