Allow use of the preferred_username OIDC claim. #1287

meson800 · 2023-03-26T17:20:44Z

Previously, Headscale would only use the email OIDC claim to set the Headscale user. In certain cases
(self-hosted SSO), it may be useful to instead use the preferred_username to set the Headscale username. This also closes the existing issue #938.

This adds a config setting to use this claim instead. The OIDC docs have been updated to include this entry as well. In addition, this adds an Authelia OIDC example to the docs.

I didn't see any existing unit or integration tests for OIDC, so wasn't sure where to add such tests. If I'm wrong, I'm happy to add them!

I did test this against my SSO server, and confirm that the preferred_username grant if you set it works as expected.

read the CONTRIBUTING guidelines
raised a GitHub issue or discussed it on the projects chat beforehand (one already exists)
added unit tests
added integration tests
updated documentation if needed
updated CHANGELOG.md

kradalby · 2023-03-27T08:52:53Z

I think this looks sensible, the tests for oidc are located here: https://github.com/juanfont/headscale/blob/main/integration/auth_oidc_test.go

Would be good if we could have one.

meson800 · 2023-03-27T12:53:36Z

Thanks. It looks like the current integration tests don't explicitly set claims, but the underlying mockoidc does let you queue up successive users with potentially different email/preferred_username claims. I'll try my hand at creating an integration test for this.

vbrandl · 2023-04-14T13:05:12Z

Were you able to implement a testcase? I'd love to see this feature since it is blocking me from using OIDC for headscale.

meson800 · 2023-04-14T13:23:09Z

@vbrandl I'm working on it, but I'm new to Go and this codebase, and still figuring out how to more efficiently run the integration tests locally without just pushing to Github. The current OIDC tests don't actually check the claim responses at all, so I also need to thread those checks in.

On my stuff, I'm running from these docker images if you want to try them out: https://github.com/meson800/headscale/pkgs/container/headscale

vbrandl · 2023-04-14T15:30:58Z

I just tested your image and it does work as expected, but I'd prefer to run the official image for my infrastructure. So I'll wait until this gets merged.

jonathanspw · 2023-04-19T03:40:26Z

Thank you for this work @meson800

jonathanspw · 2023-04-19T21:18:50Z

I did some testing on this and it appears to work great. I noticed the email is still cited on the "congrats you've registered your machine" page or whatever it is which might cause some confusion.

That should probably also reflect the username if the setting is enabled.

meson800 · 2023-04-19T21:22:00Z

I'll look into changing that welcome page template @jonathanspw to respect the setting as well. On my local machine I have the mockoidc up and running with new mock users. this will be mergable hopefully within a week or so.

meson800 · 2023-04-22T18:46:24Z

@kradalby I added integration tests as requested, so this should be good for review. In sum, this PR includes:

Added the option to use the username claim as above, and updated the documentation.
Updates to the mockoidc runner; now you can give it a series of users in the format username_claim <email_claim@example.org> and have the mockoidc report those users.
Using that functionality, there are now integration tests that register clients under test-user <test-email@example.org> with the different claim options. I do this in a kind of sketchy way, by checking the FQDN string of TailscaleClients (e.g. makes sure they end in test-user.headscale.net or test-email.headscale.net, depending on the setting. I picked this method of validating since in the existing TailscaleClient interface that HSIC implements, there isn't a user field and I didn't want to add them.
As suggested by @jonathanspw, I also updated the post-login HTML template to say "Authenticated as username, you can now close this window", where username is filled in from the preferredUsername or email claim depending on the setting.

Even with a full nix develop on both actual Linux and WSL on Windows, I was unable to run one of the go formatters as suggested in the README, and couldn't figure out the path munging needed to install the tool on my own. If I'm failing some format or lint check, I can try to fix on my end if it's easier.

kradalby · 2023-04-24T08:46:11Z

cmd/headscale/cli/mockoidc.go

@@ -63,6 +65,29 @@ func mockOIDC() error {
 		}
 		accessTTL = newTTL
 	}
+
+	mockUsers := os.Getenv("MOCKOIDC_USERS")


Would be great if this is prefixed with "HEADSCALE_INTEGRATION_"

This is (technically) not just useful for integration tests, as this environment variable controls what mockoidc does if you e.g. set the env variable and ran headscale mockoidc yourself. I followed the naming of the other variables in the mockOIDC function (MOCKOIDC_CLIENT_ID, MOCKOIDC_CLIENT_SECRET, and so on). I can still prefix this if desired.

kradalby · 2023-04-24T08:50:10Z

integration/auth_oidc_test.go

