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.

Sign up for GitHub

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

Jump to bottom

Auth: Add OIDC identities to identity cache and extract identity provider groups #12827

Merged
merged 25 commits into from
Feb 12, 2024
Merged

Auth: Add OIDC identities to identity cache and extract identity provider groups #12827

merged 25 commits into from
Feb 12, 2024

Conversation

markylaing
Copy link
Contributor

This PR adds a new server configuration key oidc.groups_claim. If set, the value of this config key is:

  1. Added to the scope of the Authorization Code Flow (UI).
  2. Sent to the client as an X-LXD-OIDC-groups-claim header, to be added to the scope for the Device Authorization Grant Flow (CLI).
  3. The value of the claim represents the groups that the identity is a member of at the identity provider level. It is extracted from both identity tokens (UI) and access tokens (CLI) and is expected to be a string slice.
  4. Identity provider groups are added to the request context.
  5. Identity provider groups are forwarded to other cluster members in the X-LXD-forwarded-identity-provider-groups header. The value of this header is a JSON encoded string.
  6. Forwarded identity provider groups are also added to the request context.

#12816 must be merged first.

@markylaing markylaing self-assigned this Feb 6, 2024
@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Feb 6, 2024
@tomponline tomponline changed the title Add OIDC identities to identity cache and extract identity provider groups Auth: Add OIDC identities to identity cache and extract identity provider groups Feb 8, 2024
@markylaing markylaing force-pushed the cache-oidc-identities branch 2 times, most recently from dfc9bb9 to 86eff12 Compare February 9, 2024 12:15
@markylaing markylaing marked this pull request as ready for review February 9, 2024 12:20
@markylaing
Copy link
Contributor Author

@tomponline this is ready for review now.

Also, do we prefer oidc.groups_claim or oidc.groups.claim for this config key?

@tomponline
Copy link
Member

oidc.groups.claim

I would say, as it allows us to use oidc.groups.foo later if needed

tomponline
tomponline reviewed Feb 9, 2024
View reviewed changes
lxd/daemon.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Feb 9, 2024
View reviewed changes
lxd/daemon.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Feb 9, 2024
View reviewed changes
lxd/daemon.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Feb 9, 2024
View reviewed changes
lxd/cluster/connect.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Feb 9, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

A handful of nits/questions, overall looks great.

tomponline
tomponline requested changes Feb 9, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Needs an API extension too please

markylaing
markylaing commented Feb 9, 2024
View reviewed changes
lxd/daemon.go Outdated Show resolved Hide resolved
@markylaing
Copy link
Contributor Author

Needs an API extension too please

What would the extension be for? This doesn't add any functionality (yet). Or is it just because we're adding a new config key?

@tomponline
Copy link
Member

tomponline commented Feb 9, 2024 •
edited
Loading

Needs an API extension too please

What would the extension be for? This doesn't add any functionality (yet). Or is it just because we're adding a new config key?

Yep the new config key is new functionality

@markylaing
Copy link
Contributor Author

Yep the new config key is new functionality

🤔 I suppose it is. You can set some config.

I'll update the PR on Monday and will add an API extension check in the client for adding this config key.

@markylaing markylaing mentioned this pull request Feb 10, 2024
Merged
@markylaing
lxd/auth/oidc: Request groups claim in OIDC scope if non-empty.
393ef21 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Write the groups claim as a header.
b78f68b 
This indicates to the client that it needs to request
the additional scope.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Add method to extract groups from custom claim.
5ca6a83 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Return identity information from Auth method.
745ba22 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Pass groups claim into OIDC verifier on initialisation.
71379a7 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Instantiate new OIDC verifier on groups claim config change.
ed22ef8 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Update call to (*config.Config).OIDCServer.
d6fcbc2 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
client: Add custom claim to scope if header is set.
76fd682 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/request: Add context keys and headers for identity provider groups.
b0a2b33 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Add new OIDC identities to identities table and refresh cache.
96b4c10 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Add identity provider groups to the request context.
34dd652 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/cluster: Forward identity provider groups to other members.
ae8d4ad 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Extract identity provider groups from forwarded request header.
80df4fa 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/cluster: Fix lint error (revive: early-return).
c550c20 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
test/suites: Test that OIDC identities are being added to the database.
b5fe028 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/auth/oidc: Use AuthError value, not reference.
9bae462 
AuthError implements xerrors.Wrapper so we can use that to inspect the
error instead of type assertion.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Wrap OIDC error and inspect with errors.As.
df2dda8 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
Copy link
Contributor Author

@tomponline I've made the requested changes. I haven't checked for the oidc_groups_claim API extension when attempting to set this config key in the client because there doesn't seem to be any existing API extension checks for config keys.

tomponline
tomponline requested changes Feb 12, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

just an api extension commit change

@markylaing
api: Adds oidc_groups_claim extension.
ae32725 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Fix lint errors (revive: exported).
14d51fd 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd: Ignore lint error (revive: defer).
458b87c 
This is a false positive.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@tomponline tomponline merged commit adfaa52 into canonical:main Feb 12, 2024
27 checks passed
markylaing added a commit to markylaing/lxd that referenced this pull request Feb 20, 2024
@markylaing
auth/oidc: Ensure the default config expiry interval is set.
34682f9 
When the `oidc.groups.claim` config key was added in canonical#12827, the
logic governing optional configuration of the OIDC verifier was
incorrect. This caused the config expiry interval to be set to zero
if not passed in by the caller.

This caused the verifier to rotate the encryption keys of the relying party
on every call. These keys are used to encrypt the `state` and `pkce`
cookies that OIDC needs to work. As they had been rotated during the
OIDC flow, the callback handler to failed to decrypt the cookies, and so
login was failing.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing markylaing mentioned this pull request Feb 20, 2024
Merged
markylaing added a commit to markylaing/lxd that referenced this pull request Feb 20, 2024
@markylaing
auth/oidc: Ensure the default config expiry interval is set.
6865f45 
When the `oidc.groups.claim` config key was added in canonical#12827, the
logic governing optional configuration of the OIDC verifier was
incorrect. This caused the config expiry interval to be set to zero
if not passed in by the caller.

This caused the verifier to rotate the encryption keys of the relying party
on every call. These keys are used to encrypt the `state` and `pkce`
cookies that OIDC needs to work. As they had been rotated during the
OIDC flow, the callback handler to failed to decrypt the cookies, and so
login was failing.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

@roosterfish roosterfish Awaiting requested review from roosterfish

@gabrielmougard gabrielmougard Awaiting requested review from gabrielmougard

@MusicDin MusicDin Awaiting requested review from MusicDin

Assignees

@markylaing markylaing

Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@markylaing @tomponline