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.

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

Add DERP generate_204 endpoint for captive portal detection. #1600

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tomtom5152
Copy link

@tomtom5152 tomtom5152 commented Nov 14, 2023

  • 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

When connecting to a DERP server the client makes a request to /generate_204 endpoint with the header X-Tailscale-Challenge. In order to pass the captive portal test, the server must provider a 204 NoContent response with X-Tailscale-Response set.

The source for this simple endpoint was taken directly from Derper source at the time of commit

@kradalby
Copy link
Collaborator

Is there a way we can have an integration test for this?

@kradalby
Copy link
Collaborator

kradalby commented Dec 9, 2023

@tomtom5152 Could you rebase this?

@tomtom5152
Copy link
Author

Sorry, only just seen this as been having to put my attentions elsewhere. I don't have time to rebase currently, but I see there was some force push just. I also looked at adding tests but couldn't figure out how best to do this with the current repo structure. If you've got any hints I might hopefully, maybe have some time to look at it again after this coming week.

@kradalby
Copy link
Collaborator

hmm, I wonder if I have seen an error message referring to the generate_204 endpoint when running tailscale debug derp <region>, so one option could be to setup a integration test (integration/ folder, look at embedded_derp_test.go):

  • Set up headscale with this new endpoint
  • Setup a tailscale client
  • execute tailscale debug derp 999
  • unmarshal it (there is probably a type in the tailscale source code you can use)
  • check if there is a warning/err for the generate_204 interface

I would appreciate if you have a go, if not, at somepoint I might have time, but that could be weeks to months these days.

I greatly appreciate this addition, but it is a feature that is hard to debug if it is wrong, so I am extra conservative for these kind of things.

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
Copy link
Collaborator

I've added a testcase as I've described for endpoint, it is currently failing, and I have not had time to investigate, feel free to take a look @tomtom5152.

cc @TotoTheDragon since we talked about this the other day.

@kradalby
Copy link
Collaborator

The DERP report looks as follows:

{
         "Info": [
          "Region 999 == \"headscale\""
         ],
         "Warnings": [
          "Having only a single DERP region (i.e. removing the default Tailscale-provided regions) is a single point of failure and could hamper connectivity",
          "Error making request to the captive portal check \"http://headscale/generate_204\"; is port 80 blocked?"
         ],
         "Errors": [
          "Error connecting to node \"headscale\" @ \"headscale:80\" over IPv4: dial tcp4: lookup headscale on 100.100.100.100:53: no such host",
          "Error connecting to node \"headscale\" @ \"headscale:80\" over IPv6: dial tcp6: lookup headscale on 100.100.100.100:53: no such host"
         ]
}

I recon we might need to listen specifically on port 80 for that endpoint.

@kradalby kradalby mentioned this pull request Feb 16, 2024
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@TotoTheDragon
Copy link
Contributor

I am not sure if the tailscale client actually uses this. But within DERPNode, there is a field called CanPort80. Maybe we should look into setting that field?

@kradalby
Copy link
Collaborator

I am not sure if the tailscale client actually uses this. But within DERPNode, there is a field called CanPort80. Maybe we should look into setting that field?

It sounds useful, but searching the clients code it doesnt look like it is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants