Feat OIDC login as namespace #350

restanrm · 2022-02-22T20:22:16Z

read the CONTRIBUTING guidelines
raised a GitHub issue or discussed it on the projects chat beforehand
added unit tests
[] added integration tests
updated documentation if needed
updated CHANGELOG.md

should close #327

kradalby

Some comments, did you update the changelog? 🤔

kradalby · 2022-02-23T07:35:32Z

namespaces.go

+	name = normalizeNamespaceRegex.ReplaceAllString(name, "-")
+
+	for _, elt := range strings.Split(name, ".") {
+		if len(elt) > 63 {


This is good, I think I would have forgotten this.

It makes me thing tho, do we need an option to only use the username or the name before @? I suspect that with emails, we can easily get too long names here?

reallylongfirstname.middlename.longlastname@reallylongdomainname.com

This will throw an error, but not sure if we will display it to users, and users might not have a clue whats going on.

Yes this is also something that I thought about without reaching a result. I could add a check at some place where we return the hostname if it's too long. In the RFC 1123 we can read the following:

Host software MUST handle host names of up to 63 characters and
SHOULD handle host names of up to 255 characters.

If we throw an error like this we should add some comments on resolution or keep the domain_map config around to solve this kind of issues.

hmm, the combinations of options here is a bit of a can of worms:

Add too long ones into domain-only namespace

strip domain and use just username

what if username is still too long

I suggest keeping it a bit simple to begin with, and actually, I wonder if people dont really need to have the domain from the email in the namespace (as they are all likely the same).

Maybe we should have a oidc.strip_email_domain which is true by default, so by default, username is used, and if people intend to have different email addresses, you turn that off and get the whole thing.

So by default:
alice@enterprise.com -> alice
bob@enterprise.com -> bob

But you can turn that off:
alice@enterprise.com -> alice.enterprice-com (or what it will become)
bob@collaboratingcompany.com -> bob.collaboratingcompany-com

Host software MUST handle host names of up to 63 characters and
SHOULD handle host names of up to 255 characters.

As for 63 vs 255, I suggest we saw that 255 is an error and 63 is a warning.

The potential issue that I can see with the strip_email_domain as default is that if some users want to login with different email provider it's possible to have an issue. It can even become a security issue (impersonating admin account). I don't know OIDC well enough to see if it can be an issue. But I guess with a tool like dex (login aggregator), it seems possible.

My reference to RFC 1123 seems incomplete. RFC 2181 says the following:

The DNS itself places only one restriction on the particular labels
that can be used to identify resource records. That one restriction
relates to the length of the label and the full name. The length of
any one label is limited to between 1 and 63 octets. A full domain
name is limited to 255 octets (including the separators).

It seems to be the last RFC describing host names. The wikipedia page have the same information: https://en.wikipedia.org/wiki/Domain_Name_System

namespaces.go

oidc.go

I keep the check server side because it's better from a security point of view.

restanrm · 2022-02-23T10:42:26Z

With this came to my mind that hostname validation is not done. @kradalby should we normalize the hostname received from the nodes ?

kradalby

With this came to my mind that hostname validation is not done. @kradalby should we normalize the hostname received from the nodes ?

Yes, but lets make an issue, so this can be tracked separately.

kradalby · 2022-02-23T12:07:28Z

CHANGELOG.md

 - Add API Key support
  - Enable remote control of `headscale` via CLI [docs](docs/remote-cli.md)
  - Enable HTTP API (beta, subject to change)
+- OIDC login will map user to an associated namespace.


When we have landed on how this work, we should elaborate here, and in the example config

Should I remove changelog modifications ?

namespaces.go

kradalby

I'll have a suggestion for the Changelog when I get to the airport, but all looks good now :)

kradalby · 2022-02-24T09:36:18Z

machine.go

+)
+
+const (
+	maxHostnameLength = 255


Actually, I just thought, is it the entire url that has the max? Or the segments between . ?

I just had a memory about some kubernetes namespace stuff.

This is mostly to raise this, I'm happy about it as is for now.

The Fully Qualified Domain Name has a maximum length of 255 and the labels (segments in between) have a maximum length of 63

Ah yes of course

kradalby

Lets do this and I'll approve :)

CHANGELOG.md

Co-authored-by: Kristoffer Dalby <kradalby@kradalby.no>

restanrm · 2022-02-24T19:04:42Z

Integrations tests seems to have a random component. One update on changelog was ok, the other was not…

kradalby · 2022-02-24T19:27:59Z

Integrations tests seems to have a random component. One update on changelog was ok, the other was not…

Sadly it's not random, but race conditions/sync issues in headscale.

I am continuously working on them, and have an draft with a couple of improvements. But for now, just rerunning it is the way 😖

kradalby · 2022-02-25T09:40:30Z

When #357 is in, can you resolve the conflicts? I think there will be some more, so you can wait for that one.

restanrm · 2022-02-25T09:55:50Z

When #357 is in, can you resolve the conflicts? I think there will be some more, so you can wait for that one.

Ok, I'll wait for #357

kradalby · 2022-02-25T10:02:05Z

Ok, I'll wait for #357

Merged now.

…mespace

restanrm added 5 commits February 22, 2022 19:59

feat(namespace): add normalization function for namespace

92ffac6

feat(oidc): bind email to namespace

0191ea9

feat: removing matchmap from headscale

717250a

chore: update formating

afd4a37

chore: update changelog

fe0b43e

restanrm requested review from juanfont and kradalby as code owners February 22, 2022 20:22

kradalby added the DO NOT MERGE label Feb 23, 2022

kradalby requested changes Feb 23, 2022

View reviewed changes

restanrm added 4 commits February 23, 2022 11:32

feat(namespace): add check function for namespace

45727db

fix(namespace): checknamespace name before actions

995731a

I keep the check server side because it's better from a security point of view.

fix(utils_test): fix namespace name

fcdbe7c

fix(namespaces_test): fix missing namespace name

cef0a2b

kradalby reviewed Feb 23, 2022

View reviewed changes

restanrm and others added 6 commits February 23, 2022 13:35

fix(namespace): remove name validation for destroy and get

7e4709c

feat: add strip_email_domain to normalization of namespace

4f1f235

feat: add length error if hostname too long

972bef1

chore: update formatting

0461166

fix: add valid test identified by linter

ae6a20e

Merge branch 'main' into feat-oidc-login-as-namespace

95453cb

kradalby reviewed Feb 24, 2022

View reviewed changes

Merge branch 'main' into feat-oidc-login-as-namespace

69f220f

kradalby reviewed Feb 24, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

kradalby removed the DO NOT MERGE label Feb 24, 2022

kradalby and others added 3 commits February 24, 2022 11:40

Merge branch 'main' into feat-oidc-login-as-namespace

aa50650

Update CHANGELOG.md

f9ce32f

Co-authored-by: Kristoffer Dalby <kradalby@kradalby.no>

Update CHANGELOG.md

47e8442

Co-authored-by: Kristoffer Dalby <kradalby@kradalby.no>

kradalby previously approved these changes Feb 24, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into feat-oidc-login-as-na…

b39faa1

…mespace

restanrm dismissed kradalby’s stale review via b39faa1 February 25, 2022 10:28

kradalby approved these changes Feb 25, 2022

View reviewed changes

kradalby merged commit b1bd17f into juanfont:main Feb 25, 2022

restanrm deleted the feat-oidc-login-as-namespace branch February 25, 2022 17:19

kev-the-dev mentioned this pull request May 5, 2023

registration fails silently with long machine hostname #1294

Closed

Uh oh!

Feat OIDC login as namespace #350

Feat OIDC login as namespace #350

Uh oh!

Conversation

restanrm commented Feb 22, 2022

Uh oh!

kradalby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

restanrm commented Feb 23, 2022

Uh oh!

kradalby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kradalby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kradalby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

restanrm commented Feb 24, 2022

Uh oh!

kradalby commented Feb 24, 2022

Uh oh!

kradalby commented Feb 25, 2022

Uh oh!

restanrm commented Feb 25, 2022

Uh oh!

kradalby commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants