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

Draft for review: script security #4693

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

StackScribe
Copy link
Contributor

@StackScribe StackScribe commented Nov 5, 2021

This is intended as a small document that just summarizes "best practices" about script security with links to other documents for details.

@daniel-beck @Wadeck @MarkEWaite This is my first stab at this. The odds of errors, omissions, or wrong style are pretty high so feel free to critique harshly.

Also, not sure how this happened but the system-administration/backing-up.adoc file has been included here. That was in #4668 which has been merged. I'll figure it out but, until I do, just ignore that part of this PR.

@StackScribe StackScribe requested review from a team as code owners November 5, 2021 02:49
@probot-autolabeler probot-autolabeler bot added the documentation Jenkins documentation, including user and developer docs, solution pages, etc. label Nov 5, 2021
@StackScribe StackScribe changed the title New page about script security Draft for review: script security Nov 5, 2021
Copy link
Contributor

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good start, thanks a lot for taking this on! ❤️

One thing to keep in mind with this chapter is that we have some advice for admins (basically all of the current content), and some other advice applies to script authors specifically.

It's kind of new to have user docs in the security chapter, unsure whether that's a separate page or should be combined here.

content/doc/book/security/_chapter.yml Outdated Show resolved Hide resolved
content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
when using the "Execute system Groovy script" step
* link:https://plugins.jenkins.io/email-ext/[Extended Email plugin]
* link:https://plugins.jenkins.io/groovy/[Job DSL plugin]
* link:https://www.jenkins.io/doc/book/pipeline/[Pipelines]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're fairly explicit in that Pipelines are not Groovy because of the missing support for some language features.

content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
Comment on lines 52 to 54
* Scripts written in languages other than Groovy can be run by an administrator
or must be approved by an administrator;
the sandbox is only for Groovy scripts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost never matters I think?

That seems to be it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, very uncommon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying lines 52-54 should be deleted?

content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
This is an alternative to the preceding bullet item.
I am guessing that the documenttion reflects the current reality.

* You should never disable the sandbox. If you disable the sandbox, a Scripted Pipeline (or a `script` step in a Declarative Pipelin) has unfettered access to Jenkins inernal objects.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normal users cannot do it; only admins. So the real advice is probably to teach users to deal with the restrictions of the sandbox, preferring Jenkins's REST API over internal (Java) APIs; minimize whole-script approvals, and instead use https://plugins.jenkins.io/workflow-cps-global-lib/ that are versioned and tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://plugins.jenkins.io/workflow-cps-global-lib/ seems to be deprecated. Is there a new version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 14 to 18
* link:/doc/book/managing/script-console/[Script Console]
* link:https://plugins.jenkins.io/groovy/[Groovy plugin]
when using the "Execute system Groovy script" step
* link:https://plugins.jenkins.io/email-ext/[Extended Email plugin]
* link:https://plugins.jenkins.io/groovy/[Job DSL plugin]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to structure this page more explicitly along the following lines to guide readers along a top-level view of the concepts involved?

  1. Scripting capabilities are very powerful, but are potentially dangerous if mishandled.
  2. The unrestricted Groovy-based Script Console is a powerful diagnostic tool and can only be used by admins. It has full access to all Jenkins internals and no restrictions.
  3. Many plugins offer scripting capabilities (this list) to users without Overall/Administer permission, and they integrate with Script Security plugin to do this more safely.
  4. Script Security plugin has a sandbox for Groovy scripts and Pipelines, and whole-script approval for all languages it supports.
  5. The sandbox allows regular users to implement scripts safely, restricting access to potentially unsafe code based on an allowlist (default + admin customizable, but that is very complicated)
  6. Whole-script approval lets users ask admins to approve entire scripts. This is generally discouraged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! As you may have guessed, I did not understand this well but this clarifies things a lot! I will rewrite.

Copy link
Contributor

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be inclined to leave the detailed technical documentation to the plugin, and limit discussion here to simple advice as @daniel-beck already began to suggest: leave the sandbox enabled; do not ever go to the script approval screen unless you absolutely know what you are doing (and if you are reading about this for the first time on jenkins.io, you do not know what you are doing); if you are very confident that you have discovered a new signature that is genuinely useful and harmless, file a PR to add it to the default whitelist and let a developer decide whether you are right or not (in the meantime step back and try to accomplish whatever you think you needed without in-process scripting).

content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
Comment on lines 42 to 43
In most cases, you should use the
link:http://localhost:4242/doc/book/managing/script-approval/#approve-assuming-permissions-check[Approve assuming permissions check] option rather than the simple Approve option.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just no.

Suggested change
In most cases, you should use the
link:http://localhost:4242/doc/book/managing/script-approval/#approve-assuming-permissions-check[Approve assuming permissions check] option rather than the simple Approve option.

Anyway this (very dubious) feature applies to the sandbox, not whole-script approval, so it is not even in the right bullet point.

Comment on lines 52 to 54
* Scripts written in languages other than Groovy can be run by an administrator
or must be approved by an administrator;
the sandbox is only for Groovy scripts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, very uncommon.

content/doc/book/security/scripted.adoc Outdated Show resolved Hide resolved
Comment on lines 56 to 57
* Users can disable the Groovy sandbox.
the entire script must be approved by the administrator unless it is in the list of administrator-managed list of approved scripts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just duplicates the first bullet point?

Suggested change
* Users can disable the Groovy sandbox.
the entire script must be approved by the administrator unless it is in the list of administrator-managed list of approved scripts.

Meg McRoberts and others added 8 commits January 11, 2022 00:26
Co-authored-by: Daniel Beck <[email protected]>
Comment from dbeck and jglick

Co-authored-by: Jesse Glick <[email protected]>
Co-authored-by: Jesse Glick <[email protected]>
Co-authored-by: Jesse Glick <[email protected]>
Co-authored-by: Jesse Glick <[email protected]>
Co-authored-by: Daniel Beck <[email protected]>
Co-authored-by: Jesse Glick <[email protected]>
Meg McRoberts and others added 2 commits January 11, 2022 00:47
Co-authored-by: Wadeck Follonier <[email protected]>
@@ -64,6 +64,7 @@ Use link:https://man7.org/linux/man-pages/man8/cron.8.html[cron]
to schedule when the backup script runs.

The shell script should create a directory such as `/mnt/backup`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@@ -186,6 +187,7 @@ Now execute the restored Jenkins instance:

[source,bash]
----

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Comment on lines 40 to 41
In most cases, you should use the
link:/doc/book/managing/script-approval/#approve-assuming-permissions-check[Approve assuming permissions check] option rather than the simple Approve option.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In most cases, you should use the
link:/doc/book/managing/script-approval/#approve-assuming-permissions-check[Approve assuming permissions check] option rather than the simple Approve option.

No this is not true at all and in fact that option should rarely if ever be used.

Anyway this option pertains to sandbox signatures, unrelated to whole-script approval which this bullet point is about.

Comment on lines 54 to 55
* Users can disable the Groovy sandbox.
The entire script must be approved by the administrator unless it is in the list of administrator-managed list of approved scripts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just part of the first bullet point?

@StackScribe
Copy link
Contributor Author

Question posed to gitter on 15 Feb 2022, should be answered somewhere in conjunction with this:

hello i'm trying to figure out how the script approval thingy works, we've got a file which contains both approvedScriptHashes and approvedSignatures
approvedScriptHashes I assume are whole .groovy scripts (well, looking at the code it's a bit more complicated than that, there's "language:" prepended to it before hashing)
approvedSignatures looks like methods which are explicitely allowed
question is, if Jenkins calls an "unknown" .groovy script from a pipeline and its hash is not in approvedScriptHashes, will it run line by line as long as all methods are explicitly allowed?

@jglick
Copy link
Contributor

jglick commented Feb 18, 2022

if Jenkins calls an "unknown" .groovy script from a pipeline

Like a load step? From a sandboxed pipeline? If so then

will it run line by line as long as all methods are explicitly allowed?

yes. If not, then it will just be run (no hash check). No admin should ever approve a script which delegates to some other unspecified script. That would be like signing a blank check and taping it to your front door.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Jenkins documentation, including user and developer docs, solution pages, etc. stalled Pull requests that are not progressing work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants