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

Generate closures as public classes #248

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@ywelsch
Contributor

ywelsch commented Jan 25, 2016

Groovy is the default scripting language in Elasticsearch, used to evaluate custom expressions. In Elasticsearch 2.0 we introduced the Java security manager as a key component to secure our software. We spent a lot of effort on getting Groovy to run under the security manager with a conservative set of permissions. We have currently hit a road block with the suppressAccessCheck which allows suppressing the standard language access checks and expose information that would normally be unavailable to malicious code. Unfortunately, not providing this permission to Groovy breaks the use of Groovy closures in our scripts.

The reason is that in the Groovy framework:

  • Closures are generated as package private classes.
  • Access to closures is cached by the Groovy runtime using the class CachedMethod.

Invoking a closure thus fails without the suppressAccessCheck permission as the class of the closure is package private but accessed from the Groovy runtime class CachedMethod in the org.codehaus.groovy.reflection package.

This one-line PR changes the access modifier of the classes generated for Groovy closures from package-private to public.

@blackdrag

This comment has been minimized.

Show comment
Hide comment
@blackdrag

blackdrag Jan 25, 2016

Contributor

How does the situation with CachedMethod change if the generated closure is public?

Contributor

blackdrag commented Jan 25, 2016

How does the situation with CachedMethod change if the generated closure is public?

@pickypg

This comment has been minimized.

Show comment
Hide comment
@pickypg

pickypg Jan 25, 2016

Contributor

I went ahead and created a JIRA issue for it: GROOVY-7735.

How does the situation with CachedMethod change if the generated closure is public?

@blackdrag As the change increases the access level of the generated Closure class, it has no negative impact on the CachedMethod class. Given that that class' job is to invoke cached methods, it seems better to have the appropriate permission level for the Closure with respect to it "doing the right thing".

Contributor

pickypg commented Jan 25, 2016

I went ahead and created a JIRA issue for it: GROOVY-7735.

How does the situation with CachedMethod change if the generated closure is public?

@blackdrag As the change increases the access level of the generated Closure class, it has no negative impact on the CachedMethod class. Given that that class' job is to invoke cached methods, it seems better to have the appropriate permission level for the Closure with respect to it "doing the right thing".

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Jan 25, 2016

Member

How does the situation with CachedMethod change if the generated closure is public?

It also means code running under securitymanager can invoke it without granting ReflectPermission("suppressAccessChecks") which is dangerous.

Basically, groovy should respect java's access modifiers here.

Member

rmuir commented Jan 25, 2016

How does the situation with CachedMethod change if the generated closure is public?

It also means code running under securitymanager can invoke it without granting ReflectPermission("suppressAccessChecks") which is dangerous.

Basically, groovy should respect java's access modifiers here.

@blackdrag

This comment has been minimized.

Show comment
Hide comment
@blackdrag

blackdrag Jan 26, 2016

Contributor

The choice was not really arbitrary. Anonymous inner classes will also not be public in Java. But well... I think there will be no real ill side effects we have to fear here. So +1 from my side... Only a minor improvement could be done and that is either to comment in code or at least in the commit message as of why we use public. Otherwise it might be changed again in the future... And of course something like a test would be nice too, but the comment is more important to me

Contributor

blackdrag commented Jan 26, 2016

The choice was not really arbitrary. Anonymous inner classes will also not be public in Java. But well... I think there will be no real ill side effects we have to fear here. So +1 from my side... Only a minor improvement could be done and that is either to comment in code or at least in the commit message as of why we use public. Otherwise it might be changed again in the future... And of course something like a test would be nice too, but the comment is more important to me

GROOVY-7735: Generate closures as public classes
Access to closures is currently cached by the Groovy runtime in org.codehaus.groovy.reflection.CachedMethod.
If closures are generated as package-private classes, invoking the cached method from CachedMethod only works
if the suppressAccessChecks permission is granted to the security manager, a permission which is dangerous
as it completely disables standard language access checks.
@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Jan 26, 2016

Contributor

@blackdrag I've added a comment to the code and an explanation for the code change in the commit message. Please let me know if anything else is needed to get this in.

Contributor

ywelsch commented Jan 26, 2016

@blackdrag I've added a comment to the code and an explanation for the code change in the commit message. Please let me know if anything else is needed to get this in.

@pickypg

This comment has been minimized.

Show comment
Hide comment
@pickypg

pickypg Jan 26, 2016

Contributor

To add to it being public, I looked for other usage of getOrAddClosureClass and I only found two other instances, both of which also pass ACC_PUBLIC:

Perhaps a future PR should deprecate the method and remove the second argument, but I'm happy to see the +1 here. (PS: Jenkins failed to actually start Gradle, which is why the build failed)

Contributor

pickypg commented Jan 26, 2016

To add to it being public, I looked for other usage of getOrAddClosureClass and I only found two other instances, both of which also pass ACC_PUBLIC:

Perhaps a future PR should deprecate the method and remove the second argument, but I'm happy to see the +1 here. (PS: Jenkins failed to actually start Gradle, which is why the build failed)

@blackdrag

This comment has been minimized.

Show comment
Hide comment
@blackdrag

blackdrag Jan 26, 2016

Contributor

those other two findings are for Closures in annotations. They actually do need to be public. Anyway... +1 for the change

Contributor

blackdrag commented Jan 26, 2016

those other two findings are for Closures in annotations. They actually do need to be public. Anyway... +1 for the change

@paulk-asert

This comment has been minimized.

Show comment
Hide comment
@paulk-asert

paulk-asert Feb 3, 2016

Contributor

+1 from my side

Contributor

paulk-asert commented Feb 3, 2016

+1 from my side

@PascalSchumacher

This comment has been minimized.

Show comment
Hide comment
@PascalSchumacher

PascalSchumacher Feb 8, 2016

Contributor

Merged: 7ced8a2 Thanks!

Contributor

PascalSchumacher commented Feb 8, 2016

Merged: 7ced8a2 Thanks!

@PascalSchumacher

This comment has been minimized.

Show comment
Hide comment
@PascalSchumacher

PascalSchumacher Feb 8, 2016

Contributor

@ywelsch Could you please close the pull request? I can not because this is a read only-repo (and I forgot to add the right comment to the commit message :().

Thanks!

Contributor

PascalSchumacher commented Feb 8, 2016

@ywelsch Could you please close the pull request? I can not because this is a read only-repo (and I forgot to add the right comment to the commit message :().

Thanks!

@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Feb 9, 2016

Contributor

@PascalSchumacher thanks for merging this!

Contributor

ywelsch commented Feb 9, 2016

@PascalSchumacher thanks for merging this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment