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]>
juanfont · Feb 25, 2025 · 1686819 · 1686819
1 parent da2ca05
commit 1686819
Show file tree

Hide file tree

Showing 8 changed files with 151 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,14 +14,16 @@
   - View of config, policy, filter, ssh policy per node, connected nodes and
     DERPmap
 
-## 0.25.1 (2025-02-24)
+## 0.25.1 (2025-02-25)
 
 ### Changes
 
 - Fix issue where registration errors are sent correctly
   [#2435](https://github.com/juanfont/headscale/pull/2435)
 - Fix issue where routes passed on registration were not saved
   [#2444](https://github.com/juanfont/headscale/pull/2444)
+- Fix issue where registration page was displayed twice
+  [#2445](https://github.com/juanfont/headscale/pull/2445)
 
 ## 0.25.0 (2025-02-11)
 

diff --git a/hscontrol/auth.go b/hscontrol/auth.go
@@ -43,10 +43,7 @@ func (h *Headscale) handleRegister(
 	}
 
 	if regReq.Followup != "" {
-		// TODO(kradalby): Does this need to return an error of some sort?
-		// Maybe if the registration fails down the line it can be sent
-		// on the channel and returned here?
-		h.waitForFollowup(ctx, regReq)
+		return h.waitForFollowup(ctx, regReq)
 	}
 
 	if regReq.Auth != nil && regReq.Auth.AuthKey != "" {
@@ -111,42 +108,51 @@ func (h *Headscale) handleExistingNode(
 		h.nodeNotifier.NotifyWithIgnore(ctx, types.UpdateExpire(node.ID, requestExpiry), node.ID)
 	}
 
+	return nodeToRegisterResponse(node), nil
+}
+
+func nodeToRegisterResponse(node *types.Node) *tailcfg.RegisterResponse {
 	return &tailcfg.RegisterResponse{
 		// TODO(kradalby): Only send for user-owned nodes
 		// and not tagged nodes when tags is working.
 		User:           *node.User.TailscaleUser(),
 		Login:          *node.User.TailscaleLogin(),
-		NodeKeyExpired: expired,
+		NodeKeyExpired: node.IsExpired(),
 
 		// Headscale does not implement the concept of machine authorization
 		// so we always return true here.
 		// Revisit this if #2176 gets implemented.
 		MachineAuthorized: true,
-	}, nil
+	}
 }
 
 func (h *Headscale) waitForFollowup(
 	ctx context.Context,
 	regReq tailcfg.RegisterRequest,
-) {
+) (*tailcfg.RegisterResponse, error) {
 	fu, err := url.Parse(regReq.Followup)
 	if err != nil {
-		return
+		return nil, NewHTTPError(http.StatusUnauthorized, "invalid followup URL", err)
 	}
 
 	followupReg, err := types.RegistrationIDFromString(strings.ReplaceAll(fu.Path, "/register/", ""))
 	if err != nil {
-		return
+		return nil, NewHTTPError(http.StatusUnauthorized, "invalid registration ID", err)
 	}
 
 	if reg, ok := h.registrationCache.Get(followupReg); ok {
 		select {
 		case <-ctx.Done():
-			return
-		case <-reg.Registered:
-			return
+			return nil, NewHTTPError(http.StatusUnauthorized, "registration timed out", err)
+		case node := <-reg.Registered:
+			if node == nil {
+				return nil, NewHTTPError(http.StatusUnauthorized, "node not found", nil)
+			}
+			return nodeToRegisterResponse(node), nil
 		}
 	}
+
+	return nil, NewHTTPError(http.StatusNotFound, "followup registration not found", nil)
 }
 
 // canUsePreAuthKey checks if a pre auth key can be used.
@@ -271,7 +277,7 @@ func (h *Headscale) handleRegisterInteractive(
 			Hostinfo:   regReq.Hostinfo,
 			LastSeen:   ptr.To(time.Now()),
 		},
-		Registered: make(chan struct{}),
+		Registered: make(chan *types.Node),
 	}
 
 	if !regReq.Expiry.IsZero() {

diff --git a/hscontrol/db/node.go b/hscontrol/db/node.go
@@ -372,7 +372,12 @@ func (hsdb *HSDatabase) HandleNodeFromAuthPath(
 				}
 
 				// Signal to waiting clients that the machine has been registered.
+				select {
+				case reg.Registered <- node:
+				default:
+				}
 				close(reg.Registered)
+
 				newNode = true
 				return node, err
 			} else {

diff --git a/hscontrol/grpcv1.go b/hscontrol/grpcv1.go
@@ -838,7 +838,7 @@ func (api headscaleV1APIServer) DebugCreateNode(
 
 			Hostinfo: &hostinfo,
 		},
-		Registered: make(chan struct{}),
+		Registered: make(chan *types.Node),
 	}
 
 	log.Debug().

diff --git a/hscontrol/types/common.go b/hscontrol/types/common.go
@@ -194,5 +194,5 @@ func (r RegistrationID) String() string {
 
 type RegisterNode struct {
 	Node       Node
-	Registered chan struct{}
+	Registered chan *Node
 }
diff --git a/hscontrol/util/util.go b/hscontrol/util/util.go
@@ -1,6 +1,13 @@
 package util
 
-import "tailscale.com/util/cmpver"
+import (
+	"errors"
+	"fmt"
+	"net/url"
+	"strings"
+
+	"tailscale.com/util/cmpver"
+)
 
 func TailscaleVersionNewerOrEqual(minimum, toCheck string) bool {
 	if cmpver.Compare(minimum, toCheck) <= 0 ||
@@ -11,3 +18,31 @@ func TailscaleVersionNewerOrEqual(minimum, toCheck string) bool {
 
 	return false
 }
+
+// ParseLoginURLFromCLILogin parses the output of the tailscale up command to extract the login URL.
+// It returns an error if not exactly one URL is found.
+func ParseLoginURLFromCLILogin(output string) (*url.URL, error) {
+	lines := strings.Split(output, "\n")
+	var urlStr string
+
+	for _, line := range lines {
+		line = strings.TrimSpace(line)
+		if strings.HasPrefix(line, "http://") || strings.HasPrefix(line, "https://") {
+			if urlStr != "" {
+				return nil, fmt.Errorf("multiple URLs found: %s and %s", urlStr, line)
+			}
+			urlStr = line
+		}
+	}
+
+	if urlStr == "" {
+		return nil, errors.New("no URL found")
+	}
+
+	loginURL, err := url.Parse(urlStr)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse URL: %w", err)
+	}
+
+	return loginURL, nil
+}
diff --git a/hscontrol/util/util_test.go b/hscontrol/util/util_test.go
@@ -93,3 +93,88 @@ func TestTailscaleVersionNewerOrEqual(t *testing.T) {
 		})
 	}
 }
+
+func TestParseLoginURLFromCLILogin(t *testing.T) {
+	tests := []struct {
+		name    string
+		output  string
+		wantURL string
+		wantErr string
+	}{
+		{
+			name: "valid https URL",
+			output: `
+To authenticate, visit:
+
+        https://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi
+
+Success.`,
+			wantURL: "https://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi",
+			wantErr: "",
+		},
+		{
+			name: "valid http URL",
+			output: `
+To authenticate, visit:
+
+        http://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi
+
+Success.`,
+			wantURL: "http://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi",
+			wantErr: "",
+		},
+		{
+			name: "no URL",
+			output: `
+To authenticate, visit:
+
+Success.`,
+			wantURL: "",
+			wantErr: "no URL found",
+		},
+		{
+			name: "multiple URLs",
+			output: `
+To authenticate, visit:
+
+        https://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi
+
+To authenticate, visit:
+
+        http://headscale.example.com/register/dv1l2k5FackOYl-7-V3mSd_E
+
+Success.`,
+			wantURL: "",
+			wantErr: "multiple URLs found: https://headscale.example.com/register/3oYCOZYA2zZmGB4PQ7aHBaMi and http://headscale.example.com/register/dv1l2k5FackOYl-7-V3mSd_E",
+		},
+		{
+			name: "invalid URL",
+			output: `
+To authenticate, visit:
+
+        invalid-url
+
+Success.`,
+			wantURL: "",
+			wantErr: "no URL found",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotURL, err := ParseLoginURLFromCLILogin(tt.output)
+			if tt.wantErr != "" {
+				if err == nil || err.Error() != tt.wantErr {
+					t.Errorf("ParseLoginURLFromCLILogin() error = %v, wantErr %v", err, tt.wantErr)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("ParseLoginURLFromCLILogin() error = %v, wantErr %v", err, tt.wantErr)
+				}
+				if gotURL.String() != tt.wantURL {
+					t.Errorf("ParseLoginURLFromCLILogin() = %v, want %v", gotURL, tt.wantURL)
+				}
+			}
+		})
+	}
+}
diff --git a/integration/tsic/tsic.go b/integration/tsic/tsic.go
@@ -503,15 +503,7 @@ func (t *TailscaleInContainer) LoginWithURL(
 		}
 	}()
 
-	urlStr := strings.ReplaceAll(stdout+stderr, "\nTo authenticate, visit:\n\n\t", "")
-	urlStr = strings.TrimSpace(urlStr)
-
-	if urlStr == "" {
-		return nil, fmt.Errorf("failed to get login URL: stdout: %s, stderr: %s", stdout, stderr)
-	}
-
-	// parse URL
-	loginURL, err = url.Parse(urlStr)
+	loginURL, err = util.ParseLoginURLFromCLILogin(stdout + stderr)
 	if err != nil {
 		return nil, err
 	}