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

[JENKINS-24513] Add access control for builds admin monitor #3919

Merged
merged 2 commits into from Mar 8, 2019

Conversation

@daniel-beck
Copy link
Member

commented Feb 28, 2019

See JENKINS-24513 (sort of). Submission in part triggered by #3908 (comment)

Builds in Jenkins run as SYSTEM by default. This can result in undesirable permission constellations allowing users to do more than they should be able to, e.g. using Pipeline Build Step Plugin.

stage 1
stage 2
stage 3

  • Admin monitor does not show in permission setups known to be too simple to matter here.
  • Progressively show whether a plugin is present implementing the necessary extension, whether it has been configured, and whether builds are still running as SYSTEM.
  • Allow resetting the "SYSTEM detector" after a configuration change.
  • Link to external docs
  • Actually write external docs (jenkins-infra/jenkins.io#2137)

Work in progress

  • Look for false positives. Notably, I have not tested this with background processes such as GitHub org or branch indexing and similar processes. The last item could potentially be very noisy.
  • (Optional) Keep track of which projects are running with SYSTEM permission rather than just a boolean.

Proposed changelog entries

  • RFE: Inform administrators about potentially unsafe permissions setup involving builds running as the virtual SYSTEM user.

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

@mention

@Wadeck

Wadeck approved these changes Mar 1, 2019

Copy link
Contributor

left a comment

I really like the approach / UI, great job!

}

}
}

This comment has been minimized.

Copy link
@Wadeck

Wadeck Mar 1, 2019

Contributor

What about using a Jelly version instead? => https://gist.github.com/Wadeck/3ecda58d1df9dc862f50e1c89c6b6d74

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 6, 2019

Member

I agree with @Wadeck . Groovy rendering is much more heavy that Jelly, and it is sensitive for monitors. Not a blocker tho, but it would be a nice improvement

Show resolved Hide resolved .../resources/jenkins/security/QueueItemAuthenticatorMonitor/message.groovy Outdated
@jglick

jglick approved these changes Mar 1, 2019

}
}

private static boolean anyBuildLaunchedAsSystemWithAuthenticatorPresent = false;

This comment has been minimized.

Copy link
@jglick

jglick Mar 1, 2019

Member

BTW need not be static.

displayName = executable.toString();
}

// TODO this is probably not intended to be used as a getter -- seems potentially unstable

This comment has been minimized.

Copy link
@jglick

jglick Mar 1, 2019

Member

I think it is OK. ExecutorChunk.canAccept calls it, as does Node.canTake, but the main call is from Executor.run—and BTW I think this could be removed now.

Show resolved Hide resolved .../resources/jenkins/security/QueueItemAuthenticatorMonitor/message.groovy Outdated

@daniel-beck daniel-beck force-pushed the daniel-beck:JENKINS-24513 branch from 75edaef to e7d5733 Mar 4, 2019

@oleg-nenashev
Copy link
Member

left a comment

I do not see anything wrong here.
Thanks @daniel-beck , it is a great enhancement

}

}
}

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 6, 2019

Member

I agree with @Wadeck . Groovy rendering is much more heavy that Jelly, and it is sensitive for monitors. Not a blocker tho, but it would be a nice improvement

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

I propose to merge it to the next weekly if there is no negative feedback

@oleg-nenashev oleg-nenashev merged commit ef958e4 into jenkinsci:master Mar 8, 2019

2 checks passed

continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

ad22 added a commit to ad22/jenkins that referenced this pull request Mar 21, 2019

[JENKINS-24513] Add access control for builds admin monitor (jenkinsc…
…i#3919)

* [JENKINS-24513] Add access control for builds admin monitor

* Fix potential encoding related issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.