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

Using Graal.js 19.0.0 via Scripting in platform/core.network #1092

Merged

Conversation

@JaroslavTulach
Copy link
Contributor

commented Jan 21, 2019

Since #1011 there is a generic way to request a ScriptEngine via Scripting API. This PR brings the benefits of that API abstractions to platform/core.network module while making sure the actual engine used for PAC resolution can be selected by the vendor of a NetBeans Platform based application and properly tested. This balances the plugability with the necessary safety needed while resolving PAC configurations.

The security is achieved by the ALLOWED_PAC_ENGINES branding API. Each vendor of a NetBeans Platform based application is encouraged to enumerate the one(s) engine that is considered trusted. By default we enlist Nashorn, Graal.js and GraalVM:js - all of them tested and to my best knowledge secure. I trust the GraalVM:js engine the most. Why list others? Because the GraalVM:js engine is only provided as an optional module - many NetBeans Platform applications won't have it - then it is natural to fallback to Nashorn (also considered secure at the moment).

Should NetBeans IDE re-brand the ALLOW_PAC_ENGINES value? Possible. NetBeans IDE is known to carry GraalVM:js implementation, so it could allow only its usage while resolving PAC. I haven't done so, but I have written a test in nb/ide.branding module to verify that GraalVM:js is really selected in the NetBeans IDE.

Why use Scripting.newBuilder().build() to create ScriptEngineManager at all? Because various vendors may have different opinion on which engine to use. @matthiasblaesing had a preference for Rhino. OracleLabs require GraalVM based implementation in VisualVM and co. The pluggable nature of the Scripting API allows anyone to hook the preferred engine of choice into own NetBeans Platform based application and then select it for PAC resolution using ALLOWED_PAC_ENGINES branding API.

To achieve the needed safety levels, this PR also upgrades from RC versions of GraalVM libraries to supported 19.0.0 release. Unlike RCs, this version is considered stable. I'd rather see next version of NetBeans using stable version of GraalVM libraries.

@jlahoda

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

@JaroslavTulach JaroslavTulach force-pushed the JaroslavTulach:CoreNetworkAndScriptingAPI branch from 09bdaca to 1894e0c Jan 21, 2019

@matthiasblaesing

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

@jlahoda if you think we have a security problem, please take it to private@netbeans.incubator.apache.org (and please CC @lbruun), there we can openly discuss whether or not there is one.

@jlahoda

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

@lbruun
Copy link
Contributor

left a comment

Overall it looks good to me.

Two comments:

  • Graal's readiness for prime time? With this change Graal now becomes the first choice for PAC Evaluator environment. PAC scripts are always pretty simple JS code so most JS implementations will handle PAC scripts well as long as they can handle simple JS constructs (nothing fancy, ECMAScript v3 compliance would probably be sufficient). So even if I've never tried out Graal myself I'm not worried here.
  • The code implicitly assumes that the ScriptEngine returned by the Scripting API is "secure by default". (except for Nashorn where an additional step is taken). Is this the case? Does the Scripting API guarantee it will return a sandboxed execution environment no matter the underlying implementation? (apologies, I haven't reviewed the Scripting API part).
@matthiasblaesing
Copy link
Contributor

left a comment

The changes look ok. I'm still not convinced though, that it is wise to use $arbitrary js engine in a security sensitive environment.

@JaroslavTulach

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

* Graal's readiness for prime time? 

I believe Graal.js is the best engine out there.

* The code implicitly assumes that the `ScriptEngine` returned by the Scripting API is "secure by default". (except for Nashorn where an additional step is taken). Is this the case? Does the Scripting API guarantee it will return a sandboxed execution environment no matter the underlying implementation?

It would be great if all the engines were "secure by default". The scripting API could ensure that - then the Nashorn additional step could be moved into the scripting API.

@JaroslavTulach

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

After five months I believe I am ready for integration. Please review (again).

  1. Meanwhile GraalVM managed to release official stable version - e.g. this PR updates to Graal.js 19.0.0
  2. the biggest complain I noticed in previous round of review: we don't want to have an open list of script engines to use for PAC evaluation - addressed by 7a90c43 and ALLOWED_PAC_ENGINES branding
  3. the PR contains a fix for networking issues during build - kind of unrelated, but I wasn't able to move on without it: a297bcf
  4. the rest of the PR is basically an attempt to make sure Scripting.newBuilder().build() provides consistent engines (Nashorn, Graal.js and GraalVM:js) secured by default
  5. profiler.oql is using Scripting as well, but it only runs trusted scripts and thus needs to run in allowAllAccess mode - hence the new API - Scripting.newBuilder().allowAllAccess(true).build()

I hope the review goes well. I'd really like to get this out of my table. I've been playing with JavaScript engines secure integration since December 2018 and it is time to move to something else.

@geertjanw

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Great work. What are some simpler and more complex scenarios we can use to try this out?

@JaroslavTulach JaroslavTulach changed the title Using Scripting API in platform/core.network Using Graal.js 19.0.0 in platform/core.network May 28, 2019

@sdedic

sdedic approved these changes May 29, 2019

@matthiasblaesing

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

I had a very short look at graal.js and it looked much better than Nashorn. At least it brought over the message, that someone cared about security and put thought into it.
I have some doubts, that the Scripting API module is helpful though. It implies that it can do things it won't be able to do. allowAllAccess will only work for Graal.js and nashorn engines. If other engines are used, the user will have a false sense of security, that is not in place. I see, that the ClassLoader magic is helpful here, but is that enough to make it worth it? The Java Scripting API feels ok for trusted scripts, but introduces some doubts when fine grained control over the engine is required.
I'm ok with merging this, but it was only a quick look (I explained on private@ why I won't go deeper).

@geertjanw

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Please do go deeper and please do share your complete feelings, we all need to know fully what the advantages and disadvantages of solutions are, and should wait until you're able to provide that, if you can indicate when/how. I.e., we need you, @matthiasblaesing!

@matthiasblaesing

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Sorry, I will close this matter for me now. Please keep it so.

@JaroslavTulach JaroslavTulach changed the title Using Graal.js 19.0.0 in platform/core.network Using Graal.js 19.0.0 via Scripting in platform/core.network May 30, 2019

@JaroslavTulach

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Thanks for your review @matthiasblaesing - I've just rewritten the initial comment in this PR to provide higher level overview of the current state of the PR. Hopefully it addresses some of your concerns.

I have some doubts, that the Scripting API module is helpful though.

Scripting API allows anyone to plug in their preferred engine of choice.

If other engines are used, the user will have a false sense of security, that is not in place.

The ALLOWED_PAC_ENGINES API allows vendor of a NetBeans Platform based application to restrict which engine is used for PAC resolution and thus guarantee the security is in place.

I see, that the ClassLoader magic is helpful here, but is that enough to make it worth it?

There is a lot of JavaScriptEngineTests that check and compare the behavior of all the known engines - Nashorn, Graal.js and GraalVM:js - all of them behave more or less identically and seem to be sufficiently secure. True, the security of Nashorn and Graal.js is achieved by special configuration of the engines via properties, reflection and ClassLoader magic. On the other hand GraalVM:js is configured via public API method calls only. As such I trust it the most and I made it the first choice for PAC resolution in the NetBeans IDE.

However I don't want to restrict freedom of NetBeans Platform vendors. They are encouraged to provide other engines and configure their security before enabling their usage in the ALLOWED_PAC_ENGINES branding API.

I'm ok with merging this

Great. I'll give you guys few more days and then finally close this eight months long endeavor of mine, hopefully.

@JaroslavTulach JaroslavTulach merged commit f6e165c into apache:master Jun 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JaroslavTulach JaroslavTulach deleted the JaroslavTulach:CoreNetworkAndScriptingAPI branch Jun 3, 2019

@geertjanw geertjanw added the NB11.1 label Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.