Security permissions for Groovy JsonSlurper

[jasontedor]

This commit adds the necessary class permissions and property
permissions for basic Groovy JsonSlurper functionality.

Closes #14787

[rmuir]

why is json parsing necessary in scripts? I don't think we need to support this. Also this kind of serialization/deserialization tends to rely on suppressAccessChecks.

I really do not think we should do this. Our scripts do not need to be parsing json.

[rmuir]

Separately, this PR is broken 80 ways from sunday.

Lets try to enumerate everything wrong with it:

• adding a third party dependency to parse json in scripts
• wrong NOTICE.txt/licensing data for said third party dependency.
• wrong thirdPartyExcludes: don't document these classes as missing, they arent missing, they use sun.misc.Unsafe!!!!

But again, we really should not make this change. Totally the wrong direction. No need for anybody to be serializing/deserializing json in scripts. Bringing in additional third party dependencies to do that, that's pure insanity.

[jasontedor]

adding a third party dependency to parse json in scripts

This dependency already exists in the 2.x line and I'm not sure if the fact that it was removed received any attention and certainly not the attention that it deserves. It did not even make the breaking changes doc. And I'm not adding groovy-all (which is what is in 2.2) back, just groovy-json. Please do not exaggerate the issue.

wrong NOTICE.txt/licensing data for said third party dependency.

Can you please explain to me why it's wrong?

wrong thirdPartyExcludes: don't document these classes as missing, they arent missing, they use sun.misc.Unsafe!!!!

This is not grounded by the facts. In particular, the use of unsafe is disabled by default, it is only enabled if some system properties are intentionally set to true. The user has to go out of their way to enable these system properties.

No need for anybody to be serializing/deserializing json in scripts.

Clearly there are users that think there is a need or there wouldn't be issues on GitHub and posts on Discourse about how it's broken in 2.2 right now.

Bringing in additional third party dependencies to do that, that's pure insanity.

I think that you're mischaracterizing this. Again, it's a dependency that was removed without broad (any?) awareness.

[rmuir]

I'm not mischaracterizing anything. This is a scripting engine, not a full blown programming language. What is the need to serialize/deserialize json?

I'm strongly opposed to this change.

[rmuir]

Clearly there are users that think there is a need or there wouldn't be issues on GitHub and posts on Discourse about how it's broken in 2.2 right now

Its one person basically. Its important to be able to distinguish non-usecases and esoteria from real use cases.

[jasontedor]

wrong NOTICE.txt/licensing data for said third party dependency.

Can you please explain to me why it's wrong?

I see why they are wrong, but note that the license and notice files are also wrong for the groovy artifact and have been since these license and notice files were committed in 180ab24. I've pushed 76e8a11 to fix this for all of these files.

[fs-chris]

Just a few comments from the one esoteric guy (there is at least one more: #14787):

In my case, I decide in the Groovy update-script whether to process the JSON document or process another argument. Only in the first case - which is very rare - the document needs to be parsed. Decision is based on the existing document being updated.

Besides that,

• Why is it ok to pass a JSON string to an indexing request (Java-API) but esoteric to pass that same string to a Groovy script in an update request?
• Why is there the documented option of a java.policy file if it's not meant to be used?
• Why are entries added to that policy file not working as expected?
• Why is there support for a fully featured scripting language like Groovy (and even the other supported languages) if only very simple things should be done there?

[rmuir]

There isn't support for stuff like this. Only just a few classes like java.lang.Integer.

Nobody needs to be parsing JSON here. Its not something we need to support, or add security holes or extra third party libraries for. I will do everything I can to prevent this.

[clintongormley]

I'm siding with Robert here. Parsing JSON in a script just seems wrong. eg we don't support loading python modules in the python scripting plugin. We're certainly not going to support such things in Painless. I'd say that scripts that need to do stuff like this should be implemented in Java rather than Groovy.

@fs-chris mentions in #14787 (comment) that:

We need to process several thousands of such requests per second. So performance is essential.

... which indicates that this should be implemented in Java as well.

[s1monw]

I also agree with @clintongormley - yet the question is if we allow this backwards break in a minor release. On my end that's a case by case decision and for this one I am on the pro-break end. Especially since afaik @bleskes and @kimchy discussed some simple workaround related to passing the json string as a parameter but I don't recall the details. maybe @bleskes can elaborate?

[bleskes]

some simple workaround related to passing the json string as a parameter but I don't recall the details

That was the option to parse the json client side and pass it as a map-of-maps parameter to the script. I mentioned it on the ticket

[jasontedor]

Superseded by #16858.

[fs-chris]

I see where this discussion leads to.
It's really annoying that a solution working well with previous releases gets broken without need.
Now we will have to implement that as a plugin which needs to be installed and maintained on all the nodes in a cluster, or stick to an old Elastic version that supports the script.
This change comes along with so many other changes since 2.0 that makes life harder.

[jasontedor]

It's really annoying that a solution working well with previous releases gets broken without need.

@fs-chris I feel your pain, but this breaking change was not made without need (see #16858) and was clearly not made lightly.

[primshnick]

If someone is using the Java API and wants to pass in json as a parameter (e.g. for updating the value of a nested object), using JsonSlurper seemed to be the accepted solution. Is there a workaround for this?

[kimchy]

@PeteyPabPro see @bleskes note here: #14787 (comment), you can pass a "map of maps" or the actual params in the parameters to the script, so you won't need to pass in JSON as a value within JSON.

[jasontedor]

@PeteyPabPro: Have you tried the previous suggestion of parsing the JSON client side and passing it to the script as a map of maps?

[primshnick]

@kimchy @jasontedor Thanks for the suggestion, let me try that.

[jasontedor]

Thanks for the suggestion, let me try that.

Thanks @PeteyPabPro; please let us know how it turns out?

[primshnick]

@jasontedor Worked perfectly, thanks.

[jasontedor]

Worked perfectly, thanks.

@PeteyPabPro: Outstanding, thanks for reporting back! This means that we can consider this a viable replacement for the previous functionality.

[disbrain]

Excuse me, but this basically means that modifying nested data in place (without previously loading the content into the client) is no longer possible? Previously, always talking about nested data, with a script I could alter the content of a nested record and reassign it without knowing what was the whole content (an array of nested objects for instance), now seems that since the payload has to be supplied as a param to the script I have to load the whole array into the client and reassign it whole through the map trick