[JENKINS-41516] Add script console listener

[meiswjn]

See JENKINS-41516.

Proposed changelog entries

• Add listener for privileged groovy script execution (including script console and CLI).

Proposed upgrade guidelines

N/A

Submitter checklist

• [x] (If applicable) Jira issue is well described
• [x] Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• [x] Appropriate autotests or explanation to why this change has no tests
• [ ] New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
• [ ] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

Any

Maintainer checklist

Before the changes are marked as ready-for-merge:

• [ ] There are at least 2 approvals for the pull request and no outstanding requests for change
• [ ] Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• [ ] Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
• [ ] Proper changelog labels are set so that the changelog can be generated automatically
• [ ] If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[daniel-beck]

This should also cover GroovyCommand and GroovyShCommand (the latter may be especially awkward to log).

What's your plan for unsandboxed pipelines and other unsandboxed script execution involving script-security?

[meiswjn]

Good point, thanks.

About the other point: Do you mean unsandboxed pipelines that run potentially dangerous functions that have been put on the allowlist?

[daniel-beck]

Do you mean unsandboxed pipelines that run potentially dangerous functions that have been put on the allowlist?

No, users can choose to disable the sandbox for otherwise sandboxed scripts (requiring admin approval unless an admin configures the script and auto-approves), making the resulting features just as powerful/dangerous as the script console.

I'm not opposed to a change like this, but it should cover all the common scripting features in Jenkins.

[meiswjn]

Do you mean unsandboxed pipelines that run potentially dangerous functions that have been put on the allowlist?

No, users can choose to disable the sandbox for otherwise sandboxed scripts (requiring admin approval unless an admin configures the script and auto-approves), making the resulting features just as powerful/dangerous as the script console.

I'm not opposed to a change like this, but it should cover all the common scripting features in Jenkins.

That's a good question that I must think about. Gotta take a look at the script-security plugin, then I may be able to come up with a technical solution. Any hints are welcome, of course.

[daniel-beck]

FTR script-security is its own thing, but it would be beneficial to have a core PR and corresponding plugin PR prepared, because only one of those would only partially address the problem.

[basil]

Our general policy is to review proposed API additions along with the proposed consumers. This can help us better understand the intended use case in order to properly review the API and ensure the API can accommodate both present and future use cases. Is there a PR demonstrating the usage of this proposed API addition?

[meiswjn]

Our general policy is to review proposed API additions along with the proposed consumers. This can help us better understand the intended use case in order to properly review the API and ensure the API can accommodate both present and future use cases. Is there a PR demonstrating the usage of this proposed API addition?

Hi @basil, Sure. Here is my planned usecase: https://github.com/jenkinsci/audit-trail-plugin/pull/72

[meiswjn]

Hey @basil, Please let me know if this PR needs further justification. Thanks!

[meiswjn]

Thanks for your review, @res0nance.

Very good point! My idea was to get the user in the listener implementation. It looked like the User object was still accessible there but I do not know a lot about how the user object is implemented.

[meiswjn]

Hi, I will be very busy for the next two weeks and probably not able to work on this PR during that time. Please feel free to continue reviewing but I may be slow to respond. Especially, I would like to test @res0nance suggestion. Thanks for your understanding.

[meiswjn]

Hey @res0nance, sorry for the delay. I implemented the requested change. Thanks for the hint!

PR is ready for review again.

[meiswjn]

@mawinter69 @basil Do you want to review this PR or should I tag the reviewer team?

[meiswjn]

Is this PR ready for merge? :) If anything else should be done on my side, please let me know.

[timja]

Please add a changelog entry

[meiswjn]

Please add a changelog entry

Done

[daniel-beck]

I know I've been absent from this PR for a bit, but would like an opportunity to review it. I do not expect that'll delay it for more than a week. Thanks :)

[meiswjn]

I know I've been absent from this PR for a bit, but would like an opportunity to review it. I do not expect that'll delay it for more than a week. Thanks :)

Sure. I appreciate your input.

[meiswjn]

Not manually tested, so no approval from my side, just comment.

No tests included as no implementation of listener. Must be tested in plugin.

BTW nothing prevents you to implement a dummy listener just for test purpose :) Having no tests means that the likelihood a regression will be introduced is increased. The last comment from Daniel also shows a non-working feature from this PR, which could have been caught potentially by the tests.

Hi @Wadeck, Thanks for your feedback. I had tests implemented in the downstream, but for the CLI the tests only covered GroovyCommand, not GroovyShCommand. I fixed it and implemented the tests in this PR as well.

[meiswjn]

Hey @daniel-beck, Did you find any time yet to review the changes? Thank you!

[daniel-beck]

@meiswjn I'll do another iteration in the next few days.

[dwnusbaum]

I'm not opposed to a change like this, but it should cover all the common scripting features in Jenkins.

For what it's worth, I think that the usage pattern for ScriptApproval is different enough that it might not make sense to include it in this API. ScriptApproval is intended for persistent scripts rather than ephemeral ones, which already makes it more auditable via things like jobConfigHistory, it is exposed to unprivileged users in regular features via classes like SecureGroovyScript, and it makes a distinction between script approval and script execution. A listener that fires whenever an admin runs an ad-hoc Groovy script in the script console or via the CLI should fire relatively rarely and only have "interesting" events, whereas firing an event for every execution of aSecureGroovyScript seems unhelpful. Maybe it would make sense to only fire an event when ScriptApproval.approveScript is called, but either way I think that ScriptApproval is quite different from the script console, and something like https://github.com/jenkinsci/script-security-plugin/pull/300 would make more sense for auditing its usage.

[daniel-beck]

firing an event for every execution of aSecureGroovyScript seems unhelpful. Maybe it would make sense to only fire an event when ScriptApproval.approveScript is called

FWIW I had similar thoughts in https://github.com/jenkinsci/script-security-plugin/pull/416#pullrequestreview-1056562733

[daniel-beck]

I added a basic logging listener to core in this PR, I hope that's OK with you.

[daniel-beck]

Some notes while I'm still playing with this (not immediately actionable I think):

• Would probably be better if "origin" was an object; could pass the Run directly rather than a string representation.
• Ideally this would also record the script binding. Depending on the script, a lot can be done with the variables passed via binding, and not having a chance to log those seems like a gap.

[meiswjn]

I'm not opposed to a change like this, but it should cover all the common scripting features in Jenkins.

For what it's worth, I think that the usage pattern for ScriptApproval is different enough that it might not make sense to include it in this API. ScriptApproval is intended for persistent scripts rather than ephemeral ones, which already makes it more auditable via things like jobConfigHistory, it is exposed to unprivileged users in regular features via classes like SecureGroovyScript, and it makes a distinction between script approval and script execution. A listener that fires whenever an admin runs an ad-hoc Groovy script in the script console or via the CLI should fire relatively rarely and only have "interesting" events, whereas firing an event for every execution of aSecureGroovyScript seems unhelpful. Maybe it would make sense to only fire an event when ScriptApproval.approveScript is called, but either way I think that ScriptApproval is quite different from the script console, and something like jenkinsci/script-security-plugin#300 would make more sense for auditing its usage.

Thanks for your feedback! My goal was to track every usage of privileged script execution - even by admins and approved scripts. For example, I do want to know when anyone uses hudson.util.Secret.decrypt and not just when it is approved for the first time. Having audit logs for potentially uninteresting events is not a problem from my perspective.

[daniel-beck]

Some more notes while I'm digging into this further:

• Groovy hook scripts should be logged (how to do that for boot failure?).
• We probably want to log the result and/or output of the script evaluation, where applicable.

---

Intermediate state at the moment with basic scripting in core (groovy CLI, groovysh CLI, hook scripts, script console)

