Skip to content

Fix magic dns and uppercase letters #387

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

Merged
kradalby merged 8 commits into from
Mar 8, 2022
Merged

Fix magic dns and uppercase letters #387

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
44a5372
fix(poll): Normalize hostname
restanrm Mar 3, 2022
6f172a6
fix(acls): remove dead error code
restanrm Mar 3, 2022
dcf3ea5
Merge branch 'main' into fix-magic-dns-and-uppercase-letters
juanfont Mar 6, 2022
1114449
change: update name of method to check and normalize Domain name
restanrm Mar 6, 2022
6cc8bbc
feat(api): add normalisation at machine register step
restanrm Mar 7, 2022
f19c048
fix: change normalization function name
restanrm Mar 7, 2022
2b68c90
chore: update changelog
restanrm Mar 7, 2022
41efe98
fix: apply fmt and fix missing name changes
restanrm Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions acls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,11 @@ import (
)

const (
errEmptyPolicy = Error("empty policy")
errInvalidAction = Error("invalid action")
errInvalidUserSection = Error("invalid user section")
errInvalidGroup = Error("invalid group")
errInvalidTag = Error("invalid tag")
errInvalidPortFormat = Error("invalid port format")
errEmptyPolicy = Error("empty policy")
errInvalidAction = Error("invalid action")
errInvalidGroup = Error("invalid group")
errInvalidTag = Error("invalid tag")
errInvalidPortFormat = Error("invalid port format")
)

const (
Expand Down Expand Up @@ -445,7 +444,7 @@ func expandGroup(
errInvalidGroup,
)
}
grp, err := NormalizeNamespaceName(group, stripEmailDomain)
grp, err := NormalizeName(group, stripEmailDomain)
Copy link
Collaborator

@kradalby kradalby Mar 7, 2022

Choose a reason for hiding this comment

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

I think as mentioned this is a bit to generic, for a unknown user to understand the context, it needs to either have FQDN or Domain or some reference to the RFC and limitations in the name.

