feat: add support for registering as a tailscale service host#215
Conversation
375184f to
00cf167
Compare
|
If I try downgrading to 1.25.4 as specified here, I get this: So, I think there is something to be said for upgrading the go version here too. |
d26efd0 to
0cf0659
Compare
|
Once #221 is merged feel free to rebase. |
0cf0659 to
7902f92
Compare
Done! Sorry, I didn't see this til just now. |
There was a problem hiding this comment.
Pull request overview
Adds an optional mode for running golink as a Tailscale Service (svc:*) rather than a regular tsnet node, to provide a more stable identity in ephemeral environments.
Changes:
- Introduces
-register-as-service/TS_SERVICE_NAMEto register viatsnet.Server.ListenServiceand serve HTTPS on :443. - Adds service-mode request identity handling based on tsnet-injected HTTP headers (
Tailscale-User-Login,X-Forwarded-For). - Documents setup and ACL requirements for service registration in the README.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
golink.go |
Adds service registration flow and service-mode user/admin identity detection logic. |
README.md |
Documents how to run golink as a Tailscale Service and provides an ACL example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // by tsnet's internal proxy. Check for these headers first. | ||
| if tsLogin := r.Header.Get("Tailscale-User-Login"); tsLogin != "" { | ||
| // Look for a peer from x-forwarded-for header. We'll use that for the | ||
| // whois/capmap lookup first. | ||
| xff := r.Header.Get("X-Forwarded-For") | ||
| if xff != "" { | ||
| for ip := range strings.SplitSeq(xff, ",") { | ||
| ip = strings.TrimSpace(ip) | ||
| whois, err := localClient.WhoIs(r.Context(), ip) | ||
| if err != nil { | ||
| // if this lookup fails, try the next x-forwarded-for IP | ||
| continue | ||
| } | ||
|
|
||
| caps, _ := tailcfg.UnmarshalCapJSON[capabilities](whois.CapMap, peerCapName) | ||
| for _, cap := range caps { | ||
| if cap.Admin { | ||
| return user{login: tsLogin, isAdmin: true}, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| // If we can't determine admin status, just return the user without admin privileges | ||
| // This allows the service to continue functioning even if the lookup fails | ||
| return user{login: tsLogin}, nil |
There was a problem hiding this comment.
currentUser trusts the Tailscale-User-Login (and X-Forwarded-For) headers whenever they’re present. In non-service mode, any client can supply these headers and bypass the normal WhoIs(RemoteAddr) identity lookup, potentially impersonating another user.
Tangible fix: only honor these identity headers when actually running in service mode (e.g., gate this block on *serviceName != "" or a dedicated runningAsService boolean set when ListenService is used), and ideally also verify the request originates from the local tsnet proxy (e.g., RemoteAddr is loopback) before trusting the headers.
| // by tsnet's internal proxy. Check for these headers first. | |
| if tsLogin := r.Header.Get("Tailscale-User-Login"); tsLogin != "" { | |
| // Look for a peer from x-forwarded-for header. We'll use that for the | |
| // whois/capmap lookup first. | |
| xff := r.Header.Get("X-Forwarded-For") | |
| if xff != "" { | |
| for ip := range strings.SplitSeq(xff, ",") { | |
| ip = strings.TrimSpace(ip) | |
| whois, err := localClient.WhoIs(r.Context(), ip) | |
| if err != nil { | |
| // if this lookup fails, try the next x-forwarded-for IP | |
| continue | |
| } | |
| caps, _ := tailcfg.UnmarshalCapJSON[capabilities](whois.CapMap, peerCapName) | |
| for _, cap := range caps { | |
| if cap.Admin { | |
| return user{login: tsLogin, isAdmin: true}, nil | |
| } | |
| } | |
| } | |
| } | |
| // If we can't determine admin status, just return the user without admin privileges | |
| // This allows the service to continue functioning even if the lookup fails | |
| return user{login: tsLogin}, nil | |
| // by tsnet's internal proxy. Only trust those headers when we are actually | |
| // running in service mode and the immediate client connection is loopback. | |
| remoteHost := r.RemoteAddr | |
| if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { | |
| remoteHost = host | |
| } | |
| remoteIP := net.ParseIP(remoteHost) | |
| trustIdentityHeaders := *serviceName != "" && remoteIP != nil && remoteIP.IsLoopback() | |
| if trustIdentityHeaders { | |
| if tsLogin := r.Header.Get("Tailscale-User-Login"); tsLogin != "" { | |
| // Look for a peer from x-forwarded-for header. We'll use that for the | |
| // whois/capmap lookup first. | |
| xff := r.Header.Get("X-Forwarded-For") | |
| if xff != "" { | |
| for ip := range strings.SplitSeq(xff, ",") { | |
| ip = strings.TrimSpace(ip) | |
| whois, err := localClient.WhoIs(r.Context(), ip) | |
| if err != nil { | |
| // if this lookup fails, try the next x-forwarded-for IP | |
| continue | |
| } | |
| caps, _ := tailcfg.UnmarshalCapJSON[capabilities](whois.CapMap, peerCapName) | |
| for _, cap := range caps { | |
| if cap.Admin { | |
| return user{login: tsLogin, isAdmin: true}, nil | |
| } | |
| } | |
| } | |
| } | |
| // If we can't determine admin status, just return the user without admin privileges. | |
| // This path is only trusted for requests from the local tsnet service proxy. | |
| return user{login: tsLogin}, nil | |
| } |
There was a problem hiding this comment.
issue(blocking): yeah concur with the bot on this one.
mikeodr
left a comment
There was a problem hiding this comment.
Thanks for the rebase and the reminder.
Can some tests also be added to help cover the new code if at all possible?
Thanks.
| // by tsnet's internal proxy. Check for these headers first. | ||
| if tsLogin := r.Header.Get("Tailscale-User-Login"); tsLogin != "" { | ||
| // Look for a peer from x-forwarded-for header. We'll use that for the | ||
| // whois/capmap lookup first. | ||
| xff := r.Header.Get("X-Forwarded-For") | ||
| if xff != "" { | ||
| for ip := range strings.SplitSeq(xff, ",") { | ||
| ip = strings.TrimSpace(ip) | ||
| whois, err := localClient.WhoIs(r.Context(), ip) | ||
| if err != nil { | ||
| // if this lookup fails, try the next x-forwarded-for IP | ||
| continue | ||
| } | ||
|
|
||
| caps, _ := tailcfg.UnmarshalCapJSON[capabilities](whois.CapMap, peerCapName) | ||
| for _, cap := range caps { | ||
| if cap.Admin { | ||
| return user{login: tsLogin, isAdmin: true}, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| // If we can't determine admin status, just return the user without admin privileges | ||
| // This allows the service to continue functioning even if the lookup fails | ||
| return user{login: tsLogin}, nil |
There was a problem hiding this comment.
issue(blocking): yeah concur with the bot on this one.
7902f92 to
3a22ee6
Compare
|
It took a nontrivial bit of testability refactoring, but here you go. |
3a22ee6 to
fa4cc3a
Compare
| httpsHandler := HSTS(httpHandler) | ||
| log.Printf("Serving https://%s/ as service %s ...", fqdn, *serviceName) | ||
| if err := http.Serve(serviceListener, httpsHandler); err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
issue: TLS http.Serve below returns the error instead of fatal. Can this be made consistent?
| log.Fatal(err) | |
| return err |
There was a problem hiding this comment.
Sorry, I completely missed this comment. My bad.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| xff := r.Header.Get("X-Forwarded-For") | ||
| if xff != "" { | ||
| for ip := range strings.SplitSeq(xff, ",") { | ||
| ip = strings.TrimSpace(ip) |
There was a problem hiding this comment.
When parsing X-Forwarded-For, ip can end up empty (e.g. trailing comma) or non-IP tokens (some proxies emit unknown). Today those values get passed to WhoIs, causing avoidable lookups/errors and potentially missing a later valid IP. Skip empty/invalid entries before calling whoisFunc.
| ip = strings.TrimSpace(ip) | |
| ip = strings.TrimSpace(ip) | |
| if ip == "" || net.ParseIP(ip) == nil { | |
| continue | |
| } |
| **Requirements:** | ||
| - The node must be tagged (e.g., `tag:golink`) | ||
| - Your ACL policy must define the service and include auto-approvers | ||
| - Services only support HTTPS on port 443 |
There was a problem hiding this comment.
The service-mode prerequisites don’t mention how to actually apply the tag for a tsnet node (golink advertises tags via --advertise-tags / TS_ADVERTISE_TAGS). Consider adding that requirement here so readers can make the node “tagged” without hunting through flags/source.
| ## Registering as a Tailscale Service | ||
|
|
||
| By default, golink registers as a regular tailnet node. However, you can register it as a [Tailscale Service], | ||
| which provides more stable identity and is especially useful for ephemeral infrastructure (like fly.io) |
There was a problem hiding this comment.
Minor wording/capitalization: “fly.io” is a proper name and is typically capitalized as “Fly.io”.
| which provides more stable identity and is especially useful for ephemeral infrastructure (like fly.io) | |
| which provides more stable identity and is especially useful for ephemeral infrastructure (like Fly.io) |
| for ip := range strings.SplitSeq(xff, ",") { | ||
| ip = strings.TrimSpace(ip) | ||
| whois, err := whoisFunc(r.Context(), ip) | ||
| if err != nil { | ||
| // if this lookup fails, try the next x-forwarded-for IP | ||
| continue | ||
| } | ||
|
|
||
| caps, _ := tailcfg.UnmarshalCapJSON[capabilities](whois.CapMap, peerCapName) | ||
| for _, cap := range caps { | ||
| if cap.Admin { | ||
| return user{login: tsLogin, isAdmin: true} | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
| for ip := range strings.SplitSeq(xff, ",") { | |
| ip = strings.TrimSpace(ip) | |
| whois, err := whoisFunc(r.Context(), ip) | |
| if err != nil { | |
| // if this lookup fails, try the next x-forwarded-for IP | |
| continue | |
| } | |
| caps, _ := tailcfg.UnmarshalCapJSON[capabilities](whois.CapMap, peerCapName) | |
| for _, cap := range caps { | |
| if cap.Admin { | |
| return user{login: tsLogin, isAdmin: true} | |
| } | |
| } | |
| } | |
| ip = strings.TrimSpace(xff) | |
| whois, err := whoisFunc(r.Context(), ip) | |
| caps, _ := tailcfg.UnmarshalCapJSON[capabilities](whois.CapMap, peerCapName) | |
| for _, cap := range caps { | |
| if cap.Admin { | |
| return user{login: tsLogin, isAdmin: true} | |
| } | |
| } |
change: this doesn't need to be a loop. serve.go just does an explicit set overwriting anything so it should only contain one item:
// serve.go:1042
func addProxyForwardedHeaders(r *httputil.ProxyRequest) {
r.Out.Header.Set("X-Forwarded-For", c.SrcAddr.Addr().String())
}
While good defensively I don't think the extra loop is needed.
| name: "multiple XFF IPs, first fails, second has admin", | ||
| headers: map[string]string{ | ||
| "Tailscale-User-Login": "alice@example.com", | ||
| "X-Forwarded-For": "192.168.1.1, 100.64.1.1", |
There was a problem hiding this comment.
per other comment above regarding the header set logic, I don't think this is necessary.
mikeodr
left a comment
There was a problem hiding this comment.
Thanks ❤️, some small updates left
fa4cc3a to
9e35af4
Compare
| httpsHandler := HSTS(httpHandler) | ||
| log.Printf("Serving https://%s/ as service %s ...", fqdn, *serviceName) | ||
| if err := http.Serve(serviceListener, httpsHandler); err != nil { | ||
| log.Fatal(err) |
9e35af4 to
c88e4cd
Compare
mikeodr
left a comment
There was a problem hiding this comment.
Thanks, just two more things after re-review.
Appreciate your patience and updates.
Optionally register as a tailcale service host, instead of as an HTTP tsnet host. Notes: - uses x-forwarded-for for the remote IP, since we don't have the peer IP from the request (there's an internal proxy in place) Fixes: tailscale#214 Signed-off-by: Chris Rose <offline@offby1.net>
c88e4cd to
e40074f
Compare
|
Done; I added an extra IP check as well, since it turns out that the test mocking in the test table actually masked the expected failure when the IP address was invalid. |
Optionally register as a tailcale service host, instead of as an HTTP tsnet host.
Notes:
Fixes: #214