Fix regression in writing authorized principals (#36213) (#36218)

Backport #36213 by peterverraedt

Fixes: #36212

Signed-off-by: Peter Verraedt <peter.verraedt@kuleuven.be>
Co-authored-by: Peter Verraedt <peter.verraedt@kuleuven.be>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

Author: 3 people

models/asymkey/ssh_key_authorized_keys.go

@@ -10,6 +10,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"sync"
 
@@ -50,12 +51,42 @@ func WriteAuthorizedStringForValidKey(key *PublicKey, w io.Writer) error {
 	return err
 }
 
+var globalVars = sync.OnceValue(func() (ret struct {
+	principalRegexp *regexp.Regexp
+},
+) {
+	// principalRegexp expresses whether a principal is considered valid.
+	// This reverse engineers how sshd parses the authorized keys file,
+	// see e.g. https://github.com/openssh/openssh-portable/blob/32deb00b38b4ee2b3302f261ea1e68c04e020a08/auth2-pubkeyfile.c#L221-L256
+	// Any newline or # comment will be stripped when parsing, so don't allow
+	// those. Also, if any space or tab is present in the principal, the part
+	// proceeding this would be parsed as an option, so just avoid any whitespace
+	// altogether.
+	ret.principalRegexp = regexp.MustCompile(`^[^\s#]+$`)
+	return ret
+})
+
 func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, err error) {
-	const tpl = AuthorizedStringCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict %s %s` + "\n"
-	pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key.Content))
-	if err != nil {
-		return false, err
+	const tpl = AuthorizedStringCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict %s` + "\n"
+
+	var sshKey string
+
+	if key.Type == KeyTypePrincipal {
+		// TODO: actually using PublicKey to store "principal" is an abuse
+		if !globalVars().principalRegexp.MatchString(key.Content) {
+			return false, fmt.Errorf("invalid principal key: %s", key.Content)
+		}
+		sshKey = fmt.Sprintf("%s # user-%d", key.Content, key.OwnerID)
+	} else {
+		pubKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key.Content))
+		if err != nil {
+			return false, err
+		}
+
+		sshKeyMarshalled := strings.TrimSpace(string(ssh.MarshalAuthorizedKey(pubKey)))
+		sshKey = fmt.Sprintf("%s user-%d", sshKeyMarshalled, key.OwnerID)
 	}
+
 	// now the key is valid, the code below could only return template/IO related errors
 	sbCmd := &strings.Builder{}
 	err = setting.SSH.AuthorizedKeysCommandTemplateTemplate.Execute(sbCmd, map[string]any{
@@ -69,9 +100,7 @@ func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, er
 		return true, err
 	}
 	sshCommandEscaped := util.ShellEscape(sbCmd.String())
-	sshKeyMarshalled := strings.TrimSpace(string(ssh.MarshalAuthorizedKey(pubKey)))
-	sshKeyComment := fmt.Sprintf("user-%d", key.OwnerID)
-	_, err = fmt.Fprintf(w, tpl, sshCommandEscaped, sshKeyMarshalled, sshKeyComment)
+	_, err = fmt.Fprintf(w, tpl, sshCommandEscaped, sshKey)
 	return true, err
 }
 

models/asymkey/ssh_key_authorized_keys_test.go

@@ -0,0 +1,90 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package asymkey
+
+import (
+	"strings"
+	"testing"
+
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestWriteAuthorizedStringForKey(t *testing.T) {
+	defer test.MockVariableValue(&setting.AppPath, "/tmp/gitea")()
+	defer test.MockVariableValue(&setting.CustomConf, "/tmp/app.ini")()
+	writeKey := func(t *testing.T, key *PublicKey) (bool, string, error) {
+		sb := &strings.Builder{}
+		valid, err := writeAuthorizedStringForKey(key, sb)
+		return valid, sb.String(), err
+	}
+	const validKeyContent = `ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAICV0MGX/W9IvLA4FXpIuUcdDcbj5KX4syHgsTy7soVgf`
+
+	testValid := func(t *testing.T, key *PublicKey, expected string) {
+		valid, content, err := writeKey(t, key)
+		assert.True(t, valid)
+		assert.Equal(t, expected, content)
+		assert.NoError(t, err)
+	}
+
+	testInvalid := func(t *testing.T, key *PublicKey) {
+		valid, content, err := writeKey(t, key)
+		assert.False(t, valid)
+		assert.Empty(t, content)
+		assert.Error(t, err)
+	}
+
+	t.Run("PublicKey", func(t *testing.T) {
+		testValid(t, &PublicKey{
+			OwnerID: 123,
+			Content: validKeyContent + " any-comment",
+			Type:    KeyTypeUser,
+		}, `# gitea public key
+command="/tmp/gitea --config=/tmp/app.ini serv key-0",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAICV0MGX/W9IvLA4FXpIuUcdDcbj5KX4syHgsTy7soVgf user-123
+`)
+	})
+
+	t.Run("PublicKeyWithNewLine", func(t *testing.T) {
+		testValid(t, &PublicKey{
+			OwnerID: 123,
+			Content: validKeyContent + "\nany-more", // the new line should be ignored
+			Type:    KeyTypeUser,
+		}, `# gitea public key
+command="/tmp/gitea --config=/tmp/app.ini serv key-0",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAICV0MGX/W9IvLA4FXpIuUcdDcbj5KX4syHgsTy7soVgf user-123
+`)
+	})
+
+	t.Run("PublicKeyInvalid", func(t *testing.T) {
+		testInvalid(t, &PublicKey{
+			OwnerID: 123,
+			Content: validKeyContent + "any-more",
+			Type:    KeyTypeUser,
+		})
+	})
+
+	t.Run("Principal", func(t *testing.T) {
+		testValid(t, &PublicKey{
+			OwnerID: 123,
+			Content: "any-content",
+			Type:    KeyTypePrincipal,
+		}, `# gitea public key
+command="/tmp/gitea --config=/tmp/app.ini serv key-0",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict any-content # user-123
+`)
+	})
+
+	t.Run("PrincipalInvalid", func(t *testing.T) {
+		testInvalid(t, &PublicKey{
+			OwnerID: 123,
+			Content: "a b",
+			Type:    KeyTypePrincipal,
+		})
+		testInvalid(t, &PublicKey{
+			OwnerID: 123,
+			Content: "a\nb",
+			Type:    KeyTypePrincipal,
+		})
+	})
+}