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

Composite claims in OIDC connector #3056

Merged
merged 15 commits into from
Oct 25, 2023
Merged

Composite claims in OIDC connector #3056

merged 15 commits into from
Oct 25, 2023

Conversation

Oded-B
Copy link
Contributor

@Oded-B Oded-B commented Jul 27, 2023

Overview

Adds the ability to compose new group claims elements in the OIDC connector, based on other claims.
So this OIDC connector config:

     claimConcatenations:
        - claimList:
           - "organization_slug"
           - "pipeline_slug"
           - "build_branch"
          delimiter: "::"
          prefix: "bk"

Would result in this element in the group claim which looks like this:: bk::acme-inc::super-duper-app::main

What this PR does / why we need it

After #2806 we are able to authenticate with JWT generated by CI systems.
The claim provided by those systems are usually static and if the upstream OIDC service doesn't support sophisticated query language in its IAM/RBAC mechanism the operator might be unable to find a configuration that provided the needed granularity.

For example, lets say you want a specific Buildkiite pipeline to run a sync action on an ArgoCD instance.
These are claim provided by Buildkite CI/CD system:

{
  "iss": "https://agent.buildkite.com",
  "sub": "organization:acme-inc:pipeline:super-duper-app:ref:refs/heads/main:commit:9f3182061f1e2cca4702c368cbc039b7dc9d4485:step:build",
  "aud": "https://buildkite.com/acme-inc",
  "iat": 1669014898,
  "nbf": 1669014898,
  "exp": 1669015198,
  "organization_slug": "acme-inc",
  "pipeline_slug": "super-duper-app",
  "build_number": 1,
  "build_branch": "main",
  "build_commit": "9f3182061f1e2cca4702c368cbc039b7dc9d4485",
  "step_key": "build",
  "job_id": "0184990a-477b-4fa8-9968-496074483cee",
  "agent_id": "0184990a-4782-42b5-afc1-16715b10b8ff"
}

AFAIKT ArgoCD doesn't have any fancy query language in its RBAC config like big cloud providers IAM (regexs are supported to action field only)
So If you want to allow pipelines runs from a specific repo and branch combination, you would need to combine organization_slug, pipeline_slug and build_branch claims ( notice the sub claim includes the git commit, making it unusable as reference for RBAC ).

With this PR you'll be able to concatenate/compose multiple claims to a new element in the group claim:

        - name: Buildkite Agents OIDC
          type: oidc
          id: bk-oidc
          config:
            issuer: https://agent.buildkite.com
            scopes:
              - groups
              - openid
            userNameKey: sub
            claimConcatenations:
              - claimList:
                  - "organization_slug"
                  - "pipeline_slug"
                  - "build_branch"
                delimiter: "::"
                prefix: "bk"

This would be the resulting group : bk::acme-inc::super-duper-app::main
And you can refrence it in ArgoCD RBAC like so:

      g, bk::acme-inc::super-duper-app::main, role:cicd-sync

Special notes for your reviewer

This is intended as a temporary solution until a more flexible solution is provided via #1635.
Because the problem is uniq to service-tied JWTs and only relevant for OIDC I believe a connecter-specific solution is acceptable.

Does this PR introduce a user-facing change?

Enhancements:
* Support composing new group claims in OIDC connector.

@Oded-B Oded-B changed the title Composite claims in OIDC connector (#3) Composite claims in OIDC connector Jul 27, 2023
@Oded-B Oded-B marked this pull request as ready for review July 27, 2023 12:04
Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

@Oded-B Thanks for the contribution.

I expect's going to be some time before we get #1635 implemented, so I suggest we take a more structured approach, anticipating that other similar features may be added to connectors.

First of all, I find it a bit misleading that groups are not mentioned anywhere even though the concatenated claims end up as groups.

Secondly, I think it would be nice to add at least one level of config above it so that we can collect similar future additions.

A couple options I can imagine:

# ...

claimModifications:
    groups:
        newFromClaims:
# ...

claimModifications:
    newGroupsFromClaims:

I'm not quite sure which one I like better yet. (@nabokihms WDYT?)

connector/oidc/oidc_test.go Outdated Show resolved Hide resolved
dependabot bot and others added 7 commits August 6, 2023 13:21
)

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.10.0 to 0.11.0.
- [Commits](golang/crypto@v0.10.0...v0.11.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Oded Ben-Ozer <[email protected]>
Bumps [helm/kind-action](https://github.com/helm/kind-action) from 1.7.0 to 1.8.0.
- [Release notes](https://github.com/helm/kind-action/releases)
- [Commits](helm/kind-action@v1.7.0...v1.8.0)

---
updated-dependencies:
- dependency-name: helm/kind-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Oded Ben-Ozer <[email protected]>
Signed-off-by: Cedric-Magnan <[email protected]>
Signed-off-by: Oded Ben-Ozer <[email protected]>
Signed-off-by: Cedric-Magnan <[email protected]>
Signed-off-by: Oded Ben-Ozer <[email protected]>
Signed-off-by: Cedric-Magnan <[email protected]>
Signed-off-by: Oded Ben-Ozer <[email protected]>
* Add the ability to composite new claims in the OIDC connector,  based on upstream claims

Signed-off-by: Oded Ben-Ozer <[email protected]>
Signed-off-by: Oded Ben-Ozer <[email protected]>
Oded-B and others added 2 commits August 6, 2023 13:31
and structure for future claim modification additions

Signed-off-by: Oded Ben-Ozer <[email protected]>
@Oded-B
Copy link
Contributor Author

Oded-B commented Aug 7, 2023

I used the 2nd option (claimModifications.newGroupsFromClaims) for now but I'll change it per @nabokihms opinion.
Also added comments for each test case.

connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
- Add missing json tag.
- Control delimiter cleaning with a configuration key.
- Use better variable names
- concatenate string using slice and join

Signed-off-by: Oded Ben-Ozer <[email protected]>
Signed-off-by: Oded Ben-Ozer <[email protected]>
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Show resolved Hide resolved
connector/oidc/oidc_test.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
- Rename some vars
- Cleanup some comments
- Tiny refactor to improve readability

Signed-off-by: Oded Ben-Ozer <[email protected]>
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Show resolved Hide resolved
connector/oidc/oidc.go Show resolved Hide resolved
Oded-B and others added 2 commits October 25, 2023 14:08
Co-authored-by: Márk Sági-Kazár <[email protected]>
Signed-off-by: Oded Ben Ozer <[email protected]>
Improve naming

Signed-off-by: Oded Ben-Ozer <[email protected]>
Signed-off-by: Oded Ben-Ozer <[email protected]>
@sagikazarmark
Copy link
Member

LGTM, thank you!

Once the build turns green, good to go.

@sagikazarmark sagikazarmark merged commit e41a28b into dexidp:master Oct 25, 2023
9 checks passed
connector/oidc/oidc.go Outdated Show resolved Hide resolved
ClearDelimiter bool `json:"clearDelimiter"`

// String to place before the first claim
Prefix string `json:"prefix"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Prefix string `json:"prefix"`
Prefix string `json:"prefix"`

Prefix scales badly because people may want to add prefixes, suffixes, or both, or only prefix specific groups.
This is the same problem we are struggling in Kubernetes (that will be fixed in upcoming releases with CEL).

}
for _, claimName := range config.ClaimList {
claimValue, ok := claims[claimName].(string)
if !ok { // Non string claim value are ignored, concatenating them doesn't really make any sense
Copy link
Member

Choose a reason for hiding this comment

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

This is obscure for users.

  1. At least an error should be returned.
  2. I do not see any obstacles to support integers or boolean claims.

connector/oidc/oidc.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants