Make Scriptler dependency optional

[daniel-beck]

Other than the 'Scriptler mode', the plugin currently looks reasonable from a security POV, as its embedded scripting uses Script Security.

I haven't done a complete check whether this would be enough to restore distribution (there may be other security issues lurking), but I'd be happy to do that if this change is (tentatively) accepted.

@reviewbybees

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[oleg-nenashev]

LGTM 🐝 . I am not sure if things like PCT/ATH will be able to start in the case of the dependency blacklisted in UC, but we can figure out it later

[kinow]

Thanks for looking into this issue Daniel!

Do you know what will happen to users with jobs using the latest version and scriptler params? Are their scripts going to disappear after installing this version?

cc @imoutsatsos

We discussed this option some time ago and decided it would be better to release a version of the plugin with scriptler as optional only after fixing the security issue in scriptler.

Then users would have time to migrate their scripts. I tried joining the team to work with the scriptler security issue but gave up.

So if there is no impact to users I'll be +1 too. Otherwise will defer merging until there is a release of scriptler with security fixes.

Any news about Scriptler security issues?

Cheers
Bruno

[daniel-beck]

Do you know what will happen to users with jobs using the latest version and scriptler params? Are their scripts going to disappear after installing this version?

• New installations of uno-choice:
  • If Scriptler was previous installed: Full functionality available. No changes.
  • If Scriptler is not already installed: It won't be installed, and inline scripts still work (just upload this plugin to a pristine instance and see for yourself, just one radio button left). If they later install Scriptler, a restart of Jenkins will make "Scriptler mode" available.
• Users updating uno-choice to a release containing this change:
  • Scriptler was previously installed and will remain installed, and uno-choice will offer the "Scriptler mode" to users. Their existing config data remains. If they don't want to use Scriptler (e.g. after migrating from "Scriptler mode" to inline scripts), they can touch plugins/scriptler.jpi.disabled or similar to get rid of Scriptler after a restart, and uno-choice will continue to work.

Except for JENKINS-33843 (which affects all installed, optional dependencies) this is a solid solution. The only drawback is some (minor) maintenance burden ensuring the plugin works correctly without Scriptler installed, as hpi:run will install optional dependencies.

Any news about Scriptler security issues?

Solution still TBD. I recently assisted Domi with fixing config-file-provider, but haven't gotten around to Scriptler yet. WIP PR at jenkinsci/scriptler-plugin#29, don't know how good it is.

[kinow]

Thanks for the thorough explanation Daniel! Sunds solid indeed! 👏

@imoutsatsos has the largest installation that I am aware with the plug-in. Ioannis, would you have time to give it a try? We can plan a hangout just for testing it, and preparing a new release, once you return from your trip.

I am going to Australia soon, so first I will review it this week or next week, and we can revert in case we find any issues (though I doubt there will be any!).

And thanks a lot for finding this neat solution Daniel!

Cheers
Bruno

[daniel-beck]

The only drawback is some (minor) maintenance burden ensuring the plugin works correctly without Scriptler installed, as hpi:run will install optional dependencies.

FWIW for interactive testing, you can always touch work/plugins/scriptler.jpi.disabled (or rm to re-enable it). I don't know whether there's a convenient solution for automated testing.

[rsandell]

I don't know whether there's a convenient solution for automated testing.

Maybe a @LocalData containing plugins/scriptler.jpi.disabled could work

[imoutsatsos]

Back in the office the week of September 17th and I'm anxious to give it a try. So thankful for the assistance of you all! Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: Bruno P. Kinoshita <[email protected]> Sent: Tuesday, September 12, 2017 12:19:16 AM To: jenkinsci/active-choices-plugin Cc: Ioannis K. Moutsatsos; Mention Subject: Re: [jenkinsci/active-choices-plugin] Make Scriptler dependency optional (#15) Thanks for the thorough explanation Daniel! Sunds solid indeed! 👏 @imoutsatsos<https://github.com/imoutsatsos> has the largest installation that I am aware with the plug-in. Ioannis, would you have time to give it a try? We can plan a hangout just for testing it, and preparing a new release, once you return from your trip. I am going to Australia soon, so first I will review it this week or next week, and we can revert in case we find any issues (though I doubt there will be any!). And thanks a lot for finding this neat solution Daniel! Cheers Bruno — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#15 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ADQBYvkQnpf_6-S2w6VDpEZCbv6G081Sks5shaPUgaJpZM4PTbMY>.

[kinow]

Sorry, didn't have time to review & test. @imoutsatsos , would you be able to give it some testing? Basically:

• git clone ... && cd active-choices-plugin
• mvn hpi:run
• create a simple project with scriptler and active-choices
• stop maven
• switch branch to this pull request (e.g. https://gist.github.com/piscisaureus/3342247)
• run mvn package & copy hpi somewhere else
• switch back to master
• run mvn hpi:run again
• install hpi manually
• confirm your scriptler scripts are still there

And then test the other scenario, starting from the branch, installing the same hpi in a fresh instance, and confirming it doesn't pull the scriptler dependency.

With that, I'd say we should be ready to merge, and prepare a release ASAP to white-list active-choices plug-in again.

Or I'll be back on October 9th, and can review and test it throughout that week, aiming at a release in the weekend of October 14/15.

Cheers
Bruno

[imoutsatsos]

@kinow @daniel-beck Thanks for the update and feedback.
I have tested the PR in my installation and looks very good! Although originally I missed Daniel's point about how to test with Scriptler disabled I managed to test successfully the following scenarios:

Start with a Jenkins installation that does not have Scriptler.
Active Choices installed and functioned successfully. Only 'Groovy script' option displayed (as expected).
Deploy a Jenkins job that previously used a Scriplter parameter. The configuration did not display the Active choices parameters. This would probably fail

Then install Scriptler (uploading hpi)
Restart Jenkins server
Active choices functions properly and now both the 'Groovy Script' and 'Scriptler script' options are available

[daniel-beck]

Deploy a Jenkins job that previously used a Scriplter parameter. The configuration did not display the Active choices parameters. This would probably fail

Should show up as unloadable data in /oldDataMonitor (same as when disabling the Scriptler plugin from file system and restarting).

[mgaunin-cloudbees]

Hi, can we expect the merge and the release soon?

[kinow]

If you read the previous comments, you'll see I'm returning from a trip in 1 week (9th october). My plan was releasing as soon as I'm back and finished testing. Ioannis already tested it and sent me his feedback. Check with Daniel about another issue that may block the release. I believe he also works at CloudBees?

[mgaunin-cloudbees]

Thanks @kinow! I thought you were already back, sorry for that.

[kinow]

Merged. Thanks a lot!