[JEP-223] Deprecate permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER #4365

mikecirioli · 2019-11-20T02:20:28Z

The permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER are effectively the same as Jenkins.ADMINISTER. This PR flags these permissions as deprecated, and updates jenkins core to instead check for Jenkins.ADMINISTER. This is intended to be a first example and that over time other plugins will follow suit.

The usage of these permissions is confusing, and has been effectively hidden (unless specifically enabled) since 2017-4-10 (https://jenkins.io/security/advisory/2017-04-10/#matrix-authorization-strategy-plugin-allowed-configuring-dangerous-permissions).

Along with this change, jenkinsci/matrix-auth-plugin#77 is proposed to help prepare any users who currently have these permissions enabled.

Proposed changelog entries

The permissions Overall/RunScripts, Overall/UploadPlugins, and Overall/ConfigureUpdateCenter permissions are now deprecated. Custom authorization strategy implementations that grant Overall/Administer without implying one or more of these three permissions and as configurations that grant any of these permissions to users without Overall/Administer will will no longer work as expected.

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@daniel-beck
@jglick

…NSTER

core/src/main/java/jenkins/management/ConsoleLink.java

core/src/main/java/jenkins/model/Jenkins.java

core/src/main/java/jenkins/management/ConsoleLink.java

Co-Authored-By: Jesse Glick <[email protected]>

Co-Authored-By: Tim Jacomb <[email protected]>

This reverts commit aa0f378.

…(effectively) what they are.

Jenkins.ADMINISTER

core/src/main/java/hudson/PluginManager.java

…that is (effectively)" This change broke existing tests involving the matrix-auth plugin when legacy behavior for SECURITY-410 is enabled. This reverts commit e7bc69d.

core/src/main/java/hudson/PluginManager.java

daniel-beck

Almost there, just a bit more cleanup please 👍

core/src/main/java/jenkins/model/Jenkins.java

core/src/main/java/hudson/PluginManager.java

core/src/main/resources/hudson/PluginManager/tabBar.jelly

daniel-beck · 2020-02-20T21:12:42Z

test/src/test/java/jenkins/model/JenkinsTest.java

@@ -317,6 +317,7 @@ public void testDoScript() throws Exception {
        wc.withBasicApiToken(User.getById("bob", true));
        wc.assertFails("script", HttpURLConnection.HTTP_FORBIDDEN);

+        //TODO: remove once RUN_SCRIPTS is finally retired


It is unclear why the permission above was changed, and why this comment is necessary.

The permission was changed back to RUN_SCRIPTS because that part of the test doesn't make sense if charlie has Jenkins.ADMINISTER. Since RUN_SCRIPTS still technically exists I thought it made more sense to leave the test as-is for now, and to add the comment to remove that block when its properly retired.

changed back … leave the test as-is for now

But then this PR wouldn't show a difference…?

Ah, now I get it.

It probably makes sense to convert this test to demonstrate that granting RUN_SCRIPTS does not allow accessing the script console anymore, since all the permission checks are now looking at ADMINISTER instead.

It probably makes sense to convert this test to demonstrate

… which is exactly what this does now 🤦‍♂

daniel-beck · 2020-02-20T21:13:54Z

core/src/main/java/hudson/PluginManager.java

@@ -1634,7 +1634,7 @@ public HttpResponse doProxyConfigure(StaplerRequest req) throws IOException, Ser
    @RequirePOST
    public HttpResponse doUploadPlugin(StaplerRequest req) throws IOException, ServletException {
        try {
-            Jenkins.get().checkPermission(UPLOAD_PLUGINS);
+            Jenkins.get().checkPermission(Jenkins.ADMINISTER);


Strictly speaking, none of the permission checks in this file matter, but if System Read is about to land, it probably makes sense to keep them.

core/src/main/resources/hudson/PluginManager/advanced.jelly

Co-Authored-By: Daniel Beck <[email protected]>

daniel-beck · 2020-02-20T22:47:43Z

Please remove all localizations of Hudson.RunScriptsPermission.Description, PluginManager.ConfigureUpdateCenterPermission.Description and PluginManager.UploadPluginsPermission.Description.

mikecirioli · 2020-02-21T01:50:33Z

@daniel-beck apparently my local system munged the italian localization. I will see if i can fix that up in the morning

daniel-beck · 2020-02-21T11:02:30Z

I will see if i can fix that up in the morning

Do the editing in a very basic text editor, not an IDE that understands the properties format.

mikecirioli · 2020-02-21T12:27:53Z

Should be good now @daniel-beck

EstherAF

lgtm

mikecirioli · 2020-02-21T15:09:52Z

FYI upgrade guide work has been planned separately from this PR

mikecirioli · 2020-02-21T16:24:23Z

FYI: jenkinsci/matrix-auth-plugin#77 to update admin monitors shown to users who currently have these permissions enabled

core/src/main/resources/hudson/Messages_it.properties

mikecirioli added 6 commits November 19, 2019 21:06

Flag RUN_SCRIPTS permission as deprecated

e9b2a73

Change permission checks for RUN_SCRIPTS to check for ADMINISTER

825afaa

flag UPLOAD_PLUGINS as deprecated

ede3a6d

change checks for UPLOAD_PLUGINS to check for ADMINISTER

adc4ef8

flag CONFIGURE_UPDATECENTER as deprecated

779ccd1

change permission checks for CONFIGURE_UPDATECENTER to check for ADMI…

3b70d30

…NSTER

mikecirioli mentioned this pull request Nov 20, 2019

Sensible permission segregation for matrix based authorization jenkinsci/jep#249

Merged

jglick approved these changes Nov 21, 2019

View reviewed changes

core/src/main/java/jenkins/management/ConsoleLink.java Show resolved Hide resolved

core/src/main/java/jenkins/model/Jenkins.java Show resolved Hide resolved

jglick reviewed Nov 21, 2019

View reviewed changes

core/src/main/java/jenkins/model/Jenkins.java Show resolved Hide resolved

daniel-beck self-requested a review November 21, 2019 08:52

timja reviewed Nov 21, 2019

View reviewed changes

core/src/main/java/jenkins/management/ConsoleLink.java Show resolved Hide resolved

timja reviewed Nov 21, 2019

View reviewed changes

core/src/main/java/jenkins/management/ConsoleLink.java Show resolved Hide resolved

oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Nov 21, 2019

mikecirioli and others added 9 commits December 6, 2019 04:54

Update core/src/main/java/jenkins/model/Jenkins.java

2bcad23

Co-Authored-By: Jesse Glick <[email protected]>

Update core/src/main/java/jenkins/management/ConsoleLink.java

aa0f378

Co-Authored-By: Tim Jacomb <[email protected]>

Update core/src/main/java/jenkins/management/ConsoleLink.java

953a5ee

Co-Authored-By: Tim Jacomb <[email protected]>

Update core/src/main/java/jenkins/management/ConsoleLink.java

db42863

Co-Authored-By: Tim Jacomb <[email protected]>

Revert "Update core/src/main/java/jenkins/management/ConsoleLink.java"

caed539

This reverts commit aa0f378.

make the deprecated permissions == Jenkins.ADMINISTER, since that is …

e7bc69d

…(effectively) what they are.

Check for Jenkins.ADMINISTER instead of Jenkins.RUN_SCRIPTS

63af530

Update jelly checks for deprecated permissions to now check for

b8e6d4d

Jenkins.ADMINISTER

Fix broken test that checked for RUN_SCRIPTS

3eaaf12

jtnord reviewed Dec 9, 2019

View reviewed changes

core/src/main/java/hudson/PluginManager.java Outdated Show resolved Hide resolved

jtnord approved these changes Dec 9, 2019

View reviewed changes

EstherAF approved these changes Dec 16, 2019

View reviewed changes

Revert "make the deprecated permissions == Jenkins.ADMINISTER, since …

8b72487

…that is (effectively)" This change broke existing tests involving the matrix-auth plugin when legacy behavior for SECURITY-410 is enabled. This reverts commit e7bc69d.

jtnord reviewed Dec 17, 2019

View reviewed changes

core/src/main/java/hudson/PluginManager.java Show resolved Hide resolved

jtnord reviewed Dec 17, 2019

View reviewed changes

core/src/main/java/hudson/PluginManager.java Show resolved Hide resolved

jtnord reviewed Dec 17, 2019

View reviewed changes

core/src/main/java/hudson/PluginManager.java Show resolved Hide resolved

re-enable setting dangerous permissions == Jenkins.ADMINISTER

17063fe

daniel-beck requested changes Feb 20, 2020

View reviewed changes

daniel-beck added squash-merge-me Unclean or useless commit history, should be merged only with squash-merge and removed ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Feb 20, 2020

mikecirioli and others added 4 commits February 20, 2020 16:32

nit: add space

625fc45

Co-Authored-By: Daniel Beck <[email protected]>

nit: add space

51276ea

Co-Authored-By: Daniel Beck <[email protected]>

nit: add space

7ab354e

Co-Authored-By: Daniel Beck <[email protected]>

remove un-needed permission checks in plugimanager jelly files

cd1adf4

daniel-beck changed the title ~~Deprecate permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER~~ [JEP-223] Deprecate permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER Feb 20, 2020

mikecirioli added 2 commits February 20, 2020 20:37

remove localizations of deprecated permissions

8050bf3

revert italian locazation that got bungled on local system

16ffec7

daniel-beck added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Feb 21, 2020

remove deprecated perms from italian localizations

2019f8d

daniel-beck approved these changes Feb 21, 2020

View reviewed changes

EstherAF approved these changes Feb 21, 2020

View reviewed changes

timja added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging labels Feb 21, 2020

daniel-beck added the upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed label Feb 21, 2020

mikecirioli mentioned this pull request Feb 21, 2020

Add admin monitor to warn users that dangerous permissions are being deprecated jenkinsci/matrix-auth-plugin#77

Closed

timja merged commit 9ead76e into jenkinsci:master Feb 22, 2020

daniel-beck mentioned this pull request Feb 22, 2020

Introduce a new summary description field for jobs #4489

Closed

10 tasks

MarkEWaite mentioned this pull request Feb 24, 2020

Add changelog 2.222 jenkins-infra/jenkins.io#2899

Merged

jglick reviewed Feb 24, 2020

View reviewed changes

core/src/main/resources/hudson/Messages_it.properties Outdated Show resolved Hide resolved

timja mentioned this pull request Apr 11, 2020

Adjust GlobalLibraries#configRoundTrip test for latest LTS jenkinsci/workflow-cps-global-lib-plugin#96

Merged

jglick mentioned this pull request May 18, 2021

[JEP-223] RUN_SCRIPTS → ADMINISTER jenkinsci/script-security-plugin#348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JEP-223] Deprecate permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER #4365

[JEP-223] Deprecate permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER #4365

mikecirioli commented Nov 20, 2019 •

edited

Loading

daniel-beck left a comment

daniel-beck Feb 20, 2020

mikecirioli Feb 20, 2020 •

edited

Loading

daniel-beck Feb 20, 2020

daniel-beck Feb 20, 2020 •

edited

Loading

daniel-beck Feb 21, 2020

daniel-beck Feb 20, 2020

daniel-beck commented Feb 20, 2020

mikecirioli commented Feb 21, 2020

daniel-beck commented Feb 21, 2020

mikecirioli commented Feb 21, 2020

EstherAF left a comment

mikecirioli commented Feb 21, 2020

mikecirioli commented Feb 21, 2020

[JEP-223] Deprecate permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER #4365

[JEP-223] Deprecate permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER #4365

Conversation

mikecirioli commented Nov 20, 2019 • edited Loading

Proposed changelog entries

Submitter checklist

Desired reviewers

daniel-beck left a comment

Choose a reason for hiding this comment

daniel-beck Feb 20, 2020

Choose a reason for hiding this comment

mikecirioli Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

daniel-beck Feb 20, 2020

Choose a reason for hiding this comment

daniel-beck Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

daniel-beck Feb 21, 2020

Choose a reason for hiding this comment

daniel-beck Feb 20, 2020

Choose a reason for hiding this comment

daniel-beck commented Feb 20, 2020

mikecirioli commented Feb 21, 2020

daniel-beck commented Feb 21, 2020

mikecirioli commented Feb 21, 2020

EstherAF left a comment

Choose a reason for hiding this comment

mikecirioli commented Feb 21, 2020

mikecirioli commented Feb 21, 2020

mikecirioli commented Nov 20, 2019 •

edited

Loading

mikecirioli Feb 20, 2020 •

edited

Loading

daniel-beck Feb 20, 2020 •

edited

Loading