Sep 01, 2022 9:54:42 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'System.out.println "init.groovy"
' with binding: 'null' with usage: 'EXECUTION' in context: '/Users/danielbeck/Repositories/github.com/daniel-beck/jenkins/war/work/init.groovy' with description: 'jenkins.util.groovy.GroovyHookScript:init' by user: 'SYSTEM'
Sep 01, 2022 9:54:42 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'System.out.println "10-init"
' with binding: 'null' with usage: 'EXECUTION' in context: '/Users/danielbeck/Repositories/github.com/daniel-beck/jenkins/war/work/init.groovy.d/10-init.groovy' with description: 'jenkins.util.groovy.GroovyHookScript:init' by user: 'SYSTEM'
Sep 01, 2022 9:57:10 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'println "lol"' with binding: '[]' with usage: 'EXECUTION' in context: 'class hudson.util.RemotingDiagnostics' with description: 'hudson.remoting.LocalChannel@720a1770' by user: 'null'
Sep 01, 2022 10:03:19 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'println jenkins.model.Jenkins.get()
' with binding: '[out:java.io.PrintWriter@770dc444,stdin:java.io.BufferedInputStream@34001a46,stdout:java.io.PrintStream@79044ff6,stderr:java.io.PrintStream@2b482f64]' with usage: 'EXECUTION' in context: 'class hudson.cli.GroovyCommand' with description: 'null' by user: 'admin'
Sep 01, 2022 10:03:29 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'null' with binding: 'null' with usage: 'OTHER' in context: 'class hudson.cli.GroovyshCommand' with description: 'Session: 24410813' by user: 'admin'
Sep 01, 2022 10:03:30 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'return 4' with binding: '[out:java.io.PrintWriter@30b775f,hudson:hudson.model.Hudson@230a2e04,jenkins:hudson.model.Hudson@230a2e04]' with usage: 'EXECUTION' in context: 'class hudson.cli.GroovyshCommand' with description: 'Session: 24410813' by user: 'admin'
Sep 01, 2022 10:03:40 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'foo = 'bar'' with binding: '[out:java.io.PrintWriter@30b775f,hudson:hudson.model.Hudson@230a2e04,jenkins:hudson.model.Hudson@230a2e04,_:4]' with usage: 'EXECUTION' in context: 'class hudson.cli.GroovyshCommand' with description: 'Session: 24410813' by user: 'admin'
Sep 01, 2022 10:03:42 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'echo foo' with binding: '[out:java.io.PrintWriter@30b775f,hudson:hudson.model.Hudson@230a2e04,jenkins:hudson.model.Hudson@230a2e04,_:bar,foo:bar]' with usage: 'EXECUTION' in context: 'class hudson.cli.GroovyshCommand' with description: 'Session: 24410813' by user: 'admin'
Sep 01, 2022 10:03:44 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'null' with binding: 'null' with usage: 'OTHER' in context: 'class hudson.cli.GroovyshCommand' with description: 'Session: 1119729556' by user: 'admin'
Sep 01, 2022 10:03:45 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'return 4' with binding: '[out:java.io.PrintWriter@421cb429,hudson:hudson.model.Hudson@230a2e04,jenkins:hudson.model.Hudson@230a2e04]' with usage: 'EXECUTION' in context: 'class hudson.cli.GroovyshCommand' with description: 'Session: 1119729556' by user: 'admin'
Sep 01, 2022 10:03:53 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'foo = 'bar'' with binding: '[out:java.io.PrintWriter@421cb429,hudson:hudson.model.Hudson@230a2e04,jenkins:hudson.model.Hudson@230a2e04,_:4]' with usage: 'EXECUTION' in context: 'class hudson.cli.GroovyshCommand' with description: 'Session: 1119729556' by user: 'admin'
Sep 01, 2022 10:03:57 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'println foo' with binding: '[out:java.io.PrintWriter@421cb429,hudson:hudson.model.Hudson@230a2e04,jenkins:hudson.model.Hudson@230a2e04,_:bar,foo:bar]' with usage: 'EXECUTION' in context: 'class hudson.cli.GroovyshCommand' with description: 'Session: 1119729556' by user: 'admin'
Sep 01, 2022 10:11:31 AM FINE jenkins.model.ScriptListener$LoggingListener
Script: 'println "uname -a".execute().text' with binding: '[]' with usage: 'EXECUTION' in context: 'class hudson.util.RemotingDiagnostics' with description: 'hudson.remoting.Channel@7fa3b31d:1' by user: 'null'

[daniel-beck]

I have not yet figured out how the Groovysh profile files work; those could potentially still be used to bypass logging.

https://github.com/apache/groovy/blob/GROOVY_2_4_21/subprojects/groovy-groovysh/src/main/groovy/org/codehaus/groovy/tools/shell/Groovysh.groovy#L397 calls https://github.com/apache/groovy/blob/GROOVY_2_4_21/subprojects/groovy-groovysh/src/main/groovy/org/codehaus/groovy/tools/shell/commands/LoadCommand.groovy#L88 calls https://github.com/apache/groovy/blob/GROOVY_2_4_21/subprojects/groovy-groovysh/src/main/groovy/org/codehaus/groovy/tools/shell/Shell.groovy#L122 calls https://github.com/apache/groovy/blob/GROOVY_2_4_21/subprojects/groovy-groovysh/src/main/groovy/org/codehaus/groovy/tools/shell/Groovysh.groovy#L158 so LoggingGroovySh should cover this (untested).

---

FTR https://github.com/daniel-beck/jenkins/tree/meiswjn-feature-add-script-console-listener is my current work in progress. The API got changed incompatibly (and will change again because it becomes apparent that there need to be more methods if we start logging output as well), so there's currently no working script-security integration yet.

[meiswjn]

Hi Daniel, thanks for all the feedback! I will wait with responding until you are done :)

Just one thing in between: I am not sure if we should log the output. The usecase that I wanted this feature for was logging when admins decrypt secrets via the console. We would need to ensure that sensitive values do not appear in the audit log.

Logging the output seems like it adds plenty of complexity. Should we view that as two isolated features, perhaps?

[daniel-beck]

The usecase that I wanted this feature for was logging when admins decrypt secrets via the console. We would need to ensure that sensitive values do not appear in the audit log.

That's what

and will change again because it becomes apparent that there need to be more methods if we start logging output as well

hints at; output would be in something like onScriptOutput since the existing parameters don't make a lot of sense except to create correlation of some kind. Leave that unimplemented, and you won't see output.

Or are you saying nobody should be able to record any output?

[meiswjn]

I would say that recording the script's output is a feature that could be considered, but I would say it is not as important as recording the script itself. I would suggest it is out of scope for this PR. Low reward, high effort from my perspective.