if err != nil {
return []string{}, fmt.Errorf(
"failed to normalize group %q, err: %w",
Expand Down
14 changes: 7 additions & 7 deletions namespaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type Namespace struct {
// CreateNamespace creates a new Namespace. Returns error if could not be created
// or another namespace already exists.
func (h *Headscale) CreateNamespace(name string) (*Namespace, error) {
err := CheckNamespaceName(name)
err := CheckName(name)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -104,7 +104,7 @@ func (h *Headscale) RenameNamespace(oldName, newName string) error {
if err != nil {
return err
}
err = CheckNamespaceName(newName)
err = CheckName(newName)
if err != nil {
return err
}
Expand Down Expand Up @@ -150,7 +150,7 @@ func (h *Headscale) ListNamespaces() ([]Namespace, error) {

// ListMachinesInNamespace gets all the nodes in a given namespace.
func (h *Headscale) ListMachinesInNamespace(name string) ([]Machine, error) {
err := CheckNamespaceName(name)
err := CheckName(name)
if err != nil {
return nil, err
}
Expand All @@ -169,7 +169,7 @@ func (h *Headscale) ListMachinesInNamespace(name string) ([]Machine, error) {

// SetMachineNamespace assigns a Machine to a namespace.
func (h *Headscale) SetMachineNamespace(machine *Machine, namespaceName string) error {
err := CheckNamespaceName(namespaceName)
err := CheckName(namespaceName)
if err != nil {
return err
}
Expand Down Expand Up @@ -237,9 +237,9 @@ func (n *Namespace) toProto() *v1.Namespace {
}
}

// NormalizeNamespaceName will replace forbidden chars in namespace
// NormalizeName will replace forbidden chars in namespace
// it can also return an error if the namespace doesn't respect RFC 952 and 1123.
func NormalizeNamespaceName(name string, stripEmailDomain bool) (string, error) {
func NormalizeName(name string, stripEmailDomain bool) (string, error) {
name = strings.ToLower(name)
name = strings.ReplaceAll(name, "'", "")
atIdx := strings.Index(name, "@")
Expand All @@ -263,7 +263,7 @@ func NormalizeNamespaceName(name string, stripEmailDomain bool) (string, error)
return name, nil
}

func CheckNamespaceName(name string) error {
func CheckName(name string) error {
if len(name) > labelHostnameLength {
return fmt.Errorf(
"Namespace must not be over 63 chars. %v doesn't comply with this rule: %w",
Expand Down
8 changes: 4 additions & 4 deletions namespaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ func (s *Suite) TestGetMapResponseUserProfiles(c *check.C) {
c.Assert(found, check.Equals, true)
}

func TestNormalizeNamespaceName(t *testing.T) {
func TestNormalizeName(t *testing.T) {
type args struct {
name string
stripEmailDomain bool
Expand Down Expand Up @@ -310,7 +310,7 @@ func TestNormalizeNamespaceName(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := NormalizeNamespaceName(tt.args.name, tt.args.stripEmailDomain)
got, err := NormalizeName(tt.args.name, tt.args.stripEmailDomain)
if (err != nil) != tt.wantErr {
t.Errorf(
"NormalizeNamespaceName() error = %v, wantErr %v",
Expand All @@ -327,7 +327,7 @@ func TestNormalizeNamespaceName(t *testing.T) {
}
}

func TestCheckNamespaceName(t *testing.T) {
func TestCheckName(t *testing.T) {
type args struct {
name string
}
Expand Down Expand Up @@ -366,7 +366,7 @@ func TestCheckNamespaceName(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if err := CheckNamespaceName(tt.args.name); (err != nil) != tt.wantErr {
if err := CheckName(tt.args.name); (err != nil) != tt.wantErr {
t.Errorf("CheckNamespaceName() error = %v, wantErr %v", err, tt.wantErr)
}
})
Expand Down
2 changes: 1 addition & 1 deletion oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ func (h *Headscale) OIDCCallback(ctx *gin.Context) {
return
}

namespaceName, err := NormalizeNamespaceName(
namespaceName, err := NormalizeName(
claims.Email,
h.cfg.OIDC.StripEmaildomain,
)
Expand Down
13 changes: 12 additions & 1 deletion poll.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,18 @@ func (h *Headscale) PollNetMapHandler(ctx *gin.Context) {
Str("machine", machine.Name).
Msg("Found machine in database")

machine.Name = req.Hostinfo.Hostname
hname, err := NormalizeName(
req.Hostinfo.Hostname,
h.cfg.OIDC.StripEmaildomain,
)
if err != nil {
log.Error().
Caller().
Str("func", "handleAuthKey").
Str("hostinfo.name", req.Hostinfo.Hostname).
Err(err)
}
machine.Name = hname
Copy link
Collaborator

@kradalby kradalby Mar 5, 2022

Choose a reason for hiding this comment

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

hmm, doing this here feels kind of wrong, I think doing it here:
https://github.com/juanfont/headscale/blob/main/api.go#L144

Would be more correct, as this is when the machine registers.

However we need a migration path. It could potentially be done in the DB migration, what do you think?

We should probably also make a tracking ticket of a unique constraint for a machine name per user, so we dont have collisions.

Copy link
Contributor Author

@restanrm restanrm Mar 6, 2022

Choose a reason for hiding this comment

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

You're right, that was my first intention. When I found this solution I thought that we could get rid of the need of a database migration, but the --hostname at tailscale up wouldn't work at all.

For the machine name per user I think it would be more a unique name on all the headscale instance. If we tag a machine, this machine should not be accessed by the name of the user that created it (it's currently not implemented).

Copy link
Contributor Author

@restanrm restanrm Mar 6, 2022

Choose a reason for hiding this comment

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

After re-reading the code. What should we do in this place ? If we keep previous behavior, normalized hostname would be replaced. If we modify in https://github.com/juanfont/headscale/blob/main/api.go#L144 we would not be able to update the hostname with tailscale up --hostname since at line

headscale/api.go

Line 126 in 435ee36

if errors.Is(err, gorm.ErrRecordNotFound) {
we check that we are only in this section if the machine doesn't already exists.

Copy link
Collaborator

@kradalby kradalby Mar 6, 2022

Choose a reason for hiding this comment

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

We could migrate all of them on server start up as part of db.go, but that would run everytime :/

Copy link
Contributor Author

@restanrm restanrm Mar 7, 2022

Choose a reason for hiding this comment

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

After some tests each time we update the hostname with tailscale up --hostname xxx a call to /machine/:id/map is done. We can also observe the following logs in the tailscaled daemon:

cmd-prometheus1-1  | 2022/03/07 08:49:11 EditPrefs: MaskedPrefs{ControlURL="https://my.redacted.headscale.domain" WantRunning=true Hostname="prometheus2"}
cmd-prometheus1-1  | 2022/03/07 08:49:11 control: HostInfo: {"IPNVersion":"1.22.0-t4e0b00ad8","BackendLogID":"3a5dce2711b4137729be89a95f315ee9ac733cf3a55dd2555c31aac5ab93a328","OS":"linux","OSVersion":"Alpine Linux v3.15; kernel=5.15.2-arch1-1","Hostname":"prometheus2","GoArch":"amd64","Services":[{"Proto":"peerapi4","Port":58436},{"Proto":"peerapi-dns-proxy","Port":1}],"NetInfo":{"MappingVariesByDestIP":false,"HairPinning":false,"WorkingIPv6":false,"WorkingUDP":true,"UPnP":false,"PMP":false,"PCP":false,"PreferredDERP":8,"DERPLatency":{"1-v4":0.088316282,"10-v4":0.158354527,"11-v4":0.237721537,"12-v4":0.099872237,"2-v4":0.15813595,"4-v4":0.026846961,"6-v4":0.224276797,"7-v4":0.254514052,"8-v4":0.018363134,"9-v4":0.121180425}}}
cmd-prometheus1-1  | 2022/03/07 08:49:11 control: PollNetMap: stream=false :0 ep=[90.27.176.219:60654 172.18.0.3:60654]
cmd-prometheus1-1  | 2022/03/07 08:49:11 wgengine: Reconfig done
cmd-prometheus1-1  | 2022/03/07 08:49:11 authReconfig: ra=false dns=true 0x01: <nil>
cmd-prometheus1-1  | 2022/03/07 08:49:11 [unexpected] peerapi listen("fd7a:115c:a1e0::1") error: listen tcp6 [fd7a:115c:a1e0::1]:0: bind: cannot assign requested address
cmd-prometheus1-1  | 2022/03/07 08:49:11 peerapi: serving on http://100.64.0.1:58436

I could not find solid proof in reasonable time from the code of tailscale that each modification of the preference would lead to a NetPollMap call, but it's very likely.

Copy link
Collaborator

@kradalby kradalby Mar 7, 2022

Choose a reason for hiding this comment

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

So we need it in both places?

Copy link
Contributor Author

@restanrm restanrm Mar 7, 2022

Choose a reason for hiding this comment

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

I think that would be better yes !

Copy link
Collaborator

@kradalby kradalby Mar 7, 2022

Choose a reason for hiding this comment

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

Lets give it a go then :)

Copy link
Contributor Author

@restanrm restanrm Mar 7, 2022

Choose a reason for hiding this comment

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

Ok, I'll made the modifications on the name of the function and add this code to both places tonight.

machine.HostInfo = HostInfo(*req.Hostinfo)
machine.DiscoKey = DiscoPublicKeyStripPrefix(req.DiscoKey)
now := time.Now().UTC()
Expand Down