+	oidcMap := map[string]string{
+		"HEADSCALE_OIDC_ISSUER":             oidcConfig.Issuer,
+		"HEADSCALE_OIDC_CLIENT_ID":          oidcConfig.ClientID,
+		"CREDENTIALS_DIRECTORY_TEST":        "/tmp",


Is this for Headscale or mockoidc? if its headscale, then prefix would be good.

Are you referring to CREDENTIALS_DIRECTORY_TEST? I copied this environment map from the TestOIDCAuthenticationPingAll test. For whatever historical reason, that test passes in the client secret with a file instead of directly, which is the reason for the directory. I could simplify if needed (I don't see why CREDENTIALS_DIRECTORY_TEST is a separate env variable).

kradalby · 2023-04-24T08:51:43Z

Otherwise, looks great!

meson800 · 2023-04-24T15:11:23Z

I fixed the lint issues and updated the gold-standard YAML files to match the added configuration option. (and rebased on main again).

Previously, Headscale would only use the `email` OIDC claim to set the Headscale user. In certain cases (self-hosted SSO), it may be useful to instead use the `preferred_username` to set the Headscale username. This also closes juanfont#938. This adds a config setting to use this claim instead. The OIDC docs have been updated to include this entry as well. In addition, this adds an Authelia OIDC example to the docs.

Updated the MockOIDC wrapper to take an environment variable that lets you set the username/email claims to return. Added two integration tests, TestOIDCEmailGrant and TestOIDCUsernameGrant, which check the username by checking the FQDN of clients. Updated the HTML template shown after OIDC login to show whatever username is used, based on the Headscale settings.

loprima-l · 2023-04-27T19:14:32Z

Isn't it better if we add an internal mail domain for this case, i think this would be easier to deal with in the api and al taht stuff :

ex:

preferred_username : loprima would make an email like loprima@headscale.sso

It would be more accurate with the Tailscale way of managing sso I guess

meson800 · 2023-04-27T19:24:18Z

I think that is something that should be up to your OIDC provider. If you're self hosting, you can return those types of fake/internal email addresses with the email grant. Other self hostable software (Nextcloud, Matrix) deals with the possible non-uniqueness of the preferred username claim by binding usernames to the OIDC provider, which is something Headscale doesn't currently do. If Headscale allows multiple OIDC providers in the future that will have ti be fixed.

…

On Thu, Apr 27, 2023, 3:14 PM loprima-l ***@***.***> wrote: Isn't it better if we add an internal mail domain for this case, i think this would be easier to deal with in the api and al taht stuff : ex: preferred_username : loprima would make an email like ***@***.*** It would be more accurate with the Tailscale way of managing sso I guess — Reply to this email directly, view it on GitHub <#1287 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOEWD7LKA676I4VSPD4DO3XDLASHANCNFSM6AAAAAAWIJSWKI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

loprima-l · 2023-04-27T19:31:32Z

I agree with that, but aren't we there to implement best practices to follow ? I think using multiple ways of storing a user "id" will be a mess that will complicate the interconnection between Headscale and other pieces of software.

My advice is that we should have one way to identify a user, if we already have email based user distinction, wouldn't it be bad to implement other ways that doesn't fit with the Tailscale way ?

meson800 · 2023-04-27T19:55:19Z

The default OIDC settings prior to this PR already strips the domain from the email claim unless you configure it not to. If you e.g. point Google's OIDC provider at it, you'll get a Headscale user with @gmail.com removed. There's still only one type of user ID with or without this PR. I think there is an advantage for allowing the option of using the preferred_username claim to create users; this is how OIDC supports letting SSO users/administrators pick stable usernames despite changing a backing email address. There is a unique source of truth on which user is which, the OIDC provider. I've only used Headscale; can you elaborate on what the Tailscale way to do this is?

…

On Thu, Apr 27, 2023, 3:31 PM loprima-l ***@***.***> wrote: I agree with that, but aren't we there to implement best practices to follow ? I think using multiple ways of storing a user "id" will be a mess that will complicate the interconnection between Headscale and other pieces of software. My advice is that we should have one way to identify a user, if we already have email based user distinction, wouldn't it be bad to implement other ways that doesn't fit with the Tailscale way ? — Reply to this email directly, view it on GitHub <#1287 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOEWDZVBYP5IX2JMG2UO3TXDLCR5ANCNFSM6AAAAAAWIJSWKI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

loprima-l · 2023-04-27T20:03:43Z

As you can see here :

Tailscale is using the provider domain, I think that's a better way to do it, maybe we must implement this way also in Headscale, but it would be necessary to use a headscale prefix, so we don't break up the compatibility with native Tascale APP, I don't know how they refer to the login methods.

meson800 · 2023-04-27T20:15:06Z

Thanks for the screenshot! My feeling is that I think this probably belongs with multi-provider support, not in this PR. I could work on it though in a future PR though. I would imagine that with multiple providers, the headscale admin could configure a suffix for each OIDC provider. E.g. Github could get a @github prefix, self-hosted Authelia could get a @internal.sso prefix, whatever.

…

On Thu, Apr 27, 2023, 4:03 PM loprima-l ***@***.***> wrote: As you can see here : [image: See here] <https://user-images.githubusercontent.com/69201633/234977645-4f4fbc59-d59e-465a-8bce-ca7594fb82f6.png> Tailscale is using the provider domain, I think that's a better way to do it, maybe we must implement this way also in Headscale, but it would be necessary to use a headscale prefix, so we don't break up the compatibility with native Tascale APP, I don't know how they refer to the login methods. — Reply to this email directly, view it on GitHub <#1287 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOEWDZMQ6ZUBNQVGUSQWQLXDLGKXANCNFSM6AAAAAAWIJSWKI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

loprima-l · 2023-04-27T20:18:11Z

I think that would be AMAZING <3

Like it will help a lot in the infinite quest of having a standardized system, I'm really busy to work on it but if you need help let me know I could write the docs if you want, I'm already planing to open a PR to add a gitbook.

meson800 · 2023-05-17T21:17:42Z

I merged against main so this can run tests again.

@kradalby do you want me to change the environment variables as in the review above? I named them to be consistent with the surrounding code.

@juanfont do you want me to update/edit this PR in some way/can you review? I see that there are other OIDC PRs that this might conflict with this (#1435), I can incorporate those changes if needed.

leeaash · 2023-07-22T07:26:44Z

When will this PR be merged to main branch, this feature is acutely I want.

joachimtingvold · 2023-09-15T11:40:23Z

Yes, please. Immutable identifier to be used by headscale would be nice to have, as most IDPs allows users to change their emails, and thus break their headscale setup/login.

meson800 · 2023-09-15T20:58:54Z

I can try to update this branch to get it considered again; a lot has changed since I submitted the PR.

jonathanspw · 2023-09-15T21:11:54Z

I can try to update this branch to get it considered again; a lot has changed since I submitted the PR.

Please do.

kradalby · 2023-09-24T21:02:38Z

Hi! as part of #1473, we have reorganised a lot of the code.

To clear PRs that needs to be rebased or redone, we are closing open PRs that will require significant code change to be merged.

In addition, the issue of the PR might in some cases have been fixed, change or no longer relevant, so it would be great if this is considered as well.

Thank you for your contribution!

If it is still relevant and the PR is reopened, we will aim at getting the changes into the next release after the reorg if accepted.

meson800 · 2023-10-30T00:17:25Z

@kradalby I can't reopen the PR myself, but this is ready to be considered again; I rebased and tested meson800:oidc_username_claim on top of the re-orged main.

Summary

read the CONTRIBUTING guidelines
raised a GitHub issue or discussed it on the projects chat beforehand (one already exists)
added unit tests
added integration tests
updated documentation if needed
updated CHANGELOG.md

This PR adds the ability to use the preferred_username claim, closing #938. I added documentation, updated the CHANGELOG.md, and added integration tests; I didn't add a unit test but can if needed.

The short version is that you can now set the use_username_claim config setting inside the oidc category. If set to true, the preferred_username claim is used to set the Headscale username instead of the email claim. I updated the way mockoidc is called to let integration tests set these claims.

Previous review comments

There were some comments on prefixing some of the constants. I've kept it consistent (e.g. not prefixing the mockoidc constants with HEADSCALE_INTEGRATION_ to stay consistent with the other existing constants and tests) for now but could easily rename those if desired.

Integration tests

In particular, the integration tests are implemented by setting the mock OIDC user to test-user <test-email@example.com>, then it checks that the FQDN of the user is test-user.headscale.net or test-email.headscale.net.

Test failures

Of note, I ran all of the integration tests in a stub PR in my repo; some integration tests appear flaky so I had to rerun them, but one still fails despite reruns, TestPingAllByIP. This one also seem to be failing in other PR runs in this repo so I don't think it's related (example 1)

Real tests

I generated a Docker image for this PR and tested it out in my tailnet, and it works, correctly using the preferred_username claim:

LarssonOliver · 2023-10-30T07:12:25Z

Looks awesome! Would really help my auth setup.

Helvio88 · 2024-01-10T18:49:56Z

I am using meson800's image and it works great. +1 to merge it!

Helvio88 · 2024-01-24T18:50:33Z

Any reason this wasn't merged? I see it's closed. I think it's a very welcome improvement.

meson800 · 2024-02-19T15:30:06Z

I think that one of the concerns is it wasn't generalizable enough. See this comment: #938 (comment)

meson800 requested review from juanfont and kradalby as code owners March 26, 2023 17:20

kradalby force-pushed the oidc_username_claim branch from 9daec94 to 056c7ca Compare March 27, 2023 08:52

meson800 force-pushed the oidc_username_claim branch from 6ed0b69 to 6fda090 Compare April 22, 2023 18:40

kradalby reviewed Apr 24, 2023

View reviewed changes

meson800 added 4 commits April 24, 2023 11:11

Added new GHA workflows for OIDC grant tests

c65587e

Updated gold compare configs and fixed lint styles

a642383

meson800 force-pushed the oidc_username_claim branch from 5d72a5b to a642383 Compare April 24, 2023 15:11

Merge remote-tracking branch 'upstream/main' into oidc_username_claim

b561002

kradalby closed this Sep 24, 2023

joachimtingvold mentioned this pull request Oct 2, 2023

Option to use Username instead of email when logging in via oidc #938

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow use of the preferred_username OIDC claim. #1287

Allow use of the preferred_username OIDC claim. #1287

meson800 commented Mar 26, 2023 •

edited

Loading

kradalby commented Mar 27, 2023

meson800 commented Mar 27, 2023

vbrandl commented Apr 14, 2023

meson800 commented Apr 14, 2023

vbrandl commented Apr 14, 2023

jonathanspw commented Apr 19, 2023

jonathanspw commented Apr 19, 2023

meson800 commented Apr 19, 2023

meson800 commented Apr 22, 2023

kradalby Apr 24, 2023

meson800 Apr 24, 2023

kradalby Apr 24, 2023

meson800 Apr 24, 2023

kradalby commented Apr 24, 2023

meson800 commented Apr 24, 2023 •

edited

Loading

loprima-l commented Apr 27, 2023

meson800 commented Apr 27, 2023 via email

loprima-l commented Apr 27, 2023

meson800 commented Apr 27, 2023 via email

loprima-l commented Apr 27, 2023

meson800 commented Apr 27, 2023 via email

loprima-l commented Apr 27, 2023

meson800 commented May 17, 2023

leeaash commented Jul 22, 2023 •

edited

Loading

joachimtingvold commented Sep 15, 2023

meson800 commented Sep 15, 2023

jonathanspw commented Sep 15, 2023

kradalby commented Sep 24, 2023

meson800 commented Oct 30, 2023 •

edited

Loading

LarssonOliver commented Oct 30, 2023

Helvio88 commented Jan 10, 2024

Helvio88 commented Jan 24, 2024

meson800 commented Feb 19, 2024

Allow use of the preferred_username OIDC claim. #1287

Allow use of the preferred_username OIDC claim. #1287

Conversation

meson800 commented Mar 26, 2023 • edited Loading

kradalby commented Mar 27, 2023

meson800 commented Mar 27, 2023

vbrandl commented Apr 14, 2023

meson800 commented Apr 14, 2023

vbrandl commented Apr 14, 2023

jonathanspw commented Apr 19, 2023

jonathanspw commented Apr 19, 2023

meson800 commented Apr 19, 2023

meson800 commented Apr 22, 2023

kradalby Apr 24, 2023

Choose a reason for hiding this comment

meson800 Apr 24, 2023

Choose a reason for hiding this comment

kradalby Apr 24, 2023

Choose a reason for hiding this comment

meson800 Apr 24, 2023

Choose a reason for hiding this comment

kradalby commented Apr 24, 2023

meson800 commented Apr 24, 2023 • edited Loading

loprima-l commented Apr 27, 2023

meson800 commented Apr 27, 2023 via email

loprima-l commented Apr 27, 2023

meson800 commented Apr 27, 2023 via email

loprima-l commented Apr 27, 2023

meson800 commented Apr 27, 2023 via email

loprima-l commented Apr 27, 2023

meson800 commented May 17, 2023

leeaash commented Jul 22, 2023 • edited Loading

joachimtingvold commented Sep 15, 2023

meson800 commented Sep 15, 2023

jonathanspw commented Sep 15, 2023

kradalby commented Sep 24, 2023

meson800 commented Oct 30, 2023 • edited Loading

Summary

Previous review comments

Integration tests

Test failures

Real tests

LarssonOliver commented Oct 30, 2023

Helvio88 commented Jan 10, 2024

Helvio88 commented Jan 24, 2024

meson800 commented Feb 19, 2024

meson800 commented Mar 26, 2023 •

edited

Loading

meson800 commented Apr 24, 2023 •

edited

Loading

leeaash commented Jul 22, 2023 •

edited

Loading

meson800 commented Oct 30, 2023 •

edited

Loading