From 2aebd2927dab9793f5efb693e0678796c6c95f4e Mon Sep 17 00:00:00 2001 From: = <=> Date: Wed, 31 Aug 2022 12:41:01 +0100 Subject: [PATCH] Random suffix only on collision. 0.16.0 introduced random suffixes to all machine given names (DNS hostnames) regardless of collisions within a namespace. This commit brings Headscale more inline with Tailscale by only adding a suffix if the hostname will collide within the namespace. The suffix generation differs from Tailscale. See https://tailscale.com/kb/1098/machine-names/ --- CHANGELOG.md | 1 + grpcv1.go | 4 +- machine.go | 64 ++++++++++++++++------ machine_test.go | 130 ++++++++++++++++++++++++++++++++++----------- protocol_common.go | 4 +- 5 files changed, 154 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5080aaf..911edf09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - Add support for generating pre-auth keys with tags [#767](https://github.com/juanfont/headscale/pull/767) - Add support for evaluating `autoApprovers` ACL entries when a machine is registered [#763](https://github.com/juanfont/headscale/pull/763) - Add config flag to allow Headscale to start if OIDC provider is down [#829](https://github.com/juanfont/headscale/pull/829) +- Random node DNS suffix only applied if names collide in namespace. [#766](https://github.com/juanfont/headscale/issues/766) ## 0.16.4 (2022-08-21) diff --git a/grpcv1.go b/grpcv1.go index 4465b720..7cb46beb 100644 --- a/grpcv1.go +++ b/grpcv1.go @@ -1,4 +1,4 @@ -//nolint +// nolint package headscale import ( @@ -479,7 +479,7 @@ func (api headscaleV1APIServer) DebugCreateMachine( Hostname: "DebugTestMachine", } - givenName, err := api.h.GenerateGivenName(request.GetName()) + givenName, err := api.h.GenerateGivenName(namespace.Name, request.GetKey(), request.GetName()) if err != nil { return nil, err } diff --git a/machine.go b/machine.go index 92fca36a..2e4f9caa 100644 --- a/machine.go +++ b/machine.go @@ -348,6 +348,22 @@ func (h *Headscale) GetMachine(namespace string, name string) (*Machine, error) return nil, ErrMachineNotFound } +// GetMachineByGivenName finds a Machine by given name and namespace and returns the Machine struct. +func (h *Headscale) GetMachineByGivenName(namespace string, givenName string) (*Machine, error) { + machines, err := h.ListMachinesInNamespace(namespace) + if err != nil { + return nil, err + } + + for _, m := range machines { + if m.GivenName == givenName { + return &m, nil + } + } + + return nil, ErrMachineNotFound +} + // GetMachineByID finds a Machine by ID and returns the Machine struct. func (h *Headscale) GetMachineByID(id uint64) (*Machine, error) { m := Machine{} @@ -1018,11 +1034,7 @@ func (machine *Machine) RoutesToProto() *v1.Routes { } } -func (h *Headscale) GenerateGivenName(suppliedName string) (string, error) { - // If a hostname is or will be longer than 63 chars after adding the hash, - // it needs to be trimmed. - trimmedHostnameLength := labelHostnameLength - MachineGivenNameHashLength - MachineGivenNameTrimSize - +func (h *Headscale) generateGivenName(suppliedName string, randomSuffix bool) (string, error) { normalizedHostname, err := NormalizeToFQDNRules( suppliedName, h.cfg.OIDC.StripEmaildomain, @@ -1031,18 +1043,40 @@ func (h *Headscale) GenerateGivenName(suppliedName string) (string, error) { return "", err } - postfix, err := GenerateRandomStringDNSSafe(MachineGivenNameHashLength) - if err != nil { - return "", err - } + if randomSuffix { + // Trim if a hostname will be longer than 63 chars after adding the hash. + trimmedHostnameLength := labelHostnameLength - MachineGivenNameHashLength - MachineGivenNameTrimSize + if len(normalizedHostname) > trimmedHostnameLength { + normalizedHostname = normalizedHostname[:trimmedHostnameLength] + } - // Verify that that the new unique name is shorter than the maximum allowed - // DNS segment. - if len(normalizedHostname) <= trimmedHostnameLength { - normalizedHostname = fmt.Sprintf("%s-%s", normalizedHostname, postfix) - } else { - normalizedHostname = fmt.Sprintf("%s-%s", normalizedHostname[:trimmedHostnameLength], postfix) + suffix, err := GenerateRandomStringDNSSafe(MachineGivenNameHashLength) + if err != nil { + return "", err + } + + normalizedHostname += "-" + suffix } return normalizedHostname, nil } + +func (h *Headscale) GenerateGivenName(namespace string, machineKey string, suppliedName string) (string, error) { + givenName, err := h.generateGivenName(suppliedName, false) + if err != nil { + return "", err + } + + // Tailscale rules (may differ) https://tailscale.com/kb/1098/machine-names/ + machine, _ := h.GetMachineByGivenName(namespace, givenName) + if machine != nil && machine.MachineKey != machineKey && machine.GivenName == givenName { + postfixedName, err := h.generateGivenName(suppliedName, true) + if err != nil { + return "", err + } + + givenName = postfixedName + } + + return givenName, nil +} diff --git a/machine_test.go b/machine_test.go index 275ab14e..0365636f 100644 --- a/machine_test.go +++ b/machine_test.go @@ -4,8 +4,8 @@ import ( "fmt" "net/netip" "reflect" + "regexp" "strconv" - "strings" "testing" "time" @@ -346,6 +346,53 @@ func (s *Suite) TestSerdeAddressStrignSlice(c *check.C) { } } +func (s *Suite) TestGenerateGivenName(c *check.C) { + namespace1, err := app.CreateNamespace("namespace-1") + c.Assert(err, check.IsNil) + + namespace2, err := app.CreateNamespace("namespace-2") + c.Assert(err, check.IsNil) + + pak, err := app.CreatePreAuthKey(namespace1.Name, false, false, nil) + c.Assert(err, check.IsNil) + + _, err = app.GetMachine("namespace-1", "testmachine") + c.Assert(err, check.NotNil) + + machine := &Machine{ + ID: 0, + MachineKey: "machine-key-1", + NodeKey: "node-key-1", + DiscoKey: "disco-key-1", + Hostname: "hostname-1", + GivenName: "hostname-1", + NamespaceID: namespace1.ID, + RegisterMethod: RegisterMethodAuthKey, + AuthKeyID: uint(pak.ID), + } + app.db.Save(machine) + + givenName, err := app.GenerateGivenName(namespace1.Name, "machine-key-2", "hostname-2") + comment := check.Commentf("Same namespace, unique machines, unique hostnames, no conflict") + c.Assert(err, check.IsNil, comment) + c.Assert(givenName, check.Equals, "hostname-2", comment) + + givenName, err = app.GenerateGivenName(namespace1.Name, "machine-key-1", "hostname-1") + comment = check.Commentf("Same namespace, same machine, same hostname, no conflict") + c.Assert(err, check.IsNil, comment) + c.Assert(givenName, check.Equals, "hostname-1", comment) + + givenName, err = app.GenerateGivenName(namespace1.Name, "machine-key-2", "hostname-1") + comment = check.Commentf("Same namespace, unique machines, same hostname, conflict") + c.Assert(err, check.IsNil, comment) + c.Assert(givenName, check.Matches, fmt.Sprintf("^hostname-1-[a-z0-9]{%d}$", MachineGivenNameHashLength), comment) + + givenName, err = app.GenerateGivenName(namespace2.Name, "machine-key-2", "hostname-1") + comment = check.Commentf("Unique namespaces, unique machines, same hostname, no conflict") + c.Assert(err, check.IsNil, comment) + c.Assert(givenName, check.Equals, "hostname-1", comment) +} + func (s *Suite) TestSetTags(c *check.C) { namespace, err := app.CreateNamespace("test") c.Assert(err, check.IsNil) @@ -917,15 +964,16 @@ func Test_getFilteredByACLPeers(t *testing.T) { } } -func TestHeadscale_GenerateGivenName(t *testing.T) { +func TestHeadscale_generateGivenName(t *testing.T) { type args struct { suppliedName string + randomSuffix bool } tests := []struct { name string h *Headscale args args - want string + want *regexp.Regexp wantErr bool }{ { @@ -939,8 +987,9 @@ func TestHeadscale_GenerateGivenName(t *testing.T) { }, args: args{ suppliedName: "testmachine", + randomSuffix: false, }, - want: "testmachine", + want: regexp.MustCompile("^testmachine$"), wantErr: false, }, { @@ -954,23 +1003,9 @@ func TestHeadscale_GenerateGivenName(t *testing.T) { }, args: args{ suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine", + randomSuffix: false, }, - want: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine", - wantErr: false, - }, - { - name: "machine name with 60 chars", - h: &Headscale{ - cfg: &Config{ - OIDC: OIDCConfig{ - StripEmaildomain: true, - }, - }, - }, - args: args{ - suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine1234567", - }, - want: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine", + want: regexp.MustCompile("^testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine$"), wantErr: false, }, { @@ -983,9 +1018,10 @@ func TestHeadscale_GenerateGivenName(t *testing.T) { }, }, args: args{ - suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine1234567890", + suppliedName: "machineeee12345678901234567890123456789012345678901234567890123", + randomSuffix: false, }, - want: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + want: regexp.MustCompile("^machineeee12345678901234567890123456789012345678901234567890123$"), wantErr: false, }, { @@ -998,10 +1034,11 @@ func TestHeadscale_GenerateGivenName(t *testing.T) { }, }, args: args{ - suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine1234567891", + suppliedName: "machineeee123456789012345678901234567890123456789012345678901234", + randomSuffix: false, }, - want: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - wantErr: false, + want: nil, + wantErr: true, }, { name: "machine name with 73 chars", @@ -1013,15 +1050,48 @@ func TestHeadscale_GenerateGivenName(t *testing.T) { }, }, args: args{ - suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine12345678901234567890", + suppliedName: "machineeee123456789012345678901234567890123456789012345678901234567890123", + randomSuffix: false, }, - want: "", + want: nil, wantErr: true, }, + { + name: "machine name with random suffix", + h: &Headscale{ + cfg: &Config{ + OIDC: OIDCConfig{ + StripEmaildomain: true, + }, + }, + }, + args: args{ + suppliedName: "test", + randomSuffix: true, + }, + want: regexp.MustCompile(fmt.Sprintf("^test-[a-z0-9]{%d}$", MachineGivenNameHashLength)), + wantErr: false, + }, + { + name: "machine name with 63 chars with random suffix", + h: &Headscale{ + cfg: &Config{ + OIDC: OIDCConfig{ + StripEmaildomain: true, + }, + }, + }, + args: args{ + suppliedName: "machineeee12345678901234567890123456789012345678901234567890123", + randomSuffix: true, + }, + want: regexp.MustCompile(fmt.Sprintf("^machineeee1234567890123456789012345678901234567890123-[a-z0-9]{%d}$", MachineGivenNameHashLength)), + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := tt.h.GenerateGivenName(tt.args.suppliedName) + got, err := tt.h.generateGivenName(tt.args.suppliedName, tt.args.randomSuffix) if (err != nil) != tt.wantErr { t.Errorf( "Headscale.GenerateGivenName() error = %v, wantErr %v", @@ -1032,9 +1102,9 @@ func TestHeadscale_GenerateGivenName(t *testing.T) { return } - if tt.want != "" && strings.Contains(tt.want, got) { + if tt.want != nil && !tt.want.MatchString(got) { t.Errorf( - "Headscale.GenerateGivenName() = %v, is not a substring of %v", + "Headscale.GenerateGivenName() = %v, does not match %v", tt.want, got, ) diff --git a/protocol_common.go b/protocol_common.go index 465279aa..42d413ef 100644 --- a/protocol_common.go +++ b/protocol_common.go @@ -150,7 +150,7 @@ func (h *Headscale) handleRegisterCommon( Bool("noise", machineKey.IsZero()). Msg("New machine not yet in the database") - givenName, err := h.GenerateGivenName(registerRequest.Hostinfo.Hostname) + givenName, err := h.GenerateGivenName(machine.Namespace.Name, machine.MachineKey, registerRequest.Hostinfo.Hostname) if err != nil { log.Error(). Caller(). @@ -374,7 +374,7 @@ func (h *Headscale) handleAuthKeyCommon( } else { now := time.Now().UTC() - givenName, err := h.GenerateGivenName(registerRequest.Hostinfo.Hostname) + givenName, err := h.GenerateGivenName(machine.Namespace.Name, MachinePublicKeyStripPrefix(machineKey), registerRequest.Hostinfo.Hostname) if err != nil { log.Error(). Caller().