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

feat: support client verify for derp #1957

Closed
wants to merge 1 commit into from

Conversation

117503445
Copy link
Contributor

@117503445 117503445 commented May 25, 2024

  • have read the CONTRIBUTING.md file
  • 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

I have initially implemented the verification function. Derp can verify whether the client is in the node list of headscale by specifying the --verify-client-url parameter to /verify in headscale.

I know there is still a lot of work to be done, such as comments, testing, documentation, etc. I am also willing to participate in the subsequent work.

Fixes #1953

@kradalby
Copy link
Collaborator

Nothing looks outrageous, but we need a way to test this, is there a way you can run a tailscale debug command or something and verify that it works or not? that should make it into a integration test.

hscontrol/handlers.go Outdated Show resolved Hide resolved
@@ -485,6 +485,8 @@ func (h *Headscale) createRouter(grpcMux *grpcRuntime.ServeMux) *mux.Router {
router.HandleFunc("/swagger/v1/openapiv2.json", headscale.SwaggerAPIv1).
Methods(http.MethodGet)

router.HandleFunc("/verify", h.VerifyHandler).Methods(http.MethodPost)
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe this should be under the derp router further down since it is only relevant if we run a derp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, helping other derps verify and running headscale build-in derp are two different things. Even without activating the built-in derp, headscale can still help other derps verify whether the client is in the nodelist.

In my own use case, my cloud server is more stable but has less bandwidth, while my homelab has more bandwidth but is often unavailable. Therefore, I install headscale on the cloud server but do not enable the built-in derp, and install derp in my homelab. At this point, I want the homelab's derp to access the cloud server's headscale via a verify-url to avoid unauthorized access to the derp.

@117503445 117503445 requested a review from ohdearaugustin as a code owner June 4, 2024 16:53
@117503445 117503445 force-pushed the 117503445/verify-client branch from fdb00de to 5c11b3b Compare June 13, 2024 02:57
@117503445
Copy link
Contributor Author

I encountered an unresolved issue.

In my testing, I created a DerpClient and DerpServer, setting the VerifyClientURL of the DerpServer to the Verify interface of Headscale. When a DerpClient created with a new privateKey attempts to connect to the DerpServer, the connection is rejected, which is the expected behavior.

However, I am unable to obtain the privateKey of the connected node, so I cannot create a legitimate DerpClient, resulting in a failure to pass the verification.

@kradalby
Copy link
Collaborator

Does that mean there is an issue in derp, tailscale client or headscale?

@117503445 117503445 force-pushed the 117503445/verify-client branch 2 times, most recently from 4117795 to c450e2f Compare June 21, 2024 13:28
@117503445 117503445 force-pushed the 117503445/verify-client branch from c450e2f to 89e6c40 Compare June 21, 2024 13:36
@117503445
Copy link
Contributor Author

Now that I've solved the problem of testing, PR Ready

@117503445
Copy link
Contributor Author

@kradalby If the code no longer requires modifications, I can begin writing the documentation and CHANGELOG.md. Thanks.

@badsmoke
Copy link

any news here? it would be fantastic if this feature made it into the next version

@ArcticLampyrid
Copy link
Contributor

I found this feature useful to me. Is there something I can help to push the process?

@117503445
Copy link
Contributor Author

I found this feature useful to me. Is there something I can help to push the process?

I compiled and used it on my own branch and had a good experience using it. I also hope this change will be merged.

@kradalby
Copy link
Collaborator

kradalby commented Aug 6, 2024

Im sorry, we are completely overrun, I have not had time to look carefully but it looks like this is still missing an integration test for the client.

The test implemented checks the web endpoint from a http request, which is also good, but for us to merge this, it needs to be verified using the Tailscale client, not our code.

I don't know how that should look, maybe a tailscale client not joined to the Headscale client (and therefore not authorised to use the derp) that fails.

We are not able to take on the burden of untested code.

@117503445
Copy link
Contributor Author

merged by #2046

@117503445 117503445 closed this Nov 23, 2024
@openapphub
Copy link

How to Use? It seems that the 0.24.0 document does not provide any information on this.

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.

[Feature] Support for derp's verify-client-url
5 participants