Skip to content

Commit 1686819

Browse files
authored
fix double login URL with OIDC (#2445)
* factor out login url parser Signed-off-by: Kristoffer Dalby <[email protected]> * move to not trigger test gen checker Signed-off-by: Kristoffer Dalby <[email protected]> * return regresp or err after waiting for registration Signed-off-by: Kristoffer Dalby <[email protected]> * update changelog Signed-off-by: Kristoffer Dalby <[email protected]> --------- Signed-off-by: Kristoffer Dalby <[email protected]>
1 parent da2ca05 commit 1686819

File tree

8 files changed

+151
-26
lines changed

8 files changed

+151
-26
lines changed

CHANGELOG.md

+3-1
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,16 @@
1414
- View of config, policy, filter, ssh policy per node, connected nodes and
1515
DERPmap
1616

17-
## 0.25.1 (2025-02-24)
17+
## 0.25.1 (2025-02-25)
1818

1919
### Changes
2020

2121
- Fix issue where registration errors are sent correctly
2222
[#2435](https://github.com/juanfont/headscale/pull/2435)
2323
- Fix issue where routes passed on registration were not saved
2424
[#2444](https://github.com/juanfont/headscale/pull/2444)
25+
- Fix issue where registration page was displayed twice
26+
[#2445](https://github.com/juanfont/headscale/pull/2445)
2527

2628
## 0.25.0 (2025-02-11)
2729

hscontrol/auth.go

+19-13
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,7 @@ func (h *Headscale) handleRegister(
4343
}
4444

4545
if regReq.Followup != "" {
46-
// TODO(kradalby): Does this need to return an error of some sort?
47-
// Maybe if the registration fails down the line it can be sent
48-
// on the channel and returned here?
49-
h.waitForFollowup(ctx, regReq)
46+
return h.waitForFollowup(ctx, regReq)
5047
}
5148

5249
if regReq.Auth != nil && regReq.Auth.AuthKey != "" {
@@ -111,42 +108,51 @@ func (h *Headscale) handleExistingNode(
111108
h.nodeNotifier.NotifyWithIgnore(ctx, types.UpdateExpire(node.ID, requestExpiry), node.ID)
112109
}
113110

111+
return nodeToRegisterResponse(node), nil
112+
}
113+
114+
func nodeToRegisterResponse(node *types.Node) *tailcfg.RegisterResponse {
114115
return &tailcfg.RegisterResponse{
115116
// TODO(kradalby): Only send for user-owned nodes
116117
// and not tagged nodes when tags is working.
117118
User: *node.User.TailscaleUser(),
118119
Login: *node.User.TailscaleLogin(),
119-
NodeKeyExpired: expired,
120+
NodeKeyExpired: node.IsExpired(),
120121

121122
// Headscale does not implement the concept of machine authorization
122123
// so we always return true here.
123124
// Revisit this if #2176 gets implemented.
124125
MachineAuthorized: true,
125-
}, nil
126+
}
126127
}
127128

128129
func (h *Headscale) waitForFollowup(
129130
ctx context.Context,
130131
regReq tailcfg.RegisterRequest,
131-
) {
132+
) (*tailcfg.RegisterResponse, error) {
132133
fu, err := url.Parse(regReq.Followup)
133134
if err != nil {
134-
return
135+
return nil, NewHTTPError(http.StatusUnauthorized, "invalid followup URL", err)
135136
}
136137

137138
followupReg, err := types.RegistrationIDFromString(strings.ReplaceAll(fu.Path, "/register/", ""))
138139
if err != nil {
139-
return
140+
return nil, NewHTTPError(http.StatusUnauthorized, "invalid registration ID", err)
140141
}
141142

142143
if reg, ok := h.registrationCache.Get(followupReg); ok {
143144
select {
144145
case <-ctx.Done():
145-
return
146-
case <-reg.Registered:
147-
return
146+
return nil, NewHTTPError(http.StatusUnauthorized, "registration timed out", err)
147+
case node := <-reg.Registered:
148+
if node == nil {
149+
return nil, NewHTTPError(http.StatusUnauthorized, "node not found", nil)
150+
}
151+
return nodeToRegisterResponse(node), nil
148152
}
149153
}
154+
155+
return nil, NewHTTPError(http.StatusNotFound, "followup registration not found", nil)
150156
}
151157

152158
// canUsePreAuthKey checks if a pre auth key can be used.
@@ -271,7 +277,7 @@ func (h *Headscale) handleRegisterInteractive(
271277
Hostinfo: regReq.Hostinfo,
272278
LastSeen: ptr.To(time.Now()),
273279
},
274-
Registered: make(chan struct{}),
280+
Registered: make(chan *types.Node),
275281
}
276282

277283
if !regReq.Expiry.IsZero() {

hscontrol/db/node.go

+5
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,12 @@ func (hsdb *HSDatabase) HandleNodeFromAuthPath(
372372
}
373373

374374
// Signal to waiting clients that the machine has been registered.
375+
select {
376+
case reg.Registered <- node:
377+
default:
378+
}
375379
close(reg.Registered)
380+
376381
newNode = true
377382
return node, err
378383
} else {

hscontrol/grpcv1.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,7 @@ func (api headscaleV1APIServer) DebugCreateNode(
838838

839839
Hostinfo: &hostinfo,
840840
},
841-
Registered: make(chan struct{}),
841+
Registered: make(chan *types.Node),
842842
}
843843

844844
log.Debug().

hscontrol/types/common.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -194,5 +194,5 @@ func (r RegistrationID) String() string {
194194

195195
type RegisterNode struct {
196196
Node Node
197-
Registered chan struct{}
197+
Registered chan *Node
198198
}

hscontrol/util/util.go

+36-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,13 @@
11
package util
22

3-
import "tailscale.com/util/cmpver"
3+
import (
4+
"errors"
5+
"fmt"
6+
"net/url"
7+
"strings"
8+
9+
"tailscale.com/util/cmpver"
10+
)
411

512
func TailscaleVersionNewerOrEqual(minimum, toCheck string) bool {
613
if cmpver.Compare(minimum, toCheck) <= 0 ||
@@ -11,3 +18,31 @@ func TailscaleVersionNewerOrEqual(minimum, toCheck string) bool {
1118

1219
return false
1320
}
21+
22+
// ParseLoginURLFromCLILogin parses the output of the tailscale up command to extract the login URL.
23+
// It returns an error if not exactly one URL is found.
24+
func ParseLoginURLFromCLILogin(output string) (*url.URL, error) {
25+
lines := strings.Split(output, "\n")
26+
var urlStr string
27+
28+
for _, line := range lines {
29+
line = strings.TrimSpace(line)
30+
if strings.HasPrefix(line, "http://") || strings.HasPrefix(line, "https://") {
31+
if urlStr != "" {
32+
return nil, fmt.Errorf("multiple URLs found: %s and %s", urlStr, line)
33+
}
34+
urlStr = line
35+
}
36+
}
37+
38+
if urlStr == "" {
39+
return nil, errors.New("no URL found")
40+
}
41+
42+
loginURL, err := url.Parse(urlStr)
43+
if err != nil {
44+
return nil, fmt.Errorf("failed to parse URL: %w", err)
45+
}
46+
47+
return loginURL, nil
48+
}

hscontrol/util/util_test.go

+85
Original file line numberDiff line numberDiff line change
@@ -93,3 +93,88 @@ func TestTailscaleVersionNewerOrEqual(t *testing.T) {
9393
})
9494
}
9595
}
96+
97+
func TestParseLoginURLFromCLILogin(t *testing.T) {
98+
tests := []struct {
99+
name string
100+
output string
101+
wantURL string
102+
wantErr string
103+
}{
104+
{
105+
name: "valid https URL",
106+
output: `
107+
To authenticate, visit:
108+
109+
https://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi
110+
111+
Success.`,
112+
wantURL: "https://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi",
113+
wantErr: "",
114+
},
115+
{
116+
name: "valid http URL",
117+
output: `
118+
To authenticate, visit:
119+
120+
http://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi
121+
122+
Success.`,
123+
wantURL: "http://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi",
124+
wantErr: "",
125+
},
126+
{
127+
name: "no URL",
128+
output: `
129+
To authenticate, visit:
130+
131+
Success.`,
132+
wantURL: "",
133+
wantErr: "no URL found",
134+
},
135+
{
136+
name: "multiple URLs",
137+
output: `
138+
To authenticate, visit:
139+
140+
https://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi
141+
142+
To authenticate, visit:
143+
144+
http://headscale.example.com/register/dv1l2k5FackOYl-7-V3mSd_E
145+
146+
Success.`,
147+
wantURL: "",
148+
wantErr: "multiple URLs found: https://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi and http://headscale.example.com/register/dv1l2k5FackOYl-7-V3mSd_E",
149+
},
150+
{
151+
name: "invalid URL",
152+
output: `
153+
To authenticate, visit:
154+
155+
invalid-url
156+
157+
Success.`,
158+
wantURL: "",
159+
wantErr: "no URL found",
160+
},
161+
}
162+
163+
for _, tt := range tests {
164+
t.Run(tt.name, func(t *testing.T) {
165+
gotURL, err := ParseLoginURLFromCLILogin(tt.output)
166+
if tt.wantErr != "" {
167+
if err == nil || err.Error() != tt.wantErr {
168+
t.Errorf("ParseLoginURLFromCLILogin() error = %v, wantErr %v", err, tt.wantErr)
169+
}
170+
} else {
171+
if err != nil {
172+
t.Errorf("ParseLoginURLFromCLILogin() error = %v, wantErr %v", err, tt.wantErr)
173+
}
174+
if gotURL.String() != tt.wantURL {
175+
t.Errorf("ParseLoginURLFromCLILogin() = %v, want %v", gotURL, tt.wantURL)
176+
}
177+
}
178+
})
179+
}
180+
}

integration/tsic/tsic.go

+1-9
Original file line numberDiff line numberDiff line change
@@ -503,15 +503,7 @@ func (t *TailscaleInContainer) LoginWithURL(
503503
}
504504
}()
505505

506-
urlStr := strings.ReplaceAll(stdout+stderr, "\nTo authenticate, visit:\n\n\t", "")
507-
urlStr = strings.TrimSpace(urlStr)
508-
509-
if urlStr == "" {
510-
return nil, fmt.Errorf("failed to get login URL: stdout: %s, stderr: %s", stdout, stderr)
511-
}
512-
513-
// parse URL
514-
loginURL, err = url.Parse(urlStr)
506+
loginURL, err = util.ParseLoginURLFromCLILogin(stdout + stderr)
515507
if err != nil {
516508
return nil, err
517509
}

0 commit comments

Comments
 (0)