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

Security permissions for Groovy closures #16196

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@jasontedor
Copy link
Member

commented Jan 23, 2016

The purpose behind this pull request is to add some permissions that Groovy needs to use closures in its current state. While granting java.lang.reflect.ReflectPermission "supressAccessChecks" is undesirable, there is an upstream pull request in apache/groovy#248 to modify the Groovy compiler so that these permissions are not necessary. Therefore, the long-term plan is to grant these permissions so as to not break existing scripts, and remove these permissions when the upstream pull request is accepted (and make a breaking change in a major release if not). These permissions are not staying around forever.

Closes #16194

Security permissions for Groovy closures
This commit adds some permissions that Groovy needs to use closures.
@@ -25,6 +25,7 @@ grant {
// needed by groovy engine
permission java.lang.RuntimePermission "accessDeclaredMembers";
permission java.lang.RuntimePermission "accessClassInPackage.sun.reflect";
permission java.lang.reflect.ReflectPermission "suppressAccessChecks";

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 23, 2016

Member

@rmuir should look at this...there was an enormous amount of work that went into removing this permission across elasticsearch.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2016

There is also a huge historical security risk. I don't think we should do this at all: closures aren't necessary for the kind of scripting happening here. This scripting is deadly and I don't think we should add even one single permission beyond what is absolutely required.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2016

Just to put another way, to justify my -1 technically:

Closures have not been supported for months in master or 2.2: nobody has noticed. There are hundreds of tests that use and abuse groovy, not a single one uses a closure. This speaks volumes: they simply aren't necessary.

The suppressAccessChecks is over the top here, it has wide implications. Taking on this enormous risk just to add some unnecessary syntactic sugar is the wrong tradeoff.

Groovy closure integration test to unit test
This commit converts an integration test for Groovy closures to a unit
test.
@nik9000

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2016

If we can't get closures without a suppressacesschecks can we throw a
meaningful error message?

Can we make a test with a complex groovy script that we can point people
that want closures to to say "sorry, we can't support closures because
groovy needs dangerous permissions for them to work but if you structure
your script like this it'll work."

Is that good enough given painless's progress?
On Jan 23, 2016 1:38 AM, "Robert Muir" notifications@github.com wrote:

Just to put another way, to justify my -1 technically:

Closures have not been supported for months in master or 2.2: nobody
has noticed. There are hundreds of tests that use and abuse groovy, not
a single one uses a closure. This speaks volumes: they simply aren't
necessary.

The suppressAccessChecks is over the top here, it has wide implications.
Taking on this enormous risk just to add some unnecessary syntactic sugar
is the wrong tradeoff.


Reply to this email directly or view it on GitHub
#16196 (comment)
.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2016

We discussed this after @ywelsch dug into exactly why groovy is in this situation. He has a potential patch to improve the situation in groovy.

Today we have no way to separate "basic scripting users" from "advanced users" (people who choose to use groovy for a reason, e.g. because they like features of it). The two different use cases will cause tension for a while.

But for now, we can optimistically commit this, with the intent we will "remove" it again one way or another: best case, our fix to groovy gets accepted, we upgrade, and then its gone. worst case, it takes longer, until groovy becomes "opt-in" as a plugin or whatever (and we have a different default language).

I do worry about subsequent commits that will enable more "long-tail" features of groovy, e.g. @Grab or similar, in the meantime we need to be careful and judicious to avoid relying on this permission as its really no good at all. And we should expect plenty of "long-tail" feature issues just like this to be opened after 2.2.

Additional security permission for Groovy closures
This commit adds an additional security permission for Groovy closures
and a corresponding test.
@ywelsch

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2016

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2016

Integrated via bdddea2.

@jasontedor jasontedor deleted the jasontedor:groovy-closure-permissions branch Jan 26, 2016

@jasontedor jasontedor removed the review label Jan 26, 2016

jasontedor added a commit that referenced this pull request Jan 26, 2016

Security permissions for Groovy closures
This commit backports commit bdddea2
from master to 2.x.

Relates #16196

jasontedor added a commit that referenced this pull request Jan 26, 2016

Security permissions for Groovy closures
This commit backports commit bdddea2
from master to 2.2.

Relates #16196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.