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

OIDC preferred_username #117

Open
ChrisPortman opened this issue May 27, 2024 · 11 comments
Open

OIDC preferred_username #117

ChrisPortman opened this issue May 27, 2024 · 11 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request needs testing (on unstable) If the feature exists, but isnt yet released

Comments

@ChrisPortman
Copy link

Hi

I'm using AzureAD as the OIDC provider, and for some reason, when processing the preferred_username claim, its returning an empty string. Would it be possible to expose the claim used for the username as a configurable.

Additionally, the preferred_username is listed as "mutable" and not recommended for use as a linking identifier (at least according to MS - https://learn.microsoft.com/en-us/entra/identity-platform/id-token-claims-reference). Previously when I've done OIDC stuff with MS, my user model had a Username and Displayname (Displayname defaulted to Username), and then when using OIDC, the username value would be sourced from the oid claim and the display name would be sourced from preferred_username/name/email (configurable probably).

The issue with the oid claim without the differentiation of username vs displayname is that the oid is a GUID which makes no sense in a UI.

Happy to help with dev, if you think this is an issue worth looking at. Happy to, as a first cut, just provide an MR making the username claim configurable (default to preferred_username). Later look at updating the user model to introduce the username vs displayname concept

@NHAS
Copy link
Owner

NHAS commented May 27, 2024

Hi again Chris, thanks for another excellent issue.

I think this may actually be two issues in one. First being "can we make preferred_username configurable" which is a definite yes, and a second security vulnerability in the fact I'm using preferred_username as an ID rather than a GUID in the claim.

I definitely agree that the first issue should be configurable and you're more than welcome to open a MR for that.
As for the second, Im going to have to have a bit of a think about how to use the supplied GUID. If you want to raise that as an additional issue that'd be great!

@NHAS NHAS added bug Something isn't working enhancement New feature or request labels May 27, 2024
@ChrisPortman
Copy link
Author

I think for using the GUID (from the oid claim, it makes sense to have a web interface where users can auth in the first instance and that initial login creates the user account in the local data store. That account would look like something like:

{
  "username": "<uuid/guid>",
  "displayname": "<preferred username>"
  ...
}

Certainly a process that requires an admin to pre-create user accounts would be tricky, because locating the GUIDs are a pain. Once the users have performed an initial login, the admin can then perform other tasks like managing their group memberships (beyond what the groups claim may have) and register tokens etc.

@NHAS
Copy link
Owner

NHAS commented May 27, 2024

I've been meaning to make device registration SSO enabled for sometime actually. So it could be quite nice to have it that on first registration it creates the association.

Just makes it basically impossible to automate.

@ChrisPortman
Copy link
Author

looks like in version 8, in the data.Config representation of config, there is the concept of having the username claim configurable:

DeviceUsernameClaim string

I just cant see anywhere its set, and there isnt a corresponding option used when loading from the config file

@NHAS
Copy link
Owner

NHAS commented May 27, 2024

Ah I am incredibly dumb and forgot that I'd implemented this:

This is in the oidc provider, you can configure it via the administrative UI under settings.

		deviceUsername := info.GetPreferredUsername()

		if len(o.details.DeviceUsernameClaim) != 0 {

			deviceUsernameClaim, ok := tokens.IDTokenClaims.GetClaim(o.details.DeviceUsernameClaim).(string)
			if !ok {
				log.Println("Error, Device Username Claim set but idP has not set attribute in users token")
				http.Error(w, "Server Error", http.StatusInternalServerError)
				return
			}

			deviceUsername = deviceUsernameClaim

		}

@ChrisPortman
Copy link
Author

Yeah, I think the corresponding struct field just needs to be added here:

GroupsClaimName string `json:",omitempty"`

@NHAS
Copy link
Owner

NHAS commented May 27, 2024

Ah yes, it does. I've just left it being configurable in the administrative UI rather than adding it to the json file. Apologies!

@NHAS
Copy link
Owner

NHAS commented May 27, 2024

I've just updated my comments to be less incorrect haha

@NHAS
Copy link
Owner

NHAS commented Sep 1, 2024

Finally getting back around to this. I'll be using the sub (or subject) data thats given in the user info as its immutable by spec.

@NHAS
Copy link
Owner

NHAS commented Sep 1, 2024

My solution to the security portion of this issue is just to store and check the Idp provided subject. So its probably no the best solution. But it is the one that makes this a security non-issue, and doesnt require significant reworking of users within wag.

@NHAS NHAS self-assigned this Sep 1, 2024
@NHAS NHAS added the needs testing (on unstable) If the feature exists, but isnt yet released label Sep 1, 2024
@JSmith-Aura
Copy link
Contributor

Hmm, seems this was somehow reverted and Im not sure how. I'll be adding it back today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request needs testing (on unstable) If the feature exists, but isnt yet released
Projects
None yet
Development

No branches or pull requests

3 participants