Skip to content
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

Jump to bottom

Fix duplicate nodes due to incorrect implementation of the protocol #1058

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Fix duplicate nodes due to incorrect implementation of the protocol #1058

merged 2 commits into from
Dec 21, 2022

Conversation

juanfont
Copy link
Owner

@juanfont juanfont commented Dec 9, 2022

Under certain conditions we could get duplicated nodes (e.g., after logout/login, reported in #1054) - due to a misunderstanding from my side on the role of MachineKey in TS2021.

MachineKey in TS2021 (the permanent-ish key associated to a client) is no longer sent in the body of the protocol requests, but obtained from the method Peer() in the controlbase.Conn struct. I was not aware of this (cheers @awsong for the heads up!). Our TS2021 was completely dropping MachineKey

Fixes #1054

@juanfont juanfont requested a review from kradalby as a code owner December 9, 2022 18:28
@juanfont juanfont force-pushed the machinekey-in-noise branch from 6bed35c to 7b7a5e0 Compare December 9, 2022 18:28
@awsong
Copy link

awsong commented Dec 10, 2022

Cheers to you @juanfont .

Thanks for the quick fix. Looking forward to trying it.

@awsong
Copy link

awsong commented Dec 12, 2022 •
edited
Loading

I tested latest Linux Tailscale client with your branch machinekey-in-noise. I could only login when Headscale DB is empty. Once I login and logout, next login will fail (tailscale up --login-server http://xxxx.com doesn't return).

@juanfont juanfont force-pushed the machinekey-in-noise branch from 12c8bec to 991abae Compare December 13, 2022 23:15
@juanfont
Copy link
Owner Author

@awsong can you describe the steps you followed? I am not being able to replicate your issue.

@awsong
Copy link

awsong commented Dec 14, 2022 •
edited
Loading

@awsong can you describe the steps you followed? I am not being able to replicate your issue.

Here's my steps:
Server side:

  1. git checkout origin/machinekey-in-noise
  2. go build -v cmd/headscale/headscale.go
  3. rm /var/lib/headscale/db.sqlite
  4. ./headscale serve

Client side:

  1. Compile latest linux CLI from source
  2. rm /var/lib/tailscale/ -rf
  3. ./tailscaled
  4. ./tailscale up --login-server http://raz.3fire.org:444 --reset , you can actually connect to this server, user name test, password VGVzdDEyMzQ1QA== (please base64 decode it)
  5. ./tailscale logout
  6. ./tailscale up --login-server http://raz.3fire.org:444 --reset

Now my server DB is empty, you can try to connect to it with user test

@juanfont
Copy link
Owner Author

@awsong I have managed to replicate it!

Looking at it...

@awsong
Copy link

awsong commented Dec 20, 2022

@awsong I have managed to replicate it!

Looking at it...

Tested your latest fix, now it works fine. Logout/login multiple times, IP stays the same, Headscale reports same machine key and different node key each time.

@juanfont
Copy link
Owner Author

@awsong thanks for checking out! It turned out to be a little bit harded to fix than I thought :)

I am now finishing integration tests for this, and then I will send it to @kradalby for review.

@juanfont juanfont force-pushed the machinekey-in-noise branch 2 times, most recently from 3da8e91 to cb8e601 Compare December 20, 2022 22:02
@evenh
Copy link
Contributor

evenh commented Dec 20, 2022

Looking forward to this fix 🙌🏻

kradalby
kradalby reviewed Dec 21, 2022
View reviewed changes
integration/auth_web_flow_test.go
}

for _, client := range allClients {
_, _, err = client.Execute([]string{"tailscale", "logout"})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make a nice .Logout() for this

@kradalby kradalby force-pushed the machinekey-in-noise branch from 108e755 to 946723f Compare December 21, 2022 18:53
kradalby
kradalby previously approved these changes Dec 21, 2022
View reviewed changes
@juanfont @kradalby
Run the Noise handlers under a new struct so we can access the noiseC…
ea5babc 
…onn from the handlers

In TS2021 the MachineKey can be obtained from noiseConn.Peer() - contrary to what I thought before,
where I assumed MachineKey was dropped in TS2021.

By having a ts2021App and hanging from there the TS2021 handlers, we can fetch again the MachineKey.
@juanfont @kradalby
Add integration tests that check logout and relogin
623f058
@kradalby kradalby force-pushed the machinekey-in-noise branch from 946723f to 623f058 Compare December 21, 2022 18:54
@juanfont juanfont merged commit b54c0e3 into main Dec 21, 2022
@juanfont juanfont deleted the machinekey-in-noise branch December 21, 2022 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kradalby kradalby kradalby approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Logout and login back got different IP address
4 participants
@juanfont @awsong @evenh @kradalby