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

Allow users without global Job/Build permission configure multibranch jobs #156

Merged
merged 3 commits into from
Apr 23, 2022
Merged

Allow users without global Job/Build permission configure multibranch jobs #156

merged 3 commits into from
Apr 23, 2022

Conversation

mymarche
Copy link
Contributor

Hello!

If somebody install gitlab-branch-source-plugin with authorization plugin (like Folder Authorization Plugin or Role-based Authorization Strategy), they might grant permission Job/Build on certain jobs without granting global permission Job/Build.
In this case, user with this permissions will get error:

hudson.security.AccessDeniedException3: <username> is missing the Job/Build permission
  at hudson.security.ACL.checkPermission(ACL.java:79)
  at hudson.security.AccessControlled.checkPermission(AccessControlled.java:47)
  at io.jenkins.plugins.gitlabserverconfig.servers.GitLabServer.getCredentials(GitLabServer.java:229)
  at io.jenkins.plugins.gitlabbranchsource.helpers.GitLabHelper.apiBuilder(GitLabHelper.java:25)
  at io.jenkins.plugins.gitlabbranchsource.GitLabSCMSource$DescriptorImpl.doFillProjectPathItems(GitLabSCMSource.java:843)
  at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:710)
  at org.kohsuke.stapler.Function$MethodFunction.invoke(Function.java:396)
Caused: java.lang.reflect.InvocationTargetException
...

This caused by check permission onli on Jenkins singleton in this line:

Documetation said:

Identify the nearest AccessControlled objects to check permissions with. If your 'this' object is already access-controlled, then that’s obviously it. Otherwise, try to look for the nearest logical ancestor. If all else fails, use the Jenkins singleton.

In this PR I try to fix AccessDeniedException by:

Fixes:
JENKINS-62478

@mymarche mymarche changed the title Bugfix: Allow users without global Job/Build permission configure multibranch jobs Allow users without global Job/Build permission configure multibranch jobs Aug 12, 2021
@mymarche mymarche marked this pull request as draft August 13, 2021 11:43
@mymarche mymarche marked this pull request as ready for review August 13, 2021 15:03
@justinharringa
Copy link

@LinuxSuRen @jetersen @MarkEWaite this seems to look good but I wonder if someone in the security SIG should give it a once-over?

@moshavnik
Copy link

@LinuxSuRen , can you speed up the merge of this PR?
is there anything we can do to help with process?

@mymarche
Copy link
Contributor Author

mymarche commented Aug 26, 2021

@baymac Hi! maybe you can help us with review this PR?

@moshavnik
Copy link

moshavnik commented Aug 26, 2021 via email

@justinharringa
Copy link

If folks are interested in helping to maintain the plugin and its tests, you might consider becoming a maintainer. I personally don't have enough time to dedicate here (and I also don't really use GitLab), unfortunately, and it seems @baymac (who did great work here) doesn't have enough time these days either.

Hope you are all well.

@mymarche
Copy link
Contributor Author

mymarche commented Sep 2, 2021

@LinuxSuRen Hi! Could you please merge this PR if you are fine with the changes?

@shelene31400
Copy link

Hello, I'm also having this problem, it would be nice if the PR is accepted :)

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Sep 22, 2021

@shelene31400 one of the most persuasive arguments to a plugin maintainer is when a user says, "I'm using this in production and it works the way I expected". You can provide that persuasion by installing the 'hpi' file into your Jenkins controller from https://ci.jenkins.io/job/Plugins/job/gitlab-branch-source-plugin/job/PR-156/ . Look for the most recent successful artifacts and find the '.hpi' file. Install that using the "Advanced" settings in the plugin manager.

If you're using a 'plugins.txt' file to manage your plugin versions as code, then you can use the 'incrementals' syntax to use that version. See https://github.com/MarkEWaite/docker-lfs/blob/4b0981a8e6689cca50f2cdfd8723e30f8d4fcaec/plugins.txt#L49 for one example.

@bitnik
Copy link

bitnik commented Sep 28, 2021

You can provide that persuasion by installing the 'hpi' file into your Jenkins controller from https://ci.jenkins.io/job/Plugins/job/gitlab-branch-source-plugin/job/PR-156/ . Look for the most recent successful artifacts and find the '.hpji' file. Install that using the "Advanced" settings in the plugin manager.

@MarkEWaite by following this, our jenkins admin installed the plugin on this PR. Then I tested it and now I can confirm that now I can configure a multibranch pipeline without global permissions, which I couldn't before.

@ohartl
Copy link

ohartl commented Sep 30, 2021

You can provide that persuasion by installing the 'hpi' file into your Jenkins controller from https://ci.jenkins.io/job/Plugins/job/gitlab-branch-source-plugin/job/PR-156/ . Look for the most recent successful artifacts and find the '.hpji' file. Install that using the "Advanced" settings in the plugin manager.

@MarkEWaite by following this, our jenkins admin installed the plugin on this PR. Then I tested it and now I can confirm that now I can configure a multibranch pipeline without global permissions, which I couldn't before.

Can also verify this PR works, LGTM!!

@usinelogicielle
Copy link

Hello, I confirm that the PR solve the problem. Our Admin installed it and we tested the plugin.
We use Jenkins 2.303.1 and GitLab 13.9.7 with PR version of GitLab Branch Source. I can now create jobs without global permissions. We use the Matrix Authorization Strategy plugin for role base authorizations for projects.
@LinuxSuRen @baymac can you please look at this PR? It looks great. Thanks in advance.
We can't wait for PR to be accepted :)

@Turiok
Copy link
Contributor

Turiok commented Oct 14, 2021

Hi @mymarche,

I would more explanation of this PR.
I tested it and it seems to work. But When I read the source code, I can't understand how.
I think all the modifications are for the method getCredentials() in class GitlabServer l230.
The changes add cases to check if the credential is in a folder level or higher. Not just on the Jenkins level.

But my comprehension of the plugin is that we have to give a credential to access to the Gitlab API when we create the Gitlab server instance in the administration panel. The GitlabServer class. So the jenkins level should be enough and with system scope's too.

But the issue seems to be a problem with the permission of the user who create the multi branch pipeline.
I can't see the link between find a credential in GitlabServer class. And the authorization of the user.

Could you help me to understand?

Kind regards,
Didier

@mymarche
Copy link
Contributor Author

Hi @mymarche,

I would more explanation of this PR. I tested it and it seems to work. But When I read the source code, I can't understand how. I think all the modifications are for the method getCredentials() in class GitlabServer l230. The changes add cases to check if the credential is in a folder level or higher. Not just on the Jenkins level.

But my comprehension of the plugin is that we have to give a credential to access to the Gitlab API when we create the Gitlab server instance in the administration panel. The GitlabServer class. So the jenkins level should be enough and with system scope's too.

But the issue seems to be a problem with the permission of the user who create the multi branch pipeline. I can't see the link between find a credential in GitlabServer class. And the authorization of the user.

Could you help me to understand?

Kind regards, Didier

Hi, @Turiok!

Yes, you absolutly right.
In this places




we need to use Jenkins.getAuthentication(), but then we cant find any credentials.

Like in this issue: JENKINS-58902

fromUri(defaultIfBlank(serverUrl, GITLAB_SERVER_URL)).build()
), withId(credentialsId));
} else {
context.checkPermission(CredentialsProvider.USE_OWN);
Copy link
Contributor

@Turiok Turiok Nov 17, 2021

Choose a reason for hiding this comment

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

Hi, @mymarche ,

Sorry to bothering you again.
I read your comment and read again the source code.
What I understand, This is this specific line which correct the problem right?
At this line Jenkins, or in your case the context of security, check if the user can configure the pipeline or not?

Strange the permission needed is the Job/Build permission and not Job/Configure but it's another problem.

But I don't understand the if else after. The personal access token(PAT) is stocked at the global level, during the creation of the gitlab server in the administration panel, so the old code should be working to find this PAT. Instead, do you try to change the code to find the PAT at folder level too?
It'll be great I think. The possibility to define the gitlab server instance during the creation of the multi branch pipeline.
But is it not another feature? And only with this change it can't work right? Because the creation of a gitlab server instance is only defined in the administration panel and not during the job creation. And to be perfect, regarding the gitlab project isolation easily. It should be Project access token and not Personnal access token.
Am I wrong on the last part or not?

Copy link
Contributor Author

@mymarche mymarche Nov 26, 2021

Choose a reason for hiding this comment

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

Hi, @Turiok !

At this line Jenkins, or in your case the context of security, check if the user can configure the pipeline or not?

Yes

Am I wrong on the last part or not?

No, you are right.
I created fix only for searching PAT in this case.
I think, define PAT for gitlab server per folder or per job is correct.
But i don't know how to implement it correctly.

@usinelogicielle
Copy link

Hello @LinuxSuRen @justinharringa @baymac @markyjackson-taulia,

If you have a little time, it would be nice to watch this PR. This is really a crucial feature for the plugin. We tested this PR on our side and it works. We have also analyzed the code modification and it does not bring any security issue.
I think it is also a highly requested feature by many people here.

Thanks in advance :)

@jetersen
Copy link
Member

@mymarche would you be able to fix the conflicts and ping me once the PR is ready for review 😄

@mymarche
Copy link
Contributor Author

@jetersen ping :)
Can you review this PR?

@jetersen jetersen added the enhancement New feature or request label Apr 23, 2022
.gitignore Outdated Show resolved Hide resolved
@jetersen jetersen merged commit 85cf3a4 into jenkinsci:master Apr 23, 2022
@jetersen
Copy link
Member

Released in https://github.com/jenkinsci/gitlab-branch-source-plugin/releases/tag/625.v85cf3a_400cfe

@ipleten
Copy link

ipleten commented Jun 21, 2022

I suspect this changes require to move token from System to Global Scope. At least it started working when i did it migrating from 1.5.9.

@usinelogicielle
Copy link

Hi @ipleten

We see this issue since a long time. I'm not remember if it's before or after this PR.
It's described here : #161

@ipleten
Copy link

ipleten commented Jun 21, 2022

Just to ensure we are on the same page.
We have multiply Jenkins installation with different plugin versions and I can confirm that v1.5.9 works with System Global creds the current version works only with Global Scope which causes security concerns (as now all jobs can obtain token)

@moshavnik
Copy link

I suspect this changes require to move token from System to Global Scope. At least it started working when i did it migrating from 1.5.9.

this implementation is very problematic since it exposes the token of the entire gitlab server to all the jobs. this poses a great security breach.
i opened a bug about this, here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet