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

Add Jenkins agent support for GitHub Committer Authorization Strategy #209

Merged
merged 9 commits into from
Jul 31, 2023

Conversation

AndreBrinkop
Copy link
Contributor

@AndreBrinkop AndreBrinkop commented Oct 21, 2022

Problem

A common use case for bigger Jenkins setups is to use a plugin like the Swarm plugin to form an ad-hoc cluster. This plugin allows that agents that can be added and removed flexibly to and from the Jenkins server. Those agents require authentication and authorization to create, configure and add new computers to the Jenkins. Using the github-oauth-plugin the authentication for the agent is already possible using an GitHub access token with minimal rights from a GitHub user.

However, the authorization is not working if the GitHub Committer Authorization Strategy is selected as the access rights retrieved from GitHub are not sufficient in this use case and there is no way to add additional custom rights to users.

Solution

To solve this problem this PR adds a new field in the GitHub Committer Authorization Strategy configuration window that allows the specification of an optional Agent User Name. The user specified in that field will be provided with the following rights that are necessary for the agent user:

  • Overall Read Permission
  • Create Computers
  • Configure Computers
  • Connect Computers

The PR also adds a help dialog for this field and it includes unit tests to test that the right permissions are assigned to (and only to) the specified agent user.

Checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@AndreBrinkop AndreBrinkop requested a review from a team as a code owner October 21, 2022 12:46
@scurvydoggo
Copy link
Member

scurvydoggo commented Jul 7, 2023

@AndreBrinkop I should have checked the PR list. I just raised a duplicate PR!

It seems this plugin is not very actively maintained, I wonder who we can reach out to?

edit: mine is #246; I closed it - let's use this PR

Copy link
Member

@scurvydoggo scurvydoggo left a comment

Choose a reason for hiding this comment

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

Thanks @AndreBrinkop! I have made a few suggestions.

I'll give this a run in my dev instance as a form of integration testing.

@AndreBrinkop
Copy link
Contributor Author

Hi @scurvydoggo, first of all thank you for your detailed feedback to my PR. I went over your suggestions and for some of them I used the implementation from your PR as I think that they are more sophisticated than my original implementation.

The only open point is whether we need the overall "Read" right for the Agent User or not. The documentation of the swarm plugins mentions that it is needed (https://github.com/jenkinsci/swarm-plugin/blob/master/docs/security.adoc#authorization) but if you have experience that it is working without it I would say we leave it out?

@scurvydoggo
Copy link
Member

Hey @AndreBrinkop , thanks so much.

Looking at their document, they are being pretty explicit in that we should use Read. It's possible that I just hadn't hit the flow / edge case that requires it? I would prefer to keep it in, since Read is a safe operation anyway, and trust the experience of those plugin authors.

Copy link
Member

@scurvydoggo scurvydoggo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@scurvydoggo scurvydoggo left a comment

Choose a reason for hiding this comment

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

Oops, I think we'll need to bump the plugin version.

It should be this line here:
https://github.com/jenkinsci/github-oauth-plugin/blame/master/pom.xml#L12

Some things on my end I need to do before merging this:

  • Test this in my dev/prod environment
  • Figure out how release notes are done
  • Check that CD best practices are in place (see this)

If the above becomes a blocker, we could look at making this an experimental release and merge it, so that you can hand it off, however let me look into the above points first.

@samrocketman
Copy link
Member

@scurvydoggo this plugin makes use of the maven release plugin which manages bumping the version as you release. However, since this plugin was last released Jenkins now supports a more direct approach to continuous delivery of plugins. I suggest following this guide so that merging to master automatically releases https://www.jenkins.io/doc/developer/publishing/releasing-cd/

@scurvydoggo scurvydoggo added the enhancement A new feature label Jul 21, 2023
@scurvydoggo
Copy link
Member

Thanks @samrocketman I have raised a PR for this here: #248

@scurvydoggo
Copy link
Member

The CD pipeline is up to date. I am now deploying this into my instance for testing.

@scurvydoggo
Copy link
Member

The UI looks good
image

@scurvydoggo
Copy link
Member

CasC is also working, and the actual functionality of the agents is ok.

@scurvydoggo scurvydoggo merged commit 88708ce into jenkinsci:master Jul 31, 2023
15 checks passed
scurvydoggo added a commit that referenced this pull request Jul 31, 2023
Add Jenkins agent support for GitHub Committer Authorization Strategy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants