jenkinsci / oidc-provider-plugin

OpenID Connect Provider Plugin for Jenkins
https://plugins.jenkins.io/oidc-provider/
MIT License
22 stars 13 forks source link

Proposal for changing sub and adding more claims #14

Closed LucaPrete closed 1 year ago

LucaPrete commented 2 years ago

What feature do you want to see added?

Hi and thanks for maintaining this plugin. It's been super useful!

I'd like to propose to slightly change the sub to

GitLab (ref)

project_path:mygroup/myproject:ref_type:branch:ref:main

GH (ref)

repo:octo-org/octo-repo:environment:prod

This way we might use one openid token to serve multiple multiple requests for multiple branches (matching against different SAs)

Following their model it might also be convenient to add more claims, although I don't see it urgent as modifying the sub.

What do you think? Should I send a patch for it? Happy to discuss more!

Upstream changes

No response

LucaPrete commented 2 years ago

Hi All!

I'd start with repo_name:branch

I put together some info to get the repo name and the branch name in Jenkins, depending on the plugin used. Nothing final or fully experimented yet. Unfortunately, Jenkins doesn't make this really easy, as it seems both params change, depending on the Jenkins version or plugin used.

For repo name:

For the branch name:

I'll start experimenting a bit these and I'll let you know how it goes. Happy to receive suggestions or discuss further.

jglick commented 2 years ago

I have a mild preference for keeping sub as a URL for the job (something which is unconditionally available) and adding claims for specific pieces of information (e.g. https://javadoc.jenkins.io/plugin/scm-api/jenkins/scm/api/SCMRevisionAction.html for branch info from multibranch Pipeline builds) where applicable & available.

This way we might use one openid token to serve multiple multiple requests for multiple branches (matching against different SAs)

Can you elaborate? How do you plan to configure services to grant or deny permissions based on the claims in the id token? I have come across very different configuration styles and levels of flexibility in GCP and AWS.

LucaPrete commented 2 years ago

Hi @jglick. Thanks for getting back!

I guess having the branch name in a claim only would also absolutely work.

I think having the sub formed as suggested would

Also, as per here (for what concerns GCP) the sub should be unique and immutable. It's what forms the principalSet and what is used in logs. If we'll need to manage the infra from IaC (i.e. Terraform), it would also help to don't have urls with columns or double slashes (i.e. //) in it.

jglick commented 2 years ago

the sub should be unique and immutable

Which is accomplished by using the job URL I think? I suppose a URI relative to the Jenkins root (e.g., jobs/my%20folder/jobs/my%20first%20project/) or an Item.fullName (e.g., my folder/my first project) would also work, since the Jenkins identity is implied by the issuer.

one conditions against the sub, instead of putting in end multiple conditions

Perhaps. It comes down to seeing some examples of what sorts of conditions you are actually working with, in which audience system. For example https://cloud.google.com/iam/docs/using-workload-identity-federation#gcloud the attribute.*/* variant looks reasonable but I have not personally tried it. Conversely, if multiple pieces of information are packed into the subject with (say) :-separated fields, do you expect to do an exact string match on the result? Some sort of regexp? Different audiences would have different abilities to match claims, and the OIDC spec does not cover this CI use case at all so everyone is making something up.

LucaPrete commented 2 years ago

@jglick thanks for getting back. While the field is a little bit different from other major providers I don't have strong feelings to change necessarily the sub. We can definitely start adding claims!

Working on the PR and getting back asap :)

LucaPrete commented 2 years ago

Hello again @jglick .

The patch should be "ready". Anyway I have unit tests failing. Weird thing is that unit tests are failing also in the plain master (please see file attached). Clearly, I must be missing something. Any thought?

Thanks,

-Luca

unit-tests-failure.txt

jglick commented 2 years ago

I do not recognize those failures. Make sure you are running a current Maven release, from the command line (do not trust some IDE), and a current JDK (8 or newer; 11 recommended), and

mvn clean verify
jglick commented 2 years ago

(By the way I will be unavailable for a couple weeks.)

LucaPrete commented 2 years ago

Hey, thanks for replying. No worries, I'll also be OOO from the end of next week to Sept 5th.

So, interesting enough mvn clean verify "works". Before I was executing either mvn test or mvn clean test and it wasn't. When I say it works, there are of course some expected failures, given I still haven't modified the tests. Now I can advance on this and submit the PR.

Thanks again and happy holidays!