Skip to content
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

Odd script config behavior #7088

Closed
mal opened this issue Jul 30, 2014 · 8 comments · Fixed by #7250
Closed

Odd script config behavior #7088

mal opened this issue Jul 30, 2014 · 8 comments · Fixed by #7250
Assignees
Labels
>docs General docs changes

Comments

@mal
Copy link

mal commented Jul 30, 2014

Just upgraded from 1.2.1 with the groovy plugin to 1.3.1 (plugin removed) and I'm seeing some weird behaviour when trying to use my installed scripts, I've written a test script here.

Settings

script.disable_dynamic: true

Query

{
  "script_fields": {
    "minimum": {
      "lang": "groovy",
      "script": "lowest",
      "params": {
        "field": "vals"
      }
    }
  }
}

Script

GroovyCollections.min(doc[field].values as Integer[])

Results

ScriptException[dynamic scripting for [groovy] disabled]

Which, while true, shouldn't apply since I'm trying to run a script stored on disk ...

@dakrone
Copy link
Member

dakrone commented Jul 30, 2014

Hi @mal,

I'm trying to reproduce this issue with your example, I actually get a different exception due to GroovyCollections not being allowed by the groovy sandbox. Is it possible that you are running mixed versions of Elasticsearch in your environment?

Can you give me the full stacktrace of the exception?

Also, when you put the script in config/scripts, do you see the log message about the script being compiled by Elasticsearch?

@mal
Copy link
Author

mal commented Jul 30, 2014

Aha, it fails to compile so I guess it doesn't get found and ES assumes lowest is a dynamic script.

[2014-07-30 11:36:37,180][WARN ][script                   ] [Honcho] failed to load/compile script [lowest]
org.elasticsearch.script.groovy.GroovyScriptCompilationException: MultipleCompilationErrorsException[startup failed:
General error during canonicalization: Method calls not allowed on [groovy.util.GroovyCollections]

I tried adding groovy.util.GroovyCollections to the receiver_whitelist, but that didn't work either:

[2014-07-30 11:39:53,907][WARN ][script                   ] [Heart Attack] failed to load/compile script [lowest]
org.elasticsearch.script.groovy.GroovyScriptCompilationException: MultipleCompilationErrorsException[startup failed:
General error during canonicalization: Property access not allowed on [java.lang.Object]

@dakrone
Copy link
Member

dakrone commented Jul 30, 2014

I tried adding groovy.util.GroovyCollections to the receiver_whitelist, but that didn't work either:

How did you add it to the receiver_whitelist? I can try to reproduce it.

Also, going forward, looking at the javadoc I don't see any reason why GroovyCollections shouldn't be part of the default whitelist, so I will open a separate issue to add that.

@mal
Copy link
Author

mal commented Jul 30, 2014

I added the following line to elasticsearch.yml:

script.groovy.sandbox.receiver_whitelist: groovy.util.GroovyCollections

@dakrone
Copy link
Member

dakrone commented Jul 30, 2014

@mal ahh okay, this is because the whitelist has to be entirely enumerated (it replaces the default whitelist, doesn't add to it), so adding that line removed the other whitelisted classes (like Object).

See: https://github.com/elasticsearch/elasticsearch/blob/b43b56a6a85f7dd131086fd83dc9267aecbbf0a3/src/main/java/org/elasticsearch/script/groovy/GroovySandboxExpressionChecker.java#L90-L111

I think this does need to be clarified in the docs, and it would be nice to have the functionality of both (replacing and adding to the whitelist).

@dakrone
Copy link
Member

dakrone commented Jul 30, 2014

Opened #7089 for adding the GroovyCollections class to the whitelist.

@mal
Copy link
Author

mal commented Jul 30, 2014

Ahh ok, is there any disadvantage to disabling the sandbox given dynamic scripting is already off and that an attacker would already need root access to edit installed scripts? If not I'll probably just do that for now.

Thanks for your help!

@dakrone
Copy link
Member

dakrone commented Jul 30, 2014

Ahh ok, is there any disadvantage to disabling the sandbox given dynamic scripting is already off and that an attacker would already need root access to edit installed scripts? If not I'll probably just do that for now.

Yea, you can do this, disable the sandbox and since dynamic scripting is disabled by default, you'll just have to put it on disk.

I'll treat this issue as an issue to update the docs, thanks for the report!

@dakrone dakrone added the doc label Aug 1, 2014
dakrone added a commit to dakrone/elasticsearch that referenced this issue Aug 13, 2014
Also clarify in the docs that changing the whitelist/blacklist settings
replace the list, they don't add to it.

Fixes elastic#7089
Fixes elastic#7088
dakrone added a commit that referenced this issue Aug 13, 2014
Also clarify in the docs that changing the whitelist/blacklist settings
replace the list, they don't add to it.

Fixes #7089
Fixes #7088
dakrone added a commit that referenced this issue Sep 8, 2014
Also clarify in the docs that changing the whitelist/blacklist settings
replace the list, they don't add to it.

Fixes #7089
Fixes #7088
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants