OIDC: Add additional scopes #14888

markylaing · 2025-01-30T10:50:39Z

Adds an oidc.additional_scopes configuration key which defaults to profile,offline_access.
Removes the oidc.groups.claim value from requested scopes.

Things I'm not sure on:

Using a comma to delimit the scope values. Identity providers appear to be quite freeform. It might be better to accept this as a space delimited as in Auth: Make OIDC scopes configurable #14832 (as the author originally suggested) to avoid issues with splitting on the comma.
Backwards and forwards compatibility. New clients talking to old servers will not request the configured scopes. Old clients talking to new servers will continue to request the groups claim, but will continue to request the profile and offline_access scopes.

Requires #14871
Closes #14141

tomponline · 2025-01-30T20:35:24Z

Can be rebased now to remove linter commit

doc/api-extensions.md

lxd/auth/oidc/oidc.go

lxd/cluster/config/config.go

lxd/auth/oidc/oidc.go

markylaing · 2025-02-05T17:16:36Z

@tomponline this is ready for review

minaelee · 2025-02-05T17:26:19Z

lxd/cluster/config/config.go

+
+	// lxdmeta:generate(entities=server; group=oidc; key=oidc.additional_scopes)
+	// Specify additional scopes to be requested when performing OIDC flows.
+	// Additional scopes might be required if managing access control in the Identity Provider (see {ref}`identity-provider-groups`).


Suggested change

// Additional scopes might be required if managing access control in the Identity Provider (see {ref}`identity-provider-groups`).

// Additional scopes might be required if managing access control in the Identity Provider (see: {ref}`identity-provider-groups`).

minaelee · 2025-02-05T17:27:29Z

lxd/cluster/config/config.go

+	// Additional scopes might be required if managing access control in the Identity Provider (see {ref}`identity-provider-groups`).
+	// The default value for this configuration option contains two scopes which are optional, but will degrade LXD OIDC functionality if removed.
+	// These are:
+	//   - `profile`: This is requested by LXD to present display names for the identification of end users. If it is removed, only email addresses will be displayed.


Suggested change

// - `profile`: This is requested by LXD to present display names for the identification of end users. If it is removed, only email addresses will be displayed.

// - `profile`: This is requested by LXD to present display names for the identification of end users. If it is removed, only email addresses are displayed.

Signed-off-by: Mark Laing <[email protected]>

There will be more additional optional configuration with the addition of `oidc.additional_scopes` so it makes sense to keep them together. This also simplifies the `NewVerifier` function. Signed-off-by: Mark Laing <[email protected]>

Additionally, defined `requiredScopes` as the scopes that are always requested. Signed-off-by: Mark Laing <[email protected]>

Previously just logging a warning here but we should return as well to avoid a nil pointer exception. Signed-off-by: Mark Laing <[email protected]>

Additionally, stop setting the groups claim as a scope. Signed-off-by: Mark Laing <[email protected]>

The additional scopes are marshalled as JSON. Signed-off-by: Mark Laing <[email protected]>

…er`. Signed-off-by: Mark Laing <[email protected]>

Signed-off-by: Mark Laing <[email protected]>

… is updated. Signed-off-by: Mark Laing <[email protected]>

This changes the following: - Stops using the value of `X-LXD-OIDC-groups-claim` as a scope. - Stops requesting `offline_access` and `profile` by default. - Gets additional scopes by unmarshalling the value of `X-LXD-OIDC-additional-scopes`. If an old client connects to a new LXD server, they will continue to request `offline_access` and `profile` scopes by default, but will not request any additional scopes. Nor will they receive the groups claim header. If a new client connects to an old server, they will only request the `openid` and `email` scopes. This is sufficient for logging in in most cases. Signed-off-by: Mark Laing <[email protected]>

Signed-off-by: Mark Laing <[email protected]>

markylaing · 2025-02-06T17:49:03Z

Discussed with Tom that this needs extra work. If both profile and offline_access scopes are removed, LXD will continue to use them as the default values.

tomponline · 2025-02-07T10:25:19Z

@markylaing shall we close this for now?

markylaing · 2025-02-07T12:12:59Z

@markylaing shall we close this for now?

I don't think so. Making this configurable is useful for @ech0-de (for Gitlab OIDC) and as far as I know the only contentious thing is how to unset both profile and offline_access scopes.

How about we introduce two keys?

oidc.scopes.additional: No default. Can be used to set additional scopes if required for retrieving group info, or if the OIDC provider requires any additional scopes (I've seen Entra ID use a user.read scope for example). Validation logic will not allow setting an additional scope that is present in oidc.scopes.omit.
oidc.scopes.omit: No default. Can be used to omit the profile and/or offline_access scopes. Validation logic will prevent setting openid, email, or any scopes present in oidc.scopes.additional.

That said, I guess another point of contention I have with our OIDC implementation is the use of headers to send e.g. the issuer URL to the LXD client. I think these values should be returned via /1.0 and this PR adds yet another header.

markylaing · 2025-02-07T12:24:59Z

@markylaing shall we close this for now?

I don't think so. Making this configurable is useful for @ech0-de (for Gitlab OIDC) and as far as I know the only contentious thing is how to unset both profile and offline_access scopes.

How about we introduce two keys?

oidc.scopes.additional: No default. Can be used to set additional scopes if required for retrieving group info, or if the OIDC provider requires any additional scopes (I've seen Entra ID use a user.read scope for example). Validation logic will not allow setting an additional scope that is present in oidc.scopes.omit.

oidc.scopes.omit: No default. Can be used to omit the profile and/or offline_access scopes. Validation logic will prevent setting openid, email, or any scopes present in oidc.scopes.additional.

That said, I guess another point of contention I have with our OIDC implementation is the use of headers to send e.g. the issuer URL to the LXD client. I think these values should be returned via /1.0 and this PR adds yet another header.

I don't like this idea either. We can just have oidc.scopes and validation that requires openid and email be included.

markylaing · 2025-02-07T18:16:49Z

Closing in favour of #14935.

markylaing added the Bug Confirmed to be a bug label Jan 30, 2025

markylaing self-assigned this Jan 30, 2025

github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Jan 30, 2025

markylaing requested a review from tomponline January 30, 2025 10:51

markylaing mentioned this pull request Jan 30, 2025

OIDC: Add Entra ID support #14889

Closed

minaelee requested changes Jan 30, 2025

View reviewed changes

doc/api-extensions.md Outdated Show resolved Hide resolved

doc/api-extensions.md Outdated Show resolved Hide resolved

BarcoMasile reviewed Jan 31, 2025

View reviewed changes

lxd/auth/oidc/oidc.go Show resolved Hide resolved

lxd/cluster/config/config.go Outdated Show resolved Hide resolved

lxd/auth/oidc/oidc.go Outdated Show resolved Hide resolved

tomponline mentioned this pull request Jan 31, 2025

Auth: Make OIDC scopes configurable #14832

Closed

markylaing force-pushed the oidc-additional-scopes branch from d7a900a to 143993a Compare February 5, 2025 10:30

markylaing requested review from minaelee and BarcoMasile February 5, 2025 17:16

minaelee reviewed Feb 5, 2025

View reviewed changes

markylaing added 15 commits February 6, 2025 17:47

api: Add oidc_additional_scopes API extension.

807d2a9

Signed-off-by: Mark Laing <[email protected]>

lxd/cluster/config: Add oidc.additional_scopes config key.

44a8508

Signed-off-by: Mark Laing <[email protected]>

lxd/cluster/config: Update groups claim description.

5717e8d

Signed-off-by: Mark Laing <[email protected]>

doc: Run make update-metadata.

0d4c36f

Signed-off-by: Mark Laing <[email protected]>

lxd/auth/oidc: Set Opts as a field on the Verifier.

93ba1f4

There will be more additional optional configuration with the addition of `oidc.additional_scopes` so it makes sense to keep them together. This also simplifies the `NewVerifier` function. Signed-off-by: Mark Laing <[email protected]>

lxd/auth/oidc: Add AdditionalScopes to Opts.

3c3539a

Additionally, defined `requiredScopes` as the scopes that are always requested. Signed-off-by: Mark Laing <[email protected]>

lxd/auth/oidc: Return nil if getting IdP groups fails.

05d5699

Previously just logging a warning here but we should return as well to avoid a nil pointer exception. Signed-off-by: Mark Laing <[email protected]>

lxd/auth/oidc: Set additional scopes when getting the relying party.

1f8fdf6

Additionally, stop setting the groups claim as a scope. Signed-off-by: Mark Laing <[email protected]>

lxd/auth/oidc: Send additional scopes as a header to the client.

f4952ff

The additional scopes are marshalled as JSON. Signed-off-by: Mark Laing <[email protected]>

lxd/cluster/config: Return additional scopes from `(*Config).OIDCServ…

c7c10dd

…er`. Signed-off-by: Mark Laing <[email protected]>

lxd: Update calls to (*Config).OIDCServer and oidc.NewVerifier.

5e6b382

Signed-off-by: Mark Laing <[email protected]>

lxd: Consider OIDC config to have changed if oidc.additional_scopes…

08a7ade

… is updated. Signed-off-by: Mark Laing <[email protected]>

lxd: Add patch moving oidc.groups.claim to oidc.additional_scopes.

450c25a

Signed-off-by: Mark Laing <[email protected]>

lxd: Fix lint error (staticcheck).

4a082dd

Signed-off-by: Mark Laing <[email protected]>

markylaing force-pushed the oidc-additional-scopes branch from 143993a to 4a082dd Compare February 6, 2025 17:48

markylaing marked this pull request as draft February 6, 2025 17:49

markylaing mentioned this pull request Feb 7, 2025

OIDC: Make scopes configurable #14935

Open

markylaing closed this Feb 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OIDC: Add additional scopes #14888

OIDC: Add additional scopes #14888

markylaing commented Jan 30, 2025 •

edited

Loading

tomponline commented Jan 30, 2025

markylaing commented Feb 5, 2025

minaelee Feb 5, 2025

minaelee Feb 5, 2025

markylaing commented Feb 6, 2025

tomponline commented Feb 7, 2025

markylaing commented Feb 7, 2025

markylaing commented Feb 7, 2025

markylaing commented Feb 7, 2025

	// Additional scopes might be required if managing access control in the Identity Provider (see {ref}`identity-provider-groups`).
	// Additional scopes might be required if managing access control in the Identity Provider (see: {ref}`identity-provider-groups`).

	// - `profile`: This is requested by LXD to present display names for the identification of end users. If it is removed, only email addresses will be displayed.
	// - `profile`: This is requested by LXD to present display names for the identification of end users. If it is removed, only email addresses are displayed.

OIDC: Add additional scopes #14888

OIDC: Add additional scopes #14888

Conversation

markylaing commented Jan 30, 2025 • edited Loading

tomponline commented Jan 30, 2025

markylaing commented Feb 5, 2025

minaelee Feb 5, 2025

Choose a reason for hiding this comment

minaelee Feb 5, 2025

Choose a reason for hiding this comment

markylaing commented Feb 6, 2025

tomponline commented Feb 7, 2025

markylaing commented Feb 7, 2025

markylaing commented Feb 7, 2025

markylaing commented Feb 7, 2025

markylaing commented Jan 30, 2025 •

edited

Loading