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 2 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
11 changes: 5 additions & 6 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
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 := NormalizeNamespaceName(
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.

We should probably generalise the name of this function now.

Copy link
Contributor

@e-zk e-zk Mar 5, 2022

Choose a reason for hiding this comment

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

Perhaps make them two different functions? NormalizeHostname and NormalizeNamespaceName?

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.

I'm in favor of something generic since hostname and namespace are both part of a FQDN. So NormalizeName or NormalizeDomainName or NormalizeHostname should work too.
I think I'll go with NormalizeName if that's ok with you.

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.

I think FQDN makes sense to have in the name, as that is what it is normalising